Fixes to JSON timestamp/duration representations

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 ace70b00..09308556 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,15 +379,20 @@ 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<InvalidOperationException>(() => new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString());
+        }
+
+        [Test]
         public void DurationField()
         {
             var message = new TestWellKnownTypes { DurationField = new Duration() };
diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs
index 790d7e89..d3c3a489 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 36012e63..1aa02e16 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<InvalidOperationException>(() => 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 597539eb..84717d66 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<InvalidOperationException>(() => 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 bde17903..563b834e 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 0d997a0a..db601c57 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 324f48fc..b8eba9d3 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
         /// </summary>
         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;
+        }
+
+
         /// <summary>
         /// Converts this <see cref="Duration"/> to a <see cref="TimeSpan"/>.
         /// </summary>
         /// <remarks>If the duration is not a precise number of ticks, it is truncated towards 0.</remarks>
         /// <returns>The value of this duration, as a <c>TimeSpan</c>.</returns>
+        /// <exception cref="InvalidOperationException">This value isn't a valid normalized duration, as
+        /// described in the documentation.</exception>
         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 d284acd6..7c50a3d7 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;
 
         /// <summary>
         /// Returns the difference between one <see cref="Timestamp"/> and another, as a <see cref="Duration"/>.
@@ -99,8 +107,14 @@ namespace Google.Protobuf.WellKnownTypes
         /// <see cref="DateTime"/> value precisely on a second.
         /// </remarks>
         /// <returns>This timestamp as a <c>DateTime</c>.</returns>
+        /// <exception cref="InvalidOperationException">The timestamp contains invalid values; either it is
+        /// incorrectly normalized or is outside the valid range.</exception>
         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
         /// <see cref="DateTimeOffset"/> value precisely on a second.
         /// </remarks>
         /// <returns>This timestamp as a <c>DateTimeOffset</c>.</returns>
+        /// <exception cref="InvalidOperationException">The timestamp contains invalid values; either it is
+        /// incorrectly normalized or is outside the valid range.</exception>
         public DateTimeOffset ToDateTimeOffset()
         {
             return new DateTimeOffset(ToDateTime(), TimeSpan.Zero);