Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.0-preview5] Loosen property name collision detection involving hidden properties #37105

Merged
merged 1 commit into from
May 28, 2020
Merged

[5.0-preview5] Loosen property name collision detection involving hidden properties #37105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 34 additions & 27 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,9 @@ public JsonClassInfo(Type type, JsonSerializerOptions options)
? StringComparer.OrdinalIgnoreCase
: StringComparer.Ordinal);

HashSet<string>? ignoredProperties = null;

// We start from the most derived type and ascend to the base type.
for (Type? currentType = type; currentType != null; currentType = currentType.BaseType)
{
foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly))
Expand All @@ -111,44 +114,55 @@ public JsonClassInfo(Type type, JsonSerializerOptions options)
continue;
}

if (IsNonPublicProperty(propertyInfo))
{
if (JsonPropertyInfo.GetAttribute<JsonIncludeAttribute>(propertyInfo) != null)
{
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType);
}

// Non-public properties should not be included for (de)serialization.
continue;
}

// For now we only support public getters\setters
// For now we only support public properties (i.e. setter and/or getter is public).
if (propertyInfo.GetMethod?.IsPublic == true ||
propertyInfo.SetMethod?.IsPublic == true)
{
JsonPropertyInfo jsonPropertyInfo = AddProperty(propertyInfo, currentType, options);
Debug.Assert(jsonPropertyInfo != null && jsonPropertyInfo.NameAsString != null);

// If the JsonPropertyNameAttribute or naming policy results in collisions, throw an exception.
string propertyName = propertyInfo.Name;

// The JsonPropertyNameAttribute or naming policy resulted in a collision.
if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo))
{
JsonPropertyInfo other = cache[jsonPropertyInfo.NameAsString];

if (other.ShouldDeserialize == false && other.ShouldSerialize == false)
if (other.IsIgnored)
{
// Overwrite the one just added since it has [JsonIgnore].
// Overwrite previously cached property since it has [JsonIgnore].
cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo;
}
else if (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name &&
(jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true))
else if (
// Does the current property have `JsonIgnoreAttribute`?
!jsonPropertyInfo.IsIgnored &&
// Is the current property hidden by the previously cached property
// (with `new` keyword, or by overriding)?
other.PropertyInfo!.Name != propertyName &&
// Was a property with the same CLR name was ignored? That property hid the current property,
// thus, if it was ignored, the current property should be ignored too.
ignoredProperties?.Contains(propertyName) != true)
{
// Check for name equality is required to determine when a new slot is used for the member.
// Therefore, if names are not the same, there is conflict due to the name policy or attributes.
// We throw if we have two public properties that have the same JSON property name, and neither have been ignored.
ThrowHelper.ThrowInvalidOperationException_SerializerPropertyNameConflict(Type, jsonPropertyInfo);
}
// else ignore jsonPropertyInfo since it has [JsonIgnore] or it's hidden by a new slot.
// Ignore the current property.
}

if (jsonPropertyInfo.IsIgnored)
{
(ignoredProperties ??= new HashSet<string>()).Add(propertyName);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style (no need to change for this PR) -- where a collection's key is of type string, providing the StringComparer is more explicit and clear (StringComparer.Ordinal just calls string.Equals as does the generic comparer you get by default so it comes to the same thing)

}
}
else
{
if (JsonPropertyInfo.GetAttribute<JsonIncludeAttribute>(propertyInfo) != null)
{
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType);
}

// Non-public properties should not be included for (de)serialization.
}
}
}

Expand Down Expand Up @@ -211,13 +225,6 @@ public JsonClassInfo(Type type, JsonSerializerOptions options)
}
}

private static bool IsNonPublicProperty(PropertyInfo propertyInfo)
{
MethodInfo? getMethod = propertyInfo.GetMethod;
MethodInfo? setMethod = propertyInfo.SetMethod;
return !((getMethod != null && getMethod.IsPublic) || (setMethod != null && setMethod.IsPublic));
}

