From 36ea12189dae5aa58311d0e9234fd88eeac1ee62 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Wed, 30 Apr 2025 00:30:21 -0700 Subject: [PATCH 1/5] initial commit, trying to fix failing tests --- .../linkedin/metadata/dao/EbeanLocalDAO.java | 29 +++++++++++++++---- .../metadata/dao/utils/EBeanDAOUtils.java | 27 +++++++++++------ .../metadata/dao/EbeanLocalDAOTest.java | 4 +-- .../testing/SoftDeletedAspectCopy.pdl | 7 +++++ .../metadata/aspect/AuditedAspect.pdl | 0 .../metadata/aspect/SoftDeletedAspect.pdl | 5 ++++ 6 files changed, 55 insertions(+), 17 deletions(-) rename {dao-api => validators}/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl (100%) diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java index 7e0b88eb0..7ebd78780 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java @@ -8,6 +8,7 @@ import com.linkedin.data.template.RecordTemplate; import com.linkedin.data.template.SetMode; import com.linkedin.data.template.UnionTemplate; +import com.linkedin.metadata.aspect.SoftDeletedAspect; import com.linkedin.metadata.dao.builder.BaseLocalRelationshipBuilder.LocalRelationshipUpdates; import com.linkedin.metadata.dao.builder.LocalRelationshipBuilderRegistry; import com.linkedin.metadata.dao.exception.ModelConversionException; @@ -791,7 +792,7 @@ protected AspectEntry getLatest(@Nonnull } final ExtraInfo extraInfo = toExtraInfo(latest); - if (isSoftDeletedAspect(latest, aspectClass)) { + if (isSoftDeletedAspect(latest)) { return new AspectEntry<>(null, extraInfo, true); } @@ -1077,6 +1078,12 @@ protected void applyTimeBasedRetention(@Nonnull @Nonnull public Map, Optional> get( @Nonnull Set> keys) { + return get(keys, false); + } + + @Nonnull + public Map, Optional> get( + @Nonnull Set> keys, boolean includeSoftDeleted) { if (keys.isEmpty()) { return Collections.emptyMap(); } @@ -1095,7 +1102,8 @@ protected void applyTimeBasedRetention(@Nonnull .collect(Collectors.toMap(Function.identity(), key -> records.stream() .filter(record -> matchKeys(key, record.getKey())) .findFirst() - .flatMap(record -> toRecordTemplate(key.getAspectClass(), record)))); + .flatMap(record -> includeSoftDeleted ? toRecordTemplateIncludeSoftDelete(key.getAspectClass(), record) + : (Optional) toRecordTemplate(key.getAspectClass(), record)))); } @Override @@ -1259,14 +1267,14 @@ List batchGetHelper(@Nonnull List resultsOldSchema = batchGetUnion(keys, keysCount, position); final List resultsNewSchema = - _localAccess.batchGetUnion(keys, keysCount, position, false, false); + _localAccess.batchGetUnion(keys, keysCount, position, false, false); // TODO: should soft-deleted aspects be included? EBeanDAOUtils.compareResults(resultsOldSchema, resultsNewSchema, "batchGet"); return resultsOldSchema; } @@ -1466,16 +1474,25 @@ URN getUrn(@Nonnull String urn) { @Nonnull static Optional toRecordTemplate(@Nonnull Class aspectClass, @Nonnull EbeanMetadataAspect aspect) { - if (isSoftDeletedAspect(aspect, aspectClass)) { + if (isSoftDeletedAspect(aspect)) { return Optional.empty(); } return Optional.of(RecordUtils.toRecordTemplate(aspectClass, aspect.getMetadata())); } + @Nonnull + static Optional toRecordTemplateIncludeSoftDelete( + @Nonnull Class aspectClass, @Nonnull EbeanMetadataAspect aspect) { + if (isSoftDeletedAspect(aspect)) { + return Optional.of(RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata())); + } + return Optional.of(RecordUtils.toRecordTemplate(aspectClass, aspect.getMetadata())); + } + @Nonnull static Optional> toRecordTemplateWithExtraInfo( @Nonnull Class aspectClass, @Nonnull EbeanMetadataAspect aspect) { - if (aspect.getMetadata() == null || isSoftDeletedAspect(aspect, aspectClass)) { + if (aspect.getMetadata() == null || isSoftDeletedAspect(aspect)) { return Optional.empty(); } final ExtraInfo extraInfo = toExtraInfo(aspect); diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java index 6ae211bb9..92c7bd4c2 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java @@ -179,19 +179,24 @@ public static boolean compareResults(@Nullable ListResult resultOld, @Nul * Checks whether the aspect record has been soft deleted. * * @param aspect aspect value - * @param aspectClass the type of the aspect * @return boolean representing whether the aspect record has been soft deleted */ - public static boolean isSoftDeletedAspect(@Nonnull EbeanMetadataAspect aspect, - @Nonnull Class aspectClass) { + public static boolean isSoftDeletedAspect(@Nonnull EbeanMetadataAspect aspect) { // Convert metadata string to record template object - final RecordTemplate metadataRecord = RecordUtils.toRecordTemplate(aspectClass, aspect.getMetadata()); - return metadataRecord.equals(DELETED_METADATA); + try { + SoftDeletedAspect attemptToCastToSDA = RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata()); + return attemptToCastToSDA.hasGma_deleted() && attemptToCastToSDA.isGma_deleted(); + } catch (Exception e) { + return false; + } } /** * Read {@link SqlRow} list into a {@link EbeanMetadataAspect} list. + * Currently only in use by AIM. + * Note this cannot read deleted metadata. + * * @param sqlRows list of {@link SqlRow} * @return list of {@link EbeanMetadataAspect} * @deprecated This method has been deprecated, use {@link #readSqlRow(SqlRow, Class)} instead. @@ -239,6 +244,10 @@ public static List readSqlR /** * Read EbeanMetadataAspect from {@link SqlRow}. + * Note that if the Aspect is (soft) deleted and the DB is on the NEW SCHEMA, the creation metadata will be + * associated with the *whole row*, not the individual aspect. There is currently no metadata stored for the deletion + * of aspects. + * * @param sqlRow {@link SqlRow} * @param aspectClass aspect class * @param aspect type @@ -258,10 +267,10 @@ private static EbeanMetadataAspect readSqlRow(Sq } if (isSoftDeletedAspect(sqlRow, columnName)) { primaryKey = new EbeanMetadataAspect.PrimaryKey(urn, aspectClass.getCanonicalName(), LATEST_VERSION); - ebeanMetadataAspect.setCreatedBy(sqlRow.getString("lastmodifiedby")); - ebeanMetadataAspect.setCreatedOn(sqlRow.getTimestamp("lastmodifiedon")); - ebeanMetadataAspect.setCreatedFor(sqlRow.getString("createdfor")); - ebeanMetadataAspect.setMetadata(DELETED_VALUE); + ebeanMetadataAspect.setCreatedBy(sqlRow.getString("lastmodifiedby")); // TODO: add support for deletion metadata + ebeanMetadataAspect.setCreatedOn(sqlRow.getTimestamp("lastmodifiedon")); // TODO: add support for deletion metadata + ebeanMetadataAspect.setCreatedFor(sqlRow.getString("createdfor")); // TODO: add support for deletion metadata + ebeanMetadataAspect.setMetadata(extractAspectJsonString(sqlRow.getString(columnName))); } else { AuditedAspect auditedAspect = RecordUtils.toRecordTemplate(AuditedAspect.class, sqlRow.getString(columnName)); primaryKey = new EbeanMetadataAspect.PrimaryKey(urn, auditedAspect.getCanonicalName(), LATEST_VERSION); diff --git a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java index 65038189a..146fcae85 100644 --- a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java +++ b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/EbeanLocalDAOTest.java @@ -2337,7 +2337,7 @@ public void testGetSoftDeletedAspect() { // latest version of metadata should be null EbeanMetadataAspect aspect = getMetadata(urn, aspectName, 0); - assertTrue(isSoftDeletedAspect(aspect, AspectFoo.class)); + assertTrue(isSoftDeletedAspect(aspect)); Optional fooOptional = dao.get(AspectFoo.class, urn); assertFalse(fooOptional.isPresent()); @@ -2523,7 +2523,7 @@ public void testUndeleteSoftDeletedAspect() { if (dao.isChangeLogEnabled()) { // version=3 should correspond to soft deleted metadata EbeanMetadataAspect aspect = getMetadata(urn, aspectName, 3); - assertTrue(isSoftDeletedAspect(aspect, AspectFoo.class)); + assertTrue(isSoftDeletedAspect(aspect)); fooOptional = dao.get(AspectFoo.class, urn, 3); assertFalse(fooOptional.isPresent()); diff --git a/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl b/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl index c392f06a1..e837bd2bc 100644 --- a/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl +++ b/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl @@ -1,5 +1,7 @@ namespace com.linkedin.testing +import com.linkedin.metadata.aspect.AuditedAspect + /** * Captures metadata on soft deleted aspect. This is a copy of SoftDeletedAspect. */ @@ -9,4 +11,9 @@ record SoftDeletedAspectCopy { * flag to indicate if the aspect is soft deleted in GMA */ gma_deleted: boolean + + /** + * The deleted content of the aspect + */ + gma_deleted_content: AuditedAspect } \ No newline at end of file diff --git a/dao-api/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl b/validators/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl similarity index 100% rename from dao-api/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl rename to validators/src/main/pegasus/com/linkedin/metadata/aspect/AuditedAspect.pdl diff --git a/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl b/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl index 99634f194..b56fea9d9 100644 --- a/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl +++ b/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl @@ -9,4 +9,9 @@ record SoftDeletedAspect { * Flag to indicate if the aspect is soft deleted in GMA */ gma_deleted: boolean + + /** + * The deleted content of the aspect + */ + gma_deleted_content: AuditedAspect } \ No newline at end of file From 255982aa5e575882275d83232d9e68db6b0cc1ca Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Wed, 30 Apr 2025 01:48:57 -0700 Subject: [PATCH 2/5] fixed --- .../main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java | 4 ++-- .../java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java index 7ebd78780..416e89d13 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java @@ -1102,7 +1102,7 @@ protected void applyTimeBasedRetention(@Nonnull .collect(Collectors.toMap(Function.identity(), key -> records.stream() .filter(record -> matchKeys(key, record.getKey())) .findFirst() - .flatMap(record -> includeSoftDeleted ? toRecordTemplateIncludeSoftDelete(key.getAspectClass(), record) + .flatMap(record -> includeSoftDeleted ? (Optional) toRecordTemplateIncludeSoftDelete(key.getAspectClass(), record) : (Optional) toRecordTemplate(key.getAspectClass(), record)))); } @@ -1481,7 +1481,7 @@ static Optional toRecordTemplate(@Nonnul } @Nonnull - static Optional toRecordTemplateIncludeSoftDelete( + static Optional toRecordTemplateIncludeSoftDelete( @Nonnull Class aspectClass, @Nonnull EbeanMetadataAspect aspect) { if (isSoftDeletedAspect(aspect)) { return Optional.of(RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata())); diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java index 92c7bd4c2..693d66e4c 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java @@ -270,7 +270,7 @@ private static EbeanMetadataAspect readSqlRow(Sq ebeanMetadataAspect.setCreatedBy(sqlRow.getString("lastmodifiedby")); // TODO: add support for deletion metadata ebeanMetadataAspect.setCreatedOn(sqlRow.getTimestamp("lastmodifiedon")); // TODO: add support for deletion metadata ebeanMetadataAspect.setCreatedFor(sqlRow.getString("createdfor")); // TODO: add support for deletion metadata - ebeanMetadataAspect.setMetadata(extractAspectJsonString(sqlRow.getString(columnName))); + ebeanMetadataAspect.setMetadata(sqlRow.getString(columnName)); } else { AuditedAspect auditedAspect = RecordUtils.toRecordTemplate(AuditedAspect.class, sqlRow.getString(columnName)); primaryKey = new EbeanMetadataAspect.PrimaryKey(urn, auditedAspect.getCanonicalName(), LATEST_VERSION); From 2ff2c8d7f7442dc5b83ca6f4c073892cdef9a286 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Wed, 30 Apr 2025 22:45:23 -0700 Subject: [PATCH 3/5] backfill complete, but not new tests added --- .../linkedin/metadata/dao/BaseLocalDAO.java | 54 +++++++++++++++++-- .../linkedin/metadata/dao/BaseReadDAO.java | 16 +++--- .../metadata/dao/utils/RecordUtils.java | 12 +++++ .../metadata/dao/BaseLocalDAOTest.java | 7 +++ .../linkedin/metadata/dao/EbeanLocalDAO.java | 1 + 5 files changed, 79 insertions(+), 11 deletions(-) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java index 6c876ca7c..1dab07797 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java @@ -17,6 +17,7 @@ import com.linkedin.metadata.annotations.Mode; import com.linkedin.metadata.annotations.UrnFilter; import com.linkedin.metadata.annotations.UrnFilterArray; +import com.linkedin.metadata.aspect.SoftDeletedAspect; import com.linkedin.metadata.backfill.BackfillMode; import com.linkedin.metadata.dao.builder.BaseLocalRelationshipBuilder.LocalRelationshipUpdates; import com.linkedin.metadata.dao.equality.DefaultEqualityTester; @@ -38,6 +39,7 @@ import com.linkedin.metadata.dao.tracking.TrackingUtils; import com.linkedin.metadata.dao.urnpath.UrnPathExtractor; import com.linkedin.metadata.dao.utils.ModelUtils; +import com.linkedin.metadata.dao.utils.RecordUtils; import com.linkedin.metadata.events.ChangeType; import com.linkedin.metadata.events.IngestionMode; import com.linkedin.metadata.events.IngestionTrackingContext; @@ -1668,7 +1670,7 @@ public Map, Optional, Optional>> urnToAspects = - get(aspectToBackfill, urns); + get(aspectToBackfill, urns, true); urnToAspects.forEach((urn, aspects) -> { aspects.forEach((aspectClass, aspect) -> aspect.ifPresent(value -> backfill(mode, value, urn))); }); @@ -1741,6 +1743,8 @@ public Map, Optional void backfill(@Nonnull BackfillMode mode, @Nonnull ASPECT aspect, @Nonnull URN urn) { + RecordTemplate oldValue = probeSoftDeletedValue(aspect); + if (mode == BackfillMode.MAE_ONLY || mode == BackfillMode.BACKFILL_ALL || mode == BackfillMode.BACKFILL_INCLUDING_LIVE_INDEX) { @@ -1749,9 +1753,9 @@ private void backfill(@Nonnull BackfillMode mode IngestionTrackingContext trackingContext = buildIngestionTrackingContext( TrackingUtils.getRandomUUID(), BACKFILL_EMITTER, System.currentTimeMillis()); - _trackingProducer.produceAspectSpecificMetadataAuditEvent(urn, aspect, aspect, aspect.getClass(), null, trackingContext, ingestionMode); + _trackingProducer.produceAspectSpecificMetadataAuditEvent(urn, oldValue, aspect, aspect.getClass(), null, trackingContext, ingestionMode); } else { - _producer.produceAspectSpecificMetadataAuditEvent(urn, aspect, aspect, aspect.getClass(), null, ingestionMode); + _producer.produceAspectSpecificMetadataAuditEvent(urn, oldValue, aspect, aspect.getClass(), null, ingestionMode); } } } @@ -1878,6 +1882,38 @@ public Optional> get return getWithExtraInfo(aspectClass, urn, LATEST_VERSION); } + /** + * Similar to {@link com.linkedin.metadata.dao.BaseReadDAO}'s {@link #get(Set, Set)} but includes a flag to be able to + * retrieve SoftDeletedAspect entries as well. + * + *

This is currently used to parse soft-deleted metadata in order to backfill the search index for deleted items. + * + *

This is not advised for general use cases since deleted metadata will be returned. + * + * @param aspectClasses the set of aspect classes to retrieve + * @param urns the set of URNs to retrieve + * @param includeSoftDeleted whether to include soft deleted aspects + * @return a map of URN to a map of aspect class to the corresponding aspect value + */ + @Nonnull + public Map, Optional>> get( + @Nonnull Set> aspectClasses, @Nonnull Set urns, boolean includeSoftDeleted) { + final Set> keys = getKeysForLatestVersion(urns, aspectClasses); + + final Map, Optional>> results = new HashMap<>(); + get(keys, includeSoftDeleted).forEach((key, value) -> { + final URN urn = key.getUrn(); + results.putIfAbsent(urn, new HashMap<>()); + results.get(urn).put(key.getAspectClass(), value); + }); + + return results; + } + + @Nonnull + public abstract Map, Optional> get( + @Nonnull Set> keys, boolean includeSoftDeleted); + /** * Generates a new string ID that's guaranteed to be globally unique. */ @@ -2093,4 +2129,16 @@ protected AspectUpdateResult aspectCallbackHelpe } return new AspectUpdateResult(newAspectValue, false); } + + @VisibleForTesting + @Nonnull + protected RecordTemplate probeSoftDeletedValue(@Nonnull ASPECT aspect) { + if (aspect instanceof SoftDeletedAspect) { + SoftDeletedAspect softDeletedAspect = (SoftDeletedAspect) aspect; + String aspectContent = softDeletedAspect.getGma_deleted_content().getAspect(); + String aspectClassName = softDeletedAspect.getGma_deleted_content().getCanonicalName(); + return RecordUtils.toRecordTemplate(aspectClassName, aspectContent); + } + return aspect; + } } diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseReadDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseReadDAO.java index 7e208c512..dd7a7be04 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseReadDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseReadDAO.java @@ -8,7 +8,6 @@ import com.linkedin.metadata.validator.AspectValidator; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -78,13 +77,7 @@ public Optional get(@Nonnull Class, Optional>> get( @Nonnull Set> aspectClasses, @Nonnull Set urns) { - - final Set> keys = new HashSet<>(); - for (URN urn : urns) { - for (Class aspect : aspectClasses) { - keys.add(new AspectKey<>(aspect, urn, LATEST_VERSION)); - } - } + final Set> keys = getKeysForLatestVersion(urns, aspectClasses); final Map, Optional>> results = new HashMap<>(); get(keys).entrySet().forEach(entry -> { @@ -134,4 +127,11 @@ protected void checkValidAspect(@Nonnull Class aspectC protected void checkValidAspects(@Nonnull Set> aspectClasses) { aspectClasses.forEach(aspectClass -> checkValidAspect(aspectClass)); } + + protected Set> getKeysForLatestVersion( + @Nonnull Set urns, @Nonnull Set> aspectClasses) { + return urns.stream() + .flatMap(urn -> aspectClasses.stream().map(aspectClass -> new AspectKey<>(aspectClass, urn, LATEST_VERSION))) + .collect(Collectors.toSet()); + } } diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/utils/RecordUtils.java b/dao-api/src/main/java/com/linkedin/metadata/dao/utils/RecordUtils.java index 881a0eaa4..d9ba109e9 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/utils/RecordUtils.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/utils/RecordUtils.java @@ -151,6 +151,18 @@ public static RecordTemplate toRecordTemplate(@Nonnull String className, @Nonnul return toRecordTemplate(clazz, dataMap); } + /** + * The string-only version of the toRecordTemplate methods. + * + * @param className FQCN of the record class extending RecordTemplate + * @param jsonString a JSON string serialized using {@link JacksonDataTemplateCodec} + * @return the created {@link RecordTemplate} + */ + @Nonnull + public static RecordTemplate toRecordTemplate(@Nonnull String className, @Nonnull String jsonString) { + return toRecordTemplate(className, toDataMap(jsonString)); + } + /** * Gets the aspect from the aspect class. * diff --git a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java index 233261861..507d44e06 100644 --- a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java +++ b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java @@ -227,6 +227,13 @@ public long newNumericId(String namespace, int maxTransactionRetry) { return Collections.emptyMap(); } + @Override + @Nonnull + public Map, Optional> get( + Set> aspectKeys, boolean includeSoftDeleted) { + return Collections.emptyMap(); + } + @Override @Nonnull public Map, AspectWithExtraInfo> getWithExtraInfo( diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java index 416e89d13..233797e7c 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/EbeanLocalDAO.java @@ -1081,6 +1081,7 @@ protected void applyTimeBasedRetention(@Nonnull return get(keys, false); } + @Override @Nonnull public Map, Optional> get( @Nonnull Set> keys, boolean includeSoftDeleted) { From 0542244e8695d1a37b412fc871c4474c2f560bb0 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Thu, 1 May 2025 22:51:07 -0700 Subject: [PATCH 4/5] more tests, but no core yet --- .../linkedin/metadata/dao/BaseLocalDAO.java | 18 ++++++++--- .../metadata/dao/BaseLocalDAOTest.java | 27 ++++++++++++++++ .../metadata/dao/utils/RecordUtilsTest.java | 3 ++ .../metadata/dao/utils/EBeanDAOUtils.java | 27 +++++++++------- .../metadata/dao/utils/EBeanDAOUtilsTest.java | 31 +++++++++++++++++++ .../testing/SoftDeletedAspectCopy.pdl | 2 +- .../metadata/aspect/SoftDeletedAspect.pdl | 6 +++- 7 files changed, 96 insertions(+), 18 deletions(-) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java index 1dab07797..4b591a7eb 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java @@ -22,6 +22,7 @@ import com.linkedin.metadata.dao.builder.BaseLocalRelationshipBuilder.LocalRelationshipUpdates; import com.linkedin.metadata.dao.equality.DefaultEqualityTester; import com.linkedin.metadata.dao.equality.EqualityTester; +import com.linkedin.metadata.dao.exception.InvalidMetadataType; import com.linkedin.metadata.dao.exception.ModelValidationException; import com.linkedin.metadata.dao.ingestion.AspectCallbackResponse; import com.linkedin.metadata.dao.ingestion.BaseLambdaFunction; @@ -61,6 +62,7 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import java.util.NoSuchElementException; import java.util.Objects; import java.util.Optional; import java.util.Set; @@ -1743,7 +1745,7 @@ public Map, Optional void backfill(@Nonnull BackfillMode mode, @Nonnull ASPECT aspect, @Nonnull URN urn) { - RecordTemplate oldValue = probeSoftDeletedValue(aspect); + RecordTemplate oldValue = probeSoftDeletedValue(aspect, urn.toString()); if (mode == BackfillMode.MAE_ONLY || mode == BackfillMode.BACKFILL_ALL @@ -2132,13 +2134,19 @@ protected AspectUpdateResult aspectCallbackHelpe @VisibleForTesting @Nonnull - protected RecordTemplate probeSoftDeletedValue(@Nonnull ASPECT aspect) { + protected static RecordTemplate probeSoftDeletedValue( + @Nonnull ASPECT aspect, @Nonnull String urn) { if (aspect instanceof SoftDeletedAspect) { SoftDeletedAspect softDeletedAspect = (SoftDeletedAspect) aspect; - String aspectContent = softDeletedAspect.getGma_deleted_content().getAspect(); - String aspectClassName = softDeletedAspect.getGma_deleted_content().getCanonicalName(); - return RecordUtils.toRecordTemplate(aspectClassName, aspectContent); + if (!softDeletedAspect.hasGma_deleted_content()) { + throw new NoSuchElementException( + String.format("SoftDeletedAspect found for urn <%s> does not have gma_deleted_content field, cannot be backfilled.", urn)); + } + return RecordUtils.toRecordTemplate( + softDeletedAspect.getGma_deleted_content().getCanonicalName(), + softDeletedAspect.getGma_deleted_content().getAspect()); } + // if not a soft deleted aspect, return the original aspect as-is return aspect; } } diff --git a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java index 507d44e06..49f375cab 100644 --- a/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java +++ b/dao-api/src/test/java/com/linkedin/metadata/dao/BaseLocalDAOTest.java @@ -4,6 +4,8 @@ import com.linkedin.data.template.RecordTemplate; import com.linkedin.data.template.SetMode; import com.linkedin.data.template.UnionTemplate; +import com.linkedin.metadata.aspect.AuditedAspect; +import com.linkedin.metadata.aspect.SoftDeletedAspect; import com.linkedin.metadata.dao.builder.BaseLocalRelationshipBuilder.LocalRelationshipUpdates; import com.linkedin.metadata.dao.ingestion.AspectCallbackMapKey; import com.linkedin.metadata.dao.ingestion.AspectCallbackRoutingClient; @@ -41,6 +43,7 @@ import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.NoSuchElementException; import java.util.Optional; import java.util.Set; import java.util.function.BiConsumer; @@ -809,4 +812,28 @@ public void testAspectCallbackHelperWithUnregisteredAspect() throws URISyntaxExc // Verify that the result is the same as the input aspect since it's not registered assertEquals(result.getUpdatedAspect(), foo); } + + @Test + public void testProbeSoftDeletedValue() throws URISyntaxException { + FooUrn urn = new FooUrn(1); + + // bypass (non-soft-deleted) pathway + AspectBar barExpected = new AspectBar().setValue("foo"); + RecordTemplate fooActual = BaseLocalDAO.probeSoftDeletedValue(barExpected, urn.toString()); + assertEquals(fooActual, barExpected); + + // missing gma_deleted_content pathway + SoftDeletedAspect softDeletedAspectExpected = new SoftDeletedAspect(); + assertThrows(NoSuchElementException.class, + () -> BaseLocalDAO.probeSoftDeletedValue(softDeletedAspectExpected, urn.toString())); + + // ideal soft-delete pathway + AuditedAspect softDeletedContent = new AuditedAspect(); + softDeletedContent.setAspect("{\"value\": \"foo\"}"); + softDeletedContent.setCanonicalName("com.linkedin.testing.AspectBar"); + + softDeletedAspectExpected.setGma_deleted_content(softDeletedContent); + RecordTemplate softDeletedAspectIdealActual = BaseLocalDAO.probeSoftDeletedValue(softDeletedAspectExpected, urn.toString()); + assertEquals(softDeletedAspectIdealActual, barExpected); + } } diff --git a/dao-api/src/test/java/com/linkedin/metadata/dao/utils/RecordUtilsTest.java b/dao-api/src/test/java/com/linkedin/metadata/dao/utils/RecordUtilsTest.java index 7b05a11ee..c872d5659 100644 --- a/dao-api/src/test/java/com/linkedin/metadata/dao/utils/RecordUtilsTest.java +++ b/dao-api/src/test/java/com/linkedin/metadata/dao/utils/RecordUtilsTest.java @@ -82,6 +82,9 @@ public void testToRecordTemplate() throws IOException { assertEquals(actual2.getClass(), AspectFoo.class); assertEquals(actual2, expected); + + RecordTemplate actual3 = RecordUtils.toRecordTemplate("com.linkedin.testing.AspectFoo", jsonString); + assertEquals(actual3, expected); } @Test(expectedExceptions = ModelConversionException.class) diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java index 693d66e4c..820207f80 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java @@ -176,22 +176,17 @@ public static boolean compareResults(@Nullable ListResult resultOld, @Nul } /** - * Checks whether the aspect record has been soft deleted. - * - * @param aspect aspect value - * @return boolean representing whether the aspect record has been soft deleted + * Same as {@link #isSoftDeletedAspect(SqlRow, String)}, but for {@link EbeanMetadataAspect}. */ public static boolean isSoftDeletedAspect(@Nonnull EbeanMetadataAspect aspect) { - // Convert metadata string to record template object try { - SoftDeletedAspect attemptToCastToSDA = RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata()); - return attemptToCastToSDA.hasGma_deleted() && attemptToCastToSDA.isGma_deleted(); + RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata()); + return true; } catch (Exception e) { return false; } } - /** * Read {@link SqlRow} list into a {@link EbeanMetadataAspect} list. * Currently only in use by AIM. @@ -244,7 +239,8 @@ public static List readSqlR /** * Read EbeanMetadataAspect from {@link SqlRow}. - * Note that if the Aspect is (soft) deleted and the DB is on the NEW SCHEMA, the creation metadata will be + * + *

Note that if the Aspect is (soft) deleted and the DB is on the NEW SCHEMA, the creation metadata will be * associated with the *whole row*, not the individual aspect. There is currently no metadata stored for the deletion * of aspects. * @@ -287,14 +283,23 @@ private static EbeanMetadataAspect readSqlRow(Sq /** * Checks whether the entity table record has been soft deleted. + * + *

NOTE: the ability to cast the aspect to a {@link SoftDeletedAspect} is sufficient to determine + * whether the aspect has been soft-deleted. This is because there are NO current use cases where we + * store a SoftDeletedAspect with the flag set to anything other than "true". + * + *

This "shallow check" is necessary because many usages of checking soft-deletion are followed by + * a deserialization call to {@link RecordUtils#toRecordTemplate(Class, String)}, which will fail if + * we try to deserialize a SoftDeletedAspect -- to another Aspect Type -- with the flag set to "false". + * * @param sqlRow {@link SqlRow} result from MySQL server * @param columnName column name of entity table * @return boolean representing whether the aspect record has been soft deleted */ public static boolean isSoftDeletedAspect(@Nonnull SqlRow sqlRow, @Nonnull String columnName) { try { - SoftDeletedAspect aspect = RecordUtils.toRecordTemplate(SoftDeletedAspect.class, sqlRow.getString(columnName)); - return aspect.hasGma_deleted() && aspect.isGma_deleted(); + RecordUtils.toRecordTemplate(SoftDeletedAspect.class, sqlRow.getString(columnName)); + return true; } catch (Exception e) { return false; } diff --git a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/EBeanDAOUtilsTest.java b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/EBeanDAOUtilsTest.java index 2f2b31dd8..b3d680cf6 100644 --- a/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/EBeanDAOUtilsTest.java +++ b/dao-impl/ebean-dao/src/test/java/com/linkedin/metadata/dao/utils/EBeanDAOUtilsTest.java @@ -58,6 +58,9 @@ public class EBeanDAOUtilsTest { + final static String SOFT_DELETED_ASPECT_WITH_DELETED_CONTENT = "{\"gma_deleted\": true," + + "\"gma_deleted_content\": {\"aspect\": \"{removed: false}\", \"canonicalName\": \"com.linkedin.common.Status\"," + + "\"lastmodifiedon\": \"0L\", \"lastmodifiedby\": \"urn:li:tester\"}}"; @Nonnull private String readSQLfromFile(@Nonnull String resourcePath) { @@ -573,6 +576,34 @@ public void testIsSoftDeletedAspect() { when(sqlRow.getString("a_aspectbaz")).thenReturn("{\"random_value\": \"baz\"}"); assertFalse(EBeanDAOUtils.isSoftDeletedAspect(sqlRow, "a_aspectbaz")); + + // NOTE how this is should return "true"; see the explanation in the method javadoc + when(sqlRow.getString("a_aspectqux")).thenReturn("{\"gma_deleted\": false}"); + assertTrue(EBeanDAOUtils.isSoftDeletedAspect(sqlRow, "a_aspectqux")); + + when(sqlRow.getString("a_aspectbax")).thenReturn(SOFT_DELETED_ASPECT_WITH_DELETED_CONTENT); + assertTrue(EBeanDAOUtils.isSoftDeletedAspect(sqlRow, "a_aspectbax")); + } + + @Test + public void testIsSoftDeletedAspectEbeanMetadataAspect() { + EbeanMetadataAspect ebeanMetadataAspect = new EbeanMetadataAspect(); + + ebeanMetadataAspect.setMetadata("{\"gma_deleted\": true}"); + assertTrue(EBeanDAOUtils.isSoftDeletedAspect(ebeanMetadataAspect)); + + ebeanMetadataAspect.setMetadata(SOFT_DELETED_ASPECT_WITH_DELETED_CONTENT); + assertTrue(EBeanDAOUtils.isSoftDeletedAspect(ebeanMetadataAspect)); + + // NOTE how this is should return "true"; see the explanation in the method javadoc + ebeanMetadataAspect.setMetadata("{\"gma_deleted\": false}"); + assertTrue(EBeanDAOUtils.isSoftDeletedAspect(ebeanMetadataAspect)); + + ebeanMetadataAspect.setMetadata("{\"aspect\": {\"value\": \"bar\"}, \"lastmodifiedby\": \"urn:li:tester\"}"); + assertFalse(EBeanDAOUtils.isSoftDeletedAspect(ebeanMetadataAspect)); + + ebeanMetadataAspect.setMetadata("{\"random_value\": \"baz\"}"); + assertFalse(EBeanDAOUtils.isSoftDeletedAspect(ebeanMetadataAspect)); } @Test diff --git a/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl b/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl index e837bd2bc..9cdc5a7d0 100644 --- a/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl +++ b/testing/test-models/src/main/pegasus/com/linkedin/testing/SoftDeletedAspectCopy.pdl @@ -15,5 +15,5 @@ record SoftDeletedAspectCopy { /** * The deleted content of the aspect */ - gma_deleted_content: AuditedAspect + gma_deleted_content: optional AuditedAspect } \ No newline at end of file diff --git a/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl b/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl index b56fea9d9..b40e9dfd8 100644 --- a/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl +++ b/validators/src/main/pegasus/com/linkedin/metadata/aspect/SoftDeletedAspect.pdl @@ -7,11 +7,15 @@ record SoftDeletedAspect { /** * Flag to indicate if the aspect is soft deleted in GMA + * + * Currently, there are no cases where the aspect is soft-deleted but this flag is NOT set to + * true: the very existence of this Aspect type indicates soft-deletion. This is to say that + * any aspect that is not soft-deleted will simply not exist -- cannot be cast -- as this type. */ gma_deleted: boolean /** * The deleted content of the aspect */ - gma_deleted_content: AuditedAspect + gma_deleted_content: optional AuditedAspect } \ No newline at end of file From 4415795e72379d44a5e36db8d04d16524b46f1d7 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Thu, 1 May 2025 23:59:18 -0700 Subject: [PATCH 5/5] fixed --- .../main/java/com/linkedin/metadata/dao/BaseLocalDAO.java | 1 - .../com/linkedin/metadata/dao/utils/EBeanDAOUtils.java | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java index 4b591a7eb..9556e6118 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/BaseLocalDAO.java @@ -22,7 +22,6 @@ import com.linkedin.metadata.dao.builder.BaseLocalRelationshipBuilder.LocalRelationshipUpdates; import com.linkedin.metadata.dao.equality.DefaultEqualityTester; import com.linkedin.metadata.dao.equality.EqualityTester; -import com.linkedin.metadata.dao.exception.InvalidMetadataType; import com.linkedin.metadata.dao.exception.ModelValidationException; import com.linkedin.metadata.dao.ingestion.AspectCallbackResponse; import com.linkedin.metadata.dao.ingestion.BaseLambdaFunction; diff --git a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java index 820207f80..305718d25 100644 --- a/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java +++ b/dao-impl/ebean-dao/src/main/java/com/linkedin/metadata/dao/utils/EBeanDAOUtils.java @@ -180,8 +180,8 @@ public static boolean compareResults(@Nullable ListResult resultOld, @Nul */ public static boolean isSoftDeletedAspect(@Nonnull EbeanMetadataAspect aspect) { try { - RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata()); - return true; + SoftDeletedAspect softDeletedAspect = RecordUtils.toRecordTemplate(SoftDeletedAspect.class, aspect.getMetadata()); + return softDeletedAspect.hasGma_deleted(); } catch (Exception e) { return false; } @@ -298,8 +298,8 @@ private static EbeanMetadataAspect readSqlRow(Sq */ public static boolean isSoftDeletedAspect(@Nonnull SqlRow sqlRow, @Nonnull String columnName) { try { - RecordUtils.toRecordTemplate(SoftDeletedAspect.class, sqlRow.getString(columnName)); - return true; + SoftDeletedAspect softDeletedAspect = RecordUtils.toRecordTemplate(SoftDeletedAspect.class, sqlRow.getString(columnName)); + return softDeletedAspect.hasGma_deleted(); } catch (Exception e) { return false; }