Skip to content

add support for undefined value in isOptional #102

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

Merged
merged 1 commit into from
Jul 24, 2017

Conversation

codebycorey
Copy link

@codebycorey codebycorey commented Jul 5, 2017

fixes #101

@codebycorey
Copy link
Author

@pleerock I was wondering if you would look over this PR

Copy link
Contributor

@adnan-kamili adnan-kamili left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't undefined be treated differently as it is not same as empty string or null. Suppose if property is of type boolean it would allow empty string or null to bypass the validation, though they are incorrect (say it is being stored in database), I think there should be a new decorator @IsNotRequired() which explicitly ignores validation if value is only undefined. Or if you want to follow semantics of Typescript, functionality of @IsOptional() and @IsNotRequired() can be switched with above semantics, which would be a breaking change.

@kavinvin
Copy link

kavinvin commented Jul 20, 2017

As @adnan-kamili said, why does @IsOptional also treat empty string as a missing value while it's actually defined and is certainly doesn't missing but only left empty? Moreover, it allows many validation hole; for example, if I use @Contains which checks for substring and then add @Optional, when a user enter an empty string it will be qualified as a valid input.

And it would be great if it both @IsOptional and @IsDefined are each other counterpart by making @IsDefined bypass all other validation like@IsOptional does; as in practice, both null and undefined won't pass any other validation anyway. It's strange if it also output error "number must be larger than 200" while the user doesn't even fill a certain property (which means undefined) or sending null value.
https://github.com/pleerock/class-validator/blob/master/src/validation/Validator.ts#L261

Added: Also, I wonder if @IsDefined should also checks null above from undefined because null is still has a meaning if you mean to keep it in database, while null can mean unknown or hasn't yet been entered by user, undefined means value is missing or unspecified (because of some error or technical intention; like, if you want to left the field use its default value or don't want to update the field). And hence its name @IsDefined, should nullchecking be considered to separate itself to @IsNotNull?

@pleerock pleerock merged commit 2b79e6a into typestack:master Jul 24, 2017
@adnan-kamili
Copy link
Contributor

@pleerock allowing null or empty string to bypass in @IsOptional is a security hole, it would allow null or "" values to bypass for every datatype like boolean, Date, number. To use this decorator one needs to add additional validation checks in the code which sort of defeats the purpose.

@codebycorey
Copy link
Author

@pleerock You merge but never released it to npm.
Anyway you can do that please.

@codebycorey codebycorey deleted the support-undefined-isoptional branch September 22, 2017 20:43
@github-actions
Copy link

github-actions bot commented Aug 5, 2020

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

IsOptional Typescript dialect
4 participants