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

feat: Add optional support to pipes #11879

Conversation

AbdlrahmanSaberAbdo
Copy link
Contributor

@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo commented Jun 22, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

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: #11865

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo changed the title Optional validation for parse enum pipe Add isOptional property to ParseEnumPipe for optional parameter support Jun 22, 2023
@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo changed the title Add isOptional property to ParseEnumPipe for optional parameter support feat: Add isOptional property to ParseEnumPipe for optional parameter support Jun 22, 2023
…dlrahmanSaberAbdo:AbdlrahmanSaberAbdo/nest into optional-validation-for-parseEnumPipe
@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo force-pushed the optional-validation-for-parseEnumPipe branch from af19852 to 10d5a86 Compare June 23, 2023 07:52
Copy link
Contributor Author

@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo left a comment

Choose a reason for hiding this comment

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

@kamilmysliwiec Thank you for your comments. I have implemented the requested changes. Should I open a new PR specifically for the other pipes? or should we first decide which pipes should be optional?

@coveralls
Copy link

coveralls commented Jun 23, 2023

Pull Request Test Coverage Report for Build 002d47fb-fb1d-47c3-a863-1ceb404778b9

  • 20 of 20 (100.0%) changed or added relevant lines in 5 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 92.863%

Totals Coverage Status
Change from base Build 3f7492c5-30a2-408e-8707-ac4c6ee9465f: 0.02%
Covered Lines: 6363
Relevant Lines: 6852

💛 - Coveralls

@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo force-pushed the optional-validation-for-parseEnumPipe branch from 0fd6722 to 9359160 Compare June 23, 2023 15:10
@micalevisk
Copy link
Member

micalevisk commented Jun 24, 2023

for consistency sake I think we should update all the other pipes here as well

  • parse-array
  • parse-bool
  • parse-float
  • parse-int
  • parse-uuid

@AbdlrahmanSaberAbdo
Copy link
Contributor Author

for consistency sake I think we should update all the other pipes here as well

  • parse-array
  • parse-bool
  • parse-float
  • parse-int
  • parse-uuid

Do you think that should I change the PR title to reflect the changes I made for adding the optional property to pipes, regardless of the branch name? Or would it be better to create a new branch and PR specifically for the other changes related to adding optional parameters to pipes?

@AbdlrahmanSaberAbdo AbdlrahmanSaberAbdo changed the title feat: Add isOptional property to ParseEnumPipe for optional parameter support feat: Add optional support to pipes Jun 25, 2023
@AbdlrahmanSaberAbdo
Copy link
Contributor Author

for consistency sake I think we should update all the other pipes here as well

  • parse-array
  • parse-bool
  • parse-float
  • parse-int
  • parse-uuid

I updated other pipes, if you think that we should do it in another PR let me know. parse-array already has an optional property.

@AbdlrahmanSaberAbdo
Copy link
Contributor Author

@kamilmysliwiec @micalevisk I checked the failed test but did not really get why it fails, I don't think that's related to my changes, any inputs would be appreciated!

Copy link
Contributor

@rubiin rubiin left a comment

Choose a reason for hiding this comment

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

Everything looks good so far, But i think a documentation update about this on the pipes section should follow as well

@kamilmysliwiec kamilmysliwiec merged commit 39b367a into nestjs:master Jun 26, 2023
2 of 3 checks passed
@kamilmysliwiec
Copy link
Member

LGTM

@danewilson
Copy link

danewilson commented Jul 3, 2023

This PR introduced a bug where these validators throw an (unexpected) error if the value is missing and the options argument wasn't supplied:

if (isNil(value) && this.options.optional) { // Cannot read properties of undefined (reading 'optional')
  return value;
}

@AbdlrahmanSaberAbdo
Copy link
Contributor Author

This PR introduced a bug where these validators throw an (unexpected) error if the value is missing and the options argument wasn't supplied:

if (isNil(value) && this.options.optional) { // Cannot read properties of undefined (reading 'optional')
  return value;
}

Thanks for pointing this out

@kamilmysliwiec
Copy link
Member

Fixed in 10.0.5

@igorgbianchi
Copy link

Could someone tells me what I'm doing wrong in this case? I'm trying to use the property optional: true but it keeps giving this error when the query param is not sent:

{
    "message": [
        "property apigeeVersion should not exist"
    ],
    "error": "Bad Request",
    "statusCode": 400
}

The defintion:

@Query('apigeeVersion', new ParseEnumPipe(ApigeeVersionEnum, {optional: true})) apigeeVersion: ApigeeVersionDto

@nestjs/core@10.3.1

@igorgbianchi
Copy link

Could someone tells me what I'm doing wrong in this case? I'm trying to use the property optional: true but it keeps giving this error when the query param is not sent:

{
    "message": [
        "property apigeeVersion should not exist"
    ],
    "error": "Bad Request",
    "statusCode": 400
}

The defintion:

@Query('apigeeVersion', new ParseEnumPipe(ApigeeVersionEnum, {optional: true})) apigeeVersion: ApigeeVersionDto

@nestjs/core@10.3.1

Fixed, changed to:

    @Query('apigeeVersion', new ParseEnumPipe(ApigeeVersionEnum, { optional: true }))
    apigeeVersion: ApigeeVersionEnum

And added before the method:

@ApiQuery({ name: 'apigeeVersion', type: ApigeeVersionDto })

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

7 participants