JamesNK / Newtonsoft.Json

Json.NET is a popular high-performance JSON framework for .NET
https://www.newtonsoft.com/json
MIT License
10.76k stars 3.25k forks source link

Custom deserializers ignored for KeyValuePair #1371

Open qbast opened 7 years ago

qbast commented 7 years ago

Source/destination types

KeyValuePair<Guid, DateTime>>

Source/destination JSON

{"Key":"fb892248-6d18-44ab-885e-5252dccc68a2","Value":1499541308509}

Expected behavior

I am using custom converter for DateTime object to handle Unix timestamp (using milliseconds instead of seconds) format. It works everywhere, except when date is embedded into KeyValuePair. In this case custom converter is ignored (its CanConvert method is never called).

Actual behavior

Deserialization throws an error with following stacktrace:

at Newtonsoft.Json.JsonTextReader.ReadStringValue(ReadType readType) at Newtonsoft.Json.JsonTextReader.ReadAsDateTime() at Newtonsoft.Json.JsonReader.ReadForType(JsonContract contract, Boolean hasConverter) at Newtonsoft.Json.Converters.KeyValuePairConverter.ReadJson(JsonReader reader, Type objectType, Object existingValue, JsonSerializer serializer) at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.DeserializeConvertable(JsonConverter converter, JsonReader reader, Type objectType, Object existingValue)

I tracked down origin of the problem to fix for issue 1322 . The change in KeyValuePairConverter replaced call to reader.ReadAndAssert(); into reader.ReadForTypeAndAssert(keyContract, false); which explicitly disables all custom converters.

Steps to reproduce


JsonConvert.DeserializeObject<KeyValuePair<Guid, DateTime>>(
                "{\"Key\":\"fb892248-6d18-44ab-885e-5252dccc68a2\",\"Value\":1499541308509}",
                new JsonSerializerSettings
                {
                    Converters = new[] {new JsonDateTimeConverter()}
                });```
wizofaus commented 6 years ago

I've confirmed that change is definitely what's broken the behavior for custom converters on built-in types used to parameterise KeyValuePair<,> (NB it doesn't seem to happen for user-defined types)

The minimum change that will restore this behavior but preserve the 1322 bugfix seems to be to do:

                var hasCustomConverter = JsonSerializer.GetMatchingConverter(serializer.Converters, valueContract.UnderlyingType);
                reader.ReadForTypeAndAssert(valueContract, hasCustomConverter != null);

But I'm fairly sure that doesn't cover quite a few corner cases. Unfortunately the code that occurs while deserializing properties in user-defined types isn't readily accessible to KeyValuePairConverter, so there'd need to be a little bit of refactoring to do this cleanly. There's also the need to consider if the same logic should be used for the Key. I'm not entirely sure why KeyValuePair should be treated as a special case other than the fact it's defined by .NET and hence can't be annotated with custom serialization behaviours. Interestingly, IDictionary<Guid, DateTime> works just fine with a custom converter for DateTime.