Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handleErrorOrRetry does not work when errors are returned in GraphQL response body #627

Closed
rajkhatri opened this issue Jul 11, 2019 · 11 comments
Labels
bug Generally incorrect behavior
Milestone

Comments

@rajkhatri
Copy link

@designatednerd I want to first thank you for getting out the latest HTTPNeworkTransport Delegates.

I just am not sure if the below function works

  func networkTransport(_ networkTransport: HTTPNetworkTransport,
                        receivedError error: Error,
                        for request: URLRequest,
                        response: URLResponse?,
                        retryHandler: @escaping (_ shouldRetry: Bool) -> Void)

According to graphQL

Notice that HTTP status codes are not relevant when using GraphQL! A GraphQL server will always respond with code 200 (unless an internal server occured, then it might be code 500). Any further information needs to be parsed from the JSON payload of the response.

Here is a screenshot from my GraphQL server with an invalid token error, returns a 401 in the errors array, but because it returns a 200, it doesn't seem to call the above function, and is actually considered a success.

Screenshot 2019-07-11 at 1 02 27 PM

I'm not sure if this is a limitation on your work, or from the existing work on how errors are handled by apollo-ios:

public func send<Operation>(operation: Operation, completionHandler: @escaping (_ response: GraphQLResponse<Operation>?, _ error: Error?) -> Void) -> Cancellable

Any insight would be appreciated.

Thanks

@designatednerd
Copy link
Contributor

ARGH. Yeah we're getting everything up to parsing errors but because GQL can potentially return a partial result and a partial error, I'll have to think through how to handle this.

For clarity, are you using an Apollo server or a different server?

@designatednerd designatednerd added the bug Generally incorrect behavior label Jul 12, 2019
@rajkhatri
Copy link
Author

Yea, wondering if parsing through each response for a non-empty error array is effective. Thats currently what we do on iOS on the response of each specific call.

We use absinthe and the elixir language on phoenix to run GraphQL. And of course Apollo-iOS 0.11.0 for our app.

Right now as a workaround for 401’s, we are checking the expiry for auth before sending the call, authenticating and then making the request.

@designatednerd
Copy link
Contributor

Checking the auth expiration using the pre-flight delegate is better if you have access to it - that way you don't burn the data/battery of making a call just to have to make two more calls (re-auth and retry).

The problem with offering retry on a non-empty error array is that a non-empty error array doesn't necessarily indicate a total failure. Because GraphQL can return a partial result with partial errors, retry may not be the appropriate response for the partial errors.

I'm gonna mull this over more on Monday.

@designatednerd designatednerd changed the title handleErrorOrRetry does not work handleErrorOrRetry does not work when errors are returned in GraphQL response body Jul 15, 2019
@designatednerd
Copy link
Contributor

I decided I'm going to leave this open, but it will probably be a bit before I address it. I need to take a deeper look at how we're handling results, especially with partial failure/partial success, since that has a ton to do with how to handle issues like this.

For now, I'd keep doing what you're doing - like I said, checking that expiration pre-flight is usually a much better way of making sure you're not making unnecessary calls.

@rajkhatri
Copy link
Author

sounds good! thanks for keeping it open and addressing it when you are able to

@mdilaveroglu
Copy link

mdilaveroglu commented Jul 31, 2019

Hey @rajkhatri

Can you show me an example go how you are handling token expiration on pre-flight? I can't manage to do it somehow.

@designatednerd
Copy link
Contributor

Please take a look at PR #770 which introduces a delegate to handle errors in the GraphQL response body

@designatednerd designatednerd added this to the 0.19.0 milestone Oct 29, 2019
@designatednerd
Copy link
Contributor

This has now shipped with 0.19.0 - give CocoaPods about half an hour to update trunk and the CDN, but for SPM or Carthage this is available immediately.

@nexig
Copy link

nexig commented Dec 3, 2019

@designatednerd I tested it on version 0.20.0 and I see it still not working. I receive response from backend with status code 200 but with 401 error in response body. I see that method from HTTPNetworkTransportRetryDelegate is not called. Should I implement it in another way?

@designatednerd
Copy link
Contributor

@nexig Yes, you should use the HTTPNetworkTransportGraphQLErrorDelegate, it's a separate method which handles GraphQL errors specifically.

@nexig
Copy link

nexig commented Dec 4, 2019

@designatednerd thanks for help, it works well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Generally incorrect behavior
Projects
None yet
Development

No branches or pull requests

4 participants