From 4c6469e5066e104363f19d1ac68fec9e55dd89ae Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Tue, 5 Oct 2021 12:09:24 -0700 Subject: [PATCH 1/4] Prevent runtime prop metadata retrieval when [JsonIgnore] is used --- .../Common/ReflectionExtensions.cs | 13 ++++ .../gen/JsonSourceGenerator.Emitter.cs | 15 ++-- .../gen/JsonSourceGenerator.Parser.cs | 23 +++++- ...ParameterizedConstructorConverter.Large.cs | 5 +- ...ParameterizedConstructorConverter.Small.cs | 44 ++++++----- .../Metadata/JsonParameterInfo.cs | 8 +- .../Metadata/JsonParameterInfoOfT.cs | 4 +- .../Metadata/JsonPropertyInfo.cs | 4 +- .../Metadata/JsonTypeInfo.Cache.cs | 12 +-- .../Serialization/Metadata/JsonTypeInfo.cs | 2 +- .../ConstructorTests.ParameterMatching.cs | 43 +++++++++++ .../tests/Common/PropertyVisibilityTests.cs | 75 +++++++++++++++++++ .../TestClasses/TestClasses.Constructor.cs | 2 +- .../Serialization/ConstructorTests.cs | 4 + .../Serialization/PropertyVisibilityTests.cs | 8 ++ 15 files changed, 207 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.Text.Json/Common/ReflectionExtensions.cs b/src/libraries/System.Text.Json/Common/ReflectionExtensions.cs index 8a1a58a97a2cba..09ecc45077fe06 100644 --- a/src/libraries/System.Text.Json/Common/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/Common/ReflectionExtensions.cs @@ -310,5 +310,18 @@ public static bool TryGetDeserializationConstructor( deserializationCtor = ctorWithAttribute ?? publicParameterlessCtor ?? lonePublicCtor; return true; } + + public static object? GetDefaultValue(this ParameterInfo parameterInfo) + { + object? defaultValue = parameterInfo.DefaultValue; + + // DBNull.Value is sometimes used as the default value (returned by reflection) of nullable params in place of null. + if (defaultValue == DBNull.Value && parameterInfo.ParameterType != typeof(DBNull)) + { + return null; + } + + return defaultValue; + } } } diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs index c17eb09a805e7a..996232479bc6b6 100644 --- a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs +++ b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs @@ -797,23 +797,28 @@ private string GenerateCtorParamMetadataInitFunc(TypeGenerationSpec typeGenerati private static {JsonParameterInfoValuesTypeRef}[] {typeGenerationSpec.TypeInfoPropertyName}{CtorParamInitMethodNameSuffix}() {{ {JsonParameterInfoValuesTypeRef}[] {parametersVarName} = new {JsonParameterInfoValuesTypeRef}[{paramCount}]; - {JsonParameterInfoValuesTypeRef} info; "); for (int i = 0; i < paramCount; i++) { ParameterInfo reflectionInfo = parameters[i].ParameterInfo; + string parameterTypeRef = reflectionInfo.ParameterType.GetCompilableName(); + + object? defaultValue = reflectionInfo.GetDefaultValue(); + string defaultValueAsStr = defaultValue == null + ? $"default({parameterTypeRef})" + : GetParamDefaultValueAsString(defaultValue); + sb.Append(@$" - {InfoVarName} = new() + {parametersVarName}[{i}] = new() {{ Name = ""{reflectionInfo.Name!}"", - ParameterType = typeof({reflectionInfo.ParameterType.GetCompilableName()}), + ParameterType = typeof({parameterTypeRef}), Position = {reflectionInfo.Position}, HasDefaultValue = {ToCSharpKeyword(reflectionInfo.HasDefaultValue)}, - DefaultValue = {GetParamDefaultValueAsString(reflectionInfo.DefaultValue)} + DefaultValue = {defaultValueAsStr} }}; - {parametersVarName}[{i}] = {InfoVarName}; "); } diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs index bbdd17bddb8c22..2785e66c7cd3a3 100644 --- a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs +++ b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs @@ -26,6 +26,7 @@ private sealed class Parser private const string SystemTextJsonNamespace = "System.Text.Json"; private const string JsonConverterAttributeFullName = "System.Text.Json.Serialization.JsonConverterAttribute"; private const string JsonConverterFactoryFullName = "System.Text.Json.Serialization.JsonConverterFactory"; + private const string JsonConverterOfTFullName = "System.Text.Json.Serialization.JsonConverter`1"; private const string JsonArrayFullName = "System.Text.Json.Nodes.JsonArray"; private const string JsonElementFullName = "System.Text.Json.JsonElement"; private const string JsonExtensionDataAttributeFullName = "System.Text.Json.Serialization.JsonExtensionDataAttribute"; @@ -101,6 +102,9 @@ private sealed class Parser private readonly Type? _dateOnlyType; private readonly Type? _timeOnlyType; + // Needed for converter validation + private readonly Type _jsonConverterOfTType; + private readonly HashSet _numberTypes = new(); private readonly HashSet _knownTypes = new(); private readonly HashSet _knownUnsupportedTypes = new(); @@ -215,6 +219,8 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene _dateOnlyType = _metadataLoadContext.Resolve(DateOnlyFullName); _timeOnlyType = _metadataLoadContext.Resolve(TimeOnlyFullName); + _jsonConverterOfTType = _metadataLoadContext.Resolve(JsonConverterOfTFullName); + PopulateKnownTypes(); } @@ -224,8 +230,12 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene INamedTypeSymbol jsonSerializerContextSymbol = compilation.GetBestTypeByMetadataName(JsonSerializerContextFullName); INamedTypeSymbol jsonSerializableAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonSerializerAttributeFullName); INamedTypeSymbol jsonSourceGenerationOptionsAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonSourceGenerationOptionsAttributeFullName); + INamedTypeSymbol jsonConverterOfTAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonConverterOfTFullName); - if (jsonSerializerContextSymbol == null || jsonSerializableAttributeSymbol == null || jsonSourceGenerationOptionsAttributeSymbol == null) + if (jsonSerializerContextSymbol == null || + jsonSerializableAttributeSymbol == null || + jsonSourceGenerationOptionsAttributeSymbol == null || + jsonConverterOfTAttributeSymbol == null) { return null; } @@ -1354,7 +1364,14 @@ private static bool PropertyAccessorCanBeReferenced(MethodInfo? accessor) return null; } - if (converterType.GetCompatibleBaseClass(JsonConverterFactoryFullName) != null) + // Validated when creating the source generation spec. + Debug.Assert(_jsonConverterOfTType != null); + + if (converterType.GetCompatibleGenericBaseClass(_jsonConverterOfTType) != null) + { + return $"new {converterType.GetCompilableName()}()"; + } + else if (converterType.GetCompatibleBaseClass(JsonConverterFactoryFullName) != null) { hasFactoryConverter = true; @@ -1368,7 +1385,7 @@ private static bool PropertyAccessorCanBeReferenced(MethodInfo? accessor) } } - return $"new {converterType.GetCompilableName()}()"; + return null; } private static string DetermineRuntimePropName(string clrPropName, string? jsonPropName, JsonKnownNamingPolicy namingPolicy) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index 3e25b10c13a237..f820ffbfe3a858 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -67,10 +67,7 @@ protected sealed override void InitializeConstructorArgumentCaches(ref ReadStack { JsonParameterInfo? parameterInfo = cache[i].Value; Debug.Assert(parameterInfo != null); - - arguments[parameterInfo.ClrInfo.Position] = parameterInfo.ShouldDeserialize - ? parameterInfo.DefaultValue - : parameterInfo.ClrDefaultValue; + arguments[parameterInfo.ClrInfo.Position] = parameterInfo.DefaultValue; } state.Current.CtorArgumentState!.Arguments = arguments; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index fc16d5f4c47994..ada072ea9ef978 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -92,34 +92,32 @@ protected override void InitializeConstructorArgumentCaches(ref ReadStack state, JsonParameterInfo? parameterInfo = cache[i].Value; Debug.Assert(parameterInfo != null); - // We can afford not to set default values for ctor arguments when we should't deserialize because the - // type parameters of the `Arguments` type provide default semantics that work well with value types. - if (parameterInfo.ShouldDeserialize) + switch (parameterInfo.ClrInfo.Position) { - int position = parameterInfo.ClrInfo.Position; - - switch (position) - { - case 0: - arguments.Arg0 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; - break; - case 1: - arguments.Arg1 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; - break; - case 2: - arguments.Arg2 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; - break; - case 3: - arguments.Arg3 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; - break; - default: - Debug.Fail("More than 4 params: we should be in override for LargeObjectWithParameterizedConstructorConverter."); - throw new InvalidOperationException(); - } + case 0: + arguments.Arg0 = GetDefaultValue(parameterInfo); + break; + case 1: + arguments.Arg1 = GetDefaultValue(parameterInfo); + break; + case 2: + arguments.Arg2 = GetDefaultValue(parameterInfo); + break; + case 3: + arguments.Arg3 = GetDefaultValue(parameterInfo); + break; + default: + Debug.Fail("More than 4 params: we should be in override for LargeObjectWithParameterizedConstructorConverter."); + throw new InvalidOperationException(); } } state.Current.CtorArgumentState!.Arguments = arguments; + + static TArg GetDefaultValue(JsonParameterInfo parameterInfo) + => parameterInfo.ShouldDeserialize + ? ((JsonParameterInfo)parameterInfo).TypedDefaultValue + : parameterInfo.ClrInfo.HasDefaultValue ? (TArg?)parameterInfo.DefaultValue! : default(TArg)!; } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs index ae2768ae0f5343..19d45242b57289 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs @@ -18,8 +18,6 @@ internal abstract class JsonParameterInfo private protected bool MatchingPropertyCanBeNull { get; private set; } - internal abstract object? ClrDefaultValue { get; } - // The default value of the parameter. This is `DefaultValue` of the `ParameterInfo`, if specified, or the CLR `default` for the `ParameterType`. public object? DefaultValue { get; private protected set; } @@ -79,14 +77,12 @@ public virtual void Initialize(JsonParameterInfoValues parameterInfo, JsonProper // prevent issues with unsupported types and helps ensure we don't accidently (de)serialize it. public static JsonParameterInfo CreateIgnoredParameterPlaceholder(JsonParameterInfoValues parameterInfo, JsonPropertyInfo matchingProperty) { - JsonParameterInfo jsonParameterInfo = matchingProperty.ConverterBase.CreateJsonParameterInfo(); + JsonParameterInfo jsonParameterInfo = new JsonParameterInfo(); jsonParameterInfo.ClrInfo = parameterInfo; jsonParameterInfo.RuntimePropertyType = matchingProperty.RuntimePropertyType!; jsonParameterInfo.NameAsUtf8Bytes = matchingProperty.NameAsUtf8Bytes!; - jsonParameterInfo.InitializeDefaultValue(matchingProperty); + jsonParameterInfo.DefaultValue = parameterInfo.DefaultValue; return jsonParameterInfo; } - - protected abstract void InitializeDefaultValue(JsonPropertyInfo matchingProperty); } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfoOfT.cs index 2195426a551edf..b275f01acd0952 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfoOfT.cs @@ -11,8 +11,6 @@ namespace System.Text.Json.Serialization.Metadata /// internal sealed class JsonParameterInfo : JsonParameterInfo { - internal override object? ClrDefaultValue => default(T); - public T TypedDefaultValue { get; private set; } = default!; public override void Initialize(JsonParameterInfoValues parameterInfo, JsonPropertyInfo matchingProperty, JsonSerializerOptions options) @@ -21,7 +19,7 @@ public override void Initialize(JsonParameterInfoValues parameterInfo, JsonPrope InitializeDefaultValue(matchingProperty); } - protected override void InitializeDefaultValue(JsonPropertyInfo matchingProperty) + private void InitializeDefaultValue(JsonPropertyInfo matchingProperty) { Debug.Assert(ClrInfo.ParameterType == matchingProperty.DeclaredPropertyType); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 3b8c003ac3066c..288ae5d5e1d03d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -42,16 +42,14 @@ internal static JsonPropertyInfo GetPropertyPlaceholder() // Create a property that is ignored at run-time. internal static JsonPropertyInfo CreateIgnoredPropertyPlaceholder( - JsonConverter converter, MemberInfo memberInfo, Type memberType, bool isVirtual, JsonSerializerOptions options) { - JsonPropertyInfo jsonPropertyInfo = converter.CreateJsonPropertyInfo(); + JsonPropertyInfo jsonPropertyInfo = new JsonPropertyInfo(); jsonPropertyInfo.Options = options; - jsonPropertyInfo.ConverterBase = converter; jsonPropertyInfo.MemberInfo = memberInfo; jsonPropertyInfo.IsIgnored = true; jsonPropertyInfo.DeclaredPropertyType = memberType; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index d3f2c3c9bee6f1..aa8ccc58849b6c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -58,6 +58,12 @@ internal static JsonPropertyInfo AddProperty( JsonNumberHandling? parentTypeNumberHandling, JsonSerializerOptions options) { + JsonIgnoreCondition? ignoreCondition = JsonPropertyInfo.GetAttribute(memberInfo)?.Condition; + if (ignoreCondition == JsonIgnoreCondition.Always) + { + return JsonPropertyInfo.CreateIgnoredPropertyPlaceholder(memberInfo, memberType, isVirtual, options); + } + JsonConverter converter = GetConverter( memberType, parentClassType, @@ -65,12 +71,6 @@ internal static JsonPropertyInfo AddProperty( out Type runtimeType, options); - JsonIgnoreCondition? ignoreCondition = JsonPropertyInfo.GetAttribute(memberInfo)?.Condition; - if (ignoreCondition == JsonIgnoreCondition.Always) - { - return JsonPropertyInfo.CreateIgnoredPropertyPlaceholder(converter, memberInfo, memberType, isVirtual, options); - } - return CreateProperty( declaredPropertyType: memberType, runtimePropertyType: runtimeType, diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index 55dd81410ce3a1..849ebad4c6056d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -527,7 +527,7 @@ private static JsonParameterInfoValues[] GetParameterInfoArray(ParameterInfo[] p ParameterType = reflectionInfo.ParameterType, Position = reflectionInfo.Position, HasDefaultValue = reflectionInfo.HasDefaultValue, - DefaultValue = reflectionInfo.DefaultValue + DefaultValue = reflectionInfo.GetDefaultValue() }; jsonParameters[i] = jsonInfo; diff --git a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs index c04815dc85e6a1..735245f22df550 100644 --- a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs +++ b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs @@ -1216,5 +1216,48 @@ public class TypeWithUri public TypeWithUri(Uri myUri = default) => MyUri = myUri; } + + [Fact] + public async Task SmallObject_DefaultParamValueUsed_WhenMatchingPropIgnored() + { + string json = @"{""Prop"":20}"; + var obj = await JsonSerializerWrapperForString.DeserializeWrapper(json); + Assert.Equal(5, obj.Prop); + } + + public class SmallType_IgnoredProp_Bind_ParamWithDefaultValue + { + [JsonIgnore] + public int Prop { get; set; } + + public SmallType_IgnoredProp_Bind_ParamWithDefaultValue(int prop = 5) + => Prop = prop; + } + + + [Fact] + public async Task LargeObject_DefaultParamValueUsed_WhenMatchingPropIgnored() + { + string json = @"{""Prop"":20}"; + var obj = await JsonSerializerWrapperForString.DeserializeWrapper(json); + Assert.Equal(5, obj.Prop); + } + + public class LargeType_IgnoredProp_Bind_ParamWithDefaultValue + { + public int W { get; set; } + + public int X { get; set; } + + public int Y { get; set; } + + public int Z { get; set; } + + [JsonIgnore] + public int Prop { get; set; } + + public LargeType_IgnoredProp_Bind_ParamWithDefaultValue(int w, int x, int y, int z, int prop = 5) + => Prop = prop; + } } } diff --git a/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.cs index a2b5845ca44dca..a41380e3e60d9c 100644 --- a/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Common/PropertyVisibilityTests.cs @@ -2742,5 +2742,80 @@ public async Task JsonIgnoreCondition_Polymorphic() Assert.Equal(3, obj.Abstract_Property); Assert.Equal(4, obj.Virtual_Property); } + + [Fact] + public async Task SerializationMetadataNotComputedWhenMemberIgnored() + { + string janePayload = @"{""Name"":""Jane Doe""}"; + +#if !BUILDING_SOURCE_GENERATOR_TESTS + // Without [JsonIgnore], serializer throws exceptions due to runtime-reflection-based property metadata inspection. + await Assert.ThrowsAsync(async () => await JsonSerializerWrapperForString.SerializeWrapper(new TypeWith_RefStringProp())); + await Assert.ThrowsAsync(async () => await JsonSerializerWrapperForString.DeserializeWrapper("{}")); + + await Assert.ThrowsAsync(async () => await JsonSerializerWrapperForString.SerializeWrapper(new TypeWith_PropWith_BadConverter())); + await Assert.ThrowsAsync(async () => await JsonSerializerWrapperForString.DeserializeWrapper("{}")); +#else + // Ref returns supported in source-gen mode + string expected = @"{""NameRef"":""John Doe"",""Name"":""John Doe""}"; + JsonTestHelper.AssertJsonEqual(expected, await JsonSerializerWrapperForString.SerializeWrapper(new TypeWith_RefStringProp())); + + var obj = await JsonSerializerWrapperForString.DeserializeWrapper(janePayload); + Assert.Equal("Jane Doe", obj.Name); + Assert.Equal("Jane Doe", obj.NameRef); + + var obj2 = new TypeWith_PropWith_BadConverter(); + obj2.Property = "Hello"; + + // Invalid converter specified, fallback to built-in converter. This should be corrected. + // https://github.com/dotnet/runtime/issues/60020. + + Assert.Equal(@"{""Property"":""Hello""}", await JsonSerializerWrapperForString.SerializeWrapper(obj2)); + + obj2 = await JsonSerializerWrapperForString.DeserializeWrapper(@"{""Property"":""World""}"); + Assert.Equal("World", obj2.Property); +#endif + + // With [JsonIgnore], serializer skips property metadata inspection + Assert.Equal(@"{""Name"":""John Doe""}", await JsonSerializerWrapperForString.SerializeWrapper(new TypeWith_IgnoredRefStringProp())); + Assert.Equal("Jane Doe", (await JsonSerializerWrapperForString.DeserializeWrapper(janePayload)).Name); + + Assert.Equal("{}", await JsonSerializerWrapperForString.SerializeWrapper(new TypeWith_IgnoredPropWith_BadConverter())); + Assert.Null((await JsonSerializerWrapperForString.DeserializeWrapper("{}")).Property); + } + + internal class TypeWith_RefStringProp + { + public ref string NameRef => ref Name; + + [JsonInclude] // This is a field. + public string Name = "John Doe"; + } + + internal class TypeWith_IgnoredRefStringProp + { + [JsonIgnore] + public ref string NameRef => ref Name; + + [JsonInclude] // This is a field. + public string Name = "John Doe"; + } + + public class TypeWith_PropWith_BadConverter + { + [JsonConverter(typeof(BadConverter))] + public string? Property { get; set; } + } + + public class TypeWith_IgnoredPropWith_BadConverter + { + [JsonIgnore] + [JsonConverter(typeof(BadConverter))] + public string? Property { get; set; } + } + + public class BadConverter + { + } } } diff --git a/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs index 890d61f97245d1..07be4540d3470b 100644 --- a/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs +++ b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs @@ -2056,7 +2056,7 @@ public void Initialize() { } public void Verify() { Assert.Equal(0, X); - Assert.Equal(0, Y); // We don't set parameter default value here. + Assert.Equal(5, Y); } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs index f0d1cb897ce2dd..5fa46302e9839e 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs @@ -125,6 +125,8 @@ protected ConstructorTests_Metadata(JsonSerializerWrapperForString stringWrapper [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(Parameterized_Class_With_ComplexTuple))] [JsonSerializable(typeof(Parameterized_Person_Simple))] + [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_ParamWithDefaultValue))] + [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] internal sealed partial class ConstructorTestsContext_Metadata : JsonSerializerContext { } @@ -241,6 +243,8 @@ public ConstructorTests_Default() [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(Parameterized_Class_With_ComplexTuple))] [JsonSerializable(typeof(Parameterized_Person_Simple))] + [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_ParamWithDefaultValue))] + [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] internal sealed partial class ConstructorTestsContext_Default : JsonSerializerContext { } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyVisibilityTests.cs index f4858804d60123..3b38cefe23e98c 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/PropertyVisibilityTests.cs @@ -264,6 +264,10 @@ public override async Task HonorJsonPropertyName_PrivateSetter() [JsonSerializable(typeof(ClassWithValueAndReferenceTypes))] [JsonSerializable(typeof(ClassWithReadOnlyStringProperty_IgnoreWhenWritingDefault))] [JsonSerializable(typeof(ConcreteDerivedClass))] + [JsonSerializable(typeof(TypeWith_RefStringProp))] + [JsonSerializable(typeof(TypeWith_IgnoredRefStringProp))] + [JsonSerializable(typeof(TypeWith_PropWith_BadConverter))] + [JsonSerializable(typeof(TypeWith_IgnoredPropWith_BadConverter))] internal sealed partial class PropertyVisibilityTestsContext_Metadata : JsonSerializerContext { } @@ -431,6 +435,10 @@ public override async Task JsonIgnoreCondition_WhenWritingNull_OnValueType_Fail_ [JsonSerializable(typeof(ClassWithValueAndReferenceTypes))] [JsonSerializable(typeof(ClassWithReadOnlyStringProperty_IgnoreWhenWritingDefault))] [JsonSerializable(typeof(ConcreteDerivedClass))] + [JsonSerializable(typeof(TypeWith_RefStringProp))] + [JsonSerializable(typeof(TypeWith_IgnoredRefStringProp))] + [JsonSerializable(typeof(TypeWith_PropWith_BadConverter))] + [JsonSerializable(typeof(TypeWith_IgnoredPropWith_BadConverter))] internal sealed partial class PropertyVisibilityTestsContext_Default : JsonSerializerContext { } From fbae7bbbbe4c90a9636795459b1586736e2e0841 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Wed, 6 Oct 2021 13:19:37 -0700 Subject: [PATCH 2/4] Address feedback --- .../gen/JsonSourceGenerator.Emitter.cs | 12 ++--- ...ParameterizedConstructorConverter.Small.cs | 44 ++++++++++--------- .../Metadata/GenericMethodHolder.cs | 40 +++++++++++++++++ .../Metadata/JsonParameterInfo.cs | 30 +++++++++++-- .../Metadata/JsonPropertyInfo.cs | 5 +++ .../Metadata/JsonPropertyInfoOfT.cs | 3 ++ .../Metadata/JsonTypeInfo.Cache.cs | 2 +- .../Serialization/Metadata/JsonTypeInfo.cs | 14 +++--- .../ConstructorTests.ParameterMatching.cs | 43 ------------------ .../TestClasses/TestClasses.Constructor.cs | 2 +- .../Serialization/ConstructorTests.cs | 4 -- 11 files changed, 110 insertions(+), 89 deletions(-) diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs index 996232479bc6b6..ec6124a8222be1 100644 --- a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs +++ b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs @@ -797,6 +797,7 @@ private string GenerateCtorParamMetadataInitFunc(TypeGenerationSpec typeGenerati private static {JsonParameterInfoValuesTypeRef}[] {typeGenerationSpec.TypeInfoPropertyName}{CtorParamInitMethodNameSuffix}() {{ {JsonParameterInfoValuesTypeRef}[] {parametersVarName} = new {JsonParameterInfoValuesTypeRef}[{paramCount}]; + {JsonParameterInfoValuesTypeRef} info; "); for (int i = 0; i < paramCount; i++) @@ -806,12 +807,10 @@ private string GenerateCtorParamMetadataInitFunc(TypeGenerationSpec typeGenerati string parameterTypeRef = reflectionInfo.ParameterType.GetCompilableName(); object? defaultValue = reflectionInfo.GetDefaultValue(); - string defaultValueAsStr = defaultValue == null - ? $"default({parameterTypeRef})" - : GetParamDefaultValueAsString(defaultValue); + string defaultValueAsStr = GetParamDefaultValueAsString(defaultValue, parameterTypeRef); sb.Append(@$" - {parametersVarName}[{i}] = new() + {InfoVarName} = new() {{ Name = ""{reflectionInfo.Name!}"", ParameterType = typeof({parameterTypeRef}), @@ -819,6 +818,7 @@ private string GenerateCtorParamMetadataInitFunc(TypeGenerationSpec typeGenerati HasDefaultValue = {ToCSharpKeyword(reflectionInfo.HasDefaultValue)}, DefaultValue = {defaultValueAsStr} }}; + {parametersVarName}[{i}] = {InfoVarName}; "); } @@ -1318,12 +1318,12 @@ private static string GetNumberHandlingAsStr(JsonNumberHandling? numberHandling) private static string ToCSharpKeyword(bool value) => value.ToString().ToLowerInvariant(); - private static string GetParamDefaultValueAsString(object? value) + private static string GetParamDefaultValueAsString(object? value, string objectTypeAsStr) { switch (value) { case null: - return "null"; + return $"default({objectTypeAsStr})"; case bool boolVal: return ToCSharpKeyword(boolVal); default: diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index ada072ea9ef978..fc16d5f4c47994 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -92,32 +92,34 @@ protected override void InitializeConstructorArgumentCaches(ref ReadStack state, JsonParameterInfo? parameterInfo = cache[i].Value; Debug.Assert(parameterInfo != null); - switch (parameterInfo.ClrInfo.Position) + // We can afford not to set default values for ctor arguments when we should't deserialize because the + // type parameters of the `Arguments` type provide default semantics that work well with value types. + if (parameterInfo.ShouldDeserialize) { - case 0: - arguments.Arg0 = GetDefaultValue(parameterInfo); - break; - case 1: - arguments.Arg1 = GetDefaultValue(parameterInfo); - break; - case 2: - arguments.Arg2 = GetDefaultValue(parameterInfo); - break; - case 3: - arguments.Arg3 = GetDefaultValue(parameterInfo); - break; - default: - Debug.Fail("More than 4 params: we should be in override for LargeObjectWithParameterizedConstructorConverter."); - throw new InvalidOperationException(); + int position = parameterInfo.ClrInfo.Position; + + switch (position) + { + case 0: + arguments.Arg0 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; + break; + case 1: + arguments.Arg1 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; + break; + case 2: + arguments.Arg2 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; + break; + case 3: + arguments.Arg3 = ((JsonParameterInfo)parameterInfo).TypedDefaultValue!; + break; + default: + Debug.Fail("More than 4 params: we should be in override for LargeObjectWithParameterizedConstructorConverter."); + throw new InvalidOperationException(); + } } } state.Current.CtorArgumentState!.Arguments = arguments; - - static TArg GetDefaultValue(JsonParameterInfo parameterInfo) - => parameterInfo.ShouldDeserialize - ? ((JsonParameterInfo)parameterInfo).TypedDefaultValue - : parameterInfo.ClrInfo.HasDefaultValue ? (TArg?)parameterInfo.DefaultValue! : default(TArg)!; } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs index a144c11d398c9d..24957202b5a2d3 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs @@ -1,6 +1,7 @@ // 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.Concurrent; using System.Collections.Generic; using System.Diagnostics; using System.Text.Json.Reflection; @@ -12,10 +13,47 @@ namespace System.Text.Json.Serialization.Metadata /// internal abstract class GenericMethodHolder { + private static ConcurrentDictionary? s_holders; + + /// + /// Returns the default value for the specified type. + /// + public abstract object? DefaultValue { get; } + /// /// Returns true if contains only default values. /// public abstract bool IsDefaultValue(object value); + + /// + /// Returns a holder instance representing a type. + /// + public static GenericMethodHolder GetHolder(Type type) + { + if (s_holders == null) + { + s_holders = new ConcurrentDictionary(); + return CreateAndCacheHolder(); + } + else + { + if (!s_holders.TryGetValue(type, out GenericMethodHolder? holder)) + { + holder = CreateAndCacheHolder(); + } + + return holder; + } + + GenericMethodHolder CreateAndCacheHolder() + { + Type holderType = typeof(GenericMethodHolder<>).MakeGenericType(type); + GenericMethodHolder holderInstance = (GenericMethodHolder)Activator.CreateInstance(holderType)!; + Debug.Assert(s_holders != null); + s_holders[type] = holderInstance; + return holderInstance; + } + } } /// @@ -23,6 +61,8 @@ internal abstract class GenericMethodHolder /// internal sealed class GenericMethodHolder : GenericMethodHolder { + public override object? DefaultValue => default(T); + public override bool IsDefaultValue(object value) { // For performance, we only want to call this method for non-nullable value types. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs index 19d45242b57289..6f62cf48f8496b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs @@ -73,15 +73,37 @@ public virtual void Initialize(JsonParameterInfoValues parameterInfo, JsonProper MatchingPropertyCanBeNull = matchingProperty.PropertyTypeCanBeNull; } - // Create a parameter that is ignored at run time. It uses the same type (typeof(sbyte)) to help - // prevent issues with unsupported types and helps ensure we don't accidently (de)serialize it. - public static JsonParameterInfo CreateIgnoredParameterPlaceholder(JsonParameterInfoValues parameterInfo, JsonPropertyInfo matchingProperty) + /// + /// Create a parameter that is ignored at run time. It uses the same type (typeof(sbyte)) to help + /// prevent issues with unsupported types and helps ensure we don't accidently (de)serialize it. + /// + public static JsonParameterInfo CreateIgnoredParameterPlaceholder( + JsonParameterInfoValues parameterInfo, + JsonPropertyInfo matchingProperty, + bool sourceGenMode) { JsonParameterInfo jsonParameterInfo = new JsonParameterInfo(); jsonParameterInfo.ClrInfo = parameterInfo; jsonParameterInfo.RuntimePropertyType = matchingProperty.RuntimePropertyType!; jsonParameterInfo.NameAsUtf8Bytes = matchingProperty.NameAsUtf8Bytes!; - jsonParameterInfo.DefaultValue = parameterInfo.DefaultValue; + + // TODO: https://github.com/dotnet/runtime/issues/60082. + // Default value initialization for params mapping to ignored properties doesn't + // account for the default value of optional parameters. This should be fixed. + + if (sourceGenMode) + { + // The value in the matching JsonPropertyInfo instance matches the parameter type. + jsonParameterInfo.DefaultValue = matchingProperty.DefaultValue; + } + else + { + // The value in the created JsonPropertyInfo instance (sbyte) + // doesn't match the parameter type, use reflection to get the default value. + GenericMethodHolder holder = GenericMethodHolder.GetHolder(parameterInfo.ParameterType); + jsonParameterInfo.DefaultValue = holder.DefaultValue; + } + return jsonParameterInfo; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 288ae5d5e1d03d..5e2258ec6dde83 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -523,5 +523,10 @@ internal JsonTypeInfo RuntimeTypeInfo internal string? ClrName { get; set; } internal bool IsVirtual { get; set; } + + /// + /// Default value used for parameterized ctor invocation. + /// + internal abstract object? DefaultValue { get; } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index f60f0450701b9d..e98f08dd54cdcd 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -28,8 +28,11 @@ internal sealed class JsonPropertyInfo : JsonPropertyInfo private bool _propertyTypeEqualsTypeToConvert; internal Func? Get { get; set; } + internal Action? Set { get; set; } + internal override object? DefaultValue => default(T); + public JsonConverter Converter { get; internal set; } = null!; internal override void Initialize( diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index aa8ccc58849b6c..f696cefe8f21cd 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -646,7 +646,7 @@ internal void InitializeParameterCache() return; } - InitializeConstructorParameters(array); + InitializeConstructorParameters(array, sourceGenMode: true); Debug.Assert(ParameterCache != null); } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index 849ebad4c6056d..821c7fd59761fd 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -130,12 +130,7 @@ internal GenericMethodHolder GenericMethods { get { - if (_genericMethods == null) - { - Type runtimePropertyClass = typeof(GenericMethodHolder<>).MakeGenericType(new Type[] { Type })!; - _genericMethods = (GenericMethodHolder)Activator.CreateInstance(runtimePropertyClass)!; - } - + _genericMethods ??= GenericMethodHolder.GetHolder(Type); return _genericMethods; } } @@ -451,7 +446,7 @@ public ParameterLookupValue(JsonPropertyInfo jsonPropertyInfo) public JsonPropertyInfo JsonPropertyInfo { get; } } - private void InitializeConstructorParameters(JsonParameterInfoValues[] jsonParameters) + private void InitializeConstructorParameters(JsonParameterInfoValues[] jsonParameters, bool sourceGenMode = false) { var parameterCache = new JsonPropertyDictionary(Options.PropertyNameCaseInsensitive, jsonParameters.Length); @@ -497,7 +492,7 @@ private void InitializeConstructorParameters(JsonParameterInfoValues[] jsonParam Debug.Assert(matchingEntry.JsonPropertyInfo != null); JsonPropertyInfo jsonPropertyInfo = matchingEntry.JsonPropertyInfo; - JsonParameterInfo jsonParameterInfo = CreateConstructorParameter(parameterInfo, jsonPropertyInfo, Options); + JsonParameterInfo jsonParameterInfo = CreateConstructorParameter(parameterInfo, jsonPropertyInfo, sourceGenMode, Options); parameterCache.Add(jsonPropertyInfo.NameAsString, jsonParameterInfo); } // It is invalid for the extension data property to bind with a constructor argument. @@ -577,11 +572,12 @@ private bool IsValidDataExtensionProperty(JsonPropertyInfo jsonPropertyInfo) private static JsonParameterInfo CreateConstructorParameter( JsonParameterInfoValues parameterInfo, JsonPropertyInfo jsonPropertyInfo, + bool sourceGenMode, JsonSerializerOptions options) { if (jsonPropertyInfo.IsIgnored) { - return JsonParameterInfo.CreateIgnoredParameterPlaceholder(parameterInfo, jsonPropertyInfo); + return JsonParameterInfo.CreateIgnoredParameterPlaceholder(parameterInfo, jsonPropertyInfo, sourceGenMode); } JsonConverter converter = jsonPropertyInfo.ConverterBase; diff --git a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs index 735245f22df550..c04815dc85e6a1 100644 --- a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs +++ b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs @@ -1216,48 +1216,5 @@ public class TypeWithUri public TypeWithUri(Uri myUri = default) => MyUri = myUri; } - - [Fact] - public async Task SmallObject_DefaultParamValueUsed_WhenMatchingPropIgnored() - { - string json = @"{""Prop"":20}"; - var obj = await JsonSerializerWrapperForString.DeserializeWrapper(json); - Assert.Equal(5, obj.Prop); - } - - public class SmallType_IgnoredProp_Bind_ParamWithDefaultValue - { - [JsonIgnore] - public int Prop { get; set; } - - public SmallType_IgnoredProp_Bind_ParamWithDefaultValue(int prop = 5) - => Prop = prop; - } - - - [Fact] - public async Task LargeObject_DefaultParamValueUsed_WhenMatchingPropIgnored() - { - string json = @"{""Prop"":20}"; - var obj = await JsonSerializerWrapperForString.DeserializeWrapper(json); - Assert.Equal(5, obj.Prop); - } - - public class LargeType_IgnoredProp_Bind_ParamWithDefaultValue - { - public int W { get; set; } - - public int X { get; set; } - - public int Y { get; set; } - - public int Z { get; set; } - - [JsonIgnore] - public int Prop { get; set; } - - public LargeType_IgnoredProp_Bind_ParamWithDefaultValue(int w, int x, int y, int z, int prop = 5) - => Prop = prop; - } } } diff --git a/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs index 07be4540d3470b..890d61f97245d1 100644 --- a/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs +++ b/src/libraries/System.Text.Json/tests/Common/TestClasses/TestClasses.Constructor.cs @@ -2056,7 +2056,7 @@ public void Initialize() { } public void Verify() { Assert.Equal(0, X); - Assert.Equal(5, Y); + Assert.Equal(0, Y); // We don't set parameter default value here. } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs index 5fa46302e9839e..f0d1cb897ce2dd 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs @@ -125,8 +125,6 @@ protected ConstructorTests_Metadata(JsonSerializerWrapperForString stringWrapper [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(Parameterized_Class_With_ComplexTuple))] [JsonSerializable(typeof(Parameterized_Person_Simple))] - [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_ParamWithDefaultValue))] - [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] internal sealed partial class ConstructorTestsContext_Metadata : JsonSerializerContext { } @@ -243,8 +241,6 @@ public ConstructorTests_Default() [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(Parameterized_Class_With_ComplexTuple))] [JsonSerializable(typeof(Parameterized_Person_Simple))] - [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_ParamWithDefaultValue))] - [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] internal sealed partial class ConstructorTestsContext_Default : JsonSerializerContext { } From 43ac70d1b98c6053e91d32d539d1e5a707c25584 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Mon, 11 Oct 2021 07:53:49 -0700 Subject: [PATCH 3/4] Re-add ctor param default handling tests --- .../ConstructorTests.ParameterMatching.cs | 91 +++++++++++++++++++ .../Serialization/ConstructorTests.cs | 8 ++ 2 files changed, 99 insertions(+) diff --git a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs index c04815dc85e6a1..e264ee95afd7c2 100644 --- a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs +++ b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs @@ -1216,5 +1216,96 @@ public class TypeWithUri public TypeWithUri(Uri myUri = default) => MyUri = myUri; } + + [Fact] + public async Task SmallObject_ClrDefaultParamValueUsed_WhenMatchingPropIgnored() + { + string json = @"{""Prop"":20}"; + var obj1 = await JsonSerializerWrapperForString.DeserializeWrapper(json); + Assert.Equal(0, obj1.Prop); + + var obj2 = await JsonSerializerWrapperForString.DeserializeWrapper(json); + Assert.Equal(0, obj2.Prop); + } + + public class SmallType_IgnoredProp_Bind_ParamWithDefaultValue + { + [JsonIgnore] + public int Prop { get; set; } + + public SmallType_IgnoredProp_Bind_ParamWithDefaultValue(int prop = 5) + => Prop = prop; + } + + public class SmallType_IgnoredProp_Bind_Param + { + [JsonIgnore] + public int Prop { get; set; } + + public SmallType_IgnoredProp_Bind_Param(int prop) + => Prop = prop; + } + + [Fact] + public async Task LargeObject_ClrDefaultParamValueUsed_WhenMatchingPropIgnored() + { + string json = @"{""Prop"":20}"; + var obj1 = await JsonSerializerWrapperForString.DeserializeWrapper(json); + Assert.Equal(0, obj1.Prop); + + var obj2 = await JsonSerializerWrapperForString.DeserializeWrapper(json); + Assert.Equal(0, obj2.Prop); + } + + public class LargeType_IgnoredProp_Bind_ParamWithDefaultValue + { + public int W { get; set; } + + public int X { get; set; } + + public int Y { get; set; } + + public int Z { get; set; } + + [JsonIgnore] + public int Prop { get; set; } + + public LargeType_IgnoredProp_Bind_ParamWithDefaultValue(int w, int x, int y, int z, int prop = 5) + => Prop = prop; + } + + public class LargeType_IgnoredProp_Bind_Param + { + public int W { get; set; } + + public int X { get; set; } + + public int Y { get; set; } + + public int Z { get; set; } + + [JsonIgnore] + public int Prop { get; set; } + + public LargeType_IgnoredProp_Bind_Param(int w, int x, int y, int z, int prop) + => Prop = prop; + } + + [Fact] + [ActiveIssue("https://github.com/dotnet/runtime/issues/34779")] + public async Task BindingBetweenRefProps() + { + string json = @"{""NameRef"":""John""}"; + await JsonSerializerWrapperForString.DeserializeWrapper(json); + } + + public class TypeWith_RefStringProp_ParamCtor + { + private string _name; + + public ref string NameRef => ref _name; + + public TypeWith_RefStringProp_ParamCtor(ref string nameRef) => _name = nameRef; + } } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs index f0d1cb897ce2dd..d06d223c4a6d1e 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs @@ -125,6 +125,10 @@ protected ConstructorTests_Metadata(JsonSerializerWrapperForString stringWrapper [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(Parameterized_Class_With_ComplexTuple))] [JsonSerializable(typeof(Parameterized_Person_Simple))] + [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_ParamWithDefaultValue))] + [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_Param))] + [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] + [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_Param))] internal sealed partial class ConstructorTestsContext_Metadata : JsonSerializerContext { } @@ -241,6 +245,10 @@ public ConstructorTests_Default() [JsonSerializable(typeof(JsonElement))] [JsonSerializable(typeof(Parameterized_Class_With_ComplexTuple))] [JsonSerializable(typeof(Parameterized_Person_Simple))] + [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_ParamWithDefaultValue))] + [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_Param))] + [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] + [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_Param))] internal sealed partial class ConstructorTestsContext_Default : JsonSerializerContext { } From 830b179748a278cf79762fd7a7b5b73319375584 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Mon, 11 Oct 2021 13:23:30 -0700 Subject: [PATCH 4/4] Remove new concurrent dictionary for generic method holders --- .../Serialization/JsonSerializerOptions.cs | 18 +++++++++-- .../Metadata/GenericMethodHolder.cs | 31 +++---------------- .../Metadata/JsonParameterInfo.cs | 13 +++++++- .../Serialization/Metadata/JsonTypeInfo.cs | 2 +- .../ConstructorTests.ParameterMatching.cs | 15 +++++++++ .../Serialization/ConstructorTests.cs | 2 ++ 6 files changed, 49 insertions(+), 32 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 2d23e7c5c9200b..f81025bbc460e5 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 @@ -600,9 +600,7 @@ internal JsonTypeInfo GetOrAddClass(Type type) { _haveTypesBeenCreated = true; - // todo: for performance and reduced instances, consider using the converters and JsonTypeInfo from s_defaultOptions by cloning (or reference directly if no changes). - // https://github.com/dotnet/runtime/issues/32357 - if (!_classes.TryGetValue(type, out JsonTypeInfo? result)) + if (!TryGetClass(type, out JsonTypeInfo? result)) { result = _classes.GetOrAdd(type, GetClassFromContextOrCreate(type)); } @@ -644,6 +642,20 @@ internal JsonTypeInfo GetOrAddClassForRootType(Type type) return jsonTypeInfo; } + internal bool TryGetClass(Type type, [NotNullWhen(true)] out JsonTypeInfo? jsonTypeInfo) + { + // todo: for performance and reduced instances, consider using the converters and JsonTypeInfo from s_defaultOptions by cloning (or reference directly if no changes). + // https://github.com/dotnet/runtime/issues/32357 + if (!_classes.TryGetValue(type, out JsonTypeInfo? result)) + { + jsonTypeInfo = null; + return false; + } + + jsonTypeInfo = result; + return true; + } + internal bool TypeIsCached(Type type) { return _classes.ContainsKey(type); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs index 24957202b5a2d3..2c143f95e59cc9 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/GenericMethodHolder.cs @@ -13,8 +13,6 @@ namespace System.Text.Json.Serialization.Metadata /// internal abstract class GenericMethodHolder { - private static ConcurrentDictionary? s_holders; - /// /// Returns the default value for the specified type. /// @@ -26,33 +24,12 @@ internal abstract class GenericMethodHolder public abstract bool IsDefaultValue(object value); /// - /// Returns a holder instance representing a type. + /// Creates a holder instance representing a type. /// - public static GenericMethodHolder GetHolder(Type type) + public static GenericMethodHolder CreateHolder(Type type) { - if (s_holders == null) - { - s_holders = new ConcurrentDictionary(); - return CreateAndCacheHolder(); - } - else - { - if (!s_holders.TryGetValue(type, out GenericMethodHolder? holder)) - { - holder = CreateAndCacheHolder(); - } - - return holder; - } - - GenericMethodHolder CreateAndCacheHolder() - { - Type holderType = typeof(GenericMethodHolder<>).MakeGenericType(type); - GenericMethodHolder holderInstance = (GenericMethodHolder)Activator.CreateInstance(holderType)!; - Debug.Assert(s_holders != null); - s_holders[type] = holderInstance; - return holderInstance; - } + Type holderType = typeof(GenericMethodHolder<>).MakeGenericType(type); + return (GenericMethodHolder)Activator.CreateInstance(holderType)!; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs index 6f62cf48f8496b..8aee3b75f34415 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs @@ -100,7 +100,18 @@ public static JsonParameterInfo CreateIgnoredParameterPlaceholder( { // The value in the created JsonPropertyInfo instance (sbyte) // doesn't match the parameter type, use reflection to get the default value. - GenericMethodHolder holder = GenericMethodHolder.GetHolder(parameterInfo.ParameterType); + Type parameterType = parameterInfo.ParameterType; + + GenericMethodHolder holder; + if (matchingProperty.Options.TryGetClass(parameterType, out JsonTypeInfo? typeInfo)) + { + holder = typeInfo.GenericMethods; + } + else + { + holder = GenericMethodHolder.CreateHolder(parameterInfo.ParameterType); + } + jsonParameterInfo.DefaultValue = holder.DefaultValue; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index 821c7fd59761fd..b7e11501e60459 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -130,7 +130,7 @@ internal GenericMethodHolder GenericMethods { get { - _genericMethods ??= GenericMethodHolder.GetHolder(Type); + _genericMethods ??= GenericMethodHolder.CreateHolder(Type); return _genericMethods; } } diff --git a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs index e264ee95afd7c2..93aff74c1b4d0b 100644 --- a/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs +++ b/src/libraries/System.Text.Json/tests/Common/ConstructorTests/ConstructorTests.ParameterMatching.cs @@ -1307,5 +1307,20 @@ public class TypeWith_RefStringProp_ParamCtor public TypeWith_RefStringProp_ParamCtor(ref string nameRef) => _name = nameRef; } + + [Fact] + public async Task BindToIgnoredPropOfSameType() + { + string json = @"{""Prop"":{}}"; + Assert.NotNull(await JsonSerializerWrapperForString.DeserializeWrapper(json)); + } + + public class ClassWithIgnoredSameType + { + [JsonIgnore] + public ClassWithIgnoredSameType Prop { get; } + + public ClassWithIgnoredSameType(ClassWithIgnoredSameType prop) { } + } } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs index d06d223c4a6d1e..deba91529ef37d 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/ConstructorTests.cs @@ -129,6 +129,7 @@ protected ConstructorTests_Metadata(JsonSerializerWrapperForString stringWrapper [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_Param))] [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_Param))] + [JsonSerializable(typeof(ClassWithIgnoredSameType))] internal sealed partial class ConstructorTestsContext_Metadata : JsonSerializerContext { } @@ -249,6 +250,7 @@ public ConstructorTests_Default() [JsonSerializable(typeof(SmallType_IgnoredProp_Bind_Param))] [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_ParamWithDefaultValue))] [JsonSerializable(typeof(LargeType_IgnoredProp_Bind_Param))] + [JsonSerializable(typeof(ClassWithIgnoredSameType))] internal sealed partial class ConstructorTestsContext_Default : JsonSerializerContext { }