Added JSON converter for TimeSpan

[YohDeadfall]

Fixes #29932.

[dotnet-issue-labeler]

Note regarding the new-api-needs-documentation label:

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.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fixes #29932.

Author:	YohDeadfall
Assignees:	-
Labels:	area-System.Text.Json, new-api-needs-documentation
Milestone:	-

[eiriktsarpalis]

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

[YohDeadfall]

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

[watfordgnf]

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.

[eiriktsarpalis]

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?

[YohDeadfall]

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.

[YohDeadfall]

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

[watfordgnf]

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).

[eiriktsarpalis]

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).

[YohDeadfall]

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

[eiriktsarpalis]

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.

[danmoseley]

the code for this seems currently less efficient: #52730

[eiriktsarpalis]

Hi @YohDeadfall, what is the progress of this PR? As discussed we would be looking to not format TimeSpan using ISO8601 durations but use the default .NET format instead.

[YohDeadfall]

Hi, I feel burned out, so no progress since the last discussion. I may finish it next week when will have some time off.

[eiriktsarpalis]

That's fine, no pressure. I'll see if I can find some time later to work on your changes.