Stable release & move to Retrofit repo

[sandwwraith]

Hi! As far as I understood from #43 the last thing that prevents this lib from being an official converter in the Retrofit repo is that it depends on the experimental parts of kotlinx.serialization API.

We've removed experimentality from StringFormat/BinaryFormat in 1.3.0 release. Am I right that serializer(java.lang.Type) is the only experimental function blocking this?

If so, can you please provide some feedback on it — whether it does the right thing, or some functionality is missing, etc. We're going to stabilize it soon so converters like this can depend on a stable API.

[JakeWharton]

Am I right that serializer(java.lang.Type) is the only experimental function blocking this?

That's right, yes.

I don't recall any feedback or issues off-hand. We're forced to go through the Java APIs due to Retrofit using a Proxy so it can be lossy, but we also have somewhat of an advantage where people tend to declare a concrete class as the type which gets used. At worst they maybe do a List or a Map wrapper.

When the format types became stable (for use, not inheritance) we added @OptIn annotations to suppress that warning. So this serializer API is the only one left.

If you are going to stabilize the API as-is we can move this converter to the Retrofit repo and do an @OptIn to suppress. We've done this in the past with coroutines APIs that were not yet released as stable but were going to be stable with the exact same API and behavior.

[sandwwraith]

We've been revisiting the design, and while the function is likely to stay, I'd like to ask about the problems with nullability:

Because Java's type tokens do not contain this information, serializer(Type) always returns non-nullable serializers for all nested types. In practice, this means the following code:

interface MyApi {
  @Get("foo")
  fun getFoo(): List<String?> // or any other nullable type in any position
}

Would fail with deserialization error on the input like ["a", null]. Have you experienced this problem? How severe is it for you?

We currently have several proposals on what to do with it:

