FusionAuth/fusionauth-netcore-client

Naming Conventions do not match .Net standards

TruDan opened this issue · 10 comments

TruDan commented

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 commented

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.

  • 😄1
robotdan commented

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 commented

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

  • 👍3
robotdan commented

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

EPRenaud commented

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
  • 👍2
robotdan commented

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

Handling via : #27

TruDan commented

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

  • 👍2
  • 🚀3
robotdan commented

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 commented

@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 commented

@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.

  • 👍2