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.

[eiriktsarpalis]

Reopening following conversation from #54187.

[eiriktsarpalis]

@steveharter, @JeremyKuhne why was this type sealed?

This was done for performance

How so? The public type itself is a converter factory so any perf impact would probably only impact the initial converter generation.

[JeremyKuhne]

How so? The public type itself is a converter factory so any perf impact would probably only impact the initial converter generation.

It was a general API best practice. Virtuals make it harder to change internal implementation details- when something can't be derived we can make assumptions and/or take shortcuts we can't otherwise do when we're sealed. FileStream comes to mind as a concrete example of where we suffered for it not being sealed (being forced to call methods such as ReadByte() that weren't strictly necessary as derived classes might depend on it).

Your factory point is valid, of course, and the team can certainly decide to unseal this if it is felt to give significant value.

[eiriktsarpalis]

Good point about changing virtuals; I have updated the OP so that virtual members are explicitly sealed.

[layomia]

Since the motivating scenario behind unsealing this type is to effectively pass state through it's constructor when using JsonConverterAttribute, I just want to point out that JsonConstructorAttribute is unsealed and exposes a virtual CreateConverter method which allows users to create converter instances using parameterized ctors. Here's an example in our tests. It is an imperative approach, whereas attribute usages are usually declarative, but I thought it was worth mentioning.

[terrajobst]

• Looks good as proposed

namespace System.Text.Json.Serialization
{
    // Unsealed
    public class JsonStringEnumConverter : JsonConverterFactory
    {
        // Existing:
        // public JsonStringEnumConverter();
        // public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true);

        // Existing methods, now sealed
        public sealed override bool CanConvert(Type typeToConvert);
        public sealed override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options);
    }
}