Apollo-android: Rx Support calls leave errors unreachable

[kassim]

When errors are thrown on a GraphQL request, the response is often still HTTP code 200 from the server, and the client is expected to look at the "errors" section of the JSON response to understand that an error has occurred.

In the current implementation we pass the result through the call emitter.onSuccess(response.data()); we prevent the user from checking these errors.

I recommend checking response.isSuccessful() first, and passing the response wrapped in an exception to emitter.onError() if response.isSuccessful() == false. Or alternatively the entire response object should be passed to onSuccessful.

[VisheshVadhera]

Oh yeah, Rx wrappers are not propagating the errors back to the user if at all an error occurs. Though I feel they certainly should be propagated back to the users via onError, not onSuccess.

[sav007]

I guess we need to pass the whole Response object and remove com.apollographql.apollo.api.Response#isSuccessful at all. GraphQL response can return data and errors at the same response (they are not mutual exclusive). So really we can't decide if response was successful by checking if errors is empty, it depends of the query. We have couple queries at Shopify when mutations was success but at the same time we get some error (like a warning). So in response we have both data and errors.

[VisheshVadhera]

Hmm, it's fair to say that if these two are not mutually exclusive, then it makes sense to send the entire response back to the user and let him/her handle the error. This would also mean making it explicitly clear in the documentation that onError will not be called when the response object contains errors.

But on the other hand, this would also mean that the users will have to do if else check for errors for every query that they make even when there are no errors, which will make code tedious on the clients end. Sending the errors down through onError and sending the results through onSuccess would certainly prevent this problem.

It's a choice which we will have to make.

[digitalbuddha]

I'd really want to send errors to onError. Personally I would hate to check error cases I'm success. For one thing it leads to inefficiency if I have a long observable chain that now goes through a bunch of operators only to be handled as error in subscribe.

[sav007]

But how can you send OnSuccess and OnError at the same time?

[VisheshVadhera]

How about making the wrapper for apolloCall return an Observable instead of a Single? This would make perfect sense for the kind of problem we are seeing right now. Right now we have the following cases to tackle:

• Response contains only data: Return the data to client via onNext followed by onComplete which would complete the stream.
• Response contains only errors: Return the errors via onError. This would immediately close the stream.
• Response contains errors as well as data: First return the data via onNext, then grab the errors and return them via onError. onComplete isn't required here at all because the stream gets closed as soon as you send errors down the stream. I feel this is perfectly fine to do, because we can have streams containing data as well as errors.

What are your thoughts?

[kassim]

I don't think that works, most people would want to know about the presence of errors before checking the data response I think.

I don't know how other implementations are done, but in ours if there's an error, then a field in the response will be null where it shouldn't normally be - and as a default, instead of checking every field for null I'd rather show an error screen (and if I want to show a partial response then do it on a case-by-case basis).

While my preference is to throw it in OnError when there are errors (and that's how it worked for me before switching from my wrapper to this Apollo one), its acceptable to assume that some developers will use the "errors" field differently. Perhaps we need two methods, or a boolean argument to say whether the presence of "errors" means the call was unsuccessful.

[sav007]

First not sure that this is good idea converting to Observable. As the call represents one shot query, so it should be following the concept of Single (you can get onSuccess or onError).

Success of the request should be defined by user not by Apollo. Why not just return the response itself and then user will decide how to check if the response success or not:

RxApollo
        .from(apolloClient.newCall(query))
        .flatMap(response -> response.hasErrors() ? Single.error(....) : Single.just(response))

[kassim]

Success of the request should be defined by user not by Apollo.

Agreed. And I also agree with

I guess we need to pass the whole Response object and remove com.apollographql.apollo.api.Response#isSuccessful

Apollo shouldn't be defining what success is or isn't. If the GraphQL spec changes and defines it then maybe.