Ensure ReadWithNumberHandling() not called for custom converters of numeric-typed collection elements

[layomia]

Fixes issue in .NET 6.0/master following #41679. The issue is not present in .NET 5.

[jozkee]

Instead of throwing, why not just normally forward the reader into the converter in case that it is not built-in? This implies not calling ReadNumberWithCustomHandling for such case.

[ahsonkhan]

Can we detect this without having to catch the exception?

[steveharter]

I'm inclined to just call the custom converter and if it doesn't know about quoted numbers, then let it throw what it would have otherwise (likely a reader exception re-thrown as JsonException).

[layomia]

Calling the custom converter is fine with me.

[ahsonkhan]

I'm inclined to just call the custom converter and if it doesn't know about quoted numbers, then let it throw what it would have otherwise (likely a reader exception re-thrown as JsonException).

Why? I haven't fully thought through the scenario or use case, so definitely not pushing back, but just curious what is the benefit to the caller? Could such a behavior be confusing or would it, in fact, match their expectation?

[layomia]

Generally, the expectation when users provide custom converters is that they are called. The exception is when JsonIgnoreCondition.WhenWritingNull/Default/IgnoreNullValues is active and the value is null/default.

The initial concern was that it didn't make sense to specify both custom handling and a custom converter for a type/property since the custom handling would not be honored (by the serializer). Thus, logic was added to throw when both custom handling and a custom handling were provided. The fix would be to remove either the custom number handling or the custom converter. The problem with this is highlighted in #42239 (comment).

The current restriction makes it impossible to have a custom converter for a property on a non-owned type which has number handling specified

The downside to removing the restriction on custom converters is that users can now specify an option which will not be honored. The upside is that we maintain the behavior that a custom converter can take over the (de)serialization of any type, even if parts of the object graph is non-owned.

[ahsonkhan]

That makes sense.

The current restriction makes it impossible to have a custom converter for a property on a non-owned type which has number handling specified

This is a great point, which helps motivate the change you are suggesting, for sure. However, doesn't a local attribute have precedence over a global converter?

Have we gotten any customer feedback around this, to validate that the current restriction is problematic?

What is the principles behind when an option is allowed to coincide with converters, and overrides its behavior, and when is it not allowed or ignored, with the converter being prioritized? Can we come up with and articulate a design rationale for why we have certain exceptions (like the ones you listed)? What is common/special about those?

I think it will be valuable to enumerate the design and precedence more broadly, across all options and permutations with converters. Should a local (or maybe even global) custom converter always take precedence, regardless of the other options, aside from converters in higher order of precedence?

[steveharter]

If someone has a custom converter for a number and wants to manually support quoted numbers (by reading or writing JsonTokenType.String) is this possible (with the new number handling option set), or is this now prevented with InvalidOperationException? Since it was possible in the past to do this (AFAIK), we shouldn't close that option IMO just because the new number handling feature is turned on for non-custom converters.

This means that a custom converter for an Int32, for example, would throw if it doesn't support or know about quoted. Likely the exception would be thrown by the reader when the custom converter calls reader.GetInt32() which will be re-thrown as JsonException.

[ahsonkhan]

The issue is not present in .NET 5.

So, is this PR for the 6.0 milestone?

[layomia]

Yes @ahsonkhan, for 6.0.

[layomia]

If someone has a custom converter for a number and wants to manually support quoted numbers (by reading or writing JsonTokenType.String) is this possible (with the new number handling option set), or is this now prevented with InvalidOperationException? Since it was possible in the past to do this (AFAIK), we shouldn't close that option IMO just because the new number handling feature is turned on for non-custom converters.

This means that a custom converter for an Int32, for example, would throw if it doesn't support or know about quoted. Likely the exception would be thrown by the reader when the custom converter calls reader.GetInt32() which will be re-thrown as JsonException.

We were throwing InvalidOperationException when a property or type had JsonNumberHandlingAttribute and there was also a custom converter. The exception would not be thrown when there was no `JsonNumberHandlingAttribute`` on the type/property but there was a converter and number handling specified globally on the options instance.

The current restriction makes it impossible to have a custom converter for a property on a non-owned type which has number handling specified:

public class NonOwnedType
{
    [JsonNumberHandling(JsonNumberHandling.WriteAsString)]
    public int Num { get; set; }
}

JsonSerializer.Serialize(new NonOwnedType()); // Works okay: {"Num":"0"}

var options = new JsonSerializerOptions { Converters = new CustomIntConverter() };

// InvalidOperationException thrown at warm up without workaround except type owner removing attribute on `Num` property,
// or using custom converter for `NonOwnedType` itself.
JsonSerializer.Serialize(new NonOwnedType(), options);

I'm not sure that this will be a popular scenario that we have to fix for 5.0.

I agree we should remove restrictions with custom converters and this feature. If both custom number handling and a custom converter are provided, we'll just call the converters regular Read and Write method, and the converter is free to do whatever. In the future we can provide a way to inspect per-property handling in custom converters, alongside other per-property features. The new commit updates the logic and tests appropriately.

[steveharter]

LGTM with the converter restrictions removed.

[ericstj]

@layomia is this PR still relevant?

[layomia]

@ericstj yes - I'll finish this PR and merge it.

[layomia]

Will circle back to this and merge it.