Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C# Proto2 feature : Groups #5183

Merged
2 changes: 2 additions & 0 deletions BUILD
Expand Up @@ -301,10 +301,12 @@ cc_library(
"src/google/protobuf/compiler/csharp/csharp_map_field.cc",
"src/google/protobuf/compiler/csharp/csharp_message.cc",
"src/google/protobuf/compiler/csharp/csharp_message_field.cc",
"src/google/protobuf/compiler/csharp/csharp_group_field.cc",
"src/google/protobuf/compiler/csharp/csharp_primitive_field.cc",
"src/google/protobuf/compiler/csharp/csharp_reflection_class.cc",
"src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.cc",
"src/google/protobuf/compiler/csharp/csharp_repeated_message_field.cc",
"src/google/protobuf/compiler/csharp/csharp_repeated_group_field.cc",
"src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.cc",
"src/google/protobuf/compiler/csharp/csharp_source_generator_base.cc",
"src/google/protobuf/compiler/csharp/csharp_wrapper_field.cc",
Expand Down
2 changes: 2 additions & 0 deletions cmake/libprotoc.cmake
Expand Up @@ -24,10 +24,12 @@ set(libprotoc_files
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_map_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_message.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_message_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_group_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_primitive_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_reflection_class.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_repeated_enum_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_repeated_message_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_repeated_group_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_repeated_primitive_field.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_source_generator_base.cc
${protobuf_source_dir}/src/google/protobuf/compiler/csharp/csharp_wrapper_field.cc
Expand Down
56 changes: 55 additions & 1 deletion csharp/src/Google.Protobuf/CodedInputStream.cs
Expand Up @@ -108,6 +108,7 @@ public sealed class CodedInputStream : IDisposable
/// The absolute position of the end of the current message.
/// </summary>
private int currentLimit = int.MaxValue;
private uint tagLimit = uint.MaxValue;

private int recursionDepth = 0;

Expand Down Expand Up @@ -373,7 +374,7 @@ public uint ReadTag()
if (IsAtEnd)
{
lastTag = 0;
return 0; // This is the only case in which we return 0.
return 0;
}

lastTag = ReadRawVarint32();
Expand All @@ -383,6 +384,10 @@ public uint ReadTag()
// If we actually read a tag with a field of 0, that's not a valid tag.
throw InvalidProtocolBufferException.InvalidTag();
}
if (ReachedLimit || ReachedTagLimit)
Copy link
Contributor

@anandolee anandolee Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand you want to ReadTag returns 0 for the expected end group thus Merge/Parse can be stopped at the tag. However end group tag is a valid tag and returns 0 makes this function logically wrong. It will also introduce some error for example in SkipGroup():

It is easy to fix SkipGroup though but I still think ReadTag should return the valid tag. For Merge/Parse stop issue, I checked with java and it is using parseUnknownField() for decision :

" done = true;\n" // it's an endgroup tag

{
return 0;
}
return lastTag;
}

Expand Down Expand Up @@ -591,6 +596,26 @@ public void ReadMessage(IMessage builder)
PopLimit(oldLimit);
}

/// <summary>
/// Reads an embedded group field from the stream.
/// </summary>
public void ReadGroup(IMessage builder)
{
if (recursionDepth >= recursionLimit)
{
throw InvalidProtocolBufferException.RecursionLimitExceeded();
}
uint oldTag = PushTag();
++recursionDepth;
builder.MergeFrom(this);
if (!ReachedTagLimit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to push and pop the expected end group tag. Can calculate the expected tag and do the compare here directly.

{
throw InvalidProtocolBufferException.TruncatedMessage();
}
--recursionDepth;
PopTag(oldTag);
}

/// <summary>
/// Reads a bytes field value from the stream.
/// </summary>
Expand Down Expand Up @@ -953,6 +978,19 @@ internal int PushLimit(int byteLimit)
return oldLimit;
}

/// <summary>
/// Sets tagLimit to the limit calculated by the last tag.
/// This is called when descending into a group message. The previous limit is returns.
/// </summary>
/// <returns>The old limit</returns>
internal uint PushTag()
{
uint oldLimit = tagLimit;
uint newLimit = WireFormat.MakeTag(WireFormat.GetTagFieldNumber(LastTag), WireFormat.WireType.EndGroup);
tagLimit = newLimit;
return oldLimit;
}

private void RecomputeBufferSizeAfterLimit()
{
bufferSize += bufferSizeAfterLimit;
Expand All @@ -978,6 +1016,14 @@ internal void PopLimit(int oldLimit)
RecomputeBufferSizeAfterLimit();
}

/// <summary>
/// Discards the current limit, returning the previous limit
/// </summary>
internal void PopTag(uint oldTag)
{
tagLimit = oldTag;
}

