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

Ignore overridden properties in JSON (de)serialization #52036

Closed
wants to merge 4 commits into from
Closed

Ignore overridden properties in JSON (de)serialization #52036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d6d7e77
fields cannot be virtual
devsko Apr 26, 2021
ad23d17
introduce dictionary overrideProperties
devsko Apr 28, 2021
a6f8943
check overridden properties
devsko Apr 29, 2021
5b26a2d
merge main
devsko Apr 29, 2021
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
14 changes: 14 additions & 0 deletions src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,20 @@ public static Dictionary<TKey, TValue> CreateDictionaryFromCollection<TKey, TVal
#endif
}

/// <summary>
/// Adds a <paramref name="value"/> to the <paramref name="dictionary"/> or returns the <paramref name="existing"/> value.
/// </summary>
public static bool TryAdd<TKey, TValue>(Dictionary<TKey, TValue> dictionary, in TKey key, in TValue value, [MaybeNullWhen(true)] out TValue existing) where TKey : notnull
{
if (!dictionary.TryGetValue(key, out existing))
{
dictionary[key] = value;
return true;
}

return false;
}

public static bool IsFinite(double value)
{
#if BUILDING_INBOX_LIBRARY
Expand Down
87 changes: 49 additions & 38 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ internal JsonTypeInfo(Type type, JsonConverter converter, Type runtimeType, Json
: StringComparer.Ordinal);

Dictionary<string, MemberInfo>? ignoredMembers = null;
Dictionary<string, PropertyInfo>? overrideProperties = null;

// We start from the most derived type.
for (Type? currentType = type; currentType != null; currentType = currentType.BaseType)
Expand All @@ -220,7 +221,7 @@ internal JsonTypeInfo(Type type, JsonConverter converter, Type runtimeType, Json
if (propertyInfo.GetMethod?.IsPublic == true ||
propertyInfo.SetMethod?.IsPublic == true)
{
CacheMember(currentType, propertyInfo.PropertyType, propertyInfo, typeNumberHandling, cache, ref ignoredMembers);
CacheMember(currentType, propertyInfo.PropertyType, propertyInfo, typeNumberHandling, cache, ref ignoredMembers, ref overrideProperties);
}
else
{
Expand All @@ -235,18 +236,13 @@ internal JsonTypeInfo(Type type, JsonConverter converter, Type runtimeType, Json

foreach (FieldInfo fieldInfo in currentType.GetFields(bindingFlags))
{
if (PropertyIsOverridenAndIgnored(fieldInfo, ignoredMembers))
{
continue;
}

bool hasJsonInclude = JsonPropertyInfo.GetAttribute<JsonIncludeAttribute>(fieldInfo) != null;

if (fieldInfo.IsPublic)
{
if (hasJsonInclude || Options.IncludeFields)
{
CacheMember(currentType, fieldInfo.FieldType, fieldInfo, typeNumberHandling, cache, ref ignoredMembers);
CacheMember(currentType, fieldInfo.FieldType, fieldInfo, typeNumberHandling, cache, ref ignoredMembers, ref overrideProperties);
}
}
else
Expand Down Expand Up @@ -328,18 +324,23 @@ private void CacheMember(
MemberInfo memberInfo,
JsonNumberHandling? typeNumberHandling,
Dictionary<string, JsonPropertyInfo> cache,
ref Dictionary<string, MemberInfo>? ignoredMembers)
ref Dictionary<string, MemberInfo>? ignoredMembers,
ref Dictionary<string, PropertyInfo>? overrideProperties)
{
JsonPropertyInfo jsonPropertyInfo = AddProperty(memberInfo, memberType, declaringType, typeNumberHandling, Options);
Debug.Assert(jsonPropertyInfo.NameAsString != null);

string memberName = memberInfo.Name;

// The JsonPropertyNameAttribute or naming policy resulted in a collision.
if (!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo))
PropertyInfo? property = memberInfo as PropertyInfo;

// Virtual properties that are overrides are implicitly ignored regardless of a JsonPropertyNameAttribute.
if ((property == null ||
overrideProperties == null ||
!overrideProperties.TryGetValue(memberName, out PropertyInfo? overrideProperty) ||
!IsOverrideOf(overrideProperty, property)) &&
!JsonHelpers.TryAdd(cache, jsonPropertyInfo.NameAsString, jsonPropertyInfo, out JsonPropertyInfo? other))
{
JsonPropertyInfo other = cache[jsonPropertyInfo.NameAsString];

// The JsonPropertyNameAttribute or naming policy resulted in a collision.
if (other.IsIgnored)
{
// Overwrite previously cached property since it has [JsonIgnore].
Expand All @@ -351,7 +352,7 @@ private void CacheMember(
// Is the current property hidden by the previously cached property
// (with `new` keyword, or by overriding)?
other.MemberInfo!.Name != memberName &&
// 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.
ignoredMembers?.ContainsKey(memberName) != true)
{
Expand All @@ -365,6 +366,34 @@ private void CacheMember(
{
(ignoredMembers ??= new Dictionary<string, MemberInfo>()).Add(memberName, memberInfo);
}
else if (property != null && IsOverride(property))
{
(overrideProperties ??= new Dictionary<string, PropertyInfo>())[memberName] = property;
}
}

private static bool IsOverrideOf(PropertyInfo property, PropertyInfo other)
{
MethodInfo? getMethod = property.GetMethod;
if (getMethod != null)
{
return getMethod.GetBaseDefinition() == other.GetMethod?.GetBaseDefinition();
}

MethodInfo? setMethod = property.SetMethod;
return setMethod != null && setMethod.GetBaseDefinition() == other.SetMethod?.GetBaseDefinition();
}

private static bool IsOverride(PropertyInfo property)
{
MethodInfo? getMethod = property.GetMethod;
if (getMethod != null)
{
return getMethod.GetBaseDefinition() != getMethod;
}

MethodInfo? setMethod = property.SetMethod;
return setMethod != null && setMethod.GetBaseDefinition() == setMethod;
}

private sealed class ParameterLookupKey
Expand Down Expand Up @@ -430,11 +459,10 @@ static Type GetMemberType(MemberInfo memberInfo)
string propertyName = jsonProperty.MemberInfo!.Name;
var key = new ParameterLookupKey(propertyName, GetMemberType(jsonProperty.MemberInfo));
var value= new ParameterLookupValue(jsonProperty);
if (!JsonHelpers.TryAdd(nameLookup, key, value))
if (!JsonHelpers.TryAdd(nameLookup, key, value, out ParameterLookupValue? existing))
{
// More than one property has the same case-insensitive name and Type.
// Remember so we can throw a nice exception if this property is used as a parameter name.
ParameterLookupValue existing = nameLookup[key];
existing!.DuplicateName = propertyName;
}
}
Expand Down Expand Up @@ -476,33 +504,16 @@ static Type GetMemberType(MemberInfo memberInfo)
ParameterCache = parameterCache;
ParameterCount = parameters.Length;
}
private static bool PropertyIsOverridenAndIgnored(MemberInfo currentMember, Dictionary<string, MemberInfo>? ignoredMembers)
private static bool PropertyIsOverridenAndIgnored(PropertyInfo currentProperty, Dictionary<string, MemberInfo>? ignoredMembers)
{
if (ignoredMembers == null || !ignoredMembers.TryGetValue(currentMember.Name, out MemberInfo? ignoredProperty))
if (ignoredMembers == null ||
!ignoredMembers.TryGetValue(currentProperty.Name, out MemberInfo? ignoredMember) ||
ignoredMember is not PropertyInfo ignoredProperty)
{
return false;
}

Debug.Assert(currentMember is PropertyInfo || currentMember is FieldInfo);
PropertyInfo? currentPropertyInfo = currentMember as PropertyInfo;
Type currentMemberType = currentPropertyInfo == null
? Unsafe.As<FieldInfo>(currentMember).FieldType
: currentPropertyInfo.PropertyType;

Debug.Assert(ignoredProperty is PropertyInfo || ignoredProperty is FieldInfo);
PropertyInfo? ignoredPropertyInfo = ignoredProperty as PropertyInfo;
Type ignoredPropertyType = ignoredPropertyInfo == null
? Unsafe.As<FieldInfo>(ignoredProperty).FieldType
: ignoredPropertyInfo.PropertyType;

return currentMemberType == ignoredPropertyType &&
PropertyIsVirtual(currentPropertyInfo) &&
PropertyIsVirtual(ignoredPropertyInfo);
}

private static bool PropertyIsVirtual(PropertyInfo? propertyInfo)
{
return propertyInfo != null && (propertyInfo.GetMethod?.IsVirtual == true || propertyInfo.SetMethod?.IsVirtual == true);
return IsOverrideOf(currentProperty, ignoredProperty);
}

private bool DetermineExtensionDataProperty(Dictionary<string, JsonPropertyInfo> cache)
Expand Down
39 changes: 39 additions & 0 deletions ...es/System.Text.Json/tests/System.Text.Json.Tests/Serialization/PropertyVisibilityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,9 @@ public static void HiddenPropertiesIgnored_WhenOverridesIgnored()
string serialized = JsonSerializer.Serialize(new DerivedClass_With_IgnoredOverride());
Assert.Equal(@"{}", serialized);

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

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

Expand Down Expand Up @@ -627,6 +630,20 @@ public static void HiddenPropertiesIgnored_WhenOverridesIgnored()
Assert.Equal(@"{""MyProp"":null}", serialized);
}

[Fact]
public static void OverriddenPropertiesIgnored_WhenRenamed()
{
string serialized = JsonSerializer.Serialize(new DerivedClass_WithRenamedOverride());
Assert.Equal(@"{""Renamed"":true}", serialized);

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

// Don't ignore virtual properties with same name and type when they are in a new slot.
serialized = JsonSerializer.Serialize(new FurtherDerivedClass_WithRenamedNewSlotVirtual());
Assert.Equal(@"{""MyProp"":false,""Renamed"":true}", serialized);
}

public class ClassWithInternalField
{
internal string MyString = "DefaultValue";
Expand Down Expand Up @@ -852,6 +869,12 @@ private class Class_With_VirtualProperty
public virtual bool MyProp { get; set; }
}

private class DerivedClass_With_IgnoredNewSlotVirtual : Class_With_VirtualProperty
{
[JsonIgnore]
public new virtual bool MyProp { get; set; }
}

private class DerivedClass_With_IgnoredOverride : Class_With_VirtualProperty
{
[JsonIgnore]
Expand Down Expand Up @@ -953,6 +976,22 @@ private class FurtherDerivedClass_With_IgnoredOverride : DerivedClass_WithConfli
public override bool MyProp { get; set; }
}

private class DerivedClass_WithRenamedOverride : Class_With_VirtualProperty
{
[JsonPropertyName("Renamed")]
public override bool MyProp { get; set; } = true;
}

private class FurtherDerivedClass_WithRenamedOverride : DerivedClass_WithRenamedOverride
{
public override bool MyProp { get; set; }
}

private class FurtherDerivedClass_WithRenamedNewSlotVirtual : DerivedClass_WithRenamedOverride
{
public new virtual bool MyProp { get; set; }
}

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