Newtonsoft.Json icon indicating copy to clipboard operation
Newtonsoft.Json copied to clipboard

JsonConverter<T> should allow override CanConvert method

Open heku opened this issue 11 months ago • 2 comments

I have a very common use case that deserialize a JSON array to a base class array, for example.

    public abstract class A { }
    public class B : A { public int ValueB { get; set; } }
    public class C : A { public int ValueC { get; set; } }

var json = """
    [
        {"ValueB":1},
        {"ValueC":2}
    ]
    """;
var array = JsonConvert.DeserializeObject<A[]>(json, new AJsonConverter());

To do this, I implement a JsonConverter for abstract class A

    public class AJsonConverter : JsonConverter<A>
    {
        public override A ReadJson(JsonReader reader, Type objectType, A existingValue, bool hasExistingValue, JsonSerializer serializer)
        {
            var json = JObject.Load(reader);
            if (json.ContainsKey("ValueB")) return json.ToObject<B>(serializer);
            if (json.ContainsKey("ValueC")) return json.ToObject<C>(serializer);
            throw new NotSupportedException();
        }

        public override void WriteJson(JsonWriter writer, A value, JsonSerializer serializer) 
             => throw new NotImplementedException();
    }

The code above always throw a stack overflow exception, after digging into the source, I found the root cause is JsonConverter<A> implements the bool CanConvert(Type objectType) like typeof(T).IsAssignableFrom(objectType), this causes json.ToObject<B>(serializer) ran into AJsonConverter again. Obviously, the most straightforward way is override the bool CanConvert(Type objectType) method to typeof(T) == objectType, however it is sealed, why it's sealed?

I know I can inherit from the non-generic JsonConverter instead, and to this simple example, removing the serializer parameter from json.ToObject<B>(serializer) also works. But it seems unreasonble to mark the CanConvert method sealed.

heku avatar May 15 '25 14:05 heku

No, it should not. Here's why:

public class AJsonConverter : JsonConverter<A>
{
    public override bool CanConvert(Type t) => true;
}

Clearly, having a converter constrained to type A should not provide means for the converter to say "Hey, i am good for converting type FooBar. Oh, i can also convert int. I'm excellent like that...!" Tell me, how would you pass an existing int or a FooBar instance into the converter's ReadJson/WriteJson methods then? I mean, just take a look at the method signatures of the overridable JsonConverter<T> ReadJson/WriteJson methods. That's why the method is sealed.

Well, you could argue that after determining that a given type is derived from A, CanConvert could invoke another (protected) overridable method that determines if that given derived type is applicable to the converter -- and there would be merit to that argument. However, i would encourage you to peruse the commit history of the project -- you might notice that there is/was only a minuscule development activity in the recent years, so in my subjective opinion it is very unlikely that any new feature or feature enhancement will see the light of day...

As to your problem, JsonConverter<T> is a convenience class on top of the non-generic JsonConverter. If the generic JsonConverter<T> doesn't suit your specific needs, use the non-generic JsonConverter as basis for your custom converter.

elgonzo avatar May 15 '25 18:05 elgonzo

I don't believe your example sufficiently justifies why the method must be sealed. A more reasonable approach would be to treat such cases as incorrect implementations rather than restricting the API. After all, users can always provide wrong implementations for any method if they choose to. like

class FooJsonConverter:JsonConverter  {
   CanRead  =>  true;
   ReadJson(...)  => throw new NotImplementedException();
}

heku avatar May 16 '25 01:05 heku