Express Middleware not being called for some endpoints

AbanobNageh commented 1 month ago

Is there an existing issue for this?

[X] 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.

Run the tests from the above repository.
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
[ ] code>@nestjs/common</code
[X] code>@nestjs/core</code
[ ] code>@nestjs/microservices</code
[ ] code>@nestjs/platform-express</code
[ ] code>@nestjs/platform-fastify</code
[ ] code>@nestjs/platform-socket.io</code
[ ] code>@nestjs/platform-ws</code
[ ] code>@nestjs/testing</code
[ ] code>@nestjs/websockets</code
[ ] 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
[X] Windows
[ ] Linux

Other

No response

satanshiro commented 1 month ago

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 commented 1 month ago

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

EeeasyCode commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

@EeeasyCode

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

AbanobNageh commented 3 weeks ago

@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 commented 3 weeks ago

@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 commented 1 week ago

i have also same issue

JadenKim-dev commented 1 week ago

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 commented 1 week ago

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 commented 1 hour ago

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 commented 1 hour ago

@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?

nestjs / nest