nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.7k stars 7.63k forks source link

Fastify + middleware wildcards #972

Closed ecrona closed 5 years ago

ecrona commented 6 years ago

I'm submitting a...


[x] Regression 
[ ] Bug report
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

I want to catch all requests with a middleware, but using * or / as path when registering the middleware simply does nothing.

Expected behavior

export class TestMiddleware implements NestMiddleware {
  resolve(...args: any[]): MiddlewareFunction {
    return (req, res, next) => {
      console.log('Request...')
      next()
    }
  }
}

@Controller('cats')
export class CatsController {
  @Get()
  findAll() {
    return 'This action returns all cats'
  }
}

@Module({
  controllers: [CatsController]
})
export class AppModule {
  configure(consumer: MiddlewareConsumer): void {
    consumer.apply(TestMiddleware).forRoutes({ path: '*', method: RequestMethod.ALL }))
  }
} 

Using this code, or with / as path, or simply '*' as an argument for the "forRoutes"-method, I'd wish to catch all routes with the middleware, it only works if I write e.g. 'cats' as path.

Environment


Nest version: 5.1.0


For Tooling issues:
- Node version: 8.9.2  
- Platform: Linux  

Others:

kamilmysliwiec commented 6 years ago

I cannot reproduce your issue. Check your target option in the tsconfig.json file (it should be es6). Also, could you provide more details if that's not the case?

ecrona commented 6 years ago

Sorry, I realized it was because of the FastifyAdapter, for some reason no path argument works when you're the fastifyAdapter, do you treat middleware rules differently using Fastify?

I also noticed res.send() doesn't work in the middleware, I presume that works differently as well.

Thanks for the quick reply!

kamilmysliwiec commented 6 years ago

Fixed in #975. Will be available in 5.2.0 (very soon)

kamilmysliwiec commented 6 years ago

5.2.0 is here!

thaoula commented 6 years ago

Hi @kamilmysliwiec,

I have installed 5.2.2 and however I cannot get our authentication middleware to run using either of the following -

.forRoutes({ path: '', method: RequestMethod.ALL } ); .forRoutes({ path: '/', method: RequestMethod.ALL } ); .forRoutes(''); .forRoutes('/'); .forRoutes('/');

For express it worked using .forRoutes('/');

Are you able to provide an example?

ayZagen commented 5 years ago

Hello @kamilmysliwiec

I tried both functional and NestMiddleware, both of them not working with fastify adapter and no error is thrown. If i remove fastify adapter and use express (which is default) middlewares work as expected. Even without forRoutes I wasn't able to make them work. Using NestJs 5.4.1

thaoula commented 5 years ago

Hi @kamilmysliwiec,

I am trying Fastify again I still cannot get Middleware to run for a wildcard route.

Are you able to please advise or show us an example where the following will work -

return consumer .apply(AuthenticationMiddleware) .with( { path: '/authentication/' }, { path: '/info/' }, { path: '/config/' }, { path: '/import/' }, { path: '/public/import/' }, { path: '/public/export/' }, ) .forRoutes('*');

The middleware never gets called.

Been stuck over and over again. Either we are missing something or something is broken.

Regards, Tarek

"@nestjs/common": "5.6.2", "@nestjs/core": "5.6.2",

v1d3rm3 commented 5 years ago

Hi @kamilmysliwiec,

I am trying Fastify again I still cannot get Middleware to run for a wildcard route.

Are you able to please advise or show us an example where the following will work -

return consumer .apply(AuthenticationMiddleware) .with( { path: '/authentication/' }, { path: '/info/' }, { path: '/config/' }, { path: '/import/' }, { path: '/public/import/' }, { path: '/public/export/' }, ) .forRoutes('*');

The middleware never gets called.

Been stuck over and over again. Either we are missing something or something is broken.

Regards, Tarek

"@nestjs/common": "5.6.2", "@nestjs/core": "5.6.2",

Same here! NestJs 5.6.2. In Fastify is not working, when i switch to Express, it works!

codeserk commented 5 years ago

For me it's not working when I have query params in the request:

/dosomething?pid=561

The middleware is configured so it works for all the endpoints of the controller:

    configure(consumer: MiddlewareConsumer) {
        consumer
            .apply(UserMiddleware)
            .forRoutes(MyController)
      }
madipta commented 5 years ago

i am having this problem too, using nest version: 5.4.0

is this solved yet?

kamilmysliwiec commented 5 years ago

Fixed in 5.7.2 :)

Tsury commented 5 years ago

Currently using v5.7.3 and this doesn't seem to be fixed. I have a very simple middleware set to * route and it does not fire when I use FastifyAdapter. When I disable it, the middleware works perfectly.

kamilmysliwiec commented 5 years ago

@Tsury could you send me a github repository URL? I'll check out locally

Tsury commented 5 years ago

Yes, I'll make some temp repo and link here.

Tsury commented 5 years ago

@kamilmysliwiec There you go

kamilmysliwiec commented 5 years ago

It should be fixed in 5.7.4

blueway commented 5 years ago

@kamilmysliwiec it was not repaired at nestjs@6.0.x,

   //some api :  /v1/get/1
           .forRoutes('/'); //   fastify is not ok,
          .forRoutes('/v1'); // only express is ok,
           .forRoutes('/v1'); // only express is ok,
          .forRoutes('*'); // fastify & express are ok,
kevpogo commented 5 years ago

Hi ! I'm in the last version of nest (6.1.1) and fastify (2.2.0). I've 2 groups of paths : /api and /merch. And I would like to apply one middleware for each paths. So I've try for /api (for testing)

[...]
export class AppModule implements NestModule {
  configure(consumer: MiddlewareConsumer): void {
    if (authConfig.isAuthenticationEnable()) {
      consumer
        .apply(AuthAPIInterceptor)
        .forRoutes({
          path: 'api/*',
          method: RequestMethod.ALL
        });
    }
  }
}

but this doesn't work. If I specify path: '*' this work but for all requests. And I want to apply only on requests begin by 'api' I've try another experience :

JasperP981 commented 5 years ago

For anyone using Fastify trying to solve this issue, one workaround is to check the path within the middleware.

Set your routes to the * wildcard:

consumer.apply(MyCustomMiddleware).forRoutes('*');

Then check the URL within the middleware

export class MyCustomMiddleware {
  use(req, res, next) {
    if (req.url && req.url.startsWith('/admin')) {
      /** Route specific tasks */
    }
    if (req.url && req.url.startsWith('/api')) {
      /** Route specific tasks */
    }
    next();
  }
}
thaoula commented 5 years ago

Hi @kamilmysliwiec,

We are in the process of moving from express to Fastify and have similar issue with the middleware not running when registering in the module using forRoute('*').

I believe the issue occurs when used in conjunction with setGlobalPrefix.

I have attached a simple demo app. The app has a controller and a middleware (it console logs and attaches a user to the request).

To test -

  1. Unzip and npm install.

To test with global prefix =/api

  1. Run npm run start.prefix (this will set the global prefix to '/api'.

To test without the global prefix

  1. Run npm run start.noprefix (this will not set the global prefix to '/api' however it will put the /api in the Controller registration.

  2. Please use GET /api/test to see the result.

Middleware Running You will see a console log statement of Test Middleware the response will contain { name: 'Test', role: 'Tester' };

Middleware Not Running The response will contain { name: 'No User' }

https://github.com/thaoula/nestjs_bugs

kamilmysliwiec commented 5 years ago

So I've just created a plain fastify application:

const instance = fastify();
instance.get('/app/test', (res, rep) => {
  rep.send('Hello world');
});
instance.use('/app/*', (req, res, next) => {
  console.log('request');
  next();
});
instance.listen(3001);

and it doesn't work either. It seems that this issue isn't related to NestJS, but rather to fastify.

thaoula commented 5 years ago

@kamilmysliwiec in my example above I have no problems with the wildcard middleware running when I don't specify the Global Prefix such as '/api'.

To get around the lack of Global Prefix and use nest-router to mount the controllers on /api.

kamilmysliwiec commented 5 years ago

@thaoula it's a different issue - https://github.com/nestjs/nest/issues/2291 (fixed already, will be published soon)

kamilmysliwiec commented 5 years ago

The reason has been described in this post: https://github.com/nestjs/nest/issues/972#issuecomment-516707805

lock[bot] commented 4 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.