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.

Sign up for GitHub

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

Jump to bottom

Fix broken SerializationSetup, bindings insertion was reversed between HOCON and SerializationSetup. #4807

Merged
merged 2 commits into from
Mar 2, 2021
Merged

Fix broken SerializationSetup, bindings insertion was reversed between HOCON and SerializationSetup. #4807

merged 2 commits into from
Mar 2, 2021

Conversation

Arkatufus
Copy link
Contributor

Closes #4761

@Arkatufus Arkatufus mentioned this pull request Mar 2, 2021
Closed
@Aaronontheweb
Copy link
Member

And this is done because the fallback serialization bindings in HOCON will always override the user-defined ones in the SerializationSetup?

@Arkatufus
Copy link
Contributor Author

Arkatufus commented Mar 2, 2021 •
edited
Loading

The way it is set up before, both SerializationSetup alias mapping and type binding mapping were done just before we process the HOCON bindings, hence why the HOCON bindings always win (the System.Object binding in the original failed case).

I just split these mappings to after each of the respective HOCON mapping so that anything that came from SerializationSetup overrides HOCON.

This is how scala does it:

val result = fromConfig ++ serializerDetails.map(d => d.alias -> d.serializer)

serializerDetails always win.

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) March 2, 2021 18:17
Aaronontheweb
Aaronontheweb approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aaronontheweb Aaronontheweb merged commit 627cc57 into akkadotnet:dev Mar 2, 2021
@Arkatufus Arkatufus deleted the #4761_Fix_SerializationSetup branch March 8, 2021 13:27
@mchandschuh
Copy link
Contributor

Awesome work guys, thanks for proving I wasn't just crazy 😛

mchandschuh
mchandschuh reviewed Mar 11, 2021
View reviewed changes
src/core/Akka/Serialization/Serialization.cs

// Add any serializer bindings that are registered via the SerializationSetup
// This has to be done here because SerializationSetup ALWAYS win.
foreach (var details in _serializerDetails)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep -- I suspected it was an ordering issue -- applying the defaults after apply the user defined settings. easy peasy!

@mchandschuh
Copy link
Contributor

@Aaronontheweb -- do we know what version this fix will be released in? I'll be able to remove my work around once the new packages are live. Thank you!

@Aaronontheweb
Copy link
Member

We just released Akka.NET v1.4.17 yesterday - this should be included in that release. @mchandschuh

@mchandschuh
Copy link
Contributor

yep -- just saw the twitter announcement -- will follow for future updates, thanks @Aaronontheweb

@Arkatufus Arkatufus restored the #4761_Fix_SerializationSetup branch April 22, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchandschuh mchandschuh mchandschuh left review comments

@Aaronontheweb Aaronontheweb Aaronontheweb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overriding Default Serializer for typeof(object) programmatically
3 participants
@Arkatufus @Aaronontheweb @mchandschuh