Skip to content
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

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

Open
Whathecode opened this issue Mar 15, 2024 · 2 comments

Comments

@Whathecode
Copy link

Whathecode commented Mar 15, 2024

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
Copy link

elgonzo commented Mar 15, 2024

Related to #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
Copy link
Author

Whathecode commented Mar 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants