-
Notifications
You must be signed in to change notification settings - Fork 21
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
Empty String Conversion to Nullable missing when switching from NodaTime.Serialization.JsonNet to NodaTime.Serialization.SystemTextJson #78
Comments
Thanks for pointing this out. I'm slightly surprised at the old converter behavior, but it would make sense for it to be consistent. I'll get to this when I have a chance. I don't expect it's massively difficult. |
I've investigated this a bit further, and basically it looks like this is a restriction in System.Text.Json - we don't have the opportunity to use the same converter for It's possible that we could have separate converters for nullable and non-nullable types, but that would be fairly ugly for something which I'm not sure was a good design decision in the first place :( |
@jskeet I recently ran into that restriction myself while testing one of the .NET 7 RCs in an app that uses NodaTime and For context, I had a library type, I was able to get things working by defining a Finally, I was able to annotate the NodaTime properties of Console.WriteLine(JsonSerializer.Deserialize(
"""
{
"SomeInstant": "2022-11-18T00:00:00Z",
"SomeNullableInstant": null
}
""",
SomeJsonSerializerContext.Default.SomeClass)); $ dotnet run
Unhandled exception. System.Text.Json.JsonException: The JSON value could not be converted to NodaTime.Instant. Path: $.SomeInstant | LineNumber: 1 | BytePositionInLine: 39.
at System.Text.Json.ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(Type propertyType)
at System.Text.Json.Serialization.Converters.ObjectDefaultConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
at System.Text.Json.Serialization.Converters.JsonMetadataServicesConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
at System.Text.Json.Serialization.JsonConverter`1.TryReadAsObject(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state, Object& value)
at System.Text.Json.Serialization.Converters.LargeObjectWithParameterizedConstructorConverter`1.ReadAndCacheConstructorArgument(ReadStack& state, Utf8JsonReader& reader, JsonParameterInfo jsonParameterInfo)
at System.Text.Json.Serialization.Converters.ObjectWithParameterizedConstructorConverter`1.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
at System.Text.Json.Serialization.JsonConverter`1.TryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, T& value)
at System.Text.Json.Serialization.JsonConverter`1.ReadCore(Utf8JsonReader& reader, JsonSerializerOptions options, ReadStack& state)
at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 utf8Json, JsonTypeInfo jsonTypeInfo, Nullable`1 actualByteCount)
at System.Text.Json.JsonSerializer.ReadFromSpan[TValue](ReadOnlySpan`1 json, JsonTypeInfo jsonTypeInfo)
at Program.<Main>$(String[] args) $ dotnet run -p:DefineConstants=NODA_TIME_JSON_CONVERTER
SomeClass { SomeInstant = 2022-11-18T00:00:00Z, SomeNullableInstant = } Not necessarily saying this is the right approach for something NodaTime would ship in-the-box, but figured I'd share just in case.
|
@austindrenski: Good to know as a workaround, thanks. Given how much it requires going against the framework, I don't want that to be the default behavior. I might look at how users could opt into it though... |
@jskeet Main File for the parsingImportant parts
Nullable ConverterImportant parts
My (simple) Model
My TestprogramInformation
|
Even better:
Main File for the parsing
|
@Cyber1000: If you'd like to create a PR for this, that would be very welcome. #88 shows how I'm planning to make the converters easier to configure. (That's just for Json.NET, but I'd do something equivalent for SystemTextJson.) |
thanks, I'll have a look into this (disclaimer: low on time, may take some time) |
Our project was previously using newtonsoft to serialize/deserialize JSON for MVC and we used the extension method provided in NodaTime.Serialization.JsonNet to configure nodatime. We recently tried switching over to NodaTime.Serialization.SystemTextJson and everything looked great at first, but then we noticed a problem.
Previously, empty strings would be converted to null for nodatime types such as LocalTime? and LocalDate? without any configuration necessary.
With the SystemTextJson extension method (ConfigureForNodaTime), it now fails for us at the model binding layer when encountering an empty string and the entire model returned to the controller is null.
Are there any plans to bring this functionality back to the SystemTextJson converters? I noticed the old converter base class does a sort of conversion for nullable types when an empty string is encountered, but the new one does not do such a conversion.
The text was updated successfully, but these errors were encountered: