Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added JSON converter for TimeSpan #51492

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
Original file line number Diff line number Diff line change
Expand Up @@ -782,6 +782,7 @@ public static partial class JsonMetadataServices
public static System.Text.Json.Serialization.JsonConverter<sbyte> SByteConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<float> SingleConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<string> StringConverter { get { throw null; } }
public static System.Text.Json.Serialization.JsonConverter<System.TimeSpan> TimeSpanConverter { get { throw null; } }
layomia marked this conversation as resolved.
Show resolved Hide resolved
[System.CLSCompliantAttribute(false)]
public static System.Text.Json.Serialization.JsonConverter<ushort> UInt16Converter { get { throw null; } }
[System.CLSCompliantAttribute(false)]
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,9 @@
<data name="FormatDateTimeOffset" xml:space="preserve">
<value>The JSON value is not in a supported DateTimeOffset format.</value>
</data>
<data name="FormatTimeSpan" xml:space="preserve">
<value>The JSON value is not in a supported TimeSpan format.</value>
</data>
<data name="FormatGuid" xml:space="preserve">
<value>The JSON value is not in a supported Guid format.</value>
</data>
Expand Down
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/src/System.Text.Json.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@
<Compile Include="System\Text\Json\Serialization\Converters\Value\SByteConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\SingleConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\StringConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\TimeSpanConverter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\UInt16Converter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\UInt32Converter.cs" />
<Compile Include="System\Text\Json\Serialization\Converters\Value\UInt64Converter.cs" />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,289 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
using System.Diagnostics;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class TimeSpanConverter : JsonConverter<TimeSpan>
{
private static JsonEncodedText Zero = JsonEncodedText.Encode("PT0S");
YohDeadfall marked this conversation as resolved.
Show resolved Hide resolved
private const ulong TicksPerYear = TimeSpan.TicksPerDay * 365;
private const ulong TicksPerMonth = TimeSpan.TicksPerDay * 30;
Comment on lines +10 to +11
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume these values are convention specified by the duration spec? I couldn't find anything definitive on the matter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this makes sense. A duration of 48 hours is not the same as a duration of 2 days per ISO 8601. Example: adding 48 hours to 2021-11-06 07:00 in North Carolina is not the same as adding 2 days to the same date. Likewise, I would not expect adding 30 days to 2020-02-01 being the same as adding 1 month to the same date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/dotnet/corefx/blob/6d723b8e5ae3129c0a94252292322fc19673478f/src/System.Private.Xml/src/System/Xml/Schema/XsdDuration.cs#L229-L236

Just because we're doing it already doesn't mean it is devoid of flaws 😄. Is this backed by the specification?

Copy link
Contributor Author

@YohDeadfall YohDeadfall Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have no original documents, so can't answer your question, but to correctly handle such things the month and year parts should be avoided, but they still can be received and it will be a valid input. The problem is that it's not compatible with .NET date and time representations.

For example, PostgreSQL has interval which stores a duration in three parts: months, days, microseconds. And to handle arithmetic operations properly it deconstructs a timestamp into parts, adds an interval to it and then assembles a new timestamp again. timestamp is a 64 bit integer measuring microseconds.

Unfortunately, TimeSpan stores ticks and can't be correctly constructed from any input. For that reason it lacks of Years and Months properties.

Therefore, while I agree with @watfordgnf that it's a wrong logic and serialization should not include years and months, deserialization of TimeSpan will cause issues if these parts are present. Surely, I can just throw a NotSupportedException for such cases and it will solve the problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NodaTime provides a pattern for JSON, but have no idea is it a standard or at least wide spread practice:

https://github.com/nodatime/nodatime/blob/af257385d785f597fc8be67c84f2cf714fbe4203/src/NodaTime/Text/DurationPattern.cs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it would be the least confusing for users if TimeSpan was serialized using its native .NET format, rather than a standard it doesn't meet (it represents a subset of it at best). Perhaps not the most convenient when interoperating with other services, but if they expect ISO 8601 durations you couldn't use them reliably anyway (bidirectionally).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After conversation with @layomia we have concluded that ISO 8601 durations is not the appropriate format for encoding TimeSpan. As @watfordgnf points out TimeSpan represents a subset of the standard so deserialization cannot be implemented without substantial sacrifices in precision.

The converter should instead use the standard .NET format for timespan. This has the benefit of being compatible with json.net (which can be an issue with, say, with document databases like cosmos currently using json.net).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should tests look like then? Do the same thing as for DateTime?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think DateTime tests might be a good starting point. Given that the parsing will be pushed out of the json layer the coverage doesn't need to be as extensive.

private const ulong TicksPerDay = (ulong)TimeSpan.TicksPerDay;
private const ulong TicksPerHour = (ulong)TimeSpan.TicksPerHour;
private const ulong TicksPerMinute = (ulong)TimeSpan.TicksPerMinute;
private const ulong TicksPerSecond = (ulong)TimeSpan.TicksPerSecond;

public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
static byte ReadByte(ref ReadOnlySpan<byte> span)
{
if (span.IsEmpty)
{
ThrowFormatException();
}

var result = span[0];
span = span[1..];
YohDeadfall marked this conversation as resolved.
Show resolved Hide resolved

return result;
}

static ulong ReadDigits(ref ReadOnlySpan<byte> span)
{
if (span.IsEmpty)
{
ThrowFormatException();
}

ulong value = 0;
do
{
uint digit = (uint)(span[0] - '0');

if (digit > 9)
{
break;
}

value = value * 10 + digit;
span = span[1..];
}
while (!span.IsEmpty);
return value;
}

var span = reader.ValueSpan;
byte current = ReadByte(ref span);
bool negative = current == '-';
ulong value;

if (negative)
{
current = ReadByte(ref span);
}

if (current != 'P')
{
ThrowFormatException();
}

checked
{
value = ReadDigits(ref span);
current = ReadByte(ref span);

ulong ticks = 0;
if (current == 'Y')
{
ticks += value * TicksPerYear;

if (span.IsEmpty)
{
return Result(ticks);
}

value = ReadDigits(ref span);
current = ReadByte(ref span);
}

if (current == 'M')
{
ticks += value * TicksPerMonth;

if (span.IsEmpty)
{
return Result(ticks);
}

value = ReadDigits(ref span);
current = ReadByte(ref span);
}

if (current == 'D')
{
ticks += value * TicksPerDay;

if (span.IsEmpty)
{
return Result(ticks);
}

current = ReadByte(ref span);
}

bool hasTime = current == 'T';
if (hasTime)
{
value = ReadDigits(ref span);
current = ReadByte(ref span);

if (current == 'H')
{
ticks += value * TicksPerHour;

if (span.IsEmpty)
{
return Result(ticks);
}

value = ReadDigits(ref span);
current = ReadByte(ref span);
}

if (current == 'M')
{
ticks += value * TicksPerMinute;

if (span.IsEmpty)
{
return Result(ticks);
}

value = ReadDigits(ref span);
current = ReadByte(ref span);
}

if (current == 'S')
{
ticks += value * TicksPerSecond;

if (span.IsEmpty)
{
return Result(ticks);
}

current = ReadByte(ref span);
}
}

if (current == '.' ||
current == ',')
{
ulong valueFraction = ReadDigits(ref span);
(ulong ticksPerPart, ulong ticksPerFraction) = ReadByte(ref span) switch
{
(byte)'Y' => (TicksPerYear, TicksPerMonth),
(byte)'M' => hasTime
? (TicksPerMinute, TicksPerSecond)
: (TicksPerMonth, TicksPerDay),
(byte)'D' => (TicksPerDay, TicksPerHour),
(byte)'H' => (TicksPerHour, TicksPerMinute),
(byte)'S' => (TicksPerSecond, 1U),
_ => (0U, 0U),
};

if (span.IsEmpty && ticksPerPart != 0)
{
ulong scale = 10;
while (valueFraction / scale != 0)
{
scale *= 10;
}

ulong ticksFraction = (ulong)(ticksPerPart * ((double)valueFraction / (double)scale));

ticks += (ulong)(ticksFraction / ticksPerFraction) * ticksPerFraction;
ticks += (ulong)(value * ticksPerPart);

return Result(ticks);
}
}
}

ThrowFormatException();
return default;

TimeSpan Result(ulong ticks) => new TimeSpan(negative ? -(long)ticks : (long)ticks);
static void ThrowFormatException() => throw ThrowHelper.GetFormatException(DataType.TimeSpan);
}

public override void Write(Utf8JsonWriter writer, TimeSpan value, JsonSerializerOptions options)
{
static int Write(Span<byte> span, char designator, ref ulong ticks, ulong ticksPerPart)
{
var value = ticks / ticksPerPart;
int written = 0;

if (value != 0)
{
ticks -= value * ticksPerPart;
written = WriteDigits(span, value);
span[written++] = (byte)designator;

return written;
}

return written;
}

static int WriteDigits(Span<byte> span, ulong value)
{
int digits = value switch
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code for this seems currently less efficient: #52730

< 10 => 1,
< 100 => 2,
< 1000 => 3,
< 10000 => 4,
< 100000 => 5,
< 1000000 => 6,
< 10000000 => 7,
_ => 0,
};

int digit = digits;
while (digit-- != 0)
{
ulong temp = value / 10;

span[digit] = (byte)('0' + value - temp * 10);
value = temp;
}

return digits;
}

if (value == TimeSpan.Zero)
{
writer.WriteStringValue(Zero);
return;
}

Span<byte> result = stackalloc byte[32];
var ticks = (ulong)value.Ticks;
var position = 0;

if (ticks > long.MaxValue)
{
result[position++] = (byte)'-';
ticks = (ulong)-value.Ticks;
}

result[position++] = (byte)'P';
position += Write(result[position..], 'Y', ref ticks, TicksPerYear);
position += Write(result[position..], 'M', ref ticks, TicksPerMonth);
position += Write(result[position..], 'D', ref ticks, TicksPerDay);

if (ticks != 0)
{
result[position++] = (byte)'T';
position += Write(result[position..], 'H', ref ticks, TicksPerHour);
position += Write(result[position..], 'M', ref ticks, TicksPerMinute);
position += Write(result[position..], 'S', ref ticks, TicksPerSecond);

if (ticks != 0)
{
for (ulong temp = ticks;
ticks - (temp /= 10) * 10 == 0;
ticks = temp);

result[position - 1] = (byte)'.';
position += WriteDigits(result[position..], ticks);
result[position++] = (byte)'S';
}
}

writer.WriteStringValue(result[0..position]);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ private static Dictionary<Type, JsonConverter> GetDefaultSimpleConverters()
Add(JsonMetadataServices.SByteConverter);
Add(JsonMetadataServices.SingleConverter);
Add(JsonMetadataServices.StringConverter);
Add(JsonMetadataServices.TimeSpanConverter);
Add(JsonMetadataServices.UInt16Converter);
Add(JsonMetadataServices.UInt32Converter);
Add(JsonMetadataServices.UInt64Converter);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,12 @@ public static partial class JsonMetadataServices
public static JsonConverter<string> StringConverter => s_stringConverter ??= new StringConverter();
private static JsonConverter<string>? s_stringConverter;

/// <summary>
/// Returns a <see cref="JsonConverter{T}"/> instance that converts <see cref="TimeSpan"/> values.
/// </summary>
public static JsonConverter<TimeSpan> TimeSpanConverter => s_timeSpanConverter ??= new TimeSpanConverter();
private static JsonConverter<TimeSpan>? s_timeSpanConverter;

/// <summary>
/// Returns a <see cref="JsonConverter{T}"/> instance that converts <see cref="ushort"/> values.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,9 @@ public static FormatException GetFormatException(DataType dateType)
case DataType.DateTimeOffset:
message = SR.FormatDateTimeOffset;
break;
case DataType.TimeSpan:
message = SR.FormatTimeSpan;
break;
case DataType.Base64String:
message = SR.CannotDecodeInvalidBase64;
break;
Expand Down Expand Up @@ -723,6 +726,7 @@ internal enum DataType
Boolean,
DateTime,
DateTimeOffset,
TimeSpan,
Base64String,
Guid,
}
Expand Down
Loading