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.

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

New tests that JsonIncludeAttribute and JsonNumberHandlingAttribute are honored during deserialization #47904

Merged
merged 3 commits into from
Mar 3, 2021

Conversation

SkiFoD
Copy link
Contributor

@SkiFoD SkiFoD commented Feb 5, 2021

I added two test for checking the attributes during parameterized ctor deserialization.
ArgumentDeserialization_Honors_JsonNumberHandling test shows that JsonIncludeAttribute is not necessary when the deserialization occurs via ctor mapping even though the properties don't have setters.
This is the only thing that bothers me.

#47855

ArgumentDeserialization_Honors_JsonInclude test is added dotnet#47855
…zation

ArgumentDeserialization_Honors_JsonNumberHandling test is added dotnet#47855
@ghost
Copy link

ghost commented Feb 5, 2021

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

I added two test for checking the attributes during parameterized ctor deserialization.
ArgumentDeserialization_Honors_JsonNumberHandling test shows that JsonIncludeAttribute is not necessary when the deserialization occurs via ctor mapping even though the properties don't have setters.
This is the only thing that bothers me.

#47855

Author: SkiFoD
Assignees: -
Labels:

area-System.Text.Json

Milestone: -

@layomia
Copy link
Contributor

layomia commented Feb 5, 2021

Thanks for the PR @SkiFoD.

ArgumentDeserialization_Honors_JsonNumberHandling test shows that JsonIncludeAttribute is not necessary when the deserialization occurs via ctor mapping even though the properties don't have setters.
This is the only thing that bothers me.

I just realized that with the current implementation, the attribute is indeed not necessary. I updated my comment at #47855 (comment) to reflect this.

@layomia layomia added this to the 6.0.0 milestone Feb 5, 2021
public int X { get; }

[JsonInclude]
public int Y { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

for variety, let this be one that has a private or internal setter.

public class Point_MembersHave_JsonInclude : ITestClass
{
[JsonInclude]
public int X { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Add another property that has a non-public accessor, without the attribute, to show that deserialization works regardless. Then, add a comment that says we should add another test for non-public members when #31511 is implemented.

@SkiFoD
Copy link
Contributor Author

SkiFoD commented Feb 7, 2021

Thank you for your response @layomia. I made a new commit with the fixes.

Base automatically changed from master to main March 1, 2021 09:07
Copy link
Contributor

@layomia layomia left a comment

Choose a reason for hiding this comment

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

Thanks!

@layomia layomia merged commit 83313ab into dotnet:main Mar 3, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants