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.

3 files changed, 55 insertions, 63 deletions

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
 {
     /// <summary>
@@ -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<BytesValue>("\"" + data.ToBase64() + "\"");
+            var parsed = JsonParser.Default.Parse<BytesValue>(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<InvalidProtocolBufferException>(() => 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<InvalidProtocolBufferException>(() => 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<InvalidProtocolBufferException>(() => parser63.Parse<TestRecursiveMessage>(data64));
+        }
 
+        /// <summary>
+        /// 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.
+        /// </summary>
+        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.
         /// </summary>
-        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);
         }
 
-        /// <summary>
-        /// 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.
-        /// </summary>
-        private void MaybeWrapInString(StringBuilder builder, object value, Action<StringBuilder, IMessage> 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<string>().Select(ToCamelCase)));
+            WriteString(builder, string.Join(",", paths.Cast<string>().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('"');
-        }
-
-        /// <summary>
-        /// Appends the given text to the string builder, escaping as required.
-        /// </summary>
-        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";