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

Introducing delegate if there is GraphQL errors #770

Merged

Conversation

kimdv
Copy link
Contributor

@kimdv kimdv commented Sep 18, 2019

The idea of this delegate is based on

It adds ability for better error handling on the clients when there is Graphql errors
Etc when a token is expired.

The diffrent from this delegate and HTTPNetworkTransportRetryDelegate is that some errors might come from the GraphQL API and not from middleware etc.

  • Add tests

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this idea! Would love to see some tests on it please 😃

Sources/Apollo/HTTPNetworkTransport.swift Outdated Show resolved Hide resolved
Sources/Apollo/HTTPNetworkTransport.swift Show resolved Hide resolved
@kimdv
Copy link
Contributor Author

kimdv commented Sep 19, 2019

Will create tests when we can mock. Depending on #767

@designatednerd
Copy link
Contributor

@kimdv #767 is finally merged if you wanna use the stuff from that still

@kimdv kimdv force-pushed the kimdv/add-retry-if-graphql-errors branch from beaa094 to ca2e22d Compare October 15, 2019 13:15
Sources/Apollo/HTTPNetworkTransport.swift Outdated Show resolved Hide resolved
completionHandler: @escaping (_ result: Result<GraphQLResponse<Operation>, Error>) -> Void) throws {
guard let delegate = self.delegate as? HTTPNetworkTransportGraphQLErrorDelegate,
let graphQLErrors = response.parseErrorsOnlyFast(),
!graphQLErrors.isEmpty else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is more of a note to self than to you: I need to add my readability isNotEmpty boolean extension to this code 🙃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #872

@kimdv kimdv force-pushed the kimdv/add-retry-if-graphql-errors branch from ca2e22d to e7ab4dc Compare October 16, 2019 05:50
@kimdv
Copy link
Contributor Author

kimdv commented Oct 16, 2019

@designatednerd I rebased and added two tests. One with error and one with none.

@kimdv
Copy link
Contributor Author

kimdv commented Oct 16, 2019

@designatednerd I rebased and added two tests. One with error and one with none.
I'm not sure that the tests are good enough. If not, please send some hints! 🚀

I also noticed tests are not executed randomly.
I think it should be enabled as it will prevent tests depend on each other 🔥

@kimdv kimdv marked this pull request as ready for review October 23, 2019 13:22
@designatednerd
Copy link
Contributor

@kimdv can you merge in and/or rebase off master so the test suite can run on Circle please?

@designatednerd designatednerd added this to the 0.19.0 milestone Oct 29, 2019
@kimdv kimdv force-pushed the kimdv/add-retry-if-graphql-errors branch from 7136765 to 2e8d94a Compare October 29, 2019 19:03
@kimdv
Copy link
Contributor Author

kimdv commented Oct 29, 2019

@designatednerd done!
Let's see what explodes 🔥🙈

@designatednerd
Copy link
Contributor

🎉 Nothing! 🎉 Mergin'

@designatednerd designatednerd merged commit 0c56d45 into apollographql:master Oct 29, 2019
@RolandasRazma
Copy link
Contributor

how do you know if failure after retrying?
if request fails and you call retryHandler(true) and it fails again I don't see how to know that you should just give up

@designatednerd
Copy link
Contributor

That's why we explicitly recommend against just calling retryHandler(true) without any context in the documentation for this method and the retry delegate for network errors.

If you're not looking at the actual errors that have come in, you could easily wind up in an infinite retry loop.

@RolandasRazma
Copy link
Contributor

RolandasRazma commented Nov 14, 2019

I'm thinking about case where server say "token invalid", I update token, and server say "still invalid". Correct thing to do is to give up, but I don't know that I already tried to update token

@designatednerd
Copy link
Contributor

It sounds a bit more like a bug in your server if a just-updated token is being rejected as invalid, but you could theoretically add some tracking of what the last (n) errors received were, and if some aspect of them (status code, message, etc) is the same, then stop retrying.

@RolandasRazma
Copy link
Contributor

maybe it's me, but I like to code defensively 🤷‍♂

@designatednerd
Copy link
Contributor

That's fair, but I think the "how do I handle situation x" stuff is more of an application-level decision, so we're letting developers handle that as defensively as they'd like to.

@kimdv
Copy link
Contributor Author

kimdv commented Nov 15, 2019

Hi!
We had the same problem but solved it in the client.

final class GraphQLClient {
    private var retryHandlers = [(Bool) -> Void]()
    private let isolationQueue = DispatchQueue(label: "GraphQLClient.isolationQueue",
                                               qos: .userInitiated,
                                               attributes: [],
                                               autoreleaseFrequency: .workItem,
                                               target: nil)
}
extension GraphQLClient: HTTPNetworkTransportGraphQLErrorDelegate {
    func networkTransport(_ networkTransport: HTTPNetworkTransport, receivedGraphQLErrors errors: [GraphQLError], retryHandler: @escaping (Bool) -> Void) {
        let errors = errors.map { OurGraphQLError(error: $0) }
        
        guard error.contains(.authTokenInvalid) else {
            retryHandler(false)
            return
        }
        
        let isTokenExchangeInProgress = scheduleForExecution(retryHandler: retryHandler)
        if isTokenExchangeInProgress { return }
        
        exchangeRefreshToken { [weak self] result in
            guard let strongSelf = self else { return }
            
            switch result {
            case .success:
                strongSelf.executeRetryHandlers(retry: true)
            
            case .failure(let error):              
                strongSelf.executeRetryHandlers(retry: false)
            }
        }
    }

    private func scheduleForExecution(retryHandler: @escaping (Bool) -> Void ) -> Bool {
        var isTokenExchangeInProgress = true
        isolationQueue.sync {
            isTokenExchangeInProgress = !retryHandlers.isEmpty
            retryHandlers.append(retryHandler)
        }
        return isTokenExchangeInProgress
    }

    private func executeRetryHandlers(retry: Bool) {
        isolationQueue.async { [weak self] in
            guard let strongSelf = self else { return }
            for handler in strongSelf.retryHandlers {
                handler(retry)
            }
            strongSelf.retryHandlers = []
        }
    }
}

In this way we can cancel if refresh token exchange fails.

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

Successfully merging this pull request may close these issues.

3 participants