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/10017 parse file pipe builder #10025

Merged

Conversation

thiagomini
Copy link
Contributor

@thiagomini thiagomini commented Jul 26, 2022

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

What is the new behavior?

Now we can specify if a given file parameter is required in the ParseFilePipeBuilder or in the ParseFilePipe:

@UploadedFile(
  new ParseFilePipeBuilder()
    .addFileTypeValidator({
      fileType: 'json',
    })
    .build({
      fileIsRequired: false,
    }),
)
file?: Express.Multer.File,

With that, when a file is explicitly set to be optional, the ParseFileValidator won't throw an error. On the other hand, when the file is required (default behavior), it will now throw a 400 error.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

ensure ParseFilePipe returns undefined when file is optional
ensure a route with ParseFilePipe and fileIsOptional set to true does not throw when no file is provided
ensure ParseFilePipe throws an error when a file is required but was not passed
ensure ParseFilePipe throws an error if isFileOptional was not explicitly provided and there is no file
ensure sample 29 controller throws an error when a file is required but none is given
Copy link
Contributor Author

@thiagomini thiagomini left a comment

Choose a reason for hiding this comment

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

Some minor fixes I'll work on soon

packages/common/test/pipes/file/parse-file.pipe.spec.ts Outdated Show resolved Hide resolved
packages/common/test/pipes/file/parse-file.pipe.spec.ts Outdated Show resolved Hide resolved
packages/common/test/pipes/file/parse-file.pipe.spec.ts Outdated Show resolved Hide resolved
sample/29-file-upload/e2e/app/app.e2e-spec.ts Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 26, 2022

Pull Request Test Coverage Report for Build 0a52d60b-a134-4623-891a-69fd95493980

  • 7 of 7 (100.0%) changed or added relevant lines in 1 file are covered.
  • 13 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 94.029%

Files with Coverage Reduction New Missed Lines %
packages/core/router/router-explorer.ts 13 79.88%
Totals Coverage Status
Change from base Build 8c0de0f2-40f8-43a4-81d6-106b72b5ecee: 0.006%
Covered Lines: 6063
Relevant Lines: 6448

💛 - Coveralls

Comment on lines 10 to 11
* Defines if file parameter is optional. Default is false.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Defines if file parameter is optional. Default is false.
*/
* Defines if file parameter is optional.
* @default false
*/

/**
* Defines if file parameter is optional. Default is false.
*/
fileIsOptional?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

what about

Suggested change
fileIsOptional?: boolean;
required?: boolean;

and then the default will be true.

@@ -25,13 +27,20 @@ export class ParseFilePipe implements PipeTransform<any> {
exceptionFactory,
errorHttpStatusCode = HttpStatus.BAD_REQUEST,
validators = [],
fileIsRequired,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to required semantic, but I maintained the full word fileIsRequired to be explicit here. In the consumer side, when creating a ParseFilePipe, I think it's worth it to specify what is required:

const parseFilePipe = new ParseFilePipe({
  validators: [],
  fileIsRequired: false,
});

change ParseFilePipe option to use fileIsRequired instead of fileIsOptional
@thiagomini thiagomini force-pushed the fix/10017-parse-file-pipe-builder branch from b273064 to 79041c1 Compare July 27, 2022 14:34
@thiagomini
Copy link
Contributor Author

thiagomini commented Jul 27, 2022

@kamilmysliwiec waiting for your review here. If this is accepted, I suppose we'll have to update the docs as well

@kamilmysliwiec
Copy link
Member

lgtm

@kamilmysliwiec kamilmysliwiec merged commit 2979ea1 into nestjs:master Jul 28, 2022
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

5 participants