corvus-dotnet / Corvus.Extensions.Newtonsoft.Json

Opinionated configuration for Newtonsoft.Json including Microsoft DI support. Sponsored by endjin.
Apache License 2.0
2 stars 0 forks source link

Change the default settings provided by the JsonSerializerSettingsProvider #166

Closed jongeorge1 closed 3 years ago

jongeorge1 commented 3 years ago

As a result of the recent changes to this library to provide support for conversion of IPropertyBag to IReadOnlyDictionary<string, object>, we have encountered some issues due to unexpected behaviour in Json.Net.

Our initial implementation checks each value in the bag to map its token type to a list of recognised values. If any unexpected token types are found during enumeration, an exception is thrown.

We have now discovered that depending on the JsonSerializerSettings in use, there may be token types we weren't expecting - the case we found was when string values are detected to be dates based on the JsonSerializerSettings.DateParseHandling setting. In this case, Json.Net will report the token type as JTokenType.Date, which caused our enumeration code to throw.

We don't want to add an equivalent of DateTime to our list of recognised values, because this "helpful" feature of determining that a string is a date is specific to Newtonsoft.Json - it is not related to any underlying part of the JSON spec, and it isn't present in System.Text.Json. As a result, we have taken the decision to return any "unrecognised" token type as a string rather than throwing an exception, because ultimately any unrecognised token type must be represented in the underlying JSON as a string.

This causes some issues when JsonSerializerSettings.DateParseHandling is set to DateTimeOffset (our current default) or DateTime; attempting to retrieve a value deserialized using these settings as a string results in a date/time string with non-standard formatting (i.e. the source string 2021-02-15T12:00:00.0000Z is returned as 02/15/2021 12:00:00 + 00:00, rather than in its original form.

In order to try and keep the behaviour of the dictionary conversion as predictable as possible, we've modified our JsonSerializerSettingsProvider to set JsonSerializerSettings.DateParseHandling to None. This means Json.Net won't attempt to do any clever conversion by default.

This only affects working with JSON data using the Newtonsoft.Json.Linq namespace - standard serialization/deserialization to/from DateTime and DateTimeOffset is not affected. However, to support backwards compatibility when this is required, we've added the ability to modify the serializer settings created by the JsonSerializerSettingsProvider at the point it's registered.

We've also had to modify the DateTimeOffsetConverter to take account of this, previously it relied on JsonSerializerSettings.DateParseHandling being set to DateTimeOffset; now it can cope with DateTimeOffset or None. It can't cope with DateTime because we believe using the DateTimeOffsetConverter but setting up your JsonSerializerSettings in this way are mutually incompatible.

mwadams commented 3 years ago

I'm happy with this solution - but I think it needs to go into the "standard usage" in the README, to avoid surprises.

What about other "well known" types like Guids?

jongeorge1 commented 3 years ago

Updated the readme.md and added specs to demonstrate the expected behaviour of other "special case" types.