Skip to content

Eliminated info creation for non-public properties - #2278

Merged
layomia merged 1 commit into
dotnet:masterfrom
YohDeadfall:use-pub-props-only
Feb 2, 2020
Merged

Eliminated info creation for non-public properties#2278
layomia merged 1 commit into
dotnet:masterfrom
YohDeadfall:use-pub-props-only

Conversation

@YohDeadfall

@YohDeadfallYohDeadfall commented Jan 28, 2020

Copy link
Copy Markdown
Contributor

Since JSON serialization doesn't support private properties at all, there is no need to create info for them. Previously discussed in #2242.

This was referenced Jan 28, 2020
Closed
Closed
layomia
layomia reviewed Jan 28, 2020
View reviewed changes
Comment threadsrc/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
CreateObject = options.MemberAccessorStrategy.CreateConstructor(type);

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

@layomialayomiaJan 28, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We should add some tests ensuring that the serializer doesn't deserialize/serialize non-public properties under any circumstance including:

  • when a derived type adds a non-public new property which collides with a public property defined on the parent
  • when there are property name collisions between public/non-public properties due to [JsonPropertyName]
  • when there are property name collisions between public/non-public properties due to JsonNamingPolicy

all such tests should have variants checking that public properties that are [JsonIgnore]'d are not accidentally (de)serialized due to mismatching JsonPropertyInfo with a colliding non-public member.

https://dotnetfiddle.net/2BD7bK goes over a few expecations.

@ahsonkhanahsonkhanJan 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@YohDeadfall - let us know if you are up for adding the tests in this PR based on what @layomia suggested (its fine if you prefer someone else take that on, but will certainly be helpful to add).

@YohDeadfallYohDeadfallJan 28, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Though about improving tests, but as a separate PR (would like to do some cleanup and improvements for better coverage/understanding). Will take a look at this tomorrow.

@ahsonkhanahsonkhan added this to the 5.0 milestone Jan 28, 2020
@ahsonkhan

ahsonkhan commented Jan 28, 2020

Copy link
Copy Markdown
Contributor

@maryamariyan - is the bot not able to auto-label PRs with areas or was this a one-off issue?

@ahsonkhanahsonkhan added the enhancement Product code improvement that does NOT require public API changes/additions label Jan 28, 2020
@ahsonkhan

ahsonkhan commented Jan 30, 2020

Copy link
Copy Markdown
Contributor

From #2246 (comment)

I believe NonPublic is necessary to support public getters and non-public setters.

@steveharter, don't we have tests for that case? This PR removed the flag and the existing tests are passing in CI. Either it wasn't needed, or we have a test gap.

@steveharter

steveharter commented Jan 30, 2020

Copy link
Copy Markdown
Contributor

@steveharter, don't we have tests for that case? This PR removed the flag and the existing tests are passing in CI. Either it wasn't needed, or we have a test gap.

Sorry I misspoke. BindingFlags.Public works fine with non-pulic setters.

@layomialayomia mentioned this pull request Feb 2, 2020
Closed
@layomia

layomia commented Feb 2, 2020

Copy link
Copy Markdown
Contributor

Merging this. Next steps from this PR are

These tasks help establish a pattern and expectations for member collision handling which is needed en-route field support.

@YohDeadfall please let us know if you can include these in your plans for tests/coverage over the next couple of days; otherwise I'm happy to get this work started.

@layomia
layomia merged commit 7c38fc9 into dotnet:masterFeb 2, 2020
@YohDeadfall
YohDeadfall deleted the use-pub-props-only branch February 3, 2020 11:38
@YohDeadfall

YohDeadfall commented Feb 3, 2020

Copy link
Copy Markdown
ContributorAuthor

@layomia, started to work on the first already. Since tests cover the issue, I can work on it too.

@YohDeadfallYohDeadfall mentioned this pull request Feb 10, 2020
Closed
@ghostghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ahsonkhanahsonkhanAwaiting requested review from ahsonkhan
1 more reviewer
@layomialayomialayomia approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@YohDeadfallYohDeadfall

Labels

area-System.Text.JsonenhancementProduct code improvement that does NOT require public API changes/additions

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@YohDeadfall@ahsonkhan@steveharter@layomia@jkotas