soft delete relationships when aspects are soft deleted (ASPECT_METADATA only) #456
Conversation
| @@ -611,10 +611,11 @@ protected <ASPECT extends RecordTemplate> long saveLatest(@Nonnull URN urn, @Non | |||
| aspectClass.getCanonicalName())); | |||
| } | |||
|
|
|||
| if (_relationshipSource == RelationshipSource.ASPECT_METADATA) { | |||
| // TODO: not yet implemented -> add support for removing relationships when the aspect is to be soft-deleted | |||
| throw new UnsupportedOperationException("This method has not been implemented yet to support the " | |||
There was a problem hiding this comment.
just curious, did we run into this exception when someone's trying to soft delete the relationship?
There was a problem hiding this comment.
No, this is under the if (_relationshipSource == RelationshipSource.ASPECT_METADATA) branch, which no one is using right now.
There was a problem hiding this comment.
Although, there is a similar exception for relationship builders which probably we ran into a few times
if (_relationshipSource == RelationshipSource.RELATIONSHIP_BUILDERS
&& _localRelationshipBuilderRegistry != null
&& _localRelationshipBuilderRegistry.isRegistered(aspectClass)) {
throw new UnsupportedOperationException(
String.format("Aspect %s cannot be soft-deleted because it has a local relationship builder registered.",
aspectClass.getCanonicalName()));
}
| for (RELATIONSHIP relationship : relationships) { | ||
| _server.createSqlUpdate(SQLStatementUtils.deleteLocalRelationshipSQL(SQLSchemaUtils.getRelationshipTableName(relationship), | ||
| RemovalOption.REMOVE_ALL_EDGES_FROM_SOURCE_TO_DESTINATION)) | ||
| .setParameter(CommonColumnName.SOURCE, getSourceUrnFromRelationship(relationship).toString()) |
There was a problem hiding this comment.
does the new relationship model have (require) source urn?
There was a problem hiding this comment.
or this implementation is only for legacy relationship models
There was a problem hiding this comment.
Yes, we're going to keep the source urn in each 2.0 relationship model. You suggested:
message Relationship {
string source = 1;
oneof relatedTo {
proto.com.linkedin.metadata.relationship.DownstreamOf = 2;
proto.com.linkedin.metadata.relationship.OwnedBy = 3;
}
}
and
message OwnedBy {
oneof destination {
proto.com.linkedin.common.CorpuserUrn = 1;
proto.com.linkedin.devx.CrewUrn = 2;
}
Owner owner = 3;
}
But we think that from the it should be better from the modeling perspective to see both the source and destination in the relationship model like:
message OwnedBy {
oneof source {
proto.com.linkedin.common.DatasetUrn = 1;
}
oneof destination {
proto.com.linkedin.common.CorpuserUrn = 2;
proto.com.linkedin.devx.CrewUrn = 3;
}
Owner owner = 4;
}
and then it can be omitted from the top-level Relationship model.
message Relationship {
oneof relatedTo {
proto.com.linkedin.metadata.relationship.DownstreamOf = 1;
proto.com.linkedin.metadata.relationship.OwnedBy = 2;
}
}
There was a problem hiding this comment.
Let me know your thoughts or if you have a strong opinion on this
There was a problem hiding this comment.
The problem for having the source urn on the OwnedBy is it is a duplicated information with the host asset and you may lose the model constrains
what if
DatasetAsset {
urn: DatasetUrn ,
ownership OwnedBy {
source: CropUserUrn, ---> this is supposed to be DatasetUrn.
destination: DatasetUrn
}
}
| for (RELATIONSHIP relationship : relationships) { | ||
| _server.createSqlUpdate(SQLStatementUtils.deleteLocalRelationshipSQL(SQLSchemaUtils.getRelationshipTableName(relationship), | ||
| RemovalOption.REMOVE_ALL_EDGES_FROM_SOURCE_TO_DESTINATION)) | ||
| .setParameter(CommonColumnName.SOURCE, getSourceUrnFromRelationship(relationship).toString()) |
There was a problem hiding this comment.
Note: the getSourceUrnFromRelationship method will need to be updated for Model 2.0 (I'll include in another PR I'm working on), because in the model the source and destination are union fields now, and the urn is from the field inside the union. The testRelationship in pdl looks like this:
record TestRelationship {
/**
* The audit stamp of the relationship
*/
auditStamp: optional AuditStamp
/**
* Urn of the source asset
*/
source: optional union[
sourceDemoAsset: DemoAssetUrn
]
/**
* Urn of the destination asset
*/
destination: optional union[
destinationTestContext: TestContextUrn
]
There was a problem hiding this comment.
Thanks for working on this. Please remember that we will probably need to handle both 1.0 and 2.0 models simultaneously during the migration phase.
| // TODO: not yet implemented -> add support for removing relationships when the aspect is to be soft-deleted | ||
| throw new UnsupportedOperationException("This method has not been implemented yet to support the " | ||
| + "ASPECT_METADATA RelationshipSource type yet."); | ||
| if (_relationshipSource == RelationshipSource.ASPECT_METADATA && oldValue != null) { |
There was a problem hiding this comment.
The order of the logics in this method is:
- remove old relationships
- insert/update entity tables
- add new relationships.
Can it first handle entity tables and then relationship tables? New order would be: 2 -> 1 -> 3
There was a problem hiding this comment.
Thanks for the suggestion. Reordered to be 2 -> 1/3 (either or)
Summary
Previously, there was a gap when using relationship builders where an aspect that was registered with a relationship builder could not be soft deleted since there was no agreed-upon way to remove relationships associated with that aspect.
Now, when deriving relationships directly from aspect metadata, we can handle soft deletion gracefully. When the source aspect is marked as soft deleted, also mark any of the relationships from the old aspect value (existing in the db) as deleted in the corresponding relationship tables.
Note on the large number of files changed in this PR. I had to add a new test aspect model which required making many minor changes to other unit tests. No need to closely review those minor changes.
Testing Done
added a unit test
Checklist