Loosen property name collision detection involving hidden properties #36936
Conversation
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
| { | ||
| // 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. | ||
| // The collision is invalid if none of the following is true: |
There was a problem hiding this comment.
This is hard to follow. Can we just state the conditions when we do throw?
There was a problem hiding this comment.
I've simplified the comments.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (jsonPropertyInfo.IsIgnored) | ||
| { | ||
| (ignoredProperties ??= new HashSet<string>()).Add(propertyName); |
There was a problem hiding this comment.
Does this have to respect the case-insensitive mode?
There was a problem hiding this comment.
This cache tells us the CLR names for properties that have been ignored. This is used when looking at properties of a base type, and there's a conflict with a property at a derived type (which has a different CLR name, but the same JSON property name, due to JsonPropertyName or the naming policy).
If another property on a derived class has the same CLR name as the current property on the base type, we know that the former hid the latter. If the property on the derived class was ignored with JsonIgnore, we know to also ignore the current type being examined on the base type. This will mitigate the property name collision.
None of these considerations has to do with case-insensitivity.
| cache[jsonPropertyInfo.NameAsString] = jsonPropertyInfo; | ||
| } | ||
| else if (other.PropertyInfo?.Name != jsonPropertyInfo.PropertyInfo?.Name && | ||
| (jsonPropertyInfo.ShouldDeserialize == true || jsonPropertyInfo.ShouldSerialize == true)) | ||
| else if (!(isIgnored || other.PropertyInfo!.Name == propertyName || ignoredProperties?.Contains(propertyName) == true)) |
There was a problem hiding this comment.
is other.PropertyInfo!.Name == propertyName protecting against duplicate properties via [JsonPropertyName] and\or duplicate properties by casing (when using case-insensitive options?). Please add a comment around this.
There was a problem hiding this comment.
other.PropertyInfo!.Name == propertyName indicates that the previously cached property ("other") hides the current property (either with the new keyword, or by overriding it). We shouldn't throw in this case, as it is likely for the two properties result in the same property name (since they have the same CLR name). Rather, we should ignore the current property.
I've added a comment.
There was a problem hiding this comment.
Per offline review\discussion, these changes look good.
Notes:
- [JsonPropertyName] does not impact "hiding". A base property that has the attribute (with name "foo") will not hide a more derived type with a "foo" CLR property -- instead it throws InvalidOperation. The same applies if the derived type has [JsonPropertyName] and a base property also has a "foo" CLR property.
- A derived type hides a base type property of the same name (return type doesn't matter). The name can be case-insensitive if the JsonSerializerOptions are set for that.
…otnet#36936) * Loosen property name collision detection involving hidden properties * Delay ignored prop cache creation; add more tests * Clarify comments
ghost
locked as resolved and limited conversation to collaborators
The change in #32107 allowed the (de)serialization of hidden properties when the properties in the derived classes are ignored/non-public.
As a result of the change, the serializer now examines properties at all levels of the inheritance chain, rather than just the ones visible from the type to be serialized. This means that
InvalidOperationExceptioncould be thrown if there is a JSON property name conflict between inheritance levels, if the property higher in the inheritance chain was not hidden by the conflicting property in the more derived class (but by another property). A property is "hidden" if it is virtual and overridden in a derived class with polymorphism or with thenewkeyword.The breaking change was flagged in ASP.NET Core preview 5 validation: dotnet/aspnetcore#22132.
To fix the break, this change loosens property name collision detection involving hidden properties. Rather than throw
InvalidOperationException, the serializer will a ignore hidden property when the property that hid it is ignored and there's a property name conflict. This aligns with Newtonsoft.Json behavior.