From d6ea15882439be9cb739e6b26e2322021cbaf708 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Tue, 17 Aug 2021 13:35:10 -0500 Subject: [PATCH] Number handling with converters (#57525) Co-authored-by: Layomi Akinrinade --- .../System.Text.Json/src/Resources/Strings.resx | 7 +- .../Text/Json/Serialization/JsonConverterOfT.cs | 4 +- .../Serialization/Metadata/JsonPropertyInfo.cs | 40 ++- .../System/Text/Json/ThrowHelper.Serialization.cs | 17 +- .../tests/Common/TestClasses/TestClasses.cs | 2 +- .../Serialization/NumberHandlingTests.cs | 276 ++++++++++++++++++--- 6 files changed, 262 insertions(+), 84 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index b3c5cc4..7fc1bd8 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -518,11 +518,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 b222942..e152005 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 @@ -170,7 +170,7 @@ namespace System.Text.Json.Serialization // 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, options); } @@ -186,7 +186,7 @@ namespace System.Text.Json.Serialization 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, options); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index b296200..0ae3d81 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -271,34 +271,32 @@ namespace System.Text.Json.Serialization.Metadata return true; } + Type potentialNumberType; if (!ConverterBase.IsInternalConverter || ((ConverterStrategy.Enumerable | ConverterStrategy.Dictionary) & ConverterStrategy) == 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 == JsonTypeInfo.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 == JsonTypeInfo.ObjectType; } internal 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 04da3b9..9de23e8 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 @@ -239,21 +239,10 @@ namespace System.Text.Json public static void ThrowInvalidOperationException_NumberHandlingOnPropertyInvalid(JsonPropertyInfo jsonPropertyInfo) { MemberInfo? memberInfo = jsonPropertyInfo.MemberInfo; + Debug.Assert(memberInfo != null); + Debug.Assert(!jsonPropertyInfo.IsForTypeInfo); - if (!jsonPropertyInfo.ConverterBase.IsInternalConverter) - { - throw new InvalidOperationException(SR.Format( - SR.NumberHandlingConverterMustBeBuiltIn, - jsonPropertyInfo.ConverterBase.GetType(), - jsonPropertyInfo.IsForTypeInfo ? jsonPropertyInfo.DeclaredPropertyType : memberInfo!.DeclaringType)); - } - - // This exception is only thrown for object properties. - Debug.Assert(!jsonPropertyInfo.IsForTypeInfo && 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/Common/TestClasses/TestClasses.cs b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.cs index 9f423cf..674f902 100644 --- a/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.cs +++ b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.cs @@ -1880,7 +1880,7 @@ namespace System.Text.Json.Serialization.Tests public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options) { - throw new NotImplementedException(); + throw new NotImplementedException("Converter was called"); } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NumberHandlingTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NumberHandlingTests.cs index d89b36a..c2ec425 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NumberHandlingTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NumberHandlingTests.cs @@ -1428,7 +1428,7 @@ namespace System.Text.Json.Serialization.Tests var obj = new AttributeAppliedToFirstLevelProp { - NestedClass = new BadProperty { MyInt = 1 } + NestedClass = new NonNumberType { MyInt = 1 } }; Assert.Equal(@"{""NestedClass"":{""MyInt"":1}}", JsonSerializer.Serialize(obj)); } @@ -1436,10 +1436,10 @@ namespace System.Text.Json.Serialization.Tests [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)] public class AttributeAppliedToFirstLevelProp { - public BadProperty NestedClass { get; set; } + public NonNumberType NestedClass { get; set; } } - public class BadProperty + public class NonNumberType { public int MyInt { get; set; } } @@ -1483,64 +1483,56 @@ namespace System.Text.Json.Serialization.Tests } [Fact] - [ActiveIssue("Need to tweak number handling option registration following code-gen support.")] - public static void Attribute_NotAllowed_On_NonNumber_NonCollection_Property() + public static void Attribute_Allowed_On_NonNumber_NonCollection_Property() { - string json = @""; - InvalidOperationException ex = Assert.Throws(() => JsonSerializer.Deserialize(json)); - string exAsStr = ex.ToString(); - Assert.Contains("MyProp", exAsStr); - Assert.Contains(typeof(ClassWith_NumberHandlingOn_ObjectProperty).ToString(), exAsStr); + const string Json = @"{""MyProp"":{""MyInt"":1}}"; - ex = Assert.Throws(() => JsonSerializer.Serialize(new ClassWith_NumberHandlingOn_ObjectProperty())); - exAsStr = ex.ToString(); - Assert.Contains("MyProp", exAsStr); - Assert.Contains(typeof(ClassWith_NumberHandlingOn_ObjectProperty).ToString(), exAsStr); + ClassWith_NumberHandlingOn_ObjectProperty obj = JsonSerializer.Deserialize(Json); + Assert.Equal(1, obj.MyProp.MyInt); + + string json = JsonSerializer.Serialize(obj); + Assert.Equal(Json, json); } public class ClassWith_NumberHandlingOn_ObjectProperty { [JsonNumberHandling(JsonNumberHandling.Strict)] - public BadProperty MyProp { get; set; } + public NonNumberType MyProp { get; set; } } [Fact] - [ActiveIssue("Need to tweak number handling option registration following code-gen support.")] - public static void Attribute_NotAllowed_On_Property_WithCustomConverter() + public static void Attribute_Allowed_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}"; + + // Converter returns 25 regardless of input. + var obj = JsonSerializer.Deserialize(json); + Assert.Equal(25, obj.Prop); - 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); + // Converter throws this exception regardless of input. + NotImplementedException ex = Assert.Throws(() => JsonSerializer.Serialize(obj)); + Assert.Equal("Converter was called", ex.Message); } public class ClassWith_NumberHandlingOn_Property_WithCustomConverter { [JsonNumberHandling(JsonNumberHandling.Strict)] [JsonConverter(typeof(ConverterForInt32))] - public int MyProp { get; set; } + public int Prop { get; set; } } [Fact] - [ActiveIssue("Need to tweak number handling option registration following code-gen support.")] - public static void Attribute_NotAllowed_On_Type_WithCustomConverter() + public static void Attribute_Allowed_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 = @"{}"; + NotImplementedException ex; - 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. + ex = Assert.Throws(() => JsonSerializer.Deserialize(json)); + Assert.Equal("Converter was called", ex.Message); + + ex = Assert.Throws(() => JsonSerializer.Serialize(new ClassWith_NumberHandlingOn_Type_WithCustomConverter())); + Assert.Equal("Converter was called", ex.Message); } [JsonNumberHandling(JsonNumberHandling.Strict)] @@ -1553,12 +1545,12 @@ namespace System.Text.Json.Serialization.Tests { public override ClassWith_NumberHandlingOn_Type_WithCustomConverter Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { - throw new NotImplementedException(); + throw new NotImplementedException("Converter was called"); } public override void Write(Utf8JsonWriter writer, ClassWith_NumberHandlingOn_Type_WithCustomConverter value, JsonSerializerOptions options) { - throw new NotImplementedException(); + throw new NotImplementedException("Converter was called"); } } @@ -1576,7 +1568,8 @@ namespace System.Text.Json.Serialization.Tests Assert.Equal(25, JsonSerializer.Deserialize(json, options)); // Converter throws this exception regardless of input. - Assert.Throws(() => JsonSerializer.Serialize(4, options)); + NotImplementedException ex = Assert.Throws(() => JsonSerializer.Serialize(4, options)); + Assert.Equal("Converter was called", ex.Message); json = @"""NaN"""; @@ -1627,6 +1620,207 @@ namespace System.Text.Json.Serialization.Tests Assert.Throws( () => new JsonNumberHandlingAttribute((JsonNumberHandling)(8))); } + + [Fact] + public static void InternalCollectionConverter_CustomNumberConverter_GlobalOption() + { + NotImplementedException ex; + + 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). + // Converter returns 25 regardless of input. + Assert.Equal(25, JsonSerializer.Deserialize>(@"[""1""]", options)[0]); + // Converter throws this exception regardless of input. + ex = Assert.Throws(() => JsonSerializer.Serialize(list, options)); + Assert.Equal("Converter was called", ex.Message); + + var list2 = new List { 1 }; + Assert.Equal(25, JsonSerializer.Deserialize>(@"[""1""]", options)[0]); + ex = Assert.Throws(() => JsonSerializer.Serialize(list2, options)); + Assert.Equal("Converter was called", ex.Message); + + // 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]); + ex = Assert.Throws(() => JsonSerializer.Serialize(obj1, options)); + Assert.Equal("Converter was called", ex.Message); + + ClassWithDictPropAndAttribute obj2 = JsonSerializer.Deserialize(@"{""Prop"":{""1"":""1""}}", options); + Assert.Equal(25, obj2.Prop[1]); + ex = Assert.Throws(() => JsonSerializer.Serialize(obj2, options)); + Assert.Equal("Converter was called", ex.Message); + } + + 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.Contains(nameof(ClassWithListPropAndAttribute_ConverterOnProp), ex.ToString()); + Assert.Contains("IntProp", ex.ToString()); + + ex = Assert.Throws(() => JsonSerializer.Serialize(new ClassWithListPropAndAttribute_ConverterOnProp())); + Assert.Contains(nameof(ClassWithListPropAndAttribute_ConverterOnProp), ex.ToString()); + Assert.Contains("IntProp", ex.ToString()); + + ex = Assert.Throws(() => JsonSerializer.Deserialize("")); + Assert.Contains(nameof(ClassWithDictPropAndAttribute_ConverterOnProp), ex.ToString()); + Assert.Contains("IntProp", ex.ToString()); + + ex = Assert.Throws(() => JsonSerializer.Serialize(new ClassWithDictPropAndAttribute_ConverterOnProp())); + Assert.Contains(nameof(ClassWithDictPropAndAttribute_ConverterOnProp), ex.ToString()); + Assert.Contains("IntProp", ex.ToString()); + } + + 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(); + } + + [Fact] + public static void InternalCollectionConverter_CustomNullableNumberConverter() + { + NotImplementedException ex; + + 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). + // Converter returns 25 regardless of input. + Assert.Equal(25, JsonSerializer.Deserialize>(@"{""1"":""1""}", options)[1]); + ex = Assert.Throws(() => JsonSerializer.Serialize(dict, options)); + Assert.Equal("Converter was called", ex.Message); + + var obj = JsonSerializer.Deserialize(@"{""Prop"":{""1"":""1""}}", options); + Assert.Equal(25, obj.Prop[1]); + ex = Assert.Throws(() => JsonSerializer.Serialize(obj, options)); + Assert.Throws(() => JsonSerializer.Serialize(dict, options)); + Assert.Equal("Converter was called", ex.Message); + } + + 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("Converter was called"); + } + } + + /// + /// Example of a custom converter that uses the options to determine behavior. + /// + [Fact] + public static void AdaptableCustomConverter() + { + // Baseline without custom converter + PlainClassWithList obj = new() { Prop = new List() { 1 } }; + string json = JsonSerializer.Serialize(obj, s_optionReadAndWriteFromStr); + Assert.Equal("{\"Prop\":[\"1\"]}", json); + + obj = JsonSerializer.Deserialize(json, s_optionReadAndWriteFromStr); + Assert.Equal(1, obj.Prop[0]); + + // First with numbers + JsonSerializerOptions options = new() + { + Converters = { new AdaptableInt32Converter() } + }; + + obj = new() { Prop = new List() { 1 } }; + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{\"Prop\":[101]}", json); + + obj = JsonSerializer.Deserialize(json, options); + Assert.Equal(1, obj.Prop[0]); + + // Then with strings + options = new() + { + NumberHandling = JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString, + Converters = { new AdaptableInt32Converter() } + }; + + obj = new() { Prop = new List() { 1 } }; + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{\"Prop\":[\"101\"]}", json); + + obj = JsonSerializer.Deserialize(json, options); + Assert.Equal(1, obj.Prop[0]); + } + + private class PlainClassWithList + { + public List Prop { get; set; } + } + + public class AdaptableInt32Converter : JsonConverter + { + public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + if ((JsonNumberHandling.AllowReadingFromString & options.NumberHandling) != 0) + { + // Assume it's a string; don't use TryParse(). + return int.Parse(reader.GetString(), CultureInfo.InvariantCulture) - 100; + } + else + { + return reader.GetInt32() - 100; + } + } + + public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options) + { + if ((JsonNumberHandling.WriteAsString & options.NumberHandling) != 0) + { + writer.WriteStringValue((value + 100).ToString(CultureInfo.InvariantCulture)); + } + else + { + writer.WriteNumberValue(value + 100); + } + } + } } public class NumberHandlingTests_AsyncStreamOverload : NumberHandlingTests_OverloadSpecific -- 2.7.4