Hyphens in parameters silently fail

Sean-Mattingly commented 7 months ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

#snipped
[Nest] 23656  - 04/04/2024, 9:23:28 AM     LOG [RouterExplorer] Mapped {/routes, GET} route +6ms
[Nest] 23656  - 04/04/2024, 9:23:28 AM     LOG [RouterExplorer] Mapped {/routes/:hyphen-param, GET} route +1ms
[Nest] 23656  - 04/04/2024, 9:23:28 AM     LOG [RouterExplorer] Mapped {/routes/dummy/:nohyphenparam, GET} route +1ms

A GET to /routes succeeds.
A GET to /routes/dummy/1 succeeds.
A GET to /routes/1 fails with 404:

{
    "statusCode": 404,
    "message": "Cannot GET /routes/1",
    "error": "Not Found"
}

Minimum reproduction code

https://github.com/Sean-Mattingly/nestjs-hyphen-demo

Steps to reproduce

Steps to reproduce:

npm install
npm start
GET to /routes/1 fails with 404 - route not found.

Expected behavior

In my humble opinion, this is an example where the principle "Dead Programs tell no Lies" applies.

The program knows that the route parameter :hyphen-param is an invalid parameter specification, thus it should throw immediately on startup to inform the dev / user that the route configuration won't work.

Additionally: Thank you for all the work on the nestjs framework. The way it enforces good SOLID and design principles is commendable. I have found myself getting unexpected benefits in design when I make a point to stick with the framework.

Package

[ ] I don't know. Or some 3rd-party package
[X] code>@nestjs/common</code
[ ] 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

9.0.0

Packages versions

"dependencies": { "@nestjs/common": "^9.0.0", "@nestjs/core": "^9.0.0", "@nestjs/platform-express": "^9.0.0", "reflect-metadata": "^0.1.13", "rxjs": "^7.2.0" }, "devDependencies": { "@nestjs/cli": "^9.0.0", "@nestjs/schematics": "^9.0.0", "@nestjs/testing": "^9.0.0", "@types/express": "^4.17.13", "@types/jest": "29.5.1", "@types/node": "18.16.12", "@types/supertest": "^2.0.11", "@typescript-eslint/eslint-plugin": "^5.0.0", "@typescript-eslint/parser": "^5.0.0", "eslint": "^8.0.1", "eslint-config-prettier": "^8.3.0", "eslint-plugin-prettier": "^4.0.0", "jest": "29.5.0", "prettier": "^2.3.2", "source-map-support": "^0.5.20", "supertest": "^6.1.3", "ts-jest": "29.1.0", "ts-loader": "^9.2.3", "ts-node": "^10.0.0", "tsconfig-paths": "4.2.0", "typescript": "^5.0.0" },

Node.js version

20.11.1

In which operating systems have you tested?

[ ] macOS
[X] Windows
[ ] Linux

Other

No response

micalevisk commented 7 months ago

there is a mention in the docs about that. Isn't about hyphens in parameters but rather routes overlapping

I don't think that nestjs could easily detect those unreachable routes without introducing some sort of performance penalty. Also, such behavior might be desirable for some users for whatever reason, so it can't be handled as an error. Do notice that those paths are passed as-is to the http adapter you're using, they are not on nestjs land domain but expressjs/fastify/whatever

thus I'd say that you should use some tool to scan everything and then report unreachable routes. I don't know any so far.

Sean-Mattingly commented 7 months ago

Ah, I see. So a GET to /routes/1-param does work then and the parameter is specified only by :hyphen, as follows:

  @Get(':hyphen-param')
  paramFails(@Param('hyphen') param: number): string {
    console.log(`Hello world! Parameter: ${param}`)
    return this.appService.getHello();
  }

So the decorator @Param('hyphen-param') in my original example:

  @Get(':hyphen-param')
  paramFails(@Param('hyphen-param') param: number): string {
    console.log(`Hello world! Parameter: ${param}`)
    return this.appService.getHello();
  }

is actually an erroneous decorator argument in my case for this particular http adapter -- maybe it works for other adapters? I don't know.

Agreed that this is in the domain of the http adapter.

micalevisk commented 7 months ago

I dislike the current behavior but I couldn't think of any other way that would be both flexible and performant and that won't introduce breaking changes

Because if we delegate that to the http adapter level, we should allow ignoring that checking because other adapter might not have it. I'm thinking on people that want to switch between adapters easily.

kamilmysliwiec commented 7 months ago

I believe this should be rather reported in the fastify/express repository as this is in the domain of the HTTP adapter (as discussed above).

nestjs / nest