From f03a010c4c47058e577d70997edae6e2253f3454 Mon Sep 17 00:00:00 2001 From: Joao Date: Wed, 15 Jan 2025 14:01:57 -0400 Subject: [PATCH 1/3] Add error handling for non-existent files and implement thumbnail deletion --- app/api/files/PDF.ts | 4 +++ app/api/files/files.ts | 9 +++++++ app/api/files/processDocument.ts | 10 ++++++-- app/api/files/specs/files.spec.ts | 27 +++++++++++++++++++++ app/api/files/specs/processDocument.spec.ts | 17 ++++++++++++- 5 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 app/api/files/specs/files.spec.ts diff --git a/app/api/files/PDF.ts b/app/api/files/PDF.ts index d1620fdcfa..6dbb0c6a36 100644 --- a/app/api/files/PDF.ts +++ b/app/api/files/PDF.ts @@ -80,6 +80,10 @@ class PDF extends EventEmitter { return Promise.resolve(response); } + async deleteThumbnail(filename: string) { + await storage.removeFile(filename, 'thumbnail'); + } + async convert() { return this.extractText().then(conversion => ({ ...conversion, diff --git a/app/api/files/files.ts b/app/api/files/files.ts index 1c91d0c9ce..dad29a898d 100644 --- a/app/api/files/files.ts +++ b/app/api/files/files.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-statements */ import entities from 'api/entities'; import { applicationEventsBus } from 'api/eventsbus'; import { mimeTypeFromUrl } from 'api/files/extensionHelper'; @@ -25,11 +26,19 @@ const deduceMimeType = (_file: FileType) => { return file; }; +export class UpdateFileError extends Error { + constructor() { + super('Can not update a File that does not exist'); + } +} + export const files = { async save(_file: FileType, index = true) { const file = deduceMimeType(_file); const existingFile = file._id ? await filesModel.getById(file._id) : undefined; + if (file._id && !existingFile) throw new UpdateFileError(); + const savedFile = await filesModel.save(await validateFile(file)); if (index) { await search.indexEntities({ sharedId: savedFile.entity }, '+fullText'); diff --git a/app/api/files/processDocument.ts b/app/api/files/processDocument.ts index f09a9a15cd..6cf7da2706 100644 --- a/app/api/files/processDocument.ts +++ b/app/api/files/processDocument.ts @@ -2,7 +2,7 @@ import { convertToPDFService } from 'api/services/convertToPDF/convertToPdfServi import settings from 'api/settings'; import { FileType } from 'shared/types/fileType'; -import { files } from './files'; +import { files, UpdateFileError } from './files'; import { PDF } from './PDF'; export const processPDF = async ( @@ -10,6 +10,7 @@ export const processPDF = async ( file: FileType & { destination?: string }, detectLanguage = true ) => { + let thumbnail; const pdf = new PDF(file); const upload = await files.save({ ...file, @@ -24,7 +25,7 @@ export const processPDF = async ( conversion.language = file.language; } - const thumbnail = await pdf.createThumbnail(upload._id.toString()); + thumbnail = await pdf.createThumbnail(upload._id.toString()); await files.save({ entity: entitySharedId, @@ -42,10 +43,15 @@ export const processPDF = async ( return saved; } catch (e) { + if (e.constructor === UpdateFileError) { + await pdf.deleteThumbnail(thumbnail); + } + await files.save({ ...upload, status: 'failed', }); + throw e; } }; diff --git a/app/api/files/specs/files.spec.ts b/app/api/files/specs/files.spec.ts new file mode 100644 index 0000000000..6235a79fc5 --- /dev/null +++ b/app/api/files/specs/files.spec.ts @@ -0,0 +1,27 @@ +import testingDB from 'api/utils/testing_db'; +import { testingEnvironment } from 'api/utils/testingEnvironment'; +import { files, UpdateFileError } from '../files'; + +describe('Files', () => { + beforeEach(async () => { + await testingEnvironment.setUp({}); + }); + + afterAll(async () => testingEnvironment.tearDown()); + + it('should not update a File if no longer exist', async () => { + const promise = files.save({ + _id: testingDB.id(), + filename: 'any_file_name', + originalname: 'any_original_name', + entity: '123', + language: 'en', + }); + + await expect(promise).rejects.toEqual(new UpdateFileError()); + + const [result] = await files.get({}); + + expect(result).toBeUndefined(); + }); +}); diff --git a/app/api/files/specs/processDocument.spec.ts b/app/api/files/specs/processDocument.spec.ts index b6576db1cb..f71e2f3af3 100644 --- a/app/api/files/specs/processDocument.spec.ts +++ b/app/api/files/specs/processDocument.spec.ts @@ -1,3 +1,4 @@ +import testingDB from 'api/utils/testing_db'; import { convertToPDFService, MimeTypeNotSupportedForConversion, @@ -5,7 +6,7 @@ import { import { testingEnvironment } from 'api/utils/testingEnvironment'; // eslint-disable-next-line node/no-restricted-import import { writeFile } from 'fs/promises'; -import { files } from '../files'; +import { files, UpdateFileError } from '../files'; import { attachmentsPath, setupTestUploadedPaths } from '../filesystem'; import { processDocument } from '../processDocument'; @@ -76,4 +77,18 @@ describe('processDocument', () => { expect(file).toBeUndefined(); }); }); + + it('should not persist file or thumbnail if there is an UpdateFileError', async () => { + const _id = testingDB.id(); + const promise = processDocument('any_entity_shared_id', { + _id, + filename: 'any_file_name', + originalname: 'any_original_name', + }); + + await expect(promise).rejects.toEqual(new UpdateFileError()); + const [file] = await files.get({ _id }); + + expect(file).toBeUndefined(); + }); }); From 8dc35e4aaf603d3c0820e4a15350500cb4f9bc81 Mon Sep 17 00:00:00 2001 From: Joao Date: Thu, 16 Jan 2025 07:34:37 -0400 Subject: [PATCH 2/3] Add tests to ensure thumbnails are undefined when file update fails --- app/api/files/specs/processDocument.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/api/files/specs/processDocument.spec.ts b/app/api/files/specs/processDocument.spec.ts index f71e2f3af3..9397ad2655 100644 --- a/app/api/files/specs/processDocument.spec.ts +++ b/app/api/files/specs/processDocument.spec.ts @@ -88,7 +88,9 @@ describe('processDocument', () => { await expect(promise).rejects.toEqual(new UpdateFileError()); const [file] = await files.get({ _id }); + const [thumbnail] = await files.get({ entity: _id, type: 'thumbnail' }); expect(file).toBeUndefined(); + expect(thumbnail).toBeUndefined(); }); }); From e33c1c6c40e559ead8a5f78d54fafea14ad4d6eb Mon Sep 17 00:00:00 2001 From: Joao Date: Fri, 17 Jan 2025 08:56:06 -0400 Subject: [PATCH 3/3] Refactor PDF processing to remove thumbnail deletion and streamline file saving --- app/api/files/PDF.ts | 4 ---- app/api/files/processDocument.ts | 17 +++++++---------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/app/api/files/PDF.ts b/app/api/files/PDF.ts index 6dbb0c6a36..d1620fdcfa 100644 --- a/app/api/files/PDF.ts +++ b/app/api/files/PDF.ts @@ -80,10 +80,6 @@ class PDF extends EventEmitter { return Promise.resolve(response); } - async deleteThumbnail(filename: string) { - await storage.removeFile(filename, 'thumbnail'); - } - async convert() { return this.extractText().then(conversion => ({ ...conversion, diff --git a/app/api/files/processDocument.ts b/app/api/files/processDocument.ts index 6cf7da2706..8aaba3f2b8 100644 --- a/app/api/files/processDocument.ts +++ b/app/api/files/processDocument.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-statements */ import { convertToPDFService } from 'api/services/convertToPDF/convertToPdfService'; import settings from 'api/settings'; import { FileType } from 'shared/types/fileType'; @@ -25,6 +26,12 @@ export const processPDF = async ( conversion.language = file.language; } + const saved = await files.save({ + ...upload, + ...conversion, + status: 'ready', + }); + thumbnail = await pdf.createThumbnail(upload._id.toString()); await files.save({ @@ -35,18 +42,8 @@ export const processPDF = async ( mimetype: 'image/jpeg', }); - const saved = await files.save({ - ...upload, - ...conversion, - status: 'ready', - }); - return saved; } catch (e) { - if (e.constructor === UpdateFileError) { - await pdf.deleteThumbnail(thumbnail); - } - await files.save({ ...upload, status: 'failed',