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.

Sign up for GitHub

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

Jump to bottom

Feature/4752 file validators pipe #9718

Merged
merged 10 commits into from
Jun 20, 2022
Merged

Feature/4752 file validators pipe #9718

merged 10 commits into from
Jun 20, 2022

Conversation

thiagomini
Copy link
Contributor

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?

We don't have validation options out of the box for the @UploadedFile() decorator.

Issue Number: #4752

What is the new behavior?

We can use some default validators using pipes: MaxFileSizeValidator and FileTypeValidator. Also, we can pass our own validators if they follow the FileValidator interface.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

This still needs tests to verify if it's actually working with files (e2e tests maybe?)
This is a continuation of a previous, stale PR

thiagomini added 7 commits June 2, 2022 14:41
@thiagomini
feat(common): add parse file pipe
e9d311a
@thiagomini
test(common): add test for parse file pipe
466abf6
@thiagomini
fix(test): remove multer types
b277f95
@thiagomini
test(common): fix import to relative path
c43dbd8 
fix import to relative path, the alias was causing cyclic references
@thiagomini
refactor(common): add braces
b178eb5 
add curly braces to if statement for consistency
@thiagomini
test(common): add builder tests
d62d96b 
add tests for parse file pipe builder and refactor associated classes
@thiagomini
docs(common): add validators docs
2ad8648
@thiagomini thiagomini mentioned this pull request Jun 2, 2022
Closed
12 tasks
@coveralls
Copy link

coveralls commented Jun 2, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 951dfe3d-110c-4cb2-a7e2-985cea4aced1

  • 52 of 54 (96.3%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 93.991%
Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/common/pipes/file/file-type.validator.ts 7 8 87.5%
packages/common/pipes/file/max-file-size.validator.ts 5 6 83.33%
Totals Coverage Status
Change from base Build f1c6da32-16aa-4a51-ab88-1d8bea09256a: 0.02%
Covered Lines: 5897
Relevant Lines: 6274
💛 - Coveralls

@kamilmysliwiec
Copy link
Member

This still needs tests to verify if it's actually working with files (e2e tests maybe?)

That sounds great!

@kamilmysliwiec kamilmysliwiec mentioned this pull request Jun 15, 2022
Merged
12 tasks
hebertcisco
hebertcisco reviewed Jun 17, 2022
View reviewed changes
Copy link

@hebertcisco hebertcisco left a comment

Choose a reason for hiding this comment

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

Very good work, we really needed this. I'm already looking forward to using this validator 😀

packages/common/pipes/file/file-validator.interface.ts
/**
* Interface describing FileValidators, which can be added to a {@link ParseFilePipe}.
*/
export abstract class FileValidator<TValidationOptions = Record<string, any>> {
Copy link

Choose a reason for hiding this comment

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

Excellent code structure, it was very well thought out in that aspect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Hebert, although this still lacks promise handling, so I'll have to add that before merging

@kamilmysliwiec
Copy link
Member

Once we have e2e tests in place, would you like to create a PR to the docs @thiagomini (adding this pipe to the list of available pipes etc, and then describing these validators in the File Upload chapter)

@thiagomini
Copy link
Contributor Author

Once we have e2e tests in place, would you like to create a PR to the docs @thiagomini (adding this pipe to the list of available pipes etc, and then describing these validators in the File Upload chapter)

Sure! I'll do that 👍

@thiagomini
refactor(common): parse file pipe
bdd9397 
add async validation feature
@thiagomini
refactor(common): file type validator
ad0d1bc 
add partial mimetype check
@thiagomini
test(sample): add file upload e2e test
2e8426b 
add test using file parse pipe
@thiagomini thiagomini mentioned this pull request Jun 17, 2022
Merged
10 tasks
@thiagomini
Copy link
Contributor Author

thiagomini commented Jun 17, 2022 •
edited
Loading

@kamilmysliwiec added async validation feature here, e2e tests and started creating the Docs PR:

  1. Pipe section
  2. File Upload section

I would appreciate some review on these as well to guarantee it can be well understood.

@thiagomini thiagomini mentioned this pull request Jun 17, 2022
Merged
10 tasks
@micalevisk micalevisk added this to the 9.0.0 milestone Jun 18, 2022
@kamilmysliwiec kamilmysliwiec merged commit 95cea89 into nestjs:9.0.0 Jun 20, 2022
@kamilmysliwiec
Copy link
Member

lgtm

@Avivbens
Copy link

@kamilmysliwiec @thiagomini Can you support regex instead OR in addition to the current type check? Excel files for example can have multiple options for mimetype

@thiagomini
Copy link
Contributor Author

@kamilmysliwiec @thiagomini Can you support regex instead OR in addition to the current type check? Excel files for example can have multiple options for mimetype

I can add that support in a new PR

@Avivbens
Copy link

@thiagomini I'd love to

@thiagomini thiagomini mentioned this pull request Jun 24, 2022
Merged
12 tasks
@thiagomini
Copy link
Contributor Author

@thiagomini I'd love to

created PR to handle it and also added some useful docs about the reliability of this pipe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hebertcisco hebertcisco hebertcisco left review comments

Assignees
No one assigned
Labels
scope: common type: feature 🎉
Projects
None yet
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@thiagomini @coveralls @kamilmysliwiec @Avivbens @hebertcisco @micalevisk