Optional query param with ValidationPipe always converts to NaN

scopsy commented 2 years ago

Is there an existing issue for this?

[X] I have searched the existing issues

Current behavior

We use a validationPipe enabled globally with transform: true. The problem is that we have a query param that is set to be optional, like in the snippet below. And the transform converts it to "NaN" instead of leaving it undefined.

  async getNotificationsFeed(
    @Query('page') page?: number,
    @Query('seen') seen?: boolean
  ) {

seen is always converted to false, even tho I expect it to be undefined or boolean. and page is converted to NaN when the user is not passing a page query param.

When looking at the ValidationPipe code, it's clear why it's happening since the type conversion always happens without checking for undefined values.

transformPrimitive(value, metadata) {
    if (!metadata.data) {
        // leave top-level query/param objects unmodified
        return value;
    }
    const { type, metatype } = metadata;
    if (type !== 'param' && type !== 'query') {
        return value;
    }
    if (metatype === Boolean) {
        return value === true || value === 'true';
    }
    if (metatype === Number) {
        return +value;
    }
    return value;
}

Minimum reproduction code

https://stackblitz.com/edit/nestjs-typescript-starter-mgxaub?file=src%2Fapp.controller.ts,src%2Fmain.ts

Steps to reproduce

Create a new project
Install class-validators and class-transform packages
Add global validation pipe with transform
Add an optional query param with number type
Make a request without page query param and the param will be Undefined

Expected behavior

Was expecting it to respect TS annotations and to allow either undefined or a transformed number

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",
    "class-transformer": "^0.5.1",
    "class-validator": "^0.13.2",
    "reflect-metadata": "^0.1.13",
    "rimraf": "^3.0.2",
    "rxjs": "^7.5.5"
  },
  "devDependencies": {
    "@nestjs/cli": "^9.0.0",
    "@nestjs/schematics": "^9.0.0",
    "@nestjs/testing": "^9.0.0",
    "@types/express": "^4.17.13",
    "@types/jest": "^28.1.4",
    "@types/node": "^18.0.3",
    "@types/supertest": "^2.0.12",
    "@typescript-eslint/eslint-plugin": "^5.30.5",
    "@typescript-eslint/parser": "^5.30.5",
    "eslint": "^8.19.0",
    "eslint-config-prettier": "^8.5.0",
    "eslint-plugin-prettier": "^4.2.1",
    "jest": "^28.1.2",
    "prettier": "^2.7.1",
    "source-map-support": "^0.5.21",
    "supertest": "^6.2.4",
    "ts-jest": "^28.0.5",
    "ts-loader": "^9.3.1",
    "ts-node": "^10.8.2",
    "tsconfig-paths": "^4.0.0",
    "typescript": "^4.7.4"
  },

Node.js version

14.17.6

In which operating systems have you tested?

[X] macOS
[ ] Windows
[X] Linux

Other

Should we add a check for undefined in the transformPrimitive function? We can also add a new parameter to make sure it doesn't break people code.

I can create a PR for this if that's the right direction to solve this.

micalevisk commented 2 years ago

I might be wrong but I think that's due to reflection limitation. We can't tell if that parameter was declared as optional. So if we add some checking for undefined, both
@Query('foo') param: number
@Query('foo') param?: number
will behave the same, right? :thinking:

scopsy commented 2 years ago

Exactly @micalevisk, this will behave just the same for both cases. This is why I thought we could introduce an option and to have another check for undefined variables and just ignore them.

micalevisk commented 2 years ago

as an workaround, you could use @Query() { page, seen }: QueryDTO

kirisakiken commented 1 year ago

@scopsy Have you tried nestjs parsers for primitive types?

  async getNotificationsFeed(
    @Query('page', ParseIntPipe) page?: number,
    @Query('seen', ParseBoolPipe) seen?: boolean
  ) {

scopsy commented 1 year ago

@kirisakiken not yet, will try to check it out 🙏

kamilmysliwiec commented 1 year ago

Let's track this here https://github.com/nestjs/nest/pull/10953

Hareloo commented 10 months ago

This isn't actually solved.. Passing no value to a query param of type number still results in NaN instead of undefined...

micalevisk commented 10 months ago

@Hareloo please open another issue instead

thw0rted commented 4 months ago

This should be fixed when https://github.com/nestjs/nest/pull/12893 is merged. Anyone finding this issue may also be interested in https://github.com/nestjs/nest/issues/13559 which I just opened.

guicuton commented 2 months ago

One way that I use to solve the optional boolean value from query string that always return as false:

  @IsOptional()
  @Transform(({ value }) => {
    return [1, '1', true, 'true'].includes(value);
  })
  active: boolean;

I don't know why but if I set the decorator @IsBoolean() the value always return as false

nestjs / nest