Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add ErrorHandler for customization of RetrofitError behavior #174

Merged
merged 1 commit into from May 27, 2013

Conversation

3 participants
Contributor

sberan commented Mar 26, 2013

Add a new interface, ErrorHandler, which is responsible for throwing customized exceptions during synchronous request errors, or directing to the appropriate callback method for asynchronous request errors.

Currently, API users must catch RetrofitError and try to determine the cause of the error based on certain properties of the HTTP response. This interface allows API clients to expose semantic exceptions to their users.

For example, if I had an API interface method to create a User, I might expose an InvalidUserException which could be declared on the createUser method, and used to indicate that something was invalid about the request.

This implementation allows exceptions to be checked or unchecked, leaving that decision up to the API designer. It also doesn't change any existing behavior. Let me know if there's anything I can do to clean it up!

Contributor

sberan commented Apr 1, 2013

Is there any way I can help this move along? Documentation? Different approach entirely?

Collaborator

JakeWharton commented Apr 1, 2013

I am working on changing the architecture of how requests get executed altogether. I don't want to merge this when the API most likely will be forced to change.

Contributor

sberan commented May 13, 2013

Congrats on the 1.0! I'm going to get this updated to the latest code line shortly.

Contributor

sberan commented May 24, 2013

@JakeWharton This is updated and tested to work against the latest codebase.

Contributor

adriancole commented May 24, 2013

I like the idea here. Only thing I'd mention is that sometimes a fallback is required. For example, if you have boolean userExists(String user) then you could configure via this or another mechanism the ability to coerce 404 to false. Do you think it should be in the ErrorHandler, or something else like @Fallback(NotFoundToFalse.class)

Contributor

sberan commented May 24, 2013

@adriancole An interesting idea! I think one could make a case that this behavior belongs in ErrorHandler, since the user is handling an HTTP error, and making a decision to swallow that error.

Another thing this would enable is completely ignoring an error - with the current implementation it is not possible to ignore an error. With a small tweak, these use cases would be possible.

Contributor

adriancole commented May 24, 2013

I use the following fallbacks quite regularly in other codebases. Particularly, these help avoid needing to constantly check null.

  • NullOn404 - ex. return null when asking for an object by id
  • VoidOn404 - ex. delete operation where the thing you were deleting
    didn't exist
  • FalseOn404 - ex. thingExists?
  • EmptyListOn404 - ex. list
  • EmptyMapOn404 - ex. list where the response is more naturally a map

food for thought!

Contributor

sberan commented May 24, 2013

@adriancole I slightly modified the ExceptionHandler interface to allow an implementation to return a default value instead of throwing an exception.

Contributor

adriancole commented May 24, 2013

comments in!

Contributor

sberan commented May 24, 2013

@adriancole I slightly disprefer the propogateOrFallback method name because it breaks the congruity with the asynchronous method's name. I updated the comments to more clearly define the terms of the contract.

Contributor

adriancole commented May 24, 2013

sounds like a plan. @JakeWharton you have any opinions on this? I'd love to use it this weekend.

Contributor

adriancole commented May 24, 2013

last "prefer" is to squash the commits into one.

@JakeWharton JakeWharton commented on an outdated diff May 25, 2013

retrofit/src/main/java/retrofit/ErrorHandler.java
+ * thrown on the interface method.
+ *
+ * @param cause the original RetrofitError exception
+ * @return a fallback object to be returned from the client interface method
+ * @throws Throwable an exception which will be thrown from the client interface method
+ */
+ Object handleError(RetrofitError cause) throws Throwable;
+
+ /**
+ * Called when errors occur during asynchronous requests. This method is responsible
+ * for calling the appropriate failure method in the callback class.
+ *
+ * @param e the original RetrofitError exception
+ * @param callback the callback which was passed to the interface method
+ */
+ void handleErrorCallback(RetrofitError e, Callback<?> callback);
@JakeWharton

JakeWharton May 25, 2013

Collaborator

I don't think we need this. You could provide an abstract class which implements Callback for behavior customization. We do something like this:

public abstract class RestCallback<T> implements Callback<T> {
  @Override public final void failure(RetrofitError e) {
    Response r = e.getResponse();
    if (r != null) {
      int status = r.getStatus();
      if (status >= 500) {
        serverError(r);
      } else if (status == 401) {
        sessionExpired(r);
      } else if (status >= 400) {
        clientError(r);
      } else {
        throw new AssertionError(e);
      }
    } else if (e.isNetworkError()) {
      networkError(e.getCause());
    } else {
      unexpectedError(e.getCause());
    }
  }

  public abstract void sessionExpired(Response r);
  public abstract void clientError(Response r);
  public abstract void serverError(Response r);
  public abstract void networkError(Throwable e);
  public abstract void unexpectedError(Throwable e);
}

