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

Fixed serialization of hidden base class members #32107

Merged
merged 16 commits into from
May 8, 2020
Merged

Fixed serialization of hidden base class members #32107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
05639b9
Fixed serialization of hidden base class members
YohDeadfall Feb 11, 2020
a01bbe8
Added tests to check non-public props are ignored
YohDeadfall Feb 3, 2020
827df7e
Simplified cache construction
YohDeadfall Feb 11, 2020
ed8ca2b
Addressed review issues
YohDeadfall Feb 23, 2020
c3d7228
Added requested tests
YohDeadfall Feb 23, 2020
8f9653b
Fixed tests
YohDeadfall Feb 23, 2020
335c9d0
Fixed handling of public hidden prop by new slot
YohDeadfall Apr 7, 2020
efd6ab9
Canged test naming style
YohDeadfall Apr 13, 2020
9e42d7c
Covered by new slot hidden member serialization
YohDeadfall Apr 13, 2020
cd56b45
Fixed failing test
YohDeadfall Apr 13, 2020
87b0c43
Added comment and improved test
YohDeadfall Apr 14, 2020
33cd6d6
Fixed rebase issues
YohDeadfall Apr 20, 2020
07fd692
Added more tests
YohDeadfall Apr 20, 2020
304fb56
Fixed tests
YohDeadfall Apr 21, 2020
dc20062
Fixed tests
YohDeadfall Apr 29, 2020
d600e91
Fixed rebase issues
YohDeadfall May 6, 2020
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
74 changes: 40 additions & 34 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 @@ -97,52 +97,58 @@ public JsonClassInfo(Type type, JsonSerializerOptions options)
case ClassType.Object:
{
CreateObject = options.MemberAccessorStrategy.CreateConstructor(type);
Dictionary<string, JsonPropertyInfo> cache = new Dictionary<string, JsonPropertyInfo>(
Options.PropertyNameCaseInsensitive
? StringComparer.OrdinalIgnoreCase
: StringComparer.Ordinal);

PropertyInfo[] properties = type.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);

Dictionary<string, JsonPropertyInfo> cache = CreatePropertyCache(properties.Length);

foreach (PropertyInfo propertyInfo in properties)
for (Type? currentType = type; currentType != null; currentType = currentType.BaseType)
{
// Ignore indexers
if (propertyInfo.GetIndexParameters().Length > 0)
foreach (PropertyInfo propertyInfo in currentType.GetProperties(BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.DeclaredOnly))
{
continue;
}

if (IsNonPublicProperty(propertyInfo))
{
if (JsonPropertyInfo.GetAttribute<JsonIncludeAttribute>(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<JsonIncludeAttribute>(propertyInfo) != null)
{
ThrowHelper.ThrowInvalidOperationException_JsonIncludeOnNonPublicInvalid(propertyInfo, currentType);
}

// 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, currentType, 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 (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name &&
Copy link
Contributor

@layomia layomia Apr 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nit: we should add a comment here saying if the property names are equal, then the new keyword was used to override the property.

(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.
}
// else ignore jsonPropertyInfo since it has [JsonIgnore].
}
}
}
Expand Down
Loading