Skip groups properly.

Now the generated code doesn't need to check for end group tags, as it will skip whole groups at a time.
Currently it will ignore extraneous end group tags, which may or may not be a good thing.
Renamed ConsumeLastField to SkipLastField as it felt more natural.
Removed WireFormat.IsEndGroupTag as it's no longer useful.

This mostly fixes issue 688.

(Generated code changes coming in next commit.)

1 files changed, 42 insertions, 13 deletions

diff --git a/csharp/src/Google.Protobuf/CodedInputStream.cs b/csharp/src/Google.Protobuf/CodedInputStream.cs
index 0e2495f1..a37fefc1 100644
--- a/csharp/src/Google.Protobuf/CodedInputStream.cs
+++ b/csharp/src/Google.Protobuf/CodedInputStream.cs
@@ -236,17 +236,16 @@ namespace Google.Protobuf
 

         #region Validation

         /// <summary>

-        /// Verifies that the last call to ReadTag() returned the given tag value.

-        /// This is used to verify that a nested group ended with the correct

-        /// end tag.

+        /// Verifies that the last call to ReadTag() returned tag 0 - in other words,

+        /// we've reached the end of the stream when we expected to.

         /// </summary>

-        /// <exception cref="InvalidProtocolBufferException">The last

+        /// <exception cref="InvalidProtocolBufferException">The 

         /// tag read was not the one specified</exception>

-        internal void CheckLastTagWas(uint value)

+        internal void CheckReadEndOfStreamTag()

         {

-            if (lastTag != value)

+            if (lastTag != 0)

             {

-                throw InvalidProtocolBufferException.InvalidEndTag();

+                throw InvalidProtocolBufferException.MoreDataAvailable();

             }

         }

         #endregion

@@ -275,6 +274,11 @@ namespace Google.Protobuf
         /// <summary>

         /// Reads a field tag, returning the tag of 0 for "end of stream".

         /// </summary>

+        /// <remarks>

+        /// If this method returns 0, it doesn't necessarily mean the end of all

+        /// the data in this CodedInputStream; it may be the end of the logical stream

+        /// for an embedded message, for example.

+        /// </remarks>

         /// <returns>The next field tag, or 0 for end of stream. (0 is never a valid tag.)</returns>

         public uint ReadTag()

         {

@@ -329,22 +333,24 @@ namespace Google.Protobuf
         }

 

         /// <summary>

-        /// Consumes the data for the field with the tag we've just read.

+        /// Skips the data for the field with the tag we've just read.

         /// This should be called directly after <see cref="ReadTag"/>, when

         /// the caller wishes to skip an unknown field.

         /// </summary>

-        public void ConsumeLastField()

+        public void SkipLastField()

         {

             if (lastTag == 0)

             {

-                throw new InvalidOperationException("ConsumeLastField cannot be called at the end of a stream");

+                throw new InvalidOperationException("SkipLastField cannot be called at the end of a stream");

             }

             switch (WireFormat.GetTagWireType(lastTag))

             {

                 case WireFormat.WireType.StartGroup:

+                    ConsumeGroup();

+                    break;

                 case WireFormat.WireType.EndGroup:

-                    // TODO: Work out how to skip them instead? See issue 688.

-                    throw new InvalidProtocolBufferException("Group tags not supported by proto3 C# implementation");

+                    // Just ignore; there's no data following the tag.

+                    break;

                 case WireFormat.WireType.Fixed32:

                     ReadFixed32();

                     break;

@@ -361,6 +367,29 @@ namespace Google.Protobuf
             }

         }

 

+        private void ConsumeGroup()

+        {

+            // Note: Currently we expect this to be the way that groups are read. We could put the recursion

+            // depth changes into the ReadTag method instead, potentially...

+            recursionDepth++;

+            if (recursionDepth >= recursionLimit)

+            {

+                throw InvalidProtocolBufferException.RecursionLimitExceeded();

+            }

+            uint tag;

+            do

+            {

+                tag = ReadTag();

+                if (tag == 0)

+                {

+                    throw InvalidProtocolBufferException.TruncatedMessage();

+                }

+                // This recursion will allow us to handle nested groups.

+                SkipLastField();

+            } while (WireFormat.GetTagWireType(tag) != WireFormat.WireType.EndGroup);

+            recursionDepth--;

+        }

+

         /// <summary>

         /// Reads a double field from the stream.

         /// </summary>

@@ -475,7 +504,7 @@ namespace Google.Protobuf
             int oldLimit = PushLimit(length);

             ++recursionDepth;

             builder.MergeFrom(this);

-            CheckLastTagWas(0);

+            CheckReadEndOfStreamTag();

             // Check that we've read exactly as much data as expected.

             if (!ReachedLimit)

             {