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

Instruct Kotlin serialization to always serialize a third party interface using a specific serializer #2060

Closed
jntakpe opened this issue Oct 13, 2022 · 14 comments
Assignees

Comments

@jntakpe
Copy link

jntakpe commented Oct 13, 2022

What is your use-case and why do you need this feature?

I need to serialize using kotlinx.serialization a third party interface IApiError returned by a Spring controller advice :

@RestControllerAdvice
class SpringAdvice(private val type: SerializerType) {

    @ExceptionHandler(CustomException::class)
    fun handle(exception: CustomException): ResponseEntity<IApiError> {
     return ResponseEntity(IApiError.of(type, exception), 400) // will return an implementation of IApiError either serializable with Jackson or Kotlinx.serialization (annotated 
    }
}

Spring will execute later the following code that will fail because SerializersKt.serializer(type) will return a Polymorphic serializer and when the serializer is polymorphic Spring fails

//class KotlinSerializationJsonEncoder
//type being `IApiError` type 
//value being `KApiError` a Kotlin annotation with `@Serializable`
Json.Default.encodeToString(SerializersKt.serializer(type), value)

Describe the solution you'd like

I would like to have a way to instruct Kotlinx.serialization that everytime the interface IApiError needs to be serialized it should use a specific serializer, in this case the KApiError class serializer.
For example, with Jackson it is possible to do the following :

SimpleAbstractTypeResolver().addMapping(IApiError::class.java, DefaultApiError::class.java)

I've tried declaring the serializer as polymorphic but Spring does not support Open polyphormism.

     serializersModule = SerializersModule {
            polymorphicDefaultSerializer(IApiError::class) {
                KApiError.serializer() as SerializationStrategy<IApiError>
            }
        }

Also I've tried declaring a custom serializer:

