Merge pull request #3702 from ansman/improve-unit-support

Improve support for Unit as the return type
square · Mar 30, 2022 · 1490e6b · 1490e6b
2 parents 4910b3c + 9eedffd
commit 1490e6b
Show file tree

Hide file tree

Showing 9 changed files with 146 additions and 49 deletions.
diff --git a/retrofit/kotlin-test/src/test/java/retrofit2/KotlinRequestFactoryTest.java 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<Unit> 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
@@ -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<String>
+    @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
@@ -24,17 +24,21 @@
 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();
 
   interface Service {
     @GET("/")
     Call<Unit> empty();
+
+    @HEAD("/")
+    Call<Unit> 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<Unit> 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
@@ -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<ResponseBody, ?> 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
@@ -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 <ResponseT, ReturnT> HttpServiceMethod<ResponseT, ReturnT> 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 <ResponseT, ReturnT> HttpServiceMethod<ResponseT, ReturnT> 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 <ResponseT, ReturnT> HttpServiceMethod<ResponseT, ReturnT> parseAnnotatio
       throw methodError(method, "Response must include generic type (e.g., Response<String>)");
     }
     // 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<ResponseBody, ResponseT> responseConverter =
@@ -103,7 +108,8 @@ static <ResponseT, ReturnT> HttpServiceMethod<ResponseT, ReturnT> parseAnnotatio
               callFactory,
               responseConverter,
               (CallAdapter<ResponseT, Call<ResponseT>>) callAdapter,
-              continuationBodyNullable);
+              continuationBodyNullable,
+              continuationIsUnit);
     }
   }
 
@@ -198,16 +204,19 @@ protected Object adapt(Call<ResponseT> call, Object[] args) {
   static final class SuspendForBody<ResponseT> extends HttpServiceMethod<ResponseT, Object> {
     private final CallAdapter<ResponseT, Call<ResponseT>> callAdapter;
     private final boolean isNullable;
+    private final boolean isUnit;
 
     SuspendForBody(
         RequestFactory requestFactory,
         okhttp3.Call.Factory callFactory,
         Converter<ResponseBody, ResponseT> responseConverter,
         CallAdapter<ResponseT, Call<ResponseT>> 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<ResponseT> 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<Unit>) call, (Continuation<Unit>) 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
@@ -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 <T : Any> Call<T?>.await(): T? {
   }
 }
 
+@JvmName("awaitUnit")
+suspend fun Call<Unit>.await() {
+  @Suppress("UNCHECKED_CAST")
+  (this as Call<Unit?>).await()
+}
+
 suspend fun <T> Call<T>.awaitResponse(): Response<T> {
   return suspendCancellableCoroutine { continuation ->
     continuation.invokeOnCancellation {

diff --git 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
@@ -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<ResponseBody> method() {
   }
 
   @Test
-  public void head() {
+  public void headVoid() {
     class Example {
       @HEAD("/foo/bar/") //
       Call<Void> method() {
@@ -879,7 +878,8 @@ Call<ResponseBody> 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 <T> Request buildRequest(Class<T> 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 <T> Request buildRequest(Class<T> 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/...src/test/java/retrofit2/TestingUtils.java → ...src/main/java/retrofit2/TestingUtils.java b/...src/test/java/retrofit2/TestingUtils.java → ...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 <T> Request buildRequest(Class<T> 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 <T> Request buildRequest(Class<T> 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) {