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

Express Middleware not being called for some endpoints #13593

Closed
3 of 15 tasks
AbanobNageh opened this issue May 16, 2024 · 23 comments · May be fixed by #14194
Closed
3 of 15 tasks

Express Middleware not being called for some endpoints #13593

AbanobNageh opened this issue May 16, 2024 · 23 comments · May be fixed by #14194
Labels
priority: medium (3) Medium priority issue that needs to be resolved scope: core

Comments

@AbanobNageh
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

This issue happens when there are 2 endpoints:

  • The first endpoint has no params, ex: /all
  • The second endpoint has a param, ex: /: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:

  • the first is /all. The middleware should run for this endpoint.
  • The second is /:id. The middleware should not run for this endpoint.

You can reproduce the issue by using the tests in the repository.

  1. Run the tests from the above repository.
  2. Notice how the /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

  • 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

10.3.8

Packages versions

{
  "name": "nestjs-middleware-exclude-issue",
  "version": "0.0.1",
  "description": "",
  "author": "",
  "private": true,
  "license": "UNLICENSED",
  "scripts": {
    "build": "nest build",
    "format": "prettier --write \"src/**/*.ts\" \"test/**/*.ts\"",
    "start": "nest start",
    "start:dev": "nest start --watch",
    "start:debug": "nest start --debug --watch",
    "start:prod": "node dist/main",
    "lint": "eslint \"{src,apps,libs,test}/**/*.ts\" --fix",
    "test": "jest",
    "test:watch": "jest --watch",
    "test:cov": "jest --coverage",
    "test:debug": "node --inspect-brk -r tsconfig-paths/register -r ts-node/register node_modules/.bin/jest --runInBand",
    "test:e2e": "jest --config ./test/jest-e2e.json"
  },
  "dependencies": {
    "@nestjs/common": "^10.3.8",
    "@nestjs/core": "^10.3.8",
    "@nestjs/platform-express": "^10.3.8",
    "reflect-metadata": "^0.2.2",
    "rxjs": "^7.8.1"
  },
  "devDependencies": {
    "@nestjs/cli": "^10.3.2",
    "@nestjs/schematics": "^10.1.1",
    "@nestjs/testing": "^10.3.8",
    "@types/express": "^4.17.21",
    "@types/jest": "^29.5.12",
    "@types/node": "^20.12.12",
    "@types/supertest": "^6.0.2",
    "@typescript-eslint/eslint-plugin": "~7.9.0",
    "@typescript-eslint/parser": "~7.9.0",
    "eslint": "^8.56.0",
    "eslint-config-prettier": "^9.1.0",
    "eslint-plugin-prettier": "^5.1.3",
    "jest": "^29.7.0",
    "prettier": "^3.2.5",
    "source-map-support": "^0.5.21",
    "supertest": "^7.0.0",
    "ts-jest": "^29.1.2",
    "ts-loader": "^9.5.1",
    "ts-node": "^10.9.2",
    "tsconfig-paths": "^4.2.0",
    "typescript": "^5.4.5"
  },
  "jest": {
    "moduleFileExtensions": [
      "js",
      "json",
      "ts"
    ],
    "rootDir": "src",
    "testRegex": ".*\\.spec\\.ts$",
    "transform": {
      "^.+\\.(t|j)s$": "ts-jest"
    },
    "collectCoverageFrom": [
      "**/*.(t|j)s"
    ],
    "coverageDirectory": "../coverage",
    "testEnvironment": "node"
  }
}

Node.js version

20.10.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@AbanobNageh AbanobNageh added the needs triage This issue has not been looked into label May 16, 2024
@satanshiro
Copy link

satanshiro commented May 21, 2024

there is another configuration where it does not work:
this will ignore previous exclude calls

consumer.apply(someMiddleware)
.exclude('id')
.exclude('somethingelse')

this works

consumer.apply(someMiddleware)
.exclude('id', 'somethingelse')

@micalevisk
Copy link
Member

@satanshiro I guess that's another issue. Would you like to create a PR to fix that one?

@EeeasyCode
Copy link

@AbanobNageh @micalevisk
this works!
I have confirmed that I can use such regular expressions to exclude specific pattern paths from the middleware.

consumer
      .apply(AppMiddleware)
      .exclude({ path: '/:id(\\d+)', method: RequestMethod.GET })
      .forRoutes(AppController);

@EeeasyCode
Copy link

and @satanshiro this code works, too!

consumer
      .apply(AppMiddleware)
      .exclude({ path: 'all', method: RequestMethod.GET })
      .exclude({ path: '/:id(\\d+)', method: RequestMethod.GET })
      .forRoutes(AppController);

@EeeasyCode
Copy link

@micalevisk
Hello, I am a university student from South Korea developing with NestJS.
I would like to contribute to NestJS, but I am not sure where to start. Could you help me?
Someday, I hope to make a significant contribution to the NestJS framework!
Thanks for reading!

@micalevisk
Copy link
Member

@EeeasyCode

  1. read the https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  2. pick any open Issue and try to solve it
    Use our discord server http://discord.gg/nestjs to discuss it further if needed.

@AbanobNageh
Copy link
Author