@JakeWharton JakeWharton and 2 others commented on an outdated diff May 25, 2013

retrofit/src/test/java/retrofit/ErrorHandlerTest.java
+import retrofit.http.Part;
+
+import java.io.IOException;
+import java.util.Collections;
+
+import static org.fest.assertions.api.Assertions.assertThat;
+import static org.junit.Assert.fail;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.verify;
+
+public class ErrorHandlerTest {
+
+
+ interface ExampleClient {
+ @POST("/") @Multipart
+ Void create(@Part("object") Object toCreate) throws InvalidObjectException;
@JakeWharton

JakeWharton May 25, 2013

Collaborator

Lowercase 'v' void

@adriancole

adriancole May 25, 2013

Contributor

I think we should support lower "void" conceding perhaps not as a part of this PR

@JakeWharton

JakeWharton May 25, 2013

Collaborator

Returning Void is weird too. Can you just switch it to Response for clarity that it's synchronous?

Collaborator

JakeWharton commented May 25, 2013

I think I'm onboard with the concept of customizing the exception thrown during synchronous invocation. As I mention above, I don't think the asynchronous one is necessary, though. Can we remove that and then I'll take a deeper look at the implementation?

Contributor

adriancole commented May 25, 2013

I'm also cool with sync-only. Solves my problems ( :) ) and allows us to move this change forward.

Contributor

sberan commented May 25, 2013

I removed the async handling - you're right that it's not necessary. I also changed the method to return a String and added a test for the fallback case as well.

I'm wondering if I should pass along some more information to the ErrorHandler as well - in order to support @adriancole 's Empty*On404 cases, the return type of the method would need to be known.

Collaborator

JakeWharton commented May 25, 2013

You have checkstyle errors:

src/main/java/retrofit/ErrorHandler.java:16: Line has trailing spaces.

@JakeWharton JakeWharton commented on an outdated diff May 25, 2013

retrofit/src/main/java/retrofit/ErrorHandler.java
+public interface ErrorHandler {
+ /**
+ * Called when errors occur during synchronous requests. The implementer
+ * may choose to propagate a custom exception, or return a fallback value.
+ *
+ * If the exception is checked, any thrown exceptions must be declared to be
+ * thrown on the interface method.
+ *
+ * @param cause the original RetrofitError exception
+ * @return a fallback object to be returned from the client interface method
+ * @throws Throwable an exception which will be thrown from the client interface method
+ */
+ Object handleError(RetrofitError cause) throws Throwable;
+
+ ErrorHandler DEFAULT = new ErrorHandler() {
+ @Override
@JakeWharton

JakeWharton May 25, 2013

Collaborator

This annotation should be on the same line as the method declaration below.

Contributor

adriancole commented May 25, 2013

Agreed. We will need a TypeToken available, either explicitly as a parameter, or in scope for the implementation (ex ThreadLocal)

Contributor

sberan commented May 26, 2013

@adriancole @JakeWharton cleaned up checkstyle errors, and added the interface method as a param to the ErrorHandler hook.

I also renamed the interface method to propagateOrFallback at @adriancole's recommendation.

Contributor

adriancole commented May 26, 2013

@sberan I'm going to try and make a realistic test case that uses the Method param.. I think we'll only need TypeToken or Class

Contributor

adriancole commented May 26, 2013

OK I've started playing around with the commit. I'm taking the following client into consideration

  interface ExampleClient {
    @GET("/")
    String throwsCustomException() throws IllegalStateException;

    @HEAD("/")
    boolean exists();
  }

I'd revise the naming and args slightly for ErrorHandler

Object fallbackOrPropagate(RetrofitError cause, Type type); // ex. convert response.status 404 -> false

With this together with Converter, I can write a fairly sensibly coupled system to account for these concerns:

    public class MyConverter implements ErrorHandler extends GsonConverter {
      @Override
      Object fromBody(TypedInput body, Type type) throws ConversionException {
        if (type == boolean.class)
          return true;
        else if (type == String.class)
          return new String(Utils.streamToBytes(body.in()));
        return super.fromResponse(response, type);
      }

      @Override
      public Object fallbackOrPropagate(RetrofitError cause, Type type) throws Throwable {
        if (cause.getResponse().getStatus() == 404 && type == boolean.class)
          return false;
        else if (cause.getResponse().getStatus() == 409)
          throw new IllegalStateException(cause.getMessage());
        throw cause;
      }
    }

Thoughts?

Contributor

adriancole commented May 26, 2013

note (to those reading email) I revised my comment on github

Contributor

adriancole commented May 26, 2013

revised my comments as there's nothing in this issue that requires action in issue #224

Contributor

sberan commented May 26, 2013

My idea with passing the entire method was that additional information such as annotations could be useful.

Contributor

adriancole commented May 26, 2013

Personally I'd rather have type and also the ability to set a method-scoped
fallback :). Lacking a method-scoped fallback, I can see how we'd need
reflection more often.

I think in the future we will have a compile processor, so probably better
to avoid passing things into user Apis that facilitate more reflection.

Wdyt?

Contributor

adriancole commented May 26, 2013

@sberan I'll push a branch showing what I mean shortly.

Contributor

adriancole commented May 26, 2013

here's what I was thinking.
adriancole/retrofit@master...exceptionHandler

This ensures reflection stuff only happens once (restMethodInfo), and allows the fallback to be specified on a method.

ex.

  static interface FallbackClient {
    @GET("/")
    boolean globalFallback();

    @GET("/")
    @Fallback(FalseOn404.class)
    boolean methodFallback();
  }

  static class FalseOn404 implements FallbackHandler  {
    public Object fallbackOrPropagate(Type type, RetrofitError error) throws Throwable {
      if (error.getResponse().getStatus() == 404 && type == boolean.class)
        return false;
      throw error;
    }
  }

feel free to cherry-pick and squash the commit into yours, if helpful.

@JakeWharton JakeWharton commented on an outdated diff May 26, 2013

retrofit/src/main/java/retrofit/ErrorHandler.java
+ * @author Sam Beran sberan@gmail.com
+ */
+public interface ErrorHandler {
+ /**
+ * Called when errors occur during synchronous requests. The implementer
+ * may choose to propagate a custom exception, or return a fallback value.
+ *
+ * If the exception is checked, any thrown exceptions must be declared to be
+ * thrown on the interface method.
+ *
+ * @param cause the original RetrofitError exception
+ * @param interfaceMethod the client interface method which invoked this request
+ * @return a fallback object to be returned from the client interface method
+ * @throws Throwable an exception which will be thrown from the client interface method
+ */
+ Object propagateOrFallback(RetrofitError cause, Method interfaceMethod) throws Throwable;
@JakeWharton

JakeWharton May 26, 2013

Collaborator

Remove the Method argument. Code generated implementations of interfaces won't have access.

Collaborator

JakeWharton commented May 26, 2013

I'm still not convinced this needs to go in. It seems like it's trying to place far too much logic inside Retrofit where the same could just as easily be accomplished with a more intelligent wrapper class.

interface FooService {
  @GET("/") String foo();
}
class FooServiceWrapper implements FooService {
  @Inject FooService delegate;
  @Inject @DefaultFoo String defaultFoo;

  @Override public String foo() {
    try {
      return delegate.foo();
    } catch (RetrofitError e) {
      Response r = e.getResponse();
      if (r != null && r.getStatus() == 404) {
        return defaultFoo;
      }
      throw e;
    }
  }
}
Contributor

adriancole commented May 26, 2013

thanks for the advice @JakeWharton I'll use the wrapper approach until/unless this goes in.

Contributor

adriancole commented May 26, 2013

@JakeWharton @swankjesse on this note.. sounds like we should add an "idea graveyard" wiki and/or issue tag.

Contributor

sberan commented May 26, 2013

@JakeWharton I definitely see your point, but it feels like exception behavior is a similar level of abstraction to return value / parameter conversion. It would be nice to not have to wrap all APIs with wrapper just to handle errors, when everything else can be handled via conversion.

Contributor

sberan commented May 26, 2013

I removed the Method parameter from the ErrorHandler interface.

Contributor

adriancole commented May 26, 2013

yeah the downside of the wrapper approach is it highlights a mix of abstractions. For example, our return val is a domain-specific object, yet we need to deal with http-specific exceptions.

Contributor

adriancole commented May 26, 2013

FWIW, I'm cool with using wrapper to do fallbacks so we can at least support application-specific exceptions in retrofit. How about if we rewind to something like:

interface ErrorHandler {
   Throwable handle(RetrofitError error);
}
Collaborator

JakeWharton commented May 26, 2013

I think that's a reasonable starting point (and it'll work with code generation!). Generating a more-specific error type is definitely useful when dealing with the synchronous API.

Add hook for customizing exceptions
The exceptions can be customized via a new ErrorHandler class, which is
responsible for returning customized exceptions during synchronous
requests.
Contributor

sberan commented May 26, 2013

@JakeWharton fantastic to hear. I really like how this ended up, API wise. I've changed the ErrorHandler interface to return an exception to propagate.

Out of curiosity, what was the issue with code generation compatibility?

Collaborator

JakeWharton commented May 27, 2013

What happens if I throw a checked exception in the handler that wasn't declared on the interface method?

Contributor

sberan commented May 27, 2013

@JakeWharton JakeWharton merged commit 054d165 into square:master May 27, 2013

1 check passed

default The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment