Fixed serialization of hidden base class members by YohDeadfall · Pull Request #32107 · dotnet/runtime

YohDeadfall · 2020-02-11T10:54:25Z

layomia

Just a few more test cases to consider.

layomia · 2020-02-18T19:52:21Z

@@ -11,6 +11,231 @@ namespace System.Text.Json.Serialization.Tests
 {
 public static class PropertyVisibilityTests


nit: can you use PascalCase for the tests names where possible and capitalize the first letter after an underscore where needed?

It's a naming used by EF Core for long test names, but am okay with changing this if you want, but first compare names.
Before: Serialize_base_public_property_on_conflict_with_derived_private
After: SerializeBasePublicPropertyOnConflictWithDerivedPrivate
The first one is more readable while the second looks like encoded data and is hard to read. So are you sure that pascal case is better? I can rename other tests and give them meaningful names instead of current (:

I do underscore-delimited pascal case chunks in such cases, e.g. Serialize_BasePublicProperty_OnConflictWithDerivedPrivate. The before pattern isn't used anywhere else in this codebase.

layomia · 2020-02-18T20:05:11Z

+ public string MyString { get; set; } = "DefaultValue";
+
+ [JsonPropertyName(nameof(MyString))]
+ internal string ConflictingString { get; set; } = "ConflictingValue";


We need a class variant where this property is public; and a test variant where the conflict spans across two inheritance levels.

layomia · 2020-02-19T00:23:14Z

@@ -11,6 +11,231 @@ namespace System.Text.Json.Serialization.Tests
 {
 public static class PropertyVisibilityTests


We need a couple more tests involving 3 levels of inheritance, particularly where we have collisions on public properties defined at different levels.

layomia

This PR should fix #30964 as well.

We need tests for the scenarios in #30964 (comment), particularly this case:

publicclassMyClass{// This should be ignored.publicintMyNumber{get;set;}}publicclassMyDerivedClass:MyClass{// This should be (de)serialized.newpublicdoubleMyNumber{get;set;}}

layomia · 2020-02-28T14:03:41Z

@@ -11,6 +11,231 @@ namespace System.Text.Json.Serialization.Tests
 {
 public static class PropertyVisibilityTests


I do underscore-delimited pascal case chunks in such cases, e.g. Serialize_BasePublicProperty_OnConflictWithDerivedPrivate. The before pattern isn't used anywhere else in this codebase.

YohDeadfall · 2020-03-26T08:17:29Z

@layomia I finally found some time to finish this, but there is one question. Is there any reason to create a JsonProperyInfo for properties marked as ignored? Probably such members should be filtered out before creation JsonProperyInfos for them, or we could ignore suck infos and not to add them into a cache.

This will help with two things:

eliminate visibility checks causes by name collisions;
decrease memory usage.

If an member exists, then it's visible already (at least supports serialization or deserialization separately). If it's in a base class, we will overwrite it. Otherwise, throw an exception.

layomia

#32107 (review) is yet to be addressed.

layomia · 2020-04-11T18:11:48Z

Is there any reason to create a JsonProperyInfo for properties marked as ignored?

Creating a JsonPropertyInfo for ignored properties helps us distinguish between JSON to be ignored on deserialization, or JSON to be put in extension data (if available).

Perhaps we can remove instances where JsonPropertyInfo.ShouldSerialize is false from the serialization cache since we'll never serialize, but that should be considered outside of this PR.

layomia · 2020-04-13T17:45:47Z

Sample test failure log: https://helix.dot.net/api/2019-06-17/jobs/2609f8f5-cf39-4b17-a5ec-d3ff24614993/workitems/System.Text.Json.Tests/console

YohDeadfall · 2020-04-13T17:48:35Z

Thanks, saw this and fixed already (:

YohDeadfall · 2020-04-30T19:09:20Z

@layomia tests fail, but for unrelated to this PR issue. Should I address it here?

jozkee · 2020-04-30T20:14:29Z

It seems to me like the test failures are related to this PR since those doesn't fail on master, only here.

YohDeadfall · 2020-04-30T20:32:15Z

The failing test should not be affected by this PR since it deserializes an object from an empty string. That area isn't touched, but will take a deeper look.

jozkee · 2020-04-30T20:34:44Z

You removed the changes to JsonClassInfo recently introduced by #34675
b3e791d#diff-ba9769bf9ab1b28aebe3b65065b96b10
Probably when merging with master.

YohDeadfall · 2020-05-07T08:17:46Z

@jozkee @layomia, I've fixed merge issues, so all tests pass.

layomia

Thanks a ton!

layomia · 2020-05-23T18:47:15Z

This is a breaking change between .NET 3.1 Core/System.Text.Json 4.7.1 and .NET 5/System.Text.Json 5.0.0.

Rather than only observing properties visible from the type to be serialized, the serializer looks at the properties for all the base types in the inheritance chain. This means that InvalidOperationException could 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 the new keyword.

The conflict would have to be fixed to avoid this exception. The most common way might be to place [JsonIgnore] on the conflicting property higher up in the inheritance chain.

layomia · 2020-05-28T23:47:31Z

Follow up: the breaking change this PR introduced was mitigated with #36936.

danmoseley · 2020-07-24T21:42:29Z

@layomia this is labeled breaking change. Can you please open an issue if necessary with https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

I don't see an existing one

layomia · 2020-10-02T18:40:46Z

Removing the breaking-change labels from this PR since the breaking change it introduced (#32107 (comment)) has been mitigated/reversed in follow up PRs #36936 & #37720 to avoid breaking the OpenMU app from internal AppCompat runs - #37640.

YohDeadfall requested review from ahsonkhan, jozkee, layomia and steveharter as code owners February 11, 2020 10:54

Dotnet-GitSync-Bot added the area-Serialization label Feb 11, 2020

YohDeadfall mentioned this pull request Feb 11, 2020
Added tests to check non-public props are ignored #32019
Closed

YohDeadfall force-pushed the fix-hidden-props branch 4 times, most recently from ad02d69 to 19c48b1Compare February 11, 2020 13:45

layomia reviewed Feb 19, 2020
View reviewed changes

ahsonkhan added area-System.Text.Json and removed area-Serialization labels Feb 19, 2020

YohDeadfall force-pushed the fix-hidden-props branch from 90e8a96 to d08435eCompare February 23, 2020 09:57

layomia suggested changes Feb 28, 2020
View reviewed changes

layomia added this to the 5.0 milestone Mar 13, 2020

YohDeadfall force-pushed the fix-hidden-props branch from 66b899a to 6d95cddCompare March 26, 2020 07:23

YohDeadfall force-pushed the fix-hidden-props branch from 6d95cdd to 6c4c9b4Compare March 26, 2020 08:45

jaredpar mentioned this pull request Mar 26, 2020
Caching.Memory tests are impacting CI reliability #34128
Closed

YohDeadfall force-pushed the fix-hidden-props branch 2 times, most recently from dd06a54 to 9748e61Compare April 7, 2020 16:32

jaredpar mentioned this pull request Apr 7, 2020
CI / PR having issues provisioning resources #34647
Closed

YohDeadfall force-pushed the fix-hidden-props branch from 9748e61 to 9c90500Compare April 8, 2020 15:35

jaredpar mentioned this pull request Apr 8, 2020
Errors installing the SDK during builds #34015
Closed

YohDeadfall mentioned this pull request Apr 10, 2020
JSON Serialization Name Collision with hidden properties #30964
Closed

layomia reviewed Apr 11, 2020
View reviewed changes

YohDeadfall added 2 commits April 30, 2020 15:16

Fixed tests
304fb56

Fixed tests
dc20062

YohDeadfall force-pushed the fix-hidden-props branch from 08d24f9 to dc20062Compare April 30, 2020 12:16

YohDeadfall requested a review from jozkee April 30, 2020 19:08

Fixed rebase issues
d600e91

YohDeadfall force-pushed the fix-hidden-props branch from 4213c34 to d600e91Compare May 6, 2020 21:10

layomia approved these changes May 8, 2020
View reviewed changes

layomia merged commit 32120db into dotnet:masterMay 8, 2020

layomia assigned YohDeadfall May 15, 2020

YohDeadfall deleted the fix-hidden-props branch May 17, 2020 18:22

layomia added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label May 23, 2020

This was referenced May 24, 2020
Loosen property name collision detection involving hidden properties #36936
Merged
Added field support to JSON serializer #36986
Merged
[5.0-preview5] Loosen property name collision detection involving hidden properties #37105
Merged

layomia mentioned this pull request Jun 10, 2020
[5.0.100-preview.6.20308.7] OpenMu button disabled #37640
Closed

layomia mentioned this pull request Jun 20, 2020
[Tests] Ensure JsonSerializer doesn't (de)serialize non-public properties #31633
Closed

loic-sharma mentioned this pull request Jul 20, 2020
Move to System.Text.Json loic-sharma/BaGet#559
Merged

danmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Jul 30, 2020

layomia removed breaking-change Issue or PR that represents a breaking API or functional change over a previous release. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet labels Oct 2, 2020

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

		@@ -11,6 +11,231 @@ namespace System.Text.Json.Serialization.Tests
		{
		public static class PropertyVisibilityTests

Uh oh!

Conversation

YohDeadfall commented Feb 11, 2020• edited by layomia Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

layomiaFeb 18, 2020

Choose a reason for hiding this comment

Uh oh!

YohDeadfallFeb 23, 2020

Choose a reason for hiding this comment

Uh oh!

layomiaFeb 28, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layomiaFeb 18, 2020

Choose a reason for hiding this comment

Uh oh!

layomiaFeb 19, 2020

Choose a reason for hiding this comment

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

layomiaFeb 28, 2020

Choose a reason for hiding this comment

Uh oh!

YohDeadfall commented Mar 26, 2020

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

layomia commented Apr 11, 2020

Uh oh!

layomia commented Apr 13, 2020

Uh oh!

YohDeadfall commented Apr 13, 2020

Uh oh!

YohDeadfall commented Apr 30, 2020

Uh oh!

jozkee commented Apr 30, 2020

Uh oh!

YohDeadfall commented Apr 30, 2020

Uh oh!

jozkee commented Apr 30, 2020

Uh oh!

YohDeadfall commented May 7, 2020

Uh oh!

layomia left a comment

Choose a reason for hiding this comment

Uh oh!

layomia commented May 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

layomia commented May 28, 2020

Uh oh!

danmoseley commented Jul 24, 2020

Uh oh!

layomia commented Oct 2, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

YohDeadfall commented Feb 11, 2020•
edited by layomia
Loading

layomia commented May 23, 2020•
edited
Loading

layomia commented Oct 2, 2020•
edited
Loading