From 4ae4e2fe08164168a77f0a3b06091db5959fb506 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Thu, 28 May 2020 03:06:38 -0400 Subject: Loosen property name collision detection involving hidden properties (#36936) (#37105) * Loosen property name collision detection involving hidden properties * Delay ignored prop cache creation; add more tests * Clarify comments --- .../Text/Json/Serialization/JsonClassInfo.cs | 61 ++++++----- .../tests/Serialization/PropertyVisibilityTests.cs | 112 +++++++++++++++++++++ 2 files changed, 146 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index f0adf741298..7c24c672dca 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -101,6 +101,9 @@ namespace System.Text.Json ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); + HashSet? ignoredProperties = null; + + // We start from the most derived type and ascend to the base type. for (Type? currentType = type; currentType != null; currentType = currentType.BaseType) { foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly)) @@ -111,44 +114,55 @@ namespace System.Text.Json continue; } - if (IsNonPublicProperty(propertyInfo)) - { - if (JsonPropertyInfo.GetAttribute(propertyInfo) != null) - { - ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType); - } - - // Non-public properties should not be included for (de)serialization. - continue; - } - - // For now we only support public getters\setters + // For now we only support public properties (i.e. setter and/or getter is public). if (propertyInfo.GetMethod?.IsPublic == true || propertyInfo.SetMethod?.IsPublic == true) { JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo, currentType, options); Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null); - // If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception. + string propertyName = propertyInfo.Name; + + // The JsonPropertyNameAttribute or naming policy resulted in a collision. if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo)) { JsonPropertyInfo other = cache[jsonPropertyInfo.NameAsString]; - if (other.ShouldDeserialize == false && other.ShouldSerialize == false) + if (other.IsIgnored) { - // Overwrite the one just added since it has [JsonIgnore]. + // Overwrite previously cached property since it has [JsonIgnore]. cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo; } - else if (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name && - (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true)) + else if ( + // Does the current property have `JsonIgnoreAttribute`? + !jsonPropertyInfo.IsIgnored && + // Is the current property hidden by the previously cached property + // (with `new` keyword, or by overriding)? + other.PropertyInfo!.Name != propertyName && + // Was a property with the same CLR name was ignored? That property hid the current property, + // thus, if it was ignored, the current property should be ignored too. + ignoredProperties?.Contains(propertyName) != true) { - // Check for name equality is required to determine when a new slot is used for the member. - // Therefore, if names are not the same, there is conflict due to the name policy or attributes. + // We throw if we have two public properties that have the same JSON property name, and neither have been ignored. ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo); } - // else ignore jsonPropertyInfo since it has [JsonIgnore] or it's hidden by a new slot. + // Ignore the current property. + } + + if (jsonPropertyInfo.IsIgnored) + { + (ignoredProperties ??= new HashSet()).Add(propertyName); } } + else + { + if (JsonPropertyInfo.GetAttribute(propertyInfo) != null) + { + ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType); + } + + // Non-public properties should not be included for (de)serialization. + } } } @@ -211,13 +225,6 @@ namespace System.Text.Json } } - private static bool IsNonPublicProperty(PropertyInfo propertyInfo) - { - MethodInfo? getMethod = propertyInfo.GetMethod; - MethodInfo? setMethod = propertyInfo.SetMethod; - return !((getMethod != null && getMethod.IsPublic) || (setMethod != null && setMethod.IsPublic)); - } - private void InitializeConstructorParameters(Dictionary propertyCache, ConstructorInfo constructorInfo) { ParameterInfo[] parameters = constructorInfo!.GetParameters(); diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 2cc311764cb..69ce86512cc 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -327,6 +327,39 @@ namespace System.Text.Json.Serialization.Tests () => JsonSerializer.Deserialize(json, options)); } + [Fact] + public static void HiddenPropertiesIgnored_WhenOverridesIgnored_AndPropertyNameConflicts() + { + string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName()); + Assert.Equal(@"{""MyProp"":null}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty_And_ConflictingPropertyName()); + Assert.Equal(@"{""MyProp"":null}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName()); + Assert.Equal(@"{""MyProp"":null}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_WithIgnoredOverride()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_ConflictingPropertyName()); + Assert.Equal(@"{""MyProp"":null}", serialized); + + Assert.Throws(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName())); + + serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_IgnoredOverride()); + Assert.Equal(@"{""MyProp"":null}", serialized); + } + public class ClassWithInternalProperty { internal string MyString { get; set; } = "DefaultValue"; @@ -474,6 +507,85 @@ namespace System.Text.Json.Serialization.Tests public new decimal MyNumeric { get; set; } = 1.5M; } + private class Class_With_VirtualProperty + { + public virtual bool MyProp { get; set; } + } + + private class DerivedClass_With_IgnoredOverride : Class_With_VirtualProperty + { + [JsonIgnore] + public override bool MyProp { get; set; } + } + + private class DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName : Class_With_VirtualProperty + { + [JsonPropertyName("MyProp")] + public string MyString { get; set; } + + [JsonIgnore] + public override bool MyProp { get; set; } + } + + private class Class_With_Property + { + public bool MyProp { get; set; } + } + + private class DerivedClass_With_NewProperty : Class_With_Property + { + [JsonIgnore] + public new bool MyProp { get; set; } + } + + private class DerivedClass_With_NewProperty_And_ConflictingPropertyName : Class_With_Property + { + [JsonPropertyName("MyProp")] + public string MyString { get; set; } + + [JsonIgnore] + public new bool MyProp { get; set; } + } + + private class DerivedClass_WithNewProperty_Of_DifferentType : Class_With_Property + { + [JsonIgnore] + public new int MyProp { get; set; } + } + + private class DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName : Class_With_Property + { + [JsonPropertyName("MyProp")] + public string MyString { get; set; } + + [JsonIgnore] + public new int MyProp { get; set; } + } + + private class DerivedClass_WithIgnoredOverride : Class_With_VirtualProperty + { + [JsonIgnore] + public override bool MyProp { get; set; } + } + + private class FurtherDerivedClass_With_ConflictingPropertyName : DerivedClass_WithIgnoredOverride + { + [JsonPropertyName("MyProp")] + public string MyString { get; set; } + } + + private class DerivedClass_WithConflictingPropertyName : Class_With_VirtualProperty + { + [JsonPropertyName("MyProp")] + public string MyString { get; set; } + } + + private class FurtherDerivedClass_With_IgnoredOverride : DerivedClass_WithConflictingPropertyName + { + [JsonIgnore] + public override bool MyProp { get; set; } + } + [Fact] public static void NoSetter() { -- cgit v1.2.3