System.Text.Json - Generic converter type creation does not honour JsonNumberHandling preferences

[storey247]

Description

When using the JsonSerializerOptions.GetConverter(Type) method to generate generic converter types within JsonConverterFactory etc. it would appear that the JsonConverter created does not honour the underlying JsonSerializerOptions.NumberHandling preferences.

This can be most problematic when trying to have a factory for types such as generic collections where you might want to handle numbers that come from both int or string types.

Instead what happens is that InvalidOperationException - Cannot get the value of a token type 'String' as a number. gets thrown and you need to handle this exception with fallback behaviour

Reproduction Steps

public void GenericTypeConverterFunctionsAsExpected()
    {
        var options = new JsonSerializerOptions() { NumberHandling = JsonNumberHandling.AllowReadingFromString };
        var converter = (JsonConverter<int>)options.GetConverter(typeof(int));
        ReadOnlySpan<byte> jsonReadOnlySpan = "{\"x\": \"123\"}"u8;
        var reader = new Utf8JsonReader(jsonReadOnlySpan);
        reader.Read(); // StartObject
        reader.Read(); // PropertyName
        reader.Read(); // Number
        var x = converter.Read(ref reader, typeof(int), options);
        x.Should().Be(123); // this throws InvalidOperationException
    }

Expected behavior

I would expect because the options being used to generate the converter has the NumberHandling set as AllowReadingFromString then the converter generated (in this case Int32Converter) to correctly handle string based numbers in the json payload.

Actual behavior

And InvalidOperationException excpetion is thrown with the error message Cannot get the value of a token type 'String' as a number. gets thrown and you need to handle this exception with fallback behaviour.

Looking into the logic for the converter here it would appear that the base JsonPrimitiveConverter is not correctly persisting the state to the converter and therefore not invoking ReadNumberWithCustomHandling

Regression?

Unsure

Known Workarounds

A possible workaround is to just use standard JsonSerializer.Deserialize techniques however this is not as performant given I have a locally persisted converter as documented here in the documentation for Factory patterns

try
{
    var element = _valueConverter.Read(ref reader, typeof(T), options);
    if (element != null) list.Add(element);
}
catch (Exception ex) when (ex is JsonException or InvalidOperationException)
{
    // try again but will be less performant sadly :(
    // this covers edge cases where the Number is in quotes for example
    try
    {
        var element = JsonSerializer.Deserialize<T>(ref reader, options);
        if (element != null) list.Add(element);
    }
    catch (Exception) when (ex is JsonException or InvalidOperationException)
    {
        // do nothing, skip invalid element
    }

Configuration

NET8 using System.Text.Json v9.0.10

Other information

No response

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

[eiriktsarpalis]

Looking into the logic for the converter here it would appear that the base JsonPrimitiveConverter is not correctly persisting the state to the converter and therefore not invoking ReadNumberWithCustomHandling

The original designers of STJ went intentionally with this approach which is why the Write/ReadNumberWithCustomHandling methods exist. I suspect the motivation was related to performance when number handling isn't turned on, but it's conceivable we could try to make this behave more uniformly provided the performance regressions aren't significant.

EDIT: Actually, it turns out that the internal methods are essential since the public Write/Read methods aren't passing the necessary context needed to determine the number handling of the present property.