Add INumberConverter<T> interface into System.Text.Json

[weifenluo]

Background and Motivation

Currently, JsonConverter<T>.ReadNumberWithCustomHandling and WriteNumberWithCustomHandling are internal, which are not callable from custom (de)serializers.

Proposed API

We can add the following INumberConverter<T> interface:

public interface INumberConverter<T>
{
    T ReadNumber(ref Utf8JsonReader reader, JsonNumberHandling handling);
    void WriteNumber(Utf8JsonWriter writer, T value, JsonNumberHandling handling);
}

and converters which override ReadNumberWithCustomHandling and WriteNumberWithCustomHandling should implement this interface, such as Int32Converter.

Usage Examples

In custom (de)serializers, we can invoke converters with extra JsonNumberHandling parameter, for example:

WriteValue(Utf8JsonWriter writer, T value, Converter<T> converter, JsonNumberHandling handling)
{
    if (converter is INumberConverter<T> numberConverter)
        numberConverter.WriteNumber(writer, value, handling);
    ...
}

Alternative Designs

I can't figure out any other way for custom (de)serializers to consume converter with an extra JsonNumberHandling parameter.

Risks

This is an addition to existing API with a simple interface implementation, the risk should be minimal, if any.

[ghost]

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

Issue Details

---

Background and Motivation

Currently, JsonConverter<T>.ReadNumberWithCustomHandling and WriteNumberWithCustomHandling are internal, which are not callable from custom (de)serializers.

Proposed API

We can add the following INumberConverter<T> interface:

public interface INumberConverter<T>
{
    T ReadNumber(ref Utf8JsonReader reader, JsonNumberHandling handling);
    void WriteNumber(Utf8JsonWriter writer, T value, JsonNumberHandling handling);
}

and converters which override ReadNumberWithCustomHandling and WriteNumberWithCustomHandling should implement this interface, such as Int32Converter.

Usage Examples

In custom (de)serializers, we can invoke converters with extra JsonNumberHandling parameter, for example:

WriteValue(Utf8JsonWriter writer, T value, Converter<T> converter, JsonNumberHandling handling)
{
    if (converter is INumberConverter<T> numberConverter)
        numberConverter.WriteNumber(writer, value, handler);
    ...
}

Alternative Designs

I can't figure out any other way for custom (de)serializers to consume converter with an extra JsonNumberHandling parameter.

Risks

This is an addition to existing API with a simple interface implementation, the risk should be minimal, if any.

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

[layomia]

What kind of custom logic would you like to implement in these methods that are not handled in the default implementations for the various internal number converters? Just trying to understand the motivation here.

[weifenluo]

There is no custom logic I would like to implement in these methods that are not handled in the default implementations for the various internal number converters. Instead I would like to consume the default implementations for the various internal number converters.

The (only) reason converters exist, is to be consumed by (de)serializers, either built-in or custom. Currently, JsonConverter<T>.ReadNumberWithCustomHandling and JsonConverter<T>.WriteNumberWithCustomHandling are internal, like a "backdoor" to built-in (de)serializers only. This is an anti-pattern IMO.

Long story short, I'm now developing (de)serializers for something like System.Data.DataTable and System.Data.DataRow, which self-contains the metadata. These (de)serializers bypasses JsonClassInfo and JsonPropertyInfo, implement its own state (ReadStack/WriteStack), and read/write JSON via converters directly. I'm expecting framework like System.Text.Json, should treat built-in and custom (de)serializers equally.

BTW, there is another "backdoor", JsonConverter<T>.TryRead and JsonConverter<T>.TryWrite, which handles state/continuation of (de)serialization. I understand this is a design decision in favor of performance, to choose the state (ReadStack/WriteStack) as ref struct, over the usability/extensibility. This, however, makes converters work for simple data type only.

There are two extensibility point of System.Text.Json: custom converter and custom (de)serializers utilizing low level Utf8JsonReader and Utf8JsonWriter. However, when coming across custom complex objects, none of the options works satisfactorily.

[weifenluo]

I really hope this can be added to a planned release. It's easy to implement, and has no risk IMO.

[weifenluo]

Here is the workaround by using reflection (hope the implementation will not change!):

using System;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Text.Json;
using System.Text.Json.Serialization;

namespace ...
{
    // There is no interface to expose number handling for built-in converters:
    // https://github.com/dotnet/runtime/issues/47689
    // We have to use reflection to call:
    // JsonConverter.IsInternalConverterForNumberType, JsonConverter<T>.ReadNumberWithCustomHandling
    // and JsonConverter<T>.WriteNumberWithCustomHandling.
    internal static class JsonConverterNumberHandling
    {
        internal static readonly Func<JsonConverter, bool> IsInternalConverterForNumberTypeGetter = BuildIsInternalConverterForNumberTypeGetter();

        private static Func<JsonConverter, bool> BuildIsInternalConverterForNumberTypeGetter()
        {
            var fieldInfo = typeof(JsonConverter).GetField(nameof(IsInternalConverterForNumberType), BindingFlags.NonPublic | BindingFlags.Instance)!;

            var paramJsonConverter = Expression.Parameter(typeof(JsonConverter));
            var expr = Expression.Field(paramJsonConverter, fieldInfo);
            return Expression.Lambda<Func<JsonConverter, bool>>(expr, paramJsonConverter).Compile();
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static bool IsInternalConverterForNumberType(this JsonConverter jsonConverter)
        {
            return IsInternalConverterForNumberTypeGetter(jsonConverter);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static T ReadNumberWithCustomHandling<T>(this JsonConverter<T> jsonConverter, ref Utf8JsonReader reader, JsonNumberHandling handling)
        {
            return JsonConverterNumberHandling<T>.ReadNumberWithCustomHandling(jsonConverter, ref reader, handling);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        private static void WriteNumberWithCustomHandling<T>(this JsonConverter<T> jsonConverter, Utf8JsonWriter writer, T value, JsonNumberHandling handling)
        {
            JsonConverterNumberHandling<T>.WriteNumberWithCustomHandling(jsonConverter, writer, value, handling);
        }

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static void WriteValue<T>(this JsonConverter<T> jsonConverter, Utf8JsonWriter writer, T value, JsonSerializerOptions options, JsonNumberHandling? handling)
        {
            if (handling.HasValue)
                jsonConverter.WriteNumberWithCustomHandling(writer, value, handling.Value);
            else
                jsonConverter.Write(writer, value, options);
        }
    }

    internal static class JsonConverterNumberHandling<T>
    {
        internal delegate T ReadNumberWithCustomHandlingDelegate(JsonConverter<T> jsonConverter, ref Utf8JsonReader utf8JsonReader, JsonNumberHandling numberHandling);

        internal static readonly ReadNumberWithCustomHandlingDelegate ReadNumberWithCustomHandling = BuildReadNumberWithCustomHandling();
        internal static readonly Action<JsonConverter<T>, Utf8JsonWriter, T, JsonNumberHandling> WriteNumberWithCustomHandling = BuildWriteNumberWithCustomHandling();

        private static ReadNumberWithCustomHandlingDelegate BuildReadNumberWithCustomHandling()
        {
            var methodInfo = typeof(JsonConverter<T>).GetMethod(nameof(ReadNumberWithCustomHandling), BindingFlags.Instance | BindingFlags.NonPublic)!;
            var paramJsonConverter = Expression.Parameter(typeof(JsonConverter<T>));
            var paramUtf8Reader = Expression.Parameter(typeof(Utf8JsonReader).MakeByRefType());
            var paramNumberHandling = Expression.Parameter(typeof(JsonNumberHandling));
            var expr = Expression.Call(paramJsonConverter, methodInfo, paramUtf8Reader, paramNumberHandling);
            return Expression.Lambda<ReadNumberWithCustomHandlingDelegate>(expr, paramJsonConverter, paramUtf8Reader, paramNumberHandling).Compile();
        }

        private static Action<JsonConverter<T>, Utf8JsonWriter, T, JsonNumberHandling> BuildWriteNumberWithCustomHandling()
        {
            var methodInfo = typeof(JsonConverter<T>).GetMethod(nameof(WriteNumberWithCustomHandling), BindingFlags.Instance | BindingFlags.NonPublic)!;
            var paramJsonConverter = Expression.Parameter(typeof(JsonConverter<T>));
            var paramUtf8Writer = Expression.Parameter(typeof(Utf8JsonWriter));
            var paramValue = Expression.Parameter(typeof(T));
            var paramNumberHandling = Expression.Parameter(typeof(JsonNumberHandling));
            var expr = Expression.Call(paramJsonConverter, methodInfo, paramUtf8Writer, paramValue, paramNumberHandling);
            return Expression.Lambda<Action<JsonConverter<T>, Utf8JsonWriter, T, JsonNumberHandling>>(expr, paramJsonConverter, paramUtf8Writer, paramValue, paramNumberHandling).Compile();
        }
    }
}

[eiriktsarpalis]

I believe this might be possible to address by exposing the OnTryWrite and OnTryRead methods, something we are considering for 7.0.0 (related to the wider theme of #36785). This would require exposing the WriteStack and ReadStack types and also the NumberHandling field as a read-only property.

I don't believe we should add an INumberConverter<T> interface. Exposing the specific WriteNumberWithCustomHandling/ReadNumberWithCustomHandling seems like a very niche use case, and we should instead be focusing on making the converter model more composable.

[weifenluo]

NumberHandling and WriteStack/ ReadStack are two different things at different layer of the serialization tree: NumberHandling is the leaf, whereas WriteStack/ReadStack are branches.

I don't agree this is a very niche use case. This is required by any serious custom serializer which bypasses the standard WriteStack/ReadStack.

[eiriktsarpalis]

NumberHandling and WriteStack/ ReadStack are two different things at different layer of the serialization tree: NumberHandling is the leaf, whereas WriteStack/ReadStack are branches.

That's not quite right, WriteStack and ReadStack are stacks and as such they do expose the NumberHandling specific to the current node being serialized. I should have probably clarified that the current actual location of the property is WriteStack.Current.NumberHandling.

[weifenluo]

What I meant is that NumberHandling is used to read/write values, whereas WriteStack/ReadStack is used to process the structure of the serialization. A custom serializer may implement its own structural logic, but it should reuse the existing, lower-level NumberHandling.

[eiriktsarpalis]

A custom serializer may implement its own structural logic, but it should reuse the existing, lower-level NumberHandling.

I'm not sure I understand. Do you have a use case where the proposed methods would get called by something other than the JsonSerializer infrastructure?

[weifenluo]

A custom serializer may implement its own structural logic, but it should reuse the existing, lower-level NumberHandling.

I'm not sure I understand. Do you have a use case where the proposed methods would get called by something other than the JsonSerializer infrastructure?

Yes, I've done developing a custom serializer totally has nothing to do with the standard JsonSerializer, targeting data objects similar to System.Data's DataRow and DataTable. These data objects contains its own metadata to process the structure. This issue and #50629 are two true blockers. For #50629, I chose to give up showing LineNumber and BytePositionInLine in the exception message; for this issue, I have to use reflection workaround as posted earlier.

[eiriktsarpalis]

So presumably you're looking for a way to reuse all the primitive number converters on top of your custom serializer implementation? That seems like a niche use case to me, it's unlikely we'd add more methods to JsonConverter<T> just to accommodate that. If you're writing a new serializer, I would probably recommend using a custom converter type that matches your requirements.

[weifenluo]

I'm not sure I'm the only one that have to write a custom serializer. Apparently STJ should not assume the top-level object can only be POCO or collection, which is supported by the standard JsonSerializer infrastructure. Your recommendation is not acceptable because custom serializer and custom converter type are two different things at different layer, again.

[devigo]

I would like to deserialize an empty string to a null value for all Nullable<T> value types using System.Text.Json when JsonSerializerOptions.NumberHandling is set to JsonNumberHandling.AllowReadingFromString.
To do this ("" -> null), I implemented a custom NullableConverterFactory and NullableConverter<T> in .NET Core 3.1, but I faced with an issue for numbers represented as a string in .NET 5. To fix this, as I understood it, I need access to JsonConverter.IsInternalConverterForNumberType, JsonConverter<T>.ReadNumberWithCustomHandling and JsonConverter<T>.WriteNumberWithCustomHandling.
So far I'm using the workaround by using reflection #47689 (comment)
Could you make the listed methods at least protected or point out the correct way to implement the desired behavior?

demo: https://dotnetfiddle.net/4bWxDM
version for .NET Core 3.1

using System.Diagnostics;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.CompilerServices;

namespace System.Text.Json.Serialization
{
    // https://stackoverflow.com/questions/65022834/how-to-deserialize-an-empty-string-to-a-null-value-for-all-nullablet-value-t
    public class NullableConverterFactory : JsonConverterFactory
    {
        public override bool CanConvert(Type typeToConvert)
        {
            return Nullable.GetUnderlyingType(typeToConvert) != null;
        }

        public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
        {
            Debug.Assert(typeToConvert.GetGenericArguments().Length > 0);

            var valueTypeToConvert = typeToConvert.GetGenericArguments()[0];

            var valueConverter = options.GetConverter(valueTypeToConvert);
            Debug.Assert(valueConverter != null);

            return (JsonConverter)Activator.CreateInstance(
                type: typeof(NullableConverter<>).MakeGenericType(valueTypeToConvert),
                bindingAttr: BindingFlags.Instance | BindingFlags.Public,
                binder: null,
                args: new object[] { valueConverter },
                culture: null);
        }

        private class NullableConverter<T> : JsonConverter<T?>
            where T : struct
        {
            private readonly JsonConverter<T> _converter;

            public NullableConverter(JsonConverter<T> converter)
            {
                _converter = converter;
            }

            public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType == JsonTokenType.Null)
                {
                    return null;
                }
                // DESERIALIZE AN EMPTY STRING TO A NULL VALUE
                if (reader.TokenType == JsonTokenType.String)
                {
                    var s = reader.GetString();
                    if (string.IsNullOrEmpty(s))
                        return null;
                }

                if (_converter != null)
                {
                    return ReadValue(_converter, ref reader, typeof(T), options, options.NumberHandling);
                }

                // fallback
                return JsonSerializer.Deserialize<T>(ref reader, options);
            }

            public override void Write(Utf8JsonWriter writer, T? value, JsonSerializerOptions options)
            {
                if (value == null)
                {
                    writer.WriteNullValue();
                }
                else if (_converter != null)
                {
                    WriteValue(_converter, writer, value.Value, options, options.NumberHandling);
                }
                else
                {
                    // fallback
                    JsonSerializer.Serialize(writer, value.Value, options);
                }
            }

            private static T ReadValue(JsonConverter<T> converter, ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, JsonNumberHandling? handling)
            {
                return converter.Read(ref reader, typeToConvert, options);
            }

            private static void WriteValue(JsonConverter<T> converter, Utf8JsonWriter writer, T value, JsonSerializerOptions options, JsonNumberHandling? handling)
            {
                converter.Write(writer, value, options);
            }
        }
    }
}

version for .NET 5 supporting JsonNumberHandling.AllowReadingFromString

using System.Diagnostics;
using System.Linq.Expressions;
using System.Reflection;
using System.Runtime.CompilerServices;

namespace System.Text.Json.Serialization
{
    // https://stackoverflow.com/questions/65022834/how-to-deserialize-an-empty-string-to-a-null-value-for-all-nullablet-value-t
    public class NullableConverterFactory : JsonConverterFactory
    {
        public override bool CanConvert(Type typeToConvert)
        {
            return Nullable.GetUnderlyingType(typeToConvert) != null;
        }

