Skip to content

Commit

Permalink
Merge pull request square#3145 from square/jakew/extend/2019-07-03
Browse files Browse the repository at this point in the history
Allow service interfaces to extend other interfaces
  • Loading branch information
squarejesse authored Jul 24, 2019
2 parents 9889954 + 948a0db commit f3c97d6
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 24 deletions.
40 changes: 31 additions & 9 deletions retrofit/src/main/java/retrofit2/Retrofit.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@
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;
Expand Down Expand Up @@ -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();
Expand All @@ -151,11 +151,33 @@ public <T> T create(final Class<T> service) {
});
}

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);
}
}
}
}
Expand Down
12 changes: 0 additions & 12 deletions retrofit/src/main/java/retrofit2/Utils.java
Original file line number Diff line number Diff line change
Expand Up @@ -317,18 +317,6 @@ static ResponseBody buffer(final ResponseBody body) throws IOException {
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) {
Expand Down
42 changes: 39 additions & 3 deletions retrofit/src/test/java/retrofit2/RetrofitTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ interface FutureMethod {
}
interface Extending extends CallMethod {
}
interface TypeParam<T> {
}
interface ExtendingTypeParam extends TypeParam<String> {
}
interface StringService {
@GET("/") String get();
}
Expand Down Expand Up @@ -129,15 +133,47 @@ interface MutableParameters {
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");
}
}

Expand Down

0 comments on commit f3c97d6

Please sign in to comment.