From 783ae912e13230b811a9a6e8c51c3e36422e2e06 Mon Sep 17 00:00:00 2001 From: Jonathan Hui Date: Wed, 27 Nov 2024 16:28:06 -0500 Subject: [PATCH] Fix silent failure when ingesting an Asset with an unrecognized field (#481) --- .../metadata/dao/utils/ModelUtils.java | 1 + .../metadata/dao/utils/ModelUtilsTest.java | 22 ++++++++++++++++ .../restli/BaseEntityResourceTest.java | 26 +++++++++++++++++++ .../metadata/validator/ValidationUtils.java | 26 +++++++++++++++++++ 4 files changed, 75 insertions(+) diff --git a/dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java b/dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java index d5caf60ba..8ff80fe4c 100644 --- a/dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java +++ b/dao-api/src/main/java/com/linkedin/metadata/dao/utils/ModelUtils.java @@ -396,6 +396,7 @@ public static List List getAspectsFromAsset(@Nonnull ASSET asset) { AssetValidator.validateAssetSchema(asset.getClass()); + ValidationUtils.validateAgainstSchema(asset); // TODO: cache the asset methods loading try { final List aspects = new ArrayList<>(); diff --git a/dao-api/src/test/java/com/linkedin/metadata/dao/utils/ModelUtilsTest.java b/dao-api/src/test/java/com/linkedin/metadata/dao/utils/ModelUtilsTest.java index c2b5153c7..f32727aa0 100644 --- a/dao-api/src/test/java/com/linkedin/metadata/dao/utils/ModelUtilsTest.java +++ b/dao-api/src/test/java/com/linkedin/metadata/dao/utils/ModelUtilsTest.java @@ -347,6 +347,28 @@ public void testGetAspectsFromAsset() { assertTrue(aspects.isEmpty()); } + @Test + public void testGetAspectsFromAssetFailUnrecognizedField() { + // same setup as the previous test... + EntityAsset asset = new EntityAsset(); + AspectFoo expectedAspectFoo = new AspectFoo().setValue("foo"); + AspectBar expectedAspectBar = new AspectBar().setValue("bar"); + asset.setFoo(expectedAspectFoo); + asset.setBar(expectedAspectBar); + + // but now, we simulate adding in an unrecognized field, which encompasses 2 cases where silent failures can occur: + // (1) curli calls with fields that are simply not a part of the model, these should error out! + // (2) (java) client calls with a newer bump to metadata-models that contain NEW fields that are + // not recognized by the server, these should also error out! + + // set the datamap to contain an unrecognized field + asset.data().put("unrecognizedField", "unrecognizedValue"); + + assertThrows(InvalidSchemaException.class, () -> { + ModelUtils.getAspectsFromAsset(asset); + }); + } + @Test public void testGetAspectTypesFromAssetType() { List> assetTypes = ModelUtils.getAspectTypesFromAssetType(BarAsset.class); diff --git a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java index d4681b29a..52d2ffc3c 100644 --- a/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java +++ b/restli-resources/src/test/java/com/linkedin/metadata/restli/BaseEntityResourceTest.java @@ -1427,6 +1427,32 @@ public void testIngestAsset() { verifyNoMoreInteractions(_mockLocalDAO); } + @Test + public void testIngestAssetFailUnrecognizedField() { + // same setup as the previous test... + FooUrn urn = makeFooUrn(1); + EntityAsset asset = new EntityAsset(); + AspectFoo foo = new AspectFoo().setValue("foo"); + AspectBar bar = new AspectBar().setValue("bar"); + asset.setUrn(urn); + asset.setFoo(foo); + asset.setBar(bar); + IngestionTrackingContext trackingContext = new IngestionTrackingContext(); + + // but now, we simulate adding in an unrecognized field, which encompasses 2 cases where silent failures can occur: + // (1) curli calls with fields that are simply not a part of the model, these should error out! + // (2) (java) client calls with a newer bump to metadata-models that contain NEW fields that are + // not recognized by the server, these should also error out! + + // set the datamap to contain an unrecognized field + asset.data().put("unrecognizedField", "unrecognizedValue"); + + IngestionParams ingestionParams1 = new IngestionParams().setTestMode(true); + ingestionParams1.setIngestionTrackingContext(trackingContext); + + assertThrows(RestLiServiceException.class, () -> runAndWait(_internalResource.ingestAsset(asset, ingestionParams1))); + } + @Test public void testGetAsset() { FooUrn urn = makeFooUrn(1); diff --git a/validators/src/main/java/com/linkedin/metadata/validator/ValidationUtils.java b/validators/src/main/java/com/linkedin/metadata/validator/ValidationUtils.java index 9b64495ac..4ac576271 100644 --- a/validators/src/main/java/com/linkedin/metadata/validator/ValidationUtils.java +++ b/validators/src/main/java/com/linkedin/metadata/validator/ValidationUtils.java @@ -6,6 +6,12 @@ import com.linkedin.data.schema.DataSchema; import com.linkedin.data.schema.RecordDataSchema; import com.linkedin.data.schema.UnionDataSchema; +import com.linkedin.data.schema.validation.CoercionMode; +import com.linkedin.data.schema.validation.RequiredMode; +import com.linkedin.data.schema.validation.UnrecognizedFieldMode; +import com.linkedin.data.schema.validation.ValidateDataAgainstSchema; +import com.linkedin.data.schema.validation.ValidationOptions; +import com.linkedin.data.schema.validation.ValidationResult; import com.linkedin.data.template.RecordTemplate; import com.linkedin.data.template.UnionTemplate; import com.linkedin.metadata.aspect.SoftDeletedAspect; @@ -40,6 +46,9 @@ public final class ValidationUtils { private static final Map SOFT_DELETED_FIELD_METADATA = fieldMetadata(new SoftDeletedAspect().schema()); + public static final ValidationOptions VALIDATION_OPTIONS = + new ValidationOptions(RequiredMode.CAN_BE_ABSENT_IF_HAS_DEFAULT, CoercionMode.NORMAL, UnrecognizedFieldMode.DISALLOW); + private ValidationUtils() { // Util class } @@ -209,4 +218,21 @@ private static DataSchema.Type getFieldOrArrayItemType(@Nonnull RecordDataSchema public static void throwNullFieldException(String field) { throw new NullFieldException(String.format("The '%s' field cannot be null.", field)); } + + /** + * Validates a model against its schema, useful for checking to make sure that a (curli) ingestion request's + * fields are all valid fields of the model. + * + *

This is copied from BaseLocalDAO, which uses this for Aspect-level ingestion validation. This is meant for + * Asset-level validation.

+ * + *

Reference ticket: META-21242

+ */ + public static void validateAgainstSchema(@Nonnull RecordTemplate model) { + ValidationResult result = ValidateDataAgainstSchema.validate(model, VALIDATION_OPTIONS); + if (!result.isValid()) { + invalidSchema(String.valueOf(result.getMessages())); + } + } + }