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

Mitigate the usage of JsonConvert.DefaultSettings in user code #4581

Merged
merged 21 commits into from Aug 10, 2023

Conversation

Applesauce314
Copy link
Contributor

@Applesauce314 Applesauce314 commented Jun 28, 2023

Description

!1881 removed direct usage of JsonConvert.DefaultSettings but the serialization was still sensitive to user code changing this value due to the use of JsonConvert.Serialize and JsonConvert.Deserialize during fast serialization/deserialization

Related issue

Fixes #1360

apmoskevitz added 3 commits June 28, 2023 09:43
…lue as possible. this helps mitigate if user code sets JsonConvert.DefaultSettings
@Applesauce314
Copy link
Contributor Author

note that the contributing guideline ask that there be an issue discussed and approved before a pull request is created, #1360 is closed, but the last comment is a solicitation for a contribution to fix the problem, if you need an open issue, let me know and I can create one.

@nohwnd
Copy link
Member

nohwnd commented Jun 28, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Applesauce314
Copy link
Contributor Author

@microsoft-github-policy-service agree company="General Dynamics"

@Applesauce314
Copy link
Contributor Author

is there anything I need to do on that test failure? it appears that that particular test often fails with that error.

@nohwnd
Copy link
Member

nohwnd commented Jul 10, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@nohwnd
Copy link
Member

nohwnd commented Jul 11, 2023

Nice, the tests finally passed, I will have to have a proper look at that code. Everything is relying on this.

It would also be nice to have an acceptance test that helps us not regress this. Would you be able to add one?

@Applesauce314
Copy link
Contributor Author

Nice, the tests finally passed, I will have to have a proper look at that code. Everything is relying on this.

It would also be nice to have an acceptance test that helps us not regress this. Would you be able to add one?

I would like to, however my system is not longer able to build successfully (we updated vs recently, maybe that), so not sure when i can get to it, don't have time to debug.

@Applesauce314
Copy link
Contributor Author

i figured out how codespaces worked, so no more using the CI to check my tests (sorry about that).
i added a test, but have not yet verified that it fails in a branch prior to my build

@Applesauce314
Copy link
Contributor Author

I verified that the tests I added fail in main prior to my changes.

@nohwnd
Copy link
Member

nohwnd commented Aug 4, 2023

I still did not get to this, sorry :/

@nohwnd nohwnd added the sprint label Aug 7, 2023
@nohwnd nohwnd merged commit 108a37c into microsoft:main Aug 10, 2023
7 checks passed
@nohwnd
Copy link
Member

nohwnd commented Aug 10, 2023

I think it is safe to merge, thank you!

@Applesauce314 Applesauce314 deleted the issue-1360 branch August 14, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests never finish when JsonConvert DefaultSettings changed
2 participants