From 1fc485928fc7a6483b700867f1a6cb2acfa8da5d Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Fri, 15 Jan 2016 11:39:27 +0000 Subject: [PATCH] Fixes to JSON timestamp/duration representations --- .../Google.Protobuf.Test/JsonFormatterTest.cs | 28 +++++++++++++++-- .../Google.Protobuf.Test/JsonParserTest.cs | 1 - .../WellKnownTypes/DurationTest.cs | 30 +++++++++++++++---- .../WellKnownTypes/TimestampTest.cs | 23 ++++++++++++++ csharp/src/Google.Protobuf/JsonFormatter.cs | 29 ++++++++++-------- csharp/src/Google.Protobuf/JsonParser.cs | 18 +++++------ .../WellKnownTypes/DurationPartial.cs | 23 ++++++++++++++ .../WellKnownTypes/TimestampPartial.cs | 22 ++++++++++++-- 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 ace70b0045..09308556ad 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,14 +379,19 @@ 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(() => new Duration { Seconds = seconds, Nanos = nanoseconds }.ToString()); + } + [Test] public void DurationField() { diff --git a/csharp/src/Google.Protobuf.Test/JsonParserTest.cs b/csharp/src/Google.Protobuf.Test/JsonParserTest.cs index 790d7e8999..d3c3a489b7 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 36012e63b6..1aa02e16c4 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(() => 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 597539eb0c..84717d665b 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(() => 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 bde179039c..563b834ece 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 0d997a0acf..db601c5784 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 324f48fc05..b8eba9d3a8 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 /// 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; + } + + /// /// Converts this to a . /// /// If the duration is not a precise number of ticks, it is truncated towards 0. /// The value of this duration, as a TimeSpan. + /// This value isn't a valid normalized duration, as + /// described in the documentation. 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 d284acd63f..7c50a3d78c 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; /// /// Returns the difference between one and another, as a . @@ -99,8 +107,14 @@ namespace Google.Protobuf.WellKnownTypes /// value precisely on a second. /// /// This timestamp as a DateTime. + /// The timestamp contains invalid values; either it is + /// incorrectly normalized or is outside the valid range. 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 /// value precisely on a second. /// /// This timestamp as a DateTimeOffset. + /// The timestamp contains invalid values; either it is + /// incorrectly normalized or is outside the valid range. public DateTimeOffset ToDateTimeOffset() { return new DateTimeOffset(ToDateTime(), TimeSpan.Zero);