-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsFixes #29932 DescriptionAdds Uses Utf8Parser.TryParse & Utf8Formatter.TryFormat for the heavy lifting. Public API Changes public static partial class JsonMetadataServices
{
public static System.Text.Json.Serialization.JsonConverter<System.TimeSpan> TimeSpanConverter { get { throw null; } }
} ConsiderationsThis is using the constant ("c") format. I think the general short format ("g") could compact things a bit, but not sure if that would round-trip with Newtonsoft and it is culture-sensitive. /cc @layomia @eiriktsarpalis @JamesNK
|
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.ReadTests.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.ReadTests.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.WriteTests.cs
Show resolved
Hide resolved
...es/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/TimeSpanConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.ReadTests.cs
Show resolved
Hide resolved
...es/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/TimeSpanConverter.cs
Outdated
Show resolved
Hide resolved
CI test failures are unrelated. @layomia has your feedback been addressed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but we should remove the 'g' format fallback, keeping in line with the implementation for other primitives that only support one popular format (e.g. ISO 8601 for DateTime
). This way there's no confusion about what formats are supported/unsupported. If we get feedback later requesting other formats, we can re-add the fallback(s).
Updated to remove the 'g' fallback. |
...tem.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs
Show resolved
Hide resolved
...es/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/TimeSpanConverter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/Value.ReadTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for removing the fallback. Just a couple minor comments left to address, then this PR is good to merge.
bool result = Utf8Formatter.TryFormat(value, output, out int bytesWritten, 'c'); | ||
Debug.Assert(result); | ||
|
||
writer.WriteStringValue(output.Slice(0, bytesWritten)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @CodeBlanch! LGTM, with a few suggestions for a follow up PR.
Test failures are unrelated to the changes in this PR.
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why delete this helper?
There was a problem hiding this comment.
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?
{ | ||
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]; |
There was a problem hiding this comment.
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!
@@ -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\"" }; |
There was a problem hiding this comment.
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.
@@ -303,6 +306,9 @@ public static void ValueFail() | |||
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<DateTimeOffset>(unexpectedString)); | |||
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<DateTimeOffset?>(unexpectedString)); | |||
|
|||
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<TimeSpan>(unexpectedString)); | |||
Assert.Throws<JsonException>(() => JsonSerializer.Deserialize<TimeSpan?>(unexpectedString)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to other logic, we should be fine, but a few more nullable TimeSpan
tests would be good. E.g. deserializing null
into a non-nullable TimeSpan
should fail, but should be fine for nullable TimeSpan?
.
[MemberData(nameof(JsonDateTimeTestData.InvalidISO8601Tests), MemberType = typeof(JsonDateTimeTestData))] | ||
public static void TryGetDateTime_HasValueSequence_False(string testString) | ||
{ | ||
static void test(string testString, bool isFinalBlock) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
static void test(string testString, bool isFinalBlock) | |
static void Test(string testString, bool isFinalBlock) |
* Follow-up feedback from #54186. * Code review. * Code review. * Dedicated constant for the max number of chars permitted to be allocated on the stack.
* Constant `stackalloc` can be optimized by the JIT. dotnet/runtime#54186 (comment) * Fix ESXXX KID computation cause by incorrect canonicalized JWK size ('crv' was computed as base64-encoded) * `EllipticalCurve` is now a class instead of a readonly struct. This reduce the cost of struct copy.
Fixes #29932
Description
Adds
TimeSpan
support to System.Text.Json JsonSerializer.Uses Utf8Parser.TryParse & Utf8Formatter.TryFormat for the heavy lifting.
Public API Changes
Considerations
This is using the constant ("c") format. I think the general short format ("g") could compact things a bit, but not sure if that would round-trip with Newtonsoft and it is culture-sensitive.
/cc @layomia @eiriktsarpalis @JamesNK