From 6d70fffc849269f7aed4ad9eaf7e272bd5f305ca Mon Sep 17 00:00:00 2001 From: devsko Date: Sun, 16 Aug 2020 21:28:58 +0200 Subject: [PATCH 01/13] IL emit Box/Unbox in member access --- .../ReflectionEmitMemberAccessor.cs | 38 ++++++++++++- .../CustomConverterTests.StructMember.cs | 55 +++++++++++++++++++ .../TestClasses/TestClasses.StructMember.cs | 43 +++++++++++++++ .../tests/System.Text.Json.Tests.csproj | 2 + 4 files changed, 136 insertions(+), 2 deletions(-) create mode 100644 src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs create mode 100644 src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index 5e7d1e8c094427..43e4fe4b9e6368 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -230,6 +230,9 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ Type? declaringType = propertyInfo.DeclaringType; Debug.Assert(declaringType != null); + Type realPropertyType = propertyInfo.PropertyType; + Debug.Assert(propertyType.IsAssignableFrom(realPropertyType)); + DynamicMethod dynamicMethod = CreateGetterMethod(propertyInfo.Name, propertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -246,6 +249,11 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ generator.Emit(OpCodes.Callvirt, realMethod); } + if (realPropertyType.IsValueType && !realPropertyType.Equals(propertyType)) + { + generator.Emit(OpCodes.Box, realPropertyType); + } + generator.Emit(OpCodes.Ret); return dynamicMethod; @@ -262,6 +270,9 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ Type? declaringType = propertyInfo.DeclaringType; Debug.Assert(declaringType != null); + Type realPropertyType = propertyInfo.PropertyType; + Debug.Assert(propertyType.IsAssignableFrom(realPropertyType)); + DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, propertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -270,19 +281,28 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ if (declaringType.IsValueType) { generator.Emit(OpCodes.Unbox, declaringType); - generator.Emit(OpCodes.Ldarg_1); + EmitLoadValue(generator, propertyType, realPropertyType); generator.Emit(OpCodes.Call, realMethod); } else { generator.Emit(OpCodes.Castclass, declaringType); - generator.Emit(OpCodes.Ldarg_1); + EmitLoadValue(generator, propertyType, realPropertyType); generator.Emit(OpCodes.Callvirt, realMethod); }; generator.Emit(OpCodes.Ret); return dynamicMethod; + + static void EmitLoadValue(ILGenerator generator, Type propertyType, Type realPropertyType) + { + generator.Emit(OpCodes.Ldarg_1); + if (realPropertyType.IsValueType && !realPropertyType.Equals(propertyType)) + { + generator.Emit(OpCodes.Unbox_Any, realPropertyType); + } + } } public override Func CreateFieldGetter(FieldInfo fieldInfo) => @@ -293,6 +313,9 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type fieldTy Type? declaringType = fieldInfo.DeclaringType; Debug.Assert(declaringType != null); + Type realFieldType = fieldInfo.FieldType; + Debug.Assert(fieldType.IsAssignableFrom(realFieldType)); + DynamicMethod dynamicMethod = CreateGetterMethod(fieldInfo.Name, fieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -303,6 +326,10 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type fieldTy : OpCodes.Castclass, declaringType); generator.Emit(OpCodes.Ldfld, fieldInfo); + if (realFieldType.IsValueType && !realFieldType.Equals(fieldType)) + { + generator.Emit(OpCodes.Box, realFieldType); + } generator.Emit(OpCodes.Ret); return dynamicMethod; @@ -316,6 +343,9 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type fieldTy Type? declaringType = fieldInfo.DeclaringType; Debug.Assert(declaringType != null); + Type realFieldType = fieldInfo.FieldType; + Debug.Assert(fieldType.IsAssignableFrom(realFieldType)); + DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, fieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -326,6 +356,10 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type fieldTy : OpCodes.Castclass, declaringType); generator.Emit(OpCodes.Ldarg_1); + if (realFieldType.IsValueType && !realFieldType.Equals(fieldType)) + { + generator.Emit(OpCodes.Unbox_Any, realFieldType); + } generator.Emit(OpCodes.Stfld, fieldInfo); generator.Emit(OpCodes.Ret); diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs new file mode 100644 index 00000000000000..a6eee61a6629f7 --- /dev/null +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs @@ -0,0 +1,55 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Xunit; + +namespace System.Text.Json.Serialization.Tests +{ + public static partial class CustomConverterTests + { + private class StructToInterfaceConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + { + return typeof(IMemberInterface).IsAssignableFrom(typeToConvert); + } + + public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + string value = reader.GetString(); + + return new StructMember(value); + } + + public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSerializerOptions options) + { + JsonSerializer.Serialize(writer, value.Value, typeof(string), options); + } + } + + [Fact] + public static void StructMemberConverter() + { + var options = new JsonSerializerOptions() + { + IncludeFields = true, + }; + options.Converters.Add(new StructToInterfaceConverter()); + + string json; + + { + TestClassWithStructMember obj = new TestClassWithStructMember(); + obj.Initialize(); + obj.Verify(); + json = JsonSerializer.Serialize(obj, options); + } + + { + TestClassWithStructMember obj = JsonSerializer.Deserialize(json, options); + obj.Verify(); + } + } + + } +} diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs new file mode 100644 index 00000000000000..0fa8a14dee3aa7 --- /dev/null +++ b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs @@ -0,0 +1,43 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace System.Text.Json.Serialization.Tests +{ + public class TestClassWithStructMember : ITestClass + { + public StructMember MyStructProperty { get; set; } + + public StructMember MyStructField; + + public void Initialize() + { + MyStructProperty = new StructMember("StructProperty"); + MyStructField = new StructMember("StructField"); + } + + public void Verify() + { + Assert.Equal("StructProperty", MyStructProperty.Value); + Assert.Equal("StructField", MyStructField.Value); + } + } + + public interface IMemberInterface + { + string Value { get; } + } + + public struct StructMember : IMemberInterface + { + public string Value { get; } + + public StructMember(string value) + { + Value = value; + } + } +} diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj index 74459b8f4f6b7a..124e3b45609bc7 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj @@ -80,6 +80,7 @@ + @@ -128,6 +129,7 @@ + From e77e7cca2add5c5e19458cdbb9cf5b453e4d6b78 Mon Sep 17 00:00:00 2001 From: devsko Date: Tue, 18 Aug 2020 11:58:17 +0200 Subject: [PATCH 02/13] Test case for object converter --- .../CustomConverterTests.StructMember.cs | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs index a6eee61a6629f7..e0ea9002d427ea 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs @@ -28,7 +28,7 @@ public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSe } [Fact] - public static void StructMemberConverter() + public static void StructMemberToInterfaceConverter() { var options = new JsonSerializerOptions() { @@ -51,5 +51,49 @@ public static void StructMemberConverter() } } + private class StructToObjectConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + { + return typeof(IMemberInterface).IsAssignableFrom(typeToConvert); + } + + public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + string value = reader.GetString(); + + return new StructMember(value); + } + + public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) + { + JsonSerializer.Serialize(writer, ((IMemberInterface)value).Value, typeof(string), options); + } + } + + [Fact] + public static void StructMemberToObjectConverter() + { + var options = new JsonSerializerOptions() + { + IncludeFields = true, + }; + options.Converters.Add(new StructToObjectConverter()); + + string json; + + { + TestClassWithStructMember obj = new TestClassWithStructMember(); + obj.Initialize(); + obj.Verify(); + json = JsonSerializer.Serialize(obj, options); + } + + { + TestClassWithStructMember obj = JsonSerializer.Deserialize(json, options); + obj.Verify(); + } + } + } } From c35942cb6caeff4ac5d96492b580c7e46b13cef0 Mon Sep 17 00:00:00 2001 From: devsko Date: Tue, 18 Aug 2020 21:12:16 +0200 Subject: [PATCH 03/13] Test case for class with primitive members and object converter --- .../CustomConverterTests.Object.cs | 66 +++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs index bdb1c6be82ff8c..e660447a028030 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs @@ -311,6 +311,72 @@ public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOp } } + private class PrimitiveConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + => true; + + public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + if (reader.TokenType == JsonTokenType.True) + { + return true; + } + + if (reader.TokenType == JsonTokenType.False) + { + return false; + } + + if (reader.TokenType == JsonTokenType.Number) + { + if (reader.TryGetInt32(out int i)) + { + return i; + } + + return reader.GetDouble(); + } + + if (reader.TokenType == JsonTokenType.String) + { + if (reader.TryGetDateTime(out DateTime datetime)) + { + return datetime; + } + + return reader.GetString(); + } + + throw new JsonException(); + } + + public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) + { + throw new InvalidOperationException("Should not get here."); + } + } + private class ClassWithPrimitives + { + [JsonConverter(typeof(PrimitiveConverter))] + public int MyInt { get; set; } + [JsonConverter(typeof(PrimitiveConverter))] + public bool MyBool { get; set; } + [JsonConverter(typeof(PrimitiveConverter))] + public string MyString { get; set; } + } + + [Fact] + public static void ClassWithPrimitivesObjectConverterDeserialize() + { + const string json = @"{""MyInt"":123,""MyBool"":true,""MyString"":""Hello""}"; + + var obj = JsonSerializer.Deserialize(json); + Assert.Equal(123, obj.MyInt); + Assert.True(obj.MyBool); + Assert.Equal("Hello", obj.MyString); + } + [Fact] public static void SystemObjectNewtonsoftCompatibleConverterDeserialize() { From 9cd5f5e60e55a470d3538b6df24feb89c8ef9de4 Mon Sep 17 00:00:00 2001 From: devsko Date: Thu, 20 Aug 2020 02:28:55 +0200 Subject: [PATCH 04/13] Address feedback --- .../Json/Serialization/JsonConverterOfT.cs | 2 +- .../ReflectionEmitMemberAccessor.cs | 71 ++++++---- .../CustomConverterTests.Object.cs | 131 ++++++++++++++++-- .../CustomConverterTests.StructMember.cs | 131 ++++++++++++++++-- .../tests/Serialization/ExtensionDataTests.cs | 3 +- .../TestClasses/TestClasses.StructMember.cs | 45 ++++++ 6 files changed, 325 insertions(+), 58 deletions(-) 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 9c360144a73b24..16b2dce3038db0 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 @@ -331,7 +331,7 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions return true; } - if (type != TypeToConvert) + if (type != TypeToConvert && IsInternalConverter) { // Handle polymorphic case and get the new converter. JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index 43e4fe4b9e6368..c8d60beee0fbbe 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -222,7 +222,7 @@ private static DynamicMethod CreateImmutableDictionaryCreateRangeDelegate(Type c public override Func CreatePropertyGetter(PropertyInfo propertyInfo) => CreateDelegate>(CreatePropertyGetter(propertyInfo, typeof(TProperty))); - private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Type propertyType) + private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Type runtimePropertyType) { MethodInfo? realMethod = propertyInfo.GetMethod; Debug.Assert(realMethod != null); @@ -230,10 +230,10 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ Type? declaringType = propertyInfo.DeclaringType; Debug.Assert(declaringType != null); - Type realPropertyType = propertyInfo.PropertyType; - Debug.Assert(propertyType.IsAssignableFrom(realPropertyType)); + Type declaredPropertyType = propertyInfo.PropertyType; + Debug.Assert(runtimePropertyType.IsAssignableFrom(declaredPropertyType)); - DynamicMethod dynamicMethod = CreateGetterMethod(propertyInfo.Name, propertyType); + DynamicMethod dynamicMethod = CreateGetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); generator.Emit(OpCodes.Ldarg_0); @@ -249,9 +249,12 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ generator.Emit(OpCodes.Callvirt, realMethod); } - if (realPropertyType.IsValueType && !realPropertyType.Equals(propertyType)) + // declaredPropertyType: Type of the property + // runtimePropertyType: of JsonConverter / JsonPropertyInfo + + if (declaredPropertyType.IsValueType && declaredPropertyType != runtimePropertyType) { - generator.Emit(OpCodes.Box, realPropertyType); + generator.Emit(OpCodes.Box, declaredPropertyType); } generator.Emit(OpCodes.Ret); @@ -262,7 +265,7 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ public override Action CreatePropertySetter(PropertyInfo propertyInfo) => CreateDelegate>(CreatePropertySetter(propertyInfo, typeof(TProperty))); - private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Type propertyType) + private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Type runtimePropertyType) { MethodInfo? realMethod = propertyInfo.SetMethod; Debug.Assert(realMethod != null); @@ -270,10 +273,10 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ Type? declaringType = propertyInfo.DeclaringType; Debug.Assert(declaringType != null); - Type realPropertyType = propertyInfo.PropertyType; - Debug.Assert(propertyType.IsAssignableFrom(realPropertyType)); + Type declaredPropertyType = propertyInfo.PropertyType; + Debug.Assert(runtimePropertyType.IsAssignableFrom(declaredPropertyType)); - DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, propertyType); + DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); generator.Emit(OpCodes.Ldarg_0); @@ -281,13 +284,13 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ if (declaringType.IsValueType) { generator.Emit(OpCodes.Unbox, declaringType); - EmitLoadValue(generator, propertyType, realPropertyType); + EmitLoadValue(generator, runtimePropertyType, declaredPropertyType); generator.Emit(OpCodes.Call, realMethod); } else { generator.Emit(OpCodes.Castclass, declaringType); - EmitLoadValue(generator, propertyType, realPropertyType); + EmitLoadValue(generator, runtimePropertyType, declaredPropertyType); generator.Emit(OpCodes.Callvirt, realMethod); }; @@ -295,12 +298,16 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ return dynamicMethod; - static void EmitLoadValue(ILGenerator generator, Type propertyType, Type realPropertyType) + static void EmitLoadValue(ILGenerator generator, Type runtimePropertyType, Type declaredPropertyType) { generator.Emit(OpCodes.Ldarg_1); - if (realPropertyType.IsValueType && !realPropertyType.Equals(propertyType)) + + // declaredPropertyType: Type of the property + // runtimePropertyType: of JsonConverter / JsonPropertyInfo + + if (declaredPropertyType.IsValueType && declaredPropertyType != runtimePropertyType) { - generator.Emit(OpCodes.Unbox_Any, realPropertyType); + generator.Emit(OpCodes.Unbox_Any, declaredPropertyType); } } } @@ -308,15 +315,15 @@ static void EmitLoadValue(ILGenerator generator, Type propertyType, Type realPro public override Func CreateFieldGetter(FieldInfo fieldInfo) => CreateDelegate>(CreateFieldGetter(fieldInfo, typeof(TProperty))); - private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type fieldType) + private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtimeFieldType) { Type? declaringType = fieldInfo.DeclaringType; Debug.Assert(declaringType != null); - Type realFieldType = fieldInfo.FieldType; - Debug.Assert(fieldType.IsAssignableFrom(realFieldType)); + Type declaredFieldType = fieldInfo.FieldType; + Debug.Assert(runtimeFieldType.IsAssignableFrom(declaredFieldType)); - DynamicMethod dynamicMethod = CreateGetterMethod(fieldInfo.Name, fieldType); + DynamicMethod dynamicMethod = CreateGetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); generator.Emit(OpCodes.Ldarg_0); @@ -326,10 +333,15 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type fieldTy : OpCodes.Castclass, declaringType); generator.Emit(OpCodes.Ldfld, fieldInfo); - if (realFieldType.IsValueType && !realFieldType.Equals(fieldType)) + + // declaredFieldType: Type of the field + // runtimeFieldType: of JsonConverter / JsonPropertyInfo + + if (declaredFieldType.IsValueType && declaredFieldType != runtimeFieldType) { - generator.Emit(OpCodes.Box, realFieldType); + generator.Emit(OpCodes.Box, declaredFieldType); } + generator.Emit(OpCodes.Ret); return dynamicMethod; @@ -338,15 +350,15 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type fieldTy public override Action CreateFieldSetter(FieldInfo fieldInfo) => CreateDelegate>(CreateFieldSetter(fieldInfo, typeof(TProperty))); - private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type fieldType) + private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtimeFieldType) { Type? declaringType = fieldInfo.DeclaringType; Debug.Assert(declaringType != null); - Type realFieldType = fieldInfo.FieldType; - Debug.Assert(fieldType.IsAssignableFrom(realFieldType)); + Type declaredFieldType = fieldInfo.FieldType; + Debug.Assert(runtimeFieldType.IsAssignableFrom(declaredFieldType)); - DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, fieldType); + DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); generator.Emit(OpCodes.Ldarg_0); @@ -356,10 +368,15 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type fieldTy : OpCodes.Castclass, declaringType); generator.Emit(OpCodes.Ldarg_1); - if (realFieldType.IsValueType && !realFieldType.Equals(fieldType)) + + // declaredFieldType: Type of the field + // runtimeFieldType: of JsonConverter / JsonPropertyInfo + + if (declaredFieldType.IsValueType && declaredFieldType != runtimeFieldType) { - generator.Emit(OpCodes.Unbox_Any, realFieldType); + generator.Emit(OpCodes.Unbox_Any, declaredFieldType); } + generator.Emit(OpCodes.Stfld, fieldInfo); generator.Emit(OpCodes.Ret); diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs index e660447a028030..4f1fe0ccd99370 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs @@ -313,11 +313,16 @@ public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOp private class PrimitiveConverter : JsonConverter { + public int ReadCallCount { get; private set; } + public int WriteCallCount { get; private set; } + public override bool CanConvert(Type typeToConvert) - => true; + => typeToConvert != typeof(ClassWithPrimitives); public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { + ReadCallCount++; + if (reader.TokenType == JsonTokenType.True) { return true; @@ -353,28 +358,128 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) { - throw new InvalidOperationException("Should not get here."); + WriteCallCount++; + + if (value is int i) + { + writer.WriteNumberValue(i); + } + else if (value is bool b) + { + writer.WriteBooleanValue(b); + } + else if (value is string s) + { + writer.WriteStringValue(s); + } + else + { + throw new NotSupportedException(); + } } } + private class ClassWithPrimitives { - [JsonConverter(typeof(PrimitiveConverter))] - public int MyInt { get; set; } - [JsonConverter(typeof(PrimitiveConverter))] - public bool MyBool { get; set; } - [JsonConverter(typeof(PrimitiveConverter))] - public string MyString { get; set; } + public int MyIntProperty { get; set; } + public bool MyBoolProperty { get; set; } + public string MyStringProperty { get; set; } +#pragma warning disable 0649 + public int MyIntField; + public bool MyBoolField; + public string MyStringField; +#pragma warning restore } [Fact] public static void ClassWithPrimitivesObjectConverterDeserialize() { - const string json = @"{""MyInt"":123,""MyBool"":true,""MyString"":""Hello""}"; + const string json = + @"{""MyIntProperty"":123,""MyBoolProperty"":true,""MyStringProperty"":""Hello""," + + @"""MyIntField"":321,""MyBoolField"":true,""MyStringField"":""World""}"; + + var converter = new PrimitiveConverter(); + var options = new JsonSerializerOptions + { + IncludeFields = true + }; + options.Converters.Add(converter); + + var obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal(6, converter.ReadCallCount); + + Assert.Equal(123, obj.MyIntProperty); + Assert.True(obj.MyBoolProperty); + Assert.Equal("Hello", obj.MyStringProperty); + Assert.Equal(321, obj.MyIntField); + Assert.True(obj.MyBoolField); + Assert.Equal("World", obj.MyStringField); + } + + [Fact] + public static void ClassWithPrimitivesObjectConverterSerialize() + { + const string expected = + @"{""MyIntProperty"":123,""MyBoolProperty"":true,""MyStringProperty"":""Hello""," + + @"""MyIntField"":321,""MyBoolField"":true,""MyStringField"":""World""}"; + + var converter = new PrimitiveConverter(); + var options = new JsonSerializerOptions + { + IncludeFields = true + }; + options.Converters.Add(converter); + + var obj = new ClassWithPrimitives + { + MyIntProperty = 123, + MyBoolProperty = true, + MyStringProperty = "Hello", + MyIntField = 321, + MyBoolField = true, + MyStringField = "World", + }; + + var json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(6, converter.WriteCallCount); + Assert.Equal(expected, json); + } + + private class ClassWithNullablePrimitives + { + public int? MyIntProperty { get; set; } + public bool? MyBoolProperty { get; set; } + public string MyStringProperty { get; set; } +#pragma warning disable 0649 + public int? MyIntField; + public bool? MyBoolField; + public string MyStringField; +#pragma warning restore + } + + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/36329")] + public static void ClassWithNullablePrimitivesObjectConverterDeserialize() + { + const string json = + @"{""MyIntProperty"":123,""MyBoolProperty"":true,""MyStringProperty"":""Hello""," + + @"""MyIntField"":321,""MyBoolField"":true,""MyStringField"":""World""}"; - var obj = JsonSerializer.Deserialize(json); - Assert.Equal(123, obj.MyInt); - Assert.True(obj.MyBool); - Assert.Equal("Hello", obj.MyString); + var options = new JsonSerializerOptions + { + IncludeFields = true + }; + + var obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal(123, obj.MyIntProperty); + Assert.True(obj.MyBoolProperty); + Assert.Equal("Hello", obj.MyStringProperty); + Assert.Equal(321, obj.MyIntField); + Assert.True(obj.MyBoolField); + Assert.Equal("World", obj.MyStringField); } [Fact] diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs index e0ea9002d427ea..94351f09147513 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs @@ -9,13 +9,21 @@ public static partial class CustomConverterTests { private class StructToInterfaceConverter : JsonConverter { + public int ReadCallCount { get; private set; } + public int WriteCallCount { get; private set; } + + public override bool HandleNull => true; + public override bool CanConvert(Type typeToConvert) { + typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert; return typeof(IMemberInterface).IsAssignableFrom(typeToConvert); } public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { + ReadCallCount++; + string value = reader.GetString(); return new StructMember(value); @@ -23,75 +31,168 @@ public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConv public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSerializerOptions options) { + WriteCallCount++; + JsonSerializer.Serialize(writer, value.Value, typeof(string), options); } } + private class StructToObjectConverter : JsonConverter + { + public int ReadCallCount { get; private set; } + public int WriteCallCount { get; private set; } + + public override bool HandleNull => true; + + public override bool CanConvert(Type typeToConvert) + { + return typeof(IMemberInterface).IsAssignableFrom(typeToConvert); + } + + public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + ReadCallCount++; + + string value = reader.GetString(); + + return new StructMember(value); + } + + public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) + { + WriteCallCount++; + + JsonSerializer.Serialize(writer, ((IMemberInterface)value).Value, typeof(string), options); + } + } + [Fact] public static void StructMemberToInterfaceConverter() { + const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + + var converter = new StructToInterfaceConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, }; - options.Converters.Add(new StructToInterfaceConverter()); + options.Converters.Add(converter); string json; { - TestClassWithStructMember obj = new TestClassWithStructMember(); + var obj = new TestClassWithStructMember(); obj.Initialize(); obj.Verify(); json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(4, converter.WriteCallCount); + Assert.Equal(expected, json); } { - TestClassWithStructMember obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); obj.Verify(); + + Assert.Equal(4, converter.ReadCallCount); } } - private class StructToObjectConverter : JsonConverter + [Fact] + public static void StructMemberToObjectConverter() { - public override bool CanConvert(Type typeToConvert) + const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + + var converter = new StructToObjectConverter(); + var options = new JsonSerializerOptions() { - return typeof(IMemberInterface).IsAssignableFrom(typeToConvert); + IncludeFields = true, + }; + options.Converters.Add(converter); + + string json; + + { + var obj = new TestClassWithStructMember(); + obj.Initialize(); + obj.Verify(); + json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(4, converter.WriteCallCount); + Assert.Equal(expected, json); } - public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { - string value = reader.GetString(); + var obj = JsonSerializer.Deserialize(json, options); + obj.Verify(); - return new StructMember(value); + Assert.Equal(4, converter.ReadCallCount); } + } - public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/36329")] + public static void NullableStructMemberToInterfaceConverter() + { + const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + + var converter = new StructToInterfaceConverter(); + var options = new JsonSerializerOptions() { - JsonSerializer.Serialize(writer, ((IMemberInterface)value).Value, typeof(string), options); + IncludeFields = true, + }; + options.Converters.Add(converter); + + string json; + + { + var obj = new TestClassWithNullableStructMember(); + obj.Initialize(); + obj.Verify(); + json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(4, converter.WriteCallCount); + Assert.Equal(expected, json); + } + + { + var obj = JsonSerializer.Deserialize(json, options); + obj.Verify(); + + Assert.Equal(4, converter.ReadCallCount); } } [Fact] - public static void StructMemberToObjectConverter() + [ActiveIssue("https://github.com/dotnet/runtime/issues/36329")] + public static void NullableStructMemberToObjectConverter() { + const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + + var converter = new StructToObjectConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, }; - options.Converters.Add(new StructToObjectConverter()); + options.Converters.Add(converter); string json; { - TestClassWithStructMember obj = new TestClassWithStructMember(); + var obj = new TestClassWithNullableStructMember(); obj.Initialize(); obj.Verify(); json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(4, converter.WriteCallCount); + Assert.Equal(expected, json); } { - TestClassWithStructMember obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); obj.Verify(); + + Assert.Equal(4, converter.ReadCallCount); } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs b/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs index a005ec38e6ef04..1c5b7a54c290cd 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs @@ -1077,8 +1077,7 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) { - // Since we are converter for object, the string converter will be called instead of this. - throw new InvalidOperationException(); + writer.WriteStringValue((string)value); } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs index 0fa8a14dee3aa7..d22cce7d10681b 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs @@ -13,16 +13,51 @@ public class TestClassWithStructMember : ITestClass public StructMember MyStructField; + public ClassMember MyClassProperty { get; set; } + + public ClassMember MyClassField; + public void Initialize() { MyStructProperty = new StructMember("StructProperty"); MyStructField = new StructMember("StructField"); + MyClassProperty = new ClassMember("ClassProperty"); + MyClassField = new ClassMember("ClassField"); } public void Verify() { Assert.Equal("StructProperty", MyStructProperty.Value); Assert.Equal("StructField", MyStructField.Value); + Assert.Equal("ClassProperty", MyClassProperty.Value); + Assert.Equal("ClassField", MyClassField.Value); + } + } + + public class TestClassWithNullableStructMember : ITestClass + { + public StructMember? MyStructProperty { get; set; } + + public StructMember? MyStructField; + + public ClassMember MyClassProperty { get; set; } + + public ClassMember MyClassField; + + public void Initialize() + { + MyStructProperty = new StructMember("StructProperty"); + MyStructField = new StructMember("StructField"); + MyClassProperty = new ClassMember("ClassProperty"); + MyClassField = new ClassMember("ClassField"); + } + + public void Verify() + { + Assert.Equal("StructProperty", MyStructProperty.Value.Value); + Assert.Equal("StructField", MyStructField.Value.Value); + Assert.Equal("ClassProperty", MyClassProperty.Value); + Assert.Equal("ClassField", MyClassField.Value); } } @@ -40,4 +75,14 @@ public StructMember(string value) Value = value; } } + + public class ClassMember : IMemberInterface + { + public string Value { get; } + + public ClassMember(string value) + { + Value = value; + } + } } From 906b41c889863275243e03e98f1ab322a179c4fd Mon Sep 17 00:00:00 2001 From: devsko Date: Thu, 20 Aug 2020 11:59:56 +0200 Subject: [PATCH 05/13] Fixed issues with JsonConverterand nullable value-types --- .../JsonSerializerOptions.Converters.cs | 37 +++++++-- .../ReflectionEmitMemberAccessor.cs | 8 +- .../CustomConverterTests.Object.cs | 1 - .../CustomConverterTests.StructMember.cs | 77 +++++++++++++++++-- 4 files changed, 106 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index 018eab87ade11e..db7e06a37deeb8 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -274,8 +274,7 @@ public JsonConverter GetConverter(Type typeToConvert) Type converterTypeToConvert = converter.TypeToConvert; - if (!converterTypeToConvert.IsAssignableFrom(typeToConvert) && - !typeToConvert.IsAssignableFrom(converterTypeToConvert)) + if (!IsCompatibleConverter(converterTypeToConvert, typeToConvert)) { ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), typeToConvert); } @@ -292,6 +291,36 @@ public JsonConverter GetConverter(Type typeToConvert) return converter; } + private static bool IsNullableValueType(Type type) + { + return type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableOfTType; + } + + private static bool IsNullableType(Type type) + { + return !type.IsValueType || IsNullableValueType(type); + } + + internal static bool IsAssignableFrom(Type type, Type from) + { + // Special case for which Type.IsAssignableFrom returns false + // type: Nullable where T : IInterface + // from: IInterface + + if (IsNullableValueType(from) && type.IsInterface) + { + return type.IsAssignableFrom(from.GetGenericArguments()[0]); + } + + return type.IsAssignableFrom(from); + } + + private static bool IsCompatibleConverter(Type converterTypeToConvert, Type typeToConvert) + { + return IsAssignableFrom(converterTypeToConvert, typeToConvert) + || IsAssignableFrom(typeToConvert, converterTypeToConvert); + } + private JsonConverter GetConverterFromAttribute(JsonConverterAttribute converterAttribute, Type typeToConvert, Type classTypeAttributeIsOn, MemberInfo? memberInfo) { JsonConverter? converter; @@ -361,9 +390,5 @@ private JsonConverter GetConverterFromAttribute(JsonConverterAttribute converter return default; } - private static bool IsNullableType(Type type) - { - return type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableOfTType; - } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index c8d60beee0fbbe..d4c4bca9ad9e07 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -231,7 +231,7 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ Debug.Assert(declaringType != null); Type declaredPropertyType = propertyInfo.PropertyType; - Debug.Assert(runtimePropertyType.IsAssignableFrom(declaredPropertyType)); + Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimePropertyType, declaredPropertyType)); DynamicMethod dynamicMethod = CreateGetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -274,7 +274,7 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ Debug.Assert(declaringType != null); Type declaredPropertyType = propertyInfo.PropertyType; - Debug.Assert(runtimePropertyType.IsAssignableFrom(declaredPropertyType)); + Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimePropertyType, declaredPropertyType)); DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -321,7 +321,7 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtime Debug.Assert(declaringType != null); Type declaredFieldType = fieldInfo.FieldType; - Debug.Assert(runtimeFieldType.IsAssignableFrom(declaredFieldType)); + Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimeFieldType, declaredFieldType)); DynamicMethod dynamicMethod = CreateGetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -356,7 +356,7 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtime Debug.Assert(declaringType != null); Type declaredFieldType = fieldInfo.FieldType; - Debug.Assert(runtimeFieldType.IsAssignableFrom(declaredFieldType)); + Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimeFieldType, declaredFieldType)); DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs index 4f1fe0ccd99370..eb723ea0921df6 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs @@ -460,7 +460,6 @@ private class ClassWithNullablePrimitives } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/36329")] public static void ClassWithNullablePrimitivesObjectConverterDeserialize() { const string json = diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs index 94351f09147513..50faeed0e70dca 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs @@ -26,14 +26,14 @@ public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConv string value = reader.GetString(); - return new StructMember(value); + return value == null ? null : new StructMember(value); } public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSerializerOptions options) { WriteCallCount++; - JsonSerializer.Serialize(writer, value.Value, typeof(string), options); + JsonSerializer.Serialize(writer, value == null ? null : value.Value, options); } } @@ -46,6 +46,7 @@ private class StructToObjectConverter : JsonConverter public override bool CanConvert(Type typeToConvert) { + typeToConvert = Nullable.GetUnderlyingType(typeToConvert) ?? typeToConvert; return typeof(IMemberInterface).IsAssignableFrom(typeToConvert); } @@ -55,14 +56,14 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS string value = reader.GetString(); - return new StructMember(value); + return value == null ? null : new StructMember(value); } public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) { WriteCallCount++; - JsonSerializer.Serialize(writer, ((IMemberInterface)value).Value, typeof(string), options); + JsonSerializer.Serialize(writer, value == null ? null : ((IMemberInterface)value).Value, options); } } @@ -131,7 +132,6 @@ public static void StructMemberToObjectConverter() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/36329")] public static void NullableStructMemberToInterfaceConverter() { const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; @@ -164,7 +164,6 @@ public static void NullableStructMemberToInterfaceConverter() } [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/36329")] public static void NullableStructMemberToObjectConverter() { const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; @@ -196,5 +195,71 @@ public static void NullableStructMemberToObjectConverter() } } + [Fact] + public static void NullableStructMemberWithNullsToInterfaceConverter() + { + const string expected = @"{""MyStructProperty"":null,""MyClassProperty"":null,""MyStructField"":null,""MyClassField"":null}"; + + var converter = new StructToInterfaceConverter(); + var options = new JsonSerializerOptions() + { + IncludeFields = true, + }; + options.Converters.Add(converter); + + string json; + + { + var obj = new TestClassWithNullableStructMember(); + json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(4, converter.WriteCallCount); + Assert.Equal(expected, json); + } + + { + var obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal(4, converter.ReadCallCount); + Assert.Null(obj.MyStructProperty); + Assert.Null(obj.MyStructField); + Assert.Null(obj.MyClassProperty); + Assert.Null(obj.MyClassField); + } + } + + [Fact] + public static void NullableStructMemberWithNullsToObjectConverter() + { + const string expected = @"{""MyStructProperty"":null,""MyClassProperty"":null,""MyStructField"":null,""MyClassField"":null}"; + + var converter = new StructToObjectConverter(); + var options = new JsonSerializerOptions() + { + IncludeFields = true, + }; + options.Converters.Add(converter); + + string json; + + { + var obj = new TestClassWithNullableStructMember(); + json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(4, converter.WriteCallCount); + Assert.Equal(expected, json); + } + + { + var obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal(4, converter.ReadCallCount); + Assert.Null(obj.MyStructProperty); + Assert.Null(obj.MyStructField); + Assert.Null(obj.MyClassProperty); + Assert.Null(obj.MyClassField); + } + } + } } From 1b795cf721290dd6e244c5018a5415a824ad8942 Mon Sep 17 00:00:00 2001 From: devsko Date: Thu, 20 Aug 2020 19:35:35 +0200 Subject: [PATCH 06/13] Address feedback --- .../src/System.Text.Json.csproj | 1 + .../Json/Serialization/JsonConverterOfT.cs | 3 +- .../JsonSerializerOptions.Converters.cs | 37 +----- .../ReflectionEmitMemberAccessor.cs | 8 +- .../src/System/Text/Json/TypeExtensions.cs | 45 ++++++++ .../CustomConverterTests.Object.cs | 109 ++++++++++-------- ... CustomConverterTests.ValueTypedMember.cs} | 84 +++++++------- .../tests/Serialization/ExtensionDataTests.cs | 2 + .../TestClasses/TestClasses.StructMember.cs | 88 -------------- .../TestClasses.ValueTypedMember.cs | 88 ++++++++++++++ .../tests/System.Text.Json.Tests.csproj | 4 +- 11 files changed, 249 insertions(+), 220 deletions(-) create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs rename src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/{CustomConverterTests.StructMember.cs => CustomConverterTests.ValueTypedMember.cs} (64%) delete mode 100644 src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs create mode 100644 src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index 9db4b3972418f9..fc5c8ce5591c9f 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -180,6 +180,7 @@ + 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 16b2dce3038db0..4736f32a588f65 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 @@ -333,7 +333,8 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions if (type != TypeToConvert && IsInternalConverter) { - // Handle polymorphic case and get the new converter. + // For internal converter only: Handle polymorphic case and get the new converter. + // Custom converter, even though polymorphic converter, get called for reading AND writing. JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); if (jsonConverter != this) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index db7e06a37deeb8..13f8720586c7f2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -18,8 +18,6 @@ public sealed partial class JsonSerializerOptions // The global list of built-in simple converters. private static readonly Dictionary s_defaultSimpleConverters = GetDefaultSimpleConverters(); - private static readonly Type s_nullableOfTType = typeof(Nullable<>); - // The global list of built-in converters that override CanConvert(). private static readonly JsonConverter[] s_defaultFactoryConverters = new JsonConverter[] { @@ -186,7 +184,7 @@ internal JsonConverter DetermineConverter(Type? parentClassType, Type runtimePro // We also throw to avoid passing an invalid argument to setters for nullable struct properties, // which would cause an InvalidProgramException when the generated IL is invoked. // This is not an issue of the converter is wrapped in NullableConverter. - if (IsNullableType(runtimePropertyType) && !IsNullableType(converter.TypeToConvert)) + if (runtimePropertyType.IsNullableType() && !converter.TypeToConvert.IsNullableType()) { ThrowHelper.ThrowInvalidOperationException_ConverterCanConvertNullableRedundant(runtimePropertyType, converter); } @@ -274,7 +272,8 @@ public JsonConverter GetConverter(Type typeToConvert) Type converterTypeToConvert = converter.TypeToConvert; - if (!IsCompatibleConverter(converterTypeToConvert, typeToConvert)) + if (!converterTypeToConvert.IsConvertibleFrom(typeToConvert) + && !typeToConvert.IsConvertibleFrom(converterTypeToConvert)) { ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), typeToConvert); } @@ -291,36 +290,6 @@ public JsonConverter GetConverter(Type typeToConvert) return converter; } - private static bool IsNullableValueType(Type type) - { - return type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableOfTType; - } - - private static bool IsNullableType(Type type) - { - return !type.IsValueType || IsNullableValueType(type); - } - - internal static bool IsAssignableFrom(Type type, Type from) - { - // Special case for which Type.IsAssignableFrom returns false - // type: Nullable where T : IInterface - // from: IInterface - - if (IsNullableValueType(from) && type.IsInterface) - { - return type.IsAssignableFrom(from.GetGenericArguments()[0]); - } - - return type.IsAssignableFrom(from); - } - - private static bool IsCompatibleConverter(Type converterTypeToConvert, Type typeToConvert) - { - return IsAssignableFrom(converterTypeToConvert, typeToConvert) - || IsAssignableFrom(typeToConvert, converterTypeToConvert); - } - private JsonConverter GetConverterFromAttribute(JsonConverterAttribute converterAttribute, Type typeToConvert, Type classTypeAttributeIsOn, MemberInfo? memberInfo) { JsonConverter? converter; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index d4c4bca9ad9e07..99549e5b740869 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -231,7 +231,7 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ Debug.Assert(declaringType != null); Type declaredPropertyType = propertyInfo.PropertyType; - Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimePropertyType, declaredPropertyType)); + Debug.Assert(runtimePropertyType.IsConvertibleFrom(declaredPropertyType)); DynamicMethod dynamicMethod = CreateGetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -274,7 +274,7 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ Debug.Assert(declaringType != null); Type declaredPropertyType = propertyInfo.PropertyType; - Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimePropertyType, declaredPropertyType)); + Debug.Assert(runtimePropertyType.IsConvertibleFrom(declaredPropertyType)); DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -321,7 +321,7 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtime Debug.Assert(declaringType != null); Type declaredFieldType = fieldInfo.FieldType; - Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimeFieldType, declaredFieldType)); + Debug.Assert(runtimeFieldType.IsConvertibleFrom(declaredFieldType)); DynamicMethod dynamicMethod = CreateGetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -356,7 +356,7 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtime Debug.Assert(declaringType != null); Type declaredFieldType = fieldInfo.FieldType; - Debug.Assert(JsonSerializerOptions.IsAssignableFrom(runtimeFieldType, declaredFieldType)); + Debug.Assert(runtimeFieldType.IsConvertibleFrom(declaredFieldType)); DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs new file mode 100644 index 00000000000000..7859525a987322 --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs @@ -0,0 +1,45 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; + +namespace System.Text.Json +{ + internal static class TypeExtensions + { + private static readonly Type s_nullableOfTType = typeof(Nullable<>); + + /// + /// Returns when the given type is of type . + /// + public static bool IsNullableValueType(this Type type) + { + return type.IsGenericType && type.GetGenericTypeDefinition() == s_nullableOfTType; + } + + /// + /// Returns when the given type is either a reference type or of type . + /// + public static bool IsNullableType(this Type type) + { + return !type.IsValueType || IsNullableValueType(type); + } + + /// + /// Returns when the given type is assignable from . + /// + /// + /// Other than also returns when is of type where : and is of type . + /// + public static bool IsConvertibleFrom(this Type type, Type from) + { + if (IsNullableValueType(from) && type.IsInterface) + { + return type.IsAssignableFrom(from.GetGenericArguments()[0]); + } + + return type.IsAssignableFrom(from); + } + + } +} diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs index eb723ea0921df6..762981d805d03e 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.Object.cs @@ -317,7 +317,8 @@ private class PrimitiveConverter : JsonConverter public int WriteCallCount { get; private set; } public override bool CanConvert(Type typeToConvert) - => typeToConvert != typeof(ClassWithPrimitives); + => typeToConvert != typeof(ClassWithPrimitives) + && typeToConvert != typeof(ClassWithNullablePrimitives); public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { @@ -392,12 +393,13 @@ private class ClassWithPrimitives } [Fact] - public static void ClassWithPrimitivesObjectConverterDeserialize() + public static void ClassWithPrimitivesObjectConverter() { - const string json = + const string expected = @"{""MyIntProperty"":123,""MyBoolProperty"":true,""MyStringProperty"":""Hello""," + @"""MyIntField"":321,""MyBoolField"":true,""MyStringField"":""World""}"; + string json; var converter = new PrimitiveConverter(); var options = new JsonSerializerOptions { @@ -405,46 +407,34 @@ public static void ClassWithPrimitivesObjectConverterDeserialize() }; options.Converters.Add(converter); - var obj = JsonSerializer.Deserialize(json, options); - - Assert.Equal(6, converter.ReadCallCount); - - Assert.Equal(123, obj.MyIntProperty); - Assert.True(obj.MyBoolProperty); - Assert.Equal("Hello", obj.MyStringProperty); - Assert.Equal(321, obj.MyIntField); - Assert.True(obj.MyBoolField); - Assert.Equal("World", obj.MyStringField); - } - - [Fact] - public static void ClassWithPrimitivesObjectConverterSerialize() - { - const string expected = - @"{""MyIntProperty"":123,""MyBoolProperty"":true,""MyStringProperty"":""Hello""," + - @"""MyIntField"":321,""MyBoolField"":true,""MyStringField"":""World""}"; - - var converter = new PrimitiveConverter(); - var options = new JsonSerializerOptions { - IncludeFields = true - }; - options.Converters.Add(converter); + var obj = new ClassWithPrimitives + { + MyIntProperty = 123, + MyBoolProperty = true, + MyStringProperty = "Hello", + MyIntField = 321, + MyBoolField = true, + MyStringField = "World", + }; - var obj = new ClassWithPrimitives + json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(6, converter.WriteCallCount); + Assert.Equal(expected, json); + } { - MyIntProperty = 123, - MyBoolProperty = true, - MyStringProperty = "Hello", - MyIntField = 321, - MyBoolField = true, - MyStringField = "World", - }; + var obj = JsonSerializer.Deserialize(json, options); - var json = JsonSerializer.Serialize(obj, options); + Assert.Equal(6, converter.ReadCallCount); - Assert.Equal(6, converter.WriteCallCount); - Assert.Equal(expected, json); + Assert.Equal(123, obj.MyIntProperty); + Assert.True(obj.MyBoolProperty); + Assert.Equal("Hello", obj.MyStringProperty); + Assert.Equal(321, obj.MyIntField); + Assert.True(obj.MyBoolField); + Assert.Equal("World", obj.MyStringField); + } } private class ClassWithNullablePrimitives @@ -460,25 +450,46 @@ private class ClassWithNullablePrimitives } [Fact] - public static void ClassWithNullablePrimitivesObjectConverterDeserialize() + public static void ClassWithNullablePrimitivesObjectConverter() { - const string json = + const string expected = @"{""MyIntProperty"":123,""MyBoolProperty"":true,""MyStringProperty"":""Hello""," + @"""MyIntField"":321,""MyBoolField"":true,""MyStringField"":""World""}"; + string json; + var converter = new PrimitiveConverter(); var options = new JsonSerializerOptions { IncludeFields = true }; - - var obj = JsonSerializer.Deserialize(json, options); - - Assert.Equal(123, obj.MyIntProperty); - Assert.True(obj.MyBoolProperty); - Assert.Equal("Hello", obj.MyStringProperty); - Assert.Equal(321, obj.MyIntField); - Assert.True(obj.MyBoolField); - Assert.Equal("World", obj.MyStringField); + options.Converters.Add(converter); + + { + var obj = new ClassWithNullablePrimitives + { + MyIntProperty = 123, + MyBoolProperty = true, + MyStringProperty = "Hello", + MyIntField = 321, + MyBoolField = true, + MyStringField = "World", + }; + + json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(6, converter.WriteCallCount); + Assert.Equal(expected, json); + } + { + var obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal(123, obj.MyIntProperty); + Assert.True(obj.MyBoolProperty); + Assert.Equal("Hello", obj.MyStringProperty); + Assert.Equal(321, obj.MyIntField); + Assert.True(obj.MyBoolField); + Assert.Equal("World", obj.MyStringField); + } } [Fact] diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs similarity index 64% rename from src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs rename to src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs index 50faeed0e70dca..5444c527c801c4 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.StructMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs @@ -7,7 +7,7 @@ namespace System.Text.Json.Serialization.Tests { public static partial class CustomConverterTests { - private class StructToInterfaceConverter : JsonConverter + private class ValueTypeToInterfaceConverter : JsonConverter { public int ReadCallCount { get; private set; } public int WriteCallCount { get; private set; } @@ -26,7 +26,7 @@ public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConv string value = reader.GetString(); - return value == null ? null : new StructMember(value); + return value == null ? null : new ValueTypedMember(value); } public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSerializerOptions options) @@ -37,7 +37,7 @@ public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSe } } - private class StructToObjectConverter : JsonConverter + private class ValueTypeToObjectConverter : JsonConverter { public int ReadCallCount { get; private set; } public int WriteCallCount { get; private set; } @@ -56,7 +56,7 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS string value = reader.GetString(); - return value == null ? null : new StructMember(value); + return value == null ? null : new ValueTypedMember(value); } public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) @@ -68,11 +68,11 @@ public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOp } [Fact] - public static void StructMemberToInterfaceConverter() + public static void ValueTypedMemberToInterfaceConverter() { - const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + const string expected = @"{""MyValueTypedProperty"":""ValueTypedProperty"",""MyRefTypedProperty"":""RefTypedProperty"",""MyValueTypedField"":""ValueTypedField"",""MyRefTypedField"":""RefTypedField""}"; - var converter = new StructToInterfaceConverter(); + var converter = new ValueTypeToInterfaceConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, @@ -82,7 +82,7 @@ public static void StructMemberToInterfaceConverter() string json; { - var obj = new TestClassWithStructMember(); + var obj = new TestClassWithValueTypedMember(); obj.Initialize(); obj.Verify(); json = JsonSerializer.Serialize(obj, options); @@ -92,7 +92,7 @@ public static void StructMemberToInterfaceConverter() } { - var obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); obj.Verify(); Assert.Equal(4, converter.ReadCallCount); @@ -100,11 +100,11 @@ public static void StructMemberToInterfaceConverter() } [Fact] - public static void StructMemberToObjectConverter() + public static void ValueTypedMemberToObjectConverter() { - const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + const string expected = @"{""MyValueTypedProperty"":""ValueTypedProperty"",""MyRefTypedProperty"":""RefTypedProperty"",""MyValueTypedField"":""ValueTypedField"",""MyRefTypedField"":""RefTypedField""}"; - var converter = new StructToObjectConverter(); + var converter = new ValueTypeToObjectConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, @@ -114,7 +114,7 @@ public static void StructMemberToObjectConverter() string json; { - var obj = new TestClassWithStructMember(); + var obj = new TestClassWithValueTypedMember(); obj.Initialize(); obj.Verify(); json = JsonSerializer.Serialize(obj, options); @@ -124,7 +124,7 @@ public static void StructMemberToObjectConverter() } { - var obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); obj.Verify(); Assert.Equal(4, converter.ReadCallCount); @@ -132,11 +132,11 @@ public static void StructMemberToObjectConverter() } [Fact] - public static void NullableStructMemberToInterfaceConverter() + public static void NullableValueTypedMemberToInterfaceConverter() { - const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + const string expected = @"{""MyValueTypedProperty"":""ValueTypedProperty"",""MyRefTypedProperty"":""RefTypedProperty"",""MyValueTypedField"":""ValueTypedField"",""MyRefTypedField"":""RefTypedField""}"; - var converter = new StructToInterfaceConverter(); + var converter = new ValueTypeToInterfaceConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, @@ -146,7 +146,7 @@ public static void NullableStructMemberToInterfaceConverter() string json; { - var obj = new TestClassWithNullableStructMember(); + var obj = new TestClassWithNullableValueTypedMember(); obj.Initialize(); obj.Verify(); json = JsonSerializer.Serialize(obj, options); @@ -156,7 +156,7 @@ public static void NullableStructMemberToInterfaceConverter() } { - var obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); obj.Verify(); Assert.Equal(4, converter.ReadCallCount); @@ -164,11 +164,11 @@ public static void NullableStructMemberToInterfaceConverter() } [Fact] - public static void NullableStructMemberToObjectConverter() + public static void NullableValueTypedMemberToObjectConverter() { - const string expected = @"{""MyStructProperty"":""StructProperty"",""MyClassProperty"":""ClassProperty"",""MyStructField"":""StructField"",""MyClassField"":""ClassField""}"; + const string expected = @"{""MyValueTypedProperty"":""ValueTypedProperty"",""MyRefTypedProperty"":""RefTypedProperty"",""MyValueTypedField"":""ValueTypedField"",""MyRefTypedField"":""RefTypedField""}"; - var converter = new StructToObjectConverter(); + var converter = new ValueTypeToObjectConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, @@ -178,7 +178,7 @@ public static void NullableStructMemberToObjectConverter() string json; { - var obj = new TestClassWithNullableStructMember(); + var obj = new TestClassWithNullableValueTypedMember(); obj.Initialize(); obj.Verify(); json = JsonSerializer.Serialize(obj, options); @@ -188,7 +188,7 @@ public static void NullableStructMemberToObjectConverter() } { - var obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); obj.Verify(); Assert.Equal(4, converter.ReadCallCount); @@ -196,11 +196,11 @@ public static void NullableStructMemberToObjectConverter() } [Fact] - public static void NullableStructMemberWithNullsToInterfaceConverter() + public static void NullableValueTypedMemberWithNullsToInterfaceConverter() { - const string expected = @"{""MyStructProperty"":null,""MyClassProperty"":null,""MyStructField"":null,""MyClassField"":null}"; + const string expected = @"{""MyValueTypedProperty"":null,""MyRefTypedProperty"":null,""MyValueTypedField"":null,""MyRefTypedField"":null}"; - var converter = new StructToInterfaceConverter(); + var converter = new ValueTypeToInterfaceConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, @@ -210,7 +210,7 @@ public static void NullableStructMemberWithNullsToInterfaceConverter() string json; { - var obj = new TestClassWithNullableStructMember(); + var obj = new TestClassWithNullableValueTypedMember(); json = JsonSerializer.Serialize(obj, options); Assert.Equal(4, converter.WriteCallCount); @@ -218,22 +218,22 @@ public static void NullableStructMemberWithNullsToInterfaceConverter() } { - var obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); Assert.Equal(4, converter.ReadCallCount); - Assert.Null(obj.MyStructProperty); - Assert.Null(obj.MyStructField); - Assert.Null(obj.MyClassProperty); - Assert.Null(obj.MyClassField); + Assert.Null(obj.MyValueTypedProperty); + Assert.Null(obj.MyValueTypedField); + Assert.Null(obj.MyRefTypedProperty); + Assert.Null(obj.MyRefTypedField); } } [Fact] - public static void NullableStructMemberWithNullsToObjectConverter() + public static void NullableValueTypedMemberWithNullsToObjectConverter() { - const string expected = @"{""MyStructProperty"":null,""MyClassProperty"":null,""MyStructField"":null,""MyClassField"":null}"; + const string expected = @"{""MyValueTypedProperty"":null,""MyRefTypedProperty"":null,""MyValueTypedField"":null,""MyRefTypedField"":null}"; - var converter = new StructToObjectConverter(); + var converter = new ValueTypeToObjectConverter(); var options = new JsonSerializerOptions() { IncludeFields = true, @@ -243,7 +243,7 @@ public static void NullableStructMemberWithNullsToObjectConverter() string json; { - var obj = new TestClassWithNullableStructMember(); + var obj = new TestClassWithNullableValueTypedMember(); json = JsonSerializer.Serialize(obj, options); Assert.Equal(4, converter.WriteCallCount); @@ -251,13 +251,13 @@ public static void NullableStructMemberWithNullsToObjectConverter() } { - var obj = JsonSerializer.Deserialize(json, options); + var obj = JsonSerializer.Deserialize(json, options); Assert.Equal(4, converter.ReadCallCount); - Assert.Null(obj.MyStructProperty); - Assert.Null(obj.MyStructField); - Assert.Null(obj.MyClassProperty); - Assert.Null(obj.MyClassField); + Assert.Null(obj.MyValueTypedProperty); + Assert.Null(obj.MyValueTypedField); + Assert.Null(obj.MyRefTypedProperty); + Assert.Null(obj.MyRefTypedField); } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs b/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs index 1c5b7a54c290cd..b2c6baa772d749 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs @@ -1077,6 +1077,8 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) { + // Since we are in a user-provided (not internal to S.T.Json) object converter, + // this converter will be called, not the internal string converter. writer.WriteStringValue((string)value); } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs deleted file mode 100644 index d22cce7d10681b..00000000000000 --- a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.StructMember.cs +++ /dev/null @@ -1,88 +0,0 @@ -// Licensed to the .NET Foundation under one or more agreements. -// The .NET Foundation licenses this file to you under the MIT license. - -using System.Collections.Generic; -using System.Linq; -using Xunit; - -namespace System.Text.Json.Serialization.Tests -{ - public class TestClassWithStructMember : ITestClass - { - public StructMember MyStructProperty { get; set; } - - public StructMember MyStructField; - - public ClassMember MyClassProperty { get; set; } - - public ClassMember MyClassField; - - public void Initialize() - { - MyStructProperty = new StructMember("StructProperty"); - MyStructField = new StructMember("StructField"); - MyClassProperty = new ClassMember("ClassProperty"); - MyClassField = new ClassMember("ClassField"); - } - - public void Verify() - { - Assert.Equal("StructProperty", MyStructProperty.Value); - Assert.Equal("StructField", MyStructField.Value); - Assert.Equal("ClassProperty", MyClassProperty.Value); - Assert.Equal("ClassField", MyClassField.Value); - } - } - - public class TestClassWithNullableStructMember : ITestClass - { - public StructMember? MyStructProperty { get; set; } - - public StructMember? MyStructField; - - public ClassMember MyClassProperty { get; set; } - - public ClassMember MyClassField; - - public void Initialize() - { - MyStructProperty = new StructMember("StructProperty"); - MyStructField = new StructMember("StructField"); - MyClassProperty = new ClassMember("ClassProperty"); - MyClassField = new ClassMember("ClassField"); - } - - public void Verify() - { - Assert.Equal("StructProperty", MyStructProperty.Value.Value); - Assert.Equal("StructField", MyStructField.Value.Value); - Assert.Equal("ClassProperty", MyClassProperty.Value); - Assert.Equal("ClassField", MyClassField.Value); - } - } - - public interface IMemberInterface - { - string Value { get; } - } - - public struct StructMember : IMemberInterface - { - public string Value { get; } - - public StructMember(string value) - { - Value = value; - } - } - - public class ClassMember : IMemberInterface - { - public string Value { get; } - - public ClassMember(string value) - { - Value = value; - } - } -} diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs new file mode 100644 index 00000000000000..7f6cbe3bff2433 --- /dev/null +++ b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs @@ -0,0 +1,88 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Linq; +using Xunit; + +namespace System.Text.Json.Serialization.Tests +{ + public class TestClassWithValueTypedMember : ITestClass + { + public ValueTypedMember MyValueTypedProperty { get; set; } + + public ValueTypedMember MyValueTypedField; + + public RefTypedMember MyRefTypedProperty { get; set; } + + public RefTypedMember MyRefTypedField; + + public void Initialize() + { + MyValueTypedProperty = new ValueTypedMember("ValueTypedProperty"); + MyValueTypedField = new ValueTypedMember("ValueTypedField"); + MyRefTypedProperty = new RefTypedMember("RefTypedProperty"); + MyRefTypedField = new RefTypedMember("RefTypedField"); + } + + public void Verify() + { + Assert.Equal("ValueTypedProperty", MyValueTypedProperty.Value); + Assert.Equal("ValueTypedField", MyValueTypedField.Value); + Assert.Equal("RefTypedProperty", MyRefTypedProperty.Value); + Assert.Equal("RefTypedField", MyRefTypedField.Value); + } + } + + public class TestClassWithNullableValueTypedMember : ITestClass + { + public ValueTypedMember? MyValueTypedProperty { get; set; } + + public ValueTypedMember? MyValueTypedField; + + public RefTypedMember MyRefTypedProperty { get; set; } + + public RefTypedMember MyRefTypedField; + + public void Initialize() + { + MyValueTypedProperty = new ValueTypedMember("ValueTypedProperty"); + MyValueTypedField = new ValueTypedMember("ValueTypedField"); + MyRefTypedProperty = new RefTypedMember("RefTypedProperty"); + MyRefTypedField = new RefTypedMember("RefTypedField"); + } + + public void Verify() + { + Assert.Equal("ValueTypedProperty", MyValueTypedProperty.Value.Value); + Assert.Equal("ValueTypedField", MyValueTypedField.Value.Value); + Assert.Equal("RefTypedProperty", MyRefTypedProperty.Value); + Assert.Equal("RefTypedField", MyRefTypedField.Value); + } + } + + public interface IMemberInterface + { + string Value { get; } + } + + public struct ValueTypedMember : IMemberInterface + { + public string Value { get; } + + public ValueTypedMember(string value) + { + Value = value; + } + } + + public class RefTypedMember : IMemberInterface + { + public string Value { get; } + + public RefTypedMember(string value) + { + Value = value; + } + } +} diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj index 124e3b45609bc7..50aed95352e451 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj @@ -80,7 +80,7 @@ - + @@ -129,7 +129,7 @@ - + From f635c21c987161340587a3c25c4462fe85238b02 Mon Sep 17 00:00:00 2001 From: devsko Date: Fri, 21 Aug 2020 08:14:17 +0200 Subject: [PATCH 07/13] Fix test for .NETStandard version --- .../CustomConverterTests.ValueTypedMember.cs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs index 5444c527c801c4..9c704e7bd1b425 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs @@ -26,7 +26,14 @@ public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConv string value = reader.GetString(); - return value == null ? null : new ValueTypedMember(value); + if (value == null) + { + return null; + } + + return value.IndexOf("ValueTyped", StringComparison.Ordinal) >= 0 + ? new ValueTypedMember(value) + : new RefTypedMember(value); } public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSerializerOptions options) @@ -56,7 +63,14 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS string value = reader.GetString(); - return value == null ? null : new ValueTypedMember(value); + if (value == null) + { + return null; + } + + return value.IndexOf("ValueTyped", StringComparison.Ordinal) >= 0 + ? new ValueTypedMember(value) + : new RefTypedMember(value); } public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) From a27f8233a17a5935f18c584d8b5bdb402c2f44d7 Mon Sep 17 00:00:00 2001 From: devsko Date: Fri, 21 Aug 2020 19:52:02 +0200 Subject: [PATCH 08/13] Fix #41146 --- .../Serialization/JsonSerializerOptions.cs | 8 +- .../ReflectionEmitMemberAccessor.cs | 26 +- .../CustomConverterTests.InvalidCast.cs | 226 ++++++++++++++++++ .../tests/System.Text.Json.Tests.csproj | 1 + 4 files changed, 249 insertions(+), 12 deletions(-) create mode 100644 src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs index 4217f61a61fc84..7d0bf44ac0025e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs @@ -495,11 +495,11 @@ internal MemberAccessor MemberAccessorStrategy { if (_memberAccessorStrategy == null) { -#if NETFRAMEWORK || NETCOREAPP - _memberAccessorStrategy = new ReflectionEmitMemberAccessor(); -#else +//#if NETFRAMEWORK || NETCOREAPP +// _memberAccessorStrategy = new ReflectionEmitMemberAccessor(); +//#else _memberAccessorStrategy = new ReflectionMemberAccessor(); -#endif +//#endif } return _memberAccessorStrategy; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index 99549e5b740869..1de529634ae823 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -231,7 +231,6 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ Debug.Assert(declaringType != null); Type declaredPropertyType = propertyInfo.PropertyType; - Debug.Assert(runtimePropertyType.IsConvertibleFrom(declaredPropertyType)); DynamicMethod dynamicMethod = CreateGetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -274,7 +273,6 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ Debug.Assert(declaringType != null); Type declaredPropertyType = propertyInfo.PropertyType; - Debug.Assert(runtimePropertyType.IsConvertibleFrom(declaredPropertyType)); DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -305,9 +303,16 @@ static void EmitLoadValue(ILGenerator generator, Type runtimePropertyType, Type // declaredPropertyType: Type of the property // runtimePropertyType: of JsonConverter / JsonPropertyInfo - if (declaredPropertyType.IsValueType && declaredPropertyType != runtimePropertyType) + if (declaredPropertyType != runtimePropertyType) { - generator.Emit(OpCodes.Unbox_Any, declaredPropertyType); + if (declaredPropertyType.IsValueType) + { + generator.Emit(OpCodes.Unbox_Any, declaredPropertyType); + } + else + { + generator.Emit(OpCodes.Castclass, declaredPropertyType); + } } } } @@ -321,7 +326,6 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtime Debug.Assert(declaringType != null); Type declaredFieldType = fieldInfo.FieldType; - Debug.Assert(runtimeFieldType.IsConvertibleFrom(declaredFieldType)); DynamicMethod dynamicMethod = CreateGetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -356,7 +360,6 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtime Debug.Assert(declaringType != null); Type declaredFieldType = fieldInfo.FieldType; - Debug.Assert(runtimeFieldType.IsConvertibleFrom(declaredFieldType)); DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); @@ -372,9 +375,16 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtime // declaredFieldType: Type of the field // runtimeFieldType: of JsonConverter / JsonPropertyInfo - if (declaredFieldType.IsValueType && declaredFieldType != runtimeFieldType) + if (declaredFieldType != runtimeFieldType) { - generator.Emit(OpCodes.Unbox_Any, declaredFieldType); + if (declaredFieldType.IsValueType) + { + generator.Emit(OpCodes.Unbox_Any, declaredFieldType); + } + else + { + generator.Emit(OpCodes.Castclass, declaredFieldType); + } } generator.Emit(OpCodes.Stfld, fieldInfo); diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs new file mode 100644 index 00000000000000..caf100691e6911 --- /dev/null +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs @@ -0,0 +1,226 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Xunit; + +namespace System.Text.Json.Serialization.Tests +{ + public static partial class CustomConverterTests + { + [Fact] + public static void InvalidCastPropertyFails() + { + var obj = new ObjectWrapperWithProperty + { + Object = new WrittenObject + { + Int = 123, + String = "Hello", + } + }; + + var json = JsonSerializer.Serialize(obj); + + Assert.Throws(() => JsonSerializer.Deserialize(json)); + } + + [Fact] + public static void InvalidCastFieldFails() + { + var options = new JsonSerializerOptions { IncludeFields = true }; + var obj = new ObjectWrapperWithField + { + Object = new WrittenObject + { + Int = 123, + String = "Hello", + } + }; + + var json = JsonSerializer.Serialize(obj); + + Assert.Throws(() => JsonSerializer.Deserialize(json, options)); + } + + /// + /// A converter that intentionally deserialize a completely unrelated typed object. + /// + public class InvalidCastConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + => true; + + public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + JsonSerializer.Deserialize(ref reader, options); + return new ReadObject { Double = Math.PI }; + } + + public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) + { + JsonSerializer.Serialize(writer, (WrittenObject)value, options); + } + } + + private class ObjectWrapperWithProperty + { + [JsonConverter(typeof(InvalidCastConverter))] + public WrittenObject Object { get; set; } + } + + private class ObjectWrapperWithField + { + [JsonConverter(typeof(InvalidCastConverter))] + public WrittenObject Object { get; set; } + } + + private class WrittenObject + { + public string String { get; set; } + public int Int { get; set; } + } + + private class ReadObject + { + public double Double { get; set; } + } + + [Fact] + public static void CastDerivedWorks() + { + var options = new JsonSerializerOptions { IncludeFields = true }; + var obj = JsonSerializer.Deserialize(@"{""DerivedProperty"":"""",""DerivedField"":""""}", options); + + Assert.IsType(obj.DerivedField); + Assert.IsType(obj.DerivedProperty); + } + + [Fact] + public static void CastBaseWorks() + { + var options = new JsonSerializerOptions { IncludeFields = true }; + var obj = JsonSerializer.Deserialize(@"{""BaseProperty"":"""",""BaseField"":""""}", options); + + Assert.IsType(obj.BaseField); + Assert.IsType(obj.BaseProperty); + } + + [Fact] + public static void CastBasePropertyFails() + { + var options = new JsonSerializerOptions { IncludeFields = true }; + Assert.Throws(() => JsonSerializer.Deserialize(@"{""DerivedProperty"":""""}", options)); + } + + [Fact] + public static void CastBaseFieldFails() + { + var options = new JsonSerializerOptions { IncludeFields = true }; + Assert.Throws(() => JsonSerializer.Deserialize(@"{""DerivedField"":""""}", options)); + } + + /// + /// A converter that deserializes an object of an derived class. + /// + private class BaseConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + => true; + + public override Base Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + reader.GetString(); + return new Derived() { String = "Hello", Double = Math.PI }; + } + + public override void Write(Utf8JsonWriter writer, Base value, JsonSerializerOptions options) + { + throw new NotImplementedException(); + } + } + + /// + /// A converter that deserializes an object of an derived class. + /// + private class DerivedConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + => true; + + public override Derived Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + reader.GetString(); + return new Derived() { String = "Hello", Double = Math.PI }; + } + + public override void Write(Utf8JsonWriter writer, Derived value, JsonSerializerOptions options) + { + throw new NotImplementedException(); + } + } + + /// + /// A converter that deserializes an object of the base class where the wrapper expects an derived object. + /// + private class InvalidBaseConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + => true; + + public override Base Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + { + reader.GetString(); + return new Base() { String = "Hello" }; + } + + public override void Write(Utf8JsonWriter writer, Base value, JsonSerializerOptions options) + { + throw new NotImplementedException(); + } + } + + private class Base + { + public string String; + } + + private class Derived : Base + { + public double Double; + } + + private class ObjectWrapperDerived + { + [JsonConverter(typeof(BaseConverter))] + public Derived DerivedProperty { get; set; } + [JsonConverter(typeof(BaseConverter))] +#pragma warning disable 0649 + public Derived DerivedField; +#pragma warning restore + } + + private class ObjectWrapperDerivedWithProperty + { + [JsonConverter(typeof(InvalidBaseConverter))] + public Derived DerivedProperty { get; set; } + } + + private class ObjectWrapperDerivedWithField + { + [JsonConverter(typeof(InvalidBaseConverter))] +#pragma warning disable 0649 + public Derived DerivedField; +#pragma warning restore + } + + private class ObjectWrapperBase + { + [JsonConverter(typeof(DerivedConverter))] + public Base BaseProperty { get; set; } + [JsonConverter(typeof(DerivedConverter))] +#pragma warning disable 0649 + public Base BaseField; +#pragma warning restore + } + } +} diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj index 50aed95352e451..7c400153ef3295 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj @@ -73,6 +73,7 @@ + From fd5d793344c8c498ed46ec8bc6172177476bbd64 Mon Sep 17 00:00:00 2001 From: devsko Date: Fri, 21 Aug 2020 20:15:35 +0200 Subject: [PATCH 09/13] Accidentally commited the local test hack --- .../Text/Json/Serialization/JsonSerializerOptions.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs index 7d0bf44ac0025e..4217f61a61fc84 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs @@ -495,11 +495,11 @@ internal MemberAccessor MemberAccessorStrategy { if (_memberAccessorStrategy == null) { -//#if NETFRAMEWORK || NETCOREAPP -// _memberAccessorStrategy = new ReflectionEmitMemberAccessor(); -//#else +#if NETFRAMEWORK || NETCOREAPP + _memberAccessorStrategy = new ReflectionEmitMemberAccessor(); +#else _memberAccessorStrategy = new ReflectionMemberAccessor(); -//#endif +#endif } return _memberAccessorStrategy; From 71a9c8f3bcc9744d51890779947c45b923caa035 Mon Sep 17 00:00:00 2001 From: devsko Date: Sun, 23 Aug 2020 10:06:51 +0200 Subject: [PATCH 10/13] Throw JsonException when a deserialized value cannot be assigned to the property/field --- .../src/Resources/Strings.resx | 8 +- .../JsonSerializerOptions.Converters.cs | 4 +- .../ReflectionEmitMemberAccessor.cs | 200 +++++++++++++----- .../Serialization/ReflectionMemberAccessor.cs | 87 +++++++- .../Text/Json/ThrowHelper.Serialization.cs | 18 ++ .../src/System/Text/Json/TypeExtensions.cs | 2 +- .../tests/JsonPropertyTests.cs | 2 +- .../CustomConverterTests.InvalidCast.cs | 12 +- .../CustomConverterTests.ValueTypedMember.cs | 120 ++++++++++- .../TestClasses.ValueTypedMember.cs | 21 ++ 10 files changed, 393 insertions(+), 81 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index d6ae6ad77482d0..f3ad2b628c40a0 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -545,4 +545,10 @@ 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. - \ No newline at end of file + + The converted value of type '{0}' could not be assigned to the property or field '{1}' of type '{2}'. + + + The converted value 'null' could not be assigned to the property or field '{0}' of type '{1}'. + + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index 13f8720586c7f2..c40a7469e34e56 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -272,8 +272,8 @@ public JsonConverter GetConverter(Type typeToConvert) Type converterTypeToConvert = converter.TypeToConvert; - if (!converterTypeToConvert.IsConvertibleFrom(typeToConvert) - && !typeToConvert.IsConvertibleFrom(converterTypeToConvert)) + if (!converterTypeToConvert.IsAssignableFromInternal(typeToConvert) + && !typeToConvert.IsAssignableFromInternal(converterTypeToConvert)) { ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), typeToConvert); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index 1de529634ae823..de9321cc2ab10f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -13,6 +13,18 @@ namespace System.Text.Json.Serialization { internal sealed class ReflectionEmitMemberAccessor : MemberAccessor { + private static readonly Type s_nullReferenceException = typeof(NullReferenceException); + private static readonly Type s_invalidCastExceptionType = typeof(InvalidCastException); + + private static readonly Type s_stringType = typeof(string); + private static readonly Type s_typeType = typeof(Type); + + private static readonly MethodInfo s_object_GetType = typeof(object).GetMethod(nameof(object.GetType))!; + private static readonly MethodInfo s_throwHelper_ThrowJsonException_DeserializeUnableToAssignValue + = typeof(ThrowHelper).GetMethod(nameof(ThrowHelper.ThrowJsonException_DeserializeUnableToAssignValue))!; + private static readonly MethodInfo s_throwHelper_ThrowJsonException_DeserializeUnableToAssignNull + = typeof(ThrowHelper).GetMethod(nameof(ThrowHelper.ThrowJsonException_DeserializeUnableToAssignNull))!; + public override JsonClassInfo.ConstructorDelegate? CreateConstructor(Type type) { Debug.Assert(type != null); @@ -262,9 +274,35 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ } public override Action CreatePropertySetter(PropertyInfo propertyInfo) => - CreateDelegate>(CreatePropertySetter(propertyInfo, typeof(TProperty))); + CreatePropertySetterDelegate(propertyInfo); + + private static Action CreatePropertySetterDelegate(PropertyInfo propertyInfo) + { + string memberName = propertyInfo.Name; + Type declaredPropertyType = propertyInfo.PropertyType; + Type runtimePropertyType = typeof(TProperty); + + DynamicMethod propertySetter = CreatePropertySetter(propertyInfo, memberName, declaredPropertyType, runtimePropertyType); + + if (declaredPropertyType == runtimePropertyType) + { + // If declared and runtime type are identical, the assignment cannot fail, thus + // there is no need for exception handling or closures. + + return CreateDelegate>(propertySetter); + } + else + { + // Return a delegate with some closures needed for creating the potential exception. + + Action propertySetterDelegate = + CreateDelegate>(propertySetter); + + return (object? obj, TProperty value) => propertySetterDelegate(obj, value, memberName, declaredPropertyType); + } + } - private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Type runtimePropertyType) + private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, string memberName, Type declaredPropertyType, Type runtimePropertyType) { MethodInfo? realMethod = propertyInfo.SetMethod; Debug.Assert(realMethod != null); @@ -272,49 +310,45 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Typ Type? declaringType = propertyInfo.DeclaringType; Debug.Assert(declaringType != null); - Type declaredPropertyType = propertyInfo.PropertyType; + // declaredPropertyType: Type of the property + // runtimePropertyType: of JsonConverter / JsonPropertyInfo - DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, runtimePropertyType); - ILGenerator generator = dynamicMethod.GetILGenerator(); + bool valueNeedsCastOrUnbox = declaredPropertyType != runtimePropertyType; - generator.Emit(OpCodes.Ldarg_0); + DynamicMethod dynamicMethod = CreateSetterMethod(memberName, runtimePropertyType, valueNeedsCastOrUnbox); + ILGenerator generator = dynamicMethod.GetILGenerator(); - if (declaringType.IsValueType) + if (!valueNeedsCastOrUnbox) { - generator.Emit(OpCodes.Unbox, declaringType); - EmitLoadValue(generator, runtimePropertyType, declaredPropertyType); - generator.Emit(OpCodes.Call, realMethod); + generator.Emit(OpCodes.Ldarg_0); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); + generator.Emit(OpCodes.Ldarg_1); } else { - generator.Emit(OpCodes.Castclass, declaringType); - EmitLoadValue(generator, runtimePropertyType, declaredPropertyType); - generator.Emit(OpCodes.Callvirt, realMethod); - }; + LocalBuilder value = generator.DeclareLocal(declaredPropertyType); - generator.Emit(OpCodes.Ret); - - return dynamicMethod; + // try + generator.BeginExceptionBlock(); - static void EmitLoadValue(ILGenerator generator, Type runtimePropertyType, Type declaredPropertyType) - { + // When applied to a reference type, the unbox.any instruction has the same effect as castclass. generator.Emit(OpCodes.Ldarg_1); + generator.Emit(OpCodes.Unbox_Any, declaredPropertyType); + generator.Emit(OpCodes.Stloc_0, value); - // declaredPropertyType: Type of the property - // runtimePropertyType: of JsonConverter / JsonPropertyInfo + EmitSetterCatchBlocks(generator); - if (declaredPropertyType != runtimePropertyType) - { - if (declaredPropertyType.IsValueType) - { - generator.Emit(OpCodes.Unbox_Any, declaredPropertyType); - } - else - { - generator.Emit(OpCodes.Castclass, declaredPropertyType); - } - } + generator.EndExceptionBlock(); + + generator.Emit(OpCodes.Ldarg_0); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); + generator.Emit(OpCodes.Ldloc_0); } + + generator.Emit(declaringType.IsValueType ? OpCodes.Call : OpCodes.Callvirt, realMethod); + generator.Emit(OpCodes.Ret); + + return dynamicMethod; } public override Func CreateFieldGetter(FieldInfo fieldInfo) => @@ -352,39 +386,72 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtime } public override Action CreateFieldSetter(FieldInfo fieldInfo) => - CreateDelegate>(CreateFieldSetter(fieldInfo, typeof(TProperty))); + CreateFieldSetterDelegate(fieldInfo); - private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtimeFieldType) + private static Action CreateFieldSetterDelegate(FieldInfo fieldInfo) { - Type? declaringType = fieldInfo.DeclaringType; - Debug.Assert(declaringType != null); - + string memberName = fieldInfo.Name; Type declaredFieldType = fieldInfo.FieldType; + Type runtimeFieldType = typeof(TProperty); - DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, runtimeFieldType); - ILGenerator generator = dynamicMethod.GetILGenerator(); + DynamicMethod fieldSetter = CreateFieldSetter(fieldInfo, memberName, declaredFieldType, runtimeFieldType); - generator.Emit(OpCodes.Ldarg_0); - generator.Emit( - declaringType.IsValueType - ? OpCodes.Unbox - : OpCodes.Castclass, - declaringType); - generator.Emit(OpCodes.Ldarg_1); + if (declaredFieldType == runtimeFieldType) + { + // If declared and runtime type are identical, the assignment cannot fail, thus + // there is no need for exception handling or closures. + + return CreateDelegate>(fieldSetter); + } + else + { + // Return a delegate with some closures needed for creating the potential exception. + + Action fieldSetterDelegate = + CreateDelegate>(fieldSetter); + + return (object? obj, TProperty value) => fieldSetterDelegate(obj, value, memberName, declaredFieldType); + } + } + + private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, string memberName, Type declaredFieldType, Type runtimeFieldType) + { + Type? declaringType = fieldInfo.DeclaringType; + Debug.Assert(declaringType != null); // declaredFieldType: Type of the field // runtimeFieldType: of JsonConverter / JsonPropertyInfo - if (declaredFieldType != runtimeFieldType) + bool valueNeedsCastOrUnbox = declaredFieldType != runtimeFieldType; + + DynamicMethod dynamicMethod = CreateSetterMethod(memberName, runtimeFieldType, valueNeedsCastOrUnbox); + ILGenerator generator = dynamicMethod.GetILGenerator(); + + if (!valueNeedsCastOrUnbox) { - if (declaredFieldType.IsValueType) - { - generator.Emit(OpCodes.Unbox_Any, declaredFieldType); - } - else - { - generator.Emit(OpCodes.Castclass, declaredFieldType); - } + generator.Emit(OpCodes.Ldarg_0); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); + generator.Emit(OpCodes.Ldarg_1); + } + else + { + LocalBuilder value = generator.DeclareLocal(declaredFieldType); + + // try + generator.BeginExceptionBlock(); + + // When applied to a reference type, the unbox.any instruction has the same effect as castclass. + generator.Emit(OpCodes.Ldarg_1); + generator.Emit(OpCodes.Unbox_Any, declaredFieldType); + generator.Emit(OpCodes.Stloc_0, value); + + EmitSetterCatchBlocks(generator); + + generator.EndExceptionBlock(); + + generator.Emit(OpCodes.Ldarg_0); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); + generator.Emit(OpCodes.Ldloc_0); } generator.Emit(OpCodes.Stfld, fieldInfo); @@ -393,6 +460,25 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtime return dynamicMethod; } + private static void EmitSetterCatchBlocks(ILGenerator generator) + { + // catch (NullReferenceException) + generator.BeginCatchBlock(s_nullReferenceException); + generator.Emit(OpCodes.Pop); + generator.Emit(OpCodes.Ldarg_2); + generator.Emit(OpCodes.Ldarg_3); + generator.Emit(OpCodes.Call, s_throwHelper_ThrowJsonException_DeserializeUnableToAssignNull); + + // catch (InvalidCastException) + generator.BeginCatchBlock(s_invalidCastExceptionType); + generator.Emit(OpCodes.Pop); + generator.Emit(OpCodes.Ldarg_1); + generator.Emit(OpCodes.Callvirt, s_object_GetType); + generator.Emit(OpCodes.Ldarg_2); + generator.Emit(OpCodes.Ldarg_3); + generator.Emit(OpCodes.Call, s_throwHelper_ThrowJsonException_DeserializeUnableToAssignValue); + } + private static DynamicMethod CreateGetterMethod(string memberName, Type memberType) => new DynamicMethod( memberName + "Getter", @@ -401,11 +487,11 @@ private static DynamicMethod CreateGetterMethod(string memberName, Type memberTy typeof(ReflectionEmitMemberAccessor).Module, skipVisibility: true); - private static DynamicMethod CreateSetterMethod(string memberName, Type memberType) => + private static DynamicMethod CreateSetterMethod(string memberName, Type memberType, bool valueNeedsCastOrUnbox) => new DynamicMethod( memberName + "Setter", typeof(void), - new[] { JsonClassInfo.ObjectType, memberType }, + valueNeedsCastOrUnbox ? new[] { JsonClassInfo.ObjectType, memberType, s_stringType, s_typeType } : new[] { JsonClassInfo.ObjectType, memberType }, typeof(ReflectionEmitMemberAccessor).Module, skipVisibility: true); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionMemberAccessor.cs index bb9537d50384a8..694146ce9fb350 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionMemberAccessor.cs @@ -150,11 +150,46 @@ public override Func CreatePropertyGetter(Property public override Action CreatePropertySetter(PropertyInfo propertyInfo) { MethodInfo setMethodInfo = propertyInfo.SetMethod!; + Type declaredType = propertyInfo.PropertyType; - return delegate (object obj, TProperty value) + if (typeof(TProperty) == declaredType) { - setMethodInfo.Invoke(obj, new object[] { value! }); - }; + // If declared and runtime type are identical, the value must be + // assignable. Just call the setter. + + return delegate (object obj, TProperty value) + { + setMethodInfo.Invoke(obj, new object[] { value! }); + }; + } + else + { + string memberName = propertyInfo.Name; + + return delegate (object obj, TProperty value) + { + // Check the value for null or a not assignable type and throw a JsonException + // when the assignment would throw a NRE or ICE. + + if (value != null) + { + Type typeOfValue = value.GetType(); + if (!declaredType.IsAssignableFrom(typeOfValue)) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignValue(typeOfValue, memberName, declaredType); + } + } + else if (declaredType.IsValueType && !declaredType.IsNullableValueType()) + { + // If null gets passed to a value-typed parameter by reflection, no + // exception is thrown. We have to throw one on our own. + + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignNull(memberName, declaredType); + } + + setMethodInfo.Invoke(obj, new object[] { value! }); + }; + } } public override Func CreateFieldGetter(FieldInfo fieldInfo) => @@ -163,10 +198,48 @@ public override Func CreateFieldGetter(FieldInfo f return (TProperty)fieldInfo.GetValue(obj)!; }; - public override Action CreateFieldSetter(FieldInfo fieldInfo) => - delegate (object obj, TProperty value) + public override Action CreateFieldSetter(FieldInfo fieldInfo) + { + Type declaredType = fieldInfo.FieldType; + + if (typeof(TProperty) == declaredType) { - fieldInfo.SetValue(obj, value); - }; + // If declared and runtime type are identical, the value must be + // assignable. Just call the setter. + + return delegate (object obj, TProperty value) + { + fieldInfo.SetValue(obj, value); + }; + } + else + { + string memberName = fieldInfo.Name; + + return delegate (object obj, TProperty value) + { + // Check the value for null or a not assignable type and throw a JsonException + // when the assignment would throw a NRE or ICE. + + if (value != null) + { + Type typeOfValue = value.GetType(); + if (!declaredType.IsAssignableFrom(typeOfValue)) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignValue(typeOfValue, memberName, declaredType); + } + } + else if (declaredType.IsValueType && !declaredType.IsNullableValueType()) + { + // If null gets passed to a value-typed parameter by reflection, no + // exception is thrown. We have to throw one on our own. + + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignNull(memberName, declaredType); + } + + fieldInfo.SetValue(obj, value); + }; + } + } } } 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 e82f44b05a593e..4e58940bc791a0 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 @@ -49,6 +49,24 @@ public static void ThrowJsonException_DeserializeUnableToConvertValue(Type prope throw ex; } + [DoesNotReturn] + [MethodImpl(MethodImplOptions.NoInlining)] + public static void ThrowJsonException_DeserializeUnableToAssignValue(Type typeOfValue, string memberName, Type declaredType) + { + var ex = new JsonException(SR.Format(SR.DeserializeUnableToAssignValue, typeOfValue, memberName, declaredType)); + ex.AppendPathInformation = true; + throw ex; + } + + [DoesNotReturn] + [MethodImpl(MethodImplOptions.NoInlining)] + public static void ThrowJsonException_DeserializeUnableToAssignNull(string memberName, Type declaredType) + { + var ex = new JsonException(SR.Format(SR.DeserializeUnableToAssignNull, memberName, declaredType)); + ex.AppendPathInformation = true; + throw ex; + } + [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowJsonException_SerializationConverterRead(JsonConverter? converter) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs index 7859525a987322..fe949642b008bb 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/TypeExtensions.cs @@ -31,7 +31,7 @@ public static bool IsNullableType(this Type type) /// /// Other than also returns when is of type where : and is of type . /// - public static bool IsConvertibleFrom(this Type type, Type from) + public static bool IsAssignableFromInternal(this Type type, Type from) { if (IsNullableValueType(from) && type.IsInterface) { diff --git a/src/libraries/System.Text.Json/tests/JsonPropertyTests.cs b/src/libraries/System.Text.Json/tests/JsonPropertyTests.cs index 4d48938ebe39e0..8193c928342f71 100644 --- a/src/libraries/System.Text.Json/tests/JsonPropertyTests.cs +++ b/src/libraries/System.Text.Json/tests/JsonPropertyTests.cs @@ -97,7 +97,7 @@ private static void AssertContents(string expectedValue, ArrayBufferWriter [InlineData(null)] public static void NameEquals_InvalidInstance_Throws(string text) { - const string ErrorMessage = "Operation is not valid due to the current state of the object."; + string ErrorMessage = new InvalidOperationException().Message; JsonProperty prop = default; AssertExtensions.Throws(() => prop.NameEquals(text), ErrorMessage); AssertExtensions.Throws(() => prop.NameEquals(text.AsSpan()), ErrorMessage); diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs index caf100691e6911..bc624547a7ff7c 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.InvalidCast.cs @@ -8,7 +8,7 @@ namespace System.Text.Json.Serialization.Tests public static partial class CustomConverterTests { [Fact] - public static void InvalidCastPropertyFails() + public static void InvalidCastRefTypedPropertyFails() { var obj = new ObjectWrapperWithProperty { @@ -21,11 +21,11 @@ public static void InvalidCastPropertyFails() var json = JsonSerializer.Serialize(obj); - Assert.Throws(() => JsonSerializer.Deserialize(json)); + var ex = Assert.Throws(() => JsonSerializer.Deserialize(json)); } [Fact] - public static void InvalidCastFieldFails() + public static void InvalidCastRefTypedFieldFails() { var options = new JsonSerializerOptions { IncludeFields = true }; var obj = new ObjectWrapperWithField @@ -39,7 +39,7 @@ public static void InvalidCastFieldFails() var json = JsonSerializer.Serialize(obj); - Assert.Throws(() => JsonSerializer.Deserialize(json, options)); + var ex = Assert.Throws(() => JsonSerializer.Deserialize(json, options)); } /// @@ -109,14 +109,14 @@ public static void CastBaseWorks() public static void CastBasePropertyFails() { var options = new JsonSerializerOptions { IncludeFields = true }; - Assert.Throws(() => JsonSerializer.Deserialize(@"{""DerivedProperty"":""""}", options)); + var ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""DerivedProperty"":""""}", options)); } [Fact] public static void CastBaseFieldFails() { var options = new JsonSerializerOptions { IncludeFields = true }; - Assert.Throws(() => JsonSerializer.Deserialize(@"{""DerivedField"":""""}", options)); + var ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""DerivedField"":""""}", options)); } /// diff --git a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs index 9c704e7bd1b425..e54eb0b0bb509d 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/CustomConverterTests/CustomConverterTests.ValueTypedMember.cs @@ -31,9 +31,23 @@ public override IMemberInterface Read(ref Utf8JsonReader reader, Type typeToConv return null; } - return value.IndexOf("ValueTyped", StringComparison.Ordinal) >= 0 - ? new ValueTypedMember(value) - : new RefTypedMember(value); + if (value.IndexOf("ValueTyped", StringComparison.Ordinal) >= 0) + { + return new ValueTypedMember(value); + } + if (value.IndexOf("RefTyped", StringComparison.Ordinal) >= 0) + { + return new RefTypedMember(value); + } + if (value.IndexOf("OtherVT", StringComparison.Ordinal) >= 0) + { + return new OtherVTMember(value); + } + if (value.IndexOf("OtherRT", StringComparison.Ordinal) >= 0) + { + return new OtherRTMember(value); + } + throw new JsonException(); } public override void Write(Utf8JsonWriter writer, IMemberInterface value, JsonSerializerOptions options) @@ -68,9 +82,23 @@ public override object Read(ref Utf8JsonReader reader, Type typeToConvert, JsonS return null; } - return value.IndexOf("ValueTyped", StringComparison.Ordinal) >= 0 - ? new ValueTypedMember(value) - : new RefTypedMember(value); + if (value.IndexOf("ValueTyped", StringComparison.Ordinal) >= 0) + { + return new ValueTypedMember(value); + } + if (value.IndexOf("RefTyped", StringComparison.Ordinal) >= 0) + { + return new RefTypedMember(value); + } + if (value.IndexOf("OtherVT", StringComparison.Ordinal) >= 0) + { + return new OtherVTMember(value); + } + if (value.IndexOf("OtherRT", StringComparison.Ordinal) >= 0) + { + return new OtherRTMember(value); + } + throw new JsonException(); } public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOptions options) @@ -81,6 +109,86 @@ public override void Write(Utf8JsonWriter writer, object value, JsonSerializerOp } } + [Fact] + public static void AssignmentToValueTypedMemberInterface() + { + var converter = new ValueTypeToInterfaceConverter(); + var options = new JsonSerializerOptions { IncludeFields = true }; + options.Converters.Add(converter); + + Exception ex; + // Invalid cast OtherVTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherVTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherVTField""}", options)); + // Invalid cast OtherRTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherRTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherRTField""}", options)); + // Invalid null + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":null}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":null}", options)); + } + + [Fact] + public static void AssignmentToValueTypedMemberObject() + { + var converter = new ValueTypeToObjectConverter(); + var options = new JsonSerializerOptions { IncludeFields = true }; + options.Converters.Add(converter); + + Exception ex; + // Invalid cast OtherVTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherVTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherVTField""}", options)); + // Invalid cast OtherRTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherRTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherRTField""}", options)); + // Invalid null + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":null}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":null}", options)); + } + + [Fact] + public static void AssignmentToNullableValueTypedMemberInterface() + { + var converter = new ValueTypeToInterfaceConverter(); + var options = new JsonSerializerOptions { IncludeFields = true }; + options.Converters.Add(converter); + + TestClassWithNullableValueTypedMember obj; + Exception ex; + // Invalid cast OtherVTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherVTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherVTField""}", options)); + // Invalid cast OtherRTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherRTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherRTField""}", options)); + // Valid null + obj = JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":null,""MyValueTypedField"":null}", options); + Assert.Null(obj.MyValueTypedProperty); + Assert.Null(obj.MyValueTypedField); + } + + [Fact] + public static void AssignmentToNullableValueTypedMemberObject() + { + var converter = new ValueTypeToObjectConverter(); + var options = new JsonSerializerOptions { IncludeFields = true }; + options.Converters.Add(converter); + + TestClassWithNullableValueTypedMember obj; + Exception ex; + // Invalid cast OtherVTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherVTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherVTField""}", options)); + // Invalid cast OtherRTMember + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":""OtherRTProperty""}", options)); + ex = Assert.Throws(() => JsonSerializer.Deserialize(@"{""MyValueTypedField"":""OtherRTField""}", options)); + // Valid null + obj = JsonSerializer.Deserialize(@"{""MyValueTypedProperty"":null,""MyValueTypedField"":null}", options); + Assert.Null(obj.MyValueTypedProperty); + Assert.Null(obj.MyValueTypedField); + } + [Fact] public static void ValueTypedMemberToInterfaceConverter() { diff --git a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs index 7f6cbe3bff2433..ab7384afce90e1 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/TestClasses/TestClasses.ValueTypedMember.cs @@ -76,6 +76,16 @@ public ValueTypedMember(string value) } } + public struct OtherVTMember : IMemberInterface + { + public string Value { get; } + + public OtherVTMember(string value) + { + Value = value; + } + } + public class RefTypedMember : IMemberInterface { public string Value { get; } @@ -85,4 +95,15 @@ public RefTypedMember(string value) Value = value; } } + + public class OtherRTMember : IMemberInterface + { + public string Value { get; } + + public OtherRTMember(string value) + { + Value = value; + } + } + } From 1f365727d13ec31f4e3fa0d46bb893972b9ebff4 Mon Sep 17 00:00:00 2001 From: devsko Date: Sun, 23 Aug 2020 13:17:12 +0200 Subject: [PATCH 11/13] Fix merge --- src/libraries/System.Text.Json/src/Resources/Strings.resx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index f3ad2b628c40a0..b68d138294227d 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -551,4 +551,7 @@ The converted value 'null' could not be assigned to the property or field '{0}' of type '{1}'. + + The object with reference id '{0}' of type '{1}' cannot be assigned to the type '{2}'. + From e66be2ec2fdcedbf8554c2eba4b5693ba8f8d7fe Mon Sep 17 00:00:00 2001 From: devsko Date: Sun, 23 Aug 2020 13:46:02 +0200 Subject: [PATCH 12/13] Fix merge again --- src/libraries/System.Text.Json/src/Resources/Strings.resx | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 39519626437a2f..b68d138294227d 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -550,8 +550,6 @@ The converted value 'null' could not be assigned to the property or field '{0}' of type '{1}'. - - The object with reference id '{0}' of type '{1}' cannot be assigned to the type '{2}'. The object with reference id '{0}' of type '{1}' cannot be assigned to the type '{2}'. From c50e8ca75f2b1b2f4ba0283ddd846849abbfea70 Mon Sep 17 00:00:00 2001 From: devsko Date: Tue, 25 Aug 2020 15:43:35 +0200 Subject: [PATCH 13/13] Small simplification of ReflectionEmitAccessor --- .../ReflectionEmitMemberAccessor.cs | 173 +++++++----------- 1 file changed, 64 insertions(+), 109 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs index de9321cc2ab10f..fce77ca2ff8bf2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReflectionEmitMemberAccessor.cs @@ -13,18 +13,6 @@ namespace System.Text.Json.Serialization { internal sealed class ReflectionEmitMemberAccessor : MemberAccessor { - private static readonly Type s_nullReferenceException = typeof(NullReferenceException); - private static readonly Type s_invalidCastExceptionType = typeof(InvalidCastException); - - private static readonly Type s_stringType = typeof(string); - private static readonly Type s_typeType = typeof(Type); - - private static readonly MethodInfo s_object_GetType = typeof(object).GetMethod(nameof(object.GetType))!; - private static readonly MethodInfo s_throwHelper_ThrowJsonException_DeserializeUnableToAssignValue - = typeof(ThrowHelper).GetMethod(nameof(ThrowHelper.ThrowJsonException_DeserializeUnableToAssignValue))!; - private static readonly MethodInfo s_throwHelper_ThrowJsonException_DeserializeUnableToAssignNull - = typeof(ThrowHelper).GetMethod(nameof(ThrowHelper.ThrowJsonException_DeserializeUnableToAssignNull))!; - public override JsonClassInfo.ConstructorDelegate? CreateConstructor(Type type) { Debug.Assert(type != null); @@ -273,36 +261,48 @@ private static DynamicMethod CreatePropertyGetter(PropertyInfo propertyInfo, Typ return dynamicMethod; } - public override Action CreatePropertySetter(PropertyInfo propertyInfo) => + public override Action CreatePropertySetter(PropertyInfo propertyInfo) => CreatePropertySetterDelegate(propertyInfo); - private static Action CreatePropertySetterDelegate(PropertyInfo propertyInfo) + private static Action CreatePropertySetterDelegate(PropertyInfo propertyInfo) { string memberName = propertyInfo.Name; Type declaredPropertyType = propertyInfo.PropertyType; Type runtimePropertyType = typeof(TProperty); - DynamicMethod propertySetter = CreatePropertySetter(propertyInfo, memberName, declaredPropertyType, runtimePropertyType); + Action propertySetter = CreateDelegate>( + CreatePropertySetter(propertyInfo, runtimePropertyType)); if (declaredPropertyType == runtimePropertyType) { // If declared and runtime type are identical, the assignment cannot fail, thus // there is no need for exception handling or closures. - return CreateDelegate>(propertySetter); + return propertySetter; } else { - // Return a delegate with some closures needed for creating the potential exception. - - Action propertySetterDelegate = - CreateDelegate>(propertySetter); + // Return a delegate with closures and exception handling. - return (object? obj, TProperty value) => propertySetterDelegate(obj, value, memberName, declaredPropertyType); + return (object obj, TProperty value) => + { + try + { + propertySetter(obj, value); + } + catch (NullReferenceException) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignNull(memberName, declaredPropertyType); + } + catch (InvalidCastException) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignValue(value!.GetType(), memberName, declaredPropertyType); + } + }; } } - private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, string memberName, Type declaredPropertyType, Type runtimePropertyType) + private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, Type runtimePropertyType) { MethodInfo? realMethod = propertyInfo.SetMethod; Debug.Assert(realMethod != null); @@ -310,39 +310,22 @@ private static DynamicMethod CreatePropertySetter(PropertyInfo propertyInfo, str Type? declaringType = propertyInfo.DeclaringType; Debug.Assert(declaringType != null); - // declaredPropertyType: Type of the property - // runtimePropertyType: of JsonConverter / JsonPropertyInfo - - bool valueNeedsCastOrUnbox = declaredPropertyType != runtimePropertyType; + Type declaredPropertyType = propertyInfo.PropertyType; - DynamicMethod dynamicMethod = CreateSetterMethod(memberName, runtimePropertyType, valueNeedsCastOrUnbox); + DynamicMethod dynamicMethod = CreateSetterMethod(propertyInfo.Name, runtimePropertyType); ILGenerator generator = dynamicMethod.GetILGenerator(); - if (!valueNeedsCastOrUnbox) - { - generator.Emit(OpCodes.Ldarg_0); - generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); - generator.Emit(OpCodes.Ldarg_1); - } - else - { - LocalBuilder value = generator.DeclareLocal(declaredPropertyType); + generator.Emit(OpCodes.Ldarg_0); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); + generator.Emit(OpCodes.Ldarg_1); - // try - generator.BeginExceptionBlock(); + // declaredPropertyType: Type of the property + // runtimePropertyType: of JsonConverter / JsonPropertyInfo + if (declaredPropertyType != runtimePropertyType) + { // When applied to a reference type, the unbox.any instruction has the same effect as castclass. - generator.Emit(OpCodes.Ldarg_1); generator.Emit(OpCodes.Unbox_Any, declaredPropertyType); - generator.Emit(OpCodes.Stloc_0, value); - - EmitSetterCatchBlocks(generator); - - generator.EndExceptionBlock(); - - generator.Emit(OpCodes.Ldarg_0); - generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); - generator.Emit(OpCodes.Ldloc_0); } generator.Emit(declaringType.IsValueType ? OpCodes.Call : OpCodes.Callvirt, realMethod); @@ -365,11 +348,7 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtime ILGenerator generator = dynamicMethod.GetILGenerator(); generator.Emit(OpCodes.Ldarg_0); - generator.Emit( - declaringType.IsValueType - ? OpCodes.Unbox - : OpCodes.Castclass, - declaringType); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); generator.Emit(OpCodes.Ldfld, fieldInfo); // declaredFieldType: Type of the field @@ -388,70 +367,65 @@ private static DynamicMethod CreateFieldGetter(FieldInfo fieldInfo, Type runtime public override Action CreateFieldSetter(FieldInfo fieldInfo) => CreateFieldSetterDelegate(fieldInfo); - private static Action CreateFieldSetterDelegate(FieldInfo fieldInfo) + private static Action CreateFieldSetterDelegate(FieldInfo fieldInfo) { string memberName = fieldInfo.Name; Type declaredFieldType = fieldInfo.FieldType; Type runtimeFieldType = typeof(TProperty); - DynamicMethod fieldSetter = CreateFieldSetter(fieldInfo, memberName, declaredFieldType, runtimeFieldType); + Action fieldSetter = CreateDelegate>( + CreateFieldSetter(fieldInfo, runtimeFieldType)); if (declaredFieldType == runtimeFieldType) { // If declared and runtime type are identical, the assignment cannot fail, thus // there is no need for exception handling or closures. - return CreateDelegate>(fieldSetter); + return fieldSetter; } else { - // Return a delegate with some closures needed for creating the potential exception. - - Action fieldSetterDelegate = - CreateDelegate>(fieldSetter); + // Return a delegate with closures and exception handling. - return (object? obj, TProperty value) => fieldSetterDelegate(obj, value, memberName, declaredFieldType); + return (object obj, TProperty value) => + { + try + { + fieldSetter(obj, value); + } + catch (NullReferenceException) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignNull(memberName, declaredFieldType); + } + catch (InvalidCastException) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToAssignValue(value!.GetType(), memberName, declaredFieldType); + } + }; } } - private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, string memberName, Type declaredFieldType, Type runtimeFieldType) + private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, Type runtimeFieldType) { Type? declaringType = fieldInfo.DeclaringType; Debug.Assert(declaringType != null); - // declaredFieldType: Type of the field - // runtimeFieldType: of JsonConverter / JsonPropertyInfo - - bool valueNeedsCastOrUnbox = declaredFieldType != runtimeFieldType; + Type declaredFieldType = fieldInfo.FieldType; - DynamicMethod dynamicMethod = CreateSetterMethod(memberName, runtimeFieldType, valueNeedsCastOrUnbox); + DynamicMethod dynamicMethod = CreateSetterMethod(fieldInfo.Name, runtimeFieldType); ILGenerator generator = dynamicMethod.GetILGenerator(); - if (!valueNeedsCastOrUnbox) - { - generator.Emit(OpCodes.Ldarg_0); - generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); - generator.Emit(OpCodes.Ldarg_1); - } - else - { - LocalBuilder value = generator.DeclareLocal(declaredFieldType); + generator.Emit(OpCodes.Ldarg_0); + generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); + generator.Emit(OpCodes.Ldarg_1); - // try - generator.BeginExceptionBlock(); + // declaredFieldType: Type of the field + // runtimeFieldType: of JsonConverter / JsonPropertyInfo + if (declaredFieldType != runtimeFieldType) + { // When applied to a reference type, the unbox.any instruction has the same effect as castclass. - generator.Emit(OpCodes.Ldarg_1); generator.Emit(OpCodes.Unbox_Any, declaredFieldType); - generator.Emit(OpCodes.Stloc_0, value); - - EmitSetterCatchBlocks(generator); - - generator.EndExceptionBlock(); - - generator.Emit(OpCodes.Ldarg_0); - generator.Emit(declaringType.IsValueType ? OpCodes.Unbox : OpCodes.Castclass, declaringType); - generator.Emit(OpCodes.Ldloc_0); } generator.Emit(OpCodes.Stfld, fieldInfo); @@ -460,25 +434,6 @@ private static DynamicMethod CreateFieldSetter(FieldInfo fieldInfo, string membe return dynamicMethod; } - private static void EmitSetterCatchBlocks(ILGenerator generator) - { - // catch (NullReferenceException) - generator.BeginCatchBlock(s_nullReferenceException); - generator.Emit(OpCodes.Pop); - generator.Emit(OpCodes.Ldarg_2); - generator.Emit(OpCodes.Ldarg_3); - generator.Emit(OpCodes.Call, s_throwHelper_ThrowJsonException_DeserializeUnableToAssignNull); - - // catch (InvalidCastException) - generator.BeginCatchBlock(s_invalidCastExceptionType); - generator.Emit(OpCodes.Pop); - generator.Emit(OpCodes.Ldarg_1); - generator.Emit(OpCodes.Callvirt, s_object_GetType); - generator.Emit(OpCodes.Ldarg_2); - generator.Emit(OpCodes.Ldarg_3); - generator.Emit(OpCodes.Call, s_throwHelper_ThrowJsonException_DeserializeUnableToAssignValue); - } - private static DynamicMethod CreateGetterMethod(string memberName, Type memberType) => new DynamicMethod( memberName + "Getter", @@ -487,11 +442,11 @@ private static DynamicMethod CreateGetterMethod(string memberName, Type memberTy typeof(ReflectionEmitMemberAccessor).Module, skipVisibility: true); - private static DynamicMethod CreateSetterMethod(string memberName, Type memberType, bool valueNeedsCastOrUnbox) => + private static DynamicMethod CreateSetterMethod(string memberName, Type memberType) => new DynamicMethod( memberName + "Setter", typeof(void), - valueNeedsCastOrUnbox ? new[] { JsonClassInfo.ObjectType, memberType, s_stringType, s_typeType } : new[] { JsonClassInfo.ObjectType, memberType }, + new[] { JsonClassInfo.ObjectType, memberType }, typeof(ReflectionEmitMemberAccessor).Module, skipVisibility: true);