        public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializerOptions options)
        {
            Debug.Assert(typeToConvert.GetGenericArguments().Length > 0);

            var valueTypeToConvert = typeToConvert.GetGenericArguments()[0];

            var valueConverter = options.GetConverter(valueTypeToConvert);
            Debug.Assert(valueConverter != null);

            return (JsonConverter)Activator.CreateInstance(
                type: typeof(NullableConverter<>).MakeGenericType(valueTypeToConvert),
                bindingAttr: BindingFlags.Instance | BindingFlags.Public,
                binder: null,
                args: new object[] { valueConverter },
                culture: null);
        }

        private class NullableConverter<T> : JsonConverter<T?>
            where T : struct
        {
            private readonly JsonConverter<T> _converter;

            public NullableConverter(JsonConverter<T> converter)
            {
                _converter = converter;
            }

            public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
            {
                if (reader.TokenType == JsonTokenType.Null)
                {
                    return null;
                }
                // DESERIALIZE AN EMPTY STRING TO A NULL VALUE
                if (reader.TokenType == JsonTokenType.String)
                {
                    var s = reader.GetString();
                    if (string.IsNullOrEmpty(s))
                        return null;
                }

                if (_converter != null)
                {
                    return ReadValue(_converter, ref reader, typeof(T), options, options.NumberHandling);
                }

                // fallback
                return JsonSerializer.Deserialize<T>(ref reader, options);
            }

            public override void Write(Utf8JsonWriter writer, T? value, JsonSerializerOptions options)
            {
                if (value == null)
                {
                    writer.WriteNullValue();
                }
                else if (_converter != null)
                {
                    WriteValue(_converter, writer, value.Value, options, options.NumberHandling);
                }
                else
                {
                    // fallback
                    JsonSerializer.Serialize(writer, value.Value, options);
                }
            }

            private static T ReadValue(JsonConverter<T> converter, ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, JsonNumberHandling? handling)
            {
+               if (handling.HasValue && converter.IsInternalConverterForNumberType())
+               {
+                   return converter.ReadNumberWithCustomHandling(ref reader, handling.Value);
+               }
+               else
+               {
                    return converter.Read(ref reader, typeToConvert, options);
+               }
            }

            private static void WriteValue(JsonConverter<T> converter, Utf8JsonWriter writer, T value, JsonSerializerOptions options, JsonNumberHandling? handling)
            {
+               if (handling.HasValue && converter.IsInternalConverterForNumberType())
+               {
+                   converter.WriteNumberWithCustomHandling(writer, value, handling.Value);
+               }
+               else
+               {
                    converter.Write(writer, value, options);
+               }
            }
        }
    }
+
+    // There is no interface to expose number handling for built-in converters:
+    // https://github.com/dotnet/runtime/issues/47689
+    // We have to use reflection to call:
+    // JsonConverter.IsInternalConverterForNumberType, JsonConverter<T>.ReadNumberWithCustomHandling
+    // and JsonConverter<T>.WriteNumberWithCustomHandling.
+    internal static class JsonConverterNumberHandling
+    {
+        internal static readonly Func<JsonConverter, bool> IsInternalConverterForNumberTypeGetter = BuildIsInternalConverterForNumberTypeGetter();
+
+        private static Func<JsonConverter, bool> BuildIsInternalConverterForNumberTypeGetter()
+        {
+            var fieldInfo = typeof(JsonConverter).GetField(nameof(IsInternalConverterForNumberType), BindingFlags.NonPublic | BindingFlags.Instance);
+
+            var paramJsonConverter = Expression.Parameter(typeof(JsonConverter));
+            var expr = Expression.Field(paramJsonConverter, fieldInfo);
+            return Expression.Lambda<Func<JsonConverter, bool>>(expr, paramJsonConverter).Compile();
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static bool IsInternalConverterForNumberType(this JsonConverter converter)
+        {
+            return IsInternalConverterForNumberTypeGetter(converter);
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static T ReadNumberWithCustomHandling<T>(this JsonConverter<T> converter, ref Utf8JsonReader reader, JsonNumberHandling handling)
+        {
+            return JsonConverterNumberHandling<T>.ReadNumberWithCustomHandling(converter, ref reader, handling);
+        }
+
+        [MethodImpl(MethodImplOptions.AggressiveInlining)]
+        public static void WriteNumberWithCustomHandling<T>(this JsonConverter<T> converter, Utf8JsonWriter writer, T value, JsonNumberHandling handling)
+        {
+            JsonConverterNumberHandling<T>.WriteNumberWithCustomHandling(converter, writer, value, handling);
+        }
+    }
+
+    internal static class JsonConverterNumberHandling<T>
+    {
+        internal delegate T ReadNumberWithCustomHandlingDelegate(JsonConverter<T> converter, ref Utf8JsonReader reader, JsonNumberHandling numberHandling);
+        internal delegate void WriteNumberWithCustomHandlingDelegate(JsonConverter<T> converter, Utf8JsonWriter writer, T value, JsonNumberHandling numberHandling);
+
+        internal static readonly ReadNumberWithCustomHandlingDelegate ReadNumberWithCustomHandling = BuildReadNumberWithCustomHandling();
+        internal static readonly WriteNumberWithCustomHandlingDelegate WriteNumberWithCustomHandling = BuildWriteNumberWithCustomHandling();
+
+        private static ReadNumberWithCustomHandlingDelegate BuildReadNumberWithCustomHandling()
+        {
+            var methodInfo = typeof(JsonConverter<T>).GetMethod(nameof(ReadNumberWithCustomHandling), BindingFlags.Instance | BindingFlags.NonPublic);
+            var paramConverter = Expression.Parameter(typeof(JsonConverter<T>));
+            var paramReader = Expression.Parameter(typeof(Utf8JsonReader).MakeByRefType());
+            var paramNumberHandling = Expression.Parameter(typeof(JsonNumberHandling));
+            var expr = Expression.Call(paramConverter, methodInfo, paramReader, paramNumberHandling);
+            return Expression.Lambda<ReadNumberWithCustomHandlingDelegate>(expr, paramConverter, paramReader, paramNumberHandling).Compile();
+        }
+
+        private static WriteNumberWithCustomHandlingDelegate BuildWriteNumberWithCustomHandling()
+        {
+            var methodInfo = typeof(JsonConverter<T>).GetMethod(nameof(WriteNumberWithCustomHandling), BindingFlags.Instance | BindingFlags.NonPublic);
+            var paramConverter = Expression.Parameter(typeof(JsonConverter<T>));
+            var paramWriter = Expression.Parameter(typeof(Utf8JsonWriter));
+            var paramValue = Expression.Parameter(typeof(T));
+            var paramNumberHandling = Expression.Parameter(typeof(JsonNumberHandling));
+            var expr = Expression.Call(paramConverter, methodInfo, paramWriter, paramValue, paramNumberHandling);
+            return Expression.Lambda<WriteNumberWithCustomHandlingDelegate>(expr, paramConverter, paramWriter, paramValue, paramNumberHandling).Compile();
+        }
+    }
}

[Neme12]

Why not add a generic converter for anything that is IUtf8Parsable and IUtf8Formattable?