From 765e22cb90209dceb49559d4d0953b7edff1c9bd Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Wed, 10 Jun 2020 12:29:33 -0700 Subject: [PATCH 1/4] Ignore properties that were hidden in a more derived type --- .../Text/Json/Serialization/JsonClassInfo.cs | 17 +++++++++-------- .../Serialization/PropertyVisibilityTests.cs | 11 ++++------- 2 files changed, 13 insertions(+), 15 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 5e4e2845e3bfb7..c0b8afb6c60e39 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 @@ -108,8 +108,14 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) { foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly)) { - // Ignore indexers - if (propertyInfo.GetIndexParameters().Length > 0) + string propertyName = propertyInfo.Name; + + if ( + // Ignore indexers. + propertyInfo.GetIndexParameters().Length > 0 || + // Was a property with the same CLR name ignored? That property hid the current property, + // thus, if it was ignored, the current property should be ignored too. + ignoredProperties?.Contains(propertyName) == true) { continue; } @@ -121,8 +127,6 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo, currentType, options); Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null); - string propertyName = propertyInfo.Name; - // The JsonPropertyNameAttribute or naming policy resulted in a collision. if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo)) { @@ -138,10 +142,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) !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) + other.PropertyInfo!.Name != propertyName) { // 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); diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index f3a20ca5d9b187..93fe16b02731b8 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -328,29 +328,26 @@ public static void Throw_PublicProperty_ConflictDuePolicy_DobuleInheritance() } [Fact] - public static void HiddenPropertiesIgnored_WhenOverridesIgnored_AndPropertyNameConflicts() + public static void HiddenPropertiesIgnored_WhenOverridesIgnored() { string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride()); - Assert.Equal(@"{""MyProp"":false}", serialized); + Assert.Equal(@"{}", 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); + Assert.Equal(@"{}", 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); + Assert.Equal(@"{}", 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); From 061e2a86df4f7bdcfe9a1662810b9a505ad7e9f1 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Wed, 10 Jun 2020 17:26:41 -0700 Subject: [PATCH 2/4] Update EnumConverterTests comment --- .../tests/Serialization/EnumConverterTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs b/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs index 5e198d6bb7e303..da25f2aab962a3 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs @@ -261,8 +261,8 @@ public static void VeryLargeAmountOfEnumsToSerialize() // Use multiple threads to perhaps go over the soft limit of 64, but not by more than a couple. Parallel.For(0, 8, i => JsonSerializer.Serialize((MyEnum)(46 + i), options)); - // Write the remaining enum values. We should not store any more values in - // the cache. If we do, we may throw OutOfMemoryException on some machines. + // Write the remaining enum values. The cache is capped to avoid + // OutOfMemoryException due to having too many cached items. for (int i = 54; i <= MaxValue; i++) { JsonSerializer.Serialize((MyEnum)i, options); From 766572fd4cc8fc2bae7d1cde857aafe43473c71b Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Fri, 12 Jun 2020 16:24:58 -0700 Subject: [PATCH 3/4] Add more tests and align with Newtonsoft.Json behavior --- .../Text/Json/Serialization/JsonClassInfo.cs | 45 +++++-- .../Serialization/PropertyVisibilityTests.cs | 120 ++++++++++++++++-- 2 files changed, 142 insertions(+), 23 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 c0b8afb6c60e39..c987abdb223d98 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 @@ -4,6 +4,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Text.Json.Serialization; @@ -101,21 +102,15 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); - HashSet? ignoredProperties = null; + Dictionary? ignoredProperties = null; - // We start from the most derived type and ascend to the base type. + // We start from the most derived type. for (Type? currentType = type; currentType != null; currentType = currentType.BaseType) { foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly)) { - string propertyName = propertyInfo.Name; - - if ( - // Ignore indexers. - propertyInfo.GetIndexParameters().Length > 0 || - // Was a property with the same CLR name ignored? That property hid the current property, - // thus, if it was ignored, the current property should be ignored too. - ignoredProperties?.Contains(propertyName) == true) + // Ignore indexers and virtual properties that have overrides that were [JsonIgnore]d. + if (propertyInfo.GetIndexParameters().Length > 0 || PropertyIsOverridenAndIgnored(propertyInfo, ignoredProperties)) { continue; } @@ -127,6 +122,8 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo, currentType, options); Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null); + string propertyName = propertyInfo.Name; + // The JsonPropertyNameAttribute or naming policy resulted in a collision. if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo)) { @@ -142,9 +139,14 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) !jsonPropertyInfo.IsIgnored && // Is the current property hidden by the previously cached property // (with `new` keyword, or by overriding)? - other.PropertyInfo!.Name != propertyName) + other.PropertyInfo!.Name != propertyName && + // Was a property with the same CLR name ignored? That property hid the current property, + // thus, if it was ignored, the current property should be ignored too. + ignoredProperties?.ContainsKey(propertyName) != true + ) { - // We throw if we have two public properties that have the same JSON property name, and neither have been ignored. + // Throw if we have two public properties with the same JSON property name, + // neither overrides or hides the other, and neither have been ignored. ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo); } // Ignore the current property. @@ -152,7 +154,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) if (jsonPropertyInfo.IsIgnored) { - (ignoredProperties ??= new HashSet()).Add(propertyName); + (ignoredProperties ??= new Dictionary())[propertyName] = propertyInfo; } } else @@ -290,6 +292,23 @@ private void InitializeConstructorParameters(Dictionary? ignoredProperties) + { + if (ignoredProperties == null || !ignoredProperties.TryGetValue(currentProperty.Name, out PropertyInfo? ignoredProperty)) + { + return false; + } + + return currentProperty.PropertyType == ignoredProperty.PropertyType && + PropertyIsVirtual(currentProperty) && + PropertyIsVirtual(ignoredProperty); + } + + private static bool PropertyIsVirtual(PropertyInfo propertyInfo) + { + return propertyInfo.GetMethod?.IsVirtual == true || propertyInfo.SetMethod?.IsVirtual == true; + } + public bool DetermineExtensionDataProperty(Dictionary cache) { JsonPropertyInfo? jsonPropertyInfo = GetPropertyWithUniqueAttribute(Type, typeof(JsonExtensionDataAttribute), cache); diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 93fe16b02731b8..c2fd2148b32840 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -50,12 +50,18 @@ public static void Serialize_PublicProperty_ConflictWithPrivateDueAttributes() { // Serialize var obj = new ClassWithPropertyNamingConflict(); + + // Newtonsoft.Json throws JsonSerializationException here because + // non-public properties are included when [JsonProperty] is placed on them. string json = JsonSerializer.Serialize(obj); Assert.Equal(@"{""MyString"":""DefaultValue""}", json); // Deserialize json = @"{""MyString"":""NewValue""}"; + + // Newtonsoft.Json throws JsonSerializationException here because + // non-public properties are included when [JsonProperty] is placed on them. obj = JsonSerializer.Deserialize(json); Assert.Equal("NewValue", obj.MyString); @@ -82,7 +88,7 @@ public static void Serialize_PublicProperty_ConflictWithPrivateDuePolicy() } [Fact] - public static void Serealize_NewSlotPublicProperty_ConflictWithBasePublicProperty() + public static void Serialize_NewSlotPublicProperty_ConflictWithBasePublicProperty() { // Serialize var obj = new ClassWithNewSlotDecimalProperty(); @@ -98,7 +104,7 @@ public static void Serealize_NewSlotPublicProperty_ConflictWithBasePublicPropert } [Fact] - public static void Serealize_NewSlotPublicProperty_SpecifiedJsonPropertyName() + public static void Serialize_NewSlotPublicProperty_SpecifiedJsonPropertyName() { // Serialize var obj = new ClassWithNewSlotAttributedDecimalProperty(); @@ -174,12 +180,19 @@ public static void Ignore_PublicProperty_ConflictWithPrivateDueAttributes() Assert.Equal(@"{}", json); + // Newtonsoft.Json has the following output because non-public properties are included when [JsonProperty] is placed on them. + // {"MyString":"ConflictingValue"} + // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); Assert.Equal("DefaultValue", obj.MyString); Assert.Equal("ConflictingValue", obj.ConflictingString); + + // The output for Newtonsoft.Json is: + // obj.ConflictingString = "NewValue" + // obj.MyString still equals "DefaultValue" } [Fact] @@ -259,10 +272,19 @@ public static void Throw_PublicProperty_ConflictDueAttributes_SingleInheritance( Assert.Throws( () => JsonSerializer.Serialize(obj)); + // The output for Newtonsoft.Json is: + // {"MyString":"ConflictingValue"} + // Conflicts at different type-hierarchy levels that are not caused by + // deriving or the new keyword are allowed. Properties on more derived types win. + // Deserialize string json = @"{""MyString"":""NewValue""}"; Assert.Throws( () => JsonSerializer.Deserialize(json)); + + // The output for Newtonsoft.Json is: + // obj.ConflictingString = "NewValue" + // obj.MyString still equals "DefaultValue" } [Fact] @@ -273,10 +295,20 @@ public static void Throw_PublicProperty_ConflictDueAttributes_DoubleInheritance( Assert.Throws( () => JsonSerializer.Serialize(obj)); + // The output for Newtonsoft.Json is: + // {"MyString":"ConflictingValue"} + // Conflicts at different type-hierarchy levels that are not caused by + // deriving or the new keyword are allowed. Properties on more derived types win. + // Deserialize string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( () => JsonSerializer.Deserialize(json)); + + // The output for Newtonsoft.Json is: + // obj.ConflictingString = "NewValue" + // obj.MyString still equals "DefaultValue" } [Fact] @@ -302,13 +334,23 @@ public static void Throw_PublicProperty_ConflictDuePolicy_SingleInheritance() // Serialize var obj = new ClassInheritedWithPropertyPolicyConflictWhichThrows(); + Assert.Throws( () => JsonSerializer.Serialize(obj, options)); + // The output for Newtonsoft.Json is: + // {"myString":"ConflictingValue"} + // Conflicts at different type-hierarchy levels that are not caused by + // deriving or the new keyword are allowed. Properties on more derived types win. + // Deserialize string json = @"{""MyString"":""NewValue""}"; Assert.Throws( () => JsonSerializer.Deserialize(json, options)); + + // The output for Newtonsoft.Json is: + // obj.myString = "NewValue" + // obj.MyString still equals "DefaultValue" } [Fact] @@ -318,13 +360,24 @@ public static void Throw_PublicProperty_ConflictDuePolicy_DobuleInheritance() // Serialize var obj = new ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows(); + Assert.Throws( () => JsonSerializer.Serialize(obj, options)); + // The output for Newtonsoft.Json is: + // {"myString":"ConflictingValue"} + // Conflicts at different type-hierarchy levels that are not caused by + // deriving or the new keyword are allowed. Properties on more derived types win. + // Deserialize string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( () => JsonSerializer.Deserialize(json, options)); + + // The output for Newtonsoft.Json is: + // obj.myString = "NewValue" + // obj.MyString still equals "DefaultValue" } [Fact] @@ -333,24 +386,44 @@ public static void HiddenPropertiesIgnored_WhenOverridesIgnored() string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride()); Assert.Equal(@"{}", serialized); + serialized = JsonSerializer.Serialize(new DerivedClass_WithVisibleProperty_Of_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(@"{}", serialized); + serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_WithConflictingNewMember()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_WithConflictingNewMember_Of_DifferentType()); + Assert.Equal(@"{""MyProp"":0}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_ConflictingNewMember()); + Assert.Equal(@"{""MyProp"":false}", serialized); + + serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_ConflictingNewMember_Of_DifferentType()); + 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(@"{}", serialized); + serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty_Of_DifferentType()); + Assert.Equal(@"{""MyProp"":false}", serialized); - serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName()); + serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty_Of_DifferentType_And_ConflictingPropertyName()); Assert.Equal(@"{""MyProp"":null}", serialized); serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_ConflictingPropertyName()); Assert.Equal(@"{""MyProp"":null}", serialized); + // Here we differ from Newtonsoft.Json, where the output would be + // {"MyProp":null} + // Conflicts at different type-hierarchy levels that are not caused by + // deriving or the new keyword are allowed. Properties on more derived types win. + // This is invalid in System.Text.Json. Assert.Throws(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName())); serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_IgnoredOverride()); @@ -515,6 +588,11 @@ private class DerivedClass_With_IgnoredOverride : Class_With_VirtualProperty public override bool MyProp { get; set; } } + private class DerivedClass_WithVisibleProperty_Of_DerivedClass_With_IgnoredOverride : DerivedClass_With_IgnoredOverride + { + public override bool MyProp { get; set; } + } + private class DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName : Class_With_VirtualProperty { [JsonPropertyName("MyProp")] @@ -529,7 +607,7 @@ private class Class_With_Property public bool MyProp { get; set; } } - private class DerivedClass_With_NewProperty : Class_With_Property + private class DerivedClass_With_Ignored_NewProperty : Class_With_Property { [JsonIgnore] public new bool MyProp { get; set; } @@ -544,13 +622,13 @@ private class DerivedClass_With_NewProperty_And_ConflictingPropertyName : Class_ public new bool MyProp { get; set; } } - private class DerivedClass_WithNewProperty_Of_DifferentType : Class_With_Property + private class DerivedClass_With_Ignored_NewProperty_Of_DifferentType : Class_With_Property { [JsonIgnore] public new int MyProp { get; set; } } - private class DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName : Class_With_Property + private class DerivedClass_With_Ignored_NewProperty_Of_DifferentType_And_ConflictingPropertyName : Class_With_Property { [JsonPropertyName("MyProp")] public string MyString { get; set; } @@ -565,6 +643,28 @@ private class DerivedClass_WithIgnoredOverride : Class_With_VirtualProperty public override bool MyProp { get; set; } } + private class DerivedClass_WithConflictingNewMember : Class_With_VirtualProperty + { + public new bool MyProp { get; set; } + } + + private class DerivedClass_WithConflictingNewMember_Of_DifferentType : Class_With_VirtualProperty + { + public new int MyProp { get; set; } + } + + private class DerivedClass_With_Ignored_ConflictingNewMember : Class_With_VirtualProperty + { + [JsonIgnore] + public new bool MyProp { get; set; } + } + + private class DerivedClass_With_Ignored_ConflictingNewMember_Of_DifferentType : Class_With_VirtualProperty + { + [JsonIgnore] + public new int MyProp { get; set; } + } + private class FurtherDerivedClass_With_ConflictingPropertyName : DerivedClass_WithIgnoredOverride { [JsonPropertyName("MyProp")] From e77b2e57e40a1b526d46f8794f6643d9c437c929 Mon Sep 17 00:00:00 2001 From: Layomi Akinrinade Date: Tue, 16 Jun 2020 12:47:38 -0700 Subject: [PATCH 4/4] Clean up using statement --- .../src/System/Text/Json/Serialization/JsonClassInfo.cs | 1 - 1 file changed, 1 deletion(-) 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 c987abdb223d98..be01634997a075 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 @@ -4,7 +4,6 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Text.Json.Serialization;