Make JsonConverterFactory.CreateConverter public.

[JeremyKuhne]

Factory based converters can't be composed into other converters as the CreateConverter API isn't public. To build on existing converters they have to be unsealed, which isn't always desirable.

See #30166 where a customer wants to provide custom behavior for string enum parsing.

namespace System.Text.Json.Serialization
{
    public abstract class JsonConverterFactory : JsonConverter
    {
        // Current:
        // protected abstract JsonConverter CreateConverter(Type typeToConvert);
        // Updated:
        public abstract JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options);
    }
}

We can also consider unsealing JsonStringEnumConverter at the cost of perf and future improvements.

cc: @steveharter

[terrajobst]

Video

• Making the API public makes sense.
• However, it seems if the scenario is composition, we should pass in the JsonSerializerOptions so that someone implementing a converter for a generic type, can ask the options for getting a converter for its type arguments.
  • The same logic would also apply to CanConvert(), however, it seems the design seems to be that the answer can be deferred.
  • In fact, it seems GetConverter() could too because the logic can be deferred to JsonConverter<T>.Read and JsonConverter<T>.Write
  • If that's the case the API can be approved.

@JeremyKuhne please take a look.

[steveharter]

Passing in JsonSerializationOptions to CreateConverter() and CanConvert() would allow for a more "dynamic" model to inspect runtime-added converters, but seems like overkill IMO.

I support the original proposal - making CreateConverter public - which allows a more "dynamic" model as well but only to invoke static\known converters (not runtime-added).

[JeremyKuhne]

While we agreed that making it public would be a good thing the question was brought up about composing on currently available converters.

You can check the added converters in the current JsonSerializerOptions.Converters in the Read/Write methods and you can look for any registered converter with JsonSerializerOptions.GetConverter().

It doesn’t make much sense to add the options to CanConvert() when you have a JsonConverter<T> as the JsonConverter<T> is already bound to a type. For the JsonConverterFactory.GetConverter() it could be useful as you are dynamically creating JsonConverter<T> s. You can delegate to other converters in Read/Write, the main advantage to adding the options to GetConverter() is that your delegation will get cached for the given T you’re creating.

The only real super awkward bit we have here is that you cannot compose our built-in converters for any given T. Once a JsonConverter<T> has been selected for a given T, it is cached. Suppose you want to make your own Uri converter that falls back to our built-in converter. The only way to do that now is to create a new JsonSerializerOptions and pull our built-in instance via JsonSerializerOptions.GetConverter() (as you’ll always get back your own registered converter). Talking with @steveharter about this we both think that we should make all of the built-in converters public post 3.0. For most of the converters it isn’t a huge deal as all they do is call the JsonReader/Writer, but for the more complicated ones it would be useful and we should be consistent.