Allow custom JsonConverters for base-classes

[Joelius300]

I'm currently trying to migrate from Newtonsoft.Json to System.Text.Json and have to update my special enums (ObjectEnums). The main reason for that is simply because IJSRuntime has been migrated to System.Text.Json and my custom converters for Newtonsoft.Json don't work anymore.

Issue and solution

I have a base class called ObjectEnum. This class can be inherited to get an enum which has backing values of different types. It is used for strongly-typed JS-interops and thus needs a custom serializer.

First of all let's look at ObjectEnum:

public abstract class ObjectEnum
{
    internal object Value { get; }
    
    protected ObjectEnum(object value) => Value = value;

    public override string ToString() => Value.ToString();
    // operator and Equals/GetHashCode implementations omitted
}

Now one of the implementations:

public class SizeEnum : ObjectEnum
{
    // "enum"-values
    public static SizeEnum Auto => new SizeEnum("auto");
    public static SizeEnum Number(int value) => new SizeEnum(value);

    // private constructors
    private SizeEnum(string value) : base(value) { }
    private SizeEnum(int value) : base(value) { }
}

Finally a use case:

public class Config
{
    public SizeEnum Size { get; set; } = SizeEnum.Auto;
}

Now if I just serialize Config without any custom converters I just get an empty object for Size because the property Value in ObjectEnum is not public so nothing is serialized.

With Newtonsoft.Json I had a really simple (write-only for simplicity here) Converter for this:

internal class ObjectEnumConverter : JsonConverter<ObjectEnum>
{
    public sealed override bool CanRead => false;
    public sealed override bool CanWrite => true;

    public sealed override ObjectEnum ReadJson(JsonReader reader, Type objectType, ObjectEnum existingValue, bool hasExistingValue, JsonSerializer serializer)
    {
        throw new NotImplementedException("Don't use me to read JSON");
    }

    public override void WriteJson(JsonWriter writer, ObjectEnum wrapper, JsonSerializer serializer)
    {
        try
        {
            // if it can be written in a single JToken, 
            // json.net understands what type the wrapped object (.Value) is and serializes it accordingly -> correct value and type (eg. bool, string, double)
            writer.WriteValue(wrapper.Value);
        }
        catch (JsonWriterException)
        {
            // if there was an error, try to explicitly serialize it before writing
            // if this also fails just let it bubble up because the developer should not have values in their enum that fail here
            serializer.Serialize(writer, wrapper.Value);
        }
    }
}

I then put a JsonConverterAttribute to the ObjectEnum-class like so:
[Newtonsoft.Json.JsonConverter(typeof(ObjectEnumConverter))]

All classes which derived from ObjectEnum automatically had this converter applied so I did not need to add this converter to any other class or create custom converters for each new enum. After adding the attribute to the class, Config got serialized correctly (see below).

Without attribute/custom serializer

{
    "Size":{}
}

With attribute/custom serializer

{
    "Size":"auto"
}

Now when porting everything to System.Text.Json I implemented a System.Text.Json.JsonConverter for ObjectEnum just like I did with Newtonsoft.Json (which was even easier). Before I show any code, yes, I checked all the Namespaces and ensured that I did not accidentally use Newtonsoft.Json classes if I didn't intend to.

internal class ObjectEnumConverter :  JsonConverter<ObjectEnum>
{
    public sealed override ObjectEnum Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException("Don't use me to read JSON");
    }

    public override void Write(System.Text.Json.Utf8JsonWriter writer, ObjectEnum value, System.Text.Json.JsonSerializerOptions options)
    {
        System.Text.Json.JsonSerializer.Serialize(writer, value.Value, value.Value.GetType(), options);
    }
}

Then I defined an attribute on my ObjectEnum just like I did before with the following code:
[System.Text.Json.Serialization.JsonConverter(typeof(ObjectEnumConverter))]

Testing that resulted in an empty object just like without custom serializer (see above json). I also put a breakpoint in the ObjectEnumConverter.Write method and I can confirm that it never actually got to this converter.

Next thing I tried was applying the attribute to the implementation. This means I put the same attribute-code on my SizeEnum-class.
This resulted in a weird FormatException which you can see below.

System.FormatException

