Unseal JsonStringEnumConverter

[josundt]

Background and Motivation

JsonStringEnumConverter can be configured using a couple of parameters. But since the type is marked sealed, it can be awkward to configure when applied via the JsonConverter attribute, leading to awkward workarounds like this one.

API Proposal

namespace System.Text.Json.Serialization
{
-    public sealed class JsonStringEnumConverter : JsonConverterFactory
+    public class JsonStringEnumConverter : JsonConverterFactory
    {
        public JsonStringEnumConverter() { }
        public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true) { }
-        public override bool CanConvert(Type typeToConvert) { throw null; }
+        public sealed override bool CanConvert(Type typeToConvert) { throw null; }
-        public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) { throw null; }
+        public sealed override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options) { throw null; }
    }
}

Risks

Unsealing public types is generally not considered a breaking change. It might break reflection in the hypothetical scenario where a user depends on the result of typeof(JsonStringEnumConverter).IsSealed.

Original proposal by @josundt (click to view)

In our APIs, we serialize most enums as strings (no case conversion) since string enums make APIs more self-explanatory, but for **flag enums** we serialize with the numeric value since strings would prevent bitwise flag enum operations.

With NewtonSoft.Json, we accomplished the described serialization behavior by creating our own converter that inherited from NewtonSoft's StringEnumConverter.

This would have been possible and even easier with System.Text.Json, if only the JsonStringEnumConverter class was not sealed.

If it was not sealed, I could have done this simply by overriding the CanConvert method like below:

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace MyNamespace
{
    public class JsonStringEnumNumericFlagConverter : JsonStringEnumConverter
    {
        public JsonStringEnumNumericFlagConverter()
            : base() {}

        public JsonStringEnumNumericFlagConverter(JsonNamingPolicy namingPolicy = null, bool allowIntegerValues = true)
            : base(namingPolicy, allowIntegerValues) {}

        public override bool CanConvert(Type typeToConvert)
        {
            return typeToConvert.IsEnum
                && typeToConvert.GetCustomAttributes(typeof(FlagsAttribute), false).Length == 0;
        }
    }
}

Can you please make this extensibility point available ("un-sealing" the class)?

[ahsonkhan]

cc @steveharter, @JeremyKuhne - why was this type sealed? Is the scenario with enum flags not meant to be supported via JsonStringEnumConverter?

[JeremyKuhne]

This was done for performance (and future implementation flexibility). We changed the APIs explicitly so you can compose it if you want to for this exact scenario. dotnet/corefx#39438 There is a specific example in the tests that shows you how to compose.

[ahsonkhan]

Thanks @JeremyKuhne, since there is a viable workaround, marking this as future.

https://github.com/dotnet/corefx/blob/7d905389f94e52c284d985e7bf3158459b30bccb/src/System.Text.Json/tests/Serialization/EnumConverterTests.cs#L158-L167

@josundt, does composition like this work for your scenario (rather than inheritance)?

[josundt]

Seems to be good enough for my requirements.
A limitation with the example above: Since the private JsonStringEnumConverter is static, it uses the default options that comes with the default constructor.
Would it be bad to make it non static and initialize like this instead?

public class NoFlagsStringEnumConverter : JsonConverterFactory
 {
     private JsonStringEnumConverter _stringEnumConverter;

     public NoFlagsStringEnumConverter()
        : this(namingPolicy: null, allowIntegerValues: true)
     {
        // An empty constructor is needed for construction via attributes
     }

     public NoFlagsStringEnumConverter(JsonNamingPolicy namingPolicy = null, bool allowIntegerValues = true)
     {
         this._stringEnumConverter = new JsonStringEnumConverter(namingPolicy, allowIntegerValues);
     }

     public override bool CanConvert(Type typeToConvert)
         => typeToConvert.IsEnum && !typeToConvert.IsDefined(typeof(FlagsAttribute), inherit: false);

     public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
         => s_stringEnumConverter.CreateConverter(typeToConvert, options);
 }

[JeremyKuhne]

Would it be bad to make it non static and initialize like this instead?

That would be fine.

[layomia]

Related: https://github.com/dotnet/corefx/issues/39408, https://github.com/dotnet/corefx/issues/41096

[layomia]

Closing - there's a workaround.