Merge pull request square#439 from adriancole/adrian.fix-423

Use ErrorHandler to coerce error responses during async invocations.

retrofit/src/main/java/retrofit/CallbackRunnable.java

@@ -17,6 +17,8 @@
 
 import java.util.concurrent.Executor;
 
+import static retrofit.RetrofitError.unexpectedError;
+
 /**
  * A {@link Runnable} executed on a background thread to invoke {@link #obtainResponse()} which
  * performs an HTTP request. The response of the request, whether it be an object or exception, is
@@ -26,10 +28,12 @@
 abstract class CallbackRunnable<T> implements Runnable {
   private final Callback<T> callback;
   private final Executor callbackExecutor;
+  private final ErrorHandler errorHandler;
 
-  CallbackRunnable(Callback<T> callback, Executor callbackExecutor) {
+  CallbackRunnable(Callback<T> callback, Executor callbackExecutor, ErrorHandler errorHandler) {
     this.callback = callback;
     this.callbackExecutor = callbackExecutor;
+    this.errorHandler = errorHandler;
   }
 
   @SuppressWarnings("unchecked")
@@ -41,10 +45,12 @@ abstract class CallbackRunnable<T> implements Runnable {
           callback.success((T) wrapper.responseBody, wrapper.response);
         }
       });
-    } catch (final RetrofitError e) {
+    } catch (RetrofitError e) {
+      Throwable cause = errorHandler.handleError(e);
+      final RetrofitError handled = cause == e ? e : unexpectedError(e.getUrl(), cause);
       callbackExecutor.execute(new Runnable() {
         @Override public void run() {
-          callback.failure(e);
+          callback.failure(handled);
         }
       });
     }

retrofit/src/main/java/retrofit/ErrorHandler.java

@@ -16,7 +16,7 @@
 package retrofit;
 
 /**
- * A hook allowing clients to customize error exceptions for synchronous requests.
+ * A hook allowing clients to customize {@link retrofit.client.Response response} exceptions.
  *
  * @author Sam Beran [email protected]
  */
