From 05639b9f86c948185e1a76d034b202c2545a7781 Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Tue, 11 Feb 2020 13:48:43 +0300 Subject: [PATCH 01/16] Fixed serialization of hidden base class members --- .../Text/Json/Serialization/JsonClassInfo.cs | 68 +++++++++---------- 1 file changed, 34 insertions(+), 34 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 12202e395b67ac..a70acc53675db9 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 @@ -97,52 +97,52 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) case ClassType.Object: { CreateObject = options.MemberAccessorStrategy.CreateConstructor(type); + Dictionary cache = CreatePropertyCache(); - PropertyInfo[] properties = type.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); - - Dictionary cache = CreatePropertyCache(properties.Length); - - foreach (PropertyInfo propertyInfo in properties) + for (Type? currentType = runtimeType; currentType != null; currentType = currentType.BaseType) { - // Ignore indexers - if (propertyInfo.GetIndexParameters().Length > 0) + foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.DeclaredOnly)) { - continue; - } - - if (IsNonPublicProperty(propertyInfo)) - { - if (JsonPropertyInfo.GetAttribute(propertyInfo) != null) + // Ignore indexers + if (propertyInfo.GetIndexParameters().Length > 0) { - ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, Type); + continue; } - // Non-public properties should not be included for (de)serialization. - continue; - } + if (IsNonPublicProperty(propertyInfo)) + { + if (JsonPropertyInfo.GetAttribute(propertyInfo) != null) + { + ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, Type); + } - // For now we only support public getters\setters - if (propertyInfo.GetMethod?.IsPublic == true || - propertyInfo.SetMethod?.IsPublic == true) - { - JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo, type, options); - Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null); + // Non-public properties should not be included for (de)serialization. + continue; + } - // If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception. - if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo)) + // For now we only support public getters\setters + if (propertyInfo.GetMethod?.IsPublic == true || + propertyInfo.SetMethod?.IsPublic == true) { - JsonPropertyInfo other = cache[jsonPropertyInfo.NameAsString]; + JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo.PropertyType, propertyInfo, type, options); + Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null); - if (other.ShouldDeserialize == false && other.ShouldSerialize == false) - { - // Overwrite the one just added since it has [JsonIgnore]. - cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo; - } - else if (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true) + // If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception. + if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo)) { - ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo); + JsonPropertyInfo other = cache[jsonPropertyInfo.NameAsString]; + + if (other.ShouldDeserialize == false && other.ShouldSerialize == false) + { + // Overwrite the one just added since it has [JsonIgnore]. + cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo; + } + else if (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true) + { + ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo); + } + // else ignore jsonPropertyInfo since it has [JsonIgnore]. } - // else ignore jsonPropertyInfo since it has [JsonIgnore]. } } } From a01bbe8adfe229fe974e92fb9662bb2aaf981221 Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Mon, 3 Feb 2020 17:43:53 +0300 Subject: [PATCH 02/16] Added tests to check non-public props are ignored --- .../Serialization/PropertyVisibilityTests.cs | 225 ++++++++++++++++++ 1 file changed, 225 insertions(+) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 9aa9cd6bb8ba7c..b61f8f19e94a69 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -11,6 +11,231 @@ namespace System.Text.Json.Serialization.Tests { public static partial class PropertyVisibilityTests { + [Fact] + public static void Serialize_new_slot_public_property() + { + // Serialize + var obj = new ClassWithNewSlotProperty(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{""MyString"":""NewDefaultValue""}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("NewValue", ((ClassWithNewSlotProperty)obj).MyString); + Assert.Equal("DefaultValue", ((ClassWithPrivateProperty)obj).MyString); + } + + [Fact] + public static void Serialize_base_public_property_on_conflict_with_derived_private() + { + // Serialize + var obj = new ClassWithNewSlotPrivateProperty(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{""MyString"":""DefaultValue""}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("NewValue", ((ClassWithPublicProperty)obj).MyString); + Assert.Equal("NewDefaultValue", ((ClassWithNewSlotPrivateProperty)obj).MyString); + } + + [Fact] + public static void Serialize_public_property_on_conflict_with_private_due_to_attributes() + { + // Serialize + var obj = new ClassWithPropertyNamingConflict(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{""MyString"":""DefaultValue""}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("NewValue", obj.MyString); + Assert.Equal("ConflictingValue", obj.ConflictingString); + } + + [Fact] + public static void Serialize_public_property_on_conflict_with_private_due_to_policy() + { + var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + + // Serialize + var obj = new ClassWithPropertyPolicyConflict(); + string json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(@"{""myString"":""DefaultValue""}", json); + + // Deserialzie + json = @"{""myString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal("NewValue", obj.MyString); + Assert.Equal("ConflictingValue", obj.myString); + } + + [Fact] + public static void Ignore_non_public_property() + { + // Serialize + var obj = new ClassWithPrivateProperty(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("DefaultValue", obj.MyString); + } + + [Fact] + public static void Ignore_ignored_new_slot_public_property() + { + // Serialize + var obj = new ClassWithIgnoredNewSlotProperty(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("NewDefaultValue", ((ClassWithIgnoredNewSlotProperty)obj).MyString); + Assert.Equal("DefaultValue", ((ClassWithPrivateProperty)obj).MyString); + } + + [Fact] + public static void Ignore_ignored_base_public_property_on_conflict_with_derived_private() + { + // Serialize + var obj = new ClassWithIgnoredPublicPropertyAndNewSlotPrivate(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("DefaultValue", ((ClassWithIgnoredPublicProperty)obj).MyString); + Assert.Equal("NewDefaultValue", ((ClassWithIgnoredPublicPropertyAndNewSlotPrivate)obj).MyString); + } + + [Fact] + public static void Ignore_public_property_on_conflict_with_private_due_to_attributes() + { + // Serialize + var obj = new ClassWithIgnoredPropertyNamingConflict(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{}", json); + + // Deserialzie + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("DefaultValue", obj.MyString); + Assert.Equal("ConflictingValue", obj.ConflictingString); + } + + [Fact] + public static void Ignore_public_property_on_conflict_with_private_due_to_policy() + { + var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + + // Serialize + var obj = new ClassWithIgnoredPropertyPolicyConflict(); + string json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(@"{}", json); + + // Deserialzie + json = @"{""myString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal("DefaultValue", obj.MyString); + Assert.Equal("ConflictingValue", obj.myString); + } + + public class ClassWithPrivateProperty + { + internal string MyString { get; set; } = "DefaultValue"; + } + + public class ClassWithNewSlotProperty : ClassWithPrivateProperty + { + public new string MyString { get; set; } = "NewDefaultValue"; + } + + public class ClassWithPublicProperty + { + public string MyString { get; set; } = "DefaultValue"; + } + + public class ClassWithNewSlotPrivateProperty : ClassWithPublicProperty + { + internal new string MyString { get; set; } = "NewDefaultValue"; + } + + public class ClassWithPropertyNamingConflict + { + public string MyString { get; set; } = "DefaultValue"; + + [JsonPropertyName(nameof(MyString))] + internal string ConflictingString { get; set; } = "ConflictingValue"; + } + + public class ClassWithPropertyPolicyConflict + { + public string MyString { get; set; } = "DefaultValue"; + + internal string myString { get; set; } = "ConflictingValue"; + } + + public class ClassWithIgnoredNewSlotProperty : ClassWithPrivateProperty + { + [JsonIgnore] + public new string MyString { get; set; } = "NewDefaultValue"; + } + + public class ClassWithIgnoredPublicProperty + { + [JsonIgnore] + public string MyString { get; set; } = "DefaultValue"; + } + + public class ClassWithIgnoredPublicPropertyAndNewSlotPrivate : ClassWithIgnoredPublicProperty + { + internal new string MyString { get; set; } = "NewDefaultValue"; + } + + public class ClassWithIgnoredPropertyNamingConflict + { + [JsonIgnore] + public string MyString { get; set; } = "DefaultValue"; + + [JsonPropertyName(nameof(MyString))] + internal string ConflictingString { get; set; } = "ConflictingValue"; + } + + public class ClassWithIgnoredPropertyPolicyConflict + { + [JsonIgnore] + public string MyString { get; set; } = "DefaultValue"; + + internal string myString { get; set; } = "ConflictingValue"; + } + [Fact] public static void NoSetter() { From 827df7e23fb574aa07e83c87d196fd37a335b45e Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Tue, 11 Feb 2020 14:16:49 +0300 Subject: [PATCH 03/16] Simplified cache construction --- .../src/System/Text/Json/Serialization/JsonClassInfo.cs | 5 ++++- 1 file changed, 4 insertions(+), 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 a70acc53675db9..f4c6368dc5b9aa 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 @@ -97,7 +97,10 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) case ClassType.Object: { CreateObject = options.MemberAccessorStrategy.CreateConstructor(type); - Dictionary cache = CreatePropertyCache(); + Dictionary cache = new Dictionary( + Options.PropertyNameCaseInsensitive + ? StringComparer.OrdinalIgnoreCase + : StringComparer.Ordinal); for (Type? currentType = runtimeType; currentType != null; currentType = currentType.BaseType) { From ed8ca2b34e51971d22eed70545444aff1f67bf37 Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Sun, 23 Feb 2020 12:30:07 +0300 Subject: [PATCH 04/16] Addressed review issues --- .../Text/Json/Serialization/JsonClassInfo.cs | 2 +- .../Serialization/PropertyVisibilityTests.cs | 42 +++++++++---------- 2 files changed, 22 insertions(+), 22 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 f4c6368dc5b9aa..1c34862229f60c 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 @@ -102,7 +102,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) ? StringComparer.OrdinalIgnoreCase : StringComparer.Ordinal); - for (Type? currentType = runtimeType; currentType != null; currentType = currentType.BaseType) + for (Type? currentType = type; currentType != null; currentType = currentType.BaseType) { foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.DeclaredOnly)) { diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index b61f8f19e94a69..9da764065384af 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -12,7 +12,7 @@ namespace System.Text.Json.Serialization.Tests public static partial class PropertyVisibilityTests { [Fact] - public static void Serialize_new_slot_public_property() + public static void SerializeNewSlotPublicProperty() { // Serialize var obj = new ClassWithNewSlotProperty(); @@ -20,29 +20,29 @@ public static void Serialize_new_slot_public_property() Assert.Equal(@"{""MyString"":""NewDefaultValue""}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); Assert.Equal("NewValue", ((ClassWithNewSlotProperty)obj).MyString); - Assert.Equal("DefaultValue", ((ClassWithPrivateProperty)obj).MyString); + Assert.Equal("DefaultValue", ((ClassWithInternalProperty)obj).MyString); } [Fact] public static void Serialize_base_public_property_on_conflict_with_derived_private() { // Serialize - var obj = new ClassWithNewSlotPrivateProperty(); + var obj = new ClassWithNewSlotInternalProperty(); string json = JsonSerializer.Serialize(obj); Assert.Equal(@"{""MyString"":""DefaultValue""}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; - obj = JsonSerializer.Deserialize(json); + obj = JsonSerializer.Deserialize(json); Assert.Equal("NewValue", ((ClassWithPublicProperty)obj).MyString); - Assert.Equal("NewDefaultValue", ((ClassWithNewSlotPrivateProperty)obj).MyString); + Assert.Equal("NewDefaultValue", ((ClassWithNewSlotInternalProperty)obj).MyString); } [Fact] @@ -54,7 +54,7 @@ public static void Serialize_public_property_on_conflict_with_private_due_to_att Assert.Equal(@"{""MyString"":""DefaultValue""}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); @@ -73,7 +73,7 @@ public static void Serialize_public_property_on_conflict_with_private_due_to_pol Assert.Equal(@"{""myString"":""DefaultValue""}", json); - // Deserialzie + // Deserialize json = @"{""myString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json, options); @@ -85,14 +85,14 @@ public static void Serialize_public_property_on_conflict_with_private_due_to_pol public static void Ignore_non_public_property() { // Serialize - var obj = new ClassWithPrivateProperty(); + var obj = new ClassWithInternalProperty(); string json = JsonSerializer.Serialize(obj); Assert.Equal(@"{}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; - obj = JsonSerializer.Deserialize(json); + obj = JsonSerializer.Deserialize(json); Assert.Equal("DefaultValue", obj.MyString); } @@ -106,12 +106,12 @@ public static void Ignore_ignored_new_slot_public_property() Assert.Equal(@"{}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); Assert.Equal("NewDefaultValue", ((ClassWithIgnoredNewSlotProperty)obj).MyString); - Assert.Equal("DefaultValue", ((ClassWithPrivateProperty)obj).MyString); + Assert.Equal("DefaultValue", ((ClassWithInternalProperty)obj).MyString); } [Fact] @@ -123,7 +123,7 @@ public static void Ignore_ignored_base_public_property_on_conflict_with_derived_ Assert.Equal(@"{}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); @@ -140,7 +140,7 @@ public static void Ignore_public_property_on_conflict_with_private_due_to_attrib Assert.Equal(@"{}", json); - // Deserialzie + // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); @@ -159,7 +159,7 @@ public static void Ignore_public_property_on_conflict_with_private_due_to_policy Assert.Equal(@"{}", json); - // Deserialzie + // Deserialize json = @"{""myString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json, options); @@ -167,12 +167,12 @@ public static void Ignore_public_property_on_conflict_with_private_due_to_policy Assert.Equal("ConflictingValue", obj.myString); } - public class ClassWithPrivateProperty + public class ClassWithInternalProperty { internal string MyString { get; set; } = "DefaultValue"; } - public class ClassWithNewSlotProperty : ClassWithPrivateProperty + public class ClassWithNewSlotProperty : ClassWithInternalProperty { public new string MyString { get; set; } = "NewDefaultValue"; } @@ -182,7 +182,7 @@ public class ClassWithPublicProperty public string MyString { get; set; } = "DefaultValue"; } - public class ClassWithNewSlotPrivateProperty : ClassWithPublicProperty + public class ClassWithNewSlotInternalProperty : ClassWithPublicProperty { internal new string MyString { get; set; } = "NewDefaultValue"; } @@ -202,7 +202,7 @@ public class ClassWithPropertyPolicyConflict internal string myString { get; set; } = "ConflictingValue"; } - public class ClassWithIgnoredNewSlotProperty : ClassWithPrivateProperty + public class ClassWithIgnoredNewSlotProperty : ClassWithInternalProperty { [JsonIgnore] public new string MyString { get; set; } = "NewDefaultValue"; From c3d7228d560f02c99e6da530373348ca692a9845 Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Sun, 23 Feb 2020 12:49:49 +0300 Subject: [PATCH 05/16] Added requested tests --- .../Serialization/PropertyVisibilityTests.cs | 130 ++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 9da764065384af..8447723dffe299 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -167,6 +167,90 @@ public static void Ignore_public_property_on_conflict_with_private_due_to_policy Assert.Equal("ConflictingValue", obj.myString); } + [Fact] + public static void Throw_when_public_properties_conflict_due_to_attributes() + { + // Serialize + var obj = new ClassWithPropertyNamingConflictWhichThrows(); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); + + // Deserialize + string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( + () => JsonSerializer.Deserialize(json)); + } + + [Fact] + public static void Throw_when_public_properties_conflict_due_to_attributes_and_inheritance() + { + // Serialize + var obj = new ClassInheritedWithPropertyNamingConflictWhichThrows(); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); + + // Deserialize + string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( + () => JsonSerializer.Deserialize(json)); + } + + [Fact] + public static void Throw_when_public_properties_conflict_due_to_attributes_and_double_inheritance() + { + // Serialize + var obj = new ClassTwiceInheritedWithPropertyNamingConflictWhichThrows(); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); + + // Deserialize + string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( + () => JsonSerializer.Deserialize(json)); + } + + [Fact] + public static void Throw_when_public_properties_conflict_due_to_policy() + { + // Serialize + var obj = new ClassWithPropertyPolicyConflictWhichThrows(); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); + + // Deserialize + string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( + () => JsonSerializer.Deserialize(json)); + } + + [Fact] + public static void Throw_when_public_properties_conflict_due_to_policy_and_inheritance() + { + // Serialize + var obj = new ClassInheritedWithPropertyPolicyConflictWhichThrows(); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); + + // Deserialize + string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( + () => JsonSerializer.Deserialize(json)); + } + + [Fact] + public static void Throw_when_public_properties_conflict_due_to_policy_and_double_inheritance() + { + // Serialize + var obj = new ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows(); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); + + // Deserialize + string json = @"{""MyString"":""NewValue""}"; + Assert.Throws( + () => JsonSerializer.Deserialize(json)); + } + public class ClassWithInternalProperty { internal string MyString { get; set; } = "DefaultValue"; @@ -195,6 +279,31 @@ public class ClassWithPropertyNamingConflict internal string ConflictingString { get; set; } = "ConflictingValue"; } + public class ClassWithPropertyNamingConflictWhichThrows + { + public string MyString { get; set; } = "DefaultValue"; + + [JsonPropertyName(nameof(MyString))] + public string ConflictingString { get; set; } = "ConflictingValue"; + } + + public class ClassInheritedWithPropertyNamingConflictWhichThrows : ClassWithPublicProperty + { + [JsonPropertyName(nameof(MyString))] + public string ConflictingString { get; set; } = "ConflictingValue"; + } + + public class ClassTwiceInheritedWithPropertyNamingConflictWhichThrowsDummy : ClassWithPublicProperty + { + + } + + public class ClassTwiceInheritedWithPropertyNamingConflictWhichThrows : ClassTwiceInheritedWithPropertyNamingConflictWhichThrowsDummy + { + [JsonPropertyName(nameof(MyString))] + public string ConflictingString { get; set; } = "ConflictingValue"; + } + public class ClassWithPropertyPolicyConflict { public string MyString { get; set; } = "DefaultValue"; @@ -202,6 +311,27 @@ public class ClassWithPropertyPolicyConflict internal string myString { get; set; } = "ConflictingValue"; } + public class ClassWithPropertyPolicyConflictWhichThrows + { + public string MyString { get; set; } = "DefaultValue"; + + public string myString { get; set; } = "ConflictingValue"; + } + + public class ClassInheritedWithPropertyPolicyConflictWhichThrows : ClassWithPublicProperty + { + public string myString { get; set; } = "ConflictingValue"; + } + + public class ClassInheritedWithPropertyPolicyConflictWhichThrowsDummy : ClassWithPublicProperty + { + } + + public class ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows : ClassInheritedWithPropertyPolicyConflictWhichThrowsDummy + { + public string myString { get; set; } = "ConflictingValue"; + } + public class ClassWithIgnoredNewSlotProperty : ClassWithInternalProperty { [JsonIgnore] From 8f9653b7dc9bba05a46e8a9e7a42a5ca44831801 Mon Sep 17 00:00:00 2001 From: Yoh Deadfall Date: Sun, 23 Feb 2020 16:21:14 +0300 Subject: [PATCH 06/16] Fixed tests --- .../Serialization/PropertyVisibilityTests.cs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 8447723dffe299..23a37890bb2dd1 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -212,43 +212,49 @@ public static void Throw_when_public_properties_conflict_due_to_attributes_and_d [Fact] public static void Throw_when_public_properties_conflict_due_to_policy() { + var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // Serialize var obj = new ClassWithPropertyPolicyConflictWhichThrows(); Assert.Throws( - () => JsonSerializer.Serialize(obj)); + () => JsonSerializer.Serialize(obj, options)); // Deserialize string json = @"{""MyString"":""NewValue""}"; Assert.Throws( - () => JsonSerializer.Deserialize(json)); + () => JsonSerializer.Deserialize(json, options)); } [Fact] public static void Throw_when_public_properties_conflict_due_to_policy_and_inheritance() { + var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // Serialize var obj = new ClassInheritedWithPropertyPolicyConflictWhichThrows(); Assert.Throws( - () => JsonSerializer.Serialize(obj)); + () => JsonSerializer.Serialize(obj, options)); // Deserialize string json = @"{""MyString"":""NewValue""}"; Assert.Throws( - () => JsonSerializer.Deserialize(json)); + () => JsonSerializer.Deserialize(json, options)); } [Fact] public static void Throw_when_public_properties_conflict_due_to_policy_and_double_inheritance() { + var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + // Serialize var obj = new ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows(); Assert.Throws( - () => JsonSerializer.Serialize(obj)); + () => JsonSerializer.Serialize(obj, options)); // Deserialize string json = @"{""MyString"":""NewValue""}"; Assert.Throws( - () => JsonSerializer.Deserialize(json)); + () => JsonSerializer.Deserialize(json, options)); } public class ClassWithInternalProperty From 335c9d0b04fe0e9c8fea88cee1afa5a5cd1fd0e4 Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Tue, 7 Apr 2020 18:37:29 +0300 Subject: [PATCH 07/16] Fixed handling of public hidden prop by new slot --- .../Text/Json/Serialization/JsonClassInfo.cs | 7 ++--- .../Serialization/PropertyVisibilityTests.cs | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 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 1c34862229f60c..8b83519949a0f8 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 @@ -127,7 +127,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) if (propertyInfo.GetMethod?.IsPublic == true || propertyInfo.SetMethod?.IsPublic == true) { - JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo.PropertyType, propertyInfo, type, options); + JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo.PropertyType, propertyInfo, currentType, options); Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null); // If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception. @@ -140,11 +140,12 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) // Overwrite the one just added since it has [JsonIgnore]. cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo; } - else if (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true) + else if (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name && + (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true)) { ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo); } - // else ignore jsonPropertyInfo since it has [JsonIgnore]. + // else ignore jsonPropertyInfo since it has [JsonIgnore] or it's hidden by a new slot. } } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 23a37890bb2dd1..2e08b329b3476a 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -81,6 +81,22 @@ public static void Serialize_public_property_on_conflict_with_private_due_to_pol Assert.Equal("ConflictingValue", obj.myString); } + [Fact] + public static void Serealize_new_slot_public_property_without_conflict_with_base_public_property() + { + // Serialize + var obj = new ClassWithNewSlotDecimalProperty(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{""MyNumeric"":1.5}", json); + + // Deserialize + json = @"{""MyNumeric"":2.5}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal(2.5M, obj.MyNumeric); + } + [Fact] public static void Ignore_non_public_property() { @@ -372,6 +388,16 @@ public class ClassWithIgnoredPropertyPolicyConflict internal string myString { get; set; } = "ConflictingValue"; } + public class ClassWithHiddenByNewSlotIntProperty + { + public int MyNumeric { get; set; } = 1; + } + + public class ClassWithNewSlotDecimalProperty : ClassWithHiddenByNewSlotIntProperty + { + public new decimal MyNumeric { get; set; } = 1.5M; + } + [Fact] public static void NoSetter() { From efd6ab9d031967b8f1b23b25c563c9d8c992778f Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Mon, 13 Apr 2020 14:54:16 +0300 Subject: [PATCH 08/16] Canged test naming style --- .../Serialization/PropertyVisibilityTests.cs | 33 +++++++++---------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 2e08b329b3476a..aceff1f3869d2a 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -12,7 +12,7 @@ namespace System.Text.Json.Serialization.Tests public static partial class PropertyVisibilityTests { [Fact] - public static void SerializeNewSlotPublicProperty() + public static void Serialize_NewSlotPublicProperty() { // Serialize var obj = new ClassWithNewSlotProperty(); @@ -29,7 +29,7 @@ public static void SerializeNewSlotPublicProperty() } [Fact] - public static void Serialize_base_public_property_on_conflict_with_derived_private() + public static void Serialize_BasePublicProperty_ConflictWithDerivedPrivate() { // Serialize var obj = new ClassWithNewSlotInternalProperty(); @@ -46,7 +46,7 @@ public static void Serialize_base_public_property_on_conflict_with_derived_priva } [Fact] - public static void Serialize_public_property_on_conflict_with_private_due_to_attributes() + public static void Serialize_PublicProperty_ConflictWithPrivateDueAttributes() { // Serialize var obj = new ClassWithPropertyNamingConflict(); @@ -63,7 +63,7 @@ public static void Serialize_public_property_on_conflict_with_private_due_to_att } [Fact] - public static void Serialize_public_property_on_conflict_with_private_due_to_policy() + public static void Serialize_PublicProperty_ConflictWithPrivateDuePolicy() { var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; @@ -82,7 +82,7 @@ public static void Serialize_public_property_on_conflict_with_private_due_to_pol } [Fact] - public static void Serealize_new_slot_public_property_without_conflict_with_base_public_property() + public static void Serealize_NewSlotPublicProperty_ConflictWithBasePublicProperty() { // Serialize var obj = new ClassWithNewSlotDecimalProperty(); @@ -98,7 +98,7 @@ public static void Serealize_new_slot_public_property_without_conflict_with_base } [Fact] - public static void Ignore_non_public_property() + public static void Ignore_NonPublicProperty() { // Serialize var obj = new ClassWithInternalProperty(); @@ -114,7 +114,7 @@ public static void Ignore_non_public_property() } [Fact] - public static void Ignore_ignored_new_slot_public_property() + public static void Ignore_NewSlotPublicPropertyIgnored() { // Serialize var obj = new ClassWithIgnoredNewSlotProperty(); @@ -131,7 +131,7 @@ public static void Ignore_ignored_new_slot_public_property() } [Fact] - public static void Ignore_ignored_base_public_property_on_conflict_with_derived_private() + public static void Ignore_BasePublicPropertyIgnored_ConflictWithDerivedPrivate() { // Serialize var obj = new ClassWithIgnoredPublicPropertyAndNewSlotPrivate(); @@ -148,7 +148,7 @@ public static void Ignore_ignored_base_public_property_on_conflict_with_derived_ } [Fact] - public static void Ignore_public_property_on_conflict_with_private_due_to_attributes() + public static void Ignore_PublicProperty_ConflictWithPrivateDueAttributes() { // Serialize var obj = new ClassWithIgnoredPropertyNamingConflict(); @@ -165,7 +165,7 @@ public static void Ignore_public_property_on_conflict_with_private_due_to_attrib } [Fact] - public static void Ignore_public_property_on_conflict_with_private_due_to_policy() + public static void Ignore_PublicProperty_ConflictWithPrivateDuePolicy() { var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; @@ -184,7 +184,7 @@ public static void Ignore_public_property_on_conflict_with_private_due_to_policy } [Fact] - public static void Throw_when_public_properties_conflict_due_to_attributes() + public static void Throw_PublicProperty_ConflictDueAttributes() { // Serialize var obj = new ClassWithPropertyNamingConflictWhichThrows(); @@ -198,7 +198,7 @@ public static void Throw_when_public_properties_conflict_due_to_attributes() } [Fact] - public static void Throw_when_public_properties_conflict_due_to_attributes_and_inheritance() + public static void Throw_PublicProperty_ConflictDueAttributes_SingleInheritance() { // Serialize var obj = new ClassInheritedWithPropertyNamingConflictWhichThrows(); @@ -212,7 +212,7 @@ public static void Throw_when_public_properties_conflict_due_to_attributes_and_i } [Fact] - public static void Throw_when_public_properties_conflict_due_to_attributes_and_double_inheritance() + public static void Throw_PublicProperty_ConflictDueAttributes_DoubleInheritance() { // Serialize var obj = new ClassTwiceInheritedWithPropertyNamingConflictWhichThrows(); @@ -226,7 +226,7 @@ public static void Throw_when_public_properties_conflict_due_to_attributes_and_d } [Fact] - public static void Throw_when_public_properties_conflict_due_to_policy() + public static void Throw_PublicProperty_ConflictDuePolicy() { var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; @@ -242,7 +242,7 @@ public static void Throw_when_public_properties_conflict_due_to_policy() } [Fact] - public static void Throw_when_public_properties_conflict_due_to_policy_and_inheritance() + public static void Throw_PublicProperty_ConflictDuePolicy_SingleInheritance() { var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; @@ -258,7 +258,7 @@ public static void Throw_when_public_properties_conflict_due_to_policy_and_inher } [Fact] - public static void Throw_when_public_properties_conflict_due_to_policy_and_double_inheritance() + public static void Throw_PublicProperty_ConflictDuePolicy_DobuleInheritance() { var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; @@ -317,7 +317,6 @@ public class ClassInheritedWithPropertyNamingConflictWhichThrows : ClassWithPubl public class ClassTwiceInheritedWithPropertyNamingConflictWhichThrowsDummy : ClassWithPublicProperty { - } public class ClassTwiceInheritedWithPropertyNamingConflictWhichThrows : ClassTwiceInheritedWithPropertyNamingConflictWhichThrowsDummy From 9e42d7ce77545d55c7a55344ff38d26f10e4d0fc Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Mon, 13 Apr 2020 15:02:38 +0300 Subject: [PATCH 09/16] Covered by new slot hidden member serialization --- .../Serialization/PropertyVisibilityTests.cs | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index aceff1f3869d2a..f9cf6c2a9e8f80 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -97,6 +97,23 @@ public static void Serealize_NewSlotPublicProperty_ConflictWithBasePublicPropert Assert.Equal(2.5M, obj.MyNumeric); } + [Fact] + public static void Serealize_NewSlotPublicProperty_SpecifiedJsonPropertyName() + { + // Serialize + var obj = new ClassWithNewSlotAttributedDecimalProperty(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{""MyNumeric"":1,""MyNewNumeric"":1.5}", json); + + // Deserialize + json = @"{""MyNumeric"":4,""MyNewNumeric"":2.5}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal(4, ((ClassWithHiddenByNewSlotIntProperty)obj).MyNumeric); + Assert.Equal(2.5M, ((ClassWithNewSlotAttributedDecimalProperty)obj).MyNumeric); + } + [Fact] public static void Ignore_NonPublicProperty() { @@ -397,6 +414,12 @@ public class ClassWithNewSlotDecimalProperty : ClassWithHiddenByNewSlotIntProper public new decimal MyNumeric { get; set; } = 1.5M; } + public class ClassWithNewSlotAttributedDecimalProperty : ClassWithHiddenByNewSlotIntProperty + { + [JsonPropertyName("MyNewNumeric")] + public new decimal MyNumeric { get; set; } = 1.5M; + } + [Fact] public static void NoSetter() { From cd56b451ddb8cab9896f6c84aa54ee8ba88c0d9f Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Mon, 13 Apr 2020 20:47:37 +0300 Subject: [PATCH 10/16] Fixed failing test --- .../tests/Serialization/PropertyVisibilityTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index f9cf6c2a9e8f80..a2ff19daabb4f1 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -104,10 +104,10 @@ public static void Serealize_NewSlotPublicProperty_SpecifiedJsonPropertyName() var obj = new ClassWithNewSlotAttributedDecimalProperty(); string json = JsonSerializer.Serialize(obj); - Assert.Equal(@"{""MyNumeric"":1,""MyNewNumeric"":1.5}", json); + Assert.Equal(@"{""MyNewNumeric"":1.5,""MyNumeric"":1}", json); // Deserialize - json = @"{""MyNumeric"":4,""MyNewNumeric"":2.5}"; + json = @"{""MyNewNumeric"":2.5,""MyNumeric"":4}"; obj = JsonSerializer.Deserialize(json); Assert.Equal(4, ((ClassWithHiddenByNewSlotIntProperty)obj).MyNumeric); From 87b0c43ea73435cca0f9038ac27abc6ea426e4d7 Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Tue, 14 Apr 2020 23:22:31 +0300 Subject: [PATCH 11/16] Added comment and improved test --- .../src/System/Text/Json/Serialization/JsonClassInfo.cs | 2 ++ .../tests/Serialization/PropertyVisibilityTests.cs | 3 ++- 2 files changed, 4 insertions(+), 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 8b83519949a0f8..cd6ff5fc45ce5e 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 @@ -143,6 +143,8 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) else if (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name && (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == 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. ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo); } // else ignore jsonPropertyInfo since it has [JsonIgnore] or it's hidden by a new slot. diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index a2ff19daabb4f1..c0b8d07fa1eb69 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -104,7 +104,8 @@ public static void Serealize_NewSlotPublicProperty_SpecifiedJsonPropertyName() var obj = new ClassWithNewSlotAttributedDecimalProperty(); string json = JsonSerializer.Serialize(obj); - Assert.Equal(@"{""MyNewNumeric"":1.5,""MyNumeric"":1}", json); + Assert.Contains(@"""MyNewNumeric"":1.5", json); + Assert.Contains(@"""MyNumeric"":1", json); // Deserialize json = @"{""MyNewNumeric"":2.5,""MyNumeric"":4}"; From 33cd6d6e2487c411b18da307180969d23bc4cf80 Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Mon, 20 Apr 2020 18:09:15 +0300 Subject: [PATCH 12/16] Fixed rebase issues --- .../src/System/Text/Json/Serialization/JsonClassInfo.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 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 cd6ff5fc45ce5e..1b87ef779a9c9b 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 @@ -116,7 +116,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) { if (JsonPropertyInfo.GetAttribute(propertyInfo) != null) { - ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, Type); + ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType); } // Non-public properties should not be included for (de)serialization. @@ -127,7 +127,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) if (propertyInfo.GetMethod?.IsPublic == true || propertyInfo.SetMethod?.IsPublic == true) { - JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo.PropertyType, propertyInfo, currentType, options); + 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. From 07fd692b36221a75e4007a8149fce0caf4880e8d Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Mon, 20 Apr 2020 18:17:11 +0300 Subject: [PATCH 13/16] Added more tests --- .../Serialization/PropertyVisibilityTests.cs | 69 ++++++++++++++++--- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index c0b8d07fa1eb69..3ed7b2ed905d22 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -169,14 +169,14 @@ public static void Ignore_BasePublicPropertyIgnored_ConflictWithDerivedPrivate() public static void Ignore_PublicProperty_ConflictWithPrivateDueAttributes() { // Serialize - var obj = new ClassWithIgnoredPropertyNamingConflict(); + var obj = new ClassWithIgnoredPropertyNamingConflictPrivate(); string json = JsonSerializer.Serialize(obj); Assert.Equal(@"{}", json); // Deserialize json = @"{""MyString"":""NewValue""}"; - obj = JsonSerializer.Deserialize(json); + obj = JsonSerializer.Deserialize(json); Assert.Equal("DefaultValue", obj.MyString); Assert.Equal("ConflictingValue", obj.ConflictingString); @@ -188,14 +188,50 @@ public static void Ignore_PublicProperty_ConflictWithPrivateDuePolicy() var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; // Serialize - var obj = new ClassWithIgnoredPropertyPolicyConflict(); + var obj = new ClassWithIgnoredPropertyPolicyConflictPrivate(); string json = JsonSerializer.Serialize(obj, options); Assert.Equal(@"{}", json); // Deserialize json = @"{""myString"":""NewValue""}"; - obj = JsonSerializer.Deserialize(json, options); + obj = JsonSerializer.Deserialize(json, options); + + Assert.Equal("DefaultValue", obj.MyString); + Assert.Equal("ConflictingValue", obj.myString); + } + + [Fact] + public static void Ignore_PublicProperty_ConflictWithPublicDueAttributes() + { + // Serialize + var obj = new ClassWithIgnoredPropertyNamingConflictPublic(); + string json = JsonSerializer.Serialize(obj); + + Assert.Equal(@"{}", json); + + // Deserialize + json = @"{""MyString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json); + + Assert.Equal("DefaultValue", obj.MyString); + Assert.Equal("ConflictingValue", obj.ConflictingString); + } + + [Fact] + public static void Ignore_PublicProperty_ConflictWithPublicDuePolicy() + { + var options = new JsonSerializerOptions { PropertyNamingPolicy = JsonNamingPolicy.CamelCase }; + + // Serialize + var obj = new ClassWithIgnoredPropertyPolicyConflictPublic(); + string json = JsonSerializer.Serialize(obj, options); + + Assert.Equal(@"{}", json); + + // Deserialize + json = @"{""myString"":""NewValue""}"; + obj = JsonSerializer.Deserialize(json, options); Assert.Equal("DefaultValue", obj.MyString); Assert.Equal("ConflictingValue", obj.myString); @@ -206,8 +242,8 @@ public static void Throw_PublicProperty_ConflictDueAttributes() { // Serialize var obj = new ClassWithPropertyNamingConflictWhichThrows(); - Assert.Throws( - () => JsonSerializer.Serialize(obj)); + var ex = Assert.Throws(() => JsonSerializer.Serialize(obj)); + Assert.Contains($"{typeof(ClassWithPublicProperty)}.{nameof(ClassWithPublicProperty.MyString)}", ex.Message); // Deserialize string json = @"{""MyString"":""NewValue""}"; @@ -388,7 +424,7 @@ public class ClassWithIgnoredPublicPropertyAndNewSlotPrivate : ClassWithIgnoredP internal new string MyString { get; set; } = "NewDefaultValue"; } - public class ClassWithIgnoredPropertyNamingConflict + public class ClassWithIgnoredPropertyNamingConflictPrivate { [JsonIgnore] public string MyString { get; set; } = "DefaultValue"; @@ -397,7 +433,7 @@ public class ClassWithIgnoredPropertyNamingConflict internal string ConflictingString { get; set; } = "ConflictingValue"; } - public class ClassWithIgnoredPropertyPolicyConflict + public class ClassWithIgnoredPropertyPolicyConflictPrivate { [JsonIgnore] public string MyString { get; set; } = "DefaultValue"; @@ -405,6 +441,23 @@ public class ClassWithIgnoredPropertyPolicyConflict internal string myString { get; set; } = "ConflictingValue"; } + public class ClassWithIgnoredPropertyNamingConflictPublic + { + [JsonIgnore] + public string MyString { get; set; } = "DefaultValue"; + + [JsonPropertyName(nameof(MyString))] + public string ConflictingString { get; set; } = "ConflictingValue"; + } + + public class ClassWithIgnoredPropertyPolicyConflictPublic + { + [JsonIgnore] + public string MyString { get; set; } = "DefaultValue"; + + public string myString { get; set; } = "ConflictingValue"; + } + public class ClassWithHiddenByNewSlotIntProperty { public int MyNumeric { get; set; } = 1; From 304fb568068338c918f2f8aabfe04149f2c891f9 Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Tue, 21 Apr 2020 12:19:41 +0300 Subject: [PATCH 14/16] Fixed tests --- .../tests/Serialization/PropertyVisibilityTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 3ed7b2ed905d22..4e05d8ae9a6b77 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -242,8 +242,8 @@ public static void Throw_PublicProperty_ConflictDueAttributes() { // Serialize var obj = new ClassWithPropertyNamingConflictWhichThrows(); - var ex = Assert.Throws(() => JsonSerializer.Serialize(obj)); - Assert.Contains($"{typeof(ClassWithPublicProperty)}.{nameof(ClassWithPublicProperty.MyString)}", ex.Message); + Assert.Throws( + () => JsonSerializer.Serialize(obj)); // Deserialize string json = @"{""MyString"":""NewValue""}"; From dc20062d7424c0a69959db78c2a5695dbdeeee1c Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Wed, 29 Apr 2020 13:49:41 +0300 Subject: [PATCH 15/16] Fixed tests --- .../tests/Serialization/PropertyVisibilityTests.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 4e05d8ae9a6b77..2cc311764cb8e1 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -208,14 +208,14 @@ public static void Ignore_PublicProperty_ConflictWithPublicDueAttributes() var obj = new ClassWithIgnoredPropertyNamingConflictPublic(); string json = JsonSerializer.Serialize(obj); - Assert.Equal(@"{}", json); + Assert.Equal(@"{""MyString"":""ConflictingValue""}", json); // Deserialize json = @"{""MyString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json); Assert.Equal("DefaultValue", obj.MyString); - Assert.Equal("ConflictingValue", obj.ConflictingString); + Assert.Equal("NewValue", obj.ConflictingString); } [Fact] @@ -227,14 +227,14 @@ public static void Ignore_PublicProperty_ConflictWithPublicDuePolicy() var obj = new ClassWithIgnoredPropertyPolicyConflictPublic(); string json = JsonSerializer.Serialize(obj, options); - Assert.Equal(@"{}", json); + Assert.Equal(@"{""myString"":""ConflictingValue""}", json); // Deserialize json = @"{""myString"":""NewValue""}"; obj = JsonSerializer.Deserialize(json, options); Assert.Equal("DefaultValue", obj.MyString); - Assert.Equal("ConflictingValue", obj.myString); + Assert.Equal("NewValue", obj.myString); } [Fact] From d600e91871e88e055d6e3b5cd45bdb3482907c0e Mon Sep 17 00:00:00 2001 From: YohDeadfall Date: Thu, 7 May 2020 00:10:21 +0300 Subject: [PATCH 16/16] Fixed rebase issues --- .../src/System/Text/Json/Serialization/JsonClassInfo.cs | 2 +- 1 file changed, 1 insertion(+), 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 1b87ef779a9c9b..184492ae5c4c6e 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 @@ -104,7 +104,7 @@ public JsonClassInfo(Type type, JsonSerializerOptions options) for (Type? currentType = type; currentType != null; currentType = currentType.BaseType) { - foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.DeclaredOnly)) + foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly)) { // Ignore indexers if (propertyInfo.GetIndexParameters().Length > 0)