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

Add support for Kotlin's Result #3873

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ androidxTestRunner = { module = "androidx.test:runner", version = "1.4.0" }
rxjava = { module = "io.reactivex:rxjava", version = "1.3.8" }
rxjava2 = { module = "io.reactivex.rxjava2:rxjava", version = "2.2.21" }
rxjava3 = { module = "io.reactivex.rxjava3:rxjava", version = "3.1.6" }
rxjavaAdapter = { module = "com.squareup.retrofit2:adapter-rxjava2", version = "2.9.0"}
rxjavaAdapter2 = { module = "com.squareup.retrofit2:converter-gson", version = "2.9.0"}
reactiveStreams = { module = "org.reactivestreams:reactive-streams", version = "1.0.4" }
scalaLibrary = { module = "org.scala-lang:scala-library", version = "2.13.10" }
gson = { module = "com.google.code.gson:gson", version = "2.10.1" }
Expand Down
2 changes: 2 additions & 0 deletions retrofit/kotlin-test/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ dependencies {
testImplementation libs.mockwebserver
testImplementation libs.kotlinStdLib
testImplementation libs.kotlinCoroutines
testImplementation libs.rxjavaAdapter
testImplementation libs.rxjavaAdapter2
}
45 changes: 43 additions & 2 deletions retrofit/kotlin-test/src/test/java/retrofit2/KotlinSuspendTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import okhttp3.mockwebserver.MockWebServer
import okhttp3.mockwebserver.SocketPolicy.DISCONNECT_AFTER_REQUEST
import okhttp3.mockwebserver.SocketPolicy.NO_RESPONSE
import org.assertj.core.api.Assertions.assertThat
import org.junit.Assert.assertTrue
import org.junit.Assert.fail
import org.junit.Ignore
import org.junit.Rule
import org.junit.Test
Expand All @@ -39,6 +37,8 @@ import java.io.IOException
import java.lang.reflect.ParameterizedType
import java.lang.reflect.Type
import kotlin.coroutines.CoroutineContext
import org.junit.Assert.*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use star imports if this project wasn't already using them

import retrofit2.converter.gson.GsonConverterFactory

class KotlinSuspendTest {
@get:Rule val server = MockWebServer()
Expand All @@ -50,6 +50,9 @@ class KotlinSuspendTest {
@GET("/") suspend fun unit()
@HEAD("/") suspend fun headUnit()

@GET("user")
suspend fun getUser(): Result<User>

@GET("/{a}/{b}/{c}")
suspend fun params(
@Path("a") a: String,
Expand All @@ -58,6 +61,8 @@ class KotlinSuspendTest {
): String
}

data class User(val id:Int,val name: String,val email:String)

@Test fun body() {
val retrofit = Retrofit.Builder()
.baseUrl(server.url("/"))
Expand Down Expand Up @@ -353,6 +358,42 @@ class KotlinSuspendTest {
}
}

@Test
fun testSuccessfulResponse() = runBlocking {
val responseBody = """
{
"id": 1,
"name": "John Doe",
"email": "[email protected]"
}
""".trimIndent()
server.enqueue(MockResponse().setResponseCode(200).setBody(responseBody))
val retrofit = Retrofit.Builder()
.baseUrl(server.url("/"))
.addCallAdapterFactory(ResultCallAdapterFactory())
.addConverterFactory(GsonConverterFactory.create())
.build()
val service = retrofit.create(Service::class.java)
val result = service.getUser()
assertEquals(1, result.getOrNull()?.id)
assertEquals("John Doe", result.getOrNull()?.name)
assertEquals("[email protected]", result.getOrNull()?.email)
}

@Test
fun testErrorResponse() = runBlocking {
server.enqueue(MockResponse().setResponseCode(404))
val retrofit = Retrofit.Builder()
.baseUrl(server.url("/"))
.addCallAdapterFactory(ResultCallAdapterFactory())
.addConverterFactory(GsonConverterFactory.create())
.build()
val service = retrofit.create(Service::class.java)
val result = service.getUser()
assert(result.isFailure)
}


@Suppress("EXPERIMENTAL_OVERRIDE")
private object DirectUnconfinedDispatcher : CoroutineDispatcher() {
override fun isDispatchNeeded(context: CoroutineContext): Boolean = false
Expand Down
10 changes: 8 additions & 2 deletions retrofit/src/main/java/retrofit2/HttpServiceMethod.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import java.lang.reflect.ParameterizedType;
import java.lang.reflect.Type;
import javax.annotation.Nullable;

import kotlin.Result;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra space and new import not alphabetized

import kotlin.Unit;
import kotlin.coroutines.Continuation;
import okhttp3.ResponseBody;
Expand Down Expand Up @@ -52,15 +54,19 @@ static <ResponseT, ReturnT> HttpServiceMethod<ResponseT, ReturnT> parseAnnotatio
// Unwrap the actual body type from Response<T>.
responseType = Utils.getParameterUpperBound(0, (ParameterizedType) responseType);
continuationWantsResponse = true;
adapterType = new Utils.ParameterizedTypeImpl(null, Call.class, responseType);
} else {
if ((getRawType(responseType).isAssignableFrom(Result.class))) {
adapterType = responseType;
} else {
adapterType = new Utils.ParameterizedTypeImpl(null, Call.class, responseType);
}
continuationIsUnit = Utils.isUnit(responseType);
// TODO figure out if type is nullable or not
// Metadata metadata = method.getDeclaringClass().getAnnotation(Metadata.class)
// Find the entry for method
// Determine if return type is nullable or not
}

adapterType = new Utils.ParameterizedTypeImpl(null, Call.class, responseType);
annotations = SkipCallbackExecutorImpl.ensurePresent(annotations);
} else {
adapterType = method.getGenericReturnType();
Expand Down
88 changes: 88 additions & 0 deletions retrofit/src/main/java/retrofit2/ResultCallAdapterFactory.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package retrofit2

import java.io.IOException
import java.lang.reflect.ParameterizedType
import java.lang.reflect.Type
import okhttp3.Request
import okio.Timeout

class ResultCallAdapterFactory : CallAdapter.Factory() {
override fun get(
returnType: Type,
annotations: Array<Annotation>,
retrofit: Retrofit
): CallAdapter<*, *>? {
if (getRawType(returnType) != Result::class.java) {
return null
}

check(returnType is ParameterizedType) {
"Result must have a generic type (e.g., Result<T>)"
}

val responseType = getParameterUpperBound(0, returnType)
return ResultCallAdapter<Any>(responseType)
}
}

class ResultCallAdapter<T>(
private val responseType: Type
) : CallAdapter<T, Call<Result<T>>> {

override fun responseType(): Type {
return responseType
}

override fun adapt(call: Call<T>): Call<Result<out T>> {
return ResultCall(call)
}
}

class ResultCall<T>(private val delegate: Call<T>) : Call<Result<out T>> {

override fun enqueue(callback: Callback<Result<T>>) {
delegate.enqueue(object : Callback<T> {
override fun onResponse(call: Call<T>, response: Response<T>) {
val result = if (response.isSuccessful) {
Result.success(response.body()!!)
} else {
Result.failure(HttpException(response))
}
callback.onResponse(this@ResultCall, Response.success(result))
}

override fun onFailure(call: Call<T>, t: Throwable) {
callback.onResponse(this@ResultCall, Response.success(Result.failure(t)))
}
})
}

override fun execute(): Response<Result<T>> {
return try {
val response = delegate.execute()
val result = if (response.isSuccessful) {
Result.success(response.body()!!)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use !! - there is always a better way to fail gracefully into the Result.failure case if this is null.

} else {
Result.failure(IOException("Unexpected error: ${response.errorBody()?.string()}"))
}
Response.success(result)
} catch (e: IOException) {
Response.success(Result.failure(e))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does the same job as the more general Exception case, so it's not needed

} catch (e: Exception) {
Response.success(Result.failure(e))
}
}
override fun isExecuted(): Boolean = delegate.isExecuted

override fun clone(): ResultCall<T> = ResultCall(delegate.clone())

override fun isCanceled(): Boolean = delegate.isCanceled

override fun cancel() = delegate.cancel()

override fun request(): Request = delegate.request()

override fun timeout(): Timeout = delegate.timeout()
}