-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
fix(common): when transforming undefined numeric values #12893
base: master
Are you sure you want to change the base?
fix(common): when transforming undefined numeric values #12893
Conversation
Transforming numeric values in validationpipe is incorrect when value is undefined closes nestjs#12864
Pull Request Test Coverage Report for Build 4421a15f-4fad-4fed-a0b6-753367b142e5
💛 - Coveralls |
@Hareloo the same issue happens for other types such as Enums and Strings, on both @query and @param decorations. Small example:
https://github.com/nestjs/nest/blob/master/packages/common/pipes/parse-enum.pipe.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change.
if (metatype === Number) { | ||
if (isUndefined(value)) { | ||
// This is a workaround to deal with optional numeric values since | ||
// optional numerics shouldn't be parsed to a valid number when | ||
// they were not defined | ||
return undefined; | ||
} | ||
return +value; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (metatype === Number) { | |
if (isUndefined(value)) { | |
// This is a workaround to deal with optional numeric values since | |
// optional numerics shouldn't be parsed to a valid number when | |
// they were not defined | |
return undefined; | |
} | |
return +value; | |
} | |
// This is a workaround to deal with optional numeric values since | |
// optional numerics shouldn't be parsed to a valid number when | |
// they were not defined | |
if (metatype === Number && !isUndefined(value)) { | |
return +value; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original change by @Hareloo is actually a 1:1 match with the implementation checking for Boolean
type a few lines above. IMO the code is fine as is as it mirrors that code and follows the same pattern (without saying whether that's a good or a bad pattern 🤷).
If we change it here then we should also change the Boolean
implementation as well.
Is this change still blocked? |
@stuarthimmer-loop it's a breaking change so we'll need to wait till the next major release |
Transforming numeric values in validationpipe is incorrect when value is undefined
closes #12864
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
This issue occurs when using ValidationPipe with
transform: true
option set.When this option is set and query/param arguments on an endpoint are used that are optional/have a default value, when no value/invalid value is passed to this query/URL param, it results in a NaN value, instead of the default value or having it be set to undefined in case it's an optional param.
Issue Number: #12864
What is the new behavior?
Values of the sort mentioned will now be parsed as undefined and converted to undefined.
Does this PR introduce a breaking change?