@@ -41,8 +41,8 @@ public interface ErrorHandler {
    * </pre>
    *
    * @param cause the original {@link RetrofitError} exception
-   * @return Throwable an exception which will be thrown from the client interface method. Must not
-   *         be {@code null}.
+   * @return Throwable an exception which will be thrown from a synchronous interface method or
+   *         passed to an asynchronous error callback. Must not be {@code null}.
    */
   Throwable handleError(RetrofitError cause);
 

retrofit/src/main/java/retrofit/RestAdapter.java

@@ -169,18 +169,18 @@ private RestAdapter(Endpoint server, Client.Provider clientProvider, Executor ht
     this.server = server;
     this.clientProvider = clientProvider;
     this.httpExecutor = httpExecutor;
-    if (Platform.HAS_RX_JAVA && httpExecutor != null) {
-      this.rxSupport = new RxSupport(httpExecutor);
-    } else {
-      this.rxSupport = null;
-    }
     this.callbackExecutor = callbackExecutor;
     this.requestInterceptor = requestInterceptor;
     this.converter = converter;
     this.profiler = profiler;
     this.errorHandler = errorHandler;
     this.log = log;
     this.logLevel = logLevel;
+    if (Platform.HAS_RX_JAVA && httpExecutor != null) {
+      this.rxSupport = new RxSupport(httpExecutor, errorHandler);
+    } else {
+      this.rxSupport = null;
+    }
   }
 
   /** Change the level of logging. */
@@ -229,9 +229,11 @@ static RestMethodInfo getMethodInfo(Map<Method, RestMethodInfo> cache, Method me
   /** Indirection to avoid VerifyError if RxJava isn't present. */
   private static final class RxSupport {
     private final Scheduler scheduler;
+    private final ErrorHandler errorHandler;
 
-    RxSupport(Executor executor) {
+    RxSupport(Executor executor, ErrorHandler errorHandler) {
       this.scheduler = Schedulers.executor(executor);
+      this.errorHandler = errorHandler;
     }
 
     Observable createRequestObservable(final Callable<ResponseWrapper> request) {
@@ -248,7 +250,7 @@ Observable createRequestObservable(final Callable<ResponseWrapper> request) {
             subscriber.onNext(wrapper.responseBody);
             subscriber.onCompleted();
           } catch (RetrofitError e) {
-            subscriber.onError(e);
+            subscriber.onError(errorHandler.handleError(e));
           } catch (Exception e) {
             // This is from the Callable.  It shouldn't actually throw.
             throw new RuntimeException(e);
@@ -307,7 +309,7 @@ private class RestHandler implements InvocationHandler {
       }
 
       Callback<?> callback = (Callback<?>) args[args.length - 1];
-      httpExecutor.execute(new CallbackRunnable(callback, callbackExecutor) {
+      httpExecutor.execute(new CallbackRunnable(callback, callbackExecutor, errorHandler) {
         @Override public ResponseWrapper obtainResponse() {
           return (ResponseWrapper) invokeRequest(interceptorTape, methodInfo, args);
         }

retrofit/src/test/java/retrofit/CallbackRunnableTest.java

@@ -4,10 +4,6 @@
 import java.util.concurrent.Executor;
 import org.junit.Before;
 import org.junit.Test;
-import retrofit.Callback;
-import retrofit.CallbackRunnable;
-import retrofit.ResponseWrapper;
-import retrofit.RetrofitError;
 
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.same;
@@ -21,10 +17,11 @@ public class CallbackRunnableTest {
   private Executor executor = spy(new SynchronousExecutor());
   private CallbackRunnable<Object> callbackRunnable;
   private Callback<Object> callback;
+  private ErrorHandler errorHandler = ErrorHandler.DEFAULT;
 
   @Before public void setUp() {
     callback = mock(Callback.class);
-    callbackRunnable = spy(new CallbackRunnable<Object>(callback, executor) {
+    callbackRunnable = spy(new CallbackRunnable<Object>(callback, executor, errorHandler) {
       @Override public ResponseWrapper obtainResponse() {
         return null; // Must be mocked.
       }

retrofit/src/test/java/retrofit/ErrorHandlerTest.java

@@ -10,8 +10,11 @@
 import retrofit.client.Request;
 import retrofit.client.Response;
 import retrofit.http.GET;
+import rx.Observable;
+import rx.Observer;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.failBecauseExceptionWasNotThrown;
 import static org.junit.Assert.fail;
 import static org.mockito.Matchers.any;
 import static org.mockito.Mockito.doReturn;
@@ -22,6 +25,12 @@ public class ErrorHandlerTest {
   interface ExampleClient {
     @GET("/")
     Response throwsCustomException() throws TestException;
+
+    @GET("/")
+    void onErrorWrappedCustomException(Callback<Response> callback);
+
+    @GET("/")
+    Observable<Response> onErrorCustomException();
   }
 
   static class TestException extends Exception {
@@ -55,12 +64,47 @@ static class MockInvalidResponseClient implements Client {
 
     try {
       client.throwsCustomException();
-      fail();
+      failBecauseExceptionWasNotThrown(TestException.class);
     } catch (TestException e) {
       assertThat(e).isSameAs(exception);
     }
   }
 
+  @Test public void onErrorWrappedCustomException() throws Throwable {
+    final TestException exception = new TestException();
+    doReturn(exception).when(errorHandler).handleError(any(RetrofitError.class));
+
+    client.onErrorWrappedCustomException(new Callback<Response>() {
+
+      @Override public void success(Response response, Response response2) {
+        failBecauseExceptionWasNotThrown(TestException.class);
+      }
+
+      @Override public void failure(RetrofitError error) {
+        assertThat(error.getCause()).isSameAs(exception);
+      }
+    });
+  }
+
+  @Test public void onErrorCustomException() throws Throwable {
+    final TestException exception = new TestException();
+    doReturn(exception).when(errorHandler).handleError(any(RetrofitError.class));
+
+    client.onErrorCustomException().subscribe(new Observer<Response>() {
+      @Override public void onCompleted() {
+        failBecauseExceptionWasNotThrown(TestException.class);
+      }
+
+      @Override public void onError(Throwable e) {
+        assertThat(e).isSameAs(exception);
+      }
+
+      @Override public void onNext(Response response) {
+        failBecauseExceptionWasNotThrown(TestException.class);
+      }
+    });
+  }
+
   @Test public void returningNullThrowsException() throws Exception {
     doReturn(null).when(errorHandler).handleError(any(RetrofitError.class));