From d309fd85d9aa2be179ebbbce94015900fe741a9b Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 15:39:47 -0300 Subject: [PATCH 1/8] feat(common): optional parse file pipe ensure ParseFilePipe returns undefined when file is optional --- .../pipes/file/parse-file-options.interface.ts | 1 + packages/common/pipes/file/parse-file.pipe.ts | 9 +++++++-- .../test/pipes/file/parse-file.pipe.spec.ts | 17 +++++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/common/pipes/file/parse-file-options.interface.ts b/packages/common/pipes/file/parse-file-options.interface.ts index e74630999a6..bc76cfae10b 100644 --- a/packages/common/pipes/file/parse-file-options.interface.ts +++ b/packages/common/pipes/file/parse-file-options.interface.ts @@ -5,4 +5,5 @@ export interface ParseFileOptions { validators?: FileValidator[]; errorHttpStatusCode?: ErrorHttpStatusCode; exceptionFactory?: (error: string) => any; + fileIsOptional?: boolean; } diff --git a/packages/common/pipes/file/parse-file.pipe.ts b/packages/common/pipes/file/parse-file.pipe.ts index 2f04024ea01..424bbeb13ff 100644 --- a/packages/common/pipes/file/parse-file.pipe.ts +++ b/packages/common/pipes/file/parse-file.pipe.ts @@ -1,9 +1,10 @@ +import { isUndefined } from '../../utils/shared.utils'; import { Injectable, Optional } from '../../decorators/core'; import { HttpStatus } from '../../enums'; -import { HttpErrorByCode } from '../../utils/http-error-by-code.util'; import { PipeTransform } from '../../interfaces/features/pipe-transform.interface'; -import { ParseFileOptions } from './parse-file-options.interface'; +import { HttpErrorByCode } from '../../utils/http-error-by-code.util'; import { FileValidator } from './file-validator.interface'; +import { ParseFileOptions } from './parse-file-options.interface'; /** * Defines the built-in ParseFile Pipe. This pipe can be used to validate incoming files @@ -35,6 +36,10 @@ export class ParseFilePipe implements PipeTransform { } async transform(value: any): Promise { + if (isUndefined(value)) { + return value; + } + if (this.validators.length) { await this.validate(value); } diff --git a/packages/common/test/pipes/file/parse-file.pipe.spec.ts b/packages/common/test/pipes/file/parse-file.pipe.spec.ts index ee06ef44b8f..2054ba163a8 100644 --- a/packages/common/test/pipes/file/parse-file.pipe.spec.ts +++ b/packages/common/test/pipes/file/parse-file.pipe.spec.ts @@ -116,5 +116,22 @@ describe('ParseFilePipe', () => { }); }); }); + + describe('when file is optional', () => { + beforeEach(() => { + parseFilePipe = new ParseFilePipe({ + validators: [new AlwaysInvalidValidator({})], + fileIsOptional: true, + }); + }); + + it('should pass validation if no file is provided', async () => { + const requestFile = undefined; + + await expect(parseFilePipe.transform(requestFile)).to.eventually.eql( + requestFile, + ); + }); + }); }); }); From 657428e9f39530b42c527794aed636103a453919 Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 15:50:49 -0300 Subject: [PATCH 2/8] test(sample): add optional file test ensure a route with ParseFilePipe and fileIsOptional set to true does not throw when no file is provided --- sample/29-file-upload/e2e/app/app.e2e-spec.ts | 6 ++++++ sample/29-file-upload/src/app.controller.ts | 8 +++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/sample/29-file-upload/e2e/app/app.e2e-spec.ts b/sample/29-file-upload/e2e/app/app.e2e-spec.ts index 4107251ff62..e079a6e6182 100644 --- a/sample/29-file-upload/e2e/app/app.e2e-spec.ts +++ b/sample/29-file-upload/e2e/app/app.e2e-spec.ts @@ -51,6 +51,12 @@ describe('E2E FileTest', () => { .expect(400); }); + it('should allow for optional file uploads with validation enabled (fixes #10017)', () => { + return request(app.getHttpServer()) + .post('/file/pass-validation') + .expect(201); + }); + afterAll(async () => { await app.close(); }); diff --git a/sample/29-file-upload/src/app.controller.ts b/sample/29-file-upload/src/app.controller.ts index 39d3af1fb75..555ac71f0f7 100644 --- a/sample/29-file-upload/src/app.controller.ts +++ b/sample/29-file-upload/src/app.controller.ts @@ -42,13 +42,15 @@ export class AppController { .addFileTypeValidator({ fileType: 'json', }) - .build(), + .build({ + fileIsOptional: true, + }), ) - file: Express.Multer.File, + file?: Express.Multer.File, ) { return { body, - file: file.buffer.toString(), + file: file?.buffer.toString(), }; } From 091252807cedd6ff0499ee0978784c4b60437e45 Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 16:13:30 -0300 Subject: [PATCH 3/8] feat(common): add required file validation ensure ParseFilePipe throws an error when a file is required but was not passed --- .../pipes/file/parse-file-options.interface.ts | 4 ++++ packages/common/pipes/file/parse-file.pipe.ts | 9 ++++++++- .../test/pipes/file/parse-file.pipe.spec.ts | 17 +++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/packages/common/pipes/file/parse-file-options.interface.ts b/packages/common/pipes/file/parse-file-options.interface.ts index bc76cfae10b..2419b537754 100644 --- a/packages/common/pipes/file/parse-file-options.interface.ts +++ b/packages/common/pipes/file/parse-file-options.interface.ts @@ -5,5 +5,9 @@ export interface ParseFileOptions { validators?: FileValidator[]; errorHttpStatusCode?: ErrorHttpStatusCode; exceptionFactory?: (error: string) => any; + + /** + * Defines if file parameter is optional. Default is false. + */ fileIsOptional?: boolean; } diff --git a/packages/common/pipes/file/parse-file.pipe.ts b/packages/common/pipes/file/parse-file.pipe.ts index 424bbeb13ff..4f7a97a7d64 100644 --- a/packages/common/pipes/file/parse-file.pipe.ts +++ b/packages/common/pipes/file/parse-file.pipe.ts @@ -20,12 +20,14 @@ import { ParseFileOptions } from './parse-file-options.interface'; export class ParseFilePipe implements PipeTransform { protected exceptionFactory: (error: string) => any; private readonly validators: FileValidator[]; + private readonly fileIsOptional: boolean; constructor(@Optional() options: ParseFileOptions = {}) { const { exceptionFactory, errorHttpStatusCode = HttpStatus.BAD_REQUEST, validators = [], + fileIsOptional, } = options; this.exceptionFactory = @@ -33,11 +35,16 @@ export class ParseFilePipe implements PipeTransform { (error => new HttpErrorByCode[errorHttpStatusCode](error)); this.validators = validators; + this.fileIsOptional = fileIsOptional ?? false; } async transform(value: any): Promise { if (isUndefined(value)) { - return value; + if (this.fileIsOptional) { + return value; + } + + throw this.exceptionFactory('File is required'); } if (this.validators.length) { diff --git a/packages/common/test/pipes/file/parse-file.pipe.spec.ts b/packages/common/test/pipes/file/parse-file.pipe.spec.ts index 2054ba163a8..865175d5506 100644 --- a/packages/common/test/pipes/file/parse-file.pipe.spec.ts +++ b/packages/common/test/pipes/file/parse-file.pipe.spec.ts @@ -133,5 +133,22 @@ describe('ParseFilePipe', () => { ); }); }); + + describe('when file is not optional', () => { + beforeEach(() => { + parseFilePipe = new ParseFilePipe({ + validators: [new AlwaysInvalidValidator({})], + fileIsOptional: false, + }); + }); + + it('should throw an error if no file is provided', async () => { + const requestFile = undefined; + + await expect(parseFilePipe.transform(requestFile)).to.be.rejectedWith( + BadRequestException, + ); + }); + }); }); }); From e9aa1ca63961586ae323f4248f04e219bafa0ba3 Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 16:20:01 -0300 Subject: [PATCH 4/8] test(common): add test for implicit values ensure ParseFilePipe throws an error if isFileOptional was not explicitly provided and there is no file --- .../test/pipes/file/parse-file.pipe.spec.ts | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/common/test/pipes/file/parse-file.pipe.spec.ts b/packages/common/test/pipes/file/parse-file.pipe.spec.ts index 865175d5506..b672bb1dd33 100644 --- a/packages/common/test/pipes/file/parse-file.pipe.spec.ts +++ b/packages/common/test/pipes/file/parse-file.pipe.spec.ts @@ -117,7 +117,7 @@ describe('ParseFilePipe', () => { }); }); - describe('when file is optional', () => { + describe('when isFileOptional is true', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ validators: [new AlwaysInvalidValidator({})], @@ -134,7 +134,7 @@ describe('ParseFilePipe', () => { }); }); - describe('when file is not optional', () => { + describe('when isFileOptional is false', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ validators: [new AlwaysInvalidValidator({})], @@ -150,5 +150,21 @@ describe('ParseFilePipe', () => { ); }); }); + + describe('when isFileOptional is not explicitly provided', () => { + beforeEach(() => { + parseFilePipe = new ParseFilePipe({ + validators: [new AlwaysInvalidValidator({})], + }); + }); + + it('should throw an error if no file is provided', async () => { + const requestFile = undefined; + + await expect(parseFilePipe.transform(requestFile)).to.be.rejectedWith( + BadRequestException, + ); + }); + }); }); }); From f45599d64c800ac2d280aa05eebb4a4e755a9fa9 Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 16:26:09 -0300 Subject: [PATCH 5/8] test(sample): add required file test ensure sample 29 controller throws an error when a file is required but none is given --- sample/29-file-upload/e2e/app/app.e2e-spec.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sample/29-file-upload/e2e/app/app.e2e-spec.ts b/sample/29-file-upload/e2e/app/app.e2e-spec.ts index e079a6e6182..8d90148bfe6 100644 --- a/sample/29-file-upload/e2e/app/app.e2e-spec.ts +++ b/sample/29-file-upload/e2e/app/app.e2e-spec.ts @@ -51,6 +51,13 @@ describe('E2E FileTest', () => { .expect(400); }); + it('should throw when file is required but no file is uploaded', async () => { + return request(app.getHttpServer()) + .post('/file/fail-validation') + .send() + .expect(400); + }); + it('should allow for optional file uploads with validation enabled (fixes #10017)', () => { return request(app.getHttpServer()) .post('/file/pass-validation') From a436faa97f7b24101b5a7cfbc2c989d72c90846f Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 16:50:22 -0300 Subject: [PATCH 6/8] refactor(sample/29): remove send method --- sample/29-file-upload/e2e/app/app.e2e-spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/sample/29-file-upload/e2e/app/app.e2e-spec.ts b/sample/29-file-upload/e2e/app/app.e2e-spec.ts index 8d90148bfe6..6f8bc903431 100644 --- a/sample/29-file-upload/e2e/app/app.e2e-spec.ts +++ b/sample/29-file-upload/e2e/app/app.e2e-spec.ts @@ -54,7 +54,6 @@ describe('E2E FileTest', () => { it('should throw when file is required but no file is uploaded', async () => { return request(app.getHttpServer()) .post('/file/fail-validation') - .send() .expect(400); }); From 72bb251465148eb04ba6ea1af9e6783860b70f10 Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Tue, 26 Jul 2022 16:50:58 -0300 Subject: [PATCH 7/8] test(common): reword test blocks --- packages/common/test/pipes/file/parse-file.pipe.spec.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/common/test/pipes/file/parse-file.pipe.spec.ts b/packages/common/test/pipes/file/parse-file.pipe.spec.ts index b672bb1dd33..dfa9113a587 100644 --- a/packages/common/test/pipes/file/parse-file.pipe.spec.ts +++ b/packages/common/test/pipes/file/parse-file.pipe.spec.ts @@ -117,7 +117,7 @@ describe('ParseFilePipe', () => { }); }); - describe('when isFileOptional is true', () => { + describe('when fileIsOptional is true', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ validators: [new AlwaysInvalidValidator({})], @@ -134,7 +134,7 @@ describe('ParseFilePipe', () => { }); }); - describe('when isFileOptional is false', () => { + describe('when fileIsOptional is false', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ validators: [new AlwaysInvalidValidator({})], @@ -151,7 +151,7 @@ describe('ParseFilePipe', () => { }); }); - describe('when isFileOptional is not explicitly provided', () => { + describe('when fileIsOptional is not explicitly provided', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ validators: [new AlwaysInvalidValidator({})], From 79041c11c657670c13c4807bd9f5fc287dc7e80d Mon Sep 17 00:00:00 2001 From: Thiago Martins Date: Wed, 27 Jul 2022 11:34:19 -0300 Subject: [PATCH 8/8] refactor(common): change parse file pipe options change ParseFilePipe option to use fileIsRequired instead of fileIsOptional --- .../file/parse-file-options.interface.ts | 5 ++-- packages/common/pipes/file/parse-file.pipe.ts | 12 +++++----- .../test/pipes/file/parse-file.pipe.spec.ts | 24 +++++++++++++------ sample/29-file-upload/src/app.controller.ts | 2 +- 4 files changed, 27 insertions(+), 16 deletions(-) diff --git a/packages/common/pipes/file/parse-file-options.interface.ts b/packages/common/pipes/file/parse-file-options.interface.ts index 2419b537754..7602f699743 100644 --- a/packages/common/pipes/file/parse-file-options.interface.ts +++ b/packages/common/pipes/file/parse-file-options.interface.ts @@ -7,7 +7,8 @@ export interface ParseFileOptions { exceptionFactory?: (error: string) => any; /** - * Defines if file parameter is optional. Default is false. + * Defines if file parameter is required. + * @default true */ - fileIsOptional?: boolean; + fileIsRequired?: boolean; } diff --git a/packages/common/pipes/file/parse-file.pipe.ts b/packages/common/pipes/file/parse-file.pipe.ts index 4f7a97a7d64..3cda9a5fc53 100644 --- a/packages/common/pipes/file/parse-file.pipe.ts +++ b/packages/common/pipes/file/parse-file.pipe.ts @@ -20,14 +20,14 @@ import { ParseFileOptions } from './parse-file-options.interface'; export class ParseFilePipe implements PipeTransform { protected exceptionFactory: (error: string) => any; private readonly validators: FileValidator[]; - private readonly fileIsOptional: boolean; + private readonly fileIsRequired: boolean; constructor(@Optional() options: ParseFileOptions = {}) { const { exceptionFactory, errorHttpStatusCode = HttpStatus.BAD_REQUEST, validators = [], - fileIsOptional, + fileIsRequired, } = options; this.exceptionFactory = @@ -35,16 +35,16 @@ export class ParseFilePipe implements PipeTransform { (error => new HttpErrorByCode[errorHttpStatusCode](error)); this.validators = validators; - this.fileIsOptional = fileIsOptional ?? false; + this.fileIsRequired = fileIsRequired ?? true; } async transform(value: any): Promise { if (isUndefined(value)) { - if (this.fileIsOptional) { - return value; + if (this.fileIsRequired) { + throw this.exceptionFactory('File is required'); } - throw this.exceptionFactory('File is required'); + return value; } if (this.validators.length) { diff --git a/packages/common/test/pipes/file/parse-file.pipe.spec.ts b/packages/common/test/pipes/file/parse-file.pipe.spec.ts index dfa9113a587..0eecaf98967 100644 --- a/packages/common/test/pipes/file/parse-file.pipe.spec.ts +++ b/packages/common/test/pipes/file/parse-file.pipe.spec.ts @@ -117,11 +117,11 @@ describe('ParseFilePipe', () => { }); }); - describe('when fileIsOptional is true', () => { + describe('when fileIsRequired is false', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ - validators: [new AlwaysInvalidValidator({})], - fileIsOptional: true, + validators: [], + fileIsRequired: false, }); }); @@ -134,11 +134,11 @@ describe('ParseFilePipe', () => { }); }); - describe('when fileIsOptional is false', () => { + describe('when fileIsRequired is true', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ - validators: [new AlwaysInvalidValidator({})], - fileIsOptional: false, + validators: [], + fileIsRequired: true, }); }); @@ -149,9 +149,19 @@ describe('ParseFilePipe', () => { BadRequestException, ); }); + + it('should pass validation if a file is provided', async () => { + const requestFile = { + path: 'some-path', + }; + + await expect(parseFilePipe.transform(requestFile)).to.eventually.eql( + requestFile, + ); + }); }); - describe('when fileIsOptional is not explicitly provided', () => { + describe('when fileIsRequired is not explicitly provided', () => { beforeEach(() => { parseFilePipe = new ParseFilePipe({ validators: [new AlwaysInvalidValidator({})], diff --git a/sample/29-file-upload/src/app.controller.ts b/sample/29-file-upload/src/app.controller.ts index 555ac71f0f7..371a1fd699e 100644 --- a/sample/29-file-upload/src/app.controller.ts +++ b/sample/29-file-upload/src/app.controller.ts @@ -43,7 +43,7 @@ export class AppController { fileType: 'json', }) .build({ - fileIsOptional: true, + fileIsRequired: false, }), ) file?: Express.Multer.File,