@Serializer(forClass = IApiError::class)
object IApiErrorSerializer: KSerializer<IApiError> { /// }

Thanks for your help 🙏

@sandwwraith
Copy link
Member

Unfortunately, there isn't a straightforward way now to override serializer for third-party class globally. I wonder why Spring doesn't support open polymorphism though.

@sdeleuze
Copy link

sdeleuze commented Jan 3, 2023

Spring typical arrangement is to have both kotlinx.serialization and Jackson in the classpath, with kotlinx.serialization converter used first to evaluate if if should be used, if not, Jackson is used. So the behavior of Spring Support is to determine early a signal that shows that kotlinx.serialization should be used.

In our unit tests, you can see what use case we support or not. List with no information on the element type (open polymorphism) won't be serialized by kotlinx.serialization while List<T> will if T is annotated with @Serializable. Open polymorphism is the way to differentiate those use cases.

That's not exactly the ask from this issue to support third party interface, by in spring-projects/spring-framework#28389 we have an issue about serialization of org.springframework.data.domain.Page<T> not working when T is annotated with @Serializable. Since Page<T> extends Iterable<T>, I am wondering if it could be recognize as a collection as List<T> to be supported. Please let me know if I should create a dedicated issue for that.

The ask from this issue could also be helpful in order to allow people to indicate : I know for sure how to serialize this interface with kotlinx.serialization, unless @sandwwraith has a better suggestion.

@sdeleuze
Copy link

@sandwwraith Any thoughts?

@sandwwraith
Copy link
Member

I'm a bit confused because it seems we're discussing a bit different things. kotlinx.serialization indeed does not work well with polymorphism for List<T>, because it would require enumerating all possible Ts beforehand in SerializersModule, and that's not very convenient. However, the original issue was about the non-generic interface IApiError, for which the SerializersModule can easily be provided, and there shouldn't be any problems there.

@sdeleuze
Copy link

Ok I will double check tomorrow that the use case described here works as expected (as you I would expected it does) and if confirmed I will comment asking closing this issue and will create a dedicated one for the need described in #2060 (comment) which is from my POV the real need for Spring use cases.

@meloning
Copy link

meloning commented Jan 29, 2023

Hello, @sdeleuze
After checking the above comments, I found the reason why IterableSerializer is not supported.

Reference comment: #151 (comment)

Personally I think Iterable is a too broad interface to be serializable, because while it can be an usual list (and why don't use List instead), it also can be lazy and/or infinite stream of elements.

So it seems to be implemented as below (Reference):
Screenshot from 2023-01-29 15-47-34
=> After debugging SerializersModule.reflectiveOrContextual(), it is as follows.

[Order of method calls]
SerializersModule.reflectiveOrContextual() ->
Class<T>.constructSerializerForGivenTypeArgs() ->
interfaceSerializer()

=> A PolymorphicSerializer is returned due to the Class<T>.constructSerializerForGivenTypeArgs() implementation.

In order to solve the serialization of org.springframework.data.domain.Page<T> mentioned in the #2060 (comment), I think the Spring module should provide a PageSerializer.

Please check the contents and review this spring-projects/spring-framework/pull/29761 again. 🙏

@sdeleuze
Copy link

sdeleuze commented Jan 30, 2023

@sandwwraith I had a deeper look and I think I am not confortable adding custom logic in Spring to prefer a specific contextual serializer and tend to agree with users ask in #1870 and #2026. I mean, it is not just about Spring, Ktor had to implement custom logic like https://github.com/ktorio/ktor/pull/2865/files.

In the discussion we had, you said

As contextual is meant to be 'fallback on missing default', not 'override the default'

But that's kind of contradict with ContextualSerializer KDoc:

Typical usage of ContextualSerializer would be ... or desire to override serialized class form in one dedicated output format.

And I agree with what the documentation says as it is a common use case.

I understand you don't want to change the existing behavior as it would break use cases, but I am still confused with it.

In #1870 (comment), you said:

Whether we should provide a new function that prioritizes module over built-in is an open question.

If you are really not up to implement this proposal which sounds attractive to me, could we maybe discuss the introduction of such new function to resolve the need described in this issue "Instruct Kotlin serialization to always serialize a third party interface using a specific serializer"?

@sandwwraith
Copy link
Member

Okay, I see — this problem indeed needs some design investigation from our side to determine whether a module-prioritizing function is enough for 'always serialize a third party interface using a specific serializer' or not

@meloning
Copy link

hello. @sandwwraith
I am very curious about the current progress regarding this comment.

could you please share? 🙏

@sandwwraith
Copy link
Member

We've discussed the matter and cam to a conclusion that serializer function indeed should be changed, but only for interfaces: lookup will be first performed in the module (unless there's no explicit @Serializable(with)), and only after, unconditional PolymorphicSerializer will be returned. We'll implement this change in a next major release.

@meloning
Copy link

Thank you for the explanation. is it okay if i understand the meaning of a next major release as version 1.5.0?

@sandwwraith
Copy link
Member

No, because 1.5.0-RC is already released. It'll be 1.6.0

@sdeleuze
Copy link

@sandwwraith Is it available in 1.6.0-RC?

@sandwwraith
Copy link
Member

@sdeleuze Unfortunately not. Given that now serializer resolution have to be also implemented in plugin due to #1348, we'll be able to sync changes in plugin and runtime only in Kotlin 2.0/serialization 1.7

sandwwraith added a commit that referenced this issue Feb 9, 2024
Now, we look up serializers for non-sealed interfaces in SerializersModule first,
and only then unconditionally return PolymorphicSerializer.
This is done because with old behavior, it was impossible to override interface serializer with context, as interfaces were treated as always having PolymorphicSerializer.

This is a behavioral change, although impact should be minimal, as for most usecases (without modules), serializer will be the same and special workarounds are performed so cache would also work as expected.

Note that KClass.serializer() function will no longer return PolymorphicSerializer for interfaces at all and return null instead. It is OK, because this function is marked with @InternalSerializationApi, and we can change its behavior.

Intrinsics in plugin with changed functionality are expected to be merged in 2.0.0-RC.

Fixes #2060
sandwwraith added a commit that referenced this issue Mar 15, 2024
Now, we look up serializers for non-sealed interfaces in SerializersModule first,
and only then unconditionally return PolymorphicSerializer.
This is done because with old behavior, it was impossible to override interface serializer with context, as interfaces were treated as always having PolymorphicSerializer.

This is a behavioral change, although impact should be minimal, as for most usecases (without modules), serializer will be the same and special workarounds are performed so cache would also work as expected.

Note that KClass.serializer() function will no longer return PolymorphicSerializer for interfaces at all and return null instead. It is OK, because this function is marked with @InternalSerializationApi, and we can change its behavior.

Intrinsics in plugin with changed functionality are expected to be merged in 2.0.0-RC.

Fixes #2060
sandwwraith added a commit to JetBrains/kotlin that referenced this issue Mar 18, 2024
…rovided for interface.

This prioritizes module contents over default polymorphic serializer.

See Kotlin/kotlinx.serialization#2060 and Kotlin/kotlinx.serialization#2565
KotlinBuild pushed a commit to JetBrains/kotlin that referenced this issue Mar 18, 2024
…rovided for interface.

This prioritizes module contents over default polymorphic serializer.

See Kotlin/kotlinx.serialization#2060 and Kotlin/kotlinx.serialization#2565
KotlinBuild pushed a commit to JetBrains/kotlin that referenced this issue Mar 21, 2024
…rovided for interface.

This prioritizes module contents over default polymorphic serializer.

See Kotlin/kotlinx.serialization#2060 and Kotlin/kotlinx.serialization#2565
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants