[System.Text.Json] Support constructor parameters in JsonConverter attribute

[RamjotSingh]

Background and Motivation

JsonConverter attribute allows customizing the json serialization and deserialization process. However, today there is no way to specify constructor arguments that the converter themselves take.

Best example of this is JsonStringEnumConverter which has following constructor

public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true)

It is pretty common to specify that you want enum members to be camel cased. However, using JsonConveter attribute it is not possible to do this, unless you create another converter which sets that by default.

Newtonsoft.Json supports the following attribute constructor allowing parameters to be specified.

public JsonConverterAttribute(Type converterType, params object[] converterParameters)

Proposed API

public JsonConverterAttribute(Type converterType, params object[] converterParameters)

Usage Examples

[JsonConverter(typeof(JsonStringEnumConverter), JsonNamingPolicy.CamelCase, true)]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and Motivation

JsonConverter attribute allows customizing the json serialization and deserialization process. However, today there is no way to specify constructor arguments that the converter themselves take.

Best example of this is JsonStringEnumConverter which has following constructor

public JsonStringEnumConverter(JsonNamingPolicy? namingPolicy = null, bool allowIntegerValues = true)

It is pretty common to specify that you want enum members to be camel cased. However, using JsonConveter attribute it is not possible to do this, unless you create another converter which sets that by default.

Newtonsoft.Json supports the following attribute constructor allowing parameters to be specified.

public JsonConverterAttribute(Type converterType, params object[] converterParameters)

Proposed API

public JsonConverterAttribute(Type converterType, params object[] converterParameters)

Usage Examples

[JsonConverter(typeof(JsonStringEnumConverter), JsonNamingPolicy.CamelCase, true)]

Author:	RamjotSingh
Assignees:	-
Labels:	api-suggestion, area-System.Text.Json, untriaged
Milestone:	-

[FilipToth]

Sounds good. Before I do any thinking about whether this fits in with all the System.Text.Json APIs. There’s a question I have:
What would get thrown if I pass in say, typeof(int)? Or would we just use the default type?

I would maybe consider making that an enum.

[eiriktsarpalis]

It would have been easier to address this particular case if JsonStringEnumConverter had not been marked sealed, but I would say that using a JsonConverterFactory is the recommended approach for working around this issue in the general case:

public class MyStringEnumConverterFactory : JsonConverterFactory
{
    private readonly JsonStringEnumConverter _converter;

    public MyStringEnumConverterFactory() : this(JsonNamingPolicy.CamelCase, allowIntegerValues: true)
    { }

    public MyStringEnumConverterFactory(JsonNamingPolicy namingPolicy, bool allowIntegerValues)
    {
        _converter = new JsonStringEnumConverter(namingPolicy, allowIntegerValues);
    }

    public override bool CanConvert(Type typeToConvert) => _converter.CanConvert(typeToConvert);
    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) => _converter.CreateConverter(typeToConvert, options);
}

[FilipToth]

@eiriktsarpalis
Just to clarify, why did we seal the JsonStringEnumConverter type? I get that there's not really a reason why would we have any other class inherit from the JsonStringEnumConverter type, but is that the only reason for sealing it? Or is there anything glaringly obvious I'm missing? Would there be any risks associated with making that class not sealed?

[eiriktsarpalis]

Just to clarify, why did we seal the JsonStringEnumConverter type? I get that there's not really a reason why would we have any other class inherit from the JsonStringEnumConverter type, but is that the only reason for sealing it?

I don't have historical context on that decision, perhaps @layomia or @steveharter might know. I don't think we should be sealing any public converter types, because it blocks scenaria like the one we're discussing here.

Would there be any risks associated with making that class not sealed?

I don't think so. Technically you would be breaking reflection code such as typeof(JsonStringEnumConverter).IsSealed but that's the only issue I can think of. @bartonjs or @stephentoub might know if there is precedent here.

[stephentoub]

Unsealing a type previously shipped as sealed is not considered to be a breaking change. Sealing a type previously shipped as unsealed is a breaking change.

My preference is always to err on the side of sealing until scenarios present themselves for which derivation is important, at which point effort needs to be invested in making sure the type was fully designed for being derived from, having one or more methods overridden, etc. (Note there are several performance benefits to types being sealed as well.)

[eiriktsarpalis]

FWIW performance should not be a substantial consideration in the case of converter factories, since they are only being used during the converter generation phase.

[RamjotSingh]

@eiriktsarpalis That is precisely what I had done. Here is my implementation at the moment. But it is not ideal to have this unnecessary code in every codebase.

    /// <summary>
    /// Since SystemTextJson at the moment does not support specifying constructor arguments to converters, this converter just
    /// forces <see cref="JsonStringEnumConverter"/> to use camel casing.
    /// </summary>
    public class JsonStringCamelTextEnumConverter : JsonConverterFactory
    {
        private readonly JsonStringEnumConverter jsonStringEnumConverter;

        /// <summary>
        /// Initializes a new instance of the <see cref="JsonStringCamelTextEnumConverter"/> class.
        /// </summary>
        public JsonStringCamelTextEnumConverter()
        {
            this.jsonStringEnumConverter = new JsonStringEnumConverter(
                JsonNamingPolicy.CamelCase,
                allowIntegerValues: true);
        }

        /// <summary>
        /// Determines if the current converter can convert the type.
        /// </summary>
        /// <param name="typeToConvert">Type to convert.</param>
        /// <returns>True if the type can be converted, false otherwise.</returns>
        public override bool CanConvert(Type typeToConvert)
        {
            return this.jsonStringEnumConverter.CanConvert(typeToConvert);
        }

        /// <summary>
        /// Creates a converter to perform conversion.
        /// </summary>
        /// <param name="typeToConvert">Type to convert.</param>
        /// <param name="options">Serializer options to use.</param>
        /// <returns>Json converter instance.</returns>
        public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
        {
            return this.jsonStringEnumConverter.CreateConverter(typeToConvert, options);
        }
    }

[FilipToth]

I don't have historical context on that decision, perhaps @layomia or @steveharter might know. I don't think we should be sealing any public converter types, because it blocks scenaria like the one we're discussing here.

Ok, thanks, I see.

I tried searching for typeof(JsonStringEnumConverter).IsSealed, didn't find any references in the runtime (My vscode has been acting up with large codebases lately), and please note that I didn't search for variations eg.

var type = typeof(JsonStringEnumConverter);
if (type.IsSealed)
    ...

I am not sure how the review team would feel about unsealing the type especially for a minor change like this where we have a workaround as mentioned by @eiriktsarpalis, although a very impractical one.

[eiriktsarpalis]

I'd support making such a change, pending feedback from @steveharter and @layomia .

[ericstj]

Here was the previous request: #30486

[eiriktsarpalis]

I think we could still do this, the existing workaround is needlessly cumbersome.