-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
Express Middleware not being called for some endpoints #13593
Comments
there is another configuration where it does not work: consumer.apply(someMiddleware)
.exclude('id')
.exclude('somethingelse') this works consumer.apply(someMiddleware)
.exclude('id', 'somethingelse') |
@satanshiro I guess that's another issue. Would you like to create a PR to fix that one? |
@AbanobNageh @micalevisk consumer
.apply(AppMiddleware)
.exclude({ path: '/:id(\\d+)', method: RequestMethod.GET })
.forRoutes(AppController); |
and @satanshiro this code works, too! consumer
.apply(AppMiddleware)
.exclude({ path: 'all', method: RequestMethod.GET })
.exclude({ path: '/:id(\\d+)', method: RequestMethod.GET })
.forRoutes(AppController); |
@micalevisk |
|
@EeeasyCode Thank you. However, How did you test this? The code you included doesn't work for me. I tried it on my reproduction repo above and the tests still fail. Also, my knowledge of Regex is lacking so excuse me if I am wrong but are you saying that for every path with a param (ex: |
I think I found the cause of the issue. When |
i have also same issue |
Constructing an exclude pattern using regular expressions to omit “all” could be a viable solution like below. consumer
.apply(AppMiddleware)
.exclude({ path: '/:id((?!all$).*)', method: RequestMethod.GET })
.forRoutes(AppController); However, unfortunately, the version of path-to-regexp in nest is 3.2.0. SyntaxError: Invalid regular expression:
... Invalid group @micalevisk, do you have any plans to update the version of path-to-regexp to the latest one? |
We can't upgrade it as it would introduce a significant breaking change to dozens of existing Nest applications. There are no plans to move from v3 in the coming months/years. |
@kamilmysliwiec In this case, what will happen to this issue? Will this issue be blocked because |
@AbanobNageh
consumer
.apply(AppMiddleware)
.exclude({ path: '/:id(\\d+)', method: RequestMethod.GET })
.forRoutes(AppController);
consumer
.apply(AppMiddleware)
.exclude({ path: '/:id(\\w{8})', method: RequestMethod.GET })
.forRoutes(AppController);
consumer
.apply(AppMiddleware)
.exclude({
path: '/:id([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12})',
method: RequestMethod.GET,
})
.forRoutes(AppController); If path parameter values are given randomly without any common characteristics, it seems difficult to exclude specific path from the |
@JadenKim-dev Thank you for the examples. Unfortunately, this would not work in my case because like you said my IDs don't have common characteristics. I use Firebase's Firestore as a database and the created documents have auto-generated IDs such as |
In addition, I have another concern. Even if the library |
@AbanobNageh consumer
.apply(AppMiddleware)
.exclude({
path: '/:id([a-zA-Z0-9_-]{20})',
method: RequestMethod.GET,
})
.forRoutes(AppController); |
I agree with it. |
Not sure if this relates to the original bug, but I'm having a problem with a very simple setup applying to all routes where my middleware is never called. @Module({})
export class AppModule {
// ...
configure(consumer: MiddlewareConsumer) {
consumer
.apply(BasicMiiddleware)
.forRoutes('*');
}
} @Injectable()
export class BasicMiddleware implements NestMiddleware {
use(req: Request, res: Response, next: NextFunction) {
console.log('This was called?!');
next();
}
} I confirmed that the EDIT Looks like it's just my documentation controller doesn't fire the middleware; other routes do, so maybe the issue is a bug with |
@jpage-godaddy // nest application setup
const app = await NestFactory.create(AppModule);
// swagger documentation setup - it is done separately
const config = new DocumentBuilder()
.setTitle('Cats example')
.setDescription('The cats API description')
.setVersion('1.0')
.addTag('cats')
.build();
const document = SwaggerModule.createDocument(app, config);
SwaggerModule.setup('api', app, document);
await app.listen(3000); If you want to do some tasks before and after the Swagger routes, you should use a global interceptor. |
I made a PR!! #14194 |
Let's track this here #14194 |
Is there an existing issue for this?
Current behavior
This issue happens when there are 2 endpoints:
/all
/:id
If the
/:id
endpoint is excluded from the middleware then the middleware is not called for both of the 2 endpoints.Minimum reproduction code
https://github.com/AbanobNageh/nestjs-middleware-exclude-issue
Steps to reproduce
The above repository has a middleware and the following endpoints:
/all
. The middleware should run for this endpoint./:id
. The middleware should not run for this endpoint.You can reproduce the issue by using the tests in the repository.
/all
endpoint test fails and the/:id
endpoint test succeeds when only the/:id
endpoint is excluded.Expected behavior
Only the excluded endpoints should be excluded from the middleware. The middleware should run for the
/all
endpoint and shouldn't run for the/:id
endpoint.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 package
No response
NestJS version
10.3.8
Packages versions
Node.js version
20.10.0
In which operating systems have you tested?
Other
No response
The text was updated successfully, but these errors were encountered: