Skip to content

PartialType can lead to 500 Internal Server Error #2623

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

Closed
2 of 4 tasks
mwanago opened this issue Sep 20, 2023 · 6 comments
Closed
2 of 4 tasks

PartialType can lead to 500 Internal Server Error #2623

mwanago opened this issue Sep 20, 2023 · 6 comments

Comments

@mwanago
Copy link

mwanago commented Sep 20, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

Let's say we have the following DTO:

import { IsString, IsNotEmpty, IsOptional } from 'class-validator';

export class CreateArticleDto {
  @IsString()
  @IsNotEmpty()
  @IsOptional()
  content?: string;

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

and use it with POST requests. The content is a nullable column in my PostgreSQL database, but title is required.

I would like to create a separate DTO for PATCH requests where every property is optional. I could use PartialType for that.

import { CreateArticleDto } from './create-article.dto';
import { PartialType } from '@nestjs/swagger';

export class UpdateArticleDto extends PartialType(CreateArticleDto) {}

Under the hood, PartialType succesfully applies the IsOptional decorator to both content and title. Unfortunately, there is a significant issue with this approach. The most important thing about the IsOptional decorator is that it allows both undefined and null values.
typestack/class-validator#491

Because of the above, using PartialType with CreateArticleDto does not cause 400 Bad Request when the user provides null for title. The validation does not work as expected, and the application will attempt to create a new row in the database using null as the value for the title column, which is likely to cause 500 Internal Server Error.

In practical terms, with my UpdateArticleDto, the users can't provide a number for the title thanks to @IsString(), but they can provide null. This is because @IsOptional() turns off the validation if the provided value equals null.

Minimum reproduction code

https://github.com/mwanago/nestjs-partial-type-issue

Steps to reproduce

  1. create an article instance in the database
  2. try making a PATCH request and provide null for title

The above results in 500 Internal Server Error

Expected behavior

Instead of 500 Internal Server Error, the class-validator should prevent the user from providing the null value for title. It should still allow the user to skip the title property, effectively setting it to undefined.
To achieve that, we can't use the IsOptional decorator.

Package version

7.1.11

NestJS version

10.1.3

Node.js version

18.6.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

No response

@mwanago mwanago changed the title PartialType can cause 500 Internal Server Error PartialType can lead to 500 Internal Server Error Sep 20, 2023
@kamilmysliwiec
Copy link
Member

I think that's something we should rather report in the class-validator repository

@mwanago
Copy link
Author

mwanago commented Sep 20, 2023

Hi @kamilmysliwiec
I linked to a ticket in the class-validator repository, but it is a few years old. I doubt it will get fixed.
typestack/class-validator#491

If they would fix it, they would probably do it in a way that would require people to pass an optional parameter, or use another decorator to avoid a significant breaking change. Because of that, it would probably require NestJS to adjust the code slightly anyways.

Under the hood, IsOptional acts like a less generic version of the ValidateIf decorator.

@ValidateIf((data, value) => value !== undefined && value !== null)

We could fix the problem on the NestJS side by using ValidateIf instead of IsOptional.

@ValidateIf((data, value) => value !== undefined)

@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this issue?

@tf3
Copy link

tf3 commented Jan 10, 2024

Hi @kamilmysliwiec. I'd be happy to put up a PR for this, since my team also would like this functionality. It looks like @nestjs/mapped-types is what would ultimately need to be updated.

It might be safest to make the new behaviour opt-in, so nothing breaks for people who are relying on the current behaviour. So perhaps PartialType could take an optional config object which has something like an allowNullProperties boolean. If it's not explicitly set to false, we default to the current behaviour. Otherwise, we implement the new behaviour.

If that sounds OK, I'll get started. Or let me know if you have other ideas about this.

@kamilmysliwiec
Copy link
Member

Sounds great @tf3

Let's track this here nestjs/mapped-types#1274

@notShadowM
Copy link

For anyone still encountering issues with PartialType DTOs in NestJS ignoring null values when you need certain validation rules to be applied — there’s a simple solution.

You can pass a second options argument to PartialType, specifically setting skipNullProperties to false. This ensures that validation rules will still be applied to fields that aren't originally marked as optional.

Example:

export class UpdateUserDto extends PartialType(CreateUserDto, {
  skipNullProperties: false,
}) {}

With this, your validation decorators will also trigger on null values for non-optional fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants