Repository: avro Updated Branches: refs/heads/branch-1.8 d35724e7f -> e39170ef8
AVRO-1967: Java: Fix NPE when calling getXyzBuilder on instance where the xyz is null Project: http://git-wip-us.apache.org/repos/asf/avro/repo Commit: http://git-wip-us.apache.org/repos/asf/avro/commit/e39170ef Tree: http://git-wip-us.apache.org/repos/asf/avro/tree/e39170ef Diff: http://git-wip-us.apache.org/repos/asf/avro/diff/e39170ef Branch: refs/heads/branch-1.8 Commit: e39170ef83fb06d09b2ad12afc9c6a1b429dfdf9 Parents: 8bed58c Author: Niels Basjes <[email protected]> Authored: Thu Dec 1 13:44:57 2016 +0100 Committer: Niels Basjes <[email protected]> Committed: Fri Dec 22 23:29:48 2017 +0100 ---------------------------------------------------------------------- CHANGES.txt | 3 + .../specific/templates/java/classic/record.vm | 14 +++- .../avro/specific/TestSpecificBuilderTree.java | 77 ++++++++++++++++++++ .../avro/examples/baseball/Player.java | 14 +++- .../tools/src/test/compiler/output/Player.java | 14 +++- 5 files changed, 113 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/avro/blob/e39170ef/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index ff4e51f..75c08ac 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -30,6 +30,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.2 (10 April 2017) INCOMPATIBLE CHANGES http://git-wip-us.apache.org/repos/asf/avro/blob/e39170ef/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm ---------------------------------------------------------------------- diff --git a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm index 50c9ecf..de7fd1c 100644 --- a/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm +++ b/lang/java/compiler/src/main/velocity/org/apache/avro/compiler/specific/templates/java/classic/record.vm @@ -231,7 +231,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); + } } /** @@ -240,7 +244,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); + } } /** @@ -289,7 +297,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())})) { http://git-wip-us.apache.org/repos/asf/avro/blob/e39170ef/lang/java/ipc/src/test/java/org/apache/avro/specific/TestSpecificBuilderTree.java ---------------------------------------------------------------------- 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 index f9856e9..1a0a486 100644 --- 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 @@ -28,6 +28,7 @@ import java.util.ArrayList; 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 { @@ -289,4 +290,80 @@ public class TestSpecificBuilderTree { 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); + } + } http://git-wip-us.apache.org/repos/asf/avro/blob/e39170ef/lang/java/tools/src/test/compiler/output-string/avro/examples/baseball/Player.java ---------------------------------------------------------------------- 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 index 77386d2..569f427 100644 --- 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 @@ -192,7 +192,11 @@ public class Player extends org.apache.avro.specific.SpecificRecordBase implemen * @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); + } } /** @@ -201,7 +205,11 @@ public class Player extends org.apache.avro.specific.SpecificRecordBase implemen * @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); + } } /** @@ -250,7 +258,7 @@ public class Player extends org.apache.avro.specific.SpecificRecordBase implemen * @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; http://git-wip-us.apache.org/repos/asf/avro/blob/e39170ef/lang/java/tools/src/test/compiler/output/Player.java ---------------------------------------------------------------------- diff --git a/lang/java/tools/src/test/compiler/output/Player.java b/lang/java/tools/src/test/compiler/output/Player.java index b20d55b..5bbb3b0 100644 --- a/lang/java/tools/src/test/compiler/output/Player.java +++ b/lang/java/tools/src/test/compiler/output/Player.java @@ -192,7 +192,11 @@ public class Player extends org.apache.avro.specific.SpecificRecordBase implemen * @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); + } } /** @@ -201,7 +205,11 @@ public class Player extends org.apache.avro.specific.SpecificRecordBase implemen * @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); + } } /** @@ -250,7 +258,7 @@ public class Player extends org.apache.avro.specific.SpecificRecordBase implemen * @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;
