From f2fe50bfc516cdab99f51fa2ca90ba0db1ef6637 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 13 Jan 2016 14:05:06 +0000 Subject: [PATCH 01/14] JSON conformance test fixes - Spot an Any without a type URL - In the conformance test runner, catch exceptions due to generally-invalid JSON --- csharp/src/Google.Protobuf.Conformance/Program.cs | 4 ++++ csharp/src/Google.Protobuf.Test/JsonParserTest.cs | 7 +++++++ csharp/src/Google.Protobuf/JsonParser.cs | 5 +++++ 3 files changed, 16 insertions(+) diff --git a/csharp/src/Google.Protobuf.Conformance/Program.cs b/csharp/src/Google.Protobuf.Conformance/Program.cs index 8f72c8f9b..c40851c65 100644 --- a/csharp/src/Google.Protobuf.Conformance/Program.cs +++ b/csharp/src/Google.Protobuf.Conformance/Program.cs @@ -101,6 +101,10 @@ namespace Google.Protobuf.Conformance { return new ConformanceResponse { ParseError = e.Message }; } + catch (InvalidJsonException e) + { + return new ConformanceResponse { ParseError = e.Message }; + } switch (request.RequestedOutputFormat) { case global::Conformance.WireFormat.JSON: diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index 303baacc1..60c4d8157 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -762,6 +762,13 @@ namespace Google.Protobuf Assert.Throws(() => Any.Parser.ParseJson(json)); } + [Test] + public void Any_NoTypeUrl() + { + string json = "{ \"foo\": \"bar\" }"; + Assert.Throws(() => Any.Parser.ParseJson(json)); + } + [Test] public void Any_WellKnownType() { diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 300a66b4b..f7ebd057a 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -464,6 +464,11 @@ namespace Google.Protobuf { tokens.Add(token); token = tokenizer.Next(); + + if (tokenizer.ObjectDepth < typeUrlObjectDepth) + { + throw new InvalidProtocolBufferException("Any message with no @type"); + } } // Don't add the @type property or its value to the recorded token list From f262611ff65d5a5d1b4665e2d339f108ef29fcf9 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 10:13:56 +0000 Subject: [PATCH 02/14] Fix handling of repeated wrappers Previously we were incorrectly packing wrapper types. This also refactors FieldCodec a bit as well, using more C# 6-ness. --- .../WellKnownTypes/WrappersTest.cs | 24 +++ .../Collections/RepeatedField.cs | 9 +- csharp/src/Google.Protobuf/FieldCodec.cs | 158 +++++++++--------- 3 files changed, 104 insertions(+), 87 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs index b72ef982e..a2c833fe6 100644 --- a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs +++ b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs @@ -148,6 +148,30 @@ namespace Google.Protobuf.WellKnownTypes Assert.AreEqual("Second", message.StringField[1]); } + [Test] + public void RepeatedWrappersBinaryFormat() + { + // At one point we accidentally used a packed format for repeated wrappers, which is wrong (and weird). + // This test is just to prove that we use the right format. + + var rawOutput = new MemoryStream(); + var output = new CodedOutputStream(rawOutput); + // Write a value of 5 + output.WriteTag(RepeatedWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteLength(2); + output.WriteTag(WrappersReflection.WrapperValueFieldNumber, WireFormat.WireType.Varint); + output.WriteInt32(5); + // Write a value of 0 (empty message) + output.WriteTag(RepeatedWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited); + output.WriteLength(0); + output.Flush(); + var expectedBytes = rawOutput.ToArray(); + + var message = new RepeatedWellKnownTypes { Int32Field = { 5, 0 } }; + var actualBytes = message.ToByteArray(); + Assert.AreEqual(expectedBytes, actualBytes); + } + [Test] public void MapWrappersSerializeDeserialize() { diff --git a/csharp/src/Google.Protobuf/Collections/RepeatedField.cs b/csharp/src/Google.Protobuf/Collections/RepeatedField.cs index e3f65afea..1cde03bc7 100644 --- a/csharp/src/Google.Protobuf/Collections/RepeatedField.cs +++ b/csharp/src/Google.Protobuf/Collections/RepeatedField.cs @@ -34,7 +34,6 @@ using System; using System.Collections; using System.Collections.Generic; using System.Text; -using Google.Protobuf.Compatibility; namespace Google.Protobuf.Collections { @@ -96,8 +95,8 @@ namespace Google.Protobuf.Collections // iteration. uint tag = input.LastTag; var reader = codec.ValueReader; - // Value types can be packed or not. - if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited) + // Non-nullable value types can be packed or not. + if (FieldCodec.IsPackedRepeatedField(tag)) { int length = input.ReadLength(); if (length > 0) @@ -134,7 +133,7 @@ namespace Google.Protobuf.Collections return 0; } uint tag = codec.Tag; - if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited) + if (codec.PackedRepeatedField) { int dataSize = CalculatePackedDataSize(codec); return CodedOutputStream.ComputeRawVarint32Size(tag) + @@ -186,7 +185,7 @@ namespace Google.Protobuf.Collections } var writer = codec.ValueWriter; var tag = codec.Tag; - if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited) + if (codec.PackedRepeatedField) { // Packed primitive type uint size = (uint)CalculatePackedDataSize(codec); diff --git a/csharp/src/Google.Protobuf/FieldCodec.cs b/csharp/src/Google.Protobuf/FieldCodec.cs index c2b8d2681..983130888 100644 --- a/csharp/src/Google.Protobuf/FieldCodec.cs +++ b/csharp/src/Google.Protobuf/FieldCodec.cs @@ -30,6 +30,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #endregion +using Google.Protobuf.Compatibility; using Google.Protobuf.WellKnownTypes; using System; using System.Collections.Generic; @@ -329,17 +330,24 @@ namespace Google.Protobuf } /// + /// /// An encode/decode pair for a single field. This effectively encapsulates /// all the information needed to read or write the field value from/to a coded /// stream. + /// + /// + /// This class is public and has to be as it is used by generated code, but its public + /// API is very limited - just what the generated code needs to call directly. + /// /// /// - /// This never writes default values to the stream, and is not currently designed - /// to play well with packed arrays. + /// This never writes default values to the stream, and does not address "packedness" + /// in repeated fields itself, other than to know whether or not the field *should* be packed. /// public sealed class FieldCodec { private static readonly T DefaultDefault; + private static readonly bool TypeSupportsPacking = typeof(T).IsValueType() && Nullable.GetUnderlyingType(typeof(T)) == null; static FieldCodec() { @@ -354,16 +362,60 @@ namespace Google.Protobuf // Otherwise it's the default value of the CLR type } - private readonly Func reader; - private readonly Action writer; - private readonly Func sizeCalculator; - private readonly uint tag; + internal static bool IsPackedRepeatedField(uint tag) => + TypeSupportsPacking && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited; + + internal bool PackedRepeatedField { get; } + + /// + /// Returns a delegate to write a value (unconditionally) to a coded output stream. + /// + internal Action ValueWriter { get; } + + /// + /// Returns the size calculator for just a value. + /// + internal Func ValueSizeCalculator { get; } + + /// + /// Returns a delegate to read a value from a coded input stream. It is assumed that + /// the stream is already positioned on the appropriate tag. + /// + internal Func ValueReader { get; } + + /// + /// Returns the fixed size for an entry, or 0 if sizes vary. + /// + internal int FixedSize { get; } + + /// + /// Gets the tag of the codec. + /// + /// + /// The tag of the codec. + /// + internal uint Tag { get; } + + /// + /// 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 + /// other string/ByteString fields it's "" or ByteString.Empty. + /// + /// + /// The default value of the codec's type. + /// + internal T DefaultValue { get; } + private readonly int tagSize; - private readonly int fixedSize; - // 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 - // other string/ByteString fields it's "" or ByteString.Empty. - private readonly T defaultValue; + + internal FieldCodec( + Func reader, + Action writer, + int fixedSize, + uint tag) : this(reader, writer, _ => fixedSize, tag) + { + FixedSize = fixedSize; + } internal FieldCodec( Func reader, @@ -380,66 +432,17 @@ namespace Google.Protobuf uint tag, T defaultValue) { - this.reader = reader; - this.writer = writer; - this.sizeCalculator = sizeCalculator; - this.fixedSize = 0; - this.tag = tag; - this.defaultValue = defaultValue; + ValueReader = reader; + ValueWriter = writer; + ValueSizeCalculator = sizeCalculator; + FixedSize = 0; + Tag = tag; + DefaultValue = defaultValue; tagSize = CodedOutputStream.ComputeRawVarint32Size(tag); + // Detect packed-ness once, so we can check for it within RepeatedField. + PackedRepeatedField = IsPackedRepeatedField(tag); } - internal FieldCodec( - Func reader, - Action writer, - int fixedSize, - uint tag) - { - this.reader = reader; - this.writer = writer; - this.sizeCalculator = _ => fixedSize; - this.fixedSize = fixedSize; - this.tag = tag; - tagSize = CodedOutputStream.ComputeRawVarint32Size(tag); - } - - /// - /// Returns the size calculator for just a value. - /// - internal Func ValueSizeCalculator { get { return sizeCalculator; } } - - /// - /// Returns a delegate to write a value (unconditionally) to a coded output stream. - /// - internal Action ValueWriter { get { return writer; } } - - /// - /// Returns a delegate to read a value from a coded input stream. It is assumed that - /// the stream is already positioned on the appropriate tag. - /// - internal Func ValueReader { get { return reader; } } - - /// - /// Returns the fixed size for an entry, or 0 if sizes vary. - /// - internal int FixedSize { get { return fixedSize; } } - - /// - /// Gets the tag of the codec. - /// - /// - /// The tag of the codec. - /// - public uint Tag { get { return tag; } } - - /// - /// Gets the default value of the codec's type. - /// - /// - /// The default value of the codec's type. - /// - public T DefaultValue { get { return defaultValue; } } - /// /// Write a tag and the given value, *if* the value is not the default. /// @@ -447,8 +450,8 @@ namespace Google.Protobuf { if (!IsDefault(value)) { - output.WriteTag(tag); - writer(output, value); + output.WriteTag(Tag); + ValueWriter(output, value); } } @@ -457,23 +460,14 @@ namespace Google.Protobuf /// /// The input stream to read from. /// The value read from the stream. - public T Read(CodedInputStream input) - { - return reader(input); - } + public T Read(CodedInputStream input) => ValueReader(input); /// /// Calculates the size required to write the given value, with a tag, /// if the value is not the default. /// - public int CalculateSizeWithTag(T value) - { - return IsDefault(value) ? 0 : sizeCalculator(value) + tagSize; - } + public int CalculateSizeWithTag(T value) => IsDefault(value) ? 0 : ValueSizeCalculator(value) + tagSize; - private bool IsDefault(T value) - { - return EqualityComparer.Default.Equals(value, defaultValue); - } + private bool IsDefault(T value) => EqualityComparer.Default.Equals(value, DefaultValue); } } From 730c38ad8c11429bc7ea0310bc1b82f0831b42a6 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 10:41:56 +0000 Subject: [PATCH 03/14] Support (and test) numeric enum parsing in JSON --- .../Google.Protobuf.Test/JsonParserTest.cs | 20 +++++++++++++++++++ csharp/src/Google.Protobuf/JsonParser.cs | 9 +++++++++ 2 files changed, 29 insertions(+) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index 60c4d8157..b8fe67f81 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -821,6 +821,26 @@ namespace Google.Protobuf Assert.Throws(() => parser63.Parse(data64)); } + [Test] + [TestCase("\"FOREIGN_BAR\"")] + [TestCase("5")] + public void EnumValid(string value) + { + string json = "{ \"singleForeignEnum\": " + value + " }"; + var parsed = TestAllTypes.Parser.ParseJson(json); + Assert.AreEqual(new TestAllTypes { SingleForeignEnum = ForeignEnum.FOREIGN_BAR }, parsed); + } + + [Test] + [TestCase("\"NOT_A_VALID_VALUE\"")] + [TestCase("100")] + [TestCase("5.5")] + public void Enum_Invalid(string value) + { + string json = "{ \"singleForeignEnum\": " + value + " }"; + Assert.Throws(() => TestAllTypes.Parser.ParseJson(json)); + } + /// /// Various tests use strings which have quotes round them for parsing or as the result /// of formatting, but without those quotes being specified in the tests (for the sake of readability). diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index f7ebd057a..25afd0f29 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -608,6 +608,15 @@ namespace Google.Protobuf throw new InvalidProtocolBufferException($"Value out of range: {value}"); } return (float) value; + case FieldType.Enum: + CheckInteger(value); + var enumValue = field.EnumType.FindValueByNumber((int) value); + if (enumValue == null) + { + throw new InvalidProtocolBufferException($"Invalid enum value: {value} for enum type: {field.EnumType.FullName}"); + } + // Just return it as an int, and let the CLR convert it. + return enumValue.Number; default: throw new InvalidProtocolBufferException($"Unsupported conversion from JSON number for field type {field.FieldType}"); } From 1a34ac03bed31434caa110acc25537d871966f9d Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 10:43:06 +0000 Subject: [PATCH 04/14] Throw a better exception when invalid base64 is detected in JSON --- csharp/src/Google.Protobuf.Test/JsonParserTest.cs | 9 +++++++++ .../Google.Protobuf/InvalidProtocolBufferException.cs | 11 +++++++++++ csharp/src/Google.Protobuf/JsonParser.cs | 9 ++++++++- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index b8fe67f81..b12fe895c 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -821,6 +821,15 @@ namespace Google.Protobuf Assert.Throws(() => parser63.Parse(data64)); } + [Test] + [TestCase("AQI")] + [TestCase("_-==")] + public void Bytes_InvalidBase64(string badBase64) + { + string json = "{ \"singleBytes\": \"" + badBase64 + "\" }"; + Assert.Throws(() => TestAllTypes.Parser.ParseJson(json)); + } + [Test] [TestCase("\"FOREIGN_BAR\"")] [TestCase("5")] diff --git a/csharp/src/Google.Protobuf/InvalidProtocolBufferException.cs b/csharp/src/Google.Protobuf/InvalidProtocolBufferException.cs index cacda6483..eeb0f13ab 100644 --- a/csharp/src/Google.Protobuf/InvalidProtocolBufferException.cs +++ b/csharp/src/Google.Protobuf/InvalidProtocolBufferException.cs @@ -30,6 +30,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #endregion +using System; using System.IO; namespace Google.Protobuf @@ -45,6 +46,11 @@ namespace Google.Protobuf { } + internal InvalidProtocolBufferException(string message, Exception innerException) + : base(message, innerException) + { + } + internal static InvalidProtocolBufferException MoreDataAvailable() { return new InvalidProtocolBufferException( @@ -82,6 +88,11 @@ namespace Google.Protobuf "Protocol message contained an invalid tag (zero)."); } + internal static InvalidProtocolBufferException InvalidBase64(Exception innerException) + { + return new InvalidProtocolBufferException("Invalid base64 data", innerException); + } + internal static InvalidProtocolBufferException InvalidEndTag() { return new InvalidProtocolBufferException( diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 25afd0f29..10b05362d 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -647,7 +647,14 @@ namespace Google.Protobuf case FieldType.String: return text; case FieldType.Bytes: - return ByteString.FromBase64(text); + try + { + return ByteString.FromBase64(text); + } + catch (FormatException e) + { + throw InvalidProtocolBufferException.InvalidBase64(e); + } case FieldType.Int32: case FieldType.SInt32: case FieldType.SFixed32: From 888e71bdfc4918dbc16b62788c808cf415b7a8b0 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 10:55:36 +0000 Subject: [PATCH 05/14] Prohibit null values in repeated and map fields in JSON --- .../Google.Protobuf.Test/JsonParserTest.cs | 30 +++++++++++++++++++ csharp/src/Google.Protobuf/JsonParser.cs | 9 +++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index b12fe895c..790d7e899 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -170,6 +170,36 @@ namespace Google.Protobuf AssertRoundtrip(message); } + [Test] + public void RepeatedField_NullElementProhibited() + { + string json = "{ \"repeated_foreign_message\": [null] }"; + Assert.Throws(() => TestAllTypes.Parser.ParseJson(json)); + } + + [Test] + public void RepeatedField_NullOverallValueAllowed() + { + string json = "{ \"repeated_foreign_message\": null }"; + Assert.AreEqual(new TestAllTypes(), TestAllTypes.Parser.ParseJson(json)); + } + + [Test] + [TestCase("{ \"mapInt32Int32\": { \"10\": null }")] + [TestCase("{ \"mapStringString\": { \"abc\": null }")] + [TestCase("{ \"mapInt32ForeignMessage\": { \"10\": null }")] + public void MapField_NullValueProhibited(string json) + { + Assert.Throws(() => TestMap.Parser.ParseJson(json)); + } + + [Test] + public void MapField_NullOverallValueAllowed() + { + string json = "{ \"mapInt32Int32\": null }"; + Assert.AreEqual(new TestMap(), TestMap.Parser.ParseJson(json)); + } + [Test] public void IndividualWrapperTypes() { diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 10b05362d..0d997a0ac 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -239,6 +239,10 @@ namespace Google.Protobuf return; } tokenizer.PushBack(token); + if (token.Type == JsonToken.TokenType.Null) + { + throw new InvalidProtocolBufferException("Repeated field elements cannot be null"); + } list.Add(ParseSingleValue(field, tokenizer)); } } @@ -270,7 +274,10 @@ namespace Google.Protobuf } object key = ParseMapKey(keyField, token.StringValue); object value = ParseSingleValue(valueField, tokenizer); - // TODO: Null handling + if (value == null) + { + throw new InvalidProtocolBufferException("Map values must not be null"); + } dictionary[key] = value; } } From c74676f07037acca34e9df0fb868b29afae15ac9 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 10:55:57 +0000 Subject: [PATCH 06/14] Report serialization errors in conformance tests --- .../Google.Protobuf.Conformance/Program.cs | 23 ++++++++++++------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/csharp/src/Google.Protobuf.Conformance/Program.cs b/csharp/src/Google.Protobuf.Conformance/Program.cs index c40851c65..f3f7e2957 100644 --- a/csharp/src/Google.Protobuf.Conformance/Program.cs +++ b/csharp/src/Google.Protobuf.Conformance/Program.cs @@ -105,15 +105,22 @@ namespace Google.Protobuf.Conformance { return new ConformanceResponse { ParseError = e.Message }; } - switch (request.RequestedOutputFormat) + try { - case global::Conformance.WireFormat.JSON: - var formatter = new JsonFormatter(new JsonFormatter.Settings(false, typeRegistry)); - return new ConformanceResponse { JsonPayload = formatter.Format(message) }; - case global::Conformance.WireFormat.PROTOBUF: - return new ConformanceResponse { ProtobufPayload = message.ToByteString() }; - default: - throw new Exception("Unsupported request output format: " + request.PayloadCase); + switch (request.RequestedOutputFormat) + { + case global::Conformance.WireFormat.JSON: + var formatter = new JsonFormatter(new JsonFormatter.Settings(false, typeRegistry)); + return new ConformanceResponse { JsonPayload = formatter.Format(message) }; + case global::Conformance.WireFormat.PROTOBUF: + return new ConformanceResponse { ProtobufPayload = message.ToByteString() }; + default: + throw new Exception("Unsupported request output format: " + request.PayloadCase); + } + } + catch (InvalidOperationException e) + { + return new ConformanceResponse { SerializeError = e.Message }; } } From 1fc485928fc7a6483b700867f1a6cb2acfa8da5d Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 11:39:27 +0000 Subject: [PATCH 07/14] Fixes to JSON timestamp/duration representations --- .../Google.Protobuf.Test/JsonFormatterTest.cs | 28 +++++++++++++++-- .../Google.Protobuf.Test/JsonParserTest.cs | 1 - .../WellKnownTypes/DurationTest.cs | 30 +++++++++++++++---- .../WellKnownTypes/TimestampTest.cs | 23 ++++++++++++++ csharp/src/Google.Protobuf/JsonFormatter.cs | 29 ++++++++++-------- csharp/src/Google.Protobuf/JsonParser.cs | 18 +++++------ .../WellKnownTypes/DurationPartial.cs | 23 ++++++++++++++ .../WellKnownTypes/TimestampPartial.cs | 22 ++++++++++++-- 8 files changed, 139 insertions(+), 35 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs index ace70b004..09308556a 100644 --- a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs @@ -315,6 +315,15 @@ namespace Google.Protobuf [Test] [TestCase("1970-01-01T00:00:00Z", 0)] + [TestCase("1970-01-01T00:00:00.000000001Z", 1)] + [TestCase("1970-01-01T00:00:00.000000010Z", 10)] + [TestCase("1970-01-01T00:00:00.000000100Z", 100)] + [TestCase("1970-01-01T00:00:00.000001Z", 1000)] + [TestCase("1970-01-01T00:00:00.000010Z", 10000)] + [TestCase("1970-01-01T00:00:00.000100Z", 100000)] + [TestCase("1970-01-01T00:00:00.001Z", 1000000)] + [TestCase("1970-01-01T00:00:00.010Z", 10000000)] + [TestCase("1970-01-01T00:00:00.100Z", 100000000)] [TestCase("1970-01-01T00:00:00.100Z", 100000000)] [TestCase("1970-01-01T00:00:00.120Z", 120000000)] [TestCase("1970-01-01T00:00:00.123Z", 123000000)] @@ -350,6 +359,14 @@ namespace Google.Protobuf [TestCase(0, 0, "0s")] [TestCase(1, 0, "1s")] [TestCase(-1, 0, "-1s")] + [TestCase(0, 1, "0.000000001s")] + [TestCase(0, 10, "0.000000010s")] + [TestCase(0, 100, "0.000000100s")] + [TestCase(0, 1000, "0.000001s")] + [TestCase(0, 10000, "0.000010s")] + [TestCase(0, 100000, "0.000100s")] + [TestCase(0, 1000000, "0.001s")] + [TestCase(0, 10000000, "0.010s")] [TestCase(0, 100000000, "0.100s")] [TestCase(0, 120000000, "0.120s")] [TestCase(0, 123000000, "0.123s")] @@ -362,14 +379,19 @@ namespace Google.Protobuf [TestCase(0, -100000000, "-0.100s")] [TestCase(1, 100000000, "1.100s")] [TestCase(-1, -100000000, "-1.100s")] - // Non-normalized examples - [TestCase(1, 2123456789, "3.123456789s")] - [TestCase(1, -100000000, "0.900s")] public void DurationStandalone(long seconds, int nanoseconds, string expected) { Assert.AreEqual(WrapInQuotes(expected), new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString()); } + [Test] + [TestCase(1, 2123456789)] + [TestCase(1, -100000000)] + public void DurationStandalone_NonNormalized(long seconds, int nanoseconds, string expected) + { + Assert.Throws(() => new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString()); + } + [Test] public void DurationField() { diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index 790d7e899..d3c3a489b 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -745,7 +745,6 @@ namespace Google.Protobuf [TestCase("--0.123456789s", Description = "Double minus sign")] // Violate upper/lower bounds in various ways [TestCase("315576000001s", Description = "Integer part too large")] - [TestCase("315576000000.000000001s", Description = "Integer part is upper bound; non-zero fraction")] [TestCase("3155760000000s", Description = "Integer part too long (positive)")] [TestCase("-3155760000000s", Description = "Integer part too long (negative)")] public void Duration_Invalid(string jsonValue) diff --git a/csharp/src/Google.Protobuf.Test/WellKnownTypes/DurationTest.cs b/csharp/src/Google.Protobuf.Test/WellKnownTypes/DurationTest.cs index 36012e63b..1aa02e16c 100644 --- a/csharp/src/Google.Protobuf.Test/WellKnownTypes/DurationTest.cs +++ b/csharp/src/Google.Protobuf.Test/WellKnownTypes/DurationTest.cs @@ -50,11 +50,6 @@ namespace Google.Protobuf.WellKnownTypes // Rounding is towards 0 Assert.AreEqual(TimeSpan.FromTicks(2), new Duration { Nanos = 250 }.ToTimeSpan()); Assert.AreEqual(TimeSpan.FromTicks(-2), new Duration { Nanos = -250 }.ToTimeSpan()); - - // Non-normalized durations - Assert.AreEqual(TimeSpan.FromSeconds(3), new Duration { Seconds = 1, Nanos = 2 * Duration.NanosecondsPerSecond }.ToTimeSpan()); - Assert.AreEqual(TimeSpan.FromSeconds(1), new Duration { Seconds = 3, Nanos = -2 * Duration.NanosecondsPerSecond }.ToTimeSpan()); - Assert.AreEqual(TimeSpan.FromSeconds(-1), new Duration { Seconds = 1, Nanos = -2 * Duration.NanosecondsPerSecond }.ToTimeSpan()); } [Test] @@ -100,5 +95,30 @@ namespace Google.Protobuf.WellKnownTypes Assert.AreEqual(new Duration { Seconds = 1 }, Duration.FromTimeSpan(TimeSpan.FromSeconds(1))); Assert.AreEqual(new Duration { Nanos = Duration.NanosecondsPerTick }, Duration.FromTimeSpan(TimeSpan.FromTicks(1))); } + + [Test] + [TestCase(0, Duration.MaxNanoseconds + 1)] + [TestCase(0, Duration.MinNanoseconds - 1)] + [TestCase(Duration.MinSeconds - 1, 0)] + [TestCase(Duration.MaxSeconds + 1, 0)] + [TestCase(1, -1)] + [TestCase(-1, 1)] + public void ToTimeSpan_Invalid(long seconds, int nanoseconds) + { + var duration = new Duration { Seconds = seconds, Nanos = nanoseconds }; + Assert.Throws(() => duration.ToTimeSpan()); + } + + [Test] + [TestCase(0, Duration.MaxNanoseconds)] + [TestCase(0, Duration.MinNanoseconds)] + [TestCase(Duration.MinSeconds, Duration.MinNanoseconds)] + [TestCase(Duration.MaxSeconds, Duration.MaxNanoseconds)] + public void ToTimeSpan_Valid(long seconds, int nanoseconds) + { + // Only testing that these values don't throw, unlike their similar tests in ToTimeSpan_Invalid + var duration = new Duration { Seconds = seconds, Nanos = nanoseconds }; + duration.ToTimeSpan(); + } } } diff --git a/csharp/src/Google.Protobuf.Test/WellKnownTypes/TimestampTest.cs b/csharp/src/Google.Protobuf.Test/WellKnownTypes/TimestampTest.cs index 597539eb0..84717d665 100644 --- a/csharp/src/Google.Protobuf.Test/WellKnownTypes/TimestampTest.cs +++ b/csharp/src/Google.Protobuf.Test/WellKnownTypes/TimestampTest.cs @@ -61,6 +61,29 @@ namespace Google.Protobuf.WellKnownTypes Assert.AreEqual(new DateTime(1969, 12, 31, 23, 59, 59).AddMilliseconds(1), t2.ToDateTime()); } + [Test] + [TestCase(Timestamp.UnixSecondsAtBclMinValue - 1, Timestamp.MaxNanos)] + [TestCase(Timestamp.UnixSecondsAtBclMaxValue + 1, 0)] + [TestCase(0, -1)] + [TestCase(0, Timestamp.MaxNanos + 1)] + public void ToDateTime_OutOfRange(long seconds, int nanoseconds) + { + var value = new Timestamp { Seconds = seconds, Nanos = nanoseconds }; + Assert.Throws(() => value.ToDateTime()); + } + + // 1ns larger or smaller than the above values + [Test] + [TestCase(Timestamp.UnixSecondsAtBclMinValue, 0)] + [TestCase(Timestamp.UnixSecondsAtBclMaxValue, Timestamp.MaxNanos)] + [TestCase(0, 0)] + [TestCase(0, Timestamp.MaxNanos)] + public void ToDateTime_ValidBoundaries(long seconds, int nanoseconds) + { + var value = new Timestamp { Seconds = seconds, Nanos = nanoseconds }; + value.ToDateTime(); + } + private static void AssertRoundtrip(Timestamp timestamp, DateTime dateTime) { Assert.AreEqual(timestamp, Timestamp.FromDateTime(dateTime)); diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs index bde179039..563b834ec 100644 --- a/csharp/src/Google.Protobuf/JsonFormatter.cs +++ b/csharp/src/Google.Protobuf/JsonFormatter.cs @@ -485,13 +485,14 @@ namespace Google.Protobuf int nanos = (int) value.Descriptor.Fields[Timestamp.NanosFieldNumber].Accessor.GetValue(value); long seconds = (long) value.Descriptor.Fields[Timestamp.SecondsFieldNumber].Accessor.GetValue(value); - // Even if the original message isn't using the built-in classes, we can still build one... and then - // rely on it being normalized. - Timestamp normalized = Timestamp.Normalize(seconds, nanos); + // Even if the original message isn't using the built-in classes, we can still build one... and its + // conversion will check whether or not it's normalized. + // TODO: Perhaps the diagnostic-only formatter should not throw for non-normalized values? + Timestamp ts = new Timestamp { Seconds = seconds, Nanos = nanos }; // Use .NET's formatting for the value down to the second, including an opening double quote (as it's a string value) - DateTime dateTime = normalized.ToDateTime(); + DateTime dateTime = ts.ToDateTime(); builder.Append(dateTime.ToString("yyyy'-'MM'-'dd'T'HH:mm:ss", CultureInfo.InvariantCulture)); - AppendNanoseconds(builder, Math.Abs(normalized.Nanos)); + AppendNanoseconds(builder, Math.Abs(ts.Nanos)); builder.Append("Z\""); } @@ -502,18 +503,22 @@ namespace Google.Protobuf int nanos = (int) value.Descriptor.Fields[Duration.NanosFieldNumber].Accessor.GetValue(value); long seconds = (long) value.Descriptor.Fields[Duration.SecondsFieldNumber].Accessor.GetValue(value); + // TODO: Perhaps the diagnostic-only formatter should not throw for non-normalized values? // Even if the original message isn't using the built-in classes, we can still build one... and then // rely on it being normalized. - Duration normalized = Duration.Normalize(seconds, nanos); + if (!Duration.IsNormalized(seconds, nanos)) + { + throw new InvalidOperationException("Non-normalized duration value"); + } // The seconds part will normally provide the minus sign if we need it, but not if it's 0... - if (normalized.Seconds == 0 && normalized.Nanos < 0) + if (seconds == 0 && nanos < 0) { builder.Append('-'); } - builder.Append(normalized.Seconds.ToString("d", CultureInfo.InvariantCulture)); - AppendNanoseconds(builder, Math.Abs(normalized.Nanos)); + builder.Append(seconds.ToString("d", CultureInfo.InvariantCulture)); + AppendNanoseconds(builder, Math.Abs(nanos)); builder.Append("s\""); } @@ -598,15 +603,15 @@ namespace Google.Protobuf // Output to 3, 6 or 9 digits. if (nanos % 1000000 == 0) { - builder.Append((nanos / 1000000).ToString("d", CultureInfo.InvariantCulture)); + builder.Append((nanos / 1000000).ToString("d3", CultureInfo.InvariantCulture)); } else if (nanos % 1000 == 0) { - builder.Append((nanos / 1000).ToString("d", CultureInfo.InvariantCulture)); + builder.Append((nanos / 1000).ToString("d6", CultureInfo.InvariantCulture)); } else { - builder.Append(nanos.ToString("d", CultureInfo.InvariantCulture)); + builder.Append(nanos.ToString("d9", CultureInfo.InvariantCulture)); } } } diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 0d997a0ac..db601c578 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -854,28 +854,24 @@ namespace Google.Protobuf try { - long seconds = long.Parse(secondsText, CultureInfo.InvariantCulture); + long seconds = long.Parse(secondsText, CultureInfo.InvariantCulture) * multiplier; int nanos = 0; if (subseconds != "") { // This should always work, as we've got 1-9 digits. int parsedFraction = int.Parse(subseconds.Substring(1)); - nanos = parsedFraction * SubsecondScalingFactors[subseconds.Length]; + nanos = parsedFraction * SubsecondScalingFactors[subseconds.Length] * multiplier; } - if (seconds >= Duration.MaxSeconds) + if (!Duration.IsNormalized(seconds, nanos)) { - // Allow precisely 315576000000 seconds, but prohibit even 1ns more. - if (seconds > Duration.MaxSeconds || nanos > 0) - { - throw new InvalidProtocolBufferException("Invalid Duration value: " + token.StringValue); - } + throw new InvalidProtocolBufferException($"Invalid Duration value: {token.StringValue}"); } - message.Descriptor.Fields[Duration.SecondsFieldNumber].Accessor.SetValue(message, seconds * multiplier); - message.Descriptor.Fields[Duration.NanosFieldNumber].Accessor.SetValue(message, nanos * multiplier); + message.Descriptor.Fields[Duration.SecondsFieldNumber].Accessor.SetValue(message, seconds); + message.Descriptor.Fields[Duration.NanosFieldNumber].Accessor.SetValue(message, nanos); } catch (FormatException) { - throw new InvalidProtocolBufferException("Invalid Duration value: " + token.StringValue); + throw new InvalidProtocolBufferException($"Invalid Duration value: {token.StringValue}"); } } diff --git a/csharp/src/Google.Protobuf/WellKnownTypes/DurationPartial.cs b/csharp/src/Google.Protobuf/WellKnownTypes/DurationPartial.cs index 324f48fc0..b8eba9d3a 100644 --- a/csharp/src/Google.Protobuf/WellKnownTypes/DurationPartial.cs +++ b/csharp/src/Google.Protobuf/WellKnownTypes/DurationPartial.cs @@ -57,15 +57,38 @@ namespace Google.Protobuf.WellKnownTypes /// public const long MinSeconds = -315576000000L; + internal const int MaxNanoseconds = NanosecondsPerSecond - 1; + internal const int MinNanoseconds = -NanosecondsPerSecond + 1; + + internal static bool IsNormalized(long seconds, int nanoseconds) + { + // Simple boundaries + if (seconds < MinSeconds || seconds > MaxSeconds || + nanoseconds < MinNanoseconds || nanoseconds > MaxNanoseconds) + { + return false; + } + // We only have a problem is one is strictly negative and the other is + // strictly positive. + return Math.Sign(seconds) * Math.Sign(nanoseconds) != -1; + } + + /// /// Converts this to a . /// /// If the duration is not a precise number of ticks, it is truncated towards 0. /// The value of this duration, as a TimeSpan. + /// This value isn't a valid normalized duration, as + /// described in the documentation. public TimeSpan ToTimeSpan() { checked { + if (!IsNormalized(Seconds, Nanos)) + { + throw new InvalidOperationException("Duration was not a valid normalized duration"); + } long ticks = Seconds * TimeSpan.TicksPerSecond + Nanos / NanosecondsPerTick; return TimeSpan.FromTicks(ticks); } diff --git a/csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs b/csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs index d284acd63..7c50a3d78 100644 --- a/csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs +++ b/csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs @@ -37,9 +37,17 @@ namespace Google.Protobuf.WellKnownTypes public partial class Timestamp { private static readonly DateTime UnixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc); - private static readonly long BclSecondsAtUnixEpoch = UnixEpoch.Ticks / TimeSpan.TicksPerSecond; - internal static readonly long UnixSecondsAtBclMinValue = -BclSecondsAtUnixEpoch; - internal static readonly long UnixSecondsAtBclMaxValue = (DateTime.MaxValue.Ticks / TimeSpan.TicksPerSecond) - BclSecondsAtUnixEpoch; + // Constants determined programmatically, but then hard-coded so they can be constant expressions. + private const long BclSecondsAtUnixEpoch = 62135596800; + internal const long UnixSecondsAtBclMaxValue = 253402300799; + internal const long UnixSecondsAtBclMinValue = -BclSecondsAtUnixEpoch; + internal const int MaxNanos = Duration.NanosecondsPerSecond - 1; + + private bool IsNormalized => + Nanos >= 0 && + Nanos <= MaxNanos && + Seconds >= UnixSecondsAtBclMinValue && + Seconds <= UnixSecondsAtBclMaxValue; /// /// Returns the difference between one and another, as a . @@ -99,8 +107,14 @@ namespace Google.Protobuf.WellKnownTypes /// value precisely on a second. /// /// This timestamp as a DateTime. + /// The timestamp contains invalid values; either it is + /// incorrectly normalized or is outside the valid range. public DateTime ToDateTime() { + if (!IsNormalized) + { + throw new InvalidOperationException(@"Timestamp contains invalid values: Seconds={Seconds}; Nanos={Nanos}"); + } return UnixEpoch.AddSeconds(Seconds).AddTicks(Nanos / Duration.NanosecondsPerTick); } @@ -114,6 +128,8 @@ namespace Google.Protobuf.WellKnownTypes /// value precisely on a second. /// /// This timestamp as a DateTimeOffset. + /// The timestamp contains invalid values; either it is + /// incorrectly normalized or is outside the valid range. public DateTimeOffset ToDateTimeOffset() { return new DateTimeOffset(ToDateTime(), TimeSpan.Zero); From 022a9b267561a3fccfcfaa7023779b969692bf74 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 11:39:54 +0000 Subject: [PATCH 08/14] Allow the original field name (rather than camel-cased) when parsing JSON --- .../src/Google.Protobuf.Test/JsonParserTest.cs | 8 ++++++++ .../Reflection/MessageDescriptor.cs | 18 +++++++++++++++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index d3c3a489b..aa090d128 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -71,6 +71,14 @@ namespace Google.Protobuf Assert.Throws(() => JsonParser.Default.Parse(json)); } + [Test] + public void OriginalFieldNameAccepted() + { + var json = "{ \"single_int32\": 10 }"; + var expected = new TestAllTypes { SingleInt32 = 10 }; + Assert.AreEqual(expected, TestAllTypes.Parser.ParseJson(json)); + } + [Test] public void SourceContextRoundtrip() { diff --git a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs index d1732b2ec..f43803a63 100644 --- a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs @@ -92,11 +92,22 @@ namespace Google.Protobuf.Reflection new FieldDescriptor(field, file, this, index, generatedCodeInfo?.PropertyNames[index])); fieldsInNumberOrder = new ReadOnlyCollection(fieldsInDeclarationOrder.OrderBy(field => field.FieldNumber).ToArray()); // TODO: Use field => field.Proto.JsonName when we're confident it's appropriate. (And then use it in the formatter, too.) - jsonFieldMap = new ReadOnlyDictionary(fieldsInNumberOrder.ToDictionary(field => JsonFormatter.ToCamelCase(field.Name))); + jsonFieldMap = CreateJsonFieldMap(fieldsInNumberOrder); file.DescriptorPool.AddSymbol(this); Fields = new FieldCollection(this); } + private static ReadOnlyDictionary CreateJsonFieldMap(IList fields) + { + var map = new Dictionary(); + foreach (var field in fields) + { + map[JsonFormatter.ToCamelCase(field.Name)] = field; + map[field.Name] = field; + } + return new ReadOnlyDictionary(map); + } + /// /// The brief name of the descriptor's target. /// @@ -255,9 +266,10 @@ namespace Google.Protobuf.Reflection // TODO: consider making this public in the future. (Being conservative for now...) /// - /// Returns a read-only dictionary mapping the field names in this message as they're used + /// Returns a read-only dictionary mapping the field names in this message as they're available /// in the JSON representation to the field descriptors. For example, a field foo_bar - /// in the message would result in an entry with a key fooBar. + /// in the message would result two entries, one with a key fooBar and one with a key + /// foo_bar, both referring to the same field. /// internal IDictionary ByJsonName() => messageDescriptor.jsonFieldMap; From f437b67f600545f432863457a39870cb74675d34 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 12:02:07 +0000 Subject: [PATCH 09/14] Extra strictness for FieldMask conversion --- .../Google.Protobuf.Test/JsonFormatterTest.cs | 10 +++++++ .../Google.Protobuf.Test/JsonParserTest.cs | 8 ++++++ csharp/src/Google.Protobuf/JsonFormatter.cs | 27 ++++++++++++++++++- csharp/src/Google.Protobuf/JsonParser.cs | 8 +++++- 4 files changed, 51 insertions(+), 2 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs index 09308556a..98b00f464 100644 --- a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs @@ -417,6 +417,16 @@ namespace Google.Protobuf AssertJson("{ 'a': null, 'b': false, 'c': 10.5, 'd': 'text', 'e': [ 't1', 5 ], 'f': { 'nested': 'value' } }", message.ToString()); } + [Test] + [TestCase("foo__bar")] + [TestCase("foo_3_ar")] + [TestCase("fooBar")] + public void FieldMaskInvalid(string input) + { + var mask = new FieldMask { Paths = { input } }; + Assert.Throws(() => JsonFormatter.Default.Format(mask)); + } + [Test] public void FieldMaskStandalone() { diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index aa090d128..e8666b36e 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -778,6 +778,14 @@ namespace Google.Protobuf CollectionAssert.AreEqual(expectedPaths, parsed.Paths); } + [Test] + [TestCase("foo_bar")] + public void FieldMask_Invalid(string jsonValue) + { + string json = WrapInQuotes(jsonValue); + Assert.Throws(() => FieldMask.Parser.ParseJson(json)); + } + [Test] public void Any_RegularMessage() { diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs index 563b834ec..573ca7666 100644 --- a/csharp/src/Google.Protobuf/JsonFormatter.cs +++ b/csharp/src/Google.Protobuf/JsonFormatter.cs @@ -224,6 +224,31 @@ namespace Google.Protobuf return !first; } + /// + /// Camel-case converter with added strictness for field mask formatting. + /// + /// The field mask is invalid for JSON representation + private static string ToCamelCaseForFieldMask(string input) + { + for (int i = 0; i < input.Length; i++) + { + char c = input[i]; + if (c >= 'A' && c <= 'Z') + { + throw new InvalidOperationException($"Invalid field mask to be converted to JSON: {input}"); + } + if (c == '_' && i < input.Length - 1) + { + char next = input[i + 1]; + if (next < 'a' || next > 'z') + { + throw new InvalidOperationException($"Invalid field mask to be converted to JSON: {input}"); + } + } + } + return ToCamelCase(input); + } + // Converted from src/google/protobuf/util/internal/utility.cc ToCamelCase // TODO: Use the new field in FieldDescriptor. internal static string ToCamelCase(string input) @@ -525,7 +550,7 @@ namespace Google.Protobuf private void WriteFieldMask(StringBuilder builder, IMessage value) { IList paths = (IList) value.Descriptor.Fields[FieldMask.PathsFieldNumber].Accessor.GetValue(value); - WriteString(builder, string.Join(",", paths.Cast().Select(ToCamelCase))); + WriteString(builder, string.Join(",", paths.Cast().Select(ToCamelCaseForFieldMask))); } private void WriteAny(StringBuilder builder, IMessage value) diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index db601c578..9e5d87110 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -894,6 +894,8 @@ namespace Google.Protobuf private static string ToSnakeCase(string text) { var builder = new StringBuilder(text.Length * 2); + // Note: this is probably unnecessary now, but currently retained to be as close as possible to the + // C++, whilst still throwing an exception on underscores. bool wasNotUnderscore = false; // Initialize to false for case 1 (below) bool wasNotCap = false; @@ -927,7 +929,11 @@ namespace Google.Protobuf else { builder.Append(c); - wasNotUnderscore = c != '_'; + if (c == '_') + { + throw new InvalidProtocolBufferException($"Invalid field mask: {text}"); + } + wasNotUnderscore = true; wasNotCap = true; } } From 52db5139c4f122bbd34ef34b4103d0b650e7c218 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 13:45:53 +0000 Subject: [PATCH 10/14] Change handling of unknown enums: we now write out the value as a number. --- .../Google.Protobuf.Test/JsonFormatterTest.cs | 19 +++++++--------- .../Google.Protobuf.Test/JsonParserTest.cs | 10 ++++----- csharp/src/Google.Protobuf/JsonFormatter.cs | 22 +++++++------------ csharp/src/Google.Protobuf/JsonParser.cs | 8 ++----- 4 files changed, 23 insertions(+), 36 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs index 98b00f464..b0f58744d 100644 --- a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs @@ -165,34 +165,31 @@ namespace Google.Protobuf } [Test] - public void UnknownEnumValueOmitted_SingleField() + public void UnknownEnumValueNumeric_SingleField() { var message = new TestAllTypes { SingleForeignEnum = (ForeignEnum) 100 }; - AssertJson("{ }", JsonFormatter.Default.Format(message)); + AssertJson("{ 'singleForeignEnum': 100 }", JsonFormatter.Default.Format(message)); } [Test] - public void UnknownEnumValueOmitted_RepeatedField() + public void UnknownEnumValueNumeric_RepeatedField() { var message = new TestAllTypes { RepeatedForeignEnum = { ForeignEnum.FOREIGN_BAZ, (ForeignEnum) 100, ForeignEnum.FOREIGN_FOO } }; - AssertJson("{ 'repeatedForeignEnum': [ 'FOREIGN_BAZ', 'FOREIGN_FOO' ] }", JsonFormatter.Default.Format(message)); + AssertJson("{ 'repeatedForeignEnum': [ 'FOREIGN_BAZ', 100, 'FOREIGN_FOO' ] }", JsonFormatter.Default.Format(message)); } [Test] - public void UnknownEnumValueOmitted_MapField() + public void UnknownEnumValueNumeric_MapField() { - // This matches the C++ behaviour. var message = new TestMap { MapInt32Enum = { { 1, MapEnum.MAP_ENUM_FOO }, { 2, (MapEnum) 100 }, { 3, MapEnum.MAP_ENUM_BAR } } }; - AssertJson("{ 'mapInt32Enum': { '1': 'MAP_ENUM_FOO', '3': 'MAP_ENUM_BAR' } }", JsonFormatter.Default.Format(message)); + AssertJson("{ 'mapInt32Enum': { '1': 'MAP_ENUM_FOO', '2': 100, '3': 'MAP_ENUM_BAR' } }", JsonFormatter.Default.Format(message)); } [Test] - public void UnknownEnumValueOmitted_RepeatedField_AllEntriesUnknown() + public void UnknownEnumValue_RepeatedField_AllEntriesUnknown() { - // *Maybe* we should hold off on writing the "[" until we find that we've got at least one value to write... - // but this is what happens at the moment, and it doesn't seem too awful. var message = new TestAllTypes { RepeatedForeignEnum = { (ForeignEnum) 200, (ForeignEnum) 100 } }; - AssertJson("{ 'repeatedForeignEnum': [ ] }", JsonFormatter.Default.Format(message)); + AssertJson("{ 'repeatedForeignEnum': [ 200, 100 ] }", JsonFormatter.Default.Format(message)); } [Test] diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index e8666b36e..fb5e083e9 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -876,18 +876,18 @@ namespace Google.Protobuf } [Test] - [TestCase("\"FOREIGN_BAR\"")] - [TestCase("5")] - public void EnumValid(string value) + [TestCase("\"FOREIGN_BAR\"", ForeignEnum.FOREIGN_BAR)] + [TestCase("5", ForeignEnum.FOREIGN_BAR)] + [TestCase("100", (ForeignEnum) 100)] + public void EnumValid(string value, ForeignEnum expectedValue) { string json = "{ \"singleForeignEnum\": " + value + " }"; var parsed = TestAllTypes.Parser.ParseJson(json); - Assert.AreEqual(new TestAllTypes { SingleForeignEnum = ForeignEnum.FOREIGN_BAR }, parsed); + Assert.AreEqual(new TestAllTypes { SingleForeignEnum = expectedValue }, parsed); } [Test] [TestCase("\"NOT_A_VALID_VALUE\"")] - [TestCase("100")] [TestCase("5.5")] public void Enum_Invalid(string value) { diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs index 573ca7666..61961c4cf 100644 --- a/csharp/src/Google.Protobuf/JsonFormatter.cs +++ b/csharp/src/Google.Protobuf/JsonFormatter.cs @@ -205,11 +205,6 @@ namespace Google.Protobuf { continue; } - // Omit awkward (single) values such as unknown enum values - if (!field.IsRepeated && !field.IsMap && !CanWriteSingleValue(value)) - { - continue; - } // Okay, all tests complete: let's write the field value... if (!first) @@ -397,7 +392,14 @@ namespace Google.Protobuf } else if (value is System.Enum) { - WriteString(builder, value.ToString()); + if (System.Enum.IsDefined(value.GetType(), value)) + { + WriteString(builder, value.ToString()); + } + else + { + WriteValue(builder, (int) value); + } } else if (value is float || value is double) { @@ -704,10 +706,6 @@ namespace Google.Protobuf bool first = true; foreach (var value in list) { - if (!CanWriteSingleValue(value)) - { - continue; - } if (!first) { builder.Append(PropertySeparator); @@ -725,10 +723,6 @@ namespace Google.Protobuf // This will box each pair. Could use IDictionaryEnumerator, but that's ugly in terms of disposal. foreach (DictionaryEntry pair in dictionary) { - if (!CanWriteSingleValue(pair.Value)) - { - continue; - } if (!first) { builder.Append(PropertySeparator); diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 9e5d87110..92029e06a 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -617,13 +617,9 @@ namespace Google.Protobuf return (float) value; case FieldType.Enum: CheckInteger(value); - var enumValue = field.EnumType.FindValueByNumber((int) value); - if (enumValue == null) - { - throw new InvalidProtocolBufferException($"Invalid enum value: {value} for enum type: {field.EnumType.FullName}"); - } // Just return it as an int, and let the CLR convert it. - return enumValue.Number; + // Note that we deliberately don't check that it's a known value. + return (int) value; default: throw new InvalidProtocolBufferException($"Unsupported conversion from JSON number for field type {field.FieldType}"); } From 8866d6a80ed30e1ff32f1c5420f1a803c26fa13a Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 13:54:17 +0000 Subject: [PATCH 11/14] Reject JSON containing the same oneof field twice --- csharp/src/Google.Protobuf.Test/JsonParserTest.cs | 7 +++++++ csharp/src/Google.Protobuf/JsonParser.cs | 15 +++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index fb5e083e9..bfbde364c 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -895,6 +895,13 @@ namespace Google.Protobuf Assert.Throws(() => TestAllTypes.Parser.ParseJson(json)); } + [Test] + public void OneofDuplicate_Invalid() + { + string json = "{ \"oneofString\": \"x\", \"oneofUint32\": 10 }"; + Assert.Throws(() => TestAllTypes.Parser.ParseJson(json)); + } + /// /// Various tests use strings which have quotes round them for parsing or as the result /// of formatting, but without those quotes being specified in the tests (for the sake of readability). diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index 92029e06a..b1a248001 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -168,6 +168,10 @@ namespace Google.Protobuf } var descriptor = message.Descriptor; var jsonFieldMap = descriptor.Fields.ByJsonName(); + // All the oneof fields we've already accounted for - we can only see each of them once. + // The set is created lazily to avoid the overhead of creating a set for every message + // we parsed, when oneofs are relatively rare. + HashSet seenOneofs = null; while (true) { token = tokenizer.Next(); @@ -183,6 +187,17 @@ namespace Google.Protobuf FieldDescriptor field; if (jsonFieldMap.TryGetValue(name, out field)) { + if (field.ContainingOneof != null) + { + if (seenOneofs == null) + { + seenOneofs = new HashSet(); + } + if (!seenOneofs.Add(field.ContainingOneof)) + { + throw new InvalidProtocolBufferException($"Multiple values specified for oneof {field.ContainingOneof.Name}"); + } + } MergeField(message, field, tokenizer); } else From b1ea15f7a5480fe946165b0257de4981edca1c82 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 14:18:16 +0000 Subject: [PATCH 12/14] Make sure that "valueField": null is parsed appropriately, i.e. that it remembers that the field is set. --- .../Google.Protobuf.Test/JsonParserTest.cs | 12 +++++- .../TestProtos/UnittestWellKnownTypes.cs | 43 +++++++++++++++++-- csharp/src/Google.Protobuf/JsonParser.cs | 21 +++++++-- .../protobuf/unittest_well_known_types.proto | 2 + 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index bfbde364c..612a7b12c 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -124,7 +124,9 @@ namespace Google.Protobuf [Test] public void SingularWrappers_ExplicitNulls() { - var message = new TestWellKnownTypes(); + // When we parse the "valueField": null part, we remember it... basically, it's one case + // where explicit default values don't fully roundtrip. + var message = new TestWellKnownTypes { ValueField = Value.ForNull() }; var json = new JsonFormatter(new JsonFormatter.Settings(true)).Format(message); var parsed = JsonParser.Default.Parse(json); Assert.AreEqual(message, parsed); @@ -150,6 +152,14 @@ namespace Google.Protobuf Assert.AreEqual(expected, parsed); } + [Test] + public void ExplicitNullValue() + { + string json = "{\"valueField\": null}"; + var message = JsonParser.Default.Parse(json); + Assert.AreEqual(new TestWellKnownTypes { ValueField = Value.ForNull() }, message); + } + [Test] public void BytesWrapper_Standalone() { diff --git a/csharp/src/Google.Protobuf.Test/TestProtos/UnittestWellKnownTypes.cs b/csharp/src/Google.Protobuf.Test/TestProtos/UnittestWellKnownTypes.cs index dfada6bdd..2079a9607 100644 --- a/csharp/src/Google.Protobuf.Test/TestProtos/UnittestWellKnownTypes.cs +++ b/csharp/src/Google.Protobuf.Test/TestProtos/UnittestWellKnownTypes.cs @@ -31,7 +31,7 @@ namespace Google.Protobuf.TestProtos { "L3Byb3RvYnVmL3NvdXJjZV9jb250ZXh0LnByb3RvGhxnb29nbGUvcHJvdG9i", "dWYvc3RydWN0LnByb3RvGh9nb29nbGUvcHJvdG9idWYvdGltZXN0YW1wLnBy", "b3RvGhpnb29nbGUvcHJvdG9idWYvdHlwZS5wcm90bxoeZ29vZ2xlL3Byb3Rv", - "YnVmL3dyYXBwZXJzLnByb3RvIpEHChJUZXN0V2VsbEtub3duVHlwZXMSJwoJ", + "YnVmL3dyYXBwZXJzLnByb3RvIr4HChJUZXN0V2VsbEtub3duVHlwZXMSJwoJ", "YW55X2ZpZWxkGAEgASgLMhQuZ29vZ2xlLnByb3RvYnVmLkFueRInCglhcGlf", "ZmllbGQYAiABKAsyFC5nb29nbGUucHJvdG9idWYuQXBpEjEKDmR1cmF0aW9u", "X2ZpZWxkGAMgASgLMhkuZ29vZ2xlLnByb3RvYnVmLkR1cmF0aW9uEisKC2Vt", @@ -51,7 +51,8 @@ namespace Google.Protobuf.TestProtos { "cm90b2J1Zi5VSW50MzJWYWx1ZRIuCgpib29sX2ZpZWxkGBAgASgLMhouZ29v", "Z2xlLnByb3RvYnVmLkJvb2xWYWx1ZRIyCgxzdHJpbmdfZmllbGQYESABKAsy", "HC5nb29nbGUucHJvdG9idWYuU3RyaW5nVmFsdWUSMAoLYnl0ZXNfZmllbGQY", - "EiABKAsyGy5nb29nbGUucHJvdG9idWYuQnl0ZXNWYWx1ZSKVBwoWUmVwZWF0", + "EiABKAsyGy5nb29nbGUucHJvdG9idWYuQnl0ZXNWYWx1ZRIrCgt2YWx1ZV9m", + "aWVsZBgTIAEoCzIWLmdvb2dsZS5wcm90b2J1Zi5WYWx1ZSKVBwoWUmVwZWF0", "ZWRXZWxsS25vd25UeXBlcxInCglhbnlfZmllbGQYASADKAsyFC5nb29nbGUu", "cHJvdG9idWYuQW55EicKCWFwaV9maWVsZBgCIAMoCzIULmdvb2dsZS5wcm90", "b2J1Zi5BcGkSMQoOZHVyYXRpb25fZmllbGQYAyADKAsyGS5nb29nbGUucHJv", @@ -162,7 +163,7 @@ namespace Google.Protobuf.TestProtos { descriptor = pbr::FileDescriptor.FromGeneratedCode(descriptorData, new pbr::FileDescriptor[] { global::Google.Protobuf.WellKnownTypes.AnyReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.ApiReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.DurationReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.EmptyReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.FieldMaskReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.SourceContextReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.StructReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.TimestampReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.TypeReflection.Descriptor, global::Google.Protobuf.WellKnownTypes.WrappersReflection.Descriptor, }, new pbr::GeneratedCodeInfo(null, new pbr::GeneratedCodeInfo[] { - new pbr::GeneratedCodeInfo(typeof(global::Google.Protobuf.TestProtos.TestWellKnownTypes), global::Google.Protobuf.TestProtos.TestWellKnownTypes.Parser, new[]{ "AnyField", "ApiField", "DurationField", "EmptyField", "FieldMaskField", "SourceContextField", "StructField", "TimestampField", "TypeField", "DoubleField", "FloatField", "Int64Field", "Uint64Field", "Int32Field", "Uint32Field", "BoolField", "StringField", "BytesField" }, null, null, null), + new pbr::GeneratedCodeInfo(typeof(global::Google.Protobuf.TestProtos.TestWellKnownTypes), global::Google.Protobuf.TestProtos.TestWellKnownTypes.Parser, new[]{ "AnyField", "ApiField", "DurationField", "EmptyField", "FieldMaskField", "SourceContextField", "StructField", "TimestampField", "TypeField", "DoubleField", "FloatField", "Int64Field", "Uint64Field", "Int32Field", "Uint32Field", "BoolField", "StringField", "BytesField", "ValueField" }, null, null, null), new pbr::GeneratedCodeInfo(typeof(global::Google.Protobuf.TestProtos.RepeatedWellKnownTypes), global::Google.Protobuf.TestProtos.RepeatedWellKnownTypes.Parser, new[]{ "AnyField", "ApiField", "DurationField", "EmptyField", "FieldMaskField", "SourceContextField", "StructField", "TimestampField", "TypeField", "DoubleField", "FloatField", "Int64Field", "Uint64Field", "Int32Field", "Uint32Field", "BoolField", "StringField", "BytesField" }, null, null, null), new pbr::GeneratedCodeInfo(typeof(global::Google.Protobuf.TestProtos.OneofWellKnownTypes), global::Google.Protobuf.TestProtos.OneofWellKnownTypes.Parser, new[]{ "AnyField", "ApiField", "DurationField", "EmptyField", "FieldMaskField", "SourceContextField", "StructField", "TimestampField", "TypeField", "DoubleField", "FloatField", "Int64Field", "Uint64Field", "Int32Field", "Uint32Field", "BoolField", "StringField", "BytesField" }, new[]{ "OneofField" }, null, null), new pbr::GeneratedCodeInfo(typeof(global::Google.Protobuf.TestProtos.MapWellKnownTypes), global::Google.Protobuf.TestProtos.MapWellKnownTypes.Parser, new[]{ "AnyField", "ApiField", "DurationField", "EmptyField", "FieldMaskField", "SourceContextField", "StructField", "TimestampField", "TypeField", "DoubleField", "FloatField", "Int64Field", "Uint64Field", "Int32Field", "Uint32Field", "BoolField", "StringField", "BytesField" }, null, null, new pbr::GeneratedCodeInfo[] { null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, }) @@ -215,6 +216,7 @@ namespace Google.Protobuf.TestProtos { BoolField = other.BoolField; StringField = other.StringField; BytesField = other.BytesField; + ValueField = other.valueField_ != null ? other.ValueField.Clone() : null; } public TestWellKnownTypes Clone() { @@ -410,6 +412,19 @@ namespace Google.Protobuf.TestProtos { } } + /// Field number for the "value_field" field. + public const int ValueFieldFieldNumber = 19; + private global::Google.Protobuf.WellKnownTypes.Value valueField_; + /// + /// Part of struct, but useful to be able to test separately + /// + public global::Google.Protobuf.WellKnownTypes.Value ValueField { + get { return valueField_; } + set { + valueField_ = value; + } + } + public override bool Equals(object other) { return Equals(other as TestWellKnownTypes); } @@ -439,6 +454,7 @@ namespace Google.Protobuf.TestProtos { if (BoolField != other.BoolField) return false; if (StringField != other.StringField) return false; if (BytesField != other.BytesField) return false; + if (!object.Equals(ValueField, other.ValueField)) return false; return true; } @@ -462,6 +478,7 @@ namespace Google.Protobuf.TestProtos { if (boolField_ != null) hash ^= BoolField.GetHashCode(); if (stringField_ != null) hash ^= StringField.GetHashCode(); if (bytesField_ != null) hash ^= BytesField.GetHashCode(); + if (valueField_ != null) hash ^= ValueField.GetHashCode(); return hash; } @@ -533,6 +550,10 @@ namespace Google.Protobuf.TestProtos { if (bytesField_ != null) { _single_bytesField_codec.WriteTagAndValue(output, BytesField); } + if (valueField_ != null) { + output.WriteRawTag(154, 1); + output.WriteMessage(ValueField); + } } public int CalculateSize() { @@ -591,6 +612,9 @@ namespace Google.Protobuf.TestProtos { if (bytesField_ != null) { size += _single_bytesField_codec.CalculateSizeWithTag(BytesField); } + if (valueField_ != null) { + size += 2 + pb::CodedOutputStream.ComputeMessageSize(ValueField); + } return size; } @@ -697,6 +721,12 @@ namespace Google.Protobuf.TestProtos { BytesField = other.BytesField; } } + if (other.valueField_ != null) { + if (valueField_ == null) { + valueField_ = new global::Google.Protobuf.WellKnownTypes.Value(); + } + ValueField.MergeFrom(other.ValueField); + } } public void MergeFrom(pb::CodedInputStream input) { @@ -832,6 +862,13 @@ namespace Google.Protobuf.TestProtos { } break; } + case 154: { + if (valueField_ == null) { + valueField_ = new global::Google.Protobuf.WellKnownTypes.Value(); + } + input.ReadMessage(valueField_); + break; + } } } } diff --git a/csharp/src/Google.Protobuf/JsonParser.cs b/csharp/src/Google.Protobuf/JsonParser.cs index b1a248001..c07b16eac 100644 --- a/csharp/src/Google.Protobuf/JsonParser.cs +++ b/csharp/src/Google.Protobuf/JsonParser.cs @@ -215,10 +215,15 @@ namespace Google.Protobuf var token = tokenizer.Next(); if (token.Type == JsonToken.TokenType.Null) { + // Clear the field if we see a null token, unless it's for a singular field of type + // google.protobuf.Value. // Note: different from Java API, which just ignores it. // TODO: Bring it more in line? Discuss... - field.Accessor.Clear(message); - return; + if (field.IsMap || field.IsRepeated || !IsGoogleProtobufValueField(field)) + { + field.Accessor.Clear(message); + return; + } } tokenizer.PushBack(token); @@ -297,14 +302,22 @@ namespace Google.Protobuf } } + private static bool IsGoogleProtobufValueField(FieldDescriptor field) + { + return field.FieldType == FieldType.Message && + field.MessageType.FullName == Value.Descriptor.FullName; + } + private object ParseSingleValue(FieldDescriptor field, JsonTokenizer tokenizer) { var token = tokenizer.Next(); if (token.Type == JsonToken.TokenType.Null) { - if (field.FieldType == FieldType.Message && field.MessageType.FullName == Value.Descriptor.FullName) + // TODO: In order to support dynamic messages, we should really build this up + // dynamically. + if (IsGoogleProtobufValueField(field)) { - return new Value { NullValue = NullValue.NULL_VALUE }; + return Value.ForNull(); } return null; } diff --git a/src/google/protobuf/unittest_well_known_types.proto b/src/google/protobuf/unittest_well_known_types.proto index 2cb7775cc..c90752440 100644 --- a/src/google/protobuf/unittest_well_known_types.proto +++ b/src/google/protobuf/unittest_well_known_types.proto @@ -39,6 +39,8 @@ message TestWellKnownTypes { google.protobuf.BoolValue bool_field = 16; google.protobuf.StringValue string_field = 17; google.protobuf.BytesValue bytes_field = 18; + // Part of struct, but useful to be able to test separately + google.protobuf.Value value_field = 19; } // A repeated field for each well-known type. From 5ee055d53dbc0539bd9bef883097b652f7210b3f Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 14:43:17 +0000 Subject: [PATCH 13/14] Remove now-fixed conformance errors. --- conformance/failure_list_csharp.txt | 73 ----------------------------- 1 file changed, 73 deletions(-) diff --git a/conformance/failure_list_csharp.txt b/conformance/failure_list_csharp.txt index feb176899..a46cee472 100644 --- a/conformance/failure_list_csharp.txt +++ b/conformance/failure_list_csharp.txt @@ -1,89 +1,16 @@ -DurationProtoInputTooLarge.JsonOutput -DurationProtoInputTooSmall.JsonOutput -FieldMaskNumbersDontRoundTrip.JsonOutput -FieldMaskPathsDontRoundTrip.JsonOutput -FieldMaskTooManyUnderscore.JsonOutput JsonInput.AnyWithValueForInteger.JsonOutput JsonInput.AnyWithValueForJsonObject.JsonOutput -JsonInput.BoolFieldAllCapitalFalse -JsonInput.BoolFieldAllCapitalTrue -JsonInput.BoolFieldCamelCaseFalse -JsonInput.BoolFieldCamelCaseTrue -JsonInput.BoolMapFieldKeyNotQuoted -JsonInput.BytesFieldInvalidBase64Characters -JsonInput.BytesFieldNoPadding -JsonInput.DoubleFieldInfinityNotQuoted -JsonInput.DoubleFieldNanNotQuoted -JsonInput.DoubleFieldNegativeInfinityNotQuoted -JsonInput.DoubleFieldTooLarge -JsonInput.DoubleFieldTooSmall -JsonInput.DurationHas3FractionalDigits.Validator -JsonInput.DurationHas6FractionalDigits.Validator -JsonInput.DurationHas9FractionalDigits.Validator -JsonInput.DurationMaxValue.JsonOutput -JsonInput.DurationMaxValue.ProtobufOutput -JsonInput.DurationMinValue.JsonOutput -JsonInput.DurationMinValue.ProtobufOutput -JsonInput.EnumFieldNotQuoted -JsonInput.EnumFieldNumericValueNonZero.JsonOutput -JsonInput.EnumFieldNumericValueNonZero.ProtobufOutput -JsonInput.EnumFieldNumericValueZero.JsonOutput -JsonInput.EnumFieldNumericValueZero.ProtobufOutput -JsonInput.EnumFieldUnknownValue.Validator -JsonInput.FieldMaskInvalidCharacter -JsonInput.FieldNameDuplicate -JsonInput.FieldNameDuplicateDifferentCasing2 JsonInput.FieldNameInLowerCamelCase.Validator JsonInput.FieldNameInSnakeCase.JsonOutput JsonInput.FieldNameInSnakeCase.ProtobufOutput -JsonInput.FieldNameNotQuoted JsonInput.FieldNameWithMixedCases.JsonOutput JsonInput.FieldNameWithMixedCases.ProtobufOutput JsonInput.FieldNameWithMixedCases.Validator -JsonInput.FloatFieldInfinityNotQuoted -JsonInput.FloatFieldNanNotQuoted -JsonInput.FloatFieldNegativeInfinityNotQuoted -JsonInput.Int32FieldLeadingZero JsonInput.Int32FieldMinFloatValue.JsonOutput JsonInput.Int32FieldMinValue.JsonOutput -JsonInput.Int32FieldNegativeWithLeadingZero -JsonInput.Int32FieldPlusSign -JsonInput.Int32MapFieldKeyNotQuoted JsonInput.Int64FieldMaxValueNotQuoted.JsonOutput JsonInput.Int64FieldMaxValueNotQuoted.ProtobufOutput -JsonInput.Int64MapFieldKeyNotQuoted -JsonInput.JsonWithComments -JsonInput.MapFieldKeyIsNull -JsonInput.MapFieldValueIsNull -JsonInput.OneofFieldDuplicate JsonInput.OriginalProtoFieldName.JsonOutput -JsonInput.OriginalProtoFieldName.ProtobufOutput -JsonInput.RepeatedBoolWrapper.ProtobufOutput -JsonInput.RepeatedDoubleWrapper.ProtobufOutput -JsonInput.RepeatedFieldMessageElementIsNull -JsonInput.RepeatedFieldPrimitiveElementIsNull -JsonInput.RepeatedFieldTrailingComma -JsonInput.RepeatedFloatWrapper.ProtobufOutput -JsonInput.RepeatedInt32Wrapper.ProtobufOutput -JsonInput.RepeatedInt64Wrapper.ProtobufOutput -JsonInput.RepeatedUint32Wrapper.ProtobufOutput -JsonInput.RepeatedUint64Wrapper.ProtobufOutput -JsonInput.StringFieldInvalidEscape -JsonInput.StringFieldSurrogateInWrongOrder JsonInput.StringFieldSurrogatePair.JsonOutput -JsonInput.StringFieldUnpairedHighSurrogate -JsonInput.StringFieldUnpairedLowSurrogate -JsonInput.StringFieldUnterminatedEscape -JsonInput.StringFieldUppercaseEscapeLetter -JsonInput.TimestampHas3FractionalDigits.Validator -JsonInput.TimestampHas6FractionalDigits.Validator -JsonInput.TimestampHas9FractionalDigits.Validator -JsonInput.TrailingCommaInAnObject -JsonInput.Uint32MapFieldKeyNotQuoted JsonInput.Uint64FieldMaxValueNotQuoted.JsonOutput JsonInput.Uint64FieldMaxValueNotQuoted.ProtobufOutput -JsonInput.Uint64MapFieldKeyNotQuoted -JsonInput.ValueAcceptNull.JsonOutput -JsonInput.ValueAcceptNull.ProtobufOutput -TimestampProtoInputTooLarge.JsonOutput -TimestampProtoInputTooSmall.JsonOutput From 030c2684899b2b84f1023d668c5bf40b6ce1143d Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 17:34:10 +0000 Subject: [PATCH 14/14] Fix broken test --- csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs index b0f58744d..9e994a6a9 100644 --- a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs @@ -384,7 +384,7 @@ namespace Google.Protobuf [Test] [TestCase(1, 2123456789)] [TestCase(1, -100000000)] - public void DurationStandalone_NonNormalized(long seconds, int nanoseconds, string expected) + public void DurationStandalone_NonNormalized(long seconds, int nanoseconds) { Assert.Throws(() => new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString()); }