From 52d9339997bfa452f0b30353bb01f90a0af30ba9 Mon Sep 17 00:00:00 2001
From: Oscar Westra van Holthe - Kind <opwvhk@users.noreply.github.com>
Date: Tue, 15 Aug 2023 21:19:27 +0200
Subject: [PATCH] AVRO-2771: Refactor custom codable check (#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.
---
 lang/java/compiler/pom.xml                    |  1 +
 .../compiler/specific/SpecificCompiler.java   |  6 +++--
 .../avro/specific/TestGeneratedCode.java      | 26 +++++++++++++++++++
 .../regression_error_field_in_record.avsc     | 22 ++++++++++++++++
 4 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 lang/java/compiler/src/test/resources/regression_error_field_in_record.avsc

diff --git a/lang/java/compiler/pom.xml b/lang/java/compiler/pom.xml
index 53719816387..fe7fbe404bf 100644
--- 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
index 1f248ed6daf..e98a50b79a0 100644
--- 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
index 0e61aa7cc15..9a334a45ab0 100644
--- 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
new file mode 100644
index 00000000000..e2fdcb9ad93
--- /dev/null
+++ 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
+    } ]
+}