@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: /:id) we would need to explicitly add the paths that should not match (ex: /all)? If this is the case then this could be an ok temporary solution for small codebases but it doesn't seem like a feasible solution for large codebases where there could be many such endpoints.

@EeeasyCode
Copy link

EeeasyCode commented Jun 10, 2024

@AbanobNageh

I think I found the cause of the issue. When /:id is entered, it receives the id in the format of /1 or /test, and in such cases, it cannot distinguish between @Get('/1') and @Get('/test'). To handle this, you should either use the regular expression I initially suggested or specify the path with a prefix in the Controller.

@satishpatar
Copy link

i have also same issue

@JadenKim-dev
Copy link
Contributor

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.
It seems that we cannot currently use the negative lookahead regular expression.​

SyntaxError: Invalid regular expression:
... Invalid group

@micalevisk, do you have any plans to update the version of path-to-regexp to the latest one?

@micalevisk
Copy link
Member

micalevisk commented Jun 23, 2024

well the latest version of path-to-regexp is v7.0.0, which is too far from v3. We must upgrade it some day but first we would have to see which breaking changes were made in v4, v5, v6 and v7. I guess we can upgrade it to v7 on nestjs v11 right away tho

@kamilmysliwiec
Copy link
Member

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.

@AbanobNageh
Copy link
Author

@kamilmysliwiec In this case, what will happen to this issue? Will this issue be blocked because path-to-regexp can't be upgraded? or do you mean that this middleware issue would be solved in some other way?

@JadenKim-dev
Copy link
Contributor

JadenKim-dev commented Jul 2, 2024

@AbanobNageh
Since the version upgrade is unavailable, you might need to write a regular expression for exclude to filter out only the id.
Your business logic or table schema may vary, but the id values likely share a common characteristic.
For example, they might be in the form of a number, have a length of 8, or be UUIDs.
You need to provide a regular expression supported by path-to-regexp that can filter based on these characteristics.

  1. id is form of number
consumer
    .apply(AppMiddleware)
    .exclude({ path: '/:id(\\d+)', method: RequestMethod.GET })
    .forRoutes(AppController);
  1. id is length of 8
consumer
    .apply(AppMiddleware)
    .exclude({ path: '/:id(\\w{8})', method: RequestMethod.GET })
    .forRoutes(AppController);
  1. id is UUID
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 exclude list without upgrading the version of path-to-regexp for now.

@AbanobNageh
Copy link
Author

@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 BUwQGZVdZGknU72LJ2Vq which, as far as I know, a regex would not be able to differentiate from an endpoint such as /all.

@AbanobNageh
Copy link
Author

In addition, I have another concern. Even if the library path-to-regexp is updated wouldn't this still mean that the solution would still be to manually exclude specific paths from each path added to the middleware? I feel like this would not be easy to do for large code bases and that this logic should be something done internally by Nest. The exclude logic worked correctly in previous versions of Nest such as Nest 6 in which the path-to-regexp library was not used for the excluded routes part.

@JadenKim-dev
Copy link
Contributor

@AbanobNageh
For Firestore’s automatically generated IDs, they all share the common characteristic of being length of 20 and using URL-safe base64 alphabet.
Therefore, I think a regular expression like the following is possible.

consumer
    .apply(AppMiddleware)
    .exclude({
      path: '/:id([a-zA-Z0-9_-]{20})',
      method: RequestMethod.GET,
    })
    .forRoutes(AppController);

@JadenKim-dev
Copy link
Contributor

JadenKim-dev commented Jul 2, 2024

In addition, I have another concern. Even if the library path-to-regexp is updated wouldn't this still mean that the solution would still be to manually exclude specific paths from each path added to the middleware? I feel like this would not be easy to do for large code bases and that this logic should be something done internally by Nest. The exclude logic worked correctly in previous versions of Nest such as Nest 6 in which the path-to-regexp library was not used for the excluded routes part.

I agree with it.
Even if the path-to-regexp version is upgraded, we need to manually register URLs to exclude in regular expression, which is quite cumbersome.
If exclusions were properly handled in previous nestjs versions, it seems that the logic or implementation might have changed.
It needs to be checked.

@JadenKim-dev
Copy link
Contributor

It seems that breaking changes about this issue was done in #3042.
Previously it simply excluded routes listed in exclude from forRoutes.
However, after the changes, the middleware now skips the routes specified in exclude by passing them with next() (code)

@jpage-godaddy
Copy link

jpage-godaddy commented Jul 26, 2024

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 configure method of my module was called.

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 SwaggerModule.

@JadenKim-dev
Copy link
Contributor

@jpage-godaddy
Since the Swagger Module is registered and executed separately from the AppModule, any middleware registered in the AppModule will not apply to the Swagger routes.
The middleware registered through the configure method in the AppModule is applied at the nest app startup, but the logic for generating and setting up the documentation through the Swagger Module is handled separately from the AppModule like below.

  // 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.

@kamilmysliwiec kamilmysliwiec added scope: core priority: medium (3) Medium priority issue that needs to be resolved and removed needs triage This issue has not been looked into labels Nov 21, 2024
@sapenlei
Copy link
Contributor

I made a PR!! #14194

@kamilmysliwiec
Copy link
Member

Let's track this here #14194

@nestjs nestjs locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority: medium (3) Medium priority issue that needs to be resolved scope: core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants