Response body empty when HttpException throw

[sav007]

Closes #361

@mandrizzle

[mandrizzle]

Is is a good idea to not close the body of the response? I am not sure of the implications of not doing so. Personally, I would have changed the HttpException class to have a static method that looks like:

  public static HttpException from(Response response) throws IOException {
     try {
       String body = response.body() != null ? response.body().string() : "";
       return new HttpException(response, response.code(), response.message(), body);
     } finally {
       closeQuietly(response);
     }
   }

So that the body does get closed but before it does, we have copied over the contents of it. Also IIRC, the ResponseBody in the Response object is what gets closed when close is called. So if we read and copy the contents over, it is probably safe to close.

[mandrizzle]

Just to add, calling string() does close ResponseBody but if no one was to call it, it would not close.

[mandrizzle]

Technically Exceptions should be Serializable. Since Response isn't Serializable, this is a good time to add transient to its definition

[mandrizzle]

Just throwing out an idea here. What if we reserve onFailure for IOExceptionsaka network issues but repurpose graphql.Response to also return server errors?

I mention this because as of right now, I think the public api for the ApolloClient is easy to screw up.

1. onFailure passes in a Throwable which means one needs to be aware to cast what is passed in back to an HttpException to be able to read server errors.

2. After thinking about it, I think we shouldn't close the ResponseBody and instead, leave it for the consumer to close it. Because the consumer might want to read in the ResponseBody from the error via BufferedSource rather than as a String. Now the problem I see is, since the ResponseBody that we need to close is part of the Exception that is thrown, it can easily be ignored or lost which means people might see this

3. There are other implications having non 200 status codes being returned as exceptions. One that I can think of is how a server error can kill a rx stream from the RxApollo package when really calling onError in streams should be for fatal, unrecoverable, exceptions. See here for more details on that.

Thoughts on this? I realize it might require a bigger, breaking, change but may be worth it.

[sav007]

@mandrizzle Thanks, these are good points:

What if we reserve onFailure for IOExceptionsaka network issues but repurpose graphql.Response to also return server errors

1 onFailure passes in a Throwable which means one needs to be aware to cast what is passed in back to an HttpException to be able to read server errors.

Except IOException we can potentially have: JsonDataException, JsonEncodingException.
I suggest to not include any exceptions in graphql.Response as this object represents the real response from server (with errors returned by GraphQL). But I suggest to wrap all Exceptions into some thing like checked ApolloException. So OnFailure will always get the ApolloException with cause == original one

2 After thinking about it, I think we shouldn't close the ResponseBody and instead, leave it for the consumer to close it. Because the consumer might want to read in the ResponseBody from the error via BufferedSource rather than as a String. Now the problem I see is, since the ResponseBody that we need to close is part of the Exception that is thrown, it can easily be ignored or lost which means people might see this

That the tradeoff of giving user to parse error body. And this is the contract that user must follow always close ApolloException

3 There are other implications having non 200 status codes being returned as exceptions. One that I can think of is how a server error can kill a rx stream from the RxApollo package when really calling onError in streams should be for fatal, unrecoverable, exceptions. See here for more details on that.

That's issue of Rx wrapper if user don't want to break the rx chain he must wrap the error (for example with onErrorResumeNext or smth). So this is not part of ApolloClient itself as it's a pure java module, but we can think about this in context of our RxWrapper