Compare commits

...
Author SHA1 Message Date
Sonarly Claude Code fbf8e5bef9 chore: improve monitoring for fix: use applicationUniversalIdentifier in deleteB
Changed the error logging in `FilesFieldDeletionJob` from `logger.log` (info level) to `logger.error` with full stack trace.

Previously, when file deletion failed in the background job, the error was silently swallowed with an info-level log message (`this.logger.log('Failed to delete file ...')`), making it invisible in production monitoring. The caught error object was also discarded entirely, losing the stack trace and root cause details.

Now the job logs at error level with the workspace ID for context and includes the full stack trace, making file deletion failures detectable by log-based alerting (Sentry, Datadog, etc.).
2026-05-03 11:40:40 +00:00
Sonarly Claude Code 6827460cf4 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`.
2026-05-03 11:40:40 +00:00
3 changed files with 71 additions and 4 deletions
@@ -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 = {
@@ -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);
@@ -32,8 +32,11 @@ export class FilesFieldDeletionJob {
fileId,
workspaceId,
});
} catch {
this.logger.log(`Failed to delete file ${fileId}`);
} catch (error) {
this.logger.error(
`Failed to delete file ${fileId} in workspace ${workspaceId}`,
error instanceof Error ? error.stack : String(error),
);
}
}
}