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

src/libraries/System.Text.Json/src/Resources/Strings.resx

@@ -536,11 +536,8 @@
   <data name="IgnoreConditionOnValueTypeInvalid" xml:space="preserve">
     <value>The ignore condition 'JsonIgnoreCondition.WhenWritingNull' is not valid on value-type member '{0}' on type '{1}'. Consider using 'JsonIgnoreCondition.WhenWritingDefault'.</value>
   </data>
-  <data name="NumberHandlingConverterMustBeBuiltIn" xml:space="preserve">
-    <value>'JsonNumberHandlingAttribute' cannot be placed on a property, field, or type that is handled by a custom converter. See usage(s) of converter '{0}' on type '{1}'.</value>
-  </data>
-  <data name="NumberHandlingOnPropertyTypeMustBeNumberOrCollection" xml:space="preserve">
-    <value>When 'JsonNumberHandlingAttribute' is placed on a property or field, the property or field must be a number or a collection of numbers. See member '{0}' on type '{1}'.</value>
+  <data name="NumberHandlingOnPropertyInvalid" xml:space="preserve">
+    <value>'JsonNumberHandlingAttribute' is only valid on a number or a collection of numbers when applied to a property or field. See member '{0}' on type '{1}'.</value>
   </data>
   <data name="ConverterCanConvertNullableRedundant" xml:space="preserve">
     <value>The converter '{0}' handles type '{1}' but is being asked to convert type '{2}'. Either create a separate converter for type '{2}' or change the converter's 'CanConvert' method to only return 'true' for a single type.</value>

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -163,7 +163,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
                 // For performance, only perform validation on internal converters on debug builds.
                 if (IsInternalConverter)
                 {
-                    if (state.Current.NumberHandling != null)
+                    if (state.Current.NumberHandling != null && IsInternalConverterForNumberType)
                     {
                         value = ReadNumberWithCustomHandling(ref reader, state.Current.NumberHandling.Value);
                     }
@@ -179,7 +179,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
                     int originalPropertyDepth = reader.CurrentDepth;
                     long originalPropertyBytesConsumed = reader.BytesConsumed;
 
-                    if (state.Current.NumberHandling != null)
+                    if (state.Current.NumberHandling != null && IsInternalConverterForNumberType)
                     {
                         value = ReadNumberWithCustomHandling(ref reader, state.Current.NumberHandling.Value);
                     }

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs

@@ -184,15 +184,10 @@ private void DetermineIgnoreCondition(JsonIgnoreCondition? ignoreCondition, bool
 
         private void DetermineNumberHandling(JsonNumberHandling? parentTypeNumberHandling)
         {
-            bool numberHandlingIsApplicable = ConverterBase.IsInternalConverterForNumberType || TypeIsCollectionOfNumbersWithInternalConverter();
+            bool numberHandlingIsApplicable = NumberHandlingIsApplicable();
 
             if (IsForClassInfo)
             {
-                if (parentTypeNumberHandling != null && !ConverterBase.IsInternalConverter)
-                {
-                    ThrowHelper.ThrowInvalidOperationException_NumberHandlingOnPropertyInvalid(this);
-                }
-
                 if (numberHandlingIsApplicable)
                 {
                     // This logic is to honor JsonNumberHandlingAttribute placed on
@@ -211,62 +206,60 @@ private void DetermineNumberHandling(JsonNumberHandling? parentTypeNumberHandlin
             else
             {
                 Debug.Assert(MemberInfo != null);
-
                 JsonNumberHandlingAttribute? attribute = GetAttribute<JsonNumberHandlingAttribute>(MemberInfo);
-                if (attribute != null && !numberHandlingIsApplicable)
+
+                if (!numberHandlingIsApplicable)
                 {
-                    ThrowHelper.ThrowInvalidOperationException_NumberHandlingOnPropertyInvalid(this);
+                    if (attribute != null)
+                    {
+                        ThrowHelper.ThrowInvalidOperationException_NumberHandlingOnPropertyInvalid(this);
+                    }
                 }
-
-                if (numberHandlingIsApplicable)
+                else
                 {
                     // Priority 1: Get handling from attribute on property or field.
-                    JsonNumberHandling? handling = attribute?.Handling;
+                    NumberHandling = attribute?.Handling;
 
                     // Priority 2: Get handling from attribute on parent class type.
-                    handling ??= parentTypeNumberHandling;
+                    NumberHandling ??= parentTypeNumberHandling;
 
                     // Priority 3: Get handling from JsonSerializerOptions instance.
-                    if (!handling.HasValue && Options.NumberHandling != JsonNumberHandling.Strict)
+                    if (!NumberHandling.HasValue && Options.NumberHandling != JsonNumberHandling.Strict)
                     {
-                        handling = Options.NumberHandling;
+                        NumberHandling = Options.NumberHandling;
                     }
-
-                    NumberHandling = handling;
                 }
             }
         }
 
-        private bool TypeIsCollectionOfNumbersWithInternalConverter()
+        private bool NumberHandlingIsApplicable()
         {
-            if (!ConverterBase.IsInternalConverter ||
-                ((ClassType.Enumerable | ClassType.Dictionary) & ClassType) == 0)
+            Type potentialNumberType;
+
+            if (((ClassType.Enumerable | ClassType.Dictionary) & ClassType) == 0)
             {
-                return false;
+                potentialNumberType = DeclaredPropertyType;
             }
-
-            Type? elementType = ConverterBase.ElementType;
-            Debug.Assert(elementType != null);
-
-            elementType = Nullable.GetUnderlyingType(elementType) ?? elementType;
-
-            if (elementType == typeof(byte) ||
-                elementType == typeof(decimal) ||
-                elementType == typeof(double) ||
-                elementType == typeof(short) ||
-                elementType == typeof(int) ||
-                elementType == typeof(long) ||
-                elementType == typeof(sbyte) ||
-                elementType == typeof(float) ||
-                elementType == typeof(ushort) ||
-                elementType == typeof(uint) ||
-                elementType == typeof(ulong) ||
-                elementType == JsonClassInfo.ObjectType)
+            else
             {
-                return true;
+                Debug.Assert(ConverterBase.ElementType != null);
+                potentialNumberType = ConverterBase.ElementType;
             }
 
-            return false;
+            potentialNumberType = Nullable.GetUnderlyingType(potentialNumberType) ?? potentialNumberType;
+
+            return potentialNumberType == typeof(byte) ||
+                potentialNumberType == typeof(decimal) ||
+                potentialNumberType == typeof(double) ||
+                potentialNumberType == typeof(short) ||
+                potentialNumberType == typeof(int) ||
+                potentialNumberType == typeof(long) ||
+                potentialNumberType == typeof(sbyte) ||
+                potentialNumberType == typeof(float) ||
+                potentialNumberType == typeof(ushort) ||
+                potentialNumberType == typeof(uint) ||
+                potentialNumberType == typeof(ulong) ||
+                potentialNumberType == JsonClassInfo.ObjectType;
         }
 
         public static TAttribute? GetAttribute<TAttribute>(MemberInfo memberInfo) where TAttribute : Attribute

src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs

@@ -244,21 +244,10 @@ public static void ThrowInvalidOperationException_IgnoreConditionOnValueTypeInva
         public static void ThrowInvalidOperationException_NumberHandlingOnPropertyInvalid(JsonPropertyInfo jsonPropertyInfo)
         {
             MemberInfo? memberInfo = jsonPropertyInfo.MemberInfo;
+            Debug.Assert(memberInfo != null);
+            Debug.Assert(!jsonPropertyInfo.IsForClassInfo);
 
-            if (!jsonPropertyInfo.ConverterBase.IsInternalConverter)
-            {
-                throw new InvalidOperationException(SR.Format(
-                    SR.NumberHandlingConverterMustBeBuiltIn,
-                    jsonPropertyInfo.ConverterBase.GetType(),
-                    jsonPropertyInfo.IsForClassInfo ? jsonPropertyInfo.DeclaredPropertyType : memberInfo!.DeclaringType));
-            }
-
-            // This exception is only thrown for object properties.
-            Debug.Assert(!jsonPropertyInfo.IsForClassInfo && memberInfo != null);
-            throw new InvalidOperationException(SR.Format(
-                SR.NumberHandlingOnPropertyTypeMustBeNumberOrCollection,
-                memberInfo.Name,
-                memberInfo.DeclaringType));
+            throw new InvalidOperationException(SR.Format(SR.NumberHandlingOnPropertyInvalid, memberInfo.Name, memberInfo.DeclaringType));
         }
 
         [DoesNotReturn]

src/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs

@@ -1519,40 +1519,31 @@ public class ClassWith_NumberHandlingOn_ObjectProperty
         }
 
         [Fact]
-        public static void Attribute_NotAllowed_On_Property_WithCustomConverter()
+        public static void Attribute_Ignored_On_Property_WithCustomConverter()
         {
-            string json = @"";
-            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<ClassWith_NumberHandlingOn_Property_WithCustomConverter>(json));
-            string exAsStr = ex.ToString();
-            Assert.Contains(typeof(ConverterForInt32).ToString(), exAsStr);
-            Assert.Contains(typeof(ClassWith_NumberHandlingOn_Property_WithCustomConverter).ToString(), exAsStr);
+            string json = @"{""Prop"":1}";
 
-            ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new ClassWith_NumberHandlingOn_Property_WithCustomConverter()));
-            exAsStr = ex.ToString();
-            Assert.Contains(typeof(ConverterForInt32).ToString(), exAsStr);
-            Assert.Contains(typeof(ClassWith_NumberHandlingOn_Property_WithCustomConverter).ToString(), exAsStr);
+            // Ensure custom converter is honored.
+            var obj = JsonSerializer.Deserialize<ClassWith_NumberHandlingOn_Property_WithCustomConverter>(json);
+            Assert.Equal(25, obj.Prop);
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(obj));
         }
 
         public class ClassWith_NumberHandlingOn_Property_WithCustomConverter
         {
             [JsonNumberHandling(JsonNumberHandling.Strict)]
             [JsonConverter(typeof(ConverterForInt32))]
-            public int MyProp { get; set; }
+            public int Prop { get; set; }
         }
 
         [Fact]
-        public static void Attribute_NotAllowed_On_Type_WithCustomConverter()
+        public static void Attribute_Ignored_On_Type_WithCustomConverter()
         {
-            string json = @"";
-            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<ClassWith_NumberHandlingOn_Type_WithCustomConverter>(json));
-            string exAsStr = ex.ToString();
-            Assert.Contains(typeof(ConverterForMyType).ToString(), exAsStr);
-            Assert.Contains(typeof(ClassWith_NumberHandlingOn_Type_WithCustomConverter).ToString(), exAsStr);
+            string json = @"{}";
 
-            ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new ClassWith_NumberHandlingOn_Type_WithCustomConverter()));
-            exAsStr = ex.ToString();
-            Assert.Contains(typeof(ConverterForMyType).ToString(), exAsStr);
-            Assert.Contains(typeof(ClassWith_NumberHandlingOn_Type_WithCustomConverter).ToString(), exAsStr);
+            // Assert regular Read/Write methods on custom converter are called.
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Deserialize<ClassWith_NumberHandlingOn_Type_WithCustomConverter>(json));
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(new ClassWith_NumberHandlingOn_Type_WithCustomConverter()));
         }
 
         [JsonNumberHandling(JsonNumberHandling.Strict)]
@@ -1639,5 +1630,113 @@ public static void JsonNumberHandling_ArgOutOfRangeFail()
             Assert.Throws<ArgumentOutOfRangeException>(
                 () => new JsonNumberHandlingAttribute((JsonNumberHandling)(8)));
         }
+
+        [Fact]
+        public static void InternalCollectionConverter_CustomNumberConverter_GlobalOption()
+        {
+            var list = new List<int> { 1 };
+            var options = new JsonSerializerOptions(s_optionReadAndWriteFromStr)
+            {
+                Converters = { new ConverterForInt32() }
+            };
+
+            // Assert converter methods are called and not Read/WriteWithNumberHandling (which would throw InvalidOperationException).
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(list, options));
+            Assert.Equal(25, JsonSerializer.Deserialize<List<int>>(@"[""1""]", options)[0]);
+
+            var list2 = new List<int?> { 1 };
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(list2, options));
+            Assert.Equal(25, JsonSerializer.Deserialize<List<int?>>(@"[""1""]", options)[0]);
+
+            // Okay to set number handling for number collection property when number is handled with custom converter;
+            // converter Read/Write methods called.
+            ClassWithListPropAndAttribute obj1 = JsonSerializer.Deserialize<ClassWithListPropAndAttribute>(@"{""Prop"":[""1""]}", options);
+            Assert.Equal(25, obj1.Prop[0]);
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(obj1, options));
+
+            ClassWithDictPropAndAttribute obj2 = JsonSerializer.Deserialize<ClassWithDictPropAndAttribute>(@"{""Prop"":{""1"":""1""}}", options);
+            Assert.Equal(25, obj2.Prop[1]);
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(obj2, options));
+        }
+
+        private class ClassWithListPropAndAttribute
+        {
+            [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
+            public List<int> Prop { get; set; }
+        }
+
+        private class ClassWithDictPropAndAttribute
+        {
+            [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
+            public Dictionary<int, int?> Prop { get; set; }
+        }
+
+        [Fact]
+        public static void InternalCollectionConverter_CustomNumberConverter_OnProperty()
+        {
+            // Invalid to set number handling for number collection property when number is handled with custom converter.
+            var ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<ClassWithListPropAndAttribute_ConverterOnProp>(""));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new ClassWithListPropAndAttribute_ConverterOnProp()));
+
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Deserialize<ClassWithDictPropAndAttribute_ConverterOnProp>(""));
+            Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new ClassWithDictPropAndAttribute_ConverterOnProp()));
+        }
+
+        private class ClassWithListPropAndAttribute_ConverterOnProp
+        {
+            [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
+            [JsonConverter(typeof(ListOfIntConverter))]
+            public List<int> IntProp { get; set; }
+        }
+
+        private class ClassWithDictPropAndAttribute_ConverterOnProp
+        {
+            [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
+            [JsonConverter(typeof(ClassWithDictPropAndAttribute_ConverterOnProp))]
+            public Dictionary<int, int?> IntProp { get; set; }
+        }
+
+        public class ListOfIntConverter : JsonConverter<List<int>>
+        {
+            public override List<int> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotImplementedException();
+            public override void Write(Utf8JsonWriter writer, List<int> value, JsonSerializerOptions options) => throw new NotImplementedException();
+        }
+
+        public class DictionaryOfNullableIntConverter : JsonConverter<Dictionary<int, int?>>
+        {
+            public override Dictionary<int, int?> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotImplementedException();
+            public override void Write(Utf8JsonWriter writer, Dictionary<int, int?> value, JsonSerializerOptions options) => throw new NotImplementedException();
+        }
+
+        [Fact]
+        public static void InternalCollectionConverter_CustomNullableNumberConverter()
+        {
+            var dict = new Dictionary<int, int?> { [1] = 1 };
+            var options = new JsonSerializerOptions(s_optionReadAndWriteFromStr)
+            {
+                Converters = { new ConverterForNullableInt32() }
+            };
+
+            // Assert converter methods are called and not Read/WriteWithNumberHandling (which would throw InvalidOperationException).
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(dict, options));
+            Assert.Equal(25, JsonSerializer.Deserialize<Dictionary<int, int?>> (@"{""1"":""1""}", options)[1]);
+
+            var obj = JsonSerializer.Deserialize<ClassWithDictPropAndAttribute>(@"{""Prop"":{""1"":""1""}}", options);
+            Assert.Equal(25, obj.Prop[1]);
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Serialize(obj, options));
+        }
+
+        public class ConverterForNullableInt32 : JsonConverter<int?>
+        {
+            public override int? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+            {
+                return 25;
+            }
+
+            public override void Write(Utf8JsonWriter writer, int? value, JsonSerializerOptions options)
+            {
+                throw new NotImplementedException();
+            }
+        }
     }
 }