Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fastify Adapter Middleware Properties on a Different Context #8837

Closed
6 of 15 tasks
jmcdo29 opened this issue Dec 20, 2021 · 27 comments
Closed
6 of 15 tasks

Fastify Adapter Middleware Properties on a Different Context #8837

jmcdo29 opened this issue Dec 20, 2021 · 27 comments
Labels
needs triage This issue has not been looked into

Comments

@jmcdo29
Copy link
Member

jmcdo29 commented Dec 20, 2021

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

There are (closed) existing issues for this, all without reproductions. (#8234 #8809 along with iamolegga/nestjs-pino#546 and mikro-orm/mikro-orm#2531) On to the issue

When running middleware, it's common for us to want to attach properties to the req object to be able to access them later. Pino and MikroORM both do this, and apparently use AsyncLocalStorage to help stay away from needing to REQUEST scope providers (which is honestly really nice). With Express, this works just fine, because Express's Request is Node's IncomingRequest object. With Fastify, however, it is a FastifyRequest which is a wrapper around IncomingRequest. With the use of middie we are able to apply express-like middleware to Fastify, which helps the merging of the frameworks inside of Nest. It appears though that req.custom applies to different property than expected, meaning that the retrieval by ALS doesn't work properly

When running the following Nest middleware

export class AppModule implements NestModule {
  constructor(private readonly appHost: HttpAdapterHost) {}
  configure(consumer: MiddlewareConsumer) {
    const isFastify = this.appHost.httpAdapter instanceof FastifyAdapter;
    consumer
      .apply((req, res, next) => {
        console.log('In Middleware');
        req.custom = 'hey look!';
        next();
      })
      .forRoutes(isFastify ? '(.*)' : '*');
  }
}

we have two different behaviors.

In Express

We can do @Req() and get the property with req.custom in a controller

In Fastify

We an do @Req() and get the property with req.raw.custom in a controller

Minimum reproduction code

https://github.com/jmcdo29/context-fastify-middleware

Steps to reproduce

pnpm i/npm i/yarn
pnpm test:e2e/npm run test:e2e/yarn test:e2e

See the test fail

Expected behavior

Regardless of the adapter used req.custom should get the custom property

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

8.2.4

Packages versions

 _   _             _      ___  _____  _____  _     _____
| \ | |           | |    |_  |/  ___|/  __ \| |   |_   _|
|  \| |  ___  ___ | |_     | |\ `--. | /  \/| |     | |
| . ` | / _ \/ __|| __|    | | `--. \| |    | |     | |
| |\  ||  __/\__ \| |_ /\__/ //\__/ /| \__/\| |_____| |_
\_| \_/ \___||___/ \__|\____/ \____/  \____/\_____/\___/

[System Information]
OS Version : Linux 5.15
NodeJS Version : v16.13.0
PNPM Version : 6.21.0

[Nest CLI]
Nest CLI Version : 8.1.6

[Nest Platform Information]
platform-express version : 8.2.4
platform-fastify version : 8.2.4
schematics version : 8.0.5
testing version : 8.2.4
common version : 8.2.4
core version : 8.2.4
cli version : 8.1.6

Node.js version

16.13.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

I'll start looking into what we can do with middie to alleviate this. Just thought I should get a proper minimum reproduction for this as it seems to affect several other packages, but no one has been able to provide something yet

@jmcdo29 jmcdo29 added the needs triage This issue has not been looked into label Dec 20, 2021
@jmcdo29
Copy link
Member Author

jmcdo29 commented Dec 21, 2021

It should also be noted that with fastify, this is the same behavior.

const fastify = require("fastify");
const middie = require("middie");

const start = async () => {
  try {
    const app = fastify();

    await app.register(middie);

    app.use((req, res, next) => {
      req.custom = "something else";
      next();
    });

    app.get("/", (req, reply) => {
      return {
        regular: req.custom,
        raw: req.raw.custom,
      };
    });
    await app.listen(3030);
  } catch (err) {
    process.stderr.write(err.toString());
    process.exit(1);
  }
};

start();
curl http://localhost:3030
{"raw":"something else"}

I'm not 100% convinced this is a bug as this is how it seems Fastify is expected to act. However, we should probably make a note that attaching properties to the req object in a middleware will attach it to req.raw inside of Fastify due to how middie works.

@kamilmysliwiec thoughts?

@jmcdo29
Copy link
Member Author

jmcdo29 commented Dec 21, 2021

Fastify middleware do not expose the send method or other methods specific to the Fastify Reply instance. This is because Fastify wraps the incoming req and res Node instances using the Request and Reply objects internally, but this is done after the middleware phase. If you need to create middleware, you have to use the Node req and res instances. Otherwise, you can use the preHandler hook that already has the Request and Reply Fastify instances. For more information, see Hooks.

Fastify docs

@kamilmysliwiec
Copy link
Member

Thanks for looking into this issue @jmcdo29 🙌

With Express, this works just fine, because Express's Request is Node's IncomingRequest object. With Fastify, however, it is a FastifyRequest which is a wrapper around IncomingRequest.

This is correct and it's expected behavior. Fastify passes raw (unwrapped) Request & Response objects to middleware functions while route handlers receive wrapped FastifyReply and FastifyRequest objects.

However, we should probably make a note that attaching properties to the req object in a middleware will attach it to req.raw inside of Fastify due to how middie works.

We should probably add a hint to the docs as it clearly led to confusion.

AsyncLocalStorage to help stay away from needing to REQUEST scope providers (which is honestly really nice)

Side note: AsyncLocalStorage relies on the async_hooks and this module is still experimental in Node v17 https://nodejs.org/api/async_hooks.html

@B4nan
Copy link

B4nan commented Dec 22, 2021

Spent some time trying to reproduce the ALS issue, here is a minimal repro:

https://github.com/B4nan/nest-fastify-als

I don't think that OP in this issue is about the same, the reproduction is just attaching things to req objects, ALS solution should not care about whether fastify wraps the request object or not. Shall I create new issue for this?

@kamilmysliwiec
Copy link
Member

@B4nan what if you use pure Fastify instead of Nest? Does this happen as well?

@B4nan
Copy link

B4nan commented Dec 22, 2021

Pure fastify does not support middlewares, right?

edit: probably via this? https://github.com/fastify/middie

@kamilmysliwiec
Copy link
Member

Correct @B4nan, using middie .

@B4nan
Copy link

B4nan commented Dec 22, 2021

Looks like it fails with bare bones fastify+middie too, so probably not nest related.

@B4nan
Copy link

B4nan commented Dec 22, 2021

https://github.com/fastify/middie/issues/124

@mcollina
Copy link

If people are passing through here, here is the Node Core issue about it: nodejs/node#41285.
It's not actually a problem of Fastify but of Node.js.

@mcollina
Copy link

FWIW I don't think this is totally fixable within Fastify, Nest or Node.js. For your use case you should be using storage.enterWith().

@mcollina
Copy link

mcollina commented Dec 22, 2021

After some investigation, I think the root cause of the problem is that Nest.js starts its own AsyncLocalStorage after body-parser has done its job with express, and before Fastify has parsed the body.

Note that entering the AsyncLocalStorage before doing the body parsing has the same effect with express (and it will lose context):

'use strict'

const { AsyncLocalStorage } = require('async_hooks')
const express = require('express')

const storage = new AsyncLocalStorage()

let counter = 0
const app = express()

app.use((req, res, next) => {
  const id = counter++
  console.log('In Middleware with id ' + id)
  storage.run({ id }, function () {
    next()
  })
})
app.use(require('body-parser').json()) // try moving this before the above middleware

app.post('/', (req, res) => {
  const store = storage.getStore()
  console.log('store is', store)
  res.end(JSON.stringify(store))
})

app.listen(3000)

All things considered, I think the best solution for Nest is to register middie in a different hook, either 'preValidation' or 'preHandler' - this difference in ordering could be the cause of other nasty bugs.

@kamilmysliwiec
Copy link
Member

Thanks for chiming in @mcollina!

Nest.js starts its own AsyncLocalStorage after body-parser has done its job with express, and before Fastify has parsed the body.

Could you elaborate more on this? Nest internally doesn't use the AsyncLocalStorage.

All things considered, I think the best solution for Nest is to register middie in a different hook, either 'preValidation' or 'preHandler' - this difference in ordering could be the cause of other nasty bugs.

This is where we currently register middie (using FastifyInstance#middie - this happens before any middleware is registered) https://github.dev/nestjs/nest/blob/411ef5e237b487c447e6b28fafaf3914ffa75778/packages/platform-fastify/adapters/fastify-adapter.ts#L459 Are there any examples of alternative ways of registering middie? I checked out the README but couldn't find anything related to the 'preValidation' or 'preHandler' hooks

@mcollina
Copy link

Could you elaborate more on this? Nest internally doesn't use the AsyncLocalStorage.

I'm unsure. Apparently quite a few Nest.js users are (possibly in combination with MikroORM?) and they are reporting this issues and others on Fastify repositories.

This is where we currently register middie (using FastifyInstance#middie - this happens before any middleware is registered) https://github.dev/nestjs/nest/blob/411ef5e237b487c447e6b28fafaf3914ffa75778/packages/platform-fastify/adapters/fastify-adapter.ts#L459

In Nest you register the body-parser middleware before everything: https://github.dev/nestjs/nest/blob/411ef5e237b487c447e6b28fafaf3914ffa75778/packages/platform-fastify/adapters/fastify-adapter.ts#L459. However my default Middie executes the middlewares during the Fastify onRequest hook, before body parsing. Due to nodejs/node#41285, parsing any body would lose context.
There might other subtle differences there, so you might want to register middie so that it executes the middlewares after body parsing is done.

Are there any examples of alternative ways of registering middie? I checked out the README but couldn't find anything related to the 'preValidation' or 'preHandler' hooks.

I forgot to do it in Middie, we just shipped it as https://github.com/fastify/middie/tree/v5.4.0. This is an example:

const fastify = require('fastify')()

fastify
  .register(require('middie'), { hook: 'preHandler' })
  .register(subsystem)

async function subsystem (fastify, opts) {
  fastify.addHook('onRequest', async (req, reply) => {
    console.log('first')
  })

  fastify.use((req, res, next) => {
    console.log('third')
    next()
  })

  fastify.addHook('onRequest', async (req, reply) => {
    console.log('second')
  })

  fastify.addHook('preHandler', async (req, reply) => {
    console.log('fourth')
  })
}

Anyway, this issue can be closed as I shipped Fastify v3.25.2 that should fix it.

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Dec 23, 2021

I'm unsure. Apparently quite a few Nest.js users are (possibly in combination with MikroORM?) and they are reporting this issues and others on Fastify repositories.

I think it's due to MikroORM that use ALS.

Anyway, this issue can be closed as I shipped Fastify v3.25.2 that should fix it.

❤️ 🙌

so you might want to register middie so that it executes the middlewares after body parsing is done.

I'll look into that

@B4nan
Copy link

B4nan commented Dec 26, 2021

Note that the issue with ALS is now addressed in latest fastify, but nest has the fastify version locked to older one, so on nest level the issue is still there.

https://github.com/nestjs/nest/blob/master/packages/platform-fastify/package.json#L20

@B4nan
Copy link

B4nan commented Jan 13, 2022

Note that the issue with ALS is now addressed in latest fastify, but nest has the fastify version locked to older one, so on nest level the issue is still there.

master/packages/platform-fastify/package.json#L20

@kamilmysliwiec @jmcdo29 can we please bump the version of fastify and ship new version of the platform? or is there anything blocking?

@kamilmysliwiec
Copy link
Member

@B4nan A new version has been published yesterday

@B4nan
Copy link

B4nan commented Jan 13, 2022

Well, but the fastify version in master is still the same 🤷

https://github.com/nestjs/nest/blob/master/packages/platform-fastify/package.json#L20

@kamilmysliwiec
Copy link
Member

That's odd. Dependabot/renovate should have updated deps automatically. I'll look into this

@B4nan
Copy link

B4nan commented Jan 20, 2022

Another bump that the fastify version in master is still old. Can we reopen this? I can understand you keep forgetting about closed issues. Or do you want me to PR this?

@kamilmysliwiec
Copy link
Member

It seems that Renovate got stuck for some reason. I haven't had enough time to investigate it yet so feel free to create a PR and I'll merge it asap.

@B4nan
Copy link

B4nan commented Jan 20, 2022

I am sure it won't get unstuck automatically (using renovate myself for quite some time), if you for some reason closed upgrade PR for fastify, it won't create new one unless there is new major version.

@B4nan
Copy link

B4nan commented Jan 20, 2022

Well, but you have opened PR for that :D

#8970

@kamilmysliwiec
Copy link
Member

If you check out this issue #4944 you'll notice that it got stuck even though I haven't closed "upgrade PR" for fastify. Luckily it seems that Dependabot created a PR for this so it will surely get merged very shortly.

@B4nan
Copy link

B4nan commented Jan 20, 2022

From what I know, dependabot does not privide any automerge (it was there but they removed it). Why don't you just push the button, given the tests are green? :]

Also I don't see any fastify PR from renovate, only presence of the work fastify on that dashboard is about something else.

@kamilmysliwiec
Copy link
Member

Also I don't see any fastify PR from renovate, only presence of the work fastify on that dashboard is about something else

This is exactly what indicates that Renovate is stuck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

4 participants