1. Do nothing — if the problem is non-existent in real life
2. Always wrap all serializers to .nullable inside this function. This would make this function unusable for formats that strictly differentiate nullable vs non-nullable: while Json format can deserialize non-null value with nullable deserializer, Protobuf can't.
3. Change signature/provide an additional function with an additional parameter that can alter this behavior. Either serializer(type: Type, wrapIntoNullable: Boolean) or more sophisticated serializer(type: Type, typeArgSerializerFactory: (Type) -> KSerializer<*>) (latter would also probably cover Kotlin/kotlinx.serialization#2025).

[JakeWharton]

It has not been a problem that I have heard of (or seen myself) because again I think people tend to write concrete types for their request and response objects rather than use a parameterized type. The more complex types, such as those which may be a list of potentially-nullable values, occur within the request or response types where the serialization compiler plugin has full access to the metadata to generate the serializer.

Even in the case where it may come up in the future, I think we can tell people to use the surrogate pattern from your docs.

[sandwwraith]

Thanks for the clarifications. I'll just leave everything as is then and prepare serializer(Type) for stabilization in 1.5.

In case this problem reappears later, we can always introduce an additional function as in option 3.

[JakeWharton]

Upstream stabilization PR: Kotlin/kotlinx.serialization#2069

[qwwdfsad]

UPD: PR is merged, the only changes are documentation; we plan to release 1.5.0 somewhere after Kotlin 1.8.0

[JakeWharton]

Thanks! I'm going to release a stable version of this library for simplicity, and then will migrate it to Retrofit for its next release.

[Antimonit]

I want to report that we just ran into this issue.

All network responses from our servers are wrapped in an object containing status of the response (returnCode) based on which we decide whether the response is successful or not. (IMO, this should have based on HTTP 2xx or 4xx codes but this is what we work with now.)

Previously, we used to have dozens of DTOs where the wrapper object was effectively duplicated everywhere, e.g.

data class AuthenticateChallengeResponse(
    val returnCode: String,
    val returnMessage: String,
    val errorDetailMap: Map<String, Any>?,
    val info: Info?,
) {
    data class Info(
        val sessionId: String
    )
}

Instead of duplicating this fixed structure again and again, we've created a generic wrapper:

data class ResponseEnvelope<out T>(
    val returnCode: String,
    val returnMessage: String,
    val errorDetailMap: Map<String, String>?,
    val info: T,
)

Now we can scrap the wrapper:

data class AuthenticateChallenge(
    val sessionId: String,
)

and replace AuthenticateChallengeResponse with ResponseEnvelope<AuthenticateChallenge?>. Or at least in theory.

---

We've ran into issues with nullable types as generic subtypes when using Moshi (square/moshi#1218) and decided to give Kotlin Serialization a try.

Tests looked promising - no issues with nullability, KType and typeOf<>() just works great, supports object and Unit types better, etc.

But today I've noticed that Retrofit basically has the same issue as Moshi - it does not take into account call-site nullability.

    @POST("/v1/auth/challenge")
    suspend fun authenticateChallenge(
        @Body request: AuthenticateChallengeRequest,
    ): ResponseEnvelope<AuthenticateChallenge?>

This fails if the response does not contain the info, e.g.

{
  "returnCode":"123456",
  "returnMessage":"Invalid xxx."
}

would throw

kotlinx.serialization.MissingFieldException: Field 'info' is required for type with serial name 'com.sample.ResponseEnvelope', but it was missing at path: $

This is not on par with how the Kotlin Serialization library works without Retrofit. The following test passes

@Test
fun `Kotlin serialization honors call-site nullability`() {
    val string = """{
        "returnCode":"123456",
        "returnMessage":"Invalid xxx."
    }""".trimIndent()
    
    val json = Json { explicitNulls = false }
    
    assertThat(
        json.decodeFromString<ResponseEnvelope<AuthenticateChallenge?>>(string)
    ).isEqualTo(
        ResponseEnvelope<AuthenticateChallenge?>(
            returnCode = "123456",
            returnMessage = "Invalid xxx.",
            errorDetailMap = null,
            info = null,
        )
    )
}

[JakeWharton]

In this specific case, it looks like you can solve the problem by changing out T to out T : Any and changing info: T to info: T?.

There's little reason to propagate the requirement of nullability into the type parameter for a wrapper object whose property which uses that type parameter is always nullable. By making the type parameter have a non-null lower bound and changing the property declaration site to always be nullable you make the type simpler to write, you force the property to always be nullable, and you work around our limitation of using Java reflection to denote Kotlin types (without kotlin-reflect).

[Antimonit]

I wanted to avoid changing info: T to info: T? because now every consumer of ResponseEnvelope is forced to have nullable info even in cases where this is known to be always there. In essence, I was trying to discriminate between these two cases:

• ResponseEnvelope<AuthenticateChallenge?>
• ResponseEnvelope<String>

From Kotlin's null-safety point of view, for the String version, the non-nullable type was nicely propagated all the way down from Retrofit and we didn't have to use !! anywhere.

---

While trying to blame Retrofit for this issue, I've realized that the structure of our responses is at fault too. We basically have a polymorphic response - we mix success and failure in the same object with no way to differentiate the type during deserialization and we perform it manually only after the response is fully parsed.

I start to have a feeling that even if this issue was addressed, we would still struggle in some cases. I'll just proceed with your suggestion and use !! here and there for now.

[JakeWharton]

If you take your test case above and use it with ResponseEnvelope<String> though it will throw an exception. My understanding is similar to what you said where you basically have an either-or situation meaning info must always be nullable.

You could alternatively model it with a sealed type which avoids needing !! and instead uses the type system to handle checking for success or failure:

sealed interface ResponseEnvelope<out T : Any>
value class ResponseSuccess<out T : Any>(val value: T) : ResponseEnvelope<T>
data class ResponseFailure(code .., details .., map ..) : ResponseEnvelope<Nothing>

[muhrifqii]

been waiting the stable release on this one @JakeWharton

[desgraci]

Any updates on this?

[JakeWharton]

No

[sandwwraith]

We've released an 1.5.0-RC with stable serializer(Type) btw

[JakeWharton]

I saw that, thanks! I'll get to this eventually.

[JakeWharton]

1.0.0 has been released!

It'll probably be a few months before it shows up in the Retrofit repo as a proper first-party offering.

[JakeWharton]

Upstream PR square/retrofit#3950

[JakeWharton]

The move is done, just not the release. I am closing this issue since its work is done. A Retrofit release will happen by the end of the year.

I will leave this project unarchived until then. After the Retrofit release, I expect to archive this project soon thereafter.

Thanks all!