-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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] Utf8JsonReader/Writer should support TimeSpan #32965
Comments
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I went through System.Text.Json and added System.Text.Json full
|
FWIW I'm not convinced that we should be adding |
Good point @eiriktsarpalis. I think in this case since DateTime & DateTimeOffset are there, and TimeSpan is a closely related type, it might be worth doing. System.Buffers.Text.Utf8Parser/Utf8Formatter has all three, not sure the reasoning behind that. PS: How did Guid get in there? 🤣 |
JsonConverter and sourcegen support was implemented by #54186. At this stage we won't be considering adding TimeSpan support at the Utf8JsonReader/Utf8JsonWriter layer but we may reconsider in the future. Closing this issue for now. |
from @blankensteiner:
As explained in the related issue #32429, I'm currently writing my own (de)serializer because I need to support creating the object without calling a constructor (using System.Runtime.Serialization.FormatterServices.GetUninitializedObject) and getting/setting private and read-only fields (using System.Reflection.FieldInfo.SetValue).
Just like JsonSerializer and JsonDocument, I am building on top of Utf8JsonReader and Utf8JsonWriter.
That decision was made because it's encouraged:
and because I want to generate the exact same JSON and if possible also respect all options (JsonWriterOptions, JsonReaderOptions, JsonSerializerOptions).
However, I find that I'm not able to just focus on mapping from C# fields to JSON properties (and vise versa), because I have to duplicate functionality/behavior. I think these methods are missing from the current API:
In my opinion, it's the domain of Utf8JsonReader and Utf8JsonWriter to know how to (de)serialize the primitive types to whatever data types JSON happens to support. They already know how to do this for most types, but inconsistently TimeSpan is left out. By that logic (the arguments for not adding it), I don't see why DateTime and DateTimeOffset are handled in the reader and writer, but not TimeSpan? The challenge is the same: We have a C#-type and a JSON string, do some magic.
In .NET Core 3.1 a TimeSpan is mapped to an object, but in .NET 5 that will change as TimeSpans are mapped to strings. That is an implementation I have to duplicate in my serializer. If that logic was instead pushed down into Utf8JsonReader and Utf8JsonWriter, all users (and people like me creating serializers) could be free'ed from dealing with that lower-level logic.
We should just pass through the wish to get a TimeSpan and not care what JSON data type the authors of Utf8JsonReader/Writer chose for it and what mapping logic there currently is.
The text was updated successfully, but these errors were encountered: