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

Adding configuration suppoer for TypeNameHandling in OrleansJsonSerializer #3298

Closed
dotanpatrich opened this issue Aug 13, 2017 · 8 comments
Closed
Assignees
Milestone

Comments

@dotanpatrich
Copy link
Contributor

We want to disable TypeNameHandling in when serializing grain state as json in some cases, so that we could refactor the contained types to a different namespace or class name.
However, currently the OrleansJsonSerializer does not offer any configuration override for this setting. One option we considered is writing a new implementation of IExternalSerializer to support this, however this meant that I could not use the JsonConverter that are already present in OrleansJsonSerializer such as GrainIdConverter, IPAddressConverter etc.
Are you OK with me adding to the OrleansJsonSerializer.UpdateSerializerSettings support for setting the TypeNameHandling?
If not, any alternative solution?

@sergeybykov sergeybykov added this to the Triage milestone Aug 15, 2017
@ReubenBond
Copy link
Member

Are you OK with me adding to the OrleansJsonSerializer.UpdateSerializerSettings support for setting the TypeNameHandling?

Sure thing, how would it look? I wonder what the alternative solutions are, eg taking JsonSerializerSettings from DI so that you can provide them.

@dotanpatrich
Copy link
Contributor Author

I thought about editing the OrleansJsonSerializer.UpdateSerializerSettings method so that it would look for a new configuration property that would hold the value for the TypeNameHandling enum value to set on the JsonSerializerSettings. If the configuration value exists it could override the default value set by the OrleansJsonSerializer.GetDefaultSerializerSettings.

As an alternative solution, or actually complementary, might be to expose as public classes the IPAddressConverter, GrainIdConverter and other JsonCoverters that are part of the OrleansJsonSerializer and declared currently as internal classes. This would allow us to implement IExternalSerializer that could make use of those JsonConverters and set the JsonSerializerSettings to what he wants. This implementation of IExternalSerializer can than be injected into the storage providers.

@DixonDs
Copy link
Contributor

DixonDs commented Aug 16, 2017

It seems to be related to #3045

@dotanpatrich
Copy link
Contributor Author

I couldn't understand what was the final outcome of #3045 and it seems to be linked to #895, which aims at supporting external serializer for storage provider. One of the alternatives suggested above was to provide such an external serializer, but I think it would be much simpler to use the existing JsonSerlizer for GrainId etc and are already part of the OrleansJsonSerializer when writing such custom external serializer.

@ReubenBond
Copy link
Member

ReubenBond commented Aug 17, 2017

As an alternative solution, or actually complementary, might be to expose as public classes the IPAddressConverter, GrainIdConverter and other JsonCoverters that are part of the OrleansJsonSerializer and declared currently as internal classes. This would allow us to implement IExternalSerializer that could make use of those JsonConverters and set the JsonSerializerSettings to what he wants. This implementation of IExternalSerializer can than be injected into the storage providers.

Good idea, @dotanpatrich - let's do that. We can keep them as nested classes of OrleansJsonSerializer. Would that be a satisfactory solution for you?

Ultimately, should JsonSerializerSettings be provided via DI to all consumers (storage providers and otherwise)?

@dotanpatrich
Copy link
Contributor Author

Thank you @ReubenBond , added a PR #3398 to make the json serializers within OrleansJsonSerializer public and will follow up next with another PR to make the settings more configurable

@dotanpatrich
Copy link
Contributor Author

Added another PR to enable setting the type name handling in OrleansJsonSerializer according to config - #3400

@ReubenBond
Copy link
Member

Thank you, @dotanpatrich! Both PRs LGTM and have been merged

@ghost ghost locked as resolved and limited conversation to collaborators Sep 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants