Merge pull request #1158 from jskeet/nonnormalized-tostring

Ensure that FieldMask, Timestamp and Duration ToString() calls don't throw

csharp/src/Google.Protobuf.Test/Google.Protobuf.Test.csproj

@@ -115,6 +115,7 @@
     <Compile Include="TestProtos\UnittestWellKnownTypes.cs" />
     <Compile Include="WellKnownTypes\AnyTest.cs" />
     <Compile Include="WellKnownTypes\DurationTest.cs" />
+    <Compile Include="WellKnownTypes\FieldMaskTest.cs" />
     <Compile Include="WellKnownTypes\TimestampTest.cs" />
     <Compile Include="WellKnownTypes\WrappersTest.cs" />
   </ItemGroup>

csharp/src/Google.Protobuf.Test/JsonFormatterTest.cs

@@ -345,6 +345,17 @@ namespace Google.Protobuf
                 new DateTime(2015, 7, 31, 10, 29, 34, DateTimeKind.Utc).ToTimestamp().ToString());
         }
 
+        [Test]
+        [TestCase(-1, -1)] // Would be valid as duration
+        [TestCase(1, Timestamp.MaxNanos + 1)]
+        [TestCase(Timestamp.UnixSecondsAtBclMaxValue + 1, 0)]
+        [TestCase(Timestamp.UnixSecondsAtBclMinValue - 1, 0)]
+        public void TimestampStandalone_NonNormalized(long seconds, int nanoseconds)
+        {
+            var timestamp = new Timestamp { Seconds = seconds, Nanos = nanoseconds };
+            Assert.Throws<InvalidOperationException>(() => JsonFormatter.Default.Format(timestamp));
+        }
+
         [Test]
         public void TimestampField()
         {
@@ -378,7 +389,8 @@ namespace Google.Protobuf
         [TestCase(-1, -100000000, "-1.100s")]
         public void DurationStandalone(long seconds, int nanoseconds, string expected)
         {
-            Assert.AreEqual(WrapInQuotes(expected), new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString());
+            var json = JsonFormatter.Default.Format(new Duration { Seconds = seconds, Nanos = nanoseconds });
+            Assert.AreEqual(WrapInQuotes(expected), json);
         }
 
         [Test]
@@ -386,7 +398,8 @@ namespace Google.Protobuf
         [TestCase(1, -100000000)]
         public void DurationStandalone_NonNormalized(long seconds, int nanoseconds)
         {
-            Assert.Throws<InvalidOperationException>(() => new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString());
+            var duration = new Duration { Seconds = seconds, Nanos = nanoseconds };
+            Assert.Throws<InvalidOperationException>(() => JsonFormatter.Default.Format(duration));
         }
 
         [Test]

csharp/src/Google.Protobuf.Test/WellKnownTypes/DurationTest.cs

@@ -120,5 +120,13 @@ namespace Google.Protobuf.WellKnownTypes
             var duration = new Duration { Seconds = seconds, Nanos = nanoseconds };
             duration.ToTimeSpan();
         }
+
+        [Test]
+        public void ToString_NonNormalized()
+        {
+            // Just a single example should be sufficient...
+            var duration = new Duration { Seconds = 1, Nanos = -1 };
+            Assert.AreEqual("{ \"@warning\": \"Invalid Duration\", \"seconds\": \"1\", \"nanos\": -1 }", duration.ToString());
+        }
     }
 }

csharp/src/Google.Protobuf.Test/WellKnownTypes/FieldMaskTest.cs

+#region Copyright notice and license
+// Protocol Buffers - Google's data interchange format
+// Copyright 2016 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#endregion
+
+
+using NUnit.Framework;
+
+namespace Google.Protobuf.WellKnownTypes
+{
+    public class FieldMaskTest
+    {
+        [Test]
+        [TestCase("foo__bar")]
+        [TestCase("foo_3_ar")]
+        [TestCase("fooBar")]
+        public void ToString_Invalid(string input)
+        {
+            var mask = new FieldMask { Paths = { input } };
+            var text = mask.ToString();
+            // More specific test below
+            Assert.That(text, Is.StringContaining("@warning"));
+            Assert.That(text, Is.StringContaining(input));
+        }
+
+        [Test]
+        public void ToString_Invalid_Precise()
+        {
+            var mask = new FieldMask { Paths = { "x", "foo__bar", @"x\y" } };
+            Assert.AreEqual(
+                "{ \"@warning\": \"Invalid FieldMask\", \"paths\": [ \"x\", \"foo__bar\", \"x\\\\y\" ] }",
+                mask.ToString());
+        }
+    }
+}

csharp/src/Google.Protobuf.Test/WellKnownTypes/TimestampTest.cs

@@ -103,5 +103,13 @@ namespace Google.Protobuf.WellKnownTypes
             Assert.AreEqual(t1, t2 + difference);
             Assert.AreEqual(t2, t1 - difference);
         }
+
+        [Test]
+        public void ToString_NonNormalized()
+        {
+            // Just a single example should be sufficient...
+            var duration = new Timestamp { Seconds = 1, Nanos = -1 };
+            Assert.AreEqual("{ \"@warning\": \"Invalid Timestamp\", \"seconds\": \"1\", \"nanos\": -1 }", duration.ToString());
+        }
     }
 }

csharp/src/Google.Protobuf/Google.Protobuf.csproj

@@ -132,6 +132,7 @@
     <Compile Include="WellKnownTypes\DurationPartial.cs" />
     <Compile Include="WellKnownTypes\Empty.cs" />
     <Compile Include="WellKnownTypes\FieldMask.cs" />
+    <Compile Include="WellKnownTypes\FieldMaskPartial.cs" />
     <Compile Include="WellKnownTypes\SourceContext.cs" />
     <Compile Include="WellKnownTypes\Struct.cs" />
     <Compile Include="WellKnownTypes\TimeExtensions.cs" />

csharp/src/Google.Protobuf/JsonFormatter.cs

@@ -37,6 +37,7 @@ using System.Text;
 using Google.Protobuf.Reflection;
 using Google.Protobuf.WellKnownTypes;
 using System.Linq;
+using System.Collections.Generic;
 
 namespace Google.Protobuf
 {
@@ -122,6 +123,8 @@ namespace Google.Protobuf
 
         private readonly Settings settings;
 
+        private bool DiagnosticOnly => ReferenceEquals(this, diagnosticFormatter);
+
         /// <summary>
         /// Creates a new formatted with the given settings.
         /// </summary>
@@ -181,7 +184,7 @@ namespace Google.Protobuf
                 WriteNull(builder);
                 return;
             }
-            if (ReferenceEquals(this, diagnosticFormatter))
+            if (DiagnosticOnly)
             {
                 ICustomDiagnosticMessage customDiagnosticMessage = message as ICustomDiagnosticMessage;
                 if (customDiagnosticMessage != null)
@@ -513,60 +516,32 @@ namespace Google.Protobuf
 
         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
             // it still works in that case.
             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 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 = ts.ToDateTime();
-            builder.Append(dateTime.ToString("yyyy'-'MM'-'dd'T'HH:mm:ss", CultureInfo.InvariantCulture));
-            AppendNanoseconds(builder, Math.Abs(ts.Nanos));
-            builder.Append("Z\"");
+            builder.Append(Timestamp.ToJson(seconds, nanos, DiagnosticOnly));
         }
 
         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);
