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 + middleware wildcards #972

Closed
ecrona opened this issue Aug 15, 2018 · 25 comments
Closed

Fastify + middleware wildcards #972

ecrona opened this issue Aug 15, 2018 · 25 comments

Comments

@ecrona
Copy link

ecrona commented Aug 15, 2018

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
Copy link
Member

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
Copy link
Author

ecrona commented Aug 16, 2018

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
Copy link
Member

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

@kamilmysliwiec kamilmysliwiec changed the title Global middleware Fastify + middleware wildcards Aug 16, 2018
@kamilmysliwiec
Copy link
Member

5.2.0 is here!

@thaoula
Copy link

thaoula commented Aug 27, 2018

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
Copy link

ayZagen commented Dec 7, 2018

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
Copy link

thaoula commented Jan 23, 2019

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
Copy link

v1d3rm3 commented Jan 24, 2019

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
Copy link

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)
      }
  • If I remove the query parameter the middleware is run and it works
  • If I remove FastifyAdapter it works with and without query parameter

@madipta
Copy link

madipta commented Feb 4, 2019

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

is this solved yet?

@kamilmysliwiec
Copy link
Member

Fixed in 5.7.2 :)

@Tsury
Copy link

Tsury commented Feb 25, 2019

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
Copy link
Member

kamilmysliwiec commented Feb 25, 2019

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

@Tsury
Copy link

Tsury commented Feb 25, 2019

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

@Tsury
Copy link

Tsury commented Feb 25, 2019

@kamilmysliwiec
Copy link
Member

It should be fixed in 5.7.4

@blueway
Copy link
Contributor

blueway commented Mar 30, 2019

@kamilmysliwiec it was not repaired at [email protected],

   //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
Copy link

kevpogo commented May 9, 2019

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 :

  • If I specify path: 'api/overview' -> this works, I pass through the middleware,
  • If I specify path: 'api/over*iew or path: 'api/over.*iew -> I don't pass through the middleware.

@kamilmysliwiec kamilmysliwiec reopened this May 9, 2019
@JasperP981
Copy link

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
Copy link

thaoula commented Jul 30, 2019

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
Copy link
Member

kamilmysliwiec commented Jul 31, 2019

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
Copy link

thaoula commented Jul 31, 2019

@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
Copy link
Member

@thaoula it's a different issue - #2291 (fixed already, will be published soon)

@kamilmysliwiec
Copy link
Member

The reason has been described in this post: #972 (comment)

@lock
Copy link

lock bot commented Dec 3, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests