Koa middleware does not call next() for all scenarios

[aryascripts]

Summary

Koa middlewares run sequentially, and each middleware calls next() function after using or updating the ctx (KoaContext). The issue with the grafast middleware that is added here (koa/v2/index.ts) is that it only calls next() in some scenarios, and not all. In theory, all scenarios of a middleware should call next, unless it's a catastrophic error.

Once this is enabled, it allows the overall koa app to process something after grafast. This could be things like collecting metrics or logging.

Steps to reproduce

Sample code

  const app = new Koa();
  const pgl = postgraphile(configuration, pool);
  const serv = pgl.createServ(grafserv);

  const server = app.listen(port);
  await serv.addTo(app, server);

  app.use(async (ctx, next) => {
    // this will never be called for most requests
    console.log("running middleware after grafast");
  })

Expected results

We should always call the next() middleware in the chain of Koa middlewares.

Actual results

Currently, we only call next middleware if result from graphile is null https://github.com/graphile/crystal/blob/91e87ab6516490a4cc7b7fc6400efb7623fbd331/grafast/grafserv/src/servers/koa/v2/index.ts#L79-L81

Solution

PR here for the fix: https://github.com/graphile/crystal/pull/2073

Call next no matter what, from the grafast/graphile middleware (see PR above)

[benjie]

Closing for the reasons outlined in https://github.com/graphile/crystal/pull/2073#issuecomment-2147648116

The code can be fixed by moving the middleware before PostGraphile:

  const app = new Koa();

  // 👇👇👇
  app.use(async (ctx, next) => {
    console.log("running middleware BEFORE grafast");
    await next(); // 👈👈 this is where PostGraphile/Grafast would run
    console.log("running middleware AFTER grafast");
  })
  // 👆👆👆

  const pgl = postgraphile(configuration, pool);
  const serv = pgl.createServ(grafserv);

  const server = app.listen(port);
  await serv.addTo(app, server);

[aryascripts]

@benjie we have a requirement to run a middleware after, which was described in the issue I created as well. Middlewares in Koa should always be calling next(), which is why they can be chained.

What you are suggesting may work, but is not standard practice in middlewares for HTTP node servers.

What you suggest will work for us

[aryascripts]

@benjie Just to clarify, today postgraphile's middleware does not follow Koa guidelines, which my PR was trying to fix. This will be unexpected to anyone trying to use Koa + Postgraphile in the future.

I request to reconsider my changes.

From Koa's docs: https://github.com/koajs/koa/blob/master/docs/guide.md#writing-middleware

When the middleware is run, it must manually invoke next() to run the "downstream" middleware.

The middleware in this repo today, does not invoke next() which is not what most developers using middlewares expect.

https://github.com/graphile/crystal/pull/2073

[cassidycodes]

@benjie I'm chiming in here because I work with @aryascripts. While your example in #2073 and your suggestion here is helpful and gives us something to work with, it was unexpected that PostGraphile would have a terminal middleware. I'd usually expect a terminal middleware to be written by application owners, rather than a library we use.

However, if you are set on keeping PostGraphile this way, perhaps we can contribute to documentation so that others don't run into the same problem.

[benjie]

Thanks for the discussion around this; it's always good to justify the decisions made in software development.

From Koa's docs: koajs/koa@master/docs/guide.md#writing-middleware

When the middleware is run, it must manually invoke next() to run the "downstream" middleware.

Correct; if you want to run the downstream middleware then you must invoke next(). We don't want to run the downstream middleware if we've already handled the request - we want to bypass it. From further down the same guide.md page you linked (and previously linked in my comment explaining the decision: https://github.com/graphile/crystal/pull/2073#issuecomment-2147648116 ):

Middleware that decide to respond to a request and wish to bypass downstream middleware may simply omit next(). Typically this will be in routing middleware, but this can be performed by any.

PostGraphile is effectively routing middleware; either it routes the request to PostGraphile's response handler, or it calls next() to hand the unhandled request down to downstream middleware. We don't want users accidentally handing the request twice via downstream middleware, so we don't call next() if we've already handled the request. This aligns with the guide.md file that you have linked to, and does not limit the actions you can take since you can add middleware before PostGraphile to add before and after actions.

The middleware in this repo today, does not invoke next() which is not what most developers using middlewares expect.

I'm not convinced this is true, I think calling next() after handling a request would be more surprising for most developers that are familiar with Koa v2+'s middleware system. We have many Koa users, and I don't recall this being raised before.

I'd usually expect a terminal middleware to be written by application owners, rather than a library we use.

Interesting take, and not one I am familiar with. Isn't it quite common for response middleware to be terminal middleware? What other libraries do you use that provide response middleware?

Just looking at some of the standard middleware the koajs project itself has authored:

• The favicon middleware only calls next() if the request was not for the favicon.
• The static middleware only calls next() if sending the file failed
• The ratelimit middleware doesn't call next() when it announces the limit is exceeded
• The jwt middleware uses ctx.throw to throw an error, thereby not calling next().

However, if you are set on keeping PostGraphile this way, perhaps we can contribute to documentation so that others don't run into the same problem.

I'm still open to being convinced otherwise, but currently I believe that PostGraphile does the correct thing as outlined by the Koa documentation, and as demonstrated by a number of the Koa official modules.

A contribution to the documentation that indicates that non-terminal middleware (compression, authentication, authorization, cors, timing, logging, telemetry, body parsing, session, etc) must be added before PostGraphile would be welcome. This may be self-evident for many of these use cases since they need to "wrap" the request (in particular timing, telemetry, session, etc) or need to execute before the request (authentication, body parsing, cors, etc) but it does seem it warrants spelling out for middlewares that only need to execute after the response has been formed.

[cassidycodes]

@benjie Thanks for the thorough and thoughtful response. Treating postgraphile as a terminal middleware makes sense to me. The node/koa ecosystem is new to me, so thank you for linking to example projects that do the same. I started looking through them myself. The way you have suggested handling telemetry, compression, etc, aligns with my model of middleware in Rails, as well. Thanks again.

[benjie]

You're welcome! I mostly did the research to ensure that we were doing the right thing, it's important to match peoples expectations and it's challenging to do when there's so many frameworks and I only use a couple of them. I am definitely not infallible!

The term "middleware" for a -ware that doesn't really go in the middle but at the end is a poor choice in terminology, so I definitely understand the confusion. But... that's just what they are called in Koa :man_shrugging: Really they're the only "-ware" it uses... They should call it "everyware" :laughing:

Contributions to the docs still very much welcome, especially since I don't use Koa myself - having people with hands on writing helpful docs for others would be really appreciated! The Koa docs in particular are extremely thin on the ground.