Skip to content

Fix: IsOptional decorator incorrectly ignores validations for null va… #2615

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

AntonioJRM1998
Copy link

@AntonioJRM1998 AntonioJRM1998 commented Jun 5, 2025

…lues

Fix implementation to ensure @IsOptional only ignores validations when properties are undefined/missing, not when they're explicitly null.

Description

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

fixes #[issue number], fixes #[issue number]

…lues

Fix implementation to ensure @IsOptional only ignores validations when properties are undefined/missing, not when they're explicitly null.
@francescosalvi
Copy link

This intent behind this change contradicts what's currently stated in the documentation

@IsOptional() Checks if given value is empty (=== null, === undefined) and if so, ignores all the validators on the property.

and what's been the de-facto behavior of the decorator for 5 years.

Aside from that, the change is still incorrect, since a key explicitly set to undefined would trigger validation:

'bar' in { bar: undefined }; // true

The PR should be rejected.

@AntonioJRM1998
Copy link
Author

Thanks for the feedback, I understand that the current behavior of @IsOptional() is to skip all validations if the value is null or undefined, even if the key is explicitly present in the request body. I also see that this behavior has been consistent for years and is part of the public contract.

However, I believe this can lead to unintuitive results — for example, when a client explicitly sends null or undefined, validations like @isnotempty() are silently ignored. In practice, it might be more useful to skip validation only if the key is missing, and still run validations if the key is present, regardless of whether the value is undefined or null.

That’s why I proposed this change. But I completely agree that changing the behavior of @IsOptional() directly would be a breaking change.

Would it make sense to introduce a new decorator, such as @IsOptionalIfPresent() or @ValidateIfKeyExists(), that follows this alternative logic? This way we can cover this use case without affecting existing applications.

@francescosalvi
Copy link

Thanks for the feedback, I understand that the current behavior of @IsOptional() is to skip all validations if the value is null or undefined, even if the key is explicitly present in the request body. I also see that this behavior has been consistent for years and is part of the public contract.

However, I believe this can lead to unintuitive results — for example, when a client explicitly sends null or undefined, validations like @isnotempty() are silently ignored. In practice, it might be more useful to skip validation only if the key is missing, and still run validations if the key is present, regardless of whether the value is undefined or null.

That’s why I proposed this change. But I completely agree that changing the behavior of @IsOptional() directly would be a breaking change.

Would it make sense to introduce a new decorator, such as @IsOptionalIfPresent() or @ValidateIfKeyExists(), that follows this alternative logic? This way we can cover this use case without affecting existing applications.

(Just to avoid confusion: I am not a maintainer/contributor)

speaking of unintuitive results: the whole library requires paying a great deal of attention (and I would add, of trust) to the documentation because inherently everything it does is magic (not really the lib's implementation's fault, but rather of its decorator-based nature); i stumbled upon this PR while looking myself if there was a way to treat null as different from "omitted", so I agree that the need to distinguish the two use cases is real, and I agree that a new distinct decorator would probably the only feasible solution that does not impact preexisting projects.

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

Successfully merging this pull request may close these issues.

2 participants