AVRO-2771: Refactor custom codable check (apache#1720)

* AVRO-2771: Refactor custom codable check Moved the isError check for the "custom codable" check to fix a backwards compatibility issue between versions 1.8.x & 1.9.x.
RanbirK · May 13, 2024 · 07d1120 · 07d1120
1 parent 35d9009
commit 07d1120
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 2 deletions.
diff --git a/lang/java/compiler/pom.xml b/lang/java/compiler/pom.xml
@@ -137,6 +137,7 @@
                 <argument>org.apache.avro.compiler.specific.SchemaTask</argument>
                 <argument>${project.basedir}/src/test/resources/full_record_v1.avsc</argument>
                 <argument>${project.basedir}/src/test/resources/full_record_v2.avsc</argument>
+                <argument>${project.basedir}/src/test/resources/regression_error_field_in_record.avsc</argument>
                 <argument>${project.basedir}/target/generated-test-sources/javacc</argument>
               </arguments>
             </configuration>

diff --git a/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java b/lang/java/compiler/src/main/java/org/apache/avro/compiler/specific/SpecificCompiler.java
@@ -946,19 +946,21 @@ public int getNonNullIndex(Schema s) {
    * record.vm can handle the schema being presented.
    */
   public boolean isCustomCodable(Schema schema) {
-    if (schema.isError())
-      return false;
     return isCustomCodable(schema, new HashSet<>());
   }
 
   private boolean isCustomCodable(Schema schema, Set<Schema> seen) {
     if (!seen.add(schema))
+      // Recursive call: assume custom codable until a caller on the call stack proves
+      // otherwise.
       return true;
     if (schema.getLogicalType() != null)
       return false;
     boolean result = true;
     switch (schema.getType()) {
     case RECORD:
+      if (schema.isError())
+        return false;
       for (Schema.Field f : schema.getFields())
         result &= isCustomCodable(f.schema(), seen);
       break;

diff --git a/lang/java/compiler/src/test/java/org/apache/avro/specific/TestGeneratedCode.java b/lang/java/compiler/src/test/java/org/apache/avro/specific/TestGeneratedCode.java
@@ -30,6 +30,8 @@
 import org.apache.avro.io.DecoderFactory;
 import org.apache.avro.io.DatumReader;
 import org.apache.avro.io.DatumWriter;
+import org.apache.avro.specific.test.RecordWithErrorField;
+import org.apache.avro.specific.test.TestError;
 import org.apache.avro.util.Utf8;
 
 import org.junit.Assert;
@@ -92,4 +94,28 @@ void withSchemaMigration() throws IOException {
     FullRecordV1 expected = new FullRecordV1(true, 87231, 731L, 54.2832F, 38.0, null, "Hello, world!");
     Assert.assertEquals(expected, dst);
   }
+
+  @Test
+  public void withErrorField() throws IOException {
+    TestError srcError = TestError.newBuilder().setMessage$("Oops").build();
+    RecordWithErrorField src = new RecordWithErrorField("Hi there", srcError);
+    Assert.assertFalse("Test schema with error field cannot allow for custom coders.",
+        ((SpecificRecordBase) src).hasCustomCoders());
+    Schema schema = RecordWithErrorField.getClassSchema();
+
+    ByteArrayOutputStream out = new ByteArrayOutputStream(1024);
+    Encoder e = EncoderFactory.get().directBinaryEncoder(out, null);
+    DatumWriter<RecordWithErrorField> w = (DatumWriter<RecordWithErrorField>) MODEL.createDatumWriter(schema);
+    w.write(src, e);
+    e.flush();
+
+    ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+    Decoder d = DecoderFactory.get().directBinaryDecoder(in, null);
+    DatumReader<RecordWithErrorField> r = (DatumReader<RecordWithErrorField>) MODEL.createDatumReader(schema);
+    RecordWithErrorField dst = r.read(null, d);
+
+    TestError expectedError = TestError.newBuilder().setMessage$("Oops").build();
+    RecordWithErrorField expected = new RecordWithErrorField("Hi there", expectedError);
+    Assert.assertEquals(expected, dst);
+  }
 }
diff --git a/lang/java/compiler/src/test/resources/regression_error_field_in_record.avsc b/lang/java/compiler/src/test/resources/regression_error_field_in_record.avsc
@@ -0,0 +1,22 @@
+{
+    "type" : "record",
+    "name" : "RecordWithErrorField",
+    "doc" : "With custom coders in Avro 1.9, previously successful records with error fields now fail to compile.",
+    "namespace" : "org.apache.avro.specific.test",
+    "fields" : [ {
+        "name" : "s",
+        "type" : [ "null", "string" ],
+        "default" : null
+    }, {
+        "name": "e",
+        "type": [ "null", {
+            "type" : "error",
+            "name" : "TestError",
+            "fields" : [ {
+                "name" : "message",
+                "type" : "string"
+            } ]
+        } ],
+        "default": null
+    } ]
+}