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

System.Text.Json: Fix polymorphic state bug when using ReferenceHandler.IgnoreCycles #111808

Merged
merged 5 commits into from
Jan 29, 2025

Conversation

yesmey
Copy link
Contributor

@yesmey yesmey commented Jan 24, 2025

Clear the polymorphic state after ignoring a reference in an array to prevent subsequent objects from inheriting it.

Add a test which triggered an assertion in


and covers the scenario reported in #111790

Fixes #111790

Clear the polymorphic state after ignoring a reference to prevent subsequent object writes from inheriting it.

worker1.Office.Staff = [worker1];

await Test_Serialize_And_SerializeAsync(worker1, """{"Office":{"Staff":[null],"Dummy":{}}}""", s_optionsIgnoreCycles);
Copy link
Member

@eiriktsarpalis eiriktsarpalis Jan 27, 2025

Choose a reason for hiding this comment

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

This looks good. Could you also include a test for the same type using ReferenceHandler.Preserve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IgnoreCycles and Preserve tests are a bit separated, so I just copied the classes over. I could otherwise just add another two lines in this test if youd like that better.

@danmoseley danmoseley requested a review from Copilot January 27, 2025 19:34
@danmoseley
Copy link
Member

[experimenting with copilot reviews]

@eiriktsarpalis
Copy link
Member

@danmoseley Seems it never provided a review?

@eiriktsarpalis eiriktsarpalis requested review from Copilot and removed request for Copilot January 28, 2025 10:54
@eiriktsarpalis
Copy link
Member

Judging by the test failures this type ReferenceHandlerTests_IgnoreCyclesContext_Default is also missing the required types.

Copy link
Member

@eiriktsarpalis eiriktsarpalis left a comment

Choose a reason for hiding this comment

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

Thanks!

@yesmey
Copy link
Contributor Author

yesmey commented Jan 29, 2025

Thanks for your patience. Seems like the remaining test failures are unrelated.

@eiriktsarpalis eiriktsarpalis merged commit bfa0276 into dotnet:main Jan 29, 2025
82 of 85 checks passed
@eiriktsarpalis
Copy link
Member

Thank you for your contribution @yesmey!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Text.Json polymorphism serialized with ReferenceHandler.IgnoreCycles
3 participants