From c612eef6d52afecf0f4df9bc1eebd7a74356785d Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Wed, 8 Nov 2023 14:43:38 +0100 Subject: [PATCH 1/7] Use etag to use preview cache --- .../controller/files-storage.controller.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts index 736d69e3e29..11232f3ebf2 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts @@ -135,7 +135,7 @@ export class FilesStorageController { @Req() req: Request, @Res({ passthrough: true }) response: Response, @Headers('Range') bytesRange?: string - ): Promise { + ): Promise { const fileResponse = await this.filesStorageUC.downloadPreview( currentUser.userId, params, @@ -143,6 +143,14 @@ export class FilesStorageController { bytesRange ); + if (req.headers['if-none-match'] === fileResponse.etag) { + response.status(304); + + return undefined; + } + + response.set({ ETag: fileResponse.etag }); + const streamableFile = this.streamFileToClient(req, fileResponse, response, bytesRange); return streamableFile; From 1fac3679de742b6cee7af83ef6390a7ff040c347 Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Wed, 8 Nov 2023 15:04:30 +0100 Subject: [PATCH 2/7] Add decorator for header --- .../files-storage/controller/files-storage.controller.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts index 11232f3ebf2..533b4c7c576 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts @@ -134,7 +134,8 @@ export class FilesStorageController { @Query() previewParams: PreviewParams, @Req() req: Request, @Res({ passthrough: true }) response: Response, - @Headers('Range') bytesRange?: string + @Headers('Range') bytesRange?: string, + @Headers('If-None-Match') etag?: string ): Promise { const fileResponse = await this.filesStorageUC.downloadPreview( currentUser.userId, @@ -143,7 +144,7 @@ export class FilesStorageController { bytesRange ); - if (req.headers['if-none-match'] === fileResponse.etag) { + if (etag === fileResponse.etag) { response.status(304); return undefined; From 71d203cb05159a1ff675be90c6391526afff5f94 Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Wed, 8 Nov 2023 15:32:04 +0100 Subject: [PATCH 3/7] Add api test --- .../files-storage-preview.api.spec.ts | 92 +++++++++++++------ 1 file changed, 63 insertions(+), 29 deletions(-) diff --git a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts index f63eef6ac68..ad49c018e4e 100644 --- a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts +++ b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts @@ -1,4 +1,7 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; +import { AntivirusService } from '@infra/antivirus'; +import { PreviewProducer } from '@infra/preview-generator'; +import { S3ClientAdapter } from '@infra/s3-client'; import { EntityManager, ObjectId } from '@mikro-orm/mongodb'; import { ICurrentUser } from '@modules/authentication'; import { JwtAuthGuard } from '@modules/authentication/guard/jwt-auth.guard'; @@ -6,9 +9,6 @@ import { ExecutionContext, INestApplication, NotFoundException, StreamableFile } import { Test, TestingModule } from '@nestjs/testing'; import { ApiValidationError } from '@shared/common'; import { EntityId, Permission } from '@shared/domain'; -import { AntivirusService } from '@infra/antivirus'; -import { PreviewProducer } from '@infra/preview-generator'; -import { S3ClientAdapter } from '@infra/s3-client'; import { cleanupCollections, mapUserToCurrentUser, roleFactory, schoolFactory, userFactory } from '@shared/testing'; import NodeClam from 'clamscan'; import { Request } from 'express'; @@ -63,6 +63,19 @@ class API { }; } + async getPreviewWithEtag(routeName: string, etag: string, query?: string | Record) { + const response = await request(this.app.getHttpServer()) + .get(routeName) + .query(query || {}) + .set('If-None-Match', etag); + + return { + result: response.body as StreamableFile, + error: response.body as ApiValidationError, + status: response.status, + }; + } + async getPreviewBytesRange(routeName: string, bytesRange: string, query?: string | Record) { const response = await request(this.app.getHttpServer()) .get(routeName) @@ -299,34 +312,55 @@ describe('File Controller (API) - preview', () => { return { uploadedFile }; }; - it('should return status 200 for successful download', async () => { - const { uploadedFile } = await setup(); - const query = { - ...defaultQueryParameters, - forceUpdate: false, - }; - - const response = await api.getPreview(`/file/preview/${uploadedFile.id}/${uploadedFile.name}`, query); - - expect(response.status).toEqual(200); + describe('WHEN header contains no etag', () => { + it('should return status 200 for successful download', async () => { + const { uploadedFile } = await setup(); + const query = { + ...defaultQueryParameters, + forceUpdate: false, + }; + + const response = await api.getPreview(`/file/preview/${uploadedFile.id}/${uploadedFile.name}`, query); + + expect(response.status).toEqual(200); + }); + + it('should return status 206 and required headers for the successful partial file stream download', async () => { + const { uploadedFile } = await setup(); + const query = { + ...defaultQueryParameters, + forceUpdate: false, + }; + + const response = await api.getPreviewBytesRange( + `/file/preview/${uploadedFile.id}/${uploadedFile.name}`, + 'bytes=0-', + query + ); + + expect(response.status).toEqual(206); + expect(response.headers['accept-ranges']).toMatch('bytes'); + expect(response.headers['content-range']).toMatch('bytes 0-3/4'); + }); }); - it('should return status 206 and required headers for the successful partial file stream download', async () => { - const { uploadedFile } = await setup(); - const query = { - ...defaultQueryParameters, - forceUpdate: false, - }; - - const response = await api.getPreviewBytesRange( - `/file/preview/${uploadedFile.id}/${uploadedFile.name}`, - 'bytes=0-', - query - ); - - expect(response.status).toEqual(206); - expect(response.headers['accept-ranges']).toMatch('bytes'); - expect(response.headers['content-range']).toMatch('bytes 0-3/4'); + describe('WHEN header contains matching etag', () => { + it('should return status 200 for successful download', async () => { + const { uploadedFile } = await setup(); + const query = { + ...defaultQueryParameters, + forceUpdate: false, + }; + const etag = 'testTag'; + + const response = await api.getPreviewWithEtag( + `/file/preview/${uploadedFile.id}/${uploadedFile.name}`, + etag, + query + ); + + expect(response.status).toEqual(304); + }); }); }); From 35b099aa6abdb4eefec921d670931b5257327791 Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Wed, 8 Nov 2023 15:35:32 +0100 Subject: [PATCH 4/7] Add more api tests --- .../controller/api-test/files-storage-preview.api.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts index ad49c018e4e..c771b022a15 100644 --- a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts +++ b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts @@ -341,6 +341,7 @@ describe('File Controller (API) - preview', () => { expect(response.status).toEqual(206); expect(response.headers['accept-ranges']).toMatch('bytes'); expect(response.headers['content-range']).toMatch('bytes 0-3/4'); + expect(response.headers.etag).toMatch('testTag'); }); }); @@ -403,6 +404,7 @@ describe('File Controller (API) - preview', () => { expect(response.status).toEqual(206); expect(response.headers['accept-ranges']).toMatch('bytes'); expect(response.headers['content-range']).toMatch('bytes 0-3/4'); + expect(response.headers.etag).toMatch('testTag'); }); }); }); From 3807831d608faa2892d6473517f68943c5d5a508 Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Thu, 9 Nov 2023 08:45:28 +0100 Subject: [PATCH 5/7] Add 304 decorator and fix test description --- .../controller/api-test/files-storage-preview.api.spec.ts | 2 +- .../files-storage/controller/files-storage.controller.ts | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts index c771b022a15..9ae6483f23d 100644 --- a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts +++ b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts @@ -346,7 +346,7 @@ describe('File Controller (API) - preview', () => { }); describe('WHEN header contains matching etag', () => { - it('should return status 200 for successful download', async () => { + it('should return status 304', async () => { const { uploadedFile } = await setup(); const query = { ...defaultQueryParameters, diff --git a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts index 533b4c7c576..9a0264c722d 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts @@ -120,6 +120,7 @@ export class FilesStorageController { @ApiOperation({ summary: 'Streamable download of a preview file.' }) @ApiResponse({ status: 200, type: StreamableFile }) @ApiResponse({ status: 206, type: StreamableFile }) + @ApiResponse({ status: 304, description: 'Not Modified' }) @ApiResponse({ status: 400, type: ApiValidationError }) @ApiResponse({ status: 403, type: ForbiddenException }) @ApiResponse({ status: 404, type: NotFoundException }) @@ -144,14 +145,14 @@ export class FilesStorageController { bytesRange ); + response.set({ ETag: fileResponse.etag }); + if (etag === fileResponse.etag) { response.status(304); return undefined; } - response.set({ ETag: fileResponse.etag }); - const streamableFile = this.streamFileToClient(req, fileResponse, response, bytesRange); return streamableFile; From c94809b6b2a0819ec44c6b71821c19cf30adf37e Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Thu, 9 Nov 2023 09:06:06 +0100 Subject: [PATCH 6/7] Use http status enum --- .../files-storage/controller/files-storage.controller.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts index 9a0264c722d..7269336c44c 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.controller.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.controller.ts @@ -148,7 +148,7 @@ export class FilesStorageController { response.set({ ETag: fileResponse.etag }); if (etag === fileResponse.etag) { - response.status(304); + response.status(HttpStatus.NOT_MODIFIED); return undefined; } From 6d0267ed7e24c1f9ba71dc81859ff1e90a8b789f Mon Sep 17 00:00:00 2001 From: Max Bischof Date: Thu, 9 Nov 2023 09:14:39 +0100 Subject: [PATCH 7/7] Add test --- .../files-storage-preview.api.spec.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts index 9ae6483f23d..6ccc6d0d5ea 100644 --- a/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts +++ b/apps/server/src/modules/files-storage/controller/api-test/files-storage-preview.api.spec.ts @@ -345,6 +345,25 @@ describe('File Controller (API) - preview', () => { }); }); + describe('WHEN header contains not matching etag', () => { + it('should return status 200 for successful download', async () => { + const { uploadedFile } = await setup(); + const query = { + ...defaultQueryParameters, + forceUpdate: false, + }; + const etag = 'otherTag'; + + const response = await api.getPreviewWithEtag( + `/file/preview/${uploadedFile.id}/${uploadedFile.name}`, + etag, + query + ); + + expect(response.status).toEqual(200); + }); + }); + describe('WHEN header contains matching etag', () => { it('should return status 304', async () => { const { uploadedFile } = await setup();