fix: use applicationUniversalIdentifier in deleteByFileId S3 path construction
https://sonarly.com/issue/33714?type=bug Files uploaded to S3 via the new Files v2 API are never actually deleted from S3 storage because `deleteByFileId` constructs the S3 key using `applicationId` (UUID) instead of `applicationUniversalIdentifier` (string identifier like "twenty-standard"), creating a path mismatch with how files were written. Fix: Fixed `deleteByFileId` in `FileStorageService` to construct the correct S3 storage path. **The bug:** Files are written to S3 at path `{workspaceId}/{applicationUniversalIdentifier}/{fileFolder}/{resourcePath}` (via `buildOnStoragePath`), but `deleteByFileId` was constructing the delete path as `{workspaceId}/{applicationId}/{file.path}` — using the application's UUID (`applicationId`) instead of its string identifier (`applicationUniversalIdentifier`). Since S3 `DeleteObject` silently succeeds on non-existent keys, files were never actually removed. **The fix:** Added an `applicationRepository.findOneOrFail()` lookup to resolve the application's `universalIdentifier` from `file.applicationId`, then reused the existing `buildOnStoragePath` method to construct the correct S3 key — the same pattern used by every other method in this service (`writeFile`, `readFile`, `delete`, etc.). The driver's `delete` call now uses `dirname`/`basename` of the full storage path, matching the convention used in the `copy` method. **Test:** Added a unit test to `file-storage.service.spec.ts` that verifies `deleteByFileId` looks up the application by ID and constructs the S3 path using `applicationUniversalIdentifier`, not `applicationId`.
This commit is contained in:
+47
@@ -16,6 +16,8 @@ describe('FileStorageService', () => {
|
||||
|
||||
const mockFileRepository = {
|
||||
save: jest.fn(),
|
||||
findOneOrFail: jest.fn(),
|
||||
delete: jest.fn(),
|
||||
};
|
||||
|
||||
const mockApplicationRepository = {
|
||||
@@ -193,6 +195,51 @@ describe('FileStorageService', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('deleteByFileId', () => {
|
||||
it('should build the correct storage path using applicationUniversalIdentifier', async () => {
|
||||
const fileId = 'file-uuid';
|
||||
const workspaceId = 'ws-uuid';
|
||||
const applicationId = 'app-uuid';
|
||||
const applicationUniversalIdentifier = 'twenty-standard';
|
||||
|
||||
mockFileRepository.findOneOrFail.mockResolvedValue({
|
||||
id: fileId,
|
||||
workspaceId,
|
||||
applicationId,
|
||||
path: 'files-field/field-uid/file-uuid.pdf',
|
||||
});
|
||||
|
||||
mockApplicationRepository.findOneOrFail.mockResolvedValue({
|
||||
id: applicationId,
|
||||
universalIdentifier: applicationUniversalIdentifier,
|
||||
workspaceId,
|
||||
});
|
||||
|
||||
mockDriver.delete.mockResolvedValue(undefined);
|
||||
mockFileRepository.delete.mockResolvedValue(undefined);
|
||||
|
||||
await service.deleteByFileId({
|
||||
fileId,
|
||||
workspaceId,
|
||||
fileFolder: 'files-field' as any,
|
||||
});
|
||||
|
||||
expect(mockApplicationRepository.findOneOrFail).toHaveBeenCalledWith({
|
||||
where: {
|
||||
id: applicationId,
|
||||
workspaceId,
|
||||
},
|
||||
});
|
||||
|
||||
expect(mockDriver.delete).toHaveBeenCalledWith({
|
||||
folderPath: `${workspaceId}/${applicationUniversalIdentifier}/files-field/field-uid`,
|
||||
filename: 'file-uuid.pdf',
|
||||
});
|
||||
|
||||
expect(mockFileRepository.delete).toHaveBeenCalledWith(fileId);
|
||||
});
|
||||
});
|
||||
|
||||
describe('checkFolderExistsLegacy', () => {
|
||||
it('should delegate to the current driver and return true', async () => {
|
||||
const checkParams = {
|
||||
|
||||
+19
-2
@@ -224,11 +224,28 @@ export class FileStorageService {
|
||||
path: Like(`${fileFolder}/%`),
|
||||
},
|
||||
});
|
||||
|
||||
const application = await this.applicationRepository.findOneOrFail({
|
||||
where: {
|
||||
id: file.applicationId,
|
||||
workspaceId,
|
||||
},
|
||||
});
|
||||
|
||||
const resourcePath = file.path.replace(`${fileFolder}/`, '');
|
||||
|
||||
const onStoragePath = this.buildOnStoragePath({
|
||||
workspaceId,
|
||||
applicationUniversalIdentifier: application.universalIdentifier,
|
||||
fileFolder,
|
||||
resourcePath,
|
||||
});
|
||||
|
||||
const driver = this.fileStorageDriverFactory.getCurrentDriver();
|
||||
|
||||
await driver.delete({
|
||||
folderPath: `${file.workspaceId}/${file.applicationId}`,
|
||||
filename: file.path,
|
||||
folderPath: dirname(onStoragePath),
|
||||
filename: basename(onStoragePath),
|
||||
});
|
||||
|
||||
await this.fileRepository.delete(fileId);
|
||||
|
||||
Reference in New Issue
Block a user