From cd3c68b255a1a644399445aafe564d96c5e1f149 Mon Sep 17 00:00:00 2001 From: Max Cai Date: Tue, 15 Oct 2013 18:11:56 +0100 Subject: [PATCH] Fix repeated field merging semantics. The public doc states that repeated fields are simply concatenated and doesn't impose a different semantics for packed fields. This CL fixes this for packed fields and adds tests covering all cases. Also fixed a bit of missed null-repeated-field treatments. Change-Id: Ie35277bb1a9f0b8171dc9d07b6adf9b9d3308de2 --- .../java/com/google/protobuf/NanoTest.java | 101 +++++++++++++++++- .../compiler/javanano/javanano_enum_field.cc | 28 +++-- .../javanano/javanano_message_field.cc | 22 ++-- .../javanano/javanano_primitive_field.cc | 31 +++--- .../unittest_repeated_merge_nano.proto | 47 ++++++++ 5 files changed, 192 insertions(+), 37 deletions(-) create mode 100644 src/google/protobuf/unittest_repeated_merge_nano.proto diff --git a/java/src/test/java/com/google/protobuf/NanoTest.java b/java/src/test/java/com/google/protobuf/NanoTest.java index dbc0dddf7..d4e9dfcbe 100644 --- a/java/src/test/java/com/google/protobuf/NanoTest.java +++ b/java/src/test/java/com/google/protobuf/NanoTest.java @@ -49,6 +49,7 @@ import com.google.protobuf.nano.NanoHasOuterClass.TestAllTypesNanoHas; import com.google.protobuf.nano.NanoOuterClass; import com.google.protobuf.nano.NanoOuterClass.TestAllTypesNano; import com.google.protobuf.nano.NanoReferenceTypes; +import com.google.protobuf.nano.TestRepeatedMergeNano; import com.google.protobuf.nano.UnittestImportNano; import com.google.protobuf.nano.UnittestMultipleNano; import com.google.protobuf.nano.UnittestRecursiveNano.RecursiveMessageNano; @@ -2685,33 +2686,125 @@ public class NanoTest extends TestCase { MessageNano.toByteArray(message); // should not NPE message.toString(); // should not NPE - message = new TestAllTypesNano(); message.repeatedNestedEnum = null; MessageNano.toByteArray(message); // should not NPE message.toString(); // should not NPE - message = new TestAllTypesNano(); message.repeatedBytes = null; MessageNano.toByteArray(message); // should not NPE message.toString(); // should not NPE - message = new TestAllTypesNano(); message.repeatedNestedMessage = null; MessageNano.toByteArray(message); // should not NPE message.toString(); // should not NPE + message.repeatedPackedInt32 = null; + MessageNano.toByteArray(message); // should not NPE + message.toString(); // should not NPE + + message.repeatedPackedNestedEnum = null; + MessageNano.toByteArray(message); // should not NPE + message.toString(); // should not NPE + // Create a second message to merge into message. TestAllTypesNano secondMessage = new TestAllTypesNano(); + secondMessage.repeatedInt32 = new int[] {1, 2, 3}; + secondMessage.repeatedNestedEnum = new int[] { + TestAllTypesNano.FOO, TestAllTypesNano.BAR + }; + secondMessage.repeatedBytes = new byte[][] {{1, 2}, {3, 4}}; TestAllTypesNano.NestedMessage nested = new TestAllTypesNano.NestedMessage(); nested.bb = 55; secondMessage.repeatedNestedMessage = - new TestAllTypesNano.NestedMessage[] { nested }; + new TestAllTypesNano.NestedMessage[] {nested}; + secondMessage.repeatedPackedInt32 = new int[] {1, 2, 3}; + secondMessage.repeatedPackedNestedEnum = new int[] { + TestAllTypesNano.FOO, TestAllTypesNano.BAR + }; // Should not NPE message.mergeFrom(CodedInputByteBufferNano.newInstance( MessageNano.toByteArray(secondMessage))); + assertEquals(3, message.repeatedInt32.length); + assertEquals(3, message.repeatedInt32[2]); + assertEquals(2, message.repeatedNestedEnum.length); + assertEquals(TestAllTypesNano.FOO, message.repeatedNestedEnum[0]); + assertEquals(2, message.repeatedBytes.length); + assertEquals(4, message.repeatedBytes[1][1]); + assertEquals(1, message.repeatedNestedMessage.length); assertEquals(55, message.repeatedNestedMessage[0].bb); + assertEquals(3, message.repeatedPackedInt32.length); + assertEquals(2, message.repeatedPackedInt32[1]); + assertEquals(2, message.repeatedPackedNestedEnum.length); + assertEquals(TestAllTypesNano.BAR, message.repeatedPackedNestedEnum[1]); + } + + public void testRepeatedMerge() throws Exception { + // Check that merging repeated fields cause the arrays to expand with + // new data. + TestAllTypesNano first = new TestAllTypesNano(); + first.repeatedInt32 = new int[] {1, 2, 3}; + TestAllTypesNano second = new TestAllTypesNano(); + second.repeatedInt32 = new int[] {4, 5}; + MessageNano.mergeFrom(first, MessageNano.toByteArray(second)); + assertEquals(5, first.repeatedInt32.length); + assertEquals(1, first.repeatedInt32[0]); + assertEquals(4, first.repeatedInt32[3]); + + first = new TestAllTypesNano(); + first.repeatedNestedEnum = new int[] {TestAllTypesNano.BAR}; + second = new TestAllTypesNano(); + second.repeatedNestedEnum = new int[] {TestAllTypesNano.FOO}; + MessageNano.mergeFrom(first, MessageNano.toByteArray(second)); + assertEquals(2, first.repeatedNestedEnum.length); + assertEquals(TestAllTypesNano.BAR, first.repeatedNestedEnum[0]); + assertEquals(TestAllTypesNano.FOO, first.repeatedNestedEnum[1]); + + first = new TestAllTypesNano(); + first.repeatedNestedMessage = new TestAllTypesNano.NestedMessage[] { + new TestAllTypesNano.NestedMessage() + }; + first.repeatedNestedMessage[0].bb = 3; + second = new TestAllTypesNano(); + second.repeatedNestedMessage = new TestAllTypesNano.NestedMessage[] { + new TestAllTypesNano.NestedMessage() + }; + second.repeatedNestedMessage[0].bb = 5; + MessageNano.mergeFrom(first, MessageNano.toByteArray(second)); + assertEquals(2, first.repeatedNestedMessage.length); + assertEquals(3, first.repeatedNestedMessage[0].bb); + assertEquals(5, first.repeatedNestedMessage[1].bb); + + first = new TestAllTypesNano(); + first.repeatedPackedSfixed64 = new long[] {-1, -2, -3}; + second = new TestAllTypesNano(); + second.repeatedPackedSfixed64 = new long[] {-4, -5}; + MessageNano.mergeFrom(first, MessageNano.toByteArray(second)); + assertEquals(5, first.repeatedPackedSfixed64.length); + assertEquals(-1, first.repeatedPackedSfixed64[0]); + assertEquals(-4, first.repeatedPackedSfixed64[3]); + + first = new TestAllTypesNano(); + first.repeatedPackedNestedEnum = new int[] {TestAllTypesNano.BAR}; + second = new TestAllTypesNano(); + second.repeatedPackedNestedEnum = new int[] {TestAllTypesNano.FOO}; + MessageNano.mergeFrom(first, MessageNano.toByteArray(second)); + assertEquals(2, first.repeatedPackedNestedEnum.length); + assertEquals(TestAllTypesNano.BAR, first.repeatedPackedNestedEnum[0]); + assertEquals(TestAllTypesNano.FOO, first.repeatedPackedNestedEnum[1]); + + // Now test repeated merging in a nested scope + TestRepeatedMergeNano firstContainer = new TestRepeatedMergeNano(); + firstContainer.contained = new TestAllTypesNano(); + firstContainer.contained.repeatedInt32 = new int[] {10, 20}; + TestRepeatedMergeNano secondContainer = new TestRepeatedMergeNano(); + secondContainer.contained = new TestAllTypesNano(); + secondContainer.contained.repeatedInt32 = new int[] {30}; + MessageNano.mergeFrom(firstContainer, MessageNano.toByteArray(secondContainer)); + assertEquals(3, firstContainer.contained.repeatedInt32.length); + assertEquals(20, firstContainer.contained.repeatedInt32[1]); + assertEquals(30, firstContainer.contained.repeatedInt32[2]); } private void assertHasWireData(MessageNano message, boolean expected) { diff --git a/src/google/protobuf/compiler/javanano/javanano_enum_field.cc b/src/google/protobuf/compiler/javanano/javanano_enum_field.cc index cdb3d0979..fe67d76b3 100644 --- a/src/google/protobuf/compiler/javanano/javanano_enum_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_enum_field.cc @@ -269,24 +269,32 @@ GenerateMergingCode(io::Printer* printer) const { " arrayLength++;\n" "}\n" "input.rewindToPosition(startPos);\n" - "this.$name$ = new $type$[arrayLength];\n" - "for (int i = 0; i < arrayLength; i++) {\n" - " this.$name$[i] = input.readInt32();\n" + "int i = this.$name$ == null ? 0 : this.$name$.length;\n" + "int[] newArray = new int[i + arrayLength];\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" "}\n" + "for (; i < newArray.length; i++) {\n" + " newArray[i] = input.readInt32();\n" + "}\n" + "this.$name$ = newArray;\n" "input.popLimit(limit);\n"); } else { printer->Print(variables_, - "int arrayLength = com.google.protobuf.nano.WireFormatNano.getRepeatedFieldArrayLength(input, $tag$);\n" - "int i = this.$name$.length;\n" + "int arrayLength = com.google.protobuf.nano.WireFormatNano.\n" + " getRepeatedFieldArrayLength(input, $tag$);\n" + "int i = this.$name$ == null ? 0 : this.$name$.length;\n" "int[] newArray = new int[i + arrayLength];\n" - "System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "this.$name$ = newArray;\n" - "for (; i < this.$name$.length - 1; i++) {\n" - " this.$name$[i] = input.readInt32();\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "}\n" + "for (; i < newArray.length - 1; i++) {\n" + " newArray[i] = input.readInt32();\n" " input.readTag();\n" "}\n" "// Last one without readTag.\n" - "this.$name$[i] = input.readInt32();\n"); + "newArray[i] = input.readInt32();\n" + "this.$name$ = newArray;\n"); } } diff --git a/src/google/protobuf/compiler/javanano/javanano_message_field.cc b/src/google/protobuf/compiler/javanano/javanano_message_field.cc index 04f1c1412..027428f0e 100644 --- a/src/google/protobuf/compiler/javanano/javanano_message_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_message_field.cc @@ -235,34 +235,36 @@ GenerateMergingCode(io::Printer* printer) const { " .getRepeatedFieldArrayLength(input, $tag$);\n" "int i = this.$name$ == null ? 0 : this.$name$.length;\n" "$type$[] newArray = new $type$[i + arrayLength];\n" - "if (this.$name$ != null) {\n" - " System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" "}\n" - "this.$name$ = newArray;\n" - "for (; i < this.$name$.length - 1; i++) {\n" - " this.$name$[i] = new $type$();\n"); + "for (; i < newArray.length - 1; i++) {\n" + " newArray[i] = new $type$();\n"); if (descriptor_->type() == FieldDescriptor::TYPE_GROUP) { printer->Print(variables_, - " input.readGroup(this.$name$[i], $number$);\n"); + " input.readGroup(newArray[i], $number$);\n"); } else { printer->Print(variables_, - " input.readMessage(this.$name$[i]);\n"); + " input.readMessage(newArray[i]);\n"); } printer->Print(variables_, " input.readTag();\n" "}\n" "// Last one without readTag.\n" - "this.$name$[i] = new $type$();\n"); + "newArray[i] = new $type$();\n"); if (descriptor_->type() == FieldDescriptor::TYPE_GROUP) { printer->Print(variables_, - "input.readGroup(this.$name$[i], $number$);\n"); + "input.readGroup(newArray[i], $number$);\n"); } else { printer->Print(variables_, - "input.readMessage(this.$name$[i]);\n"); + "input.readMessage(newArray[i]);\n"); } + + printer->Print(variables_, + "this.$name$ = newArray;\n"); } void RepeatedMessageFieldGenerator:: diff --git a/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc b/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc index 8097be8bd..0562a6e08 100644 --- a/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc +++ b/src/google/protobuf/compiler/javanano/javanano_primitive_field.cc @@ -519,35 +519,40 @@ GenerateMergingCode(io::Printer* printer) const { " arrayLength++;\n" "}\n" "input.rewindToPosition(startPos);\n" - "this.$name$ = new $type$[arrayLength];\n" - "for (int i = 0; i < arrayLength; i++) {\n" - " this.$name$[i] = input.read$capitalized_type$();\n" + "int i = this.$name$ == null ? 0 : this.$name$.length;\n" + "$type$[] newArray = new $type$[i + arrayLength];\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" "}\n" + "for (; i < newArray.length; i++) {\n" + " newArray[i] = input.read$capitalized_type$();\n" + "}\n" + "this.$name$ = newArray;\n" "input.popLimit(limit);\n"); } else { printer->Print(variables_, "int arrayLength = com.google.protobuf.nano.WireFormatNano\n" " .getRepeatedFieldArrayLength(input, $tag$);\n" - "int i = this.$name$.length;\n"); + "int i = this.$name$ == null ? 0 : this.$name$.length;\n"); if (GetJavaType(descriptor_) == JAVATYPE_BYTES) { printer->Print(variables_, - "byte[][] newArray = new byte[i + arrayLength][];\n" - "System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "this.$name$ = newArray;\n"); + "byte[][] newArray = new byte[i + arrayLength][];\n"); } else { printer->Print(variables_, - "$type$[] newArray = new $type$[i + arrayLength];\n" - "System.arraycopy(this.$name$, 0, newArray, 0, i);\n" - "this.$name$ = newArray;\n"); + "$type$[] newArray = new $type$[i + arrayLength];\n"); } printer->Print(variables_, - "for (; i < this.$name$.length - 1; i++) {\n" - " this.$name$[i] = input.read$capitalized_type$();\n" + "if (i != 0) {\n" + " java.lang.System.arraycopy(this.$name$, 0, newArray, 0, i);\n" + "}\n" + "for (; i < newArray.length - 1; i++) {\n" + " newArray[i] = input.read$capitalized_type$();\n" " input.readTag();\n" "}\n" "// Last one without readTag.\n" - "this.$name$[i] = input.read$capitalized_type$();\n"); + "newArray[i] = input.read$capitalized_type$();\n" + "this.$name$ = newArray;\n"); } } diff --git a/src/google/protobuf/unittest_repeated_merge_nano.proto b/src/google/protobuf/unittest_repeated_merge_nano.proto new file mode 100644 index 000000000..4f03224b5 --- /dev/null +++ b/src/google/protobuf/unittest_repeated_merge_nano.proto @@ -0,0 +1,47 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2008 Google Inc. All rights reserved. +// http://code.google.com/p/protobuf/ +// +// Redistribution and use in source and binary forms, with or without +// modification, are permitted provided that the following conditions are +// met: +// +// * Redistributions of source code must retain the above copyright +// notice, this list of conditions and the following disclaimer. +// * Redistributions in binary form must reproduce the above +// copyright notice, this list of conditions and the following disclaimer +// in the documentation and/or other materials provided with the +// distribution. +// * Neither the name of Google Inc. nor the names of its +// contributors may be used to endorse or promote products derived from +// this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +// Author: maxtroy@google.com (Max Cai) + +package protobuf_unittest; + +import "google/protobuf/unittest_nano.proto"; + +option java_package = "com.google.protobuf.nano"; +option java_multiple_files = true; + +// A container message for testing the merging of repeated fields at a +// nested level. Other tests will be done using the repeated fields in +// TestAllTypesNano. +message TestRepeatedMergeNano { + + optional TestAllTypesNano contained = 1; + +}