From b4a58173f24a6689d96ad4a294713f3f6b91ee17 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 6 Jan 2016 12:05:31 +0000 Subject: Ensure all formatted well-known-type values are valid JSON This involves quoting timestamp/duration/field-mask values, even when they're not in fields. It's better for consistency. Fixes issue #1097. --- .../src/Google.Protobuf.Test/JsonFormatterTest.cs | 41 ++++++++++------- csharp/src/Google.Protobuf.Test/JsonParserTest.cs | 25 +++++++---- csharp/src/Google.Protobuf/JsonFormatter.cs | 52 ++++++---------------- 3 files changed, 55 insertions(+), 63 deletions(-) (limited to 'csharp/src') diff --git a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs index 8473b4be..2cf2d5fc 100644 --- a/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs @@ -37,6 +37,8 @@ using UnitTest.Issues.TestProtos; using Google.Protobuf.WellKnownTypes; using Google.Protobuf.Reflection; +using static Google.Protobuf.JsonParserTest; // For WrapInQuotes + namespace Google.Protobuf { /// @@ -319,23 +321,28 @@ namespace Google.Protobuf } [Test] - public void TimestampStandalone() + [TestCase("1970-01-01T00:00:00Z", 0)] + [TestCase("1970-01-01T00:00:00.100Z", 100000000)] + [TestCase("1970-01-01T00:00:00.120Z", 120000000)] + [TestCase("1970-01-01T00:00:00.123Z", 123000000)] + [TestCase("1970-01-01T00:00:00.123400Z", 123400000)] + [TestCase("1970-01-01T00:00:00.123450Z", 123450000)] + [TestCase("1970-01-01T00:00:00.123456Z", 123456000)] + [TestCase("1970-01-01T00:00:00.123456700Z", 123456700)] + [TestCase("1970-01-01T00:00:00.123456780Z", 123456780)] + [TestCase("1970-01-01T00:00:00.123456789Z", 123456789)] + public void TimestampStandalone(string expected, int nanos) { - Assert.AreEqual("1970-01-01T00:00:00Z", new Timestamp().ToString()); - Assert.AreEqual("1970-01-01T00:00:00.100Z", new Timestamp { Nanos = 100000000 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.120Z", new Timestamp { Nanos = 120000000 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123Z", new Timestamp { Nanos = 123000000 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123400Z", new Timestamp { Nanos = 123400000 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123450Z", new Timestamp { Nanos = 123450000 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123456Z", new Timestamp { Nanos = 123456000 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123456700Z", new Timestamp { Nanos = 123456700 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123456780Z", new Timestamp { Nanos = 123456780 }.ToString()); - Assert.AreEqual("1970-01-01T00:00:00.123456789Z", new Timestamp { Nanos = 123456789 }.ToString()); + Assert.AreEqual(WrapInQuotes(expected), new Timestamp { Nanos = nanos }.ToString()); + } - // One before and one after the Unix epoch - Assert.AreEqual("1673-06-19T12:34:56Z", + [Test] + public void TimestampStandalone_FromDateTime() + { + // One before and one after the Unix epoch, more easily represented via DateTime. + Assert.AreEqual("\"1673-06-19T12:34:56Z\"", new DateTime(1673, 6, 19, 12, 34, 56, DateTimeKind.Utc).ToTimestamp().ToString()); - Assert.AreEqual("2015-07-31T10:29:34Z", + Assert.AreEqual("\"2015-07-31T10:29:34Z\"", new DateTime(2015, 7, 31, 10, 29, 34, DateTimeKind.Utc).ToTimestamp().ToString()); } @@ -367,7 +374,7 @@ namespace Google.Protobuf [TestCase(1, -100000000, "0.900s")] public void DurationStandalone(long seconds, int nanoseconds, string expected) { - Assert.AreEqual(expected, new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString()); + Assert.AreEqual(WrapInQuotes(expected), new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString()); } [Test] @@ -399,11 +406,11 @@ namespace Google.Protobuf public void FieldMaskStandalone() { var fieldMask = new FieldMask { Paths = { "", "single", "with_underscore", "nested.field.name", "nested..double_dot" } }; - Assert.AreEqual(",single,withUnderscore,nested.field.name,nested..doubleDot", fieldMask.ToString()); + Assert.AreEqual("\",single,withUnderscore,nested.field.name,nested..doubleDot\"", fieldMask.ToString()); // Invalid, but we shouldn't create broken JSON... fieldMask = new FieldMask { Paths = { "x\\y" } }; - Assert.AreEqual(@"x\\y", fieldMask.ToString()); + Assert.AreEqual(@"""x\\y""", fieldMask.ToString()); } [Test] diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index 874489e4..3babb391 100644 --- a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs +++ b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs @@ -149,7 +149,7 @@ namespace Google.Protobuf { ByteString data = ByteString.CopyFrom(1, 2, 3); // Can't do this with attributes... - var parsed = JsonParser.Default.Parse("\"" + data.ToBase64() + "\""); + var parsed = JsonParser.Default.Parse(WrapInQuotes(data.ToBase64())); var expected = new BytesValue { Value = data }; Assert.AreEqual(expected, parsed); } @@ -565,9 +565,9 @@ namespace Google.Protobuf public void Timestamp_Valid(string jsonValue, string expectedFormatted) { expectedFormatted = expectedFormatted ?? jsonValue; - string json = "\"" + jsonValue + "\""; + string json = WrapInQuotes(jsonValue); var parsed = Timestamp.Parser.ParseJson(json); - Assert.AreEqual(expectedFormatted, parsed.ToString()); + Assert.AreEqual(WrapInQuotes(expectedFormatted), parsed.ToString()); } [Test] @@ -592,7 +592,7 @@ namespace Google.Protobuf [TestCase("2100-02-29T14:46:23.123456789Z", Description = "Feb 29th on a non-leap-year")] public void Timestamp_Invalid(string jsonValue) { - string json = "\"" + jsonValue + "\""; + string json = WrapInQuotes(jsonValue); Assert.Throws(() => Timestamp.Parser.ParseJson(json)); } @@ -666,9 +666,9 @@ namespace Google.Protobuf public void Duration_Valid(string jsonValue, string expectedFormatted) { expectedFormatted = expectedFormatted ?? jsonValue; - string json = "\"" + jsonValue + "\""; + string json = WrapInQuotes(jsonValue); var parsed = Duration.Parser.ParseJson(json); - Assert.AreEqual(expectedFormatted, parsed.ToString()); + Assert.AreEqual(WrapInQuotes(expectedFormatted), parsed.ToString()); } // The simplest way of testing that the value has parsed correctly is to reformat it, @@ -697,7 +697,7 @@ namespace Google.Protobuf [TestCase("-3155760000000s", Description = "Integer part too long (negative)")] public void Duration_Invalid(string jsonValue) { - string json = "\"" + jsonValue + "\""; + string json = WrapInQuotes(jsonValue); Assert.Throws(() => Duration.Parser.ParseJson(json)); } @@ -713,7 +713,7 @@ namespace Google.Protobuf [TestCase("fooBar.bazQux", "foo_bar.baz_qux")] public void FieldMask_Valid(string jsonValue, params string[] expectedPaths) { - string json = "\"" + jsonValue + "\""; + string json = WrapInQuotes(jsonValue); var parsed = FieldMask.Parser.ParseJson(json); CollectionAssert.AreEqual(expectedPaths, parsed.Paths); } @@ -789,7 +789,16 @@ namespace Google.Protobuf var parser63 = new JsonParser(new JsonParser.Settings(63)); Assert.Throws(() => parser63.Parse(data64)); + } + /// + /// 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). + /// This method simply returns the input, wrapped in double quotes. + /// + internal static string WrapInQuotes(string text) + { + return '"' + text + '"'; } } } diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs index 7b99f314..bde17903 100644 --- a/csharp/src/Google.Protobuf/JsonFormatter.cs +++ b/csharp/src/Google.Protobuf/JsonFormatter.cs @@ -142,7 +142,7 @@ namespace Google.Protobuf StringBuilder builder = new StringBuilder(); if (message.Descriptor.IsWellKnownType) { - WriteWellKnownTypeValue(builder, message.Descriptor, message, false); + WriteWellKnownTypeValue(builder, message.Descriptor, message); } else { @@ -393,7 +393,7 @@ namespace Google.Protobuf IMessage message = (IMessage) value; if (message.Descriptor.IsWellKnownType) { - WriteWellKnownTypeValue(builder, message.Descriptor, value, true); + WriteWellKnownTypeValue(builder, message.Descriptor, value); } else { @@ -412,7 +412,7 @@ namespace Google.Protobuf /// values are using the embedded well-known types, in order to allow for dynamic messages /// in the future. /// - private void WriteWellKnownTypeValue(StringBuilder builder, MessageDescriptor descriptor, object value, bool inField) + private void WriteWellKnownTypeValue(StringBuilder builder, MessageDescriptor descriptor, object value) { // Currently, we can never actually get here, because null values are always handled by the caller. But if we *could*, // this would do the right thing. @@ -438,17 +438,17 @@ namespace Google.Protobuf } if (descriptor.FullName == Timestamp.Descriptor.FullName) { - MaybeWrapInString(builder, value, WriteTimestamp, inField); + WriteTimestamp(builder, (IMessage) value); return; } if (descriptor.FullName == Duration.Descriptor.FullName) { - MaybeWrapInString(builder, value, WriteDuration, inField); + WriteDuration(builder, (IMessage) value); return; } if (descriptor.FullName == FieldMask.Descriptor.FullName) { - MaybeWrapInString(builder, value, WriteFieldMask, inField); + WriteFieldMask(builder, (IMessage) value); return; } if (descriptor.FullName == Struct.Descriptor.FullName) @@ -475,26 +475,9 @@ namespace Google.Protobuf WriteMessage(builder, (IMessage) value); } - /// - /// Some well-known types end up as string values... so they need wrapping in quotes, but only - /// when they're being used as fields within another message. - /// - private void MaybeWrapInString(StringBuilder builder, object value, Action action, bool inField) - { - if (inField) - { - builder.Append('"'); - action(builder, (IMessage) value); - builder.Append('"'); - } - else - { - action(builder, (IMessage) value); - } - } - private void WriteTimestamp(StringBuilder builder, IMessage value) { + builder.Append('"'); // TODO: In the common case where this *is* using the built-in Timestamp type, we could // avoid all the reflection at this point, by casting to Timestamp. In the interests of // avoiding subtle bugs, don't do that until we've implemented DynamicMessage so that we can prove @@ -509,11 +492,12 @@ namespace Google.Protobuf DateTime dateTime = normalized.ToDateTime(); builder.Append(dateTime.ToString("yyyy'-'MM'-'dd'T'HH:mm:ss", CultureInfo.InvariantCulture)); AppendNanoseconds(builder, Math.Abs(normalized.Nanos)); - builder.Append('Z'); + builder.Append("Z\""); } private void WriteDuration(StringBuilder builder, IMessage value) { + builder.Append('"'); // TODO: Same as for WriteTimestamp int nanos = (int) value.Descriptor.Fields[Duration.NanosFieldNumber].Accessor.GetValue(value); long seconds = (long) value.Descriptor.Fields[Duration.SecondsFieldNumber].Accessor.GetValue(value); @@ -530,13 +514,13 @@ namespace Google.Protobuf builder.Append(normalized.Seconds.ToString("d", CultureInfo.InvariantCulture)); AppendNanoseconds(builder, Math.Abs(normalized.Nanos)); - builder.Append('s'); + builder.Append("s\""); } private void WriteFieldMask(StringBuilder builder, IMessage value) { IList paths = (IList) value.Descriptor.Fields[FieldMask.PathsFieldNumber].Accessor.GetValue(value); - AppendEscapedString(builder, string.Join(",", paths.Cast().Select(ToCamelCase))); + WriteString(builder, string.Join(",", paths.Cast().Select(ToCamelCase))); } private void WriteAny(StringBuilder builder, IMessage value) @@ -566,7 +550,7 @@ namespace Google.Protobuf builder.Append(PropertySeparator); WriteString(builder, AnyWellKnownTypeValueField); builder.Append(NameValueSeparator); - WriteWellKnownTypeValue(builder, descriptor, message, true); + WriteWellKnownTypeValue(builder, descriptor, message); } else { @@ -674,7 +658,7 @@ namespace Google.Protobuf case Value.ListValueFieldNumber: // Structs and ListValues are nested messages, and already well-known types. var nestedMessage = (IMessage) specifiedField.Accessor.GetValue(message); - WriteWellKnownTypeValue(builder, nestedMessage.Descriptor, nestedMessage, true); + WriteWellKnownTypeValue(builder, nestedMessage.Descriptor, nestedMessage); return; case Value.NullValueFieldNumber: WriteNull(builder); @@ -771,15 +755,6 @@ namespace Google.Protobuf private void WriteString(StringBuilder builder, string text) { builder.Append('"'); - AppendEscapedString(builder, text); - builder.Append('"'); - } - - /// - /// Appends the given text to the string builder, escaping as required. - /// - private void AppendEscapedString(StringBuilder builder, string text) - { for (int i = 0; i < text.Length; i++) { char c = text[i]; @@ -839,6 +814,7 @@ namespace Google.Protobuf break; } } + builder.Append('"'); } private const string Hex = "0123456789abcdef"; -- cgit v1.2.3