feat(@nestjs/common) improve the ValidationPipe #383

fwoelffel · 2018-01-29T14:09:07Z

TLDR; This PR adds some options to the ValidationPipe.

Motivation

Take the following code base:

// pagination.dto.ts
import { IsInt, IsOptional, IsPositive } from "class-validator";
import { Type } from "class-transformer";
export class PaginationOptionsDto {
  @IsInt()
  @IsPositive()
  @IsOptional()
  @Type(() => Number)
  readonly offset?: number;

  @IsInt()
  @IsPositive()
  @IsOptional()
  @Type(() => Number)
  readonly limit?: number;
}

// users.controller.ts
// ...
/**
 * A `GET` request against the `/users?name=James&offset=20&limit=10` endpoint
 * should return a paginated list of the users named James.
 */
@Get('users')
async getUsers(
  @Query(new ValidationPipe()) pagination?: PaginationOptionsDto,
  @Query('name') nameFilter?: string
) {
  console.log(pagination instanceof PaginationOptionsDto);
  console.log(pagination);
  return this.usersService.getUsers({name: nameFilter}, pagination);
}
// ...

Now if you take a look at the console output, you will see that:

pagination is not an instance of PaginationOptionsDto which can cause issues if you want to define some methods in this class.
pagination is an object (any) equal to {name: 'James', offset: 20, limit: 10}.

Weird, uh? I know that "since it's a validation pipe it should only perform validation", but still... weird.

Proposal

This PR adds the support of the following options to the ValidationPipe:

transform: The pipe will return an instance of the expected type.
strip: The pipe will return an instance of the expected type without the extra properties (ie: the name property on the example above). This option implicitly sets transform to true.
reject: The pipe will throw an error if it finds any unexpected property. This option implicitly sets transform and strip to true.

This is what it looks like:

@Get('/users')
async getUsers(
  @Query(new TransformationPipe({strip: true})) pagination?: PaginationOptionsDto,
  @Query('name') nameFilter?: string
) {
  console.log(pagination instanceof PaginationOptionsDto); // true
  console.log(pagination); // {limit: 10, offset: 20}
  return this.usersService.getUsers(pagination, {name: nameFilter});
}

This makes use of the class-validator latest release which provides the whitelisting feature.

coveralls · 2018-01-29T14:16:10Z

Coverage increased (+0.1%) to 94.666% when pulling 27c0839 on fwoelffel:improve-validation-pipe into 1812567 on nestjs:master.

lucasmonstrox · 2018-01-31T19:34:24Z

PLEASE MERGE 🗡

chanlito · 2018-02-02T16:11:06Z

@kamilmysliwiec pls merge this

ShadowManu

I'm not even a common participant in the nest community (just a newcomer) but if you're interested, I gave some input on this PR as I found the Pipe (and its configuration) useful (I may do a following PR on validation groups ;) )

ShadowManu · 2018-02-03T02:45:23Z

src/common/pipes/validation.pipe.ts

+  private shouldReject: boolean;
+
+  constructor(options?: ValidationPipeOptions) {
+    this.shouldTransform = options && (options.transform || options.strip || options.reject);


these || are too opinionated. If I want stripping but not transforming, I can't. ~~Same between strip and reject~~.
There is not an issue at all to let them be independent of each other.

You're right, people might want to strip and don't get the transformed value. However, I this represents the more common usecase. I'm setting transform = true by default.

ShadowManu · 2018-02-03T02:51:03Z

src/common/pipes/validation.pipe.ts

 @Pipe()
 export class ValidationPipe implements PipeTransform<any> {
+
+  private shouldTransform: boolean;


You're using a different naming scheme from class-validator.
Why rename whitelist -> shouldStrip and forbidNonWhitelisted -> shouldReject. You should use same names and not change the mnemonics without a hard reason. You end up loosing the original class-validator documentation references as a user.

fwoelffel · 2018-02-03T15:55:08Z

Thanks for your review, I'm looking into it asap

allow to whitelist and forbid properties without transforming the returned object

adopt the class-validator naming scheme

add support for all class-validator options

fwoelffel · 2018-02-04T00:01:38Z

@ShadowManu My last commit adds support for all class-validator options 😉 However I haven't yet added any test for this

ShadowManu · 2018-02-04T05:06:09Z

src/common/pipes/validation.pipe.ts

-    this.shouldTransform = options && (options.transform || options.strip || options.reject);
-    this.shouldStrip = options && (options.strip || options.reject);
-    this.shouldReject = options && options.reject;
+    this.returnTransformed = (options && 'transform' in options) ? options.transform : true;


Its not particularly good to use the delete thing (specially if that object doesn't start from your code. A recommendation for this body:

const { transform, ...validatorOptions } = options; this.returnTransformed = transform != null ? transform : true; // I don't agree with a default true value, as I said this.validatorOptions = options;

I don't want the transform property to get passed to the validate function. What harm could this delete do? If you have a more elegant solution to remove it from the options object let me know.

As for the transform default value, I think we should wait for more reviews. You wanted to be able to get the plain object value and with transform: false you are 😉
If I set this default value to false we're getting back to @Query(new ValidationPipe()) pagination?: PaginationOptionsDto not being an actual PaginationOptionsDto which is not the expected behavior.

The first line of my snippet separates the transform property from the validatorOptions (it doesnt have transform.

About the typing disparity, while I prefer pojo objects, you are right: the typescript compiler will let you call methods on the object and the default implementation should comply with that.

Well, I didn't know about this use of the spread operator and I must say it looks really nice :D

ShadowManu · 2018-02-04T05:08:35Z

src/common/pipes/validation.pipe.ts

    if (errors.length > 0) {
      throw new BadRequestException(errors);
    }
-    return this.shouldTransform ? entity : value;
+    return this.returnTransformed ? entity


I believe not a lot of people are keen on using nested ternary operators.

More so, why do you need the second one? The classToPlain(entity) part. And I don't get too much requiring Object.keys.

I like nested ternaries.

The second one is needed when you want to get the non-transformed stripped value. (basically, you get the stripped entity as a plain object)

Object.keys is used to check if the user has provided some class-validator options which means the returned object is a mutated version of value (stripped properties, etc.)

ShadowManu · 2018-02-04T05:12:17Z

I may help you with the tests tomorrow.

initialize the properties in a more elegant way

chanlito · 2018-02-04T15:04:27Z

Instead of using a constructor, why not use a decorator maybe @dto() passing options to this decorator, this way we can define just a single new ValidationPipe() using nest global pipe. Less code to write 😀

kamilmysliwiec · 2018-02-04T16:11:42Z

Hi everyone,
Cool PR, will review asap, thanks @fwoelffel ❤️

ShadowManu · 2018-02-04T20:03:10Z

@chanlito for learning purposes, how would you create that decorator: What it decorates, how it works and why it works? Haven't really authored decorators myself.

chanlito · 2018-02-05T01:39:55Z

@ShadowManu I was thinking of something like the following without using the constructor:

@Dto() // or @Dto({ transform: true, strip: true, reject: false })
export class CreateCatsDto {
  @IsNotEmpty()
  @IsString()
  readonly name: string;

  // @Transform() or @Type()
  readonly color: string;
}

Now we can strip unwanted incoming fields, validate and tranform.

With reflect metadata we can achieve the @Dto() above.

Then somewhere in our nest bootstrap, we can do the following.

...
app.useGlobalPipe(new ValidationPipe(this.reflector))
...

Inside the validation pipe we could check if the class has been decorated by @Dto, if it has we run transformation or validation on it, else just return the value. 😉

fwoelffel · 2018-02-05T09:07:45Z

@chanlito
IMO this is not such a good idea because it means that every endpoint will perform the validation with the exact same options. What if I want to strip everywhere, except for one endpoint? I don't think that your proposition would allow that.

lucasmonstrox · 2018-02-05T12:07:56Z

@fwoelffel nice job dude.

chanlito · 2018-02-05T12:26:47Z

@fwoelffel I don't think that your proposition would allow that, yes it does using reflect metadata and a custom decorator e.g. @Dto(), otherwise we'll have to do new ValidationPipe() on every @Body() or @Query()

fwoelffel · 2018-02-05T13:32:05Z

@chanlito How could one activate the stripping for one single controller endpoint and not the other endpoints if the class-validator options are defined in the @Dto decorator and no longer in the ValidationPipe? I think your proposition will reduce the granularity of control we have over the endpoints validation.

kamilmysliwiec · 2018-02-05T14:30:20Z

@fwoelffel this PR looks like a great job 🙂 thanks man, really appreciate it. I'm gonna review today. Question: would you want to update a docs as well? At the moment, we only have a mention that ValidationPipe exists out-of-the-box. I feel like we should provide a small separated chapter that describes those properties and how to use them. Do you want to do that? ❤️

fwoelffel · 2018-02-05T15:24:30Z

@kamilmysliwiec Yup caaan do

kamilmysliwiec · 2018-02-05T15:35:36Z

cool @fwoelffel 😃 This PR is ready to merge 🎉 looking forward to it

kamilmysliwiec · 2018-02-11T11:28:01Z

Great job @fwoelffel, thanks 🎉 Looking forward to those small fixes in the docs PR!

lock · 2019-09-23T17:12:17Z

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.

fwoelffel and others added 2 commits January 29, 2018 14:30

feat(@nestjs/common) improve the ValidationPipe

dc24a20

tests(@nestjs/common): add tests for the ValidationPipe

ba00ce6

fwoelffel mentioned this pull request Feb 2, 2018

ValidationPipe not working #379

Closed

ShadowManu reviewed Feb 3, 2018

View reviewed changes

fwoelffel added 3 commits February 4, 2018 00:43

feat(@nestjs/common): improve the ValidationPipe

eebd72c

allow to whitelist and forbid properties without transforming the returned object

refactor(@nestjs/common): rename the ValidationPipe properties

9992996

adopt the class-validator naming scheme

feat(@nestjs/common): improve the ValidationPipe

ff229b5

add support for all class-validator options

ShadowManu reviewed Feb 4, 2018

View reviewed changes

refactor(@nestjs/common): rewrite the ValidationPipe constructor

aaab2bc

initialize the properties in a more elegant way

kamilmysliwiec mentioned this pull request Feb 5, 2018

Problem with ValidationPipe example #355

Closed

refactor(@nestjs/common): mark the ValidatorPipe parameters as optional

27c0839

fwoelffel mentioned this pull request Feb 5, 2018

docs(@nestjs/common): Update the ValidationPipe documentation nestjs/docs.nestjs.com#46

Merged

kamilmysliwiec merged commit 27c0839 into nestjs:master Feb 11, 2018

fwoelffel deleted the improve-validation-pipe branch May 30, 2019 12:41

lock bot locked as resolved and limited conversation to collaborators Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(@nestjs/common) improve the ValidationPipe #383

feat(@nestjs/common) improve the ValidationPipe #383

fwoelffel commented Jan 29, 2018

coveralls commented Jan 29, 2018 •

edited

Loading

lucasmonstrox commented Jan 31, 2018

chanlito commented Feb 2, 2018

ShadowManu left a comment

ShadowManu Feb 3, 2018 •

edited

Loading

fwoelffel Feb 3, 2018

ShadowManu Feb 3, 2018

fwoelffel commented Feb 3, 2018

fwoelffel commented Feb 4, 2018

ShadowManu Feb 4, 2018

fwoelffel Feb 4, 2018 •

edited

Loading

fwoelffel Feb 4, 2018 •

edited

Loading

ShadowManu Feb 4, 2018

fwoelffel Feb 4, 2018

ShadowManu Feb 4, 2018

ShadowManu Feb 4, 2018

fwoelffel Feb 4, 2018 •

edited

Loading

ShadowManu commented Feb 4, 2018

chanlito commented Feb 4, 2018

kamilmysliwiec commented Feb 4, 2018 •

edited

Loading

ShadowManu commented Feb 4, 2018

chanlito commented Feb 5, 2018

fwoelffel commented Feb 5, 2018

lucasmonstrox commented Feb 5, 2018

chanlito commented Feb 5, 2018

fwoelffel commented Feb 5, 2018

kamilmysliwiec commented Feb 5, 2018

fwoelffel commented Feb 5, 2018 •

edited

Loading

kamilmysliwiec commented Feb 5, 2018

kamilmysliwiec commented Feb 11, 2018

lock bot commented Sep 23, 2019

feat(@nestjs/common) improve the ValidationPipe #383

feat(@nestjs/common) improve the ValidationPipe #383

Conversation

fwoelffel commented Jan 29, 2018

Motivation

Proposal

coveralls commented Jan 29, 2018 • edited Loading

lucasmonstrox commented Jan 31, 2018

chanlito commented Feb 2, 2018

ShadowManu left a comment

Choose a reason for hiding this comment

ShadowManu Feb 3, 2018 • edited Loading

Choose a reason for hiding this comment

fwoelffel Feb 3, 2018

Choose a reason for hiding this comment

ShadowManu Feb 3, 2018

Choose a reason for hiding this comment

fwoelffel commented Feb 3, 2018

fwoelffel commented Feb 4, 2018

ShadowManu Feb 4, 2018

Choose a reason for hiding this comment

fwoelffel Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

fwoelffel Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

ShadowManu Feb 4, 2018

Choose a reason for hiding this comment

fwoelffel Feb 4, 2018

Choose a reason for hiding this comment

ShadowManu Feb 4, 2018

Choose a reason for hiding this comment

ShadowManu Feb 4, 2018

Choose a reason for hiding this comment

fwoelffel Feb 4, 2018 • edited Loading

Choose a reason for hiding this comment

ShadowManu commented Feb 4, 2018

chanlito commented Feb 4, 2018

kamilmysliwiec commented Feb 4, 2018 • edited Loading

ShadowManu commented Feb 4, 2018

chanlito commented Feb 5, 2018

fwoelffel commented Feb 5, 2018

lucasmonstrox commented Feb 5, 2018

chanlito commented Feb 5, 2018

fwoelffel commented Feb 5, 2018

kamilmysliwiec commented Feb 5, 2018

fwoelffel commented Feb 5, 2018 • edited Loading

kamilmysliwiec commented Feb 5, 2018

kamilmysliwiec commented Feb 11, 2018

lock bot commented Sep 23, 2019

coveralls commented Jan 29, 2018 •

edited

Loading

ShadowManu Feb 3, 2018 •

edited

Loading

fwoelffel Feb 4, 2018 •

edited

Loading

fwoelffel Feb 4, 2018 •

edited

Loading

fwoelffel Feb 4, 2018 •

edited

Loading

kamilmysliwiec commented Feb 4, 2018 •

edited

Loading

fwoelffel commented Feb 5, 2018 •

edited

Loading