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

fix(common)!: when transforming optional boolean parameters on ValidationPipe #10953

Merged
merged 2 commits into from Apr 5, 2023

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Jan 26, 2023

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: the issue #10246 is broader than what this PR address. For optional boolean values, when they are not supplied, the value is parsed to false

What is the new behavior?

(using ValidationPipe({ transform: true }))

This only fixes(?) the following use case:

@Query('foo') foo?: boolean
@Param('foo') foo?: boolean

now leads to foo === undefined if no query/path parameter for foo key was supplied instead of always failing back to false. Note that marking them as optional doesn't matter

Does this PR introduce a breaking change?

  • Yes
  • No
  • Maybe - if people are not treating optional values properly, since, strictly speaking, if the request doesn't have some field (a expected boolean one), the value of that field should be undefined instead of false (unless this was a design decision for ValidationPipe when dealing with primitive values). If this introduces a breaking change, you can close it. If they want to model false as the default value, they should use this:
@Query('foo', new DefaultValue(false)): foo = false

when transforming booleans with non-valid boolean values
@micalevisk micalevisk force-pushed the fix/partial-issue-10246 branch 2 times, most recently from 43bb6c2 to 5f5fe8b Compare January 26, 2023 01:28
@coveralls
Copy link

coveralls commented Jan 26, 2023

Pull Request Test Coverage Report for Build b72a66f9-002f-4c4f-a4a0-6cd60f2101b6

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.002%) to 93.411%

Totals Coverage Status
Change from base Build 6cae7f16-f5dd-4f5d-a093-b62a08f9388d: 0.002%
Covered Lines: 6209
Relevant Lines: 6647

💛 - Coveralls

@micalevisk micalevisk force-pushed the fix/partial-issue-10246 branch 2 times, most recently from 9e3b699 to 1abd56a Compare January 26, 2023 01:36
on `ValidationPipe`, it must not return a valid boolean
@micalevisk micalevisk changed the title fix(common): when transforming optional boolean parameters on ValidationPipe fix(common)!: when transforming optional boolean parameters on ValidationPipe Feb 1, 2023
@damhungthinh
Copy link

May I know when this fixed will be release?
I'm facing with this transform issue too: optional params will be fallback to default value.

Thanks.

@micalevisk
Copy link
Member Author

@damhungthinh it will probably be released with nestjs v10

I have no clue on when tho. Sorry.

@kamilmysliwiec kamilmysliwiec added this to the 10.0.0 milestone Apr 5, 2023
@kamilmysliwiec kamilmysliwiec changed the base branch from master to 10.0.0 April 5, 2023 11:11
@kamilmysliwiec kamilmysliwiec merged commit 252e864 into nestjs:10.0.0 Apr 5, 2023
@micalevisk micalevisk deleted the fix/partial-issue-10246 branch April 6, 2023 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants