Repository: square/retrofit
Issue: Add Service instance to `Invocation`
Labels: Enhancement
PR welcome

What kind of issue is this?

 - [ ] Question. This issue tracker is not the place for questions. If you want to ask how to do
       something, or to understand why something isn't working the way you expect it to, use Stack
       Overflow. https://stackoverflow.com/questions/tagged/retrofit

 - [ ] Bug report. If you’ve found a bug, spend the time to write a failing test. Bugs with tests
       get fixed. Here’s an example: https://gist.github.com/swankjesse/6608b4713ad80988cdc9

 - [x] Feature Request. Start by telling us what problem you’re trying to solve. Often a solution
       already exists! Don’t send pull requests to implement new features without first getting our
       support. Sometimes we leave features out on purpose to keep the project small.

Since Retrofit supports service interface hierarchies, the `method()` & `arguments()` of the `Invocation` are no more sufficient to distinguish the invocation. A method might be declared in a super class with Retrofit annotations but the subclass is the interface which gets created.

```kt
interface Super {
  @GET("/super") fun getSuper()
}

// This interface gets instantiated via Retrofit.create(Sub::class)
interface Sub: Super {
  fun getSuperFallback(error: Throwable) { ... }
}
```

The primary use case right now is to be able to support/implement Resilience4j's CircuitBreaker fallback mechanism but there are [other library use cases](https://github.com/LianjiaTech/retrofit-spring-boot-starter/issues/109) and it might open the door to many others.

Issue comments:
Are you asking for the actual instance or just the service class?
I am asking for the actual instance 🤔 (then the real service class can be deduced)
This is really easy to add, but I just want to make sure I understand completely what you want so I can try to balance it with any and all other options we have.

So are you basically would unsafe cast back to the `Sub` type in order to call this fallback function?
In my case, it would be called via reflection on the instance, but some other integration would require an unsafe cast, yes.
I think we'll add both the class and the instance.
soft bump ⬆️ @JakeWharton 😅
Just ran into another use case for this! This time for distinguishing the correct "api" subclass.

Pull request review comments:
Trying to maintain binary compatibility for anyone using this method by approximating the service class as `method.declaringClass` and instance as `null`.

Would love to know if this is good enough or if its okay to break compatibility.
Didn't see another way of maintaining compatibility than making the `instance` nullable

Patch that resolved the issue:
diff --git a/retrofit/src/main/java/retrofit2/HttpServiceMethod.java b/retrofit/src/main/java/retrofit2/HttpServiceMethod.java
index 38b7318a57..d285c0ae31 100644
--- a/retrofit/src/main/java/retrofit2/HttpServiceMethod.java
+++ b/retrofit/src/main/java/retrofit2/HttpServiceMethod.java
@@ -154,8 +154,9 @@ private static <ResponseT> Converter<ResponseBody, ResponseT> createResponseConv
   }
 
   @Override
