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.9k stars 7.66k forks source link

Query use ParseEnumPipe throw error when query property is optional #12680

Closed recallwei closed 2 days ago

recallwei commented 1 year ago

Is there an existing issue for this?

Current behavior

I want to parse a query property which is an enum type using ParseEnumPipe.

@Post('login')
async login(
    @Query(
      'type',
      new ParseEnumPipe(LoginType, {
        exceptionFactory: () => {
          const i18n = I18nContext.current<I18nTranslations>()!
          return new BadRequestException(
            i18n.t('auth.LOGIN.TYPE_NOT_SUPPORTED')
          )
        }
      })
    )
    type: LoginType,
    @Body() loginDto: LoginDto,
    @I18n() i18n: I18nContext<I18nTranslations>
  ) {}

enum LoginType {
  'username' = 'username',
  'email' = 'email'
}

But if query type is undefined, nest will throw an error below:

image

To avoid this error, I must change type: LoginType to type: string. Is this expected behaviour?

Minimum reproduction code

The above code

Steps to reproduce

Parse an enum query string using ParseEnumPipe.

Expected behavior

type: LoginType should work, expect this query string to be parsed as an enum type.

Package

Other package

No response

NestJS version

10.2.7

Packages versions

{
  "name": "dolphin-admin-nest",
  "description": "Dolphin Admin Nest is the back-end service of Dolphin Admin, based on Nest.js, TypeScript, Prisma and PostgresSQL.",
  "author": "Bruce Song <recall4056@gmail.com> (https://github.com/recallwei/)",
  "scripts": {
    "dev": "nest start -w",
    "dev:docker": "docker-compose up -d && pnpm dev",
    "dev:pm2": "pnpm pm2 start ecosystem.config.js --env development",
    "start": "nest start",
    "debug": "nest start -w",
    "prod": "node dist/main",
    "prod:pm2": "pnpm pm2 reload ecosystem.config.js --env production",
    "build": "nest build",
    "build:webpack": "nest build --webpack",
    "type:check": "tsc --pretty --noEmit",
    "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",
    "prisma:generate": "prisma generate",
    "prisma:generate:watch": "prisma generate --watch",
    "prisma:migrate:dev": "prisma migrate dev --skip-seed",
    "prisma:migrate:deploy": "prisma migrate deploy",
    "prisma:seed": "prisma db seed",
    "prisma:reset": "prisma migrate reset",
    "prisma:studio": "prisma studio",
    "prisma:validate": "prisma validate",
    "prisma:format": "prisma format",
    "cspell:check": "cspell \"**\"",
    "eslint:check": "eslint \"{src,apps,libs,test}/**/*.ts\" --color",
    "eslint:fix": "pnpm eslint:check --fix",
    "prettier:check": "prettier --check --ignore-unknown .",
    "prettier:fix": "prettier --write --ignore-unknown .",
    "cz": "git-cz",
    "preinstall": "npx only-allow pnpm",
    "prepare": "husky install"
  },
  "dependencies": {
    "@nestjs/axios": "^3.0.1",
    "@nestjs/bull": "^10.0.1",
    "@nestjs/common": "^10.2.7",
    "@nestjs/config": "^3.1.1",
    "@nestjs/core": "^10.2.7",
    "@nestjs/event-emitter": "^2.0.2",
    "@nestjs/jwt": "^10.1.1",
    "@nestjs/mapped-types": "2.0.2",
    "@nestjs/mongoose": "^10.0.1",
    "@nestjs/platform-express": "^10.2.7",
    "@nestjs/schedule": "^4.0.0",
    "@nestjs/swagger": "^7.1.14",
    "@nestjs/throttler": "^5.0.1",
    "@node-rs/bcrypt": "^1.7.3",
    "@prisma/client": "5.5.2",
    "axios": "^1.6.0",
    "bull": "^4.11.4",
    "chalk": "^5.3.0",
    "class-transformer": "^0.5.1",
    "class-validator": "^0.14.0",
    "cos-nodejs-sdk-v5": "^2.12.5",
    "cron": "3.1.3",
    "figlet": "^1.7.0",
    "gradient-string": "^2.0.2",
    "joi": "^17.11.0",
    "mongoose": "^8.0.0",
    "multer": "1.4.5-lts.1",
    "nestjs-i18n": "^10.3.7",
    "pug": "^3.0.2",
    "reflect-metadata": "^0.1.13",
    "rxjs": "^7.8.1",
    "uuid": "^9.0.1"
  },
  "devDependencies": {
    "@brucesong/eslint-config-ts": "^1.0.12",
    "@commitlint/cli": "^18.2.0",
    "@commitlint/config-conventional": "^18.1.0",
    "@nestjs/cli": "^10.2.1",
    "@nestjs/schematics": "^10.0.3",
    "@nestjs/testing": "^10.2.7",
    "@swc/cli": "^0.1.62",
    "@swc/core": "^1.3.95",
    "@types/express": "^4.17.20",
    "@types/figlet": "^1.5.7",
    "@types/gradient-string": "^1.1.4",
    "@types/jest": "^29.5.7",
    "@types/multer": "^1.4.9",
    "@types/node": "^20.8.10",
    "@types/supertest": "^2.0.15",
    "@types/uuid": "^9.0.6",
    "commitizen": "^4.3.0",
    "cspell": "^7.3.8",
    "cz-git": "^1.7.1",
    "eslint": "^8.52.0",
    "husky": "^8.0.3",
    "jest": "^29.7.0",
    "lint-staged": "^15.0.2",
    "pm2": "^5.3.0",
    "prettier": "^3.0.3",
    "prisma": "^5.5.2",
    "source-map-support": "^0.5.21",
    "supertest": "^6.3.3",
    "ts-jest": "^29.1.1",
    "ts-loader": "^9.5.0",
    "ts-node": "^10.9.1",
    "tsconfig-paths": "^4.2.0",
    "typescript": "^5.2.2"
  },
  "jest": {
    "moduleFileExtensions": [
      "js",
      "json",
      "ts"
    ],
    "rootDir": "src",
    "testRegex": ".*\\.spec\\.ts$",
    "transform": {
      "^.+\\.(t|j)s$": "ts-jest"
    },
    "collectCoverageFrom": [
      "**/*.(t|j)s"
    ],
    "coverageDirectory": "../coverage",
    "testEnvironment": "node",
    "moduleNameMapper": {
      "@/(.*)": "<rootDir>/$1"
    }
  },
  "prisma": {
    "schema": "prisma/schema.prisma",
    "seed": "ts-node prisma/seed.ts"
  },
  "config": {
    "commitizen": {
      "path": "node_modules/cz-git"
    }
  },
  "private": true,
  "license": "MIT"
}

Node.js version

20 lts

In which operating systems have you tested?

Other

No response

recallwei commented 1 year ago

Maybe the expression above is not good. The query string type here is required. But when it is not passed, should return a friendly error message.

kamilmysliwiec commented 1 year ago

Would you like to create a PR for this issue?

If not, please provide a minimum reproduction repository (Git repository/StackBlitz/CodeSandbox project).

savitakatwe commented 1 year ago

create a custom validation to ensure the parameter exists before trying to parse it. Here's an example: `@Post('login') async login( @Query('type') type: string, @Body() loginDto: LoginDto, @I18n() i18n: I18nContext ) { if (!type || !Object.values(LoginType).includes(type as LoginType)) { const i18n = I18nContext.current()!; throw new BadRequestException(i18n.t('auth.LOGIN.TYPE_NOT_SUPPORTED')); }

// At this point, you know that 'type' exists and is a valid LoginType enum value. // You can now safely use it. } `

This way, you avoid the ParseEnumPipe from directly trying to parse an undefined value.

micalevisk commented 1 year ago

I didn't manage to reproduce that

image

image


why reproductions are required

micalevisk commented 1 year ago

@recallwei please double-check if this isn't due to the line const i18n = I18nContext.current<I18nTranslations>()!

recallwei commented 1 year ago

@recallwei please double-check if this isn't due to the line const i18n = I18nContext.current<I18nTranslations>()!

Sorry for wasting everyone's time. I created a minimal repository for reproduction at codesandbox, but everything is ok. It confused me a lot. The error occurs when the pipeline executes transform.

My origin repo at https://github.com/bit-ocean-studio/dolphin-admin-nest

I tried to make another reproduction at https://codesandbox.io/p/github/bit-ocean-studio/dolphin-admin-nest/main?file=%2Fsrc%2Fmodules%2Fauth%2Fauth.controller.ts%3A32%2C48&workspaceId=9227e773-346e-4830-aeb4-a342dcced73b This is a copy from my repo, I deleted most of the files that are not related to this error to make this repro clean(like database and other modules).

API endpoint is /auth/login

JoCat commented 5 months ago

I ran into a similar problem today. It happens only when I use swc. If I disable it, everything works correctly.

Method example:

export enum City {
  KRD = 'krd',
  SOCHI = 'sochi',
}

...

@Get(':query')
@ApiOperation({
  tags: ['search'],
  summary: 'Search for services/doctors/price by name',
})
@ApiParam({
  name: 'query',
  description: 'Query text',
})
@ApiQuery({
  name: 'city',
  required: false,
  enum: City,
  description:
    'City identifier. May be omitted, default value is `krd`.',
})
@ApiOkResponse({
  description: 'Returns an object with a successful or empty search result',
})
getSearch(
  @Param('query') query: string,
  @Query('city') city: City,
) {
  return this.searchService.search(query, city);
}

I also have ValidationPipe globally installed

app.useGlobalPipes(
  new ValidationPipe({
    whitelist: true,
    transform: true,
  }),
);

If I set the variable type as string then everything works, if I set enum then it doesn't work with swc. When setting ParseEnumPipe the situation is similar.

@Query('city', new ParseEnumPipe(City, { optional: true })) city: City,
micalevisk commented 5 months ago

@JoCat please share a minimum reproduction repository.

recallwei commented 5 months ago

I remember that I also used swc.

JoCat commented 5 months ago

@JoCat please share a minimum reproduction repository.

https://github.com/JoCat/nest-swc-error-reproduction

Tchekda commented 1 month ago

I am facing the exact same issue and disabling SWC indeed helps !

Should we open an issue on swc repo, so a fix can be implemented ?

micalevisk commented 1 month ago

@JoCat not sure if I follow. I just tested your repro and everything went fine here. Both tsc and swc are behaving the same

@Tchekda please share a minimum reproduction repository here if you believe that this is related with Nestjs

Tchekda commented 1 month ago

@micalevisk The repo was already shared above. To be honest, I don't really know whose fault it is (nestjs, swc, class-transformer) but what is clear:

If you use ParseEnumPipe from NestJs with an optional query param, you get an error when the complier is swc but not when it's the default one (webpack ?).

Here was the bug report I made on the discord server:

I have an endpoint with an optional query param called mapType which can be of type MapType (a TS enum with 2 values) My controller function looks like this:

enum MapType {
  regionMap = 'regionMap',
  regionMapPolygon = 'regionMapPolygon',
}
all(
  @Query('mapType', new ParseEnumPipe(MapType)) mapType: MapType,
): Promise<MapResponseDto> {

When there is a mapType param in my request, it all works as expected. But when I don't put it (no key no value) I get a class-transformer error: The error comes from MetadataStorage.getAncestors with Cannot read properties of undefined (reading 'constructor') because it's trying to access target.prototype.constructor After some debugging, here is what I found:

kamilmysliwiec commented 2 days ago

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