gildor / kotlin-coroutines-retrofit

Kotlin Coroutines await() extension for Retrofit Call
Apache License 2.0
849 stars 64 forks source link

Handling Call<Unit> #22

Closed k-kagurazaka closed 7 years ago

k-kagurazaka commented 7 years ago

Hello, I have a question. Some my APIs return "204: NO CONTENT" so I handle it by Retrofit like:

fun myApi(): Call<Unit>

In this case, myApi().await() throws NPE since the response body is always null. How to solve the problem? Thanks.

gildor commented 7 years ago

You have a few options, one of them to add custom type adapter to your json mapping library that returns Unit instance instead null. I think this is the most correct way to do that. Also you can add retrofit converter factory for that, but if you already use gson/Moshi converter factor y, then better to add custom type adapter. Also you can use awaitResponse() instead

On Sat, Jul 8, 2017, 18:11 Keita Kagurazaka notifications@github.com wrote:

Hello, I have a question. Some my API returns "204: NO CONTENT" so I handle it by Retrofit like:

fun myApi(): Call

In this case, myApi().await() throws NPE since the response body is always null. How to solve the problem? Thanks.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gildor/kotlin-coroutines-retrofit/issues/22, or mute the thread https://github.com/notifications/unsubscribe-auth/AALWoWZHvyJB9CltTcApeMEJFmxiN2XQks5sL1XRgaJpZM4ORsk_ .

k-kagurazaka commented 7 years ago

@gildor Thanks for your quick reply. Your idea looks good. I'll try it.

Thanks again!

LouisCAD commented 7 years ago

Hi @k-kagurazaka, Did you ended up writing some code to handle Call<Unit> with Moshi or something else?

On my side, I first had the following solution:

call.awaitResponse().let {
    if (!it.isSuccessful) throw HttpException(it)
}
LouisCAD commented 7 years ago

Hi @gildor, I wrote a snippet to allow it with a method named awaitSuccess() that returns Unit on Call<Void> methods. Tell me what you think about it and if I should submit a PR.

/**
 * Suspend extension that allows suspend [Call] inside coroutine.
 *
 * @return [Unit] or throw exception
 */
public suspend fun Call<Void>.awaitSuccess() {
    return suspendCancellableCoroutine { continuation ->
        enqueue(object : Callback<Void> {
            override fun onResponse(call: Call<Void>?, response: Response<Void>) {
                if (response.isSuccessful) {
                    continuation.resume(Unit)
                } else {
                    continuation.resumeWithException(HttpException(response))
                }
            }

            override fun onFailure(call: Call<Void>, t: Throwable) {
                // Don't bother with resuming the continuation if it is already cancelled.
                if (continuation.isCancelled) return
                continuation.resumeWithException(t)
            }
        })

        registerOnCompletion(continuation)
    }
}

If anyone want to try it in his project, you'll also need the following method which is private in the library:

private fun Call<*>.registerOnCompletion(continuation: CancellableContinuation<*>) {
    continuation.invokeOnCompletion {
        if (continuation.isCancelled)
            try {
                cancel()
            } catch (ex: Throwable) {
                //Ignore cancel exception
            }
    }
}
k-kagurazaka commented 7 years ago

@LouisCAD I tried to create a Gson TypeAdapter but it doesn't work since Gson skip to pass null response to TypeAdapters. Currently, I write an extension function awaitUnit() almost same as you post above.

JakeWharton commented 7 years ago

Neither of those are the correct solution. See https://github.com/square/retrofit/issues/2329#issuecomment-301316873 to support Unit and then neither the chosen converter nor anything that deals with call adapters has to change.

On Sat, Sep 9, 2017, 10:19 AM Keita Kagurazaka notifications@github.com wrote:

@LouisCAD https://github.com/louiscad I tried to create a Gson TypeAdapter but it doesn't work since Gson skip to pass null response to TypeAdapters. Currently, I write an extension function awaitUnit() almost same as you post above.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/gildor/kotlin-coroutines-retrofit/issues/22#issuecomment-328280104, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEEEcFbcMkZ_HQCzXSn7tjpqelYPRw9ks5sgp5kgaJpZM4ORsk_ .

gildor commented 7 years ago

@LouisCAD I'm against including such ad hoc API to this library, it's not kotlin-coroutines-retrofit specific problem and shouldn't be a part of API. The best solution, of course: add support for Unit to Retrofit, as Jake mentioned.

A solution, based on type adapter maybe not the best, but still, it covers some cases of Gson usage, including the case of Retrofit:

I use this Unit type adapter for Gson.

class UnitAdapter : JsonSerializer<Unit>, JsonDeserializer<Unit> {

    override fun serialize(src: Unit?, typeOfSrc: Type?, context: JsonSerializationContext?): JsonElement = JsonNull.INSTANCE

    override fun deserialize(json: JsonElement?, typeOfT: Type?, context: JsonDeserializationContext?): Unit? = Unit

}
LouisCAD commented 7 years ago

@gildor The solution linked by Jake Wharton (which I thumbed up) works regardless of the converter(s) you use (Gson, Moshi, Protobuf, etc): You add this in a Kotlin file of your choice:

object UnitConverterFactory : Converter.Factory() {
  override fun responseBodyConverter(type: Type, annotations: Array<out Annotation>,
      retrofit: Retrofit): Converter<ResponseBody, *>? {
    return if (type == Unit::class.java) UnitConverter else null
  }

  private object UnitConverter : Converter<ResponseBody, Unit> {
    override fun convert(value: ResponseBody) {
      value.close()
    }
  }
}

Then, you just have to use it when building your Retrofit:

Retrofit.Builder()
        .client(yourClient)
        .baseUrl("yourUrl")
        .addConverterFactory(UnitConverterFactory) // The important line
        .addConverterFactory(/*Moshi, Gson, Protobuf or whatever*/)
        .build()
        .create(YourApi::class.java)!!

BTW, this solution could be linked in the README for others to see how to get Call<Unit> to work

gildor commented 7 years ago

@LouisCAD Yes, of course, I agree that solution, that covers all Retrofit type adapters is better in case of Retrofit.

I just reacted to @k-kagurazaka comment, that it's possible even with null:

I tried to create a Gson TypeAdapter but it doesn't work since Gson skip to pass null response to TypeAdapters.

And also, Gson can be used without Retrofit, so sometimes you still need adapter for Unit

gildor commented 7 years ago

@LouisCAD Agree about Readme, but probably Kotlin section on http://square.github.io/retrofit/ would be a better place for this. @JakeWharton do you have plans to add Kotlin specific docs to Retrofit website?

k-kagurazaka commented 7 years ago

Thanks @gildor . I had the wrong something.