consuldotnet

consuldotnet copied to clipboard

Hello,

I am using your library to register my services for my API gateway without issues, but I noticed that it still has a reference to Newtonsoft.Json as the only library I use. Would it be possible to switch to System.Text.Json serializers?

Regards

Hi @Syzuna,

Thanks for the interest.

Would it be possible to switch to System.Text.Json serializers?

I guess so. https://www.nuget.org/packages/System.Text.Json/ supports both of the targets we ship (net461 and netstandard2.0).

Would you like to contribute a PR?

I can try to clear some time to look into it the next days

Thanks @Syzuna, looking forward to it!

This might help you in your endeavour: https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to

@highlyunavailable I imagine that Newtonsoft.Json was chosen because System.Text.Json did not exist back then? Do you foresee any potential issues preventing this migration? Thanks!

I imagine that Newtonsoft.Json was chosen because System.Text.Json did not exist back then? Do you foresee any potential issues preventing this migration? Thanks!

Yep, that's correct, and that's why I went to the effort to ILMerge Newtonsoft.Json into the DLL so that it didn't actually have a dependency. I'm not sure if I'm using any of the banned features (e.g. serializing fields, private setters, etc.) but it might be worth swapping them over and seeing how many tests fail! It's mostly just an unknown at this point. If it could be fairly easily switched to System.Text.Json that'd be a huge win.

Are there any contribution guidelines I should read? Like which branch should I use as base and so on?

Thanks for your insight, @highlyunavailable! It's great to know that you're still around to help us 😄

@Syzuna we still need to write contribution guidelines, but here is the gist of it:

• use master as the base
• try to do "atomic" changes in a PR, as it makes it easier to review them (i.e. only switch to System.Text.Json — if anything else would be needed as a prerequisite, try to make those changes in a separate PR and mention it here)
• make sure the tests pass (and are updated if new features/behaviour is introduced) — the CI should run automatically when you push to your branch in your fork, and will run again once you create a PR here
• update CHANGELOG.md with a description of your changes
• try to use the same style as the rest of the project (we intend to introduce .editorconfig and fix the few inconsistencies that exist today in the future)

Specifically for this change, you will also need to update the CI (.github/workflows/ci.yml) to remove the "Repack" step, as it won't be needed anymore once the dependency to Newtonsoft.Json is dropped.

Thanks again for looking into this, it's much appreciated!

The biggest blocker is going to be the converters. It's likely not hard to port them, but make sure you actually need to! For example, the KVPairConverter was needed because Newtonsoft.Json was failing to parse very large ulongs and couldn't properly parse base64 values. If System.Text.Json works better for this, then we might not need it. I'm also using regex for the Go Duration parsing which is less than ideal but it was very much a "I had the tool at the time so I used it".

Sorry for bothering you @jgiannuzzi Are there any tricks to get the Actions to run in the fork? It just does not show up in the Actions tab and also does not run on push.

Edit: Nvm the Action is working now

@jgiannuzzi @highlyunavailable

Sorry for bothering again but I kinda hit a big issue I wanted to discuss. System.Text.Json does not rly seem to like non public classes and properties like used in the Keyring Requests e.g. Is there any reason I cannot make then public or do you have any other ideas how to play around that shortcoming/design choice of MS?

Edit: Also apparently using dynamic is a big red flag too...

The private "*Request" classes are just there to construct the right object "shape" for the request. Could an internal class work or does it need to be public? It doesn't MATTER technically if it's public, but it's just API clutter if it is. Custom converter could be your answer here, just to get rid of the intermediate object.

All the Dynamic uses are basically just trying to expose JObjects. I honestly don't know why anyone would use the Raw endpoint in the first place but it was in the API so I wrote it, but you could probably make a custom converter for this endpoint that just turns the JSON into dynamics of the right JSON type.

I tried internal classes but it wont accept them... I can make custom converters for them as well and just make them internal but I dunno if that is worth the time that would consume instead of making the classes public with an internal setter which is fine (dont ask me why).

And yeah I wont get around making custom converters for the dynamic part I guess. But with currently having the Request classes public I am at least down to only 7 failed tests... was a long way

With the 5.0 version System.Text.Json supports private and internal property setters and getters via the [JsonInclude] attribute. https://docs.microsoft.com/en-us/dotnet/standard/serialization/system-text-json-migrate-from-newtonsoft-how-to?pivots=dotnet-5-0#non-public-property-setters-and-getters I see the serialization is feature rich with this version.

EDIT: System.Text.Json works with .NETStandard 2.0.

@Syzuna What is your progress? Where is the branch you are working on?

@Syzuna What is your progress? Where is the branch you are working on?

https://github.com/Syzuna/consuldotnet/commits/master

@wxtsxt My progress is kinda stuck at the moment, as I got distracted by bugs that sometimes are not easily figured out in libraries I am a team member for and personal stuff.

Wanted to finish this a while ago... So if you want to take over or sth I dont mind.