fusionauth-netcore-client icon indicating copy to clipboard operation
fusionauth-netcore-client copied to clipboard

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

Open robotdan opened this issue 5 years ago • 7 comments

Description

See this comment https://github.com/FusionAuth/fusionauth-netcore-client/issues/13#issuecomment-608204114

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.

robotdan avatar Apr 03 '20 14:04 robotdan

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.

tyduptyler13 avatar Apr 14 '20 20:04 tyduptyler13

Thanks for that insight @tyduptyler13 very helpful.

robotdan avatar Apr 14 '20 22:04 robotdan

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/

lukevp avatar May 26 '20 01:05 lukevp

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

katherine-teng-alida avatar Jan 27 '24 00:01 katherine-teng-alida

@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 avatar Feb 02 '24 21:02 mark-robustelli

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.

mark-robustelli avatar Feb 13 '24 23:02 mark-robustelli

thanks so much @mark-robustelli.

katherine-teng-alida avatar Feb 14 '24 18:02 katherine-teng-alida