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
66.91k stars 7.55k forks source link

"mapChildrenToValidationErrors" method returns incorrect error message when validating an object which is in an array #12164

Closed anhpt97 closed 1 year ago

anhpt97 commented 1 year ago

Is there an existing issue for this?

Current behavior

I just discovered something that I think could be a bug here: https://github.com/nestjs/nest/blob/master/packages/common/pipes/validation.pipe.ts#L266

When I validate a complex DTO:

import { Type } from 'class-transformer';
import { IsArray, IsNotEmpty, IsString, ValidateNested } from 'class-validator';

export class LoginDto3 {
  @IsString()
  @IsNotEmpty()
  username: string;

  @IsString()
  @IsNotEmpty()
  password: string;
}

export class LoginDto2 {
  @IsArray()
  @ValidateNested({ each: true })
  @Type(() => LoginDto3)
  username: LoginDto3[];

  @IsString()
  @IsNotEmpty()
  password: string;
}

export class LoginDto {
  @ValidateNested()
  @Type(() => LoginDto2)
  username: LoginDto2;

  @IsString()
  @IsNotEmpty()
  password: string;
}

Result: Screenshot from 2023-07-30 19-39-47

Minimum reproduction code

https://stackblitz.com/edit/nestjs-typescript-starter-jtqamz

Steps to reproduce

No response

Expected behavior

I think the code should be fixed to like this: Screenshot from 2023-07-30 20-37-07

Result: Screenshot from 2023-07-30 19-26-10

Package

Other package

No response

NestJS version

No response

Packages versions

Node.js version

No response

In which operating systems have you tested?

Other

No response

anhpt97 commented 1 year ago

P.S. I'm temporarily using a regular expression to fix this issue: message = message.replace(/()(\.)(\d[^.]*)()/g, '$1['$3']$4');

micalevisk commented 1 year ago

I believe that we can't use isNumberString from class-validator because ValidationPipe doesn't requires class-validator (see loadValidator method)

anhpt97 commented 1 year ago

I believe that we can't use isNumberString from class-validator because ValidationPipe doesn't requires class-validator (see loadValidator method)

Yeah, I understand, so I just edited the content of this issue. I used this regular expression: /^\d/.test(error.property) (check if "error.property" must start with a numeric character)

micalevisk commented 1 year ago

What about negative numbers?

anhpt97 commented 1 year ago

What about negative numbers?

Hmmm, so the regular expression would look like this: /^\-?\d/ It seems that I have not been able to cover all possible scenarios.

anhpt97 commented 1 year ago

After a few hours of searching, I think this is probably the best regular expression: /[^\p{L}]/gu

Screenshot from 2023-07-31 00-08-49

In case if the value of "error.property" is all letters, part of the error message will have dot notation style (a.b), otherwise it will have bracket notation style (a['b']). Although I would like it to have dot notation style if error.property = "abc123", for example.

Sorry if my wording is a bit confusing.

micalevisk commented 1 year ago

Ok can you clarify why exactly do you think that the current behavior is wrong? Just to align the expectations here

anhpt97 commented 1 year ago

@micalevisk I set logging in multiple places in 3 functions ("flattenValidationErrors", "mapChildrenToValidationErrors" and "prependConstraintsWithParentProp") and found out the value of "error.property" is having issues. As you can see from the example above, "username.username.0.username" doesn't seem to be correct but should be "username.username[0].username". (Sorry if my answer doesn't really answer your question.)

micalevisk commented 1 year ago

I don't know why we have that dot notation (which is invalid for JS) but there is an unit test asserting such scenario, see:

https://github.com/nestjs/nest/blob/2b1b63a74f76d9816a84ad4638d213663eee2dbe/packages/common/test/pipes/validation.pipe.spec.ts#L179-L183

so, to me, the current behavior seems to be correct as it is aligned with the test suite :thinking:

anhpt97 commented 1 year ago

Hmmm, I haven't read through the "validation.pipe.spec.ts" file so I didn't think it was a correct behavior. If that's the author's expectation then I'll probably solve the problem myself, although I don't think "test.0.prop1" is correct 😀.

kamilmysliwiec commented 1 year ago

This is just an error message - it's not supposed to be parsed and executed as JS instruction, that's why we decided that .0. notation is good enough

anhpt97 commented 1 year ago

Dziękuję za wyjaśnienie, panie Myśliwiec! :)