From b3c24e0ed285b9457560040fe49cb4f9616e721b Mon Sep 17 00:00:00 2001
From: Nicholas Seckar <seckar@google.com>
Date: Tue, 24 Sep 2013 20:52:54 -0700
Subject: [PATCH] Fix roundtrip failure with groups when unknown fields are
 enabled.

When parsing a group, the group's end tag should not be stored within the
message's unknownFieldData. Not only does this waste space, it is also output
the next time the group is serialized, resulting in two end tags for that group.
The resulting bytes are not always a valid protocol buffer and may fail to
parse.

This change ensures that group end tags do not result in an unknownFieldData
entry, and that messages with groups can be roundtripped without corruption.

Change-Id: I240f858a7217a7652b756598c34aacad5dcc3363

Conflicts:
	java/src/test/java/com/google/protobuf/NanoTest.java
---
 .../google/protobuf/nano/WireFormatNano.java    |  9 +++++++--
 .../test/java/com/google/protobuf/NanoTest.java | 17 +++++++++++++++++
 .../protobuf/unittest_extension_nano.proto      |  6 ++++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/java/src/main/java/com/google/protobuf/nano/WireFormatNano.java b/java/src/main/java/com/google/protobuf/nano/WireFormatNano.java
index c901e59c92..301ff1db77 100644
--- a/java/src/main/java/com/google/protobuf/nano/WireFormatNano.java
+++ b/java/src/main/java/com/google/protobuf/nano/WireFormatNano.java
@@ -119,6 +119,9 @@ public final class WireFormatNano {
    * <p>Generated messages will call this for unknown fields if the store_unknown_fields
    * option is on.
    *
+   * <p>Note that the tag might be a end-group tag (rather than the start of an unknown field) in
+   * which case we do not want to add an unknown field entry.
+   *
    * @param data a Collection in which to store the data.
    * @param input the input buffer.
    * @param tag the tag of the field.
@@ -130,11 +133,13 @@ public final class WireFormatNano {
       final CodedInputByteBufferNano input,
       final int tag) throws IOException {
     int startPos = input.getPosition();
-    boolean skip = input.skipField(tag);
+    if (!input.skipField(tag)) {
+      return false;  // This wasn't an unknown field, it's an end-group tag.
+    }
     int endPos = input.getPosition();
     byte[] bytes = input.getData(startPos, endPos - startPos);
     data.add(new UnknownFieldData(tag, bytes));
-    return skip;
+    return true;
   }
 
   /**
diff --git a/java/src/test/java/com/google/protobuf/NanoTest.java b/java/src/test/java/com/google/protobuf/NanoTest.java
index 848361953b..ca0bcda488 100644
--- a/java/src/test/java/com/google/protobuf/NanoTest.java
+++ b/java/src/test/java/com/google/protobuf/NanoTest.java
@@ -35,6 +35,7 @@ import com.google.protobuf.nano.EnumClassNanoMultiple;
 import com.google.protobuf.nano.EnumClassNanos;
 import com.google.protobuf.nano.Extensions;
 import com.google.protobuf.nano.Extensions.AnotherMessage;
+import com.google.protobuf.nano.Extensions.MessageWithGroup;
 import com.google.protobuf.nano.FileScopeEnumMultiple;
 import com.google.protobuf.nano.FileScopeEnumRefNano;
 import com.google.protobuf.nano.InternalNano;
@@ -505,6 +506,22 @@ public class NanoTest extends TestCase {
     assertEquals(1, newMsg.optionalGroup.a);
   }
 
+  public void testNanoOptionalGroupWithUnknownFieldsEnabled() throws Exception {
+    MessageWithGroup msg = new MessageWithGroup();
+    MessageWithGroup.Group grp = new MessageWithGroup.Group();
+    grp.a = 1;
+    msg.group = grp;
+    byte [] serialized = MessageNano.toByteArray(msg);
+
+    MessageWithGroup parsed = MessageWithGroup.parseFrom(serialized);
+    assertTrue(msg.group != null);
+    assertEquals(1, msg.group.a);
+
+    byte [] serialized2 = MessageNano.toByteArray(parsed);
+    assertEquals(serialized2.length, serialized.length);
+    MessageWithGroup parsed2 = MessageWithGroup.parseFrom(serialized2);
+  }
+
   public void testNanoOptionalNestedMessage() throws Exception {
     TestAllTypesNano msg = new TestAllTypesNano();
     TestAllTypesNano.NestedMessage nestedMsg = new TestAllTypesNano.NestedMessage();
diff --git a/src/google/protobuf/unittest_extension_nano.proto b/src/google/protobuf/unittest_extension_nano.proto
index f2906f0023..104cfa749a 100644
--- a/src/google/protobuf/unittest_extension_nano.proto
+++ b/src/google/protobuf/unittest_extension_nano.proto
@@ -42,3 +42,9 @@ message ContainerMessage {
     optional bool another_thing = 100;
   }
 }
+
+message MessageWithGroup {
+  optional group Group = 1 {
+    optional int32 a = 2;
+  }
+}