Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[System.Text.Json] Fix bug complex properties#37586

Closed
MarcoRossignoli wants to merge 3 commits intodotnet:masterfrom
MarcoRossignoli:jsonbug
Closed

[System.Text.Json] Fix bug complex properties#37586
MarcoRossignoli wants to merge 3 commits intodotnet:masterfrom
MarcoRossignoli:jsonbug

Conversation

@MarcoRossignoli
Copy link
Copy Markdown
Member

@MarcoRossignoli MarcoRossignoli commented May 10, 2019

@MarcoRossignoli MarcoRossignoli changed the title fix bug complex properties Fix bug complex properties May 10, 2019
@MarcoRossignoli MarcoRossignoli changed the title Fix bug complex properties [System.Text.Json] Fix bug complex properties May 10, 2019
@MarcoRossignoli
Copy link
Copy Markdown
Member Author


if (!propertyInfo.ShouldDeserialize)
{
return false;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If I understood well we should return true only if we're sure to be at the end of json object root

@ahsonkhan ahsonkhan added this to the 3.0 milestone May 11, 2019
@ahsonkhan ahsonkhan requested a review from steveharter May 11, 2019 00:04
@ahsonkhan
Copy link
Copy Markdown

fixes #37567

Suggest me more test case if needed.

I would add a test with properties that have private setters like the one that was stated in the original issue (both for top-level properties, and for complex properties/nested).

@MarcoRossignoli
Copy link
Copy Markdown
Member Author

Close because I confirm that update #37578 fix bug my tests passed. cc: @steveharter @ahsonkhan

@MarcoRossignoli MarcoRossignoli deleted the jsonbug branch May 11, 2019 09:52
@ahsonkhan
Copy link
Copy Markdown

Close because I confirm that update #37578 fix bug my tests passed.

Both of them? #37567 as well?

@steveharter consider adding the test cases from @MarcoRossignoli from this PR into yours.

@MarcoRossignoli
Copy link
Copy Markdown
Member Author

Both of them? #37567 as well?

I think so I did some test with @steveharter clone and sample on #37567 works

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

2 participants