Merge pull request #3145 from square/jakew/extend/2019-07-03

Allow service interfaces to extend other interfaces

retrofit/src/main/java/retrofit2/Retrofit.java

@@ -22,7 +22,10 @@ import java.lang.reflect.Modifier;
 import java.lang.reflect.Proxy;
 import java.lang.reflect.Type;
 import java.net.URL;
+import java.util.ArrayDeque;
 import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Deque;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
@@ -128,10 +131,7 @@ public final class Retrofit {
    */
   @SuppressWarnings("unchecked") // Single-interface proxy creation guarded by parameter safety.
   public <T> T create(final Class<T> service) {
-    Utils.validateServiceInterface(service);
-    if (validateEagerly) {
-      eagerlyValidateMethods(service);
-    }
+    validateServiceInterface(service);
     return (T) Proxy.newProxyInstance(service.getClassLoader(), new Class<?>[] { service },
         new InvocationHandler() {
           private final Platform platform = Platform.get();
@@ -151,11 +151,33 @@ public final class Retrofit {
         });
   }
 
-  private void eagerlyValidateMethods(Class<?> service) {
-    Platform platform = Platform.get();
-    for (Method method : service.getDeclaredMethods()) {
-      if (!platform.isDefaultMethod(method) && !Modifier.isStatic(method.getModifiers())) {
-        loadServiceMethod(method);
+  private void validateServiceInterface(Class<?> service) {
+    if (!service.isInterface()) {
+      throw new IllegalArgumentException("API declarations must be interfaces.");
+    }
+
+    Deque<Class<?>> check = new ArrayDeque<>(1);
+    check.add(service);
+    while (!check.isEmpty()) {
+      Class<?> candidate = check.removeFirst();
+      if (candidate.getTypeParameters().length != 0) {
+        StringBuilder message = new StringBuilder("Type parameters are unsupported on ")
+            .append(candidate.getName());
+        if (candidate != service) {
+          message.append(" which is an interface of ")
+              .append(service.getName());
+        }
+        throw new IllegalArgumentException(message.toString());
+      }
+      Collections.addAll(check, candidate.getInterfaces());
+    }
+
+    if (validateEagerly) {
+      Platform platform = Platform.get();
+      for (Method method : service.getDeclaredMethods()) {
+        if (!platform.isDefaultMethod(method) && !Modifier.isStatic(method.getModifiers())) {
+          loadServiceMethod(method);
+        }
       }
     }
   }

retrofit/src/main/java/retrofit2/Utils.java

@@ -317,18 +317,6 @@ final class Utils {
     return ResponseBody.create(body.contentType(), body.contentLength(), buffer);
   }
 
-  static <T> void validateServiceInterface(Class<T> service) {
-    if (!service.isInterface()) {
-      throw new IllegalArgumentException("API declarations must be interfaces.");
-    }
-    // Prevent API interfaces from extending other interfaces. This not only avoids a bug in
-    // Android (http://b.android.com/58753) but it forces composition of API declarations which is
-    // the recommended pattern.
-    if (service.getInterfaces().length > 0) {
-      throw new IllegalArgumentException("API interfaces must not extend other interfaces.");
-    }
-  }
-
   static Type getParameterUpperBound(int index, ParameterizedType type) {
     Type[] types = type.getActualTypeArguments();
     if (index < 0 || index >= types.length) {

retrofit/src/test/java/retrofit2/RetrofitTest.java

@@ -85,6 +85,10 @@ public final class RetrofitTest {
   }
   interface Extending extends CallMethod {
   }
+  interface TypeParam<T> {
+  }
+  interface ExtendingTypeParam extends TypeParam<String> {
+  }
   interface StringService {
     @GET("/") String get();
   }
@@ -129,15 +133,47 @@ public final class RetrofitTest {
     assertThat(example.toString()).isNotEmpty();
   }
 
-  @Test public void interfaceWithExtendIsNotSupported() {
+  @Test public void interfaceWithTypeParameterThrows() {
+    Retrofit retrofit = new Retrofit.Builder()
+        .baseUrl(server.url("/"))
+        .build();
+
+    server.enqueue(new MockResponse().setBody("Hi"));
+
+    try {
+      retrofit.create(TypeParam.class);
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertThat(e).hasMessage("Type parameters are unsupported on retrofit2.RetrofitTest$TypeParam");
+    }
+  }
+
+  @Test public void interfaceWithExtend() throws IOException {
     Retrofit retrofit = new Retrofit.Builder()
         .baseUrl(server.url("/"))
         .build();
+
+    server.enqueue(new MockResponse().setBody("Hi"));
+
+    Extending extending = retrofit.create(Extending.class);
+    String result = extending.getResponseBody().execute().body().string();
+    assertEquals("Hi", result);
+  }
+
+  @Test public void interfaceWithExtendWithTypeParameterThrows() {
+    Retrofit retrofit = new Retrofit.Builder()
+        .baseUrl(server.url("/"))
+        .build();
+
+    server.enqueue(new MockResponse().setBody("Hi"));
+
     try {
-      retrofit.create(Extending.class);
+      retrofit.create(ExtendingTypeParam.class);
       fail();
     } catch (IllegalArgumentException e) {
-      assertThat(e).hasMessage("API interfaces must not extend other interfaces.");
+      assertThat(e).hasMessage(
+          "Type parameters are unsupported on retrofit2.RetrofitTest$TypeParam "
+              + "which is an interface of retrofit2.RetrofitTest$ExtendingTypeParam");
     }
   }