Fixes to JSON timestamp/duration representations

4 files changed, 66 insertions, 26 deletions

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);