Skip to content

Commit

Permalink
Loosen property name collision detection involving hidden properties (d…
Browse files Browse the repository at this point in the history 
…otnet#36936) (dotnet#37105)

* Loosen property name collision detection involving hidden properties

* Delay ignored prop cache creation; add more tests

* Clarify comments
  • Loading branch information
@layomia
layomia authored May 28, 2020
1 parent 201841e commit 4ae4e2f
Show file tree
Hide file tree
Showing 2 changed files with 146 additions and 27 deletions.
61 changes: 34 additions & 27 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
View file
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);
}
}
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
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

0 comments on commit 4ae4e2f

Please sign in to comment.