-  final @Nullable ReturnT invoke(Object[] args) {
-    Call<ResponseT> call = new OkHttpCall<>(requestFactory, args, callFactory, responseConverter);
+  final @Nullable ReturnT invoke(Object instance, Object[] args) {
+    Call<ResponseT> call =
+        new OkHttpCall<>(requestFactory, instance, args, callFactory, responseConverter);
     return adapt(call, args);
   }
 
diff --git a/retrofit/src/main/java/retrofit2/Invocation.java b/retrofit/src/main/java/retrofit2/Invocation.java
index 84e50e089f..cd6d7a72f2 100644
--- a/retrofit/src/main/java/retrofit2/Invocation.java
+++ b/retrofit/src/main/java/retrofit2/Invocation.java
@@ -20,6 +20,7 @@
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
+import javax.annotation.Nullable;
 
 /**
  * A single invocation of a Retrofit service interface method. This class captures both the method
@@ -35,8 +36,9 @@
  *     Invocation invocation = request.tag(Invocation.class);
  *     if (invocation != null) {
  *       System.out.printf("%s.%s %s%n",
- *           invocation.method().getDeclaringClass().getSimpleName(),
- *           invocation.method().getName(), invocation.arguments());
+ *           invocation.service().getSimpleName(),
+ *           invocation.method().getName(),
+ *           invocation.arguments());
  *     }
  *     return chain.proceed(request);
  *   }
@@ -49,21 +51,50 @@
  * types for parameters!
  */
 public final class Invocation {
+  public static <T> Invocation of(Class<T> service, T instance, Method method, List<?> arguments) {
+    Objects.requireNonNull(service, "service == null");
+    Objects.requireNonNull(instance, "instance == null");
+    Objects.requireNonNull(method, "method == null");
+    Objects.requireNonNull(arguments, "arguments == null");
+    return new Invocation(service, instance, method, new ArrayList<>(arguments)); // Defensive copy.
+  }
+
+  @Deprecated
   public static Invocation of(Method method, List<?> arguments) {
     Objects.requireNonNull(method, "method == null");
     Objects.requireNonNull(arguments, "arguments == null");
-    return new Invocation(method, new ArrayList<>(arguments)); // Defensive copy.
+    return new Invocation(
+        method.getDeclaringClass(), null, method, new ArrayList<>(arguments)); // Defensive copy.
   }
 
+  private final Class<?> service;
+  @Nullable private final Object instance;
   private final Method method;
   private final List<?> arguments;
 
   /** Trusted constructor assumes ownership of {@code arguments}. */
-  Invocation(Method method, List<?> arguments) {
+  Invocation(Class<?> service, @Nullable Object instance, Method method, List<?> arguments) {
+    this.service = service;
+    this.instance = instance;
     this.method = method;
     this.arguments = Collections.unmodifiableList(arguments);
   }
 
+  public Class<?> service() {
+    return service;
+  }
+
+  /**
+   * The instance of {@link #service}.
+   * <p>
+   * This will never be null when created by Retrofit. Null will only be returned when created
+   * by {@link #of(Method, List)}.
+   */
+  @Nullable
+  public Object instance() {
+    return instance;
+  }
+
   public Method method() {
     return method;
   }
@@ -74,7 +105,6 @@ public List<?> arguments() {
 
   @Override
   public String toString() {
-    return String.format(
-        "%s.%s() %s", method.getDeclaringClass().getName(), method.getName(), arguments);
+    return String.format("%s.%s() %s", service.getName(), method.getName(), arguments);
   }
 }
diff --git a/retrofit/src/main/java/retrofit2/KotlinExtensions.kt b/retrofit/src/main/java/retrofit2/KotlinExtensions.kt
index 09b81b2628..2444c883ca 100644
--- a/retrofit/src/main/java/retrofit2/KotlinExtensions.kt
+++ b/retrofit/src/main/java/retrofit2/KotlinExtensions.kt
@@ -39,9 +39,10 @@ suspend fun <T : Any> Call<T>.await(): T {
           val body = response.body()
           if (body == null) {
             val invocation = call.request().tag(Invocation::class.java)!!
+            val service = invocation.service()
             val method = invocation.method()
             val e = KotlinNullPointerException(
-              "Response from ${method.declaringClass.name}.${method.name}" +
+              "Response from ${service.name}.${method.name}" +
                 " was null but response body type was declared as non-null",
             )
             continuation.resumeWithException(e)
diff --git a/retrofit/src/main/java/retrofit2/OkHttpCall.java b/retrofit/src/main/java/retrofit2/OkHttpCall.java
index 14c53521f7..ff67612263 100644
--- a/retrofit/src/main/java/retrofit2/OkHttpCall.java
+++ b/retrofit/src/main/java/retrofit2/OkHttpCall.java
@@ -32,6 +32,7 @@
 
 final class OkHttpCall<T> implements Call<T> {
   private final RequestFactory requestFactory;
+  private final Object instance;
   private final Object[] args;
   private final okhttp3.Call.Factory callFactory;
   private final Converter<ResponseBody, T> responseConverter;
@@ -49,10 +50,12 @@ final class OkHttpCall<T> implements Call<T> {
 
   OkHttpCall(
       RequestFactory requestFactory,
+      Object instance,
       Object[] args,
       okhttp3.Call.Factory callFactory,
       Converter<ResponseBody, T> responseConverter) {
     this.requestFactory = requestFactory;
+    this.instance = instance;
     this.args = args;
     this.callFactory = callFactory;
     this.responseConverter = responseConverter;
@@ -61,7 +64,7 @@ final class OkHttpCall<T> implements Call<T> {
   @SuppressWarnings("CloneDoesntCallSuperClone") // We are a final type & this saves clearing state.
   @Override
   public OkHttpCall<T> clone() {
-    return new OkHttpCall<>(requestFactory, args, callFactory, responseConverter);
+    return new OkHttpCall<>(requestFactory, instance, args, callFactory, responseConverter);
   }
 
   @Override
@@ -205,7 +208,7 @@ public Response<T> execute() throws IOException {
   }
 
   private okhttp3.Call createRawCall() throws IOException {
-    okhttp3.Call call = callFactory.newCall(requestFactory.create(args));
+    okhttp3.Call call = callFactory.newCall(requestFactory.create(instance, args));
     if (call == null) {
       throw new NullPointerException("Call.Factory returned null.");
     }
diff --git a/retrofit/src/main/java/retrofit2/RequestFactory.java b/retrofit/src/main/java/retrofit2/RequestFactory.java
index 52a4b1e6f0..6a30a99190 100644
--- a/retrofit/src/main/java/retrofit2/RequestFactory.java
+++ b/retrofit/src/main/java/retrofit2/RequestFactory.java
@@ -63,10 +63,11 @@
 import retrofit2.http.Url;
 
 final class RequestFactory {
-  static RequestFactory parseAnnotations(Retrofit retrofit, Method method) {
-    return new Builder(retrofit, method).build();
+  static RequestFactory parseAnnotations(Retrofit retrofit, Class<?> service, Method method) {
+    return new Builder(retrofit, service, method).build();
   }
 
+  private final Class<?> service;
   private final Method method;
   private final HttpUrl baseUrl;
   final String httpMethod;
@@ -80,6 +81,7 @@ static RequestFactory parseAnnotations(Retrofit retrofit, Method method) {
   final boolean isKotlinSuspendFunction;
 
   RequestFactory(Builder builder) {
+    service = builder.service;
     method = builder.method;
     baseUrl = builder.retrofit.baseUrl;
     httpMethod = builder.httpMethod;
@@ -93,7 +95,7 @@ static RequestFactory parseAnnotations(Retrofit retrofit, Method method) {
     isKotlinSuspendFunction = builder.isKotlinSuspendFunction;
   }
 
-  okhttp3.Request create(Object[] args) throws IOException {
+  okhttp3.Request create(@Nullable Object instance, Object[] args) throws IOException {
     @SuppressWarnings("unchecked") // It is an error to invoke a method with the wrong arg types.
     ParameterHandler<Object>[] handlers = (ParameterHandler<Object>[]) parameterHandlers;
 
@@ -129,7 +131,10 @@ okhttp3.Request create(Object[] args) throws IOException {
       handlers[p].apply(requestBuilder, args[p]);
     }
 
-    return requestBuilder.get().tag(Invocation.class, new Invocation(method, argumentList)).build();
+    return requestBuilder
+        .get()
+        .tag(Invocation.class, new Invocation(service, instance, method, argumentList))
+        .build();
   }
 
   /**
@@ -144,6 +149,7 @@ static final class Builder {
     private static final Pattern PARAM_NAME_REGEX = Pattern.compile(PARAM);
 
     final Retrofit retrofit;
+    final Class<?> service;
     final Method method;
     final Annotation[] methodAnnotations;
     final Annotation[][] parameterAnnotationsArray;
@@ -168,8 +174,9 @@ static final class Builder {
     @Nullable ParameterHandler<?>[] parameterHandlers;
     boolean isKotlinSuspendFunction;
 
-    Builder(Retrofit retrofit, Method method) {
+    Builder(Retrofit retrofit, Class<?> service, Method method) {
       this.retrofit = retrofit;
+      this.service = service;
       this.method = method;
       this.methodAnnotations = method.getAnnotations();
       this.parameterTypes = method.getGenericParameterTypes();
diff --git a/retrofit/src/main/java/retrofit2/Retrofit.java b/retrofit/src/main/java/retrofit2/Retrofit.java
index a2b68f4d6d..c66f821251 100644
--- a/retrofit/src/main/java/retrofit2/Retrofit.java
+++ b/retrofit/src/main/java/retrofit2/Retrofit.java
@@ -174,7 +174,7 @@ public <T> T create(final Class<T> service) {
                 Reflection reflection = Platform.reflection;
                 return reflection.isDefaultMethod(method)
                     ? reflection.invokeDefaultMethod(method, service, proxy, args)
-                    : loadServiceMethod(method).invoke(args);
+                    : loadServiceMethod(service, method).invoke(proxy, args);
               }
             });
   }
@@ -205,13 +205,13 @@ private void validateServiceInterface(Class<?> service) {
         if (!reflection.isDefaultMethod(method)
             && !Modifier.isStatic(method.getModifiers())
             && !method.isSynthetic()) {
-          loadServiceMethod(method);
+          loadServiceMethod(service, method);
         }
       }
     }
   }
 
-  ServiceMethod<?> loadServiceMethod(Method method) {
+  ServiceMethod<?> loadServiceMethod(Class<?> service, Method method) {
     // Note: Once we are minSdk 24 this whole method can be replaced by computeIfAbsent.
     Object lookup = serviceMethodCache.get(method);
 
@@ -229,7 +229,7 @@ ServiceMethod<?> loadServiceMethod(Method method) {
         if (lookup == null) {
           // On successful lock insertion, perform the work and update the map before releasing.
           // Other threads may be waiting on lock now and will expect the parsed model.
-          ServiceMethod<Object> result = ServiceMethod.parseAnnotations(this, method);
+          ServiceMethod<Object> result = ServiceMethod.parseAnnotations(this, service, method);
           serviceMethodCache.put(method, result);
           return result;
         }
diff --git a/retrofit/src/main/java/retrofit2/ServiceMethod.java b/retrofit/src/main/java/retrofit2/ServiceMethod.java
index ea4330ed63..f3117e1186 100644
--- a/retrofit/src/main/java/retrofit2/ServiceMethod.java
+++ b/retrofit/src/main/java/retrofit2/ServiceMethod.java
@@ -22,8 +22,8 @@
 import javax.annotation.Nullable;
 
 abstract class ServiceMethod<T> {
-  static <T> ServiceMethod<T> parseAnnotations(Retrofit retrofit, Method method) {
-    RequestFactory requestFactory = RequestFactory.parseAnnotations(retrofit, method);
+  static <T> ServiceMethod<T> parseAnnotations(Retrofit retrofit, Class<?> service, Method method) {
+    RequestFactory requestFactory = RequestFactory.parseAnnotations(retrofit, service, method);
 
     Type returnType = method.getGenericReturnType();
     if (Utils.hasUnresolvableType(returnType)) {
@@ -39,5 +39,5 @@ static <T> ServiceMethod<T> parseAnnotations(Retrofit retrofit, Method method) {
     return HttpServiceMethod.parseAnnotations(retrofit, method, requestFactory);
   }
 
-  abstract @Nullable T invoke(Object[] args);
+  abstract @Nullable T invoke(Object instance, Object[] args);
 }
diff --git a/samples/src/main/java/com/example/retrofit/ErrorHandlingAdapter.java b/samples/src/main/java/com/example/retrofit/ErrorHandlingAdapter.java
index 1c7fb0e1c7..afc7d329b4 100644
--- a/samples/src/main/java/com/example/retrofit/ErrorHandlingAdapter.java
+++ b/samples/src/main/java/com/example/retrofit/ErrorHandlingAdapter.java
@@ -125,8 +125,8 @@ public void enqueue(final MyCallback<T> callback) {
             @Override
             public void onResponse(Call<T> call, Response<T> response) {
               // TODO if 'callbackExecutor' is not null, the 'callback' methods should be executed
-              // on that executor by submitting a Runnable. This is left as an exercise for the
-              // reader.
+              //  on that executor by submitting a Runnable. This is left as an exercise for the
+              //  reader.
 
               int code = response.code();
               if (code >= 200 && code < 300) {
@@ -145,8 +145,8 @@ public void onResponse(Call<T> call, Response<T> response) {
             @Override
             public void onFailure(Call<T> call, Throwable t) {
               // TODO if 'callbackExecutor' is not null, the 'callback' methods should be executed
-              // on that executor by submitting a Runnable. This is left as an exercise for the
-              // reader.
+              //  on that executor by submitting a Runnable. This is left as an exercise for the
+              //  reader.
 
               if (t instanceof IOException) {
                 callback.networkError((IOException) t);
diff --git a/samples/src/main/java/com/example/retrofit/InvocationMetrics.java b/samples/src/main/java/com/example/retrofit/InvocationMetrics.java
index a4d5d768af..0d491b76b6 100644
--- a/samples/src/main/java/com/example/retrofit/InvocationMetrics.java
+++ b/samples/src/main/java/com/example/retrofit/InvocationMetrics.java
@@ -55,7 +55,7 @@ public Response intercept(Chain chain) throws IOException {
       if (invocation != null) {
         System.out.printf(
             "%s.%s %s HTTP %s (%.0f ms)%n",
-            invocation.method().getDeclaringClass().getSimpleName(),
+            invocation.service().getSimpleName(),
             invocation.method().getName(),
             invocation.arguments(),
             response.code(),


Test patch:
diff --git a/retrofit/java-test/src/test/java/retrofit2/InvocationTest.java b/retrofit/java-test/src/test/java/retrofit2/InvocationTest.java
index 654d96fe3d..4fdebd0d4e 100644
--- a/retrofit/java-test/src/test/java/retrofit2/InvocationTest.java
+++ b/retrofit/java-test/src/test/java/retrofit2/InvocationTest.java
@@ -39,6 +39,8 @@ Call<ResponseBody> postMethod(
         @Path("p1") String p1, @Query("p2") String p2, @Body RequestBody body);
   }
 
+  interface ExampleSub extends Example {}
+
   @Test
   public void invocationObjectOnCallAndRequestTag() {
     Retrofit retrofit =
@@ -58,10 +60,52 @@ public void invocationObjectOnCallAndRequestTag() {
     assertThat(invocation.arguments()).isEqualTo(Arrays.asList("one", "two", requestBody));
   }
 
+  @Test
+  public void invocationCorrectlyIdentifiesServiceMethodInvocation() {
+    Retrofit retrofit =
+        new Retrofit.Builder()
+            .baseUrl("http://example.com/")
+            .callFactory(new OkHttpClient())
+            .build();
+
+    ExampleSub example = retrofit.create(ExampleSub.class);
+    RequestBody requestBody = RequestBody.create(MediaType.get("text/plain"), "three");
+    Call<ResponseBody> call = example.postMethod("one", "two", requestBody);
+
+    Invocation invocation = call.request().tag(Invocation.class);
+    assertThat(invocation.service()).isEqualTo(ExampleSub.class);
+    assertThat(invocation.instance()).isSameInstanceAs(example);
+    assertThat(invocation.method().getName()).isEqualTo("postMethod");
+    assertThat(invocation.method().getDeclaringClass()).isEqualTo(Example.class);
+    assertThat(invocation.arguments()).isEqualTo(Arrays.asList("one", "two", requestBody));
+  }
+
+  @Test
+  public void nullService() {
+    try {
+      Invocation.of(
+          null, new Object(), Object.class.getDeclaredMethods()[0], Arrays.asList("one", "two"));
+      fail();
+    } catch (NullPointerException expected) {
+      assertThat(expected).hasMessageThat().isEqualTo("service == null");
+    }
+  }
+
+  @Test
+  public void nullInstance() {
+    try {
+      Invocation.of(
+          Object.class, null, Object.class.getDeclaredMethods()[0], Arrays.asList("one", "two"));
+      fail();
+    } catch (NullPointerException expected) {
+      assertThat(expected).hasMessageThat().isEqualTo("instance == null");
+    }
+  }
+
   @Test
   public void nullMethod() {
     try {
-      Invocation.of(null, Arrays.asList("one", "two"));
+      Invocation.of(Object.class, new Object(), null, Arrays.asList("one", "two"));
       fail();
     } catch (NullPointerException expected) {
       assertThat(expected).hasMessageThat().isEqualTo("method == null");
@@ -70,6 +114,26 @@ public void nullMethod() {
 
   @Test
   public void nullArguments() {
+    try {
+      Invocation.of(Object.class, new Object(), Example.class.getDeclaredMethods()[0], null);
+      fail();
+    } catch (NullPointerException expected) {
+      assertThat(expected).hasMessageThat().isEqualTo("arguments == null");
+    }
+  }
+
+  @Test
+  public void deprecatedNullMethod() {
+    try {
+      Invocation.of(null, Arrays.asList("one", "two"));
+      fail();
+    } catch (NullPointerException expected) {
+      assertThat(expected).hasMessageThat().isEqualTo("method == null");
+    }
+  }
+
+  @Test
+  public void deprecatedNullArguments() {
     try {
       Invocation.of(Example.class.getDeclaredMethods()[0], null);
       fail();
diff --git a/retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java b/retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java
index 703133ea87..912b138404 100644
--- a/retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java
+++ b/retrofit/test-helpers/src/main/java/retrofit2/TestingUtils.java
@@ -31,7 +31,7 @@ static <T> Request buildRequest(Class<T> cls, Retrofit.Builder builder, Object..
 
     Method method = onlyMethod(cls);
     try {
-      return RequestFactory.parseAnnotations(retrofit, method).create(args);
+      return RequestFactory.parseAnnotations(retrofit, cls, method).create(null, args);
     } catch (RuntimeException e) {
       throw e;
     } catch (Exception e) {