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

Remove dep on NewtonSoft JSON and use System.Text.Json instead #16

Open
robotdan opened this issue Apr 3, 2020 · 7 comments
Open

Remove dep on NewtonSoft JSON and use System.Text.Json instead #16

robotdan opened this issue Apr 3, 2020 · 7 comments
Assignees
Labels
enhancement New feature or request externally blocked

Comments

@robotdan
Copy link
Member

robotdan commented Apr 3, 2020

Description

See this comment #13 (comment)

Thanks to @ukevp for this suggestion:

Another standardization thing you might consider is your current use of newtonsoft.json. The history of that library is pretty interesting. It's one of the top libraries in the .net ecosystem. The original developer, James King, wrote the library independently, but later was hired by Microsoft. He works on the team that built System.Text.Json, which is the official successor, included in ASP.NET Core, and is faster than the old library. So you may consider taking a dependency on that library instead, it is compatible with all the versions that this library is targeting as well. I'm not sure what effort it would be to migrate to it though so that would be up to you if it was worth the effort to do that now.

Here's the library: https://www.nuget.org/packages/System.Text.Json

I think this is a nice-to-have, and I don't know the level of effort to do this - we have some custom configuration currently that we'd have to re-work into the new method.

@tyduptyler13 tyduptyler13 self-assigned this Apr 14, 2020
@tyduptyler13
Copy link
Contributor

This is something I was intending to include from the beginning but is blocked by several missing features we are taking advantage of in Newtonsofts version.

  1. Custom enum names This is a tough one but we could probably fix it ourselves or through yet another library (but that kind of defeats the purpose of moving to the built in library)
  2. Polymorphic deserialization (Don't have an issue for this one but its mentioned everywhere in their docs that references Newtonsoft) This one is a hard stop unless we write a converter that is smart enough to get the type field and return the right type. Based on their docs I believe this would also require a change to our json serialization in FusionAuth because it wants the type discriminator to come as the first property otherwise it blows up, otherwise I haven't found a way to temporarily consume the json object to a dictionary or something to to some peeking.

Based on my attempt to implement it I would say this is something to look into whenever they release version 5 of System.Text.Json, otherwise this is a blocked issue.

@robotdan
Copy link
Member Author

Thanks for that insight @tyduptyler13 very helpful.

@lukevp
Copy link
Contributor

lukevp commented May 26, 2020

For #1 looks like 5.0 will support enum serialization but that would require retargeting this lib, which would reduce the # of possible users, so better not to do it.
For #2, How about the new source generators announced a few weeks ago? Perhaps this could be useful? You could emit the code needed to do the analysis automatically.
https://devblogs.microsoft.com/dotnet/introducing-c-source-generators/

@katherine-teng-alida
Copy link

Is it possible to bump to Newtonsoft.Json v13 while we wait so we can eliminate the version 12 vulnerability?
https://www.ibm.com/support/pages/security-bulletin-vulnerability-newtonsoftjson-120122727dll-has-afftected-net-agent-0

@mark-robustelli
Copy link

@katherine-teng-alida On the surface that seems reasonable. We are currently evaluating other possible changes to some of the client libraries and the way they are created. I should have some time next week to dig into this a little more and see what would be involved in updating the version of Newtonsoft.Json in the mean time.

@mark-robustelli
Copy link

I wanted to give an update. I am running into some problems with the tests after the the upgrade (some problems existed before the newtonsoft update). I am trying to work through them now.

@katherine-teng-alida
Copy link

thanks so much @mark-robustelli.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request externally blocked
Projects
None yet
Development

No branches or pull requests

5 participants