apollo-server-express: allow customization of how response is sent (eg, allow you to call next afterwards)

[South-Paw]

Hi there,

Have been upgrading my stack to v3 and have noticed that express middlewares placed after the graphql server middleware are not run. I'm reasonably sure this was working in my v2 project beforehand.

My use case is that I have a logger that reports when a request is complete and it is not called after a request to the /graphql endpoint but is correctly called after requests to other endpoints on the express server.

I believe the fix is simple, there are 3 places where next() just needs to be called so express can continue running middlewares.

I made a copy of ApolloServer.ts in my project and added the next() calls on

• Line 126 before the return statement.
• Line 168 after the else case just return next();
• Line 188 after the else case just return next(error);

After making these changes, I can see my post-request logger working again on the /graphql endpoint.

Hopefully this is clear enough, apologies that I do not have time to create a reproduction repo to show the issue.

[glasser]

I believe the standard way of doing logging in Express is to use the res.on('finished') event or the on-finished package from an early middleware, instead of assuming that all requests will fall through to the end of your app. For example, the built-in express.static middleware does not call next() when it serves a file, and Express loggers like morgan use on-finished. See other discussion in #3833.

[glasser]

(I don't believe this changed in AS3, but if you can share a pair of reproductions showing how it changed (eg codesandbox.io or repos I can clone) I'd be happy to at least update the migration guide.)

[South-Paw]

Hi thanks for your reply!

Firstly, apologies; I went back to my v2 project and the middlewares I thought were being called after AS are not relevant (and checking the AS code confirms this wouldn't have worked in v2 anyhow).

I checked the express v4 documentation and can't find any mention of res.on('finished') but did find this stackoverflow post about it being 'undocumented' in addition to your own comment on #3833 - would you be able to point me to any relevant express documentation I've missed here?

I understand and confirmed for myself from the express documentation that there's no clear right or wrong in expecting next() to be called so that the app can fall through middleware after a request is sent. I guess I just expected that since I'm using the AS as a 'middleware' it would call next() like (some) other middlewares I use do.

However as you mentioned in #3833:

we could add some sort of server.applyMiddleware({ alwaysCallNext: true })

would this be something you would still consider adding? It'd mean that the change is opt-in for anyone who wants/needs it and can be done as a minor version bump for AS3 that wouldn't disrupt or break any current implementations as I can understand why you might not want to just throw 3 next() calls into the code without making it a major release 😄. Thoughts?

[South-Paw]

I have gone ahead and implemented the on-finished package as you suggested to see if that resolves this for me.

I can see the 'finished' request log occurring before other middlewares have done their logging though which indicates this hasn't helped solve the issue.

[glasser]

finish is an event on the Node core http response object.

How do the other middlewares do their logging? Do they also use finish?

It just seems strange that we would make our "handle a request and send a full response" middleware work differently from core Express middleware.

[South-Paw]

How do the other middlewares do their logging? Do they also use finish?

I took a look at pino and it does indeed also use res.on('finish'). I'll try using that instead of the on-finished package and see if the results are any different.

It just seems strange that we would make our "handle a request and send a full response" middleware work differently from core Express middleware.

I understand what you mean there, but I don't see it doing any harm to AS by adding opt-in support for this given how small of a change it is and because I'm not the first to request/find this sort of thing (#3833).

I think a little more context about what I'm using AS with would help - our express server uses routing-controllers to create the express app (as we have a bunch of REST APIs for third parties as well as internal GraphQL APIs on the same app).

routing-controllers has before and after middlewares and the logger in question is one of a few after middlewares that run once the request has completed. I just expect that the after middlewares will continue working when I combine routing-controllers with apollo-server-express and until now they've worked fine with every other middleware we've attached to the server.

[glasser]

What happens when you combine after middlewares with express.static/serve-static, the standard middleware that works like Apollo Server?

[South-Paw]

Hey @glasser,

I really appreciate you taking the time to reply to me each time and answer my questions. However, this is really is starting to feel like we're making a mountain out of a mole hill here.

This is (in my view) a really simple and reasonable request that has come up before (#3833) and I can't understand why Apollo would be reluctant to add an alwaysCallNext option (that a maintainer has already suggested) to support some of their customers use cases...?

Please understand and empathize with me here a bit - I'm not trying to be rude, difficult or unreasonable, I just have a problem and a tech stack that we've already invested in and we want to use Apollo's solution for this part of it.

What happens when you combine after middlewares with express.static/serve-static, the standard middleware that works like Apollo Server?

I wouldn't expect to mount serve-static as an after middleware. It would more than likely be mounted as before middleware which is the same as how I currently mount the apollo graphql server. An after middleware is for things that happen after the request/action is handled (in the case of routing controllers documention, they are putting a logging middleware after the action).

I still believe your original suggestion for an alwaysCallNext or fallthrough option in #3833 is the best of both worlds here.

Thanks

[glasser]

Hmm. I really think boolean alwaysCallNext or fallthrough are a bit weird unless somebody can show an example of another express library that makes a similar choice.

But what about something a bit more flexible instead. Right now we have this weird Connect-compatibility code:

          // Using `.send` is a best practice for Express, but we also just use
          // `.end` for compatibility with `connect`.
          if (typeof res.send === 'function') {
            res.send(graphqlResponse);
          } else {
            res.end(graphqlResponse);
          }

What if we combined this concern with cleaning up the compatibility code, and had an option to applyMiddleware like sendGraphQLResponse: (res: express.Response, httpQueryResponse: HTTPQueryResponse, next: express.NextFunction) which we'd use for the "send an actual GraphQL response" and "send HttpQueryError" codepaths. The default implementation would be

(res, {graphQLResponse}, next) => {
  res.send(graphqlResponse);
}

but connect users could call res.end instead of res.send, and folks in your position could (conditionally?) add a call to next().

Would that solve your issue?

(I'm passing in the whole httpQueryResponse with headers and status code just in case it's helpful to look at it for context but I think it's reasonable to not require you to copy headers and status code to res in your callback.)

[glasser]

Reopening and renaming this because I think it would be a helpful hook; I don't think it makes sense for the core team's roadmap but we are happy to review a PR implementing something like my suggestion above.

[glasser]

I still think that "working kind of like how express.static works" is the best default for Apollo Server.

But Apollo Server 4 makes building your own web framework integration easy. The entire Express integration is barely a hundred lines of code including imports and comments. If you'd like to build your own Express integration that calls next, it's now straightforward.