private void InitializeConstructorParameters(Dictionary<string, JsonPropertyInfo> propertyCache, ConstructorInfo constructorInfo)
{
ParameterInfo[] parameters = constructorInfo!.GetParameters();
Expand Down
112 changes: 112 additions & 0 deletions src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,39 @@ public static void Throw_PublicProperty_ConflictDuePolicy_DobuleInheritance()
() => JsonSerializer.Deserialize<ClassTwiceInheritedWithPropertyPolicyConflictWhichThrows>(json, options));
}

[Fact]
public static void HiddenPropertiesIgnored_WhenOverridesIgnored_AndPropertyNameConflicts()
{
string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride());
Assert.Equal(@"{""MyProp"":false}", serialized);

serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName());
Assert.Equal(@"{""MyProp"":null}", serialized);

serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty());
Assert.Equal(@"{""MyProp"":false}", serialized);

serialized = JsonSerializer.Serialize(new DerivedClass_With_NewProperty_And_ConflictingPropertyName());
Assert.Equal(@"{""MyProp"":null}", serialized);

serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType());
Assert.Equal(@"{""MyProp"":false}", serialized);

serialized = JsonSerializer.Serialize(new DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName());
Assert.Equal(@"{""MyProp"":null}", serialized);

serialized = JsonSerializer.Serialize(new DerivedClass_WithIgnoredOverride());
Assert.Equal(@"{""MyProp"":false}", serialized);

serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_ConflictingPropertyName());
Assert.Equal(@"{""MyProp"":null}", serialized);

Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(new DerivedClass_WithConflictingPropertyName()));

serialized = JsonSerializer.Serialize(new FurtherDerivedClass_With_IgnoredOverride());
Assert.Equal(@"{""MyProp"":null}", serialized);
}

public class ClassWithInternalProperty
{
internal string MyString { get; set; } = "DefaultValue";
Expand Down Expand Up @@ -474,6 +507,85 @@ public class ClassWithNewSlotAttributedDecimalProperty : ClassWithHiddenByNewSlo
public new decimal MyNumeric { get; set; } = 1.5M;
}

private class Class_With_VirtualProperty
{
public virtual bool MyProp { get; set; }
}

private class DerivedClass_With_IgnoredOverride : Class_With_VirtualProperty
{
[JsonIgnore]
public override bool MyProp { get; set; }
}

private class DerivedClass_With_IgnoredOverride_And_ConflictingPropertyName : Class_With_VirtualProperty
{
[JsonPropertyName("MyProp")]
public string MyString { get; set; }

[JsonIgnore]
public override bool MyProp { get; set; }
}

private class Class_With_Property
{
public bool MyProp { get; set; }
}

private class DerivedClass_With_NewProperty : Class_With_Property
{
[JsonIgnore]
public new bool MyProp { get; set; }
}

private class DerivedClass_With_NewProperty_And_ConflictingPropertyName : Class_With_Property
{
[JsonPropertyName("MyProp")]
public string MyString { get; set; }

[JsonIgnore]
public new bool MyProp { get; set; }
}

private class DerivedClass_WithNewProperty_Of_DifferentType : Class_With_Property
{
[JsonIgnore]
public new int MyProp { get; set; }
}

private class DerivedClass_WithNewProperty_Of_DifferentType_And_ConflictingPropertyName : Class_With_Property
{
[JsonPropertyName("MyProp")]
public string MyString { get; set; }

[JsonIgnore]
public new int MyProp { get; set; }
}

private class DerivedClass_WithIgnoredOverride : Class_With_VirtualProperty
{
[JsonIgnore]
public override bool MyProp { get; set; }
}

private class FurtherDerivedClass_With_ConflictingPropertyName : DerivedClass_WithIgnoredOverride
{
[JsonPropertyName("MyProp")]
public string MyString { get; set; }
}

private class DerivedClass_WithConflictingPropertyName : Class_With_VirtualProperty
{
[JsonPropertyName("MyProp")]
public string MyString { get; set; }
}

private class FurtherDerivedClass_With_IgnoredOverride : DerivedClass_WithConflictingPropertyName
{
[JsonIgnore]
public override bool MyProp { get; set; }
}

[Fact]
public static void NoSetter()
{
Expand Down