nestjs / nest

A progressive Node.js framework for building efficient, scalable, and enterprise-grade server-side applications with TypeScript/JavaScript 🚀
https://nestjs.com
MIT License
67.76k stars 7.64k forks source link

Trailing slash causes get with param to be executed #3077

Closed gultyayev closed 5 years ago

gultyayev commented 5 years ago

Bug Report

Current behavior

I have a controller users with two methods

@Get() findAll() {} and @Get(':id') findOne() {}

If I access a localhost:3000/users it works well, however if I try to access localhost:3000/users/ it causes findOne to be executed and an error is thrown by mongoose that

CastError: Cast to ObjectId failed for value "" at path "_id" for model "User"

Input Code

repo

Expected behavior

The findAll method should be called instead.

Possible Solution

Environment


"@nestjs/common": "^6.7.2",
"@nestjs/core": "^6.7.2",


For Tooling issues:
- Node version: XX  
- Platform:  

Others:

joeyslack commented 5 years ago

AFAIK this is fairly typical for most MVC-like frameworks. Since the framework is simply exploding params based on '/', it expects you are passing an id (which is null).

Although it would be possible to do some magic in the framework to loopback if no param is found, perhaps this is not the right approach. There are legitimate reasons for this, as there could be an error parsing an integer, a bad parameter is passed, or otherwise.

You could, however, just do this check yourself, like:

@Get(':id')
findOne(@Param() params): string {
  if (!params || !params.id) {
    // Use findMany if params aren't specified
    return this.findMany();
  }
}

or be explicit in the path and override default behavior

@Get('users/')
findAllWithTrailingSlash() {
  ...
}

or, mix and match with the @Redirect() decorator, to throw requests at the right action

gultyayev commented 5 years ago

As far as I remember there is no such a problem with Laravel (PHP framework) and this doesn't seem to be "normal".

In what kind of situation would I need this empty parameter? Thus there would be two references to the same method for the same route although in one case it may have a trailing slash and in another may not.

Can you imagine or have encountered a real case where you would need users/ to be treated different than users or why it would be OK?

In terms of "typical for most MVC-like frameworks" don't be like others - be better.

gultyayev commented 5 years ago

The same applies to @Post().

Personally I see two possible solutions to this: 1) Make @${method}(:id) catch only those routes where this "id" is set like "/users/123" and not "users/" 2) Make ":id" optional so that @Get(":id") would catch users/, users and users/123, so that no need to break DRY by repeating same method calls for literally same paths (like suggested above by @joeyslack).

kamilmysliwiec commented 5 years ago

Read more here https://github.com/fastify/fastify/blob/master/docs/Server.md#ignoretrailingslash

You can pass fastify options through FastifyAdapter constructor:

new FastifyAdapter({
 ignoreTrailingSlash: true
}),
lock[bot] commented 4 years ago

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.