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 #1457

Closed
preardon opened this issue Mar 22, 2021 · 16 comments · Fixed by #1470 or #1471
Closed

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

preardon opened this issue Mar 22, 2021 · 16 comments · Fixed by #1470 or #1471

Comments

@preardon
Copy link
Member

Update of Dependencies

Replace the use of Newtonsoft.Json to use the newer System.Text.Json in Brighter

Performance Gains ( https://devblogs.microsoft.com/dotnet/whats-next-for-system-text-json/ ) I'm aware this link is old, however I believe its only gotten better and know that there have been a few extra features added as part of 5.0

System.Text.Json
https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to?pivots=dotnet-5-0

@penderi
Copy link
Contributor

penderi commented Mar 22, 2021

I hadn't even thogut of this, but I like it!

@preardon
Copy link
Member Author

@penderi you know me, I just see Newtonsoft.Json and want to rip it out and replace it with System.Text.Json

Looks like its only really used for the In Memory Inbox, but if this table is to be believed
image

(and its even an iteration old) then 2x desterilisation and 1.5x serialisation could be worth it for the effort required

@iancooper
Copy link
Member

@preardon it should definitely go on our list, but perhaps post v9 at this point. We do have some older dependencies, such as LibLog and this one that need to go, but I suspect we will only get to LibLog if we want to "call" this release as done. Further reducing the set of our dependencies outside the BCL for the core libraries would also be good. If we end up with just Polly for that, it would be a huge win.

@iancooper
Copy link
Member

That said @preardon feel free to do the work, and we can see when it lands.

@preardon
Copy link
Member Author

@iancooper I'll take a look today from memory you were only using it in one or two places, if it isn't much I'll get a PR in for it

@iancooper
Copy link
Member

@preardon it's in a few places on Brighter, where we want to serialize, such as monitoring, as well as the Outbox/Inbox. You will also find it's used in transports and a lot in the tests. Worth doing, but a bit of a chore. Thanks for picking it up.

@preardon
Copy link
Member Author

@iancooper yeah this went a bit more than I original thought ~111 file PR will be inbound after some extra testing)

On another note, do you have strong feelings about Property Name Casing in Json, I tend to prefer Camel for json and Pascal for Dotnet. I've just run into some tests where I have to make a decision?

@holytshirt
Copy link
Member

I prefer Camel for Json and Pascal for Dotnet too.
Just set it in the JsonSerializerOptions

@preardon
Copy link
Member Author

@holytshirt My exact feels, the issues was the Test was Comparing strings so I could either Change the string to be Camel or the Serialiser to be Pascal, I've Created a static Instance of SerializerOptions so that we can manage them globally

@holytshirt
Copy link
Member

We should probably update the json string compares. FluentAssertions has a nice json addon
https://github.com/fluentassertions/fluentassertions.json

@holytshirt
Copy link
Member

But you don't have to worry about adding that!

@preardon
Copy link
Member Author

@holytshirt @iancooper I take it the Easiest way to run the tests in your CI pipelines is a PR, I cant run the AWSQS ones locally as I don't have the infrastructure setup locally, Also having some wierdness around the SQLLite Inbox and In Memory Inbox, and I'm fairly confident that I haven't touched anything around them?

preardon added a commit to preardon/Brighter that referenced this issue Mar 29, 2021
preardon added a commit to preardon/Brighter that referenced this issue Mar 29, 2021
preardon added a commit to preardon/Brighter that referenced this issue Mar 29, 2021
iancooper added a commit that referenced this issue Mar 29, 2021
Replaced NewtonSoft.Json with System.Text.Json #1457
@preardon
Copy link
Member Author

@iancooper I've just check the tests from that PR, there is 1 in Redis that I think I caused (Json Convert Issue, I'll get another PR in to cover that)

@preardon
Copy link
Member Author

PR #1471 is in to Fix this issues with Redis

@preardon
Copy link
Member Author

@iancooper Did you want me to close this now, or will it be closed as part of the Release of v9

@iancooper
Copy link
Member

Go ahead and close it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants