AVRO-1967: Java: Fix NPE when calling getXyzBuilder on instance where…

… the xyz is null
mkw · Dec 17, 2016 · 152fa09 · 152fa09
1 parent a53a4fd
commit 152fa09
Show file tree

Hide file tree

Showing 5 changed files with 113 additions and 9 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -106,6 +106,9 @@ Trunk (not yet released)
 
     AVRO-1966: Java: Fix NPE When copying builder with nullable record. (Niels Basjes)
 
+    AVRO-1967: Java: Fix NPE when calling getXyzBuilder on instance where the xyz is null
+    (Niels Basjes)
+
 Avro 1.8.1 (14 May 2016)
 
   INCOMPATIBLE CHANGES

diff --git a/...iler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm b/...iler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm
@@ -221,7 +221,11 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or
    * @return A new ${this.mangle($schema.getName())} RecordBuilder
    */
   public static #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder newBuilder(#if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder other) {
-    return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder(other);
+    if (other == null) {
+      return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder();
+    } else {
+      return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder(other);
+    }
   }
 
   /**
@@ -230,7 +234,11 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or
    * @return A new ${this.mangle($schema.getName())} RecordBuilder
    */
   public static #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder newBuilder(#if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())} other) {
-    return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder(other);
+    if (other == null) {
+      return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder();
+    } else {
+      return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder(other);
+    }
   }
 
   /**
@@ -279,7 +287,7 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or
      * @param other The existing instance to copy.
      */
     private Builder(#if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())} other) {
-      #if ($schema.isError())super(other)#else
+#if ($schema.isError())      super(other)#else
       super(SCHEMA$)#end;
 #foreach ($field in $schema.getFields())
       if (isValidValue(fields()[$field.pos()], other.${this.mangle($field.name(), $schema.isError())})) {

diff --git a/lang/java/ipc/src/test/java/org/apache/avro/specific/TestSpecificBuilderTree.java b/lang/java/ipc/src/test/java/org/apache/avro/specific/TestSpecificBuilderTree.java
@@ -27,6 +27,7 @@
 import static org.apache.avro.test.nullable.Nullable.*;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class TestSpecificBuilderTree {
@@ -283,4 +284,80 @@ public void copyBuilderWithNullables() {
     builderCopy.getNullableRecordBuilder();
   }
 
+  @Test
+  public void copyBuilderWithNullablesAndSetToNull() {
+    // Create builder with all values default to null, yet unset.
+    RecordWithNullables.Builder builder = RecordWithNullables.newBuilder();
+
+    // Ensure all values have not been set
+    assertFalse(builder.hasNullableRecordBuilder());
+    assertFalse(builder.hasNullableRecord());
+    assertFalse(builder.hasNullableString());
+    assertFalse(builder.hasNullableLong  ());
+    assertFalse(builder.hasNullableInt   ());
+    assertFalse(builder.hasNullableMap   ());
+    assertFalse(builder.hasNullableArray ());
+
+    // Set all values to null
+    builder.setNullableRecordBuilder(null);
+    builder.setNullableRecord(null);
+    builder.setNullableString(null);
+    builder.setNullableLong  (null);
+    builder.setNullableInt   (null);
+    builder.setNullableMap   (null);
+    builder.setNullableArray (null);
+
+    // A Builder remains False because it is null
+    assertFalse(builder.hasNullableRecordBuilder());
+
+    // Ensure all values have been set
+    assertTrue(builder.hasNullableRecord());
+    assertTrue(builder.hasNullableString());
+    assertTrue(builder.hasNullableLong  ());
+    assertTrue(builder.hasNullableInt   ());
+    assertTrue(builder.hasNullableMap   ());
+    assertTrue(builder.hasNullableArray ());
+
+    // Implicitly create a builder instance and clear the actual value.
+    builder.getNullableRecordBuilder();
+    assertTrue(builder.hasNullableRecordBuilder());
+    assertFalse(builder.hasNullableRecord());
+
+    // Create a copy of this builder.
+    RecordWithNullables.Builder builderCopy = RecordWithNullables.newBuilder(builder);
+
+    // Ensure all values are still the same
+    assertTrue(builder.hasNullableRecordBuilder());
+    assertFalse(builder.hasNullableRecord());
+    assertTrue(builder.hasNullableString());
+    assertTrue(builder.hasNullableLong  ());
+    assertTrue(builder.hasNullableInt   ());
+    assertTrue(builder.hasNullableMap   ());
+    assertTrue(builder.hasNullableArray ());
+  }
+
+  @Test
+  public void getBuilderForRecordWithNullRecord() {
+    // Create a record with all nullable fields set to the default value : null
+    RecordWithNullables recordWithNullables = RecordWithNullables.newBuilder().build();
+
+    // Now create a Builder using this record as the base
+    RecordWithNullables.Builder builder = RecordWithNullables.newBuilder(recordWithNullables);
+
+    // In the past this caused an NPE
+    builder.getNullableRecordBuilder();
+  }
+
+  @Test
+  public void getBuilderForNullRecord() {
+    // In the past this caused an NPE
+    RecordWithNullables.newBuilder((RecordWithNullables)null);
+  }
+
+  @Test
+  public void getBuilderForNullBuilder() {
+    // In the past this caused an NPE
+    RecordWithNullables.newBuilder((RecordWithNullables.Builder)null);
+  }
+
 }
diff --git a/lang/java/tools/src/test/compiler/output-string/avro/examples/baseball/Player.java b/lang/java/tools/src/test/compiler/output-string/avro/examples/baseball/Player.java
@@ -182,7 +182,11 @@ public static avro.examples.baseball.Player.Builder newBuilder() {
    * @return A new Player RecordBuilder
    */
   public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.baseball.Player.Builder other) {
-    return new avro.examples.baseball.Player.Builder(other);
+    if (other == null) {
+      return new avro.examples.baseball.Player.Builder();
+    } else {
+      return new avro.examples.baseball.Player.Builder(other);
+    }
   }
 
   /**
@@ -191,7 +195,11 @@ public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.bas
    * @return A new Player RecordBuilder
    */
   public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.baseball.Player other) {
-    return new avro.examples.baseball.Player.Builder(other);
+    if (other == null) {
+      return new avro.examples.baseball.Player.Builder();
+    } else {
+      return new avro.examples.baseball.Player.Builder(other);
+    }
   }
 
   /**
@@ -240,7 +248,7 @@ private Builder(avro.examples.baseball.Player.Builder other) {
      * @param other The existing instance to copy.
      */
     private Builder(avro.examples.baseball.Player other) {
-            super(SCHEMA$);
+      super(SCHEMA$);
       if (isValidValue(fields()[0], other.number)) {
         this.number = data().deepCopy(fields()[0].schema(), other.number);
         fieldSetFlags()[0] = true;

diff --git a/lang/java/tools/src/test/compiler/output/Player.java b/lang/java/tools/src/test/compiler/output/Player.java
@@ -182,7 +182,11 @@ public static avro.examples.baseball.Player.Builder newBuilder() {
    * @return A new Player RecordBuilder
    */
   public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.baseball.Player.Builder other) {
-    return new avro.examples.baseball.Player.Builder(other);
+    if (other == null) {
+      return new avro.examples.baseball.Player.Builder();
+    } else {
+      return new avro.examples.baseball.Player.Builder(other);
+    }
   }
 
   /**
@@ -191,7 +195,11 @@ public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.bas
    * @return A new Player RecordBuilder
    */
   public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.baseball.Player other) {
-    return new avro.examples.baseball.Player.Builder(other);
+    if (other == null) {
+      return new avro.examples.baseball.Player.Builder();
+    } else {
+      return new avro.examples.baseball.Player.Builder(other);
+    }
   }
 
   /**
@@ -240,7 +248,7 @@ private Builder(avro.examples.baseball.Player.Builder other) {
      * @param other The existing instance to copy.
      */
     private Builder(avro.examples.baseball.Player other) {
-            super(SCHEMA$);
+      super(SCHEMA$);
       if (isValidValue(fields()[0], other.number)) {
         this.number = data().deepCopy(fields()[0].schema(), other.number);
         fieldSetFlags()[0] = true;