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

Replace Newtonsoft.Json with System.Text.Json #206

Closed
aaronpowell opened this issue Feb 11, 2022 · 8 comments
Closed

Replace Newtonsoft.Json with System.Text.Json #206

aaronpowell opened this issue Feb 11, 2022 · 8 comments
Assignees
Labels

Comments

@aaronpowell
Copy link
Contributor

With it built into the .NET framework, there's probably no need to rely on an external package for serialisation/deserialisation of the JSON is there?

@Aniruddh25
Copy link
Contributor

@mbhaskar Could you please sync with Aaron about your need for using Newtonsoft.Json See this opposite issue #60

@VinnieKhanna
Copy link
Contributor

I'd like to make the case against entirely relying on System.Text.Json. While working on stored procedure work, I came across the need to support more customized deserialization from strings to enums. Json converters like Kebab casing and workarounds like specifying with the EnumMember attribute aren't natively supported by .NET.

This issue describes what I'm referring to in detail.

From the developers working on this issue: dotnet/runtime#29975 (comment)

Let me be absolutely clear, providing built-in support for everything is an explicit anti-goal for System.Text.Json. Instead, we aim to provide good extensibility points so that third-party extensions like the one you cited can be successful.

Others echo this sentiment throughout the issue. Point being - I don't think we should remove Newtonsoft if it is serving a valid purpose in our code, as System.Text.Json isn't necessarily meant to be an all-encompassing solution, it seems.

@aaronburtle
Copy link
Contributor

aaronburtle commented Jul 14, 2022

There might be some use cases where changing over doesn't really get us much, if anything, and STJ is missing features which would make doing so overly burdensome. However, the performance differences between the two are large enough that even if using STJ in a particular case is difficult to do, we should aspire to do so anyway. And in particular for those areas where perf testing shows that we get a significant speed up in runtime, or significant drop in memory allocation by using STJ, in my opinion, we should do whatever we can to make that work.

@ayush3797
Copy link
Contributor

@Aniruddh25, does this cover Cosmos as well?

@ayush3797
Copy link
Contributor

ayush3797 commented Feb 6, 2023

Did a bit of research/trial, came across some articles which suggest System.Text.Json might not be very ready to replace for every feature that Newtonsoft provides. One of them is on the json object comparison front. There has been a github issue filed for the same here as well: dotnet/runtime#33388 and another one here: dotnet/runtime#56592

@ayush3797
Copy link
Contributor

ayush3797 commented Feb 6, 2023

While I see substitutes of JObject in Newtonsoft like JsonNode in System.Text.Json.Nodes namespace, there is no mention of how we can handle comparison here in this article either which talks about migration from Newtonsoft to System.text.json: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft?pivots=dotnet-6-0.

@ayush3797
Copy link
Contributor

ayush3797 commented Feb 7, 2023

As indicated by the articles linked above, there is no out of the box mechanism provided by system.text.json library as of now to compare two json objects semantically. However, issues are raised already on github to provide such features. We can write our own custom comparator though, but for now, as per discussion with @Aniruddh25, we have decided to keep things as is.

I see Newtonsoft mostly being used in testing only apart from CosmosMutationEngine.cs and CosmosQueryEngine.cs classes. So we need to see what we want to do with those classes @tarazou9 @mbhaskar.

@Aniruddh25
Copy link
Contributor

Duplicate of #60

@Aniruddh25 Aniruddh25 marked this as a duplicate of #60 Feb 18, 2023
@Aniruddh25 Aniruddh25 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants