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

Don't overwrite default serializer settings (#21) #27

Merged
merged 1 commit into from
Oct 21, 2020

Conversation

epbensimpson
Copy link
Contributor

Fixes an issue described in #21 where DefaultRESTClient is setting JsonConvert.DefaultSettings instead of supplying the settings directly to the SerializeObject and DeserializeObject calls.

@TruDan
Copy link

TruDan commented Oct 21, 2020

@robotdan This would also fix alot of issues we're currently having in our UserApi uSvc. as it cannot deserialize any object with a datetimeoffset that doesn't come from fusionauth, since we don't use unix timestamps anywhere.

Could this be merged? It's a very small change with no effect to the usage of the client library. But with a very big positive impact for anyone using the library. Newtonsoft.Json is the most popular json library for C#. Very widely used.

@mooreds
Copy link
Contributor

mooreds commented Oct 21, 2020

We already use Newtonsoft.Json in some of the other parts of this lib. I'm fine with that, but my question is does this require changes to fusionauth-client-builder? I don't believe so, based on this code search: https://github.com/FusionAuth/fusionauth-client-builder/search?q=DefaultRESTClient

@epbensimpson
Copy link
Contributor Author

I'm not super familiar with how these clients are generated from the templates but I did have a quick look and it didn't seem like DefaultRESTClient was part of that.

@robotdan
Copy link
Member

robotdan commented Oct 21, 2020

Thanks for the follow up @TruDan and @epbensimpson - we really appreciate your expertise since we (or at least I am) .Net dumb.

If I understand correctly, the serialization settings we are configuring currently are changing the global default configuration for Newtonsoft so it affects any others users of this library in the same runtime?

If that is the case, this would be a great change to make. If you can confirm the above, I am up for merging this PR.

@epbensimpson is correct - this change is only in this repo, the template stuff in the client builder just uses this base rest client code.

For reference, here are the related templates:
https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/client/netcore.client.ftl
https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/client/netcore.client.interface.ftl
https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/client/netcore.client.sync.ftl
https://github.com/FusionAuth/fusionauth-client-builder/blob/master/src/main/client/netcore.domain.ftl

@robotdan
Copy link
Member

I'll merge this - if anyone wants to confirm my comments here #27 (comment) that would be great! (just for my own knowledge)

@robotdan robotdan merged commit f2d6487 into FusionAuth:master Oct 21, 2020
@epbensimpson
Copy link
Contributor Author

If I understand correctly, the serialization settings we are configuring currently are changing the global default configuration for Newtonsoft so it affects any others users of this library in the same runtime?

Correct - I discovered this issue as we had code that was relying on enums being serialized as the integer value, not as the string representation, and this broke suddenly but I couldn't find anywhere in our code we were adding the StringEnumConverter to the default settings. Eventually I tracked it down to this library based on the IdentityProviderConverter that was being added as well.

@robotdan
Copy link
Member

Thanks for the info @epbensimpson much appreciated.

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

Successfully merging this pull request may close these issues.

4 participants