From 9eedffd1b53cf6e4f1a3c188b305010c57c09929 Mon Sep 17 00:00:00 2001 From: Nicklas Ansman Giertz Date: Fri, 25 Feb 2022 13:04:12 -0500 Subject: [PATCH] Improve support for Unit as the return type Specifically this change fixes two cases: - Using Unit for with suspending functions for responses without a body. - Using Unit for HEAD requests. --- .../retrofit2/KotlinRequestFactoryTest.java | 27 ++++++++++++++ .../test/java/retrofit2/KotlinSuspendTest.kt | 24 +++++++++++++ .../test/java/retrofit2/KotlinUnitTest.java | 18 +++++++++- .../java/retrofit2/BuiltInConverters.java | 12 ++----- .../java/retrofit2/HttpServiceMethod.java | 28 +++++++++++---- .../main/java/retrofit2/KotlinExtensions.kt | 7 ++++ retrofit/src/main/java/retrofit2/Utils.java | 15 ++++++++ .../java/retrofit2/RequestFactoryTest.java | 35 +++---------------- .../main}/java/retrofit2/TestingUtils.java | 29 +++++++++++++++ 9 files changed, 146 insertions(+), 49 deletions(-) create mode 100644 retrofit/kotlin-test/src/test/java/retrofit2/KotlinRequestFactoryTest.java rename retrofit/{src/test => test-helpers/src/main}/java/retrofit2/TestingUtils.java (54%) diff --git a/retrofit/kotlin-test/src/test/java/retrofit2/KotlinRequestFactoryTest.java b/retrofit/kotlin-test/src/test/java/retrofit2/KotlinRequestFactoryTest.java new file mode 100644 index 0000000000..2b35b37d85 --- /dev/null +++ b/retrofit/kotlin-test/src/test/java/retrofit2/KotlinRequestFactoryTest.java @@ -0,0 +1,27 @@ +package retrofit2; + +import kotlin.Unit; +import okhttp3.Request; +import org.junit.Test; +import retrofit2.http.HEAD; + +import static org.assertj.core.api.Assertions.assertThat; +import static retrofit2.TestingUtils.buildRequest; + +public final class KotlinRequestFactoryTest { + @Test + public void headUnit() { + class Example { + @HEAD("/foo/bar/") + Call method() { + return null; + } + } + + Request request = buildRequest(Example.class); + assertThat(request.method()).isEqualTo("HEAD"); + assertThat(request.headers().size()).isZero(); + assertThat(request.url().toString()).isEqualTo("http://example.com/foo/bar/"); + assertThat(request.body()).isNull(); + } +} diff --git a/retrofit/kotlin-test/src/test/java/retrofit2/KotlinSuspendTest.kt b/retrofit/kotlin-test/src/test/java/retrofit2/KotlinSuspendTest.kt index e3f6ed66db..260fd7ab98 100644 --- a/retrofit/kotlin-test/src/test/java/retrofit2/KotlinSuspendTest.kt +++ b/retrofit/kotlin-test/src/test/java/retrofit2/KotlinSuspendTest.kt @@ -33,6 +33,7 @@ import org.junit.Rule import org.junit.Test import retrofit2.helpers.ToStringConverterFactory import retrofit2.http.GET +import retrofit2.http.HEAD import retrofit2.http.Path import java.io.IOException import java.lang.reflect.ParameterizedType @@ -46,6 +47,8 @@ class KotlinSuspendTest { @GET("/") suspend fun body(): String @GET("/") suspend fun bodyNullable(): String? @GET("/") suspend fun response(): Response + @GET("/") suspend fun unit() + @HEAD("/") suspend fun headUnit() @GET("/{a}/{b}/{c}") suspend fun params( @@ -178,6 +181,27 @@ class KotlinSuspendTest { } } + @Test fun unit() { + val retrofit = Retrofit.Builder().baseUrl(server.url("/")).build() + val example = retrofit.create(Service::class.java) + server.enqueue(MockResponse().setBody("Unit")) + runBlocking { example.unit() } + } + + @Test fun unitNullableBody() { + val retrofit = Retrofit.Builder().baseUrl(server.url("/")).build() + val example = retrofit.create(Service::class.java) + server.enqueue(MockResponse().setResponseCode(204)) + runBlocking { example.unit() } + } + + @Test fun headUnit() { + val retrofit = Retrofit.Builder().baseUrl(server.url("/")).build() + val example = retrofit.create(Service::class.java) + server.enqueue(MockResponse()) + runBlocking { example.headUnit() } + } + @Test fun params() { val retrofit = Retrofit.Builder() .baseUrl(server.url("/")) diff --git a/retrofit/kotlin-test/src/test/java/retrofit2/KotlinUnitTest.java b/retrofit/kotlin-test/src/test/java/retrofit2/KotlinUnitTest.java index 389441b166..e9161b4259 100644 --- a/retrofit/kotlin-test/src/test/java/retrofit2/KotlinUnitTest.java +++ b/retrofit/kotlin-test/src/test/java/retrofit2/KotlinUnitTest.java @@ -24,6 +24,7 @@ import org.junit.Rule; import org.junit.Test; import retrofit2.http.GET; +import retrofit2.http.HEAD; public final class KotlinUnitTest { @Rule public final MockWebServer server = new MockWebServer(); @@ -31,10 +32,13 @@ public final class KotlinUnitTest { interface Service { @GET("/") Call empty(); + + @HEAD("/") + Call head(); } @Test - public void unitOnClasspath() throws IOException { + public void unitGet() throws IOException { Retrofit retrofit = new Retrofit.Builder().baseUrl(server.url("/")).build(); Service example = retrofit.create(Service.class); @@ -44,4 +48,16 @@ public void unitOnClasspath() throws IOException { assertThat(response.isSuccessful()).isTrue(); assertThat(response.body()).isSameAs(Unit.INSTANCE); } + + @Test + public void unitHead() throws IOException { + Retrofit retrofit = new Retrofit.Builder().baseUrl(server.url("/")).build(); + Service example = retrofit.create(Service.class); + + server.enqueue(new MockResponse().setBody("Hi")); + + Response response = example.head().execute(); + assertThat(response.isSuccessful()).isTrue(); + assertThat(response.body()).isSameAs(Unit.INSTANCE); + } } diff --git a/retrofit/src/main/java/retrofit2/BuiltInConverters.java b/retrofit/src/main/java/retrofit2/BuiltInConverters.java index 68f414cfca..889f647f8b 100644 --- a/retrofit/src/main/java/retrofit2/BuiltInConverters.java +++ b/retrofit/src/main/java/retrofit2/BuiltInConverters.java @@ -25,8 +25,6 @@ import retrofit2.http.Streaming; final class BuiltInConverters extends Converter.Factory { - /** Not volatile because we don't mind multiple threads discovering this. */ - private boolean checkForKotlinUnit = true; @Override public @Nullable Converter responseBodyConverter( @@ -39,14 +37,8 @@ final class BuiltInConverters extends Converter.Factory { if (type == Void.class) { return VoidResponseBodyConverter.INSTANCE; } - if (checkForKotlinUnit) { - try { - if (type == Unit.class) { - return UnitResponseBodyConverter.INSTANCE; - } - } catch (NoClassDefFoundError ignored) { - checkForKotlinUnit = false; - } + if (Utils.isUnit(type)) { + return UnitResponseBodyConverter.INSTANCE; } return null; } diff --git a/retrofit/src/main/java/retrofit2/HttpServiceMethod.java b/retrofit/src/main/java/retrofit2/HttpServiceMethod.java index 54ddc24724..2ca5c7f0ae 100644 --- a/retrofit/src/main/java/retrofit2/HttpServiceMethod.java +++ b/retrofit/src/main/java/retrofit2/HttpServiceMethod.java @@ -23,6 +23,7 @@ import java.lang.reflect.ParameterizedType; import java.lang.reflect.Type; import javax.annotation.Nullable; +import kotlin.Unit; import kotlin.coroutines.Continuation; import okhttp3.ResponseBody; @@ -38,6 +39,7 @@ static HttpServiceMethod parseAnnotatio boolean isKotlinSuspendFunction = requestFactory.isKotlinSuspendFunction; boolean continuationWantsResponse = false; boolean continuationBodyNullable = false; + boolean continuationIsUnit = false; Annotation[] annotations = method.getAnnotations(); Type adapterType; @@ -51,6 +53,7 @@ static HttpServiceMethod parseAnnotatio responseType = Utils.getParameterUpperBound(0, (ParameterizedType) responseType); continuationWantsResponse = true; } else { + 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 @@ -77,8 +80,10 @@ static HttpServiceMethod parseAnnotatio throw methodError(method, "Response must include generic type (e.g., Response)"); } // TODO support Unit for Kotlin? - if (requestFactory.httpMethod.equals("HEAD") && !Void.class.equals(responseType)) { - throw methodError(method, "HEAD method must use Void as response type."); + if (requestFactory.httpMethod.equals("HEAD") + && !Void.class.equals(responseType) + && !Utils.isUnit(responseType)) { + throw methodError(method, "HEAD method must use Void or Unit as response type."); } Converter responseConverter = @@ -103,7 +108,8 @@ static HttpServiceMethod parseAnnotatio callFactory, responseConverter, (CallAdapter>) callAdapter, - continuationBodyNullable); + continuationBodyNullable, + continuationIsUnit); } } @@ -198,16 +204,19 @@ protected Object adapt(Call call, Object[] args) { static final class SuspendForBody extends HttpServiceMethod { private final CallAdapter> callAdapter; private final boolean isNullable; + private final boolean isUnit; SuspendForBody( RequestFactory requestFactory, okhttp3.Call.Factory callFactory, Converter responseConverter, CallAdapter> callAdapter, - boolean isNullable) { + boolean isNullable, + boolean isUnit) { super(requestFactory, callFactory, responseConverter); this.callAdapter = callAdapter; this.isNullable = isNullable; + this.isUnit = isUnit; } @Override @@ -226,9 +235,14 @@ protected Object adapt(Call call, Object[] args) { // force suspension to occur so that it can be instead delivered to the continuation to // bypass this restriction. try { - return isNullable - ? KotlinExtensions.awaitNullable(call, continuation) - : KotlinExtensions.await(call, continuation); + if (isUnit) { + //noinspection unchecked Checked by isUnit + return KotlinExtensions.awaitUnit((Call) call, (Continuation) continuation); + } else if (isNullable) { + return KotlinExtensions.awaitNullable(call, continuation); + } else { + return KotlinExtensions.await(call, continuation); + } } catch (Exception e) { return KotlinExtensions.suspendAndThrow(e, continuation); } diff --git a/retrofit/src/main/java/retrofit2/KotlinExtensions.kt b/retrofit/src/main/java/retrofit2/KotlinExtensions.kt index b9f57d999d..d334b25136 100644 --- a/retrofit/src/main/java/retrofit2/KotlinExtensions.kt +++ b/retrofit/src/main/java/retrofit2/KotlinExtensions.kt @@ -20,6 +20,7 @@ package retrofit2 import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.suspendCancellableCoroutine +import java.lang.reflect.ParameterizedType import kotlin.coroutines.intrinsics.COROUTINE_SUSPENDED import kotlin.coroutines.intrinsics.intercepted import kotlin.coroutines.intrinsics.suspendCoroutineUninterceptedOrReturn @@ -83,6 +84,12 @@ suspend fun Call.await(): T? { } } +@JvmName("awaitUnit") +suspend fun Call.await() { + @Suppress("UNCHECKED_CAST") + (this as Call).await() +} + suspend fun Call.awaitResponse(): Response { return suspendCancellableCoroutine { continuation -> continuation.invokeOnCancellation { diff --git a/retrofit/src/main/java/retrofit2/Utils.java b/retrofit/src/main/java/retrofit2/Utils.java index f922d005ae..114b8ea97e 100644 --- a/retrofit/src/main/java/retrofit2/Utils.java +++ b/retrofit/src/main/java/retrofit2/Utils.java @@ -29,6 +29,7 @@ import java.util.NoSuchElementException; import java.util.Objects; import javax.annotation.Nullable; +import kotlin.Unit; import okhttp3.ResponseBody; import okio.Buffer; @@ -534,4 +535,18 @@ static void throwIfFatal(Throwable t) { throw (LinkageError) t; } } + + /** Not volatile because we don't mind multiple threads discovering this. */ + private static boolean checkForKotlinUnit = true; + + static boolean isUnit(Type type) { + if (checkForKotlinUnit) { + try { + return type == Unit.class; + } catch (NoClassDefFoundError ignored) { + checkForKotlinUnit = false; + } + } + return false; + } } diff --git a/retrofit/src/test/java/retrofit2/RequestFactoryTest.java b/retrofit/src/test/java/retrofit2/RequestFactoryTest.java index aeb3ce98cc..e0f4005099 100644 --- a/retrofit/src/test/java/retrofit2/RequestFactoryTest.java +++ b/retrofit/src/test/java/retrofit2/RequestFactoryTest.java @@ -20,9 +20,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; +import static retrofit2.TestingUtils.buildRequest; import java.io.IOException; -import java.lang.reflect.Method; import java.math.BigInteger; import java.net.URI; import java.util.Arrays; @@ -41,7 +41,6 @@ import org.junit.Ignore; import org.junit.Test; import retrofit2.helpers.NullObjectConverterFactory; -import retrofit2.helpers.ToStringConverterFactory; import retrofit2.http.Body; import retrofit2.http.DELETE; import retrofit2.http.Field; @@ -851,7 +850,7 @@ Call method() { } @Test - public void head() { + public void headVoid() { class Example { @HEAD("/foo/bar/") // Call method() { @@ -879,7 +878,8 @@ Call method() { fail(); } catch (IllegalArgumentException e) { assertThat(e) - .hasMessage("HEAD method must use Void as response type.\n for method Example.method"); + .hasMessage( + "HEAD method must use Void or Unit as response type.\n for method Example.method"); } } @@ -3276,33 +3276,6 @@ private static void assertBody(RequestBody body, String expected) { } } - static Request buildRequest(Class cls, Retrofit.Builder builder, Object... args) { - okhttp3.Call.Factory callFactory = - request -> { - throw new UnsupportedOperationException("Not implemented"); - }; - - Retrofit retrofit = builder.callFactory(callFactory).build(); - - Method method = TestingUtils.onlyMethod(cls); - try { - return RequestFactory.parseAnnotations(retrofit, method).create(args); - } catch (RuntimeException e) { - throw e; - } catch (Exception e) { - throw new AssertionError(e); - } - } - - static Request buildRequest(Class cls, Object... args) { - Retrofit.Builder retrofitBuilder = - new Retrofit.Builder() - .baseUrl("http://example.com/") - .addConverterFactory(new ToStringConverterFactory()); - - return buildRequest(cls, retrofitBuilder, args); - } - static void assertMalformedRequest(Class cls, Object... args) { try { Request request = buildRequest(cls, args); diff --git a/retrofit/src/test/java/retrofit2/TestingUtils.java b/retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java similarity index 54% rename from retrofit/src/test/java/retrofit2/TestingUtils.java rename to retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java index f68b724cf7..f03ef7b3b2 100644 --- a/retrofit/src/test/java/retrofit2/TestingUtils.java +++ b/retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java @@ -17,8 +17,37 @@ import java.lang.reflect.Method; import java.util.Arrays; +import okhttp3.Request; +import retrofit2.helpers.ToStringConverterFactory; final class TestingUtils { + static Request buildRequest(Class cls, Retrofit.Builder builder, Object... args) { + okhttp3.Call.Factory callFactory = + request -> { + throw new UnsupportedOperationException("Not implemented"); + }; + + Retrofit retrofit = builder.callFactory(callFactory).build(); + + Method method = onlyMethod(cls); + try { + return RequestFactory.parseAnnotations(retrofit, method).create(args); + } catch (RuntimeException e) { + throw e; + } catch (Exception e) { + throw new AssertionError(e); + } + } + + static Request buildRequest(Class cls, Object... args) { + Retrofit.Builder retrofitBuilder = + new Retrofit.Builder() + .baseUrl("http://example.com/") + .addConverterFactory(new ToStringConverterFactory()); + + return buildRequest(cls, retrofitBuilder, args); + } + static Method onlyMethod(Class c) { Method[] declaredMethods = c.getDeclaredMethods(); if (declaredMethods.length == 1) {