mutations for afterware to support retrying requests

[tscanlin]

Touched on in #439.

This is an attempt at implementing a request retry when a request returns with a 401.

Changes in this PR plus the below afterware seem to work for our purposes.

        {
          applyAfterware(res, next) {
            if (res.response.status === 401) {
                return res.response.json().then((json) => {
                    if (json.errorCode === 'TOKEN_NEEDS_REFRESH') {
                        console.log(`Error: ${json.errorCode}`)

                        // Get the new oauth2 token.
                        return fetch(`${ckHttpClient.API_URL}/oauth2/token`, {
                            method: 'POST',
                            headers: getHeaders(),
                            body: JSON.stringify({
                                'refreshToken' : window._REFRESH_TOKEN
                            })
                        }).then((res) => {
                            return res.json()
                        }).then((data) => {
                            // Put the token on the window object.
                            if (data.accessToken && data.refreshToken) {
                                window._ACCESS_TOKEN = data.accessToken
                                window._REFRESH_TOKEN = data.refreshToken

                                lastRequest.options.headers.Authorization = `Bearer ${data.accessToken}`
                            }
                            // Repeat the request again.
                            networkInterface.fetchFromRemoteEndpoint(lastRequest)
                                .then((newResponse) => {
                                    res.response = newResponse
                                    next(newResponse)
                                })
                        })
                    }
                    // Hard 401 failure, we can't refresh, so logout
                    console.log('Error: Hard 401 failure, logout')
                    logout()
                })
            }
            next()
            return response
          }
        }

Is this the best way to handle this? Or do you guys have something else in mind?

I mostly just wanted to share my approach here to get feedback and share in case anyone else is trying to solve the same problem.

TODO:

• If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change
• Add your name and email to the AUTHORS file (optional)
• If this was a change that affects the external API, update the docs and post a link to the PR in the discussion

[apollo-cla]

@tscanlin: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[helfer]

Hi @tscanlin 👋

I think this is a great way of handling this, as long as you don't somehow end up in a loop. Since the change to source code is very minimal, I'm more than happy to merge this. Thanks a lot for the PR!

[tscanlin]

Sweet, thanks @helfer! Loving the Apollo project so far 👍

[Multiply]

@tscanlin Do you just store lastRequest in a middleware, or how do you get the correct request?

[tscanlin]

@Multiply that is a really great question. :)

In the file where I declare all the middleware and afterware I declared a lastRequest variable at the top of the file to hold onto that state. Then in the middleware I set lastRequest to the request that gets passed in so that I can have access to the lastRequest variable in the afterware.

But having this piece of external state is probably not the best solution. It would be better if the request got passed in to the afterware as well.

@helfer would you accept a PR to pass the request in to the applyAfterwares function? Or what do you think would be the best way to handle this? (https://github.com/tscanlin/apollo-client/blob/5e8d939cf4067dc1d55421cbd2318ad7cffab278/src/transport/networkInterface.ts#L183)

[Multiply]

@tscanlin I tried the same, but since many requests could fire at the same time lastRequest wouldn't be the correct request anymore.

I eventually set options._requestId to some incremented number, and stored it in an object, with that as a key. Then when it got to the applyAfterware I could check if it exists by checking the options.

However, I would be ALL for the proposed change, of having both the request and response as properties.

[tscanlin]

That's a good point. Glad you thought of a solution that accounts for simultaneous requests. Thanks for sharing it.

Is there any reason you couldn't just pass the whole request to the afterware via the options object?
something like options._request? I know that would end up passing an extra invalid option the the fetch call, but it doesn't seem to throw an exception. ¯_(ツ)_/¯

[Multiply]

@tscanlin I guess that makes more sense, as I wouldn't have to maintain an object somewhere else in my authentication middleware, but I actually wanted a specific request id, so I could track it better when it went back and re-requested the resource. I guess I'll do both, until we have a better solution.

I am still really liking the idea of having the request in the afterware, tho.

[tscanlin]

Yeah, I'd still be up for making that change if they agree to it.

I'm curious what you mean by "so I could track it better when it went back and re-requested the resource", what's your use case here? Maybe its something I haven't thought of yet.

[Multiply]

@tscanlin In dev-mode I track all incoming and outgoing requests, with a request and response id, and push that to Arangodb, both on the client and any of my several backends, and I then end up with a nice graph of the full session.

In this case it'll also store a parent request ID, that is then marked with a 401, and I can then easily find all failing requests and see how they resolved.

There's a lot of different use cases :)

[tscanlin]

That's a very cool use case :) Arangodb looks pretty awesome. Are you using Apollo to query it?

I can imagine it would really help to have the global map of requests like you say especially for debugging. Have you noticed any performance hits with having that giant map of requests? Or do you have caching / only use for dev?

[Multiply]

@tscanlin I am using arangojs, and then Apollo on top, yes. I am going to try do some 'smart' graph queries to optimize the performance of GraphQL overall. Fingers crossed, my prototype will work soon enough.

I only use it for dev at the moment, but I have no problems with even millions of records. I've optimized my indexes a lot over the last few weeks, so that helps a lot.
For production I am using Cassandra and ElasticSearch for the same approach for other apps, and without any issues so far, however they lack graph-support (Unless you pay loads of money), so you have to do some glue by hand. I hoped TitanDB (GraphDB using Cass + ES) would be an active open source community, but it died when DataStax acquired them. My hopes are up for JanusGraph (fork of TitanDB) tho.