External/Contextual serializers are not supported

Description

External and Contextual serializers registered via SerialModule are ignored at the moment.

(https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/custom_serializers.md#registering-and-context)

Issue

Seems like serializer is obtained using the serializerByTypeToken(type) method, which is not aware of registered serial modules.

retrofit2-kotlinx-serialization-converter/src/main/java/com/jakewharton/retrofit2/converter/kotlinx/serialization/Factory.kt

Line 23 in d7d7472

val loader = serializerByTypeToken(type)

Not sure if it's feasible to fix it in a reasonably clean manner without breaking changes but as quick check this line makes the test pass:

    val loader = (serializer as FromString).format.context.getContextualOrDefault((type as Class<*>).kotlin)

To Reproduce

Failing Test (following existing deserialize() test):

    data class User(val name: String)

    object UserSerializer : KSerializer<User> {
        override val descriptor = PrimitiveDescriptor("User", STRING)

        override fun deserialize(decoder: Decoder): User =
                UserResponse.serializer().deserialize(decoder).run {
                    User(name = name)
                }

        override fun serialize(encoder: Encoder, value: User): Unit = TODO()

        @Serializable
        private data class UserResponse(val name: String)
    }

    @Before
    fun setUp() {
        val contentType = MediaType.get("application/json; charset=utf-8")
        val retrofit = Retrofit.Builder()
                .baseUrl(server.url("/"))
                .addConverterFactory(Json {
                    serialModule = serializersModuleOf(User::class, UserSerializer)
                }.asConverterFactory(contentType))
                .build()
        service = retrofit.create(Service::class.java)
    }

    @Test
    fun deserialize() {
        server.enqueue(MockResponse().setBody("""{"name":"Bob"}"""))
        val user = service.deserialize().execute().body()!!
        assertEquals(User("Bob"), user)
    }

This seems like a kotlinx.serialization bug. Want to file one?

Your fix breaks cases such as List<User>.

It probably breaks a few other things as well, was more like a quick check if I'm on the right track.

Not sure what to report to kotlinx.serialization tho. serializerByTypeToken() works as advertised for Type and for the all-inclusive case they have extensions like https://github.com/Kotlin/kotlinx.serialization/blob/e8a3bf2ad8fa1d11f3c2fe868f26806ba6e646e9/runtime/commonMain/src/kotlinx/serialization/SerialImplicits.kt#L67 but it works with reified, not sure how to make use of it here.

I would think the library needs a way to support contextual serializers with a Type, not just Class. It's also not clear how you would use contextual serializers with a type that itself has a generic parameter.

I'm not entirely sure that there's even a way to make generics work generally from the kotlinx.serialization side. Maybe some interesting application of JsonParametricSerializer, but even then you could only register that for List::class. I don't see a way around requiring end users to register a contextual serializer for List<User> explicitly.

Yeah it was less a specific question and more meant to emphasize that this library is not the right place to handle these questions. The fact that you want User::class handled differently is not a concern of this library. We just know to ask for whatever serializes User::class from kotlinx.serialization and whether that's built-in, serializable, or backed by a custom serializer doesn't matter. I'm perfectly fine switching the API that the library uses to ask the question about what serializes a Type, but implementing logic for things like contextual serializers or generic types is not the business I want to be in.

Agreed. I think providing an entirely separate contextual serializer-backed converter would probably be the way to go so you actually need to opt-in while also adding the caveat that generic types need to be registered manually in the serialModule.

Maybe deprecating the existing functions that take StringFormat/ByteFormat and replace with a function that takes a JsonConfiguration directly?

The library knows nothing about the chosen serialization format so we cannot use anything tied to a specific format. I'll have to look into how SerialModule is used.

@JakeWharton, I think this issue could be resolved via Kotlin/kotlinx.serialization#803, which should be relatively easy to implement.

So where does this stand now they they went H.A.M. on their codebase and everything has been moved and/or renamed?

Want to send a PR with the fix?

Good question. I should have some time to look into the changes to make sure this feature can be implemented. If so I'll try and get a PR pushed.

Released in 0.7.0