diff --git a/app/src/forms/file/service.js b/app/src/forms/file/service.js index 3972f5461..08329ed0f 100644 --- a/app/src/forms/file/service.js +++ b/app/src/forms/file/service.js @@ -2,6 +2,7 @@ const config = require('config'); const uuid = require('uuid'); const path = require('path'); const { FileStorage } = require('../common/models'); +const log = require('../../components/log')(module.filename); const storageService = require('./storage/storageService'); const PERMANENT_STORAGE = config.get('files.permanent'); @@ -144,18 +145,35 @@ const service = { * @param {string} updatedBy the user who is saving the submission. */ moveSubmissionFile: async (submissionId, fileStorage, updatedBy) => { - // Move the file from its current directory to the "submissions" subdirectory. - const path = await storageService.move(fileStorage, 'submissions', submissionId); - if (!path) { - throw new Error('Error moving files for submission'); - } + let trx; + try { + trx = await FileStorage.startTransaction(); + + // Move the file from its current directory to the "submissions" subdirectory. + const path = await storageService.move(fileStorage, 'submissions', submissionId); + if (!path) { + throw new Error('Error moving files for submission'); + } + + await FileStorage.query(trx).patchAndFetchById(fileStorage.id, { + formSubmissionId: submissionId, + path: path, + storage: PERMANENT_STORAGE, + updatedBy: updatedBy, + }); - await FileStorage.query().patchAndFetchById(fileStorage.id, { - formSubmissionId: submissionId, - path: path, - storage: PERMANENT_STORAGE, - updatedBy: updatedBy, - }); + await trx.commit(); + + // Only after successful commit, remove the original object from storage. + try { + await storageService.delete(fileStorage); + } catch (delErr) { + log.error(`Failed to delete original file after move for file ${fileStorage.id}`, delErr); + } + } catch (err) { + if (trx) await trx.rollback(); + throw err; + } }, moveSubmissionFiles: async (submissionId, currentUser) => { diff --git a/app/src/forms/file/storage/objectStorageService.js b/app/src/forms/file/storage/objectStorageService.js index 4f3180a1e..90b9bbbe8 100644 --- a/app/src/forms/file/storage/objectStorageService.js +++ b/app/src/forms/file/storage/objectStorageService.js @@ -168,18 +168,10 @@ class ObjectStorageService { async move(fileStorage, ...subdirs) { try { - const sourcePath = fileStorage.path; const file = await this.copyFile(fileStorage, ...subdirs); if (file) { // this doesn't return the new key/path, but we can build it const newPath = this._join(this._key, ...subdirs, fileStorage.id); - // now delete original... - const params = { - Bucket: this._bucket, - Key: sourcePath, - }; - - await this._s3.send(new DeleteObjectCommand(params)); return newPath; } diff --git a/app/src/forms/submission/updateService.js b/app/src/forms/submission/updateService.js index 8a932f0ca..7bf8f6eb1 100644 --- a/app/src/forms/submission/updateService.js +++ b/app/src/forms/submission/updateService.js @@ -47,9 +47,14 @@ const service = { const fileStorage = await fileService.read(fileId); if (!fileStorage.formSubmissionId) { await FileStorage.query(trx).patchAndFetchById(fileId, { formSubmissionId: id, updatedBy: user.usernameIdp }); - fileService.moveSubmissionFile(id, fileStorage, user.usernameIdp).catch((error) => { - log.error('Error moving file', error); - }); + + // move file and update storage/path atomically to ensure we don't swallow errors + try { + await fileService.moveSubmissionFile(id, fileStorage, user.usernameIdp); + } catch (error) { + log.error('Error moving file', { fileId, error: error?.message || error }); + throw error; + } } } }, diff --git a/app/tests/unit/forms/file/service.spec.js b/app/tests/unit/forms/file/service.spec.js index 8170c8fd2..46fcf8be0 100644 --- a/app/tests/unit/forms/file/service.spec.js +++ b/app/tests/unit/forms/file/service.spec.js @@ -271,6 +271,120 @@ describe('create', () => { }); }); + describe('moveSubmissionFile', () => { + it('should update file path after S3 move succeeds', async () => { + const submissionId = 'test-submission-id'; + const updatedBy = 'test-user'; + const fileStorage = { + id: 'test-file-id', + path: 'chefs/prod/uploads/test-file-id', + formSubmissionId: null, + storage: 'objectStorage', + }; + const newPath = `chefs/prod/submissions/${submissionId}/test-file-id`; + + // Mock storageService.move to return new path + storageService.move = jest.fn().mockResolvedValue(newPath); + + // Mock FileStorage.query().patchAndFetchById to return updated record + FileStorage.query = jest.fn().mockReturnValue({ + patchAndFetchById: jest.fn().mockResolvedValue({ + ...fileStorage, + path: newPath, + formSubmissionId: submissionId, + }), + }); + + // Mock storageService.delete (called after DB commit) + storageService.delete = jest.fn().mockResolvedValue(true); + + await service.moveSubmissionFile(submissionId, fileStorage, updatedBy); + + expect(storageService.move).toHaveBeenCalledWith(fileStorage, 'submissions', submissionId); + expect(FileStorage.query().patchAndFetchById).toHaveBeenCalledWith( + fileStorage.id, + expect.objectContaining({ + path: newPath, + formSubmissionId: submissionId, + }) + ); + expect(storageService.delete).toHaveBeenCalledWith(fileStorage); + }); + + it('should not delete original file if DB update fails', async () => { + const submissionId = 'test-submission-id'; + const updatedBy = 'test-user'; + const fileStorage = { + id: 'test-file-id', + path: 'chefs/prod/uploads/test-file-id', + }; + const newPath = `chefs/prod/submissions/${submissionId}/test-file-id`; + + // Mock storageService.move to return new path (S3 copy succeeds) + storageService.move = jest.fn().mockResolvedValue(newPath); + + // Mock transaction + const mockTrx = { + commit: jest.fn().mockResolvedValue(), + rollback: jest.fn().mockResolvedValue(), + }; + + FileStorage.startTransaction = jest.fn().mockResolvedValue(mockTrx); + + // Mock DB update to fail + FileStorage.query = jest.fn().mockReturnValue({ + patchAndFetchById: jest.fn().mockRejectedValue(new Error('DB update failed')), + }); + + storageService.delete = jest.fn().mockResolvedValue(true); + + await expect(service.moveSubmissionFile(submissionId, fileStorage, updatedBy)).rejects.toThrow('DB update failed'); + + // delete should NOT have been called + expect(storageService.delete).not.toHaveBeenCalled(); + // rollback should have been called + expect(mockTrx.rollback).toHaveBeenCalled(); + }); + + it('should log error but not fail if S3 delete fails after DB commit', async () => { + const submissionId = 'test-submission-id'; + const updatedBy = 'test-user'; + const fileStorage = { + id: 'test-file-id', + path: 'chefs/prod/uploads/test-file-id', + }; + const newPath = `chefs/prod/submissions/${submissionId}/test-file-id`; + + storageService.move = jest.fn().mockResolvedValue(newPath); + + const mockTrx = { + commit: jest.fn().mockResolvedValue(), + rollback: jest.fn().mockResolvedValue(), + }; + + FileStorage.startTransaction = jest.fn().mockResolvedValue(mockTrx); + + FileStorage.query = jest.fn().mockReturnValue({ + patchAndFetchById: jest.fn().mockResolvedValue({ + ...fileStorage, + path: newPath, + formSubmissionId: submissionId, + }), + }); + + // Mock delete to fail after DB commit + storageService.delete = jest.fn().mockRejectedValue(new Error('S3 delete failed')); + + // Should NOT throw even though delete failed + await expect(service.moveSubmissionFile(submissionId, fileStorage, updatedBy)).resolves.not.toThrow(); + + // Commit should have succeeded + expect(mockTrx.commit).toHaveBeenCalled(); + // Delete was attempted + expect(storageService.delete).toHaveBeenCalled(); + }); + }); + describe('clone coverage', () => { it('clone: should create a new file record with new ID', async () => { FileStorage.startTransaction = jest.fn().mockResolvedValue(mockTransaction); diff --git a/app/tests/unit/forms/submission/updateService.spec.js b/app/tests/unit/forms/submission/updateService.spec.js index 70c6784fe..72acbfdea 100644 --- a/app/tests/unit/forms/submission/updateService.spec.js +++ b/app/tests/unit/forms/submission/updateService.spec.js @@ -239,6 +239,31 @@ describe('updateService', () => { expect(patchAndFetchById).not.toHaveBeenCalled(); expect(fileService.moveSubmissionFile).not.toHaveBeenCalled(); }); + + it('should throw error from moveSubmissionFile instead of silently catching', async () => { + const fileIds = ['f1']; + const data = {}; + const id = 557; + const user = { usernameIdp: 'uploader' }; + const trx = {}; + + const submissionService = { + _findFileIds: jest.fn().mockReturnValue(fileIds), + }; + + fileService.read.mockResolvedValue({ id: 'f1', formSubmissionId: null }); + + const patchAndFetchById = jest.fn().mockResolvedValue({}); + FileStorage.query.mockReturnValue({ patchAndFetchById }); + + // Mock moveSubmissionFile to throw + fileService.moveSubmissionFile.mockRejectedValue(new Error('File move failed')); + + // Should throw, not silently catch + await expect(updateService._handleFileUploads(submissionService, data, id, user, trx)).rejects.toThrow('File move failed'); + + expect(fileService.moveSubmissionFile).toHaveBeenCalledWith(id, { id: 'f1', formSubmissionId: null }, 'uploader'); + }); }); describe('update', () => {