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

Kotlinx serialization favors sealed base serializer even when explicitly using subclass #310

Open
trema96 opened this issue Dec 10, 2021 · 5 comments

Comments

@trema96
Copy link

trema96 commented Dec 10, 2021

Problem description

When using kotlinx serialization if a type implements a sealed interface kmongo will try to serialize it using the interface serializer even if the compile-time type of the object is an implementation of the aforementioned interface.

For example if i have the following types

sealed interface Foo {
    @Serializable
    data class FooA(val x: Int) : Foo
    @Serializable
    data class FooB(val x: Double) : Foo
    @Serializable
    data class FooC(val z: String) : Foo
}

and i attempt to insert an instance of FooA in a collection of only FooAs

client.getDatabase("testDb").getCollection<FooA>("fooAs").insertOne(FooA(1))

i get the error

Exception in thread "main" kotlinx.serialization.SerializationException: Class 'FooA' is not registered for polymorphic serialization in the scope of 'Foo'.

Note that this does not happen if Foo is not sealed, or if FooA is not the top-level document, as in the following example

@Serializable
data class FooContainer(val foo: FooA)

client.getDatabase("testDb").getCollection<FooContainer>("fooContainers").insertOne(FooContainer(FooA(1))) //Ok

Cause

The issue seems to be in the implementation of KMongoSerializationRepository.getSerializer which is used to get the serializer when inserting documents and favors the superclass serializer.

Workaround

Explicitly specifying the serializer of the various Foo implementations like in the following example solves the issue

registerSerializer(FooA.serializer())
registerSerializer(FooB.serializer())
registerSerializer(FooC.serializer())

Adding a module that specifies contextual serializers for the implementations of Foo also works.

@zigzago zigzago added the bug label Dec 19, 2021
@zigzago zigzago changed the title Kotlinx serialization favors sealed base serializer even when explicitly using subclass Sealed interface not serializable without custom polymorphic module May 13, 2022
@zigzago
Copy link
Member

zigzago commented May 13, 2022

Since Kotlin/kotlinx.serialization#1576 fix you can use the @Serializable annotation on sealed interface. But there is still an issue when accessing serializer from KClass (Kotlin/kotlinx.serialization#1869).

Workaround for now: use sealed class or define custom PolymorphicSerializer

@trema96
Copy link
Author

trema96 commented May 13, 2022

Sorry, I did not explain myself well.

The issue is that I would expect FooA : Foo to be serialized in two different ways when it is added in a collection of Foo vs a collection of FooA: in a collection of Foo it must include the type information, while in a collection of FooA it should not.
For example in a collection of various Foo I want something like this

[
  {
      "_id": { "$oid": "627e215005eed8558ea6f3c3" },
      "___type": "Foo.FooA",
      "x": 1
  },
  {
      "_id": { "$oid": "627e215005eed8558ea6f3c5" },
      "___type": "Foo.FooC",
      "z": "a"
  }
]

and in a collection of only FooA I want

[
  {
      "_id": { "$oid": "627e215005eed8558ea6f3c3" },
      "x": 1
  },
  {
      "_id": { "$oid": "627e215005eed8558ea6f3c5" },
      "x": 2
  }
]

I would say that a situation where I want both of these cases is unlikely, but in my case I wanted to have a separate collection for each of FooA, FooB and FooC, yet they needed to implement a sealed interface for other reasons.

By allowing the serialization of Foo, using only your suggested workarounds (sealed class or polymorphic serializer) I also get the superfluous type information in the collection of only FooA ("___type": "Foo.FooA"). Using registerSerializer as I suggested, instead, this field is omitted. You can combine both of these workarounds to obtain the desired behavior with collections of Foo and FooA.

As I previously mentioned I think the problem is that the implementation of KMongoSerializationRepository.getSerializer always uses the serializer of the superclass of T if it is sealed. What I think should happen is that if I'm serializing an instance of FooA in a collection of Foos it should use the serializer for Foo, while if I'm serializing it in a collection of FooAs it should use the serializer of FooA.

@zigzago
Copy link
Member

zigzago commented May 13, 2022

Thank you for the explanation.

Unfortunately the change would not be backward compatible. It would be also problematic for serialization without collection context:

FooA(1).bson // -> do we use Foo or FooA serializer?

May be it would be also counter-intuitive:

database.getCollection<FooA>("foo").insert(FooA(2))
database.getCollection<Foo>("foo").find() // -> fail at runtime

database.getCollection<Foo>("foo2").insert(FooA(1))
database.getCollection<FooB>("foo2").find() // -> fail at runtime

Though I agree with your remarks, the fix is not simple - it would need backward compatible flag & custom serializer for bson/json serialization - I think we are going to stay with your workaround :(

@trema96
Copy link
Author

trema96 commented May 13, 2022

I imagined it would not be simple to change this behavior, I mostly opened this issue in hope it would help if someone else incurs in the same problem. In case should we change the title back to something that better summarize the issue? In general the problem is with any sealed supertype (class or interface).

Anyway thank you for this great tool!

@zigzago zigzago changed the title Sealed interface not serializable without custom polymorphic module Kotlinx serialization favors sealed base serializer even when explicitly using subclass May 13, 2022
@zigzago
Copy link
Member

zigzago commented May 13, 2022

Than you for reporting the issue !

zigzago added a commit that referenced this issue May 14, 2022
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

2 participants