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

System.Text.Json: Add TimeSpanConverter #54186

Merged
merged 11 commits into from
Jul 7, 2021
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 @@ -900,6 +900,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; } }
[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 @@ -165,6 +165,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
Expand Up @@ -117,12 +117,6 @@ public static bool IsValidDateTimeOffsetParseLength(int length)
return IsInRangeInclusive(length, JsonConstants.MinimumDateTimeParseLength, JsonConstants.MaximumEscapedDateTimeOffsetParseLength);
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool IsValidDateTimeOffsetParseLength(long length)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why delete this helper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't being used anymore so I cleaned it up. The code that was calling it (in Utf8JsonReader.TryGet) is now calling IsInRangeInclusive directly so it can pass in the max length based on encoded string or not. You want me to put it back with the other feedback items for a follow-up?

{
return IsInRangeInclusive(length, JsonConstants.MinimumDateTimeParseLength, JsonConstants.MaximumEscapedDateTimeOffsetParseLength);
}

/// <summary>
/// Parse the given UTF-8 <paramref name="source"/> as extended ISO 8601 format.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1073,25 +1073,25 @@ internal bool TryGetDateTimeCore(out DateTime value)
{
ReadOnlySpan<byte> span = stackalloc byte[0];

int maximumLength = _stringHasEscaping ? JsonConstants.MaximumEscapedDateTimeOffsetParseLength : JsonConstants.MaximumDateTimeOffsetParseLength;

if (HasValueSequence)
{
long sequenceLength = ValueSequence.Length;

if (!JsonHelpers.IsValidDateTimeOffsetParseLength(sequenceLength))
if (!JsonHelpers.IsInRangeInclusive(sequenceLength, JsonConstants.MinimumDateTimeParseLength, maximumLength))
{
value = default;
return false;
}

Debug.Assert(sequenceLength <= JsonConstants.MaximumEscapedDateTimeOffsetParseLength);
Span<byte> stackSpan = stackalloc byte[(int)sequenceLength];

Span<byte> stackSpan = stackalloc byte[_stringHasEscaping ? JsonConstants.MaximumEscapedDateTimeOffsetParseLength : JsonConstants.MaximumDateTimeOffsetParseLength];
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for these changes. Searching for "stackalloc byte" in the STJ.csproj shows there might be a few more places that would benefit from this pattern, particularly in JsonReaderHelper.*.cs. We'd take a follow up PR to address this if you're up for it!

ValueSequence.CopyTo(stackSpan);
span = stackSpan;
span = stackSpan.Slice(0, (int)sequenceLength);
}
else
{
if (!JsonHelpers.IsValidDateTimeOffsetParseLength(ValueSpan.Length))
if (!JsonHelpers.IsInRangeInclusive(ValueSpan.Length, JsonConstants.MinimumDateTimeParseLength, maximumLength))
{
value = default;
return false;
Expand Down Expand Up @@ -1141,25 +1141,25 @@ internal bool TryGetDateTimeOffsetCore(out DateTimeOffset value)
{
ReadOnlySpan<byte> span = stackalloc byte[0];

int maximumLength = _stringHasEscaping ? JsonConstants.MaximumEscapedDateTimeOffsetParseLength : JsonConstants.MaximumDateTimeOffsetParseLength;

if (HasValueSequence)
{
long sequenceLength = ValueSequence.Length;

if (!JsonHelpers.IsValidDateTimeOffsetParseLength(sequenceLength))
if (!JsonHelpers.IsInRangeInclusive(sequenceLength, JsonConstants.MinimumDateTimeParseLength, maximumLength))
{
value = default;
return false;
}

Debug.Assert(sequenceLength <= JsonConstants.MaximumEscapedDateTimeOffsetParseLength);
Span<byte> stackSpan = stackalloc byte[(int)sequenceLength];

Span<byte> stackSpan = stackalloc byte[_stringHasEscaping ? JsonConstants.MaximumEscapedDateTimeOffsetParseLength : JsonConstants.MaximumDateTimeOffsetParseLength];
ValueSequence.CopyTo(stackSpan);
span = stackSpan;
span = stackSpan.Slice(0, (int)sequenceLength);
}
else
{
if (!JsonHelpers.IsValidDateTimeOffsetParseLength(ValueSpan.Length))
if (!JsonHelpers.IsInRangeInclusive(ValueSpan.Length, JsonConstants.MinimumDateTimeParseLength, maximumLength))
{
value = default;
return false;
Expand Down Expand Up @@ -1210,24 +1210,25 @@ internal bool TryGetGuidCore(out Guid value)
{
ReadOnlySpan<byte> span = stackalloc byte[0];

int maximumLength = _stringHasEscaping ? JsonConstants.MaximumEscapedGuidLength : JsonConstants.MaximumFormatGuidLength;

if (HasValueSequence)
{
long sequenceLength = ValueSequence.Length;
if (sequenceLength > JsonConstants.MaximumEscapedGuidLength)
if (sequenceLength > maximumLength)
{
value = default;
return false;
}

Debug.Assert(sequenceLength <= JsonConstants.MaximumEscapedGuidLength);
Span<byte> stackSpan = stackalloc byte[(int)sequenceLength];

Span<byte> stackSpan = stackalloc byte[_stringHasEscaping ? JsonConstants.MaximumEscapedGuidLength : JsonConstants.MaximumFormatGuidLength];
ValueSequence.CopyTo(stackSpan);
span = stackSpan;
span = stackSpan.Slice(0, (int)sequenceLength);
}
else
{
if (ValueSpan.Length > JsonConstants.MaximumEscapedGuidLength)
if (ValueSpan.Length > maximumLength)
{
value = default;
return false;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Buffers;
using System.Buffers.Text;
using System.Diagnostics;

namespace System.Text.Json.Serialization.Converters
{
internal sealed class TimeSpanConverter : JsonConverter<TimeSpan>
{
private const int MinimumTimeSpanFormatLength = 8; // hh:mm:ss
private const int MaximumTimeSpanFormatLength = 26; // -dddddddd.hh:mm:ss.fffffff
private const int MaximumEscapedTimeSpanFormatLength = JsonConstants.MaxExpansionFactorWhileEscaping * MaximumTimeSpanFormatLength;

public override TimeSpan Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
if (reader.TokenType != JsonTokenType.String)
{
throw ThrowHelper.GetInvalidOperationException_ExpectedString(reader.TokenType);
}

bool isEscaped = reader._stringHasEscaping;
int maximumLength = isEscaped ? MaximumEscapedTimeSpanFormatLength : MaximumTimeSpanFormatLength;

ReadOnlySpan<byte> source = stackalloc byte[0];

if (reader.HasValueSequence)
{
ReadOnlySequence<byte> valueSequence = reader.ValueSequence;
long sequenceLength = valueSequence.Length;

if (!JsonHelpers.IsInRangeInclusive(sequenceLength, MinimumTimeSpanFormatLength, maximumLength))
{
throw ThrowHelper.GetFormatException(DataType.TimeSpan);
}

Span<byte> stackSpan = stackalloc byte[isEscaped ? MaximumEscapedTimeSpanFormatLength : MaximumTimeSpanFormatLength];
valueSequence.CopyTo(stackSpan);
source = stackSpan.Slice(0, (int)sequenceLength);
}
else
{
source = reader.ValueSpan;

if (!JsonHelpers.IsInRangeInclusive(source.Length, MinimumTimeSpanFormatLength, maximumLength))
{
throw ThrowHelper.GetFormatException(DataType.TimeSpan);
}
}

if (isEscaped)
{
int backslash = source.IndexOf(JsonConstants.BackSlash);
Debug.Assert(backslash != -1);

Span<byte> sourceUnescaped = stackalloc byte[source.Length];

JsonReaderHelper.Unescape(source, sourceUnescaped, backslash, out int written);
Debug.Assert(written > 0);

source = sourceUnescaped.Slice(0, written);
Debug.Assert(!source.IsEmpty);
}

byte firstChar = source[0];
if (!JsonHelpers.IsDigit(firstChar) && firstChar != '-')
{
// Note: Utf8Parser.TryParse allows for leading whitespace so we
// need to exclude that case here.
throw ThrowHelper.GetFormatException(DataType.TimeSpan);
}

bool result = Utf8Parser.TryParse(source, out TimeSpan tmpValue, out int bytesConsumed, 'c');

// Note: Utf8Parser.TryParse will return true for invalid input so
// long as it starts with an integer. Example: "2021-06-18" or
// "1$$$$$$$$$$". We need to check bytesConsumed to know if the
// entire source was actually valid.

if (result && source.Length == bytesConsumed)
{
return tmpValue;
}

throw ThrowHelper.GetFormatException(DataType.TimeSpan);
}

public override void Write(Utf8JsonWriter writer, TimeSpan value, JsonSerializerOptions options)
{
Span<byte> output = stackalloc byte[MaximumTimeSpanFormatLength];

bool result = Utf8Formatter.TryFormat(value, output, out int bytesWritten, 'c');
Debug.Assert(result);

writer.WriteStringValue(output.Slice(0, bytesWritten));
Copy link
Contributor

@layomia layomia Jul 1, 2021

Choose a reason for hiding this comment

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

[Non-blocking for this PR] When #54254 goes in, we should consider updating this to use the new Utf8JsonWriter.WriteRawValue API so we can avoid logic that checks whether the input payload needs to be escaped. We'd first need to quote the formatted value.

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private void RootBuiltInConverters()

private static Dictionary<Type, JsonConverter> GetDefaultSimpleConverters()
{
const int NumberOfSimpleConverters = 23;
const int NumberOfSimpleConverters = 24;
var converters = new Dictionary<Type, JsonConverter>(NumberOfSimpleConverters);

// Use a dictionary for simple converters.
Expand All @@ -72,6 +72,7 @@ private void RootBuiltInConverters()
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 @@ -110,6 +110,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();
layomia marked this conversation as resolved.
Show resolved Hide resolved
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;
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
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
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ public static IEnumerable<object[]> InvalidISO8601Tests()
yield return new object[] { "\"1997-07-16T19:20:30.4555555+1400\"" };
yield return new object[] { "\"1997-07-16T19:20:30.4555555-1400\"" };


// Proper format but invalid calendar date, time, or time zone designator fields
yield return new object[] { "\"1997-00-16T19:20:30.4555555\"" };
yield return new object[] { "\"1997-07-16T25:20:30.4555555\"" };
Expand All @@ -215,6 +214,7 @@ public static IEnumerable<object[]> InvalidISO8601Tests()
yield return new object[] { "\"1997-07-16T19:20:30.45555555550000000\"" };
yield return new object[] { "\"1997-07-16T19:20:30.45555555555555555\"" };
yield return new object[] { "\"1997-07-16T19:20:30.45555555555555555555\"" };
yield return new object[] { "\"1997-07-16T19:20:30.4555555555555555+01:300\"" };
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 we have DateTimeOffset failure tests for leading whitespace. We'd take a follow up to add these.


// Hex strings

Expand Down
Loading