Batched network interface runs middleware and afterware multiple times

[sheerun]

Introduction

Here's how you introduce middleware in your docs:

"In both examples, we’ll show how you would add an authentication token to the HTTP header of the requests being sent by the client."

networkInterface.use([{
  applyMiddleware(req, next) {
    if (!req.options.headers) {
      req.options.headers = {};  // Create the header object if needed.
    }
    req.options.headers['authorization'] = localStorage.getItem('token') ? localStorage.getItem('token') : null;
    next();
  }
}]);

‘Afterware’ is very similar to a middleware, except that a afterware runs after a request has been made, that is when a response is going to get processed. It’s perfect for responding to the situation where a user becomes logged out during their session.

To support this you show simple afterware that introspects responses's code:

networkInterface.useAfter([{
  applyAfterware({ response }, next) {
    if (response.status === 401) {
      logout();
    }
    next();
  }
}]);

To me message is simple: middleware and afterware allow you to change request and introspect response to/from the graphql backend. You also properly recognize that middleware is mostly useful for authenticating request, and afterware is mostly useful for introspecting responses, e.g. to logout when status is 401.

Additionally you have concept of networkInterfaces that (I suppose) ideally could be just drop-in replaced. e.g. I could exchange NetworkInteface with BatchedNetworkInterface without changing any code.

Issue

Unfortunately both of these use cases fail with current implementation of batched network interface. Namely is runs middleware and afterware for each of batched queries instead all of them in whole. It also breaks the interface of applyAfterware, because e.g. response.status is unavailable.

The logic for network interface assumes, middleware and afterware runs just one time, middleware adding authentication headers, and afterware checking for response status after receiving it from server. If we exchanged network interface for batched network interface for examples above, we:

1. Would unnecessairly add authentication headers to 10 requests, even batching network interface concatenates all of them anyway later on. It's meaningless to set different authentication headers on each request, as batching network interface will just select one of them, by merging all the options on all requests together..
2. applyAfterware would be called multiple times, for each batched response
3. On server we also need to handle this case on graphql server when we server 401 response, i.e. send json array as response if query is batched, and single json object if query is not batched. On top of it batched response needs to contain as many blank objects as batch queries performed, otherwise afterware won't be called even once...
4. Because afterware is called multiple time, logout is dispatched multiple times
5. Actually it would be dispatched multiple times, because middleware code throws, as request.status is not available as payload..

Summary

I hope I've shown you that the decision to run middleware and afterware for each batched query and response for batched network interface is wrong.

How you could fix it? Run middleware and afterware just one time, on request object after the batching is performed. The interface and contract for both should be 100% the same as on network interface.

If you want a possibility to middleware individual queries within batch, you can introduce extra queryMiddleware and queryAfterware that work of individual queries, not on whole requests.

Thank you, you're awesome

[helfer]

@sheerun Good point, I think you're right about running only once, even on batched queries. I do think that we'll still need a kind of middleware and afterware that runs for each individual query, and the names queryMiddleware and queryAfterware seem quite reasonable to me.

Would be great if you (or someone else) could make a PR. Don't be afraid to ping me directly if you need any pointers/hints. 🙂

[helfer]

Having thought about this a little bit more, I don't think middleware and afterware is currently used to alter the body of the GraphQL request (nor is it recommended), so an initial fix for this should simply be to change the behavior of batched network interface to run on the batched query.

If it turns out that there's a good use-case for queryMiddleware and queryAfterware, we can always add those later.

[giautm]

Please consider about authMiddleware like react-relay-network-layer. ;)

authMiddleware({
    token: () => store.get('jwt'),
    tokenRefreshPromise: (req) => {
      console.log('[client.js] resolve token refresh', req);
      return fetch('/jwt/refresh')
        .then(res => res.json())
        .then(json => {
          const token = json.token;
          store.set('jwt', token);
          return token;
        })
        .catch(err => console.log('[client.js] ERROR can not refresh token', err));
    },
  }),

[lewisf]

@helfer I'll take a stab at doing the fix you suggested.

[helfer]

@lewisf Great, I look forward to it! 🙂

[sondremare]

@lewisf Any progress? Let me know if I can assist with anything

[lewisf]

Hey @sondremare, got busy with other things. I have test cases, and a work in progress branch that I'll make a PR with in the upcoming days.

[helfer]

This is fixed now, thanks to @lewisf ! ❤️

[sheerun]

@helfer @lewisf Looks good. It's just shame you didn't change behavior of applyMiddleware to what batchMiddleware is implemented here. queryMiddleware is both more generic name and would allow for better uniformity and backward compatibility of passing request / response to applyMiddleware

• applyMiddleware - apply middleware to request (always called one time)
• applyAfterware - apply middleware to response (always called one time)
• queryMiddleware - apply middleware to request payload (for each query in request)
• queryAfterware - apply middleware to response payload (for each query in request)

These generic api methods can be implemented by network interfaces.

On the other hand applyBatchMiddleware doesn't sound nowhere generic and it seems it'll be implemented just by batchNetworkInterface. It's a shame because I thought one could replace network interface easily and standard interface is the key enabler of it.

[stubailo]

I thought one could replace network interface easily and standard interface is the key enabler of it.

Interesting - I never really considered that sharing middlewares between different network interfaces would be that useful. Since different interfaces have different transport mechanisms, seems like it would be hard. Of course some stuff like the query response data will be the same across transports, but that seems like the minority.