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

Prefer Contextual serializer of default Polymorphic serializer. #2026

Closed
wants to merge 1 commit into from

Conversation

AdlerFleurant
Copy link

According to documentation, contextual are preferred over other serializers. However, this was not the case for interfaces.

Polymorphic Serializer was always returned even if a contextual serializer was defined. This change ensures that contextual serializers actually win over other serializers.

Fixes #1645

Fixes Kotlin#1645

Signed-off-by: Adler Fleurant <[email protected]>
@sandwwraith
Copy link
Member

I'm not sure this is a good idea to change the behavior of a stable function in such a significant way.
Also, documentation for SerializersModule.serializer(type: KType) is correct: Attempts to create a serializer for the given [type] and fallbacks to [contextual][SerializersModule.getContextual] lookup for non-serializable types.. Interfaces are always polymorphically serializable, so there's no contradiction.

However, probably specific exceptions for interfaces should be made, if we take into consideration #1207

@pdvrieze
Copy link
Contributor

pdvrieze commented Sep 7, 2022

Another problem with preferring contextual (in the absense of a @Contextual annotation is that it would be inconsistent with how serializers are statically resolved (by the plugin) as there will not be a context per definition. As to #1207, the main challenge there is runtime lookup of a serializer that is statically attached to an interface.

@AdlerFleurant
Copy link
Author

@sandwwraith and @pdvrieze, I think it makes sense not to prefer contextual serializers. It is still however preferable to use contextual serializers over the default PolymorphicSerializer when a contextual serializer is defined. I will be updating this so that contextual serializers are preferred over the default polymorphicSerializer instead of all serializers.

Would that work?

@AdlerFleurant AdlerFleurant changed the title Prefer Contextual serializer Prefer Contextual serializer of default Polymorphic serializer. Sep 7, 2022
@AdlerFleurant AdlerFleurant marked this pull request as draft September 7, 2022 22:58
@sandwwraith
Copy link
Member

This is still a big semantic change that can break someone's code, unfortunately. I think you can always specify the serializer explicitly in encodeToString if you want.

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.

3 participants