fix: properly validate that aspects can only be of type record (#144)

jywadhwani · web-flow · commit d2d88bb47d76 · 2022-01-21T13:51:06.000-08:00
* properly validate that aspects can only be of type record

* add more tests to validation util

* remove union test

* change name of method
diff --git a/dao-api/src/main/pegasus/com/linkedin/metadata/dummy/DummyAspect.pdl b/dao-api/src/main/pegasus/com/linkedin/metadata/dummy/DummyAspect.pdl
@@ -1,8 +1,6 @@
 namespace com.linkedin.metadata.dummy
 
-import com.linkedin.common.Urn
-
 /**
  * Not to be used
  */
-typeref DummyAspect = union[Urn]
+typeref DummyAspect = union[DummyRecord]
diff --git a/dao-api/src/main/pegasus/com/linkedin/metadata/dummy/DummyRecord.pdl b/dao-api/src/main/pegasus/com/linkedin/metadata/dummy/DummyRecord.pdl
@@ -0,0 +1,9 @@
+namespace com.linkedin.metadata.dummy
+
+/**
+ * Not to be used
+ */
+record DummyRecord {
+
+  value: string
+}
diff --git a/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaArrayAspect.pdl b/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaArrayAspect.pdl
@@ -0,0 +1,5 @@
+namespace com.linkedin.testing
+
+typeref PizzaArrayAspect = union[
+  array[long]
+]
diff --git a/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaEnumAspect.pdl b/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaEnumAspect.pdl
@@ -0,0 +1,5 @@
+namespace com.linkedin.testing
+
+typeref PizzaEnumAspect = union[
+  PizzaSize
+]
diff --git a/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaMapAspect.pdl b/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaMapAspect.pdl
@@ -0,0 +1,5 @@
+namespace com.linkedin.testing
+
+typeref PizzaMapAspect = union[
+  map[string, boolean]
+]
diff --git a/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaStringAspect.pdl b/testing/test-models/src/main/pegasus/com/linkedin/testing/PizzaStringAspect.pdl
@@ -0,0 +1,5 @@
+namespace com.linkedin.testing
+
+typeref PizzaStringAspect = union[
+  string
+]
diff --git a/validators/build.gradle b/validators/build.gradle
@@ -12,4 +12,5 @@ dependencies {
   annotationProcessor externalDependency.lombok
 
   testCompile externalDependency.guava
+  testCompile project(':testing:test-models')
 }
diff --git a/validators/src/main/java/com/linkedin/metadata/validator/AspectValidator.java b/validators/src/main/java/com/linkedin/metadata/validator/AspectValidator.java
@@ -28,7 +28,7 @@ private AspectValidator() {
    */
   public static void validateAspectUnionSchema(@Nonnull UnionDataSchema schema, @Nonnull String aspectClassName) {
 
-    if (!ValidationUtils.isUnionWithOnlyComplexMembers(schema)) {
+    if (!ValidationUtils.isUnionWithOnlyRecordMembers(schema)) {
       ValidationUtils.invalidSchema("Aspect '%s' must be a union containing only record type members", aspectClassName);
     }
   }
@@ -47,7 +47,7 @@ public static void validateAspectUnionSchema(@Nonnull Class<? extends UnionTempl
 
   private static boolean isValidMetadataField(RecordDataSchema.Field field) {
     return field.getName().equals("metadata") && !field.getOptional()
-        && field.getType().getType() == DataSchema.Type.UNION && ValidationUtils.isUnionWithOnlyComplexMembers(
+        && field.getType().getType() == DataSchema.Type.UNION && ValidationUtils.isUnionWithOnlyRecordMembers(
         (UnionDataSchema) field.getType());
   }
 }
diff --git a/validators/src/main/java/com/linkedin/metadata/validator/EntityValidator.java b/validators/src/main/java/com/linkedin/metadata/validator/EntityValidator.java
@@ -89,7 +89,7 @@ public static void validateEntityUnionSchema(@Nonnull Class<? extends UnionTempl
    */
   public static void validateEntityUnionSchema(@Nonnull UnionDataSchema schema, @Nonnull String entityClassName) {
 
-    if (!ValidationUtils.isUnionWithOnlyComplexMembers(schema)) {
+    if (!ValidationUtils.isUnionWithOnlyRecordMembers(schema)) {
       ValidationUtils.invalidSchema("Entity '%s' must be a union containing only record type members", entityClassName);
     }
   }
diff --git a/validators/src/main/java/com/linkedin/metadata/validator/RelationshipValidator.java b/validators/src/main/java/com/linkedin/metadata/validator/RelationshipValidator.java
@@ -94,7 +94,7 @@ public static void validateRelationshipUnionSchema(@Nonnull Class<? extends Unio
    */
   public static void validateRelationshipUnionSchema(@Nonnull UnionDataSchema schema, @Nonnull String relationshipClassName) {
 
-    if (!ValidationUtils.isUnionWithOnlyComplexMembers(schema)) {
+    if (!ValidationUtils.isUnionWithOnlyRecordMembers(schema)) {
       ValidationUtils.invalidSchema("Relationship '%s' must be a union containing only record type members", relationshipClassName);
     }
   }
diff --git a/validators/src/main/java/com/linkedin/metadata/validator/ValidationUtils.java b/validators/src/main/java/com/linkedin/metadata/validator/ValidationUtils.java
@@ -157,8 +157,9 @@ public static List<RecordDataSchema.Field> fieldsUsingInvalidType(@Nonnull Recor
         .collect(Collectors.toList());
   }
 
-  public static boolean isUnionWithOnlyComplexMembers(UnionDataSchema unionDataSchema) {
-    return unionDataSchema.getMembers().stream().allMatch(member -> member.getType().isComplex());
+  public static boolean isUnionWithOnlyRecordMembers(UnionDataSchema unionDataSchema) {
+    return unionDataSchema.getMembers().stream().allMatch(member -> member.getType().getDereferencedDataSchema().getType().equals(
+        DataSchema.Type.RECORD));
   }
 
   @Nonnull
diff --git a/validators/src/test/java/com/linkedin/metadata/validator/ValidationUtilsTest.java b/validators/src/test/java/com/linkedin/metadata/validator/ValidationUtilsTest.java
@@ -0,0 +1,38 @@
+package com.linkedin.metadata.validator;
+
+import com.linkedin.data.schema.UnionDataSchema;
+import com.linkedin.testing.PizzaArrayAspect;
+import com.linkedin.testing.PizzaEnumAspect;
+import com.linkedin.testing.PizzaMapAspect;
+import com.linkedin.testing.PizzaStringAspect;
+import org.testng.annotations.Test;
+
+import static org.testng.Assert.*;
+
+
+public class ValidationUtilsTest {
+
+  @Test
+  public void testUnionWithEnums() {
+    UnionDataSchema dataSchema = ValidationUtils.getUnionSchema(PizzaEnumAspect.class);
+    assertFalse(ValidationUtils.isUnionWithOnlyRecordMembers(dataSchema));
+  }
+
+  @Test
+  public void testUnionWithMaps() {
+    UnionDataSchema dataSchema = ValidationUtils.getUnionSchema(PizzaMapAspect.class);
+    assertFalse(ValidationUtils.isUnionWithOnlyRecordMembers(dataSchema));
+  }
+
+  @Test
+  public void testUnionWithArrays() {
+    UnionDataSchema dataSchema = ValidationUtils.getUnionSchema(PizzaArrayAspect.class);
+    assertFalse(ValidationUtils.isUnionWithOnlyRecordMembers(dataSchema));
+  }
+
+  @Test
+  public void testUnionWithPrimitive() {
+    UnionDataSchema dataSchema = ValidationUtils.getUnionSchema(PizzaStringAspect.class);
+    assertFalse(ValidationUtils.isUnionWithOnlyRecordMembers(dataSchema));
+  }
+}

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +namespace com.linkedin.testing
++
 +typeref PizzaStringAspect = union[
 +  string
 +]
Original file line number	Diff line number	Diff line change
`@@ -12,4 +12,5 @@ dependencies {`
`12`	`12`	`annotationProcessor externalDependency.lombok`
`13`	`13`
`14`	`14`	`testCompile externalDependency.guava`
	`15`	`+ testCompile project(':testing:test-models')`
`15`	`16`	`}`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ private AspectValidator() {`
`28`	`28`	`*/`
`29`	`29`	`public static void validateAspectUnionSchema(@Nonnull UnionDataSchema schema, @Nonnull String aspectClassName) {`
`30`	`30`
`31`		`- if (!ValidationUtils.isUnionWithOnlyComplexMembers(schema)) {`
	`31`	`+ if (!ValidationUtils.isUnionWithOnlyRecordMembers(schema)) {`
`32`	`32`	`ValidationUtils.invalidSchema("Aspect '%s' must be a union containing only record type members", aspectClassName);`
`33`	`33`	`}`
`34`	`34`	`}`
`@@ -47,7 +47,7 @@ public static void validateAspectUnionSchema(@Nonnull Class<? extends UnionTempl`
`47`	`47`
`48`	`48`	`private static boolean isValidMetadataField(RecordDataSchema.Field field) {`
`49`	`49`	`return field.getName().equals("metadata") && !field.getOptional()`
`50`		`- && field.getType().getType() == DataSchema.Type.UNION && ValidationUtils.isUnionWithOnlyComplexMembers(`
	`50`	`+ && field.getType().getType() == DataSchema.Type.UNION && ValidationUtils.isUnionWithOnlyRecordMembers(`
`51`	`51`	`(UnionDataSchema) field.getType());`
`52`	`52`	`}`
`53`	`53`	`}`
Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,7 @@ public static void validateEntityUnionSchema(@Nonnull Class<? extends UnionTempl`
`89`	`89`	`*/`
`90`	`90`	`public static void validateEntityUnionSchema(@Nonnull UnionDataSchema schema, @Nonnull String entityClassName) {`
`91`	`91`
`92`		`- if (!ValidationUtils.isUnionWithOnlyComplexMembers(schema)) {`
	`92`	`+ if (!ValidationUtils.isUnionWithOnlyRecordMembers(schema)) {`
`93`	`93`	`ValidationUtils.invalidSchema("Entity '%s' must be a union containing only record type members", entityClassName);`
`94`	`94`	`}`
`95`	`95`	`}`
Original file line number	Diff line number	Diff line change
`@@ -94,7 +94,7 @@ public static void validateRelationshipUnionSchema(@Nonnull Class<? extends Unio`
`94`	`94`	`*/`
`95`	`95`	`public static void validateRelationshipUnionSchema(@Nonnull UnionDataSchema schema, @Nonnull String relationshipClassName) {`
`96`	`96`
`97`		`- if (!ValidationUtils.isUnionWithOnlyComplexMembers(schema)) {`
	`97`	`+ if (!ValidationUtils.isUnionWithOnlyRecordMembers(schema)) {`
`98`	`98`	`ValidationUtils.invalidSchema("Relationship '%s' must be a union containing only record type members", relationshipClassName);`
`99`	`99`	`}`
`100`	`100`	`}`