/// <summary>
/// Returns whether or not all the data before the limit has been read.
/// </summary>
Expand All @@ -995,6 +1041,14 @@ internal bool ReachedLimit
}
}

internal bool ReachedTagLimit
{
get
{
return tagLimit == lastTag;
}
}

/// <summary>
/// Returns true if the stream has reached the end of the input. This is the
/// case if either the end of the underlying input source has been reached or
Expand Down
9 changes: 9 additions & 0 deletions csharp/src/Google.Protobuf/CodedOutputStream.cs
Expand Up @@ -303,6 +303,15 @@ public void WriteMessage(IMessage value)
value.WriteTo(this);
}

/// <summary>
/// Writes a group, without a tag, to the stream.
/// </summary>
/// <param name="value">The value to write</param>
public void WriteGroup(IMessage value)
{
value.WriteTo(this);
}

/// <summary>
/// Write a byte string, without a tag, to the stream.
/// The data is length-prefixed.
Expand Down
33 changes: 30 additions & 3 deletions csharp/src/Google.Protobuf/FieldCodec.cs
Expand Up @@ -225,6 +225,19 @@ public static FieldCodec<T> ForEnum<T>(uint tag, Func<T, int> toInt32, Func<int,
(output, value) => output.WriteMessage(value), message => CodedOutputStream.ComputeMessageSize(message), tag);
}

/// <summary>
/// Retrieves a codec suitable for a group field with the given tag.
/// </summary>
/// <param name="startTag">The start group tag.</param>
/// <param name="endTag">The end group tag.</param>
/// <param name="parser">A parser to use for the group message type.</param>
/// <returns>A codec for given tag</returns>
public static FieldCodec<T> ForGroup<T>(uint startTag, uint endTag, MessageParser<T> parser) where T : IMessage<T>
{
return new FieldCodec<T>(input => { T message = parser.CreateTemplate(); input.ReadGroup(message); return message; },
(output, value) => output.WriteGroup(value), message => CodedOutputStream.ComputeGroupSize(message), startTag, endTag);
}

/// <summary>
/// Creates a codec for a wrapper type of a class - which must be string or ByteString.
/// </summary>
Expand All @@ -235,7 +248,7 @@ public static FieldCodec<T> ForEnum<T>(uint tag, Func<T, int> toInt32, Func<int,
input => WrapperCodecs.Read<T>(input, nestedCodec),
(output, value) => WrapperCodecs.Write<T>(output, value, nestedCodec),
value => WrapperCodecs.CalculateSize<T>(value, nestedCodec),
tag,
tag, 0,
null); // Default value for the wrapper
}

Expand All @@ -250,7 +263,7 @@ public static FieldCodec<T> ForEnum<T>(uint tag, Func<T, int> toInt32, Func<int,
input => WrapperCodecs.Read<T>(input, nestedCodec),
(output, value) => WrapperCodecs.Write<T>(output, value.Value, nestedCodec),
value => value == null ? 0 : WrapperCodecs.CalculateSize<T>(value.Value, nestedCodec),
tag,
tag, 0,
null); // Default value for the wrapper
}

Expand Down Expand Up @@ -399,6 +412,14 @@ static FieldCodec()
/// </value>
internal uint Tag { get; }

/// <summary>
/// Gets the end tag of the codec or 0 if there is no end tag
/// </summary>
/// <value>
/// The end tag of the codec.
/// </value>
internal uint EndTag { get; }

/// <summary>
/// Default value for this codec. Usually the same for every instance of the same type, but
/// for string/ByteString wrapper fields the codec's default value is null, whereas for
Expand All @@ -424,7 +445,8 @@ static FieldCodec()
Func<CodedInputStream, T> reader,
Action<CodedOutputStream, T> writer,
Func<T, int> sizeCalculator,
uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault)
uint tag,
uint endTag = 0) : this(reader, writer, sizeCalculator, tag, endTag, DefaultDefault)
{
}

Expand All @@ -433,6 +455,7 @@ static FieldCodec()
Action<CodedOutputStream, T> writer,
Func<T, int> sizeCalculator,
uint tag,
uint endTag,
T defaultValue)
{
ValueReader = reader;
Expand All @@ -455,6 +478,10 @@ public void WriteTagAndValue(CodedOutputStream output, T value)
{
output.WriteTag(Tag);
ValueWriter(output, value);
if (EndTag != 0)
{
output.WriteTag(EndTag);
}
}
}

Expand Down
29 changes: 28 additions & 1 deletion csharp/src/Google.Protobuf/UnknownField.cs
Expand Up @@ -55,6 +55,7 @@ internal sealed class UnknownField
private List<uint> fixed32List;
private List<ulong> fixed64List;
private List<ByteString> lengthDelimitedList;
private List<UnknownFieldSet> groupList;

