[5.0-preview5] Loosen property name collision detection involving hidden properties #37105
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
…otnet#36936) * Loosen property name collision detection involving hidden properties * Delay ignored prop cache creation; add more tests * Clarify comments
layomia requested review from Anipik, BrennanConroy, danmoseley, ericstj and steveharter 
danmoseley reviewed May 28, 2020
| | ||
| if (jsonPropertyInfo.IsIgnored) | ||
| { | ||
| (ignoredProperties ??= new HashSet<string>()).Add(propertyName); |
Member
There was a problem hiding this comment.
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)
Contributor Author
| Approved offline by tactics. |
layomia added api-approved Contributor Author
| Libraries Build iOS x64 Release run was canceled. |
Member
| @layomia you need a reviewer to sign off before merging a PR. Leave it merged now but please get a reviewer. |
ghost locked as resolved and limited conversation to collaborators Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Description
Ports #36936 to preview 5.
Minor clean-up logic from #36648 was also in-lined here to mitigate a merge conflict.
Description from original PR (click to view)
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.Customer Impact
Prevents a regression between previews 4 & 5 where
JsonSerializerthrowsInvalidOperationExceptionin scenarios that were previously working. Fixes issue caught during preview 5 validation that could affect real world apps - dotnet/aspnetcore#22132.Risk
Tests have been added for this scenario. Also, the fix was tested in an exact repro for the scenario: https://github.com/layomia/OpenMURepro.