Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
3 of 15 tasks
recallwei opened this issue Nov 2, 2023 · 14 comments
Closed
3 of 15 tasks

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

recallwei opened this issue Nov 2, 2023 · 14 comments
Labels
needs triage This issue has not been looked into

Comments

@recallwei
Copy link

recallwei commented Nov 2, 2023

Is there an existing issue for this?

  • I have searched the existing issues

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

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

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 <[email protected]> (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?

  • macOS
  • Windows
  • Linux

Other

No response

@recallwei recallwei added the needs triage This issue has not been looked into label Nov 2, 2023
@recallwei
Copy link
Author

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
Copy link
Member

kamilmysliwiec commented Nov 2, 2023

Would you like to create a PR for this issue?

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

@savitakatwe
Copy link

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
Copy link
Member

micalevisk commented Nov 2, 2023

I didn't manage to reproduce that

image

image


why reproductions are required

@micalevisk
Copy link
Member

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

@recallwei
Copy link
Author

@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
Copy link

JoCat commented Jun 19, 2024

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
Copy link
Member

@JoCat please share a minimum reproduction repository.

@recallwei
Copy link
Author

I remember that I also used swc.

@JoCat
Copy link

JoCat commented Jun 21, 2024

@JoCat please share a minimum reproduction repository.

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

@Tchekda
Copy link

Tchekda commented Oct 8, 2024

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 micalevisk reopened this Oct 8, 2024
@micalevisk
Copy link
Member

@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

@micalevisk micalevisk closed this as not planned Won't fix, can't repro, duplicate, stale Oct 9, 2024
@Tchekda
Copy link

Tchekda commented Oct 12, 2024

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

  • The target passed to that variable is a plain object representing my enum, which explains why it doesn't have any prorotype: { regionMap: 'regionMap', regionMapPolygon: 'regionMapPolygon' }
  • First I thought it was coming from my ParseEnumPipe but the transform function is never called when the error occurs
  • After going deep into class-transformer stack-trace, I found that the query value is first transformed into the TS type (MapType in my case) and then passed to the pipe.
  • When changing the query param type, the issue goes away as target in class-transformer is [Function: String]:
all(
  @Query('mapType', new ParseEnumPipe(MapType)) mapType: string,
): Promise<MapResponseDto> {
  • But then TS yells at me because the controller's code is expecting an enum type and not a generic string.
  • I also tried the following code which passes [Function: Object] as target :
all(
  @Query('mapType', new ParseEnumPipe(MapType)) mapType: keyof typeof MapType,
): Promise<MapResponseDto> {
  • Also not a perfect solution but somehow works with valid, invalid and empty query params

@kamilmysliwiec
Copy link
Member

Let's track this here #14181

@nestjs nestjs locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

No branches or pull requests

6 participants