throw better error msg on default parse failure (linkedin#501)

RTBHOUSE · Aug 23, 2023 · ed58305 · ed58305
1 parent e3fa1db
commit ed58305
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 14 deletions.
diff --git a/parser/src/main/java/com/linkedin/avroutil1/parser/avsc/AvscParser.java b/parser/src/main/java/com/linkedin/avroutil1/parser/avsc/AvscParser.java
@@ -400,10 +400,10 @@ private AvroNamedSchema parseNamedSchema(
                             }
                             LiteralOrIssue defaultValueOrIssue = parseLiteral(fieldDefaultValueNode, defaultValueExpectedSchema, fieldName.getValue(), context);
                             if (defaultValueOrIssue.getIssue() == null) {
-                                //TODO - allow parsing default values that are branch != 0 (and add an issue)
                                 defaultValue = defaultValueOrIssue.getLiteral();
                             } else {
-                                context.addIssue(defaultValueOrIssue.getIssue());
+                                defaultValue = new AvscUnparsedLiteral(fieldDefaultValueNode,
+                                    defaultValueOrIssue.getIssue().getLocation());
                             }
                             //TODO - handle issues
                         } else {

diff --git a/parser/src/main/java/com/linkedin/avroutil1/writer/avsc/AvscSchemaWriter.java b/parser/src/main/java/com/linkedin/avroutil1/writer/avsc/AvscSchemaWriter.java
@@ -34,6 +34,7 @@
 import com.linkedin.avroutil1.model.AvroUnionSchema;
 import com.linkedin.avroutil1.model.JsonPropertiesContainer;
 import com.linkedin.avroutil1.model.SchemaOrRef;
+import com.linkedin.avroutil1.parser.avsc.AvscUnparsedLiteral;
 import java.io.StringReader;
 import java.io.StringWriter;
 import java.nio.charset.StandardCharsets;
@@ -324,7 +325,7 @@ protected void emitRecordFields(AvroRecordSchema schema, AvscWriterContext conte
 
       if (field.hasDefaultValue()) {
         AvroLiteral defaultValue = field.getDefaultValue();
-        JsonValue defaultValueLiteral = writeDefaultValue(fieldSchema, defaultValue);
+        JsonValue defaultValueLiteral = writeDefaultValue(fieldSchema, defaultValue, field);
         fieldBuilder.add("default", defaultValueLiteral);
       }
       //TODO - order
@@ -340,10 +341,25 @@ protected void emitRecordFields(AvroRecordSchema schema, AvscWriterContext conte
     output.add("fields", arrayBuilder);
   }
 
-  protected JsonValue writeDefaultValue(AvroSchema fieldSchema, AvroLiteral literal) {
-    AvroType type = fieldSchema.type();
+  protected JsonValue writeDefaultValue(AvroSchema schemaForLiteral, AvroLiteral literal, AvroSchemaField field) {
+    AvroType type = schemaForLiteral.type();
     String temp;
     AvroSchema valueSchema;
+
+    // if literal is of type AvscUnparsedLiteral, throw an exception
+    if (literal instanceof AvscUnparsedLiteral) {
+      if (!type.equals(AvroType.UNION)) {
+        throw new IllegalArgumentException(
+            "Default value for field \"" + field.getName() + "\" is a malformed avro " + type.toTypeName()
+                + " default value. Default value: " + ((AvscUnparsedLiteral) literal).getDefaultValueNode().toString());
+      } else {
+        // union defaults are commonly malformed so add what the likely issue is in the error msg
+        throw new IllegalArgumentException("Default value for union field \"" + field.getName()
+            + "\" should be the type of the 1st schema in the union. Default value: "
+            + ((AvscUnparsedLiteral) literal).getDefaultValueNode().toString());
+      }
+    }
+
     switch (type) {
       case NULL:
         //noinspection unused (kept as a sanity check)
@@ -389,7 +405,7 @@ protected JsonValue writeDefaultValue(AvroSchema fieldSchema, AvroLiteral litera
         valueSchema = arraySchema.getValueSchema();
         JsonArrayBuilder arrayBuilder = Json.createArrayBuilder();
         for  (AvroLiteral element : array) {
-          JsonValue serializedElement = writeDefaultValue(valueSchema, element);
+          JsonValue serializedElement = writeDefaultValue(valueSchema, element, field);
           arrayBuilder.add(serializedElement);
         }
         return arrayBuilder.build();
@@ -400,23 +416,23 @@ protected JsonValue writeDefaultValue(AvroSchema fieldSchema, AvroLiteral litera
         valueSchema = mapSchema.getValueSchema();
         JsonObjectBuilder objectBuilder = Json.createObjectBuilder();
         for (Map.Entry<String, AvroLiteral> entry : map.entrySet()) {
-          JsonValue serializedValue = writeDefaultValue(valueSchema, entry.getValue());
+          JsonValue serializedValue = writeDefaultValue(valueSchema, entry.getValue(), field);
           objectBuilder.add(entry.getKey(), serializedValue);
         }
         return objectBuilder.build();
       case UNION:
         //default values for unions must be of the 1st type in the union
-        AvroUnionSchema unionSchema = (AvroUnionSchema) fieldSchema;
+        AvroUnionSchema unionSchema = (AvroUnionSchema) schemaForLiteral;
         AvroSchema firstBranchSchema = unionSchema.getTypes().get(0).getSchema();
-        return writeDefaultValue(firstBranchSchema, literal);
+        return writeDefaultValue(firstBranchSchema, literal, field);
       case RECORD:
-        AvroRecordSchema recordSchema = (AvroRecordSchema) fieldSchema;
+        AvroRecordSchema recordSchema = (AvroRecordSchema) schemaForLiteral;
         JsonObjectBuilder recordObjectBuilder = Json.createObjectBuilder();
         Map<String, AvroLiteral> recordLiteralMap = ((AvroRecordLiteral) literal).getValue();
 
-        for (AvroSchemaField field : recordSchema.getFields()) {
-          recordObjectBuilder.add(field.getName(),
-              writeDefaultValue(field.getSchema(), recordLiteralMap.get(field.getName())));
+        for (AvroSchemaField innerField : recordSchema.getFields()) {
+          recordObjectBuilder.add(innerField.getName(),
+              writeDefaultValue(innerField.getSchema(), recordLiteralMap.get(innerField.getName()), field));
         }
         return recordObjectBuilder.build();
       default:

diff --git a/parser/src/test/java/com/linkedin/avroutil1/parser/avsc/AvscParserTest.java b/parser/src/test/java/com/linkedin/avroutil1/parser/avsc/AvscParserTest.java
@@ -265,7 +265,7 @@ public void testParsingHorribleDefaultValues() throws Exception {
             List<AvscIssue> issuesWithField = result.getIssues(field);
             Assert.assertFalse(issuesWithField.isEmpty(), "field " + field.getName() + " has no issues?!");
             Assert.assertFalse(issuesWithField.stream().noneMatch(issue -> issue.getMessage().contains("default value")));
-            Assert.assertNull(field.getDefaultValue());
+            Assert.assertTrue(field.getDefaultValue() instanceof AvscUnparsedLiteral);
         }
     }
 

diff --git a/parser/src/test/java/com/linkedin/avroutil1/writer/avsc/AvscSchemaWriterTest.java b/parser/src/test/java/com/linkedin/avroutil1/writer/avsc/AvscSchemaWriterTest.java
@@ -93,6 +93,27 @@ public void testWritingSimpleNamedSchemas() throws Exception {
     testParsingCycle(avsc);
   }
 
+  @Test
+  void testBadDefaultValues() throws IOException {
+    String recordAvsc = TestUtil.load("schemas/badDefaultValues/BadRecord.avsc");
+    Assert.assertThrows(IllegalArgumentException.class, () -> testAvscWriterForBadSchema(recordAvsc));
+
+    String unionAvsc = TestUtil.load("schemas/badDefaultValues/BadUnion.avsc");
+    Assert.assertThrows(IllegalArgumentException.class, () -> testAvscWriterForBadSchema(unionAvsc));
+
+    String badPrimitiveUnionAvsc = TestUtil.load("schemas/badDefaultValues/BadPrimitiveUnion.avsc");
+    Assert.assertThrows(IllegalArgumentException.class, () -> testAvscWriterForBadSchema(badPrimitiveUnionAvsc));
+  }
+
+  void testAvscWriterForBadSchema(String avsc) {
+    AvscParser parser = new AvscParser();
+    AvscParseResult parseResults = parser.parse(avsc);
+    AvroSchema parsed = parseResults.getTopLevelSchema();
+    Assert.assertNotNull(parsed);
+    AvscSchemaWriter writer = new AvscSchemaWriter();
+    writer.writeSingle(parsed);
+  }
+
   /**
    * given an avsc, parses and re-prints it using our code
    * and compares the result to vanilla avro.

diff --git a/parser/src/test/resources/schemas/badDefaultValues/BadPrimitiveUnion.avsc b/parser/src/test/resources/schemas/badDefaultValues/BadPrimitiveUnion.avsc
@@ -0,0 +1,14 @@
+{
+  "type": "record",
+  "name": "User",
+  "fields": [
+    {
+      "name": "name",
+      "type": [
+        "null",
+        "string"
+      ],
+      "default": "shouldBeNull"
+    }
+  ]
+}
diff --git a/parser/src/test/resources/schemas/badDefaultValues/BadRecord.avsc b/parser/src/test/resources/schemas/badDefaultValues/BadRecord.avsc
@@ -0,0 +1,30 @@
+{
+  "type": "record",
+  "name": "User",
+  "fields": [
+    {
+      "name": "name",
+      "type": {
+        "type": "record",
+        "name": "Name",
+        "fields": [
+          {
+            "name": "first",
+            "type": "string"
+          },
+          {
+            "name": "last",
+            "type": "string"
+          }
+        ]
+      }
+    },
+    {
+      "name": "recursive",
+      "type": "Name",
+      "default": {
+        "first": null
+      }
+    }
+  ]
+}
diff --git a/parser/src/test/resources/schemas/badDefaultValues/BadUnion.avsc b/parser/src/test/resources/schemas/badDefaultValues/BadUnion.avsc
@@ -0,0 +1,14 @@
+{
+  "type": "record",
+  "name": "User",
+  "fields": [
+    {
+      "name": "name",
+      "type": [
+        "null",
+        "User"
+      ],
+      "default": "shouldBeNull"
+    }
+  ]
+}