From f249b18d937e20c3457f010658e0e7e477724033 Mon Sep 17 00:00:00 2001 From: Niels Basjes Date: Thu, 1 Dec 2016 13:44:57 +0100 Subject: [PATCH 1/2] AVRO-1967: Java: Fix NPE when calling getXyzBuilder on instance where the xyz is null --- CHANGES.txt | 3 + .../specific/templates/java/classic/record.vm | 16 +++- .../specific/TestSpecificBuilderTree.java | 77 +++++++++++++++++++ .../avro/examples/baseball/Player.java | 22 ++++-- .../src/test/compiler/output/Player.java | 22 ++++-- 5 files changed, 122 insertions(+), 18 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index fa11bb82130..d5341e8905c 100644 --- 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/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 c333dd076d5..230159f3937 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 @@ -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(other); + } else { + return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder(other); + } } /** @@ -279,10 +287,10 @@ 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())})) { + if (other != null && isValidValue(fields()[$field.pos()], other.${this.mangle($field.name(), $schema.isError())})) { this.${this.mangle($field.name(), $schema.isError())} = data().deepCopy(fields()[$field.pos()].schema(), other.${this.mangle($field.name(), $schema.isError())}); fieldSetFlags()[$field.pos()] = true; } 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 87b9e6f6e94..6a698336989 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 @@ -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 index da58e20c88e..161ba63d3fb 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 @@ -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(other); + } else { + return new avro.examples.baseball.Player.Builder(other); + } } /** @@ -240,20 +248,20 @@ private Builder(avro.examples.baseball.Player.Builder other) { * @param other The existing instance to copy. */ private Builder(avro.examples.baseball.Player other) { - super(SCHEMA$); - if (isValidValue(fields()[0], other.number)) { + super(SCHEMA$); + if (other != null && isValidValue(fields()[0], other.number)) { this.number = data().deepCopy(fields()[0].schema(), other.number); fieldSetFlags()[0] = true; } - if (isValidValue(fields()[1], other.first_name)) { + if (other != null && isValidValue(fields()[1], other.first_name)) { this.first_name = data().deepCopy(fields()[1].schema(), other.first_name); fieldSetFlags()[1] = true; } - if (isValidValue(fields()[2], other.last_name)) { + if (other != null && isValidValue(fields()[2], other.last_name)) { this.last_name = data().deepCopy(fields()[2].schema(), other.last_name); fieldSetFlags()[2] = true; } - if (isValidValue(fields()[3], other.position)) { + if (other != null && isValidValue(fields()[3], other.position)) { this.position = data().deepCopy(fields()[3].schema(), other.position); fieldSetFlags()[3] = true; } diff --git a/lang/java/tools/src/test/compiler/output/Player.java b/lang/java/tools/src/test/compiler/output/Player.java index 0a0b88203dc..9f9a746c8ff 100644 --- 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(other); + } else { + return new avro.examples.baseball.Player.Builder(other); + } } /** @@ -240,20 +248,20 @@ private Builder(avro.examples.baseball.Player.Builder other) { * @param other The existing instance to copy. */ private Builder(avro.examples.baseball.Player other) { - super(SCHEMA$); - if (isValidValue(fields()[0], other.number)) { + super(SCHEMA$); + if (other != null && isValidValue(fields()[0], other.number)) { this.number = data().deepCopy(fields()[0].schema(), other.number); fieldSetFlags()[0] = true; } - if (isValidValue(fields()[1], other.first_name)) { + if (other != null && isValidValue(fields()[1], other.first_name)) { this.first_name = data().deepCopy(fields()[1].schema(), other.first_name); fieldSetFlags()[1] = true; } - if (isValidValue(fields()[2], other.last_name)) { + if (other != null && isValidValue(fields()[2], other.last_name)) { this.last_name = data().deepCopy(fields()[2].schema(), other.last_name); fieldSetFlags()[2] = true; } - if (isValidValue(fields()[3], other.position)) { + if (other != null && isValidValue(fields()[3], other.position)) { this.position = data().deepCopy(fields()[3].schema(), other.position); fieldSetFlags()[3] = true; } From 341e338273befb4943b42a33578eb0dc09e00224 Mon Sep 17 00:00:00 2001 From: Niels Basjes Date: Thu, 1 Dec 2016 22:05:14 +0100 Subject: [PATCH 2/2] AVRO-1967: Simplified the fix --- .../compiler/specific/templates/java/classic/record.vm | 4 ++-- .../output-string/avro/examples/baseball/Player.java | 10 +++++----- lang/java/tools/src/test/compiler/output/Player.java | 10 +++++----- 3 files changed, 12 insertions(+), 12 deletions(-) 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 230159f3937..ccec4b60c1d 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 @@ -235,7 +235,7 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or */ 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) { if (other == null) { - return new #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(); } else { return new #if ($schema.getNamespace())$schema.getNamespace().#end${this.mangle($schema.getName())}.Builder(other); } @@ -290,7 +290,7 @@ public class ${this.mangle($schema.getName())}#if ($schema.isError()) extends or #if ($schema.isError()) super(other)#else super(SCHEMA$)#end; #foreach ($field in $schema.getFields()) - if (other != null && isValidValue(fields()[$field.pos()], other.${this.mangle($field.name(), $schema.isError())})) { + if (isValidValue(fields()[$field.pos()], other.${this.mangle($field.name(), $schema.isError())})) { this.${this.mangle($field.name(), $schema.isError())} = data().deepCopy(fields()[$field.pos()].schema(), other.${this.mangle($field.name(), $schema.isError())}); fieldSetFlags()[$field.pos()] = true; } 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 161ba63d3fb..4dff5ef5053 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 @@ -196,7 +196,7 @@ public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.bas */ public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.baseball.Player other) { if (other == null) { - return new avro.examples.baseball.Player.Builder(other); + return new avro.examples.baseball.Player.Builder(); } else { return new avro.examples.baseball.Player.Builder(other); } @@ -249,19 +249,19 @@ private Builder(avro.examples.baseball.Player.Builder other) { */ private Builder(avro.examples.baseball.Player other) { super(SCHEMA$); - if (other != null && isValidValue(fields()[0], other.number)) { + if (isValidValue(fields()[0], other.number)) { this.number = data().deepCopy(fields()[0].schema(), other.number); fieldSetFlags()[0] = true; } - if (other != null && isValidValue(fields()[1], other.first_name)) { + if (isValidValue(fields()[1], other.first_name)) { this.first_name = data().deepCopy(fields()[1].schema(), other.first_name); fieldSetFlags()[1] = true; } - if (other != null && isValidValue(fields()[2], other.last_name)) { + if (isValidValue(fields()[2], other.last_name)) { this.last_name = data().deepCopy(fields()[2].schema(), other.last_name); fieldSetFlags()[2] = true; } - if (other != null && isValidValue(fields()[3], other.position)) { + if (isValidValue(fields()[3], other.position)) { this.position = data().deepCopy(fields()[3].schema(), other.position); fieldSetFlags()[3] = true; } diff --git a/lang/java/tools/src/test/compiler/output/Player.java b/lang/java/tools/src/test/compiler/output/Player.java index 9f9a746c8ff..26fcbc0d559 100644 --- a/lang/java/tools/src/test/compiler/output/Player.java +++ b/lang/java/tools/src/test/compiler/output/Player.java @@ -196,7 +196,7 @@ public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.bas */ public static avro.examples.baseball.Player.Builder newBuilder(avro.examples.baseball.Player other) { if (other == null) { - return new avro.examples.baseball.Player.Builder(other); + return new avro.examples.baseball.Player.Builder(); } else { return new avro.examples.baseball.Player.Builder(other); } @@ -249,19 +249,19 @@ private Builder(avro.examples.baseball.Player.Builder other) { */ private Builder(avro.examples.baseball.Player other) { super(SCHEMA$); - if (other != null && isValidValue(fields()[0], other.number)) { + if (isValidValue(fields()[0], other.number)) { this.number = data().deepCopy(fields()[0].schema(), other.number); fieldSetFlags()[0] = true; } - if (other != null && isValidValue(fields()[1], other.first_name)) { + if (isValidValue(fields()[1], other.first_name)) { this.first_name = data().deepCopy(fields()[1].schema(), other.first_name); fieldSetFlags()[1] = true; } - if (other != null && isValidValue(fields()[2], other.last_name)) { + if (isValidValue(fields()[2], other.last_name)) { this.last_name = data().deepCopy(fields()[2].schema(), other.last_name); fieldSetFlags()[2] = true; } - if (other != null && isValidValue(fields()[3], other.position)) { + if (isValidValue(fields()[3], other.position)) { this.position = data().deepCopy(fields()[3].schema(), other.position); fieldSetFlags()[3] = true; }