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

Null value passes when skipMissingProperties set to true #308

Closed
iveselin opened this issue Jan 24, 2019 · 11 comments
Closed

Null value passes when skipMissingProperties set to true #308

iveselin opened this issue Jan 24, 2019 · 11 comments
Labels
type: feature Issues related to new features.
Milestone

Comments

@iveselin
Copy link

iveselin commented Jan 24, 2019

When skipMissingProperties is set to true, if property has null value, it will not validate with IsNotEmpty decorator and null value will be forwarded to controller/service/db repo etc. and cause a crash
Source of error

Example

Json input

{
    "startDate":null 
}

Part of UpdateProjectDto

 @IsNotEmpty()
 @IsDateString()
 readonly startDate: Date;

Nest.js framework controller example of code which will receive poData with startDate set to null and crash at DB query...

@Patch('/:id')
@UsePipes(new ValidationPipe({ skipMissingProperties: true }))
    updateById(@Param('id', new ParseIntPipe()) id, @Body() poData: UpdateProjectDto) {
        //#update route
        return this.projectsService.updateById(id, poData);
   }
@ghost
Copy link

ghost commented Mar 11, 2019

any idea why this might be anyone?

@zhenwenc
Copy link

Just search the source code :-)

// ValidationExecutor#execute
...
            if ((value === null || value === undefined) && this.validatorOptions && this.validatorOptions.skipMissingProperties === true) {
                return;
            }
...

https://github.com/typestack/class-validator/blob/master/src/validation/ValidationExecutor.ts#L100

@jeromesth
Copy link

I'm having a similar issue.
Why would we consider null as equivalent to undefined in this case?
I believe this behavior breaks partial object validation for non-nullable fields.

For example:

    @IsString()
    @IsNotEmpty()
    @IsDefined()
    name: string;

Those two objects would both pass validation when the skipMissingProperties is applied

validate({}, { skipMissingProperties: true})
validate({ "name": null }, { skipMissingProperties: true})

Which, in my case, triggers a database Column 'name' cannot be null error.

Wouldn't it make more sense if skipMissingProperties and the IsDefined decorators relied solely on whether the property is, well, defined instead of also checking if it is null?

@thomas-ama
Copy link

Some properties of my model can be null, some can't. None can be undefined.
In case of PUT / PATCH HTTP requests, I'm using { skipMissingProperties: true} to validate the body of my request because it contains only updated properties.

However:

  • { skipMissingProperties: true} doesn't validate null values
  • @IsDefined() makes no differences between null and undefined
    So, it is not possible to validate correctly the request body.

Solutions:

  • skipMissingProperties could skip only undefined values (but it would introduce breaking changes I guess)
  • Add a skipUndefinedProperties option (quite easy from what I read)

@muyu66
Copy link

muyu66 commented Jun 3, 2019

"skipUndefinedProperties " is right

@henrikra
Copy link
Contributor

henrikra commented Jun 5, 2019

I really also think that handling null and undefined the same doesn't make sense! That is like handling numbers and strings the same 😄

@henrikra
Copy link
Contributor

henrikra commented Jun 5, 2019

People in this thread go to see the PR that could possibly solve this issue and give it a review! #353

@tocosastalo
Copy link

temporary solution

turn off skipMissingProperties: {skipMissingProperties: false}

and use @ValidateIf decorator:

@ValidateIf(o => 'name' in o)
@IsString()
@IsNotEmpty()
readonly name?: string;

@vlapo
Copy link
Contributor

vlapo commented Jul 13, 2019

I agree skipMissingProperties is little bit misleading. We should introduce skipUndefinedProperties and also skipNullProperties. skipMissingProperties should be deprecated and removed in 1.0 version. What do you think about skipNullProperties? Is it useable option, too?

@vlapo vlapo added the type: feature Issues related to new features. label Jul 13, 2019
@iveselin
Copy link
Author

Yes, in some APIs you want to delete null values, and i some you want to save them. These two options would give us the diversity we need.

@lock
Copy link

lock bot commented Sep 13, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: feature Issues related to new features.
Development

No branches or pull requests

8 participants