JamesNK / Newtonsoft.Json

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

JsonConverter for struct not used for nullable fields when passed through JsonSerializerSettings #2939

Open Whathecode opened 4 months ago

Whathecode commented 4 months ago

Source/destination types

public struct Test
{
    public int A;
    public int B;
}

public class Nested
{
    public Test? TestField;
}

public class TestConverter : JsonConverter<Test>
{
    public override Test ReadJson(JsonReader reader, Type objectType, Test existingValue, bool hasExistingValue,
        JsonSerializer serializer)
    {
        int a = reader.ReadAsInt32() ?? throw new InvalidOperationException();
        int b = reader.ReadAsInt32() ?? throw new InvalidOperationException();

        return new Test { A = a, B = b };
    }

    public override void WriteJson(JsonWriter writer, Test value, JsonSerializer serializer)
    {
        writer.WriteStartArray();
        writer.WriteValue(value.A);
        writer.WriteValue(value.B);
        writer.WriteEndArray();
    }
}

Source/destination JSON

{"TestField":{"A":42,"B":0}}

Expected behavior

When passing TestConverter through JsonSerializersSettings, as per the code in "steps to reproduce", for the serialized value to show up as:

{"TestField":[42,0]}

Things work as expected when applying [JsonConverter(typeof(TestConverter))] to struct Test, but that is not an option for external types.

Actual behavior

The custom JsonConverter goes unused. This line in the library seems to return null, which is where I would expect the registered converter to be used.

Steps to reproduce

var test = new Nested { TestField = new Test { A = 42 } };

var settings = new JsonSerializerSettings() { Converters = new List<JsonConverter> { new TestConverter() } };
var serialized = JsonConvert.SerializeObject(test, settings);
elgonzo commented 4 months ago

Related to https://github.com/JamesNK/Newtonsoft.Json/issues/2245. As commented on in that issue, the workaround for this limitation is using the non-generic JsonConverter base class and overriding the CanConvert method in your converter and compare the provided type object against both Test and Nullable<Test>. If you need to do this with several nullable value types, it is probably advisable to create an intermediate generic abstract class inheriting from JsonConverter class which would then serve as generic base class for the various nullable-capable json converters for desired value types.

Also keep in mind that WriteJson method of your converter would need to deal with Nullable\<T> instances representing null in whatever way is desired, with the ReadJson method likely also needing to deal with the json input possibly containing null values instead of data for a Test instance.

(P.S. I am just a user and not associated with the Newtonsoft.Json project)

Whathecode commented 4 months ago

Related to #2245.

@elgonzo

Yes and no. (I actually read this issue before posting.)

Yes, in the responses the OP seems to basically suggest the desired behavior I outline here. The behavior which works, but only in case the converter is registered through an attribute. So maybe this has been implemented since, or the OP was unaware.

No, in that the OP started from the outset with a broader (non-generic) converter, and incorrectly implemented CanConvert.

The discrepancy in behavior between the two ways of registering is what I primarily want to highlight here. Documentation-wise (or the limited source code I browsed), I see no indication this is intentional. But, it strikes me as extremely odd (and even unlikely) such an old established, widely used, library would contain a bug in such a core feature.

Out-of-the-box support for things like this is why serialization frameworks exist. 🤔 So I still feel I may be overlooking something.