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

returning different error in -[HTTPNetworkTransportRetryDelegate networkTransport:receivedError:for:response:retryHandler] #1121

Closed
RolandasRazma opened this issue Apr 3, 2020 · 7 comments

Comments

@RolandasRazma
Copy link
Contributor

RolandasRazma commented Apr 3, 2020

The problem I'm facing:
doing apolloClient.fetch()
request fails for reason Y (getting 401 to delegate)
trying to update token
token update fails for reason X
calling retryHandler(false)
request fails and callback in apolloClient.fetch() see error Y, where real error is X

What I would like to do is to modify (add new one and deprecate existing one) -[HTTPNetworkTransportRetryDelegate networkTransport:receivedError:for:response:retryHandler]
where you could return different error if you don't want to retry request

does that make sense @designatednerd or maybe there is better way to do that?

How it works now:

// HTTPNetworkTransportPreflightDelegate

func networkTransport(_ networkTransport: HTTPNetworkTransport, willSend request: inout URLRequest) {
  oauth.authenticate(&request)
}

// HTTPNetworkTransportRetryDelegate

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

  guard
    (response as? HTTPURLResponse)?.statusCode == 401 // Unauthorized - meaning token is probably expired
  else {
    retryHandler(false)
    return
  }
  
  oauth.updateToken { updateTokenError in
    retryHandler(updateTokenError == nil)
  }
}

// App

apolloClient.fetch(query: ...) { (result: Result<GraphQLResult<Query.Data>, Swift.Error>) in
  // Error is 401, and it is correct from apollo perspective, but its not real error if oauth.updateToken fails for whatever reason - lets say internet connectivity problem
}

What I would like to do is:

// HTTPNetworkTransportRetryDelegate

func networkTransport(_ networkTransport: HTTPNetworkTransport, receivedError error: Swift.Error, for request: URLRequest, response: URLResponse?, continueHandler: @escaping (ContinueAction) -> Void) {

  guard
    (response as? HTTPURLResponse)?.statusCode == 401 // Unauthorized - meaning token is probably expired
  else {
    continueHandler(.fail(error))
    return
  }
  
  oauth.updateToken { updateTokenError in
    if let updateTokenError = updateTokenError {
      continueHandler(.fail(updateTokenError))	
    } else {
      continueHandler(.retry)
    }
  }
  
}

// App

apolloClient.fetch(query: ...) { (result: Result<GraphQLResult<Query.Data>, Swift.Error>) in
  // Error would be updateTokenError, in internet connection problem it would be "fail to reach host" instead of 401 what came from apollo
}

I think this could be done without breaking changes by forwarding to existing method if new one is not implemented

@designatednerd
Copy link
Contributor

So it sounds like the error from the retry is not actually being propagated to the original call is the biggest issue here - is that correct?

@RolandasRazma
Copy link
Contributor Author

no, because my token update fails I know that there is no point in retrying, in delegate say "don't retry" - everything works correctly.
Except that logically I have different error (from token update) and not from apollo, but I have no way of propagating it up the chain

@RolandasRazma
Copy link
Contributor Author

updated ticket with code samples, maybe will be easier to understand

@RolandasRazma
Copy link
Contributor Author

would you accept such PR?

@designatednerd
Copy link
Contributor

Thanks for adding the code samples, that does help. Yeah, I think this would be reasonable. Would you like to take a stab at a PR on this?

@RolandasRazma
Copy link
Contributor Author

yes, will do that

@designatednerd
Copy link
Contributor

This has shipped with 0.26.0!

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

No branches or pull requests

2 participants