Stacktrace:

   at System.Text.StringBuilder.AppendFormatHelper(IFormatProvider provider, String format, ParamsArray args)
   at System.String.FormatHelper(IFormatProvider provider, String format, ParamsArray args)
   at System.String.Format(String format, Object arg0)
   at System.SR.Format(String resourceFormat, Object p1)
   at System.Text.Json.ThrowHelper.ThrowInvalidOperationException_SerializationConverterOnAttributeNotCompatible(Type classType, PropertyInfo propertyInfo)
   at System.Text.Json.JsonSerializerOptions.GetConverterFromAttribute(JsonConverterAttribute converterAttribute, Type typeToConvert, Type classType, PropertyInfo propertyInfo)
   at System.Text.Json.JsonSerializerOptions.GetConverter(Type typeToConvert)
   at System.Text.Json.JsonClassInfo.GetClassType(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonClassInfo.CreateProperty(Type declaredPropertyType, Type runtimePropertyType, PropertyInfo propertyInfo, Type parentClassType, JsonSerializerOptions options)
   at System.Text.Json.JsonClassInfo.AddProperty(Type propertyType, PropertyInfo propertyInfo, Type classType, JsonSerializerOptions options)
   at System.Text.Json.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type classType)
   at System.Text.Json.WriteStackFrame.Initialize(Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.WriteCore(Utf8JsonWriter writer, PooledByteBufferWriter output, Object value, Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.WriteCore(PooledByteBufferWriter output, Object value, Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.WriteCoreString(Object value, Type type, JsonSerializerOptions options)
   at System.Text.Json.JsonSerializer.Serialize[TValue](TValue value, JsonSerializerOptions options)

Message:

Index (zero based) must be greater than or equal to zero and less than the size of the argument list.

Source:

System.Private.CoreLib

After that I created a new JsonConverter for my SizeEnum. I only had to change the typeparam (and the name) from my ObjectEnumConverter.

internal class SizeEnumConverter : JsonConverter<SizeEnum>
{
    public sealed override SizeEnum Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException("Don't use me to read JSON");
    }

    public override void Write(System.Text.Json.Utf8JsonWriter writer, SizeEnum value, System.Text.Json.JsonSerializerOptions options)
    {
        System.Text.Json.JsonSerializer.Serialize(writer, value.Value, value.Value.GetType(), options);
    }
}

I used this converter in the JsonConverterAttribute on SizeEnum like so:
[System.Text.Json.Serialization.JsonConverter(typeof(SizeEnumConverter))]

This works! But I really don't think we want to persue this. It would be much more practical and nice if you were able to just put one converter for all ObjectEnums on the ObjectEnum-class and this one would be used if there is not a more concrete one (basically the same behaviour like Newtonsoft.Json).

Let me know what you think of this idea, I think it would be a great addition :)

Ps. It also doesn't work if you just use JsonSerializer.Serialize with JsonSerializerOptions where you add a new instance of ObjectEnumConverter to the Converters property. This approach wouldn't work for me anyway since my concern is IJSRuntime which currently doesn't support using your own JsonSerializerOptions.

[ahsonkhan]

Thanks for the detailed write-up and motivating scenario :)

Next thing I tried was applying the attribute to the implementation. This means I put the same attribute-code on my SizeEnum-class.
This resulted in a weird FormatException which you can see below.

As an aside, @martincostello discovered this error as well and it has been fixed: dotnet/corefx#39789
So you should get a more meaningful exception and error message now for incompatible converters.

cc @steveharter

[Joelius300]

Workaround without adding redundant converters

For anyone who currently needs a workaround for this without adding all these additional converters, I might have a solution:

If you make the ObjectEnumConverter generic but use where to restrict to the type you want to convert, like so:

internal class ObjectEnumConverter<TObjEnum> : JsonConverter<TObjEnum>
    where TObjEnum : ObjectEnum
{
    public sealed override TObjEnum Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException("Don't use me to read JSON");
    }

    public override void Write(System.Text.Json.Utf8JsonWriter writer, TObjEnum value, System.Text.Json.JsonSerializerOptions options)
    {
        System.Text.Json.JsonSerializer.Serialize(writer, value.Value, value.Value.GetType(), options);
    }
}

.. you have the possibility to declare the JsonConverterAttribute with the correct generic parameter and it works!

[System.Text.Json.Serialization.JsonConverter(typeof(ObjectEnumConverter<SizeEnum>))]
public class SizeEnum : ObjectEnum 
{
    ...
}

You still need more JsonConverterAttributes than I'd wish for but it's definitely better than adding lots of redundant converters.

[layomia]

The factory pattern is the recommended way to handle polymorphic support - https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-converters-how-to#steps-to-follow-the-factory-pattern. The solution provided here #30427 (comment) is also correct.

Please re-open if you still face problems.

[Joelius300]

Does that mean you'll never support custom converters for base classes? I'm using blazor and currently JsRuntime doesn't support customized serialization so I rely on attributes. As far as I know (I haven't tested it in a while) I can't use a converter factory in combination with the JsonConverterAttribute on open generic types. I think it would be nice to at least allow that or, as first proposed by this issue, allow a JsonConverter for class A (put on class A with a JsonConverterAttribute) to take effect for any class derived from class A that doesn't have a more fitting converter registered.

[layomia]

Given the scenario where you can't easily configure a JsonSerializerOptions instance, it's reasonable to have a way for the attribute to apply to derived types. For now, the work-around is to place the converter attribute on each type.

[eiriktsarpalis]

If I'm understanding the request correctly, it would be to expose an option like the following?

[JsonConverter(typeof(ObjectEnumConverterFactory), isCovariantConverter: true)]
public abstract class ObjectEnum
{
}

public class SizeEnum : ObjectEnum // by default gets handled using a JsonConverter<SizeEnum> instance 
                                   // that is created by ObjectEnumConverterFactory
{
}

I have a few concerns about adopting such a mechanism:

1. Type converters are not covariant in principle: serialization is a covariant operation however deserialization is contravariant. In practice this means that while for most custom converters serialization would work trivially, deserialization would almost invariably lead to runtime type errors. For instance, we use "covariant" converters internally to implement collection serialization: we apply a bunch of tricks including reflection to make deserialization occassionally work, but in many cases we simply have to throw an exception. Newtonsoft.Json converters are not immune from this problem either.
2. Assuming Add AttributeTargets.Interface to JsonConverterAttribute #33112 is implemented, adding "covariant" converter support can result in diamond ambiguities in interface hierarchies. We would probably need to restrict the feature to classes.
3. Our serialization infrastructure works under the assumption that a member of type T is handled by a JsonConverter<T> instance. We would need to restrict the feature to converter factories that generate the right converter types for the right derived types.

In light of the above, I don't think we should be adding this feature. It would serve a corner case with a known workaround and deserialization would be unsound for most use cases.

cc @layomia @steveharter for thoughts.

[eiriktsarpalis]

Triage: as stated above there are too many problems with adding such a feature and it really only serves to address a corner case that already has a known workaround. I'll be closing this issue, feel free to reopen if there is more to discuss about this.