-
-            // 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.
-            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 (seconds == 0 && nanos < 0)
-            {
-                builder.Append('-');
-            }
-
-            builder.Append(seconds.ToString("d", CultureInfo.InvariantCulture));
-            AppendNanoseconds(builder, Math.Abs(nanos));
-            builder.Append("s\"");
+            builder.Append(Duration.ToJson(seconds, nanos, DiagnosticOnly));
         }
 
         private void WriteFieldMask(StringBuilder builder, IMessage value)
         {
-            IList paths = (IList) value.Descriptor.Fields[FieldMask.PathsFieldNumber].Accessor.GetValue(value);
-            WriteString(builder, string.Join(",", paths.Cast<string>().Select(ToCamelCaseForFieldMask)));
+            var paths = (IList<string>) value.Descriptor.Fields[FieldMask.PathsFieldNumber].Accessor.GetValue(value);
+            builder.Append(FieldMask.ToJson(paths, DiagnosticOnly));
         }
 
         private void WriteAny(StringBuilder builder, IMessage value)
         {
-            if (ReferenceEquals(this, diagnosticFormatter))
+            if (DiagnosticOnly)
             {
                 WriteDiagnosticOnlyAny(builder, value);
                 return;
@@ -627,31 +602,6 @@ namespace Google.Protobuf
             return parts[1];
         }
 
-        /// <summary>
-        /// Appends a number of nanoseconds to a StringBuilder. Either 0 digits are added (in which
-        /// case no "." is appended), or 3 6 or 9 digits.
-        /// </summary>
-        private static void AppendNanoseconds(StringBuilder builder, int nanos)
-        {
-            if (nanos != 0)
-            {
-                builder.Append('.');
-                // Output to 3, 6 or 9 digits.
-                if (nanos % 1000000 == 0)
-                {
-                    builder.Append((nanos / 1000000).ToString("d3", CultureInfo.InvariantCulture));
-                }
-                else if (nanos % 1000 == 0)
-                {
-                    builder.Append((nanos / 1000).ToString("d6", CultureInfo.InvariantCulture));
-                }
-                else
-                {
-                    builder.Append(nanos.ToString("d9", CultureInfo.InvariantCulture));
-                }
-            }
-        }
-
         private void WriteStruct(StringBuilder builder, IMessage message)
         {
             builder.Append("{ ");
@@ -785,7 +735,7 @@ namespace Google.Protobuf
         /// <remarks>
         /// Other than surrogate pair handling, this code is mostly taken from src/google/protobuf/util/internal/json_escaping.cc.
         /// </remarks>
-        private void WriteString(StringBuilder builder, string text)
+        internal static void WriteString(StringBuilder builder, string text)
         {
             builder.Append('"');
             for (int i = 0; i < text.Length; i++)

csharp/src/Google.Protobuf/WellKnownTypes/DurationPartial.cs

@@ -31,12 +31,14 @@
 #endregion
 
 using System;
+using System.Globalization;
+using System.Text;
 
 namespace Google.Protobuf.WellKnownTypes
 {
     // Manually-written partial class for the Duration well-known type,
     // providing a conversion to TimeSpan and convenience operators.
-    public partial class Duration
+    public partial class Duration : ICustomDiagnosticMessage
     {
         /// <summary>
         /// The number of nanoseconds in a second.
@@ -73,7 +75,6 @@ namespace Google.Protobuf.WellKnownTypes
             return Math.Sign(seconds) * Math.Sign(nanoseconds) != -1;
         }
 
-
         /// <summary>
         /// Converts this <see cref="Duration"/> to a <see cref="TimeSpan"/>.
         /// </summary>
@@ -180,5 +181,90 @@ namespace Google.Protobuf.WellKnownTypes
             }
             return new Duration { Seconds = seconds, Nanos = nanoseconds };
         }
+
+        /// <summary>
+        /// Converts a duration specified in seconds/nanoseconds to a string.
+        /// </summary>
+        /// <remarks>
+        /// If the value is a normalized duration in the range described in <c>duration.proto</c>,
+        /// <paramref name="diagnosticOnly"/> is ignored. Otherwise, if the parameter is <c>true</c>,
+        /// a JSON object with a warning is returned; if it is <c>false</c>, an <see cref="InvalidOperationException"/> is thrown.
+        /// </remarks>
+        /// <param name="seconds">Seconds portion of the duration.</param>
+        /// <param name="nanoseconds">Nanoseconds portion of the duration.</param>
+        /// <param name="diagnosticOnly">Determines the handling of non-normalized values</param>
+        /// <exception cref="InvalidOperationException">The represented duration is invalid, and <paramref name="diagnosticOnly"/> is <c>false</c>.</exception>
+        internal static string ToJson(long seconds, int nanoseconds, bool diagnosticOnly)
+        {
+            if (IsNormalized(seconds, nanoseconds))
+            {
+                var builder = new StringBuilder();
+                builder.Append('"');
+                // The seconds part will normally provide the minus sign if we need it, but not if it's 0...
+                if (seconds == 0 && nanoseconds < 0)
+                {
+                    builder.Append('-');
+                }
+
+                builder.Append(seconds.ToString("d", CultureInfo.InvariantCulture));
+                AppendNanoseconds(builder, Math.Abs(nanoseconds));
+                builder.Append("s\"");
+                return builder.ToString();
+            }
+            if (diagnosticOnly)
+            {
+                // Note: the double braces here are escaping for braces in format strings.
+                return string.Format(CultureInfo.InvariantCulture,
+                    "{{ \"@warning\": \"Invalid Duration\", \"seconds\": \"{0}\", \"nanos\": {1} }}",
+                    seconds,
+                    nanoseconds);
+            }
+            else
+            {
+                throw new InvalidOperationException("Non-normalized duration value");
+            }
+        }
+
+        /// <summary>
+        /// Returns a string representation of this <see cref="Duration"/> for diagnostic purposes.
+        /// </summary>
+        /// <remarks>
+        /// Normally the returned value will be a JSON string value (including leading and trailing quotes) but
+        /// when the value is non-normalized or out of range, a JSON object representation will be returned
+        /// instead, including a warning. This is to avoid exceptions being thrown when trying to
+        /// diagnose problems - the regular JSON formatter will still throw an exception for non-normalized
+        /// values.
+        /// </remarks>
+        /// <returns>A string representation of this value.</returns>
+        public string ToDiagnosticString()
+        {
+            return ToJson(Seconds, Nanos, true);
+        }
+
+        /// <summary>
+        /// Appends a number of nanoseconds to a StringBuilder. Either 0 digits are added (in which
+        /// case no "." is appended), or 3 6 or 9 digits. This is internal for use in Timestamp as well
+        /// as Duration.
+        /// </summary>
+        internal static void AppendNanoseconds(StringBuilder builder, int nanos)
+        {
+            if (nanos != 0)
+            {
+                builder.Append('.');
+                // Output to 3, 6 or 9 digits.
+                if (nanos % 1000000 == 0)
+                {
+                    builder.Append((nanos / 1000000).ToString("d3", CultureInfo.InvariantCulture));
+                }
+                else if (nanos % 1000 == 0)
+                {
+                    builder.Append((nanos / 1000).ToString("d6", CultureInfo.InvariantCulture));
+                }
+                else
+                {
+                    builder.Append(nanos.ToString("d9", CultureInfo.InvariantCulture));
+                }
+            }
+        }
     }
 }

csharp/src/Google.Protobuf/WellKnownTypes/FieldMaskPartial.cs

+#region Copyright notice and license
+// Protocol Buffers - Google's data interchange format
+// Copyright 2016 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#endregion
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+
+namespace Google.Protobuf.WellKnownTypes
+{
+    // Manually-written partial class for the FieldMask well-known type.
+    public partial class FieldMask : ICustomDiagnosticMessage
+    {
+        /// <summary>
+        /// Converts a timestamp  specified in seconds/nanoseconds to a string.
+        /// </summary>
+        /// <remarks>
+        /// If the value is a normalized duration in the range described in <c>field_mask.proto</c>,
+        /// <paramref name="diagnosticOnly"/> is ignored. Otherwise, if the parameter is <c>true</c>,
+        /// a JSON object with a warning is returned; if it is <c>false</c>, an <see cref="InvalidOperationException"/> is thrown.
+        /// </remarks>
+        /// <param name="paths">Paths in the field mask</param>
+        /// <param name="diagnosticOnly">Determines the handling of non-normalized values</param>
+        /// <exception cref="InvalidOperationException">The represented duration is invalid, and <paramref name="diagnosticOnly"/> is <c>false</c>.</exception>
+        internal static string ToJson(IList<string> paths, bool diagnosticOnly)
+        {
+            var firstInvalid = paths.FirstOrDefault(p => !ValidatePath(p));
+            if (firstInvalid == null)
+            {
+                var builder = new StringBuilder();
+                JsonFormatter.WriteString(builder, string.Join(",", paths.Select(JsonFormatter.ToCamelCase)));
+                return builder.ToString();
+            }
+            else
+            {
+                if (diagnosticOnly)
+                {
+                    var builder = new StringBuilder();
+                    builder.Append("{ \"@warning\": \"Invalid FieldMask\", \"paths\": ");
+                    JsonFormatter.Default.WriteList(builder, (IList) paths);
+                    builder.Append(" }");
+                    return builder.ToString();
+                }
+                else
+                {
+                    throw new InvalidOperationException($"Invalid field mask to be converted to JSON: {firstInvalid}");
+                }
+            }
+        }
+
+        /// <summary>
+        /// Camel-case converter with added strictness for field mask formatting.
+        /// </summary>
+        /// <exception cref="InvalidOperationException">The field mask is invalid for JSON representation</exception>
+        private static bool ValidatePath(string input)
+        {
+            for (int i = 0; i < input.Length; i++)
+            {
+                char c = input[i];
+                if (c >= 'A' && c <= 'Z')
+                {
+                    return false;
+                }
+                if (c == '_' && i < input.Length - 1)
+                {
+                    char next = input[i + 1];
+                    if (next < 'a' || next > 'z')
+                    {
+                        return false;
+                    }
+                }
+            }
+            return true;
+        }
+
+        /// <summary>
+        /// Returns a string representation of this <see cref="FieldMask"/> for diagnostic purposes.
+        /// </summary>
+        /// <remarks>
+        /// Normally the returned value will be a JSON string value (including leading and trailing quotes) but
+        /// when the value is non-normalized or out of range, a JSON object representation will be returned
+        /// instead, including a warning. This is to avoid exceptions being thrown when trying to
+        /// diagnose problems - the regular JSON formatter will still throw an exception for non-normalized
+        /// values.
+        /// </remarks>
+        /// <returns>A string representation of this value.</returns>
+        public string ToDiagnosticString()
+        {
+            return ToJson(Paths, true);
+        }
+    }
+}

csharp/src/Google.Protobuf/WellKnownTypes/TimestampPartial.cs

@@ -31,10 +31,12 @@
 #endregion
 
 using System;
+using System.Globalization;
+using System.Text;
 
 namespace Google.Protobuf.WellKnownTypes
 {
-    public partial class Timestamp
+    public partial class Timestamp : ICustomDiagnosticMessage
     {
         private static readonly DateTime UnixEpoch = new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
         // Constants determined programmatically, but then hard-coded so they can be constant expressions.
@@ -43,11 +45,11 @@ namespace Google.Protobuf.WellKnownTypes
         internal const long UnixSecondsAtBclMinValue = -BclSecondsAtUnixEpoch;
         internal const int MaxNanos = Duration.NanosecondsPerSecond - 1;
 
-        private bool IsNormalized =>
-            Nanos >= 0 &&
-            Nanos <= MaxNanos &&
-            Seconds >= UnixSecondsAtBclMinValue &&
-            Seconds <= UnixSecondsAtBclMaxValue;
+        private static bool IsNormalized(long seconds, int nanoseconds) =>
+            nanoseconds >= 0 &&
+            nanoseconds <= MaxNanos &&
+            seconds >= UnixSecondsAtBclMinValue &&
+            seconds <= UnixSecondsAtBclMaxValue;
 
         /// <summary>
         /// Returns the difference between one <see cref="Timestamp"/> and another, as a <see cref="Duration"/>.
@@ -111,7 +113,7 @@ namespace Google.Protobuf.WellKnownTypes
         /// incorrectly normalized or is outside the valid range.</exception>
         public DateTime ToDateTime()
         {
-            if (!IsNormalized)
+            if (!IsNormalized(Seconds, Nanos))
             {
                 throw new InvalidOperationException(@"Timestamp contains invalid values: Seconds={Seconds}; Nanos={Nanos}");
             }
@@ -181,5 +183,59 @@ namespace Google.Protobuf.WellKnownTypes
             }
             return new Timestamp { Seconds = seconds, Nanos = nanoseconds };
         }
+
+        /// <summary>
+        /// Converts a timestamp specified in seconds/nanoseconds to a string.
+        /// </summary>
+        /// <remarks>
+        /// If the value is a normalized duration in the range described in <c>timestamp.proto</c>,
+        /// <paramref name="diagnosticOnly"/> is ignored. Otherwise, if the parameter is <c>true</c>,
+        /// a JSON object with a warning is returned; if it is <c>false</c>, an <see cref="InvalidOperationException"/> is thrown.
+        /// </remarks>
+        /// <param name="seconds">Seconds portion of the duration.</param>
+        /// <param name="nanoseconds">Nanoseconds portion of the duration.</param>
+        /// <param name="diagnosticOnly">Determines the handling of non-normalized values</param>
+        /// <exception cref="InvalidOperationException">The represented duration is invalid, and <paramref name="diagnosticOnly"/> is <c>false</c>.</exception>
+        internal static string ToJson(long seconds, int nanoseconds, bool diagnosticOnly)
+        {
+            if (IsNormalized(seconds, nanoseconds))
+            {
+                // Use .NET's formatting for the value down to the second, including an opening double quote (as it's a string value)
+                DateTime dateTime = UnixEpoch.AddSeconds(seconds);
+                var builder = new StringBuilder();
+                builder.Append('"');
+                builder.Append(dateTime.ToString("yyyy'-'MM'-'dd'T'HH:mm:ss", CultureInfo.InvariantCulture));
+                Duration.AppendNanoseconds(builder, nanoseconds);
+                builder.Append("Z\"");
+                return builder.ToString();
+            }
+            if (diagnosticOnly)
+            {
+                return string.Format(CultureInfo.InvariantCulture,
+                    "{{ \"@warning\": \"Invalid Timestamp\", \"seconds\": \"{0}\", \"nanos\": {1} }}",
+                    seconds,
+                    nanoseconds);
+            }
+            else
+            {
+                throw new InvalidOperationException("Non-normalized timestamp value");
+            }
+        }
+
+        /// <summary>
+        /// Returns a string representation of this <see cref="Timestamp"/> for diagnostic purposes.
+        /// </summary>
+        /// <remarks>
+        /// Normally the returned value will be a JSON string value (including leading and trailing quotes) but
+        /// when the value is non-normalized or out of range, a JSON object representation will be returned
+        /// instead, including a warning. This is to avoid exceptions being thrown when trying to
+        /// diagnose problems - the regular JSON formatter will still throw an exception for non-normalized
+        /// values.
+        /// </remarks>
+        /// <returns>A string representation of this value.</returns>
+        public string ToDiagnosticString()
+        {
+            return ToJson(Seconds, Nanos, true);
+        }
     }
 }