graphile / crystal

🔮 Graphile's Crystal Monorepo; home to Grafast, PostGraphile, pg-introspection, pg-sql2 and much more!
https://graphile.org/
Other
12.49k stars 568 forks source link

feat: call next() from koa middleware for downstream middlewares #2073

Closed aryascripts closed 4 weeks ago

aryascripts commented 1 month ago

Description

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.

Performance impact

There is no performance impact to graphile or grafast. next() will be the next middleware in the chain of middlewares in the Koa app.

Security impact

unknown, or none.

Testing

There were no tests here.

I need some help with the tests, because I tried to create a new test file for this.Currently it's not committed because I can't verify that it runs. When I try running the tests, I get a few different errors. Are there any docs on the set up required for this repository? What databases are needed? What commands to run to run the tests? (yarn test doesn't work right after yarn install)

I was able to add a test file to test this change.

Checklist

changeset-bot[bot] commented 1 month ago

🦋 Changeset detected

Latest commit: 01dd77106448416151c93af7af10ec029419811f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages | Name | Type | | ------------------- | ----- | | grafserv | Patch | | @grafserv/persisted | Patch | | pgl | Patch | | postgraphile | Patch | | graphile-build-pg | Patch | | graphile-utils | Patch | | graphile | Patch |

Not sure what this means? Click here to learn what changesets are.

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/aryascripts/crystal/new/patch-1?filename=.changeset/lemon-crabs-destroy.md&value=---%0A%22grafserv%22%3A%20patch%0A---%0A%0Afeat%3A%20call%20next()%20from%20koa%20middleware%20for%20downstream%20middlewares%0A)

benjie commented 4 weeks ago

In theory, all scenarios of a middleware should call next, unless it's a catastrophic error.

That does not seem to align with the Koa documentation:

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. -- https://github.com/koajs/koa/blob/master/docs/guide.md#response-middleware

My understanding is that "terminal middleware" (or as Koa calls them, response middleware) need not call next(); in fact doing so could cause bugs in people's code by having multiple response handlers execute unexpectedly. Koa's nested middleware pattern allows each early middleware to perform actions both before and after any later middleware, for example logging and metrics.

For example:

app.use(async function (ctx, next) {
  console.log('>> one');
  await next();
  console.log('<< one');
});

app.use(async function (ctx, next) {
  console.log('>> two');
  await next();
  console.log('<< two');
});

app.use(async function (ctx, next) {
  ctx.body = 'hello';
});

this would log >> one, >> two, then it would set the response, then it would log << two and then << one.

You must make sure any logging/metrics middleware is registered before any terminal middlewares that they wish to instrument.