/// <summary>
/// Creates a new UnknownField.
Expand All @@ -77,7 +78,8 @@ public override bool Equals(object other)
&& Lists.Equals(varintList, otherField.varintList)
&& Lists.Equals(fixed32List, otherField.fixed32List)
&& Lists.Equals(fixed64List, otherField.fixed64List)
&& Lists.Equals(lengthDelimitedList, otherField.lengthDelimitedList);
&& Lists.Equals(lengthDelimitedList, otherField.lengthDelimitedList)
&& Lists.Equals(groupList, otherField.groupList);
}

/// <summary>
Expand All @@ -90,6 +92,7 @@ public override int GetHashCode()
hash = hash * 47 + Lists.GetHashCode(fixed32List);
hash = hash * 47 + Lists.GetHashCode(fixed64List);
hash = hash * 47 + Lists.GetHashCode(lengthDelimitedList);
hash = hash * 47 + Lists.GetHashCode(groupList);
return hash;
}

Expand Down Expand Up @@ -133,6 +136,15 @@ internal void WriteTo(int fieldNumber, CodedOutputStream output)
output.WriteBytes(value);
}
}
if (groupList != null)
{
foreach (UnknownFieldSet value in groupList)
{
output.WriteTag(fieldNumber, WireFormat.WireType.StartGroup);
value.WriteTo(output);
output.WriteTag(fieldNumber, WireFormat.WireType.EndGroup);
}
}
}

/// <summary>
Expand Down Expand Up @@ -168,6 +180,14 @@ internal int GetSerializedSize(int fieldNumber)
result += CodedOutputStream.ComputeBytesSize(value);
}
}
if (groupList != null)
{
result += CodedOutputStream.ComputeTagSize(fieldNumber) * 2 * groupList.Count;
foreach (UnknownFieldSet value in groupList)
{
result += value.CalculateSize();
}
}
return result;
}

Expand All @@ -182,6 +202,7 @@ internal UnknownField MergeFrom(UnknownField other)
fixed32List = AddAll(fixed32List, other.fixed32List);
fixed64List = AddAll(fixed64List, other.fixed64List);
lengthDelimitedList = AddAll(lengthDelimitedList, other.lengthDelimitedList);
groupList = AddAll(groupList, other.groupList);
return this;
}

Expand Down Expand Up @@ -245,6 +266,12 @@ internal UnknownField AddLengthDelimited(ByteString value)
return this;
}

internal UnknownField AddGroup(UnknownFieldSet value)
{
groupList = Add(groupList, value);
return this;
}

/// <summary>
/// Adds <paramref name="value"/> to the <paramref name="list"/>, creating
/// a new list if <paramref name="list"/> is null. The list is returned - either
Expand Down
8 changes: 7 additions & 1 deletion csharp/src/Google.Protobuf/UnknownFieldSet.cs
Expand Up @@ -215,7 +215,13 @@ private void MergeFieldFrom(CodedInputStream input)
}
case WireFormat.WireType.StartGroup:
{
input.SkipGroup(tag);
uint endTag = WireFormat.MakeTag(number, WireFormat.WireType.EndGroup);
UnknownFieldSet set = new UnknownFieldSet();
while (input.ReadTag() != endTag)
{
set.MergeFieldFrom(input);
}
GetOrAddField(number).AddGroup(set);
return;
}
case WireFormat.WireType.EndGroup:
Expand Down
4 changes: 4 additions & 0 deletions src/Makefile.am
Expand Up @@ -450,6 +450,8 @@ libprotoc_la_SOURCES = \
google/protobuf/compiler/csharp/csharp_message.h \
google/protobuf/compiler/csharp/csharp_message_field.cc \
google/protobuf/compiler/csharp/csharp_message_field.h \
google/protobuf/compiler/csharp/csharp_group_field.cc \
google/protobuf/compiler/csharp/csharp_group_field.h \
google/protobuf/compiler/csharp/csharp_options.h \
google/protobuf/compiler/csharp/csharp_primitive_field.cc \
google/protobuf/compiler/csharp/csharp_primitive_field.h \
Expand All @@ -459,6 +461,8 @@ libprotoc_la_SOURCES = \
google/protobuf/compiler/csharp/csharp_repeated_enum_field.h \
google/protobuf/compiler/csharp/csharp_repeated_message_field.cc \
google/protobuf/compiler/csharp/csharp_repeated_message_field.h \
google/protobuf/compiler/csharp/csharp_repeated_group_field.cc \
google/protobuf/compiler/csharp/csharp_repeated_group_field.h \
google/protobuf/compiler/csharp/csharp_repeated_primitive_field.cc \
google/protobuf/compiler/csharp/csharp_repeated_primitive_field.h \
google/protobuf/compiler/csharp/csharp_source_generator_base.cc \
Expand Down