Address edge scenarios with JsonSerializer's property visibility (#37720)

* Ignore properties that were hidden in a more derived type

* Update EnumConverterTests comment

* Add more tests and align with Newtonsoft.Json behavior

* Clean up using statement

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 631d47d..c414353 100644 (file)
--- 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,15 +101,15 @@ namespace System.Text.Json
                                 ? StringComparer.OrdinalIgnoreCase
                                 : StringComparer.Ordinal);
 
-                        HashSet<string>? ignoredProperties = null;
+                        Dictionary<string, PropertyInfo>? 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))
                             {
-                                // Ignore indexers
-                                if (propertyInfo.GetIndexParameters().Length > 0)
+                                // Ignore indexers and virtual properties that have overrides that were [JsonIgnore]d.
+                                if (propertyInfo.GetIndexParameters().Length > 0 || PropertyIsOverridenAndIgnored(propertyInfo, ignoredProperties))
                                 {
                                     continue;
                                 }
@@ -139,11 +139,13 @@ namespace System.Text.Json
                                             // 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,
+                                            // 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)
+                                            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.
@@ -151,7 +153,7 @@ namespace System.Text.Json
 
                                     if (jsonPropertyInfo.IsIgnored)
                                     {
-                                        (ignoredProperties ??= new HashSet<string>()).Add(propertyName);
+                                        (ignoredProperties ??= new Dictionary<string, PropertyInfo>())[propertyName] = propertyInfo;
                                     }
                                 }
                                 else
@@ -289,6 +291,23 @@ namespace System.Text.Json
             PropertyCache = propertyCache;
         }
 
+        private static bool PropertyIsOverridenAndIgnored(PropertyInfo currentProperty, Dictionary<string, PropertyInfo>? 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<string, JsonPropertyInfo> cache)
         {
             JsonPropertyInfo? jsonPropertyInfo = GetPropertyWithUniqueAttribute(Type, typeof(JsonExtensionDataAttribute), cache);

diff --git a/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs b/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs
index 5e198d6..da25f2a 100644 (file)
--- a/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs
+++ b/src/libraries/System.Text.Json/tests/Serialization/EnumConverterTests.cs
@@ -261,8 +261,8 @@ namespace System.Text.Json.Serialization.Tests
             // 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);

diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
index f3a20ca..c2fd214 100644 (file)
--- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
+++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
@@ -50,12 +50,18 @@ namespace System.Text.Json.Serialization.Tests
         {
             // 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<ClassWithPropertyNamingConflict>(json);
 
             Assert.Equal("NewValue", obj.MyString);
@@ -82,7 +88,7 @@ namespace System.Text.Json.Serialization.Tests
         }
 
         [Fact]
-        public static void Serealize_NewSlotPublicProperty_ConflictWithBasePublicProperty()
+        public static void Serialize_NewSlotPublicProperty_ConflictWithBasePublicProperty()
         {
             // Serialize
             var obj = new ClassWithNewSlotDecimalProperty();
@@ -98,7 +104,7 @@ namespace System.Text.Json.Serialization.Tests
         }
 
         [Fact]
-        public static void Serealize_NewSlotPublicProperty_SpecifiedJsonPropertyName()
+        public static void Serialize_NewSlotPublicProperty_SpecifiedJsonPropertyName()
         {
             // Serialize
             var obj = new ClassWithNewSlotAttributedDecimalProperty();
@@ -174,12 +180,19 @@ namespace System.Text.Json.Serialization.Tests
 
             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<ClassWithIgnoredPropertyNamingConflictPrivate>(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 @@ namespace System.Text.Json.Serialization.Tests
             Assert.Throws<InvalidOperationException>(
                 () => 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<InvalidOperationException>(
                 () => JsonSerializer.Deserialize<ClassInheritedWithPropertyNamingConflictWhichThrows>(json));
+
+            // The output for Newtonsoft.Json is:
+            // obj.ConflictingString = "NewValue"
+            // obj.MyString still equals "DefaultValue"
         }
 
         [Fact]
@@ -273,10 +295,20 @@ namespace System.Text.Json.Serialization.Tests
             Assert.Throws<InvalidOperationException>(
                 () => 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<InvalidOperationException>(
                 () => JsonSerializer.Deserialize<ClassTwiceInheritedWithPropertyNamingConflictWhichThrows>(json));
+
+            // The output for Newtonsoft.Json is:
+            // obj.ConflictingString = "NewValue"
+            // obj.MyString still equals "DefaultValue"
         }
 
         [Fact]
@@ -302,13 +334,23 @@ namespace System.Text.Json.Serialization.Tests
 
             // Serialize
             var obj = new ClassInheritedWithPropertyPolicyConflictWhichThrows();
+
             Assert.Throws<InvalidOperationException>(
                 () => 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<InvalidOperationException>(
                 () => JsonSerializer.Deserialize<ClassInheritedWithPropertyPolicyConflictWhichThrows>(json, options));
+
+            // The output for Newtonsoft.Json is:
+            // obj.myString = "NewValue"
+            // obj.MyString still equals "DefaultValue"
         }
 
         [Fact]
@@ -318,42 +360,70 @@ namespace System.Text.Json.Serialization.Tests
 
             // Serialize
             var obj = new ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows();
+
             Assert.Throws<InvalidOperationException>(
                 () => 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<InvalidOperationException>(
                 () => JsonSerializer.Deserialize<ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows>(json, options));
+
+            // The output for Newtonsoft.Json is:
+            // obj.myString = "NewValue"
+            // obj.MyString still equals "DefaultValue"
         }
 
         [Fact]
-        public static void HiddenPropertiesIgnored_WhenOverridesIgnored_AndPropertyNameConflicts()
+        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());
+            serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_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_WithConflictingNewMember());
+            Assert.Equal(@"{""MyProp"":false}", serialized);
+
+            serialized = JsonSerializer.Serialize(new DerivedClass_WithConflictingNewMember_Of_DifferentType());
+            Assert.Equal(@"{""MyProp"":0}", serialized);
 
-            serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType());
+            serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_ConflictingNewMember());
             Assert.Equal(@"{""MyProp"":false}", serialized);
 
-            serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName());
+            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_WithIgnoredOverride());
+            serialized = JsonSerializer.Serialize(new DerivedClass_With_Ignored_NewProperty_Of_DifferentType());
             Assert.Equal(@"{""MyProp"":false}", serialized);
 
+            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<InvalidOperationException>(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName()));
 
             serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_IgnoredOverride());
@@ -518,6 +588,11 @@ namespace System.Text.Json.Serialization.Tests
             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")]
@@ -532,7 +607,7 @@ namespace System.Text.Json.Serialization.Tests
             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; }
@@ -547,13 +622,13 @@ namespace System.Text.Json.Serialization.Tests
             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; }
@@ -568,6 +643,28 @@ namespace System.Text.Json.Serialization.Tests
             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")]