Skip to content

Commit

Permalink
Fix silent failure when ingesting an Asset with an unrecognized field (
Browse files Browse the repository at this point in the history
  • Loading branch information
jphui authored Nov 27, 2024
1 parent 5261abf commit 783ae91
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ public static <ASSET extends RecordTemplate> List<Class<? extends RecordTemplate
@Nonnull
public static <ASSET extends RecordTemplate> List<RecordTemplate> getAspectsFromAsset(@Nonnull ASSET asset) {
AssetValidator.validateAssetSchema(asset.getClass());
ValidationUtils.validateAgainstSchema(asset);
// TODO: cache the asset methods loading
try {
final List<RecordTemplate> aspects = new ArrayList<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Class<? extends RecordTemplate>> assetTypes = ModelUtils.getAspectTypesFromAssetType(BarAsset.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -40,6 +46,9 @@ public final class ValidationUtils {

private static final Map<String, String> 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
}
Expand Down Expand Up @@ -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.
*
* <p>This is copied from BaseLocalDAO, which uses this for Aspect-level ingestion validation. This is meant for
* Asset-level validation.</p>
*
* <p>Reference ticket: META-21242</p>
*/
public static void validateAgainstSchema(@Nonnull RecordTemplate model) {
ValidationResult result = ValidateDataAgainstSchema.validate(model, VALIDATION_OPTIONS);
if (!result.isValid()) {
invalidSchema(String.valueOf(result.getMessages()));
}
}

}

0 comments on commit 783ae91

Please sign in to comment.