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.

Sign up for GitHub

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

Added JSON converter for TimeSpan #51492

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
36778ed
Added JSON converter for TimeSpan
YohDeadfall Apr 19, 2021
02cefb2
Removed whitespaces
YohDeadfall Apr 19, 2021
6902565
Update src/libraries/System.Text.Json/src/System/Text/Json/Serializat…
YohDeadfall Apr 20, 2021
4ca13f0
Replaced range indexers by slices
YohDeadfall Apr 20, 2021
60fa12f
Fixed modifier order
YohDeadfall Apr 20, 2021
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
289 changes: 289 additions & 0 deletions ...System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/TimeSpanConverter.cs
View file
Open in desktop
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 readonly JsonEncodedText Zero = JsonEncodedText.Encode("PT0S");
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.

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

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 •
edited
Loading

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.Slice(1);

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.Slice(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.Slice(position), 'Y', ref ticks, TicksPerYear);
position += Write(result.Slice(position), 'M', ref ticks, TicksPerMonth);
position += Write(result.Slice(position), 'D', ref ticks, TicksPerDay);

if (ticks != 0)
{
result[position++] = (byte)'T';
position += Write(result.Slice(position), 'H', ref ticks, TicksPerHour);
position += Write(result.Slice(position), 'M', ref ticks, TicksPerMinute);
position += Write(result.Slice(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.Slice(position), ticks);
result[position++] = (byte)'S';
}
}

writer.WriteStringValue(result.Slice(0, position));
}
}
}
1 change: 1 addition & 0 deletions ...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions ....Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs
View file
Open in desktop
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
4 changes: 4 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.cs
View file
Open in desktop
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