From f8faef77c447f4c25f6d66b9489a5a9aed428b63 Mon Sep 17 00:00:00 2001 From: jubnl Date: Mon, 17 Aug 2026 06:01:28 +0200 Subject: [PATCH] refactor(files): trash deletion through storage --- server/src/nest/files/files.service.ts | 24 ++++++++++------- server/tests/helpers/mcp-test-controllers.ts | 2 +- server/tests/helpers/plugin-host.ts | 2 +- server/tests/unit/nest/files.service.test.ts | 26 +++++++++---------- .../unit/nest/trip-read-model.service.test.ts | 2 +- server/tests/unit/nest/trips.service.test.ts | 2 +- 6 files changed, 31 insertions(+), 27 deletions(-) diff --git a/server/src/nest/files/files.service.ts b/server/src/nest/files/files.service.ts index 2a02649bc..2cc579022 100644 --- a/server/src/nest/files/files.service.ts +++ b/server/src/nest/files/files.service.ts @@ -1,6 +1,5 @@ import { Injectable } from '@nestjs/common'; import path from 'path'; -import fs from 'fs'; import type { Request } from 'express'; import type { TrekWsPayload, TrekWsTripEventName } from '@trek/shared'; import { RealtimeService } from '../realtime/realtime.service'; @@ -11,6 +10,7 @@ import { verifyJwtAndLoadUser } from '../auth/jwt-verify'; import type { User, TripFile } from '../../types'; import { DatabaseService, type TripAccess } from '../database/database.service'; import { DEFAULT_ALLOWED_EXTENSIONS, filesDir } from './files.constants'; +import { StorageService } from '../storage/storage.service'; type Trip = TripAccess; type FilePermission = 'file_upload' | 'file_edit' | 'file_delete'; @@ -60,6 +60,7 @@ export class FilesService { private readonly permissions: PermissionsService, private readonly realtime: RealtimeService, private readonly tokens: EphemeralTokenService, + private readonly storage: StorageService, ) {} verifyTripAccess(tripId: string | number, userId: number) { @@ -105,6 +106,12 @@ export class FilesService { return { error: 'Authentication required', status: 401 }; } + /** + * Absolute-path resolution for the one remaining raw-fs consumer, the plugin + * RPC read (files.rpc.ts). Deferred beyond slice 4 by user decision + * 2026-08-16; everything else in this domain addresses storage as + * ('files', ). + */ resolveFilePath(filename: string): { resolved: string; safe: boolean } { const safeName = path.basename(filename); const filePath = path.join(filesDir, safeName); @@ -244,14 +251,12 @@ export class FilesService { } async permanentDeleteFile(file: TripFile): Promise { - const { resolved } = this.resolveFilePath(file.filename); - // `force: true` swallows ENOENT, replacing the prior existsSync+unlink - // double-call that blocked the event loop twice per deletion. Only - // drop the DB row when the on-disk unlink either succeeded or the - // file was already gone — otherwise a permission / ENOSPC failure - // would orphan the bytes on disk with no DB pointer left to clean it. + // storage.delete is idempotent on a missing object (the old rm force:true + // contract). Only drop the DB row when the delete either succeeded or the + // object was already gone — otherwise a permission / ENOSPC failure + // would orphan the bytes with no DB pointer left to clean them. try { - await fs.promises.rm(resolved, { force: true }); + await this.storage.delete('files', path.basename(file.filename)); } catch (e) { console.error(`[files] unlink failed for ${file.filename}, keeping DB row:`, e); throw e; @@ -266,9 +271,8 @@ export class FilesService { // and a retry via the single-file delete path can try again. const successfullyUnlinked: number[] = []; await Promise.all(trashed.map(async (file) => { - const { resolved } = this.resolveFilePath(file.filename); try { - await fs.promises.rm(resolved, { force: true }); + await this.storage.delete('files', path.basename(file.filename)); successfullyUnlinked.push(Number(file.id)); } catch (e) { console.error(`[files] unlink failed for ${file.filename}, keeping DB row:`, e); diff --git a/server/tests/helpers/mcp-test-controllers.ts b/server/tests/helpers/mcp-test-controllers.ts index 198837143..a0bad3369 100644 --- a/server/tests/helpers/mcp-test-controllers.ts +++ b/server/tests/helpers/mcp-test-controllers.ts @@ -145,7 +145,7 @@ export function createMcpTestRegistry(): McpRegistry { const readModelService = new TripReadModelService( dbService, membersService, daysService, accommodationsService, budgetService, packingService, reservationsService, collabService, placesService, todoService, - new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService()), + new FilesService(dbService, permissionsService, realtimeService, new EphemeralTokenService(), generalStorage), ); const calendarService = new CalendarService(dbService, reservationsService); // The nine addon-gated surfaces read their toggle off an injected service now diff --git a/server/tests/helpers/plugin-host.ts b/server/tests/helpers/plugin-host.ts index e0bf6340d..90b437cf3 100644 --- a/server/tests/helpers/plugin-host.ts +++ b/server/tests/helpers/plugin-host.ts @@ -92,7 +92,7 @@ export function createPluginRpcHostFactory(dbs: DatabaseService): PluginRpcHostF const queryHelpers = new QueryHelpersService(dbs); const todos = new TodoService(dbs, permissions, realtime); const packing = new PackingService(dbs, permissions, realtime, notificationsStub()); - const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService()); + const files = new FilesService(dbs, permissions, realtime, new EphemeralTokenService(), generalStorage); const reservations = new ReservationsService(dbs, permissions, budget, realtime, notificationsStub(), new ReservationsReadRepository(dbs)); const collab = new CollabService(dbs, permissions, realtime, notificationsStub(), generalStorage); const vacay = new VacayService(dbs, realtime, notificationsStub()); diff --git a/server/tests/unit/nest/files.service.test.ts b/server/tests/unit/nest/files.service.test.ts index f636c2005..4477acdf2 100644 --- a/server/tests/unit/nest/files.service.test.ts +++ b/server/tests/unit/nest/files.service.test.ts @@ -8,7 +8,6 @@ */ import { describe, it, expect, vi, beforeAll, beforeEach, afterAll, afterEach } from 'vitest'; import path from 'path'; -import fs from 'fs'; // ── DB setup ────────────────────────────────────────────────────────────────── @@ -76,7 +75,9 @@ import type { TripFile, User } from '../../../src/types'; import { RealtimeService } from '../../../src/nest/realtime/realtime.service'; import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service'; -const svc = new FilesService(new DatabaseService(testDb), permissionsStub, new RealtimeService(), new EphemeralTokenService()); +const storageDelete = vi.fn(); +const storageStub = { delete: storageDelete } as unknown as import('../../../src/nest/storage/storage.service').StorageService; +const svc = new FilesService(new DatabaseService(testDb), permissionsStub, new RealtimeService(), new EphemeralTokenService(), storageStub); beforeAll(() => { createTables(testDb); @@ -347,20 +348,20 @@ describe('toggleStarred / softDeleteFile / restoreFile', () => { // ── permanentDeleteFile / emptyTrash (unlink-first semantics) ───────────────── describe('permanentDeleteFile', () => { - it('FILE-SVC-022: removes the resolved path with force:true, then the DB row', async () => { + it('FILE-SVC-022: deletes the storage object (idempotent on missing), then the DB row', async () => { const { user, trip } = seedTrip(); const file = makeFile(trip.id, user.id, { filename: 'on-disk.pdf' }); - const rm = vi.spyOn(fs.promises, 'rm').mockResolvedValue(undefined); + storageDelete.mockResolvedValue(undefined); await svc.permanentDeleteFile(svc.getFileById(file.id, trip.id) as TripFile); - expect(rm).toHaveBeenCalledWith(svc.resolveFilePath('on-disk.pdf').resolved, { force: true }); + expect(storageDelete).toHaveBeenCalledWith('files', 'on-disk.pdf'); expect(svc.getFileById(file.id, trip.id)).toBeUndefined(); }); - it('FILE-SVC-023: an unlink failure logs [files], rethrows and keeps the DB row', async () => { + it('FILE-SVC-023: a delete failure logs [files], rethrows and keeps the DB row', async () => { const { user, trip } = seedTrip(); const file = makeFile(trip.id, user.id, { filename: 'stuck.pdf' }); const boom = new Error('EACCES'); - vi.spyOn(fs.promises, 'rm').mockRejectedValue(boom); + storageDelete.mockRejectedValue(boom); const err = vi.spyOn(console, 'error').mockImplementation(() => {}); await expect(svc.permanentDeleteFile(svc.getFileById(file.id, trip.id) as TripFile)).rejects.toThrow('EACCES'); expect(err).toHaveBeenCalledWith('[files] unlink failed for stuck.pdf, keeping DB row:', boom); @@ -376,7 +377,7 @@ describe('emptyTrash', () => { const kept = makeFile(trip.id, user.id, { filename: 'kept.pdf' }); svc.softDeleteFile(a.id); svc.softDeleteFile(b.id); - vi.spyOn(fs.promises, 'rm').mockResolvedValue(undefined); + storageDelete.mockResolvedValue(undefined); await expect(svc.emptyTrash(trip.id)).resolves.toBe(2); expect(svc.listFiles(trip.id, true)).toEqual([]); @@ -390,8 +391,8 @@ describe('emptyTrash', () => { svc.softDeleteFile(good.id); svc.softDeleteFile(bad.id); const boom = new Error('EBUSY'); - vi.spyOn(fs.promises, 'rm').mockImplementation((p) => - String(p).includes('bad.pdf') ? Promise.reject(boom) : Promise.resolve() + storageDelete.mockImplementation((_category: string, name: string) => + name.includes('bad.pdf') ? Promise.reject(boom) : Promise.resolve() ); const err = vi.spyOn(console, 'error').mockImplementation(() => {}); @@ -401,11 +402,10 @@ describe('emptyTrash', () => { expect(svc.getDeletedFile(bad.id, trip.id)).toBeDefined(); }); - it('FILE-SVC-026: an empty trash resolves to 0 without touching the disk', async () => { + it('FILE-SVC-026: an empty trash resolves to 0 without touching storage', async () => { const { trip } = seedTrip(); - const rm = vi.spyOn(fs.promises, 'rm'); await expect(svc.emptyTrash(trip.id)).resolves.toBe(0); - expect(rm).not.toHaveBeenCalled(); + expect(storageDelete).not.toHaveBeenCalled(); }); }); diff --git a/server/tests/unit/nest/trip-read-model.service.test.ts b/server/tests/unit/nest/trip-read-model.service.test.ts index e0616faea..262652900 100644 --- a/server/tests/unit/nest/trip-read-model.service.test.ts +++ b/server/tests/unit/nest/trip-read-model.service.test.ts @@ -103,7 +103,7 @@ const buildReadModel = (database: DatabaseService, roster: TripMembersService = new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub(), makeStorageFixture('').storage), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new EphemeralTokenService()), + new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new EphemeralTokenService(), makeStorageFixture('').storage), ); const svc = buildReadModel(dbs()); diff --git a/server/tests/unit/nest/trips.service.test.ts b/server/tests/unit/nest/trips.service.test.ts index d13222754..61a5a8bb6 100644 --- a/server/tests/unit/nest/trips.service.test.ts +++ b/server/tests/unit/nest/trips.service.test.ts @@ -133,7 +133,7 @@ const readModelSvc = new TripReadModelService( new CollabService(dbs(), new PermissionsService(dbs()), new RealtimeService(), notificationsStub(), coversFx.storage), placesSvc, new TodoService(dbs(), new PermissionsService(dbs()), new RealtimeService()), - new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new EphemeralTokenService()), + new FilesService(dbs(), new PermissionsService(dbs()), new RealtimeService(), new EphemeralTokenService(), coversFx.storage), );