Apollo-android: Consider wrapping any exception with ApolloException

[sav007]

The idea is to introduce new checked exception ApolloException and wrap any exception thrown at runtime with this class.

Change the signature of com.apollographql.android.ApolloCall#execute and com.apollographql.android.ApolloCall.Callback#onFailure to:

@Nonnull Response<T> execute() throws ApolloException;

and 

void onFailure(@Nonnull ApolloException t);

Introduce 3 codes/types of the ApolloException:

• ApolloException.PARSE represents parsing response error (like malformed response, etc.)
• ApolloException.HTTP represents any network errors (like statusCode != 200, timeout etc.)
• ApolloException.RUNTIME any other errors

[sav007]

@mandrizzle @digitalbuddha @BenSchwab @marwanad does this suggestion make sense?

[mandrizzle]

I like the idea. I do have some questions/suggestions

1. Why use an enum PARSE, HTTP, RUNTIME to determine the type of exception rather than making them subclasses: ApolloParseException, ApolloHttpException?

2. I think it isn't clear why we have a onFailure that passes in an ApolloException when execute also throws an ApolloException. If we were to do what I suggested above we could change the signature to look like:

@Nonnull Response<T> execute() throws ApolloParseException, ApolloServerErrorException;

void onFailure(@NonNull IOException e);

Which I think is a much clearer API not to mention, we can leverage java's exception handling syntax this way:

try {
    return client.execute()
} catch (ApolloParseException ape) {
    return ...;
} catch (ApolloServerErrorException asee) {
     if (asee.code() == 401) {
       ...
     }
}

vs doing

try {
    Response response = client.execute()
} catch (ApolloException ae) {
    switch(ae.getType()) {
    case HTTP:
         if (asee.code() == 401) {
           ...
         }
     break;
     ...
}

Thoughts?

[sav007]

@mandrizzle

Why use an enum PARSE, HTTP, RUNTIME to determine the type of exception rather than making them subclasses: ApolloParseException, ApolloHttpException?

yeah we can do this

I think it isn't clear why we have a onFailure that passes in an ApolloException when execute also throws an ApolloException

execute - is for sync request
onFailure - is used inside callback that passed as parameter to enqueue that guarantees to not throw exception but return them in callback

[mandrizzle]

onFailure - is used inside callback that passed as parameter to enqueue that guarantees to not throw exception but return them in callback

Ohh that makes more sense, for some reason I thought they were part of the same interface 💁‍♂️

Anyways, the retrofit community had a similar discussion about this when they were planning retrofit2 one of the proposed ideas was to have a callback that had more callbacks that looks something like this:

public interface Callback<T> {
  void success(T result); // 2XX responses
  void networkError(IOException e); // IOException from underlying input stream
  void unexpectedError(Exception e); // Error from converter or internal Retrofit error.
  void httpError(int status, ...); // non-2XX responses
}

source

I think there is an opportunity here to make a clear api without making a breaking change for the enqueue method.

Just throwing out an idea, suppose we change the Callback interface from this:

interface Callback<T> {
   void onResponse(@Nonnull Response<T> response);
   void onFailure(@Nonnull Throwable t);
}

To this:

public abstract class Callback<T> {
  // The required API is still the same
  public abstract void onResponse(@Nonnull Response<T> response);
  public abstract void onFailure(@Nonnull Throwable t);

  // Optional callbacks, route to onFailure if not overridden
  public void networkError(IOException e) {
    onFailure(e);
  }
  public void parseError(ApolloParseException e) {
    onFailure(e);
  }
  public void httpError(int status, ResponseBody body, ApolloServerErrorException e) {
    onFailure(e);
  }
}

The user then has the option to implement whatever they choose, while at the same time, we are minimizing the impact of this change