diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index a14cc263f1eb57..62937bf75a4145 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -536,11 +536,8 @@ The ignore condition 'JsonIgnoreCondition.WhenWritingNull' is not valid on value-type member '{0}' on type '{1}'. Consider using 'JsonIgnoreCondition.WhenWritingDefault'. - - '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}'. - - - 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}'. + + '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}'. 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. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs index 38bd9cb480f8b5..854eb76429c3eb 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs +++ b/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); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs index 4d7d9f40f2c816..4f27bb9e1e55ba 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs +++ b/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(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(MemberInfo memberInfo) where TAttribute : Attribute diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs index 842b5dc3033158..607f627be8e2af 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs +++ b/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] diff --git a/src/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs b/src/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs index f306e3a3287484..12e825616a20d4 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/NumberHandlingTests.cs +++ b/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(() => JsonSerializer.Deserialize(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(() => 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(json); + Assert.Equal(25, obj.Prop); + Assert.Throws(() => 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(() => JsonSerializer.Deserialize(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(() => 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(() => JsonSerializer.Deserialize(json)); + Assert.Throws(() => JsonSerializer.Serialize(new ClassWith_NumberHandlingOn_Type_WithCustomConverter())); } [JsonNumberHandling(JsonNumberHandling.Strict)] @@ -1639,5 +1630,113 @@ public static void JsonNumberHandling_ArgOutOfRangeFail() Assert.Throws( () => new JsonNumberHandlingAttribute((JsonNumberHandling)(8))); } + + [Fact] + public static void InternalCollectionConverter_CustomNumberConverter_GlobalOption() + { + var list = new List { 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(() => JsonSerializer.Serialize(list, options)); + Assert.Equal(25, JsonSerializer.Deserialize>(@"[""1""]", options)[0]); + + var list2 = new List { 1 }; + Assert.Throws(() => JsonSerializer.Serialize(list2, options)); + Assert.Equal(25, JsonSerializer.Deserialize>(@"[""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(@"{""Prop"":[""1""]}", options); + Assert.Equal(25, obj1.Prop[0]); + Assert.Throws(() => JsonSerializer.Serialize(obj1, options)); + + ClassWithDictPropAndAttribute obj2 = JsonSerializer.Deserialize(@"{""Prop"":{""1"":""1""}}", options); + Assert.Equal(25, obj2.Prop[1]); + Assert.Throws(() => JsonSerializer.Serialize(obj2, options)); + } + + private class ClassWithListPropAndAttribute + { + [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)] + public List Prop { get; set; } + } + + private class ClassWithDictPropAndAttribute + { + [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)] + public Dictionary 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(() => JsonSerializer.Deserialize("")); + Assert.Throws(() => JsonSerializer.Serialize(new ClassWithListPropAndAttribute_ConverterOnProp())); + + Assert.Throws(() => JsonSerializer.Deserialize("")); + Assert.Throws(() => JsonSerializer.Serialize(new ClassWithDictPropAndAttribute_ConverterOnProp())); + } + + private class ClassWithListPropAndAttribute_ConverterOnProp + { + [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)] + [JsonConverter(typeof(ListOfIntConverter))] + public List IntProp { get; set; } + } + + private class ClassWithDictPropAndAttribute_ConverterOnProp + { + [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)] + [JsonConverter(typeof(ClassWithDictPropAndAttribute_ConverterOnProp))] + public Dictionary IntProp { get; set; } + } + + public class ListOfIntConverter : JsonConverter> + { + public override List Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotImplementedException(); + public override void Write(Utf8JsonWriter writer, List value, JsonSerializerOptions options) => throw new NotImplementedException(); + } + + public class DictionaryOfNullableIntConverter : JsonConverter> + { + public override Dictionary Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotImplementedException(); + public override void Write(Utf8JsonWriter writer, Dictionary value, JsonSerializerOptions options) => throw new NotImplementedException(); + } + + [Fact] + public static void InternalCollectionConverter_CustomNullableNumberConverter() + { + var dict = new Dictionary { [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(() => JsonSerializer.Serialize(dict, options)); + Assert.Equal(25, JsonSerializer.Deserialize> (@"{""1"":""1""}", options)[1]); + + var obj = JsonSerializer.Deserialize(@"{""Prop"":{""1"":""1""}}", options); + Assert.Equal(25, obj.Prop[1]); + Assert.Throws(() => JsonSerializer.Serialize(obj, options)); + } + + public class ConverterForNullableInt32 : JsonConverter + { + 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(); + } + } } }