diff --git a/apps/server/src/modules/files-storage/controller/files-storage.consumer.spec.ts b/apps/server/src/modules/files-storage/controller/files-storage.consumer.spec.ts index 124f486dc68..81fd2884347 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.consumer.spec.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.consumer.spec.ts @@ -7,6 +7,7 @@ import { courseFactory, fileRecordFactory, setupEntities } from '@shared/testing import { LegacyLogger } from '@src/core/logger'; import { FileRecord, FileRecordParentType } from '../entity'; import { FilesStorageService } from '../service/files-storage.service'; +import { PreviewService } from '../service/preview.service'; import { FileRecordResponse } from './dto'; import { FilesStorageConsumer } from './files-storage.consumer'; @@ -33,6 +34,10 @@ describe('FilesStorageConsumer', () => { provide: FilesStorageService, useValue: createMock(), }, + { + provide: PreviewService, + useValue: createMock(), + }, { provide: LegacyLogger, useValue: createMock(), @@ -165,9 +170,9 @@ describe('FilesStorageConsumer', () => { const parentId = new ObjectId().toHexString(); const fileRecords = fileRecordFactory.buildList(3); - filesStorageService.deleteFilesOfParent.mockResolvedValue([fileRecords, fileRecords.length]); + filesStorageService.getFileRecordsOfParent.mockResolvedValue([fileRecords, fileRecords.length]); - return { parentId }; + return { parentId, fileRecords }; }; it('should call filesStorageService.deleteFilesOfParent with params', async () => { @@ -175,7 +180,15 @@ describe('FilesStorageConsumer', () => { await service.deleteFilesOfParent(parentId); - expect(filesStorageService.deleteFilesOfParent).toBeCalledWith(parentId); + expect(filesStorageService.getFileRecordsOfParent).toBeCalledWith(parentId); + }); + + it('should call filesStorageService.deleteFilesOfParent with params', async () => { + const { parentId, fileRecords } = setup(); + + await service.deleteFilesOfParent(parentId); + + expect(filesStorageService.deleteFilesOfParent).toBeCalledWith(fileRecords); }); it('should return array instances of FileRecordResponse', async () => { @@ -191,7 +204,7 @@ describe('FilesStorageConsumer', () => { const setup = () => { const parentId = new ObjectId().toHexString(); - filesStorageService.deleteFilesOfParent.mockResolvedValue([[], 0]); + filesStorageService.getFileRecordsOfParent.mockResolvedValue([[], 0]); return { parentId }; }; diff --git a/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts b/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts index e7dee3c6b0d..aabefa60f16 100644 --- a/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts +++ b/apps/server/src/modules/files-storage/controller/files-storage.consumer.ts @@ -7,12 +7,14 @@ import { LegacyLogger } from '@src/core/logger'; import { FilesStorageEvents, FilesStorageExchange, ICopyFileDO, IFileDO } from '@src/shared/infra/rabbitmq'; import { FilesStorageMapper } from '../mapper'; import { FilesStorageService } from '../service/files-storage.service'; +import { PreviewService } from '../service/preview.service'; import { CopyFilesOfParentPayload, FileRecordParams } from './dto'; @Injectable() export class FilesStorageConsumer { constructor( private readonly filesStorageService: FilesStorageService, + private readonly previewService: PreviewService, private logger: LegacyLogger, // eslint-disable-next-line @typescript-eslint/no-unused-vars private readonly orm: MikroORM // don't remove it, we need it for @UseRequestContext @@ -61,7 +63,11 @@ export class FilesStorageConsumer { public async deleteFilesOfParent(@RabbitPayload() payload: EntityId): Promise> { this.logger.debug({ action: 'deleteFilesOfParent', payload }); - const [fileRecords, total] = await this.filesStorageService.deleteFilesOfParent(payload); + const [fileRecords, total] = await this.filesStorageService.getFileRecordsOfParent(payload); + + await this.previewService.deletePreviews(fileRecords); + await this.filesStorageService.deleteFilesOfParent(fileRecords); + const response = FilesStorageMapper.mapToFileRecordListResponse(fileRecords, total); return { message: response.data }; 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 564919670e4..736d69e3e29 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 @@ -1,3 +1,4 @@ +import { Authenticate, CurrentUser, ICurrentUser } from '@modules/authentication'; import { BadRequestException, Body, @@ -22,10 +23,10 @@ import { UseInterceptors, } from '@nestjs/common'; import { ApiConsumes, ApiHeader, ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; -import { ApiValidationError, RequestLoggingInterceptor } from '@shared/common'; +import { ApiValidationError, RequestLoggingInterceptor, RequestTimeout } from '@shared/common'; import { PaginationParams } from '@shared/controller'; -import { ICurrentUser, Authenticate, CurrentUser } from '@modules/authentication'; import { Request, Response } from 'express'; +import { config } from '../files-storage.config'; import { GetFileResponse } from '../interface'; import { FilesStorageMapper } from '../mapper'; import { FileRecordMapper } from '../mapper/file-record.mapper'; @@ -126,6 +127,7 @@ export class FilesStorageController { @ApiResponse({ status: 500, type: InternalServerErrorException }) @ApiHeader({ name: 'Range', required: false }) @Get('/preview/:fileRecordId/:fileName') + @RequestTimeout(config().INCOMING_REQUEST_TIMEOUT) async downloadPreview( @Param() params: DownloadFileParams, @CurrentUser() currentUser: ICurrentUser, diff --git a/apps/server/src/modules/files-storage/dto/file.dto.ts b/apps/server/src/modules/files-storage/dto/file.dto.ts index 540e35bb7e6..9668ac3af72 100644 --- a/apps/server/src/modules/files-storage/dto/file.dto.ts +++ b/apps/server/src/modules/files-storage/dto/file.dto.ts @@ -1,6 +1,7 @@ +import { File } from '@shared/infra/s3-client'; import { Readable } from 'stream'; -export class FileDto { +export class FileDto implements File { constructor(file: FileDto) { this.name = file.name; this.data = file.data; diff --git a/apps/server/src/modules/files-storage/service/files-storage-delete.service.spec.ts b/apps/server/src/modules/files-storage/service/files-storage-delete.service.spec.ts index cd76b564b31..3705f93b51c 100644 --- a/apps/server/src/modules/files-storage/service/files-storage-delete.service.spec.ts +++ b/apps/server/src/modules/files-storage/service/files-storage-delete.service.spec.ts @@ -171,35 +171,13 @@ describe('FilesStorageService delete methods', () => { return { parentId, fileRecords }; }; - it('should call findBySchoolIdAndParentId once with correct params', async () => { - const { parentId } = setup(); - - await service.deleteFilesOfParent(parentId); - - expect(fileRecordRepo.findByParentId).toHaveBeenNthCalledWith(1, parentId); - }); - it('should call delete with correct params', async () => { - const { parentId, fileRecords } = setup(); + const { fileRecords } = setup(); - await service.deleteFilesOfParent(parentId); + await service.deleteFilesOfParent(fileRecords); expect(service.delete).toHaveBeenCalledWith(fileRecords); }); - - it('should return file records and count', async () => { - const { parentId, fileRecords } = setup(); - - const responseData = await service.deleteFilesOfParent(parentId); - expect(responseData[0]).toEqual( - expect.arrayContaining([ - expect.objectContaining({ ...fileRecords[0] }), - expect.objectContaining({ ...fileRecords[1] }), - expect.objectContaining({ ...fileRecords[2] }), - ]) - ); - expect(responseData[1]).toEqual(fileRecords.length); - }); }); describe('WHEN no files exists', () => { @@ -215,43 +193,17 @@ describe('FilesStorageService delete methods', () => { const { parentId } = params; spy = jest.spyOn(service, 'delete'); - fileRecordRepo.findByParentId.mockResolvedValueOnce([fileRecords, fileRecords.length]); - return { parentId }; + return { parentId, fileRecords }; }; it('should not call delete', async () => { - const { parentId } = setup(); + const { fileRecords } = setup(); - await service.deleteFilesOfParent(parentId); + await service.deleteFilesOfParent(fileRecords); expect(service.delete).toHaveBeenCalledTimes(0); }); - - it('should return empty counted type', async () => { - const { parentId } = setup(); - - const result = await service.deleteFilesOfParent(parentId); - - expect(result).toEqual([[], 0]); - }); - }); - - describe('WHEN repository throw an error', () => { - const setup = () => { - const { params } = buildFileRecordsWithParams(); - const { parentId } = params; - - fileRecordRepo.findByParentId.mockRejectedValueOnce(new Error('bla')); - - return { parentId }; - }; - - it('should pass the error', async () => { - const { parentId } = setup(); - - await expect(service.deleteFilesOfParent(parentId)).rejects.toThrow(new Error('bla')); - }); }); describe('WHEN service.delete throw an error', () => { @@ -272,9 +224,9 @@ describe('FilesStorageService delete methods', () => { }; it('should pass the error', async () => { - const { parentId } = setup(); + const { fileRecords } = setup(); - await expect(service.deleteFilesOfParent(parentId)).rejects.toThrow(new Error('bla')); + await expect(service.deleteFilesOfParent(fileRecords)).rejects.toThrow(new Error('bla')); }); }); }); diff --git a/apps/server/src/modules/files-storage/service/files-storage.service.ts b/apps/server/src/modules/files-storage/service/files-storage.service.ts index 0b851ac8de0..209f1804d3e 100644 --- a/apps/server/src/modules/files-storage/service/files-storage.service.ts +++ b/apps/server/src/modules/files-storage/service/files-storage.service.ts @@ -304,14 +304,10 @@ export class FilesStorageService { await this.deleteWithRollbackByError(fileRecords); } - public async deleteFilesOfParent(parentId: EntityId): Promise> { - const [fileRecords, count] = await this.getFileRecordsOfParent(parentId); - - if (count > 0) { + public async deleteFilesOfParent(fileRecords: FileRecord[]): Promise { + if (fileRecords.length > 0) { await this.delete(fileRecords); } - - return [fileRecords, count]; } // restore diff --git a/apps/server/src/modules/files-storage/service/preview.service.spec.ts b/apps/server/src/modules/files-storage/service/preview.service.spec.ts index 3dd334a4da2..f02f48aee21 100644 --- a/apps/server/src/modules/files-storage/service/preview.service.spec.ts +++ b/apps/server/src/modules/files-storage/service/preview.service.spec.ts @@ -594,10 +594,10 @@ describe('PreviewService', () => { }; }; - it('does not pass error', async () => { - const { fileRecord } = setup(); + it('should throw error', async () => { + const { fileRecord, error } = setup(); - await previewService.deletePreviews([fileRecord]); + await expect(previewService.deletePreviews([fileRecord])).rejects.toThrowError(error); }); }); }); diff --git a/apps/server/src/modules/files-storage/service/preview.service.ts b/apps/server/src/modules/files-storage/service/preview.service.ts index 8e5a1470784..2d56ec91b0d 100644 --- a/apps/server/src/modules/files-storage/service/preview.service.ts +++ b/apps/server/src/modules/files-storage/service/preview.service.ts @@ -45,27 +45,17 @@ export class PreviewService { bytesRange, }; - if (previewParams.forceUpdate) { - await this.generatePreview(previewFileParams); - } - const response = await this.tryGetPreviewOrGenerate(previewFileParams); return response; } public async deletePreviews(fileRecords: FileRecord[]): Promise { - try { - const paths = fileRecords.map((fileRecord) => - createPreviewDirectoryPath(fileRecord.getSchoolId(), fileRecord.id) - ); + const paths = fileRecords.map((fileRecord) => createPreviewDirectoryPath(fileRecord.getSchoolId(), fileRecord.id)); - const promises = paths.map((path) => this.storageClient.deleteDirectory(path)); + const promises = paths.map((path) => this.storageClient.deleteDirectory(path)); - await Promise.all(promises); - } catch (error) { - this.logger.warn(error); - } + await Promise.all(promises); } private checkIfPreviewPossible(fileRecord: FileRecord): void | UnprocessableEntityException { @@ -79,6 +69,10 @@ export class PreviewService { let file: GetFileResponse; try { + if (params.previewParams.forceUpdate) { + await this.generatePreview(params); + } + file = await this.getPreviewFile(params); } catch (error) { if (!(error instanceof NotFoundException)) { diff --git a/apps/server/src/modules/files-storage/uc/files-storage-delete.uc.spec.ts b/apps/server/src/modules/files-storage/uc/files-storage-delete.uc.spec.ts index a1aaf0342ee..b12006367aa 100644 --- a/apps/server/src/modules/files-storage/uc/files-storage-delete.uc.spec.ts +++ b/apps/server/src/modules/files-storage/uc/files-storage-delete.uc.spec.ts @@ -1,5 +1,6 @@ import { createMock, DeepMocked } from '@golevelup/ts-jest'; import { ObjectId } from '@mikro-orm/mongodb'; +import { AuthorizationReferenceService } from '@modules/authorization/domain'; import { HttpService } from '@nestjs/axios'; import { ForbiddenException } from '@nestjs/common'; import { Test, TestingModule } from '@nestjs/testing'; @@ -8,7 +9,6 @@ import { AntivirusService } from '@shared/infra/antivirus'; import { S3ClientAdapter } from '@shared/infra/s3-client'; import { fileRecordFactory, setupEntities } from '@shared/testing'; import { LegacyLogger } from '@src/core/logger'; -import { AuthorizationReferenceService } from '@modules/authorization/domain'; import { FileRecordParams } from '../controller/dto'; import { FileRecord, FileRecordParentType } from '../entity'; import { FileStorageAuthorizationContext } from '../files-storage.const'; @@ -123,7 +123,7 @@ describe('FilesStorageUC delete methods', () => { const mockedResult = [[fileRecord], 0] as Counted; authorizationReferenceService.checkPermissionByReferences.mockResolvedValueOnce(); - filesStorageService.deleteFilesOfParent.mockResolvedValueOnce(mockedResult); + filesStorageService.getFileRecordsOfParent.mockResolvedValueOnce(mockedResult); return { params, userId, mockedResult, requestParams, fileRecord }; }; @@ -143,11 +143,11 @@ describe('FilesStorageUC delete methods', () => { }); it('should call service with correct params', async () => { - const { requestParams, userId } = setup(); + const { requestParams, userId, fileRecord } = setup(); await filesStorageUC.deleteFilesOfParent(userId, requestParams); - expect(filesStorageService.deleteFilesOfParent).toHaveBeenCalledWith(requestParams.parentId); + expect(filesStorageService.deleteFilesOfParent).toHaveBeenCalledWith([fileRecord]); }); it('should call deletePreviews', async () => { @@ -189,10 +189,14 @@ describe('FilesStorageUC delete methods', () => { describe('WHEN service throws error', () => { const setup = () => { + const { fileRecords } = buildFileRecordsWithParams(); + + authorizationReferenceService.checkPermissionByReferences.mockResolvedValueOnce(); const { requestParams, userId } = createParams(); const error = new Error('test'); authorizationReferenceService.checkPermissionByReferences.mockResolvedValueOnce(); + filesStorageService.getFileRecordsOfParent.mockResolvedValueOnce([fileRecords, fileRecords.length]); filesStorageService.deleteFilesOfParent.mockRejectedValueOnce(error); return { requestParams, userId, error }; diff --git a/apps/server/src/modules/files-storage/uc/files-storage.uc.ts b/apps/server/src/modules/files-storage/uc/files-storage.uc.ts index 41387b6394a..47eb7c79104 100644 --- a/apps/server/src/modules/files-storage/uc/files-storage.uc.ts +++ b/apps/server/src/modules/files-storage/uc/files-storage.uc.ts @@ -1,9 +1,9 @@ +import { AuthorizationContext } from '@modules/authorization'; +import { AuthorizationReferenceService } from '@modules/authorization/domain'; import { HttpService } from '@nestjs/axios'; import { Injectable, NotFoundException } from '@nestjs/common'; import { Counted, EntityId } from '@shared/domain'; import { LegacyLogger } from '@src/core/logger'; -import { AuthorizationContext } from '@modules/authorization'; -import { AuthorizationReferenceService } from '@modules/authorization/domain'; import { AxiosRequestConfig, AxiosResponse } from 'axios'; import busboy from 'busboy'; import { Request } from 'express'; @@ -164,8 +164,9 @@ export class FilesStorageUC { // delete public async deleteFilesOfParent(userId: EntityId, params: FileRecordParams): Promise> { await this.checkPermission(userId, params.parentType, params.parentId, FileStorageAuthorizationContext.delete); - const [fileRecords, count] = await this.filesStorageService.deleteFilesOfParent(params.parentId); + const [fileRecords, count] = await this.filesStorageService.getFileRecordsOfParent(params.parentId); await this.previewService.deletePreviews(fileRecords); + await this.filesStorageService.deleteFilesOfParent(fileRecords); return [fileRecords, count]; } @@ -175,8 +176,8 @@ export class FilesStorageUC { const { parentType, parentId } = fileRecord.getParentInfo(); await this.checkPermission(userId, parentType, parentId, FileStorageAuthorizationContext.delete); - await this.filesStorageService.delete([fileRecord]); await this.previewService.deletePreviews([fileRecord]); + await this.filesStorageService.delete([fileRecord]); return fileRecord; }