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

Naming Conventions do not match .Net standards #21

Open
TruDan opened this issue Jun 3, 2020 · 10 comments
Open

Naming Conventions do not match .Net standards #21

TruDan opened this issue Jun 3, 2020 · 10 comments

Comments

@TruDan
Copy link

TruDan commented Jun 3, 2020

It's very clear you guys are Java developers. Lowercase properties, reverse-domain namespaces. To a .Net developer, this feels really gross to work with.

My recommendations are:

  • Namespace should be FusionAuth instead of io.fusionauth
  • Public properties should be PascalCase, and not camelCase.
    • In this case, all JSON serialization will be broken, so to fix that the JsonSerializerSettings.ContractResolver must be set to new CamelCasePropertyNamesContractResolver()
  • In DefaultRESTClient, you override the default JsonConvert serializer settings with your own settings. This actually caused us problems because your handling of DateTimeOffset is different to our own API's (and only now by searching the source code, i found why we were experiencing issues). Please instead store your own instance of JsonSerializerSettings with your customizations, and reference this when serializing/deserializing JSON. I tend to make a helper class for this, so you can still keep the "JsonConvert" style. For an example, see here: https://github.com/kennyvv/Alex/blob/master/src/Alex.ResourcePackLib/Json/MCJsonConvert.cs
@mooreds
Copy link
Contributor

mooreds commented Jun 3, 2020

Thanks for your feedback! I can't promise when the engineering team will have time to evaluate and prioritize this work, but I really appreciate the feedback.

@robotdan
Copy link
Member

robotdan commented Jun 4, 2020

It's very clear you guys are Java developers.

Ha.. yes. This is true.

I think we do want to be as idiomatic as we can be in each client library we build to follow the norms and conventions for the language.

My guess is that changing the namespace is a breaking change for anyone compiling against this library?

@TruDan
Copy link
Author

TruDan commented Jun 4, 2020

My guess is that changing the namespace is a breaking change for anyone compiling against this library?

Yes, but as long as all of the classes stay the same name, this shouldn't be too much of a problem. VisualStudio/JetBrains Rider (most used C# IDE's) will suggest "Import FusionAuthClient from X namespace?" then everyone will see that the namespace has changed. It shouldn't cause too much trouble for any developer

@robotdan
Copy link
Member

robotdan commented Jun 5, 2020

Great, thanks @TruDan - really appreciate the feedback and suggestions.

@EPRenaud
Copy link
Contributor

Hi there, I think this issue has two very distinct components.

  1. The naming conventions, although unexpected, do not have adverse effects on the actual usage.
  2. The overridden default JsonConvert serializer settings, on the other hand, generate issues as it interferes with the host application and causes weird bugs

@robotdan
Copy link
Member

Thanks @EPRenaud re: the second component -yes, this is something we should fix for sure.

Handling via : #27

robotdan added a commit that referenced this issue Oct 21, 2020
Don't overwrite default serializer settings (#21)
@TruDan
Copy link
Author

TruDan commented Oct 21, 2020

It's worth noting here, I looked deep into this issue today, newtonsoft always uses the settings from DefaultSettings factory method, and even if you pass jsonSerializerSettings as the 2nd parameter however your passed on settings will be applied ontop of the default

Tl:;Dr; all this issue does is reverse the control of the root settings to the developer and not the fusionauth library. So it is possible that settings the developer overrides on DefaultSettings will break json serialisation in fusionauth.

To prevent that happening the key settings must be defined. I might try to make a PR for this, I can explain better in code :) C# is my native language, English 2nd

@robotdan
Copy link
Member

Thanks for that great detail @TruDan - I merged #27.

So it is possible that settings the developer overrides on DefaultSettings will break json serialisation in fusionauth.

This is not ideal I suppose, but this is probably better than the other way around. At least we can blame the developer for breaking us, instead of us breaking the developer.

To prevent that happening the key settings must be defined. I might try to make a PR for this, I can explain better in code :) C# is my native language, English 2nd

This would be great, thanks for your contribution!

@epbensimpson
Copy link
Contributor

epbensimpson commented Oct 21, 2020

@TruDan I think I see what you're saying, it's a bit annoying how that works :( Whether it's an issue may depend on whether those settings are null by default on JsonSerializerSettings or not. If they're not null, it shouldn't matter because they will be applied over the top of those generated by the DefaultSettings delegate, and the specified converters take precedence.

Edit: Confirmed that any non-specified settings on JsonSerializerSettings don't get applied over the top of the settings returned by the delegate as they're null by default, so it will probably be a good idea to specify every setting that the FA-client expects when creating the JsonSerializerSettings instance. I'm not sure if/when I'll be able to make that change, but at least the #27 change will prevent issues in consumers for now!

@TruDan
Copy link
Author

TruDan commented Oct 22, 2020

@TruDan I think I see what you're saying, it's a bit annoying how that works :( Whether it's an issue may depend on whether those settings are null by default on JsonSerializerSettings or not. If they're not null, it shouldn't matter because they will be applied over the top of those generated by the DefaultSettings delegate, and the specified converters take precedence.

Edit: Confirmed that any non-specified settings on JsonSerializerSettings don't get applied over the top of the settings returned by the delegate as they're null by default, so it will probably be a good idea to specify every setting that the FA-client expects when creating the JsonSerializerSettings instance. I'm not sure if/when I'll be able to make that change, but at least the #27 change will prevent issues in consumers for now!

I heard from a collegue today that actually if you don't use JsonConvert but instead use the JsonTextReader with an instance of JsonSerializer etc then it will not use those defaults, but only the settings you defined.

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

No branches or pull requests

5 participants