refactor(files): trip files and file links through repositories, with transactions added

This commit is contained in:
jubnl
2026-09-23 18:22:02 +02:00
parent db2db7f5a3
commit 399ca58a7d
14 changed files with 882 additions and 142 deletions
@@ -1,4 +1,28 @@
import type { BudgetItems } from '../entities/BudgetItems.entity';
import { columnRef } from '../dialect/sql-functions';
import { TrekRepository } from './_shared/trek-repository';
export class BudgetItemsRepository extends TrekRepository<BudgetItems> {}
// ---------------------------------------------------------------------------
// Plan 3e Task 1 (`FilesService.findForeignLinkTarget`, R12) — additive,
// append-only per that task's own file-ownership rule (Task 2/budget owns
// every other method appended to this file).
// ---------------------------------------------------------------------------
export class BudgetItemsRepository extends TrekRepository<BudgetItems> {
/**
* FL4 (`FilesService.findForeignLinkTarget`'s budget-expense branch) —
* `SELECT 1 FROM budget_items WHERE id = ? AND trip_id = ?`, re-expressed
* as "what trip does this row belong to" (R12, the same shape as
* `ReservationsRepository.findTripId`'s docstring). `trip_id` is a
* `persist(false)` mirror of the `trip` relation — `columnRef`, not a bare
* select (the program-wide trap).
*/
async findTripId(id: number): Promise<number | undefined> {
const platform = this.getEntityManager().getPlatform();
const row = await this.qb('b')
.select([columnRef(platform, 'b.trip_id').as('trip_id')])
.where({ id })
.execute<{ trip_id: number } | undefined>('get', false);
return row?.trip_id;
}
}
@@ -933,4 +933,30 @@ export class DayAssignmentsRepository extends TrekRepository<DayAssignments> {
async clearStay(id: number): Promise<void> {
await this.nativeUpdate({ id }, { accommodation_id: null });
}
// ---------------------------------------------------------------------------
// Plan 3e Task 1 (`FilesService.findForeignLinkTarget`, R12) — additive,
// append-only per that task's own file-ownership rule.
// ---------------------------------------------------------------------------
/**
* FL3 (`FilesService.findForeignLinkTarget`'s assignment branch) — `SELECT
* 1 FROM day_assignments a JOIN days d ON a.day_id = d.id WHERE a.id = ?
* AND d.trip_id = ?`, re-expressed as "what trip does this row belong to"
* via its `day` relation (R12 — "DayAssignmentsRepository.findTripId via
* its days join"; `day_assignments` has no `trip_id` column of its own).
* The same join shape `ReservationsRepository.getAssignmentTripId`
* (RS22) already used from a DIFFERENT repository before this file was
* this task's own to append to — this copy lives here instead, per R12's
* "one method per target table, on that table's own repository" ruling.
*/
async findTripId(id: number): Promise<number | undefined> {
const platform = this.getEntityManager().getPlatform();
const row = await this.qb('da')
.join('da.day', 'd')
.select([columnRef(platform, 'd.trip_id').as('trip_id')])
.where({ id })
.execute<{ trip_id: number } | undefined>('get', false);
return row?.trip_id;
}
}
@@ -1,4 +1,142 @@
import type { FileLinks } from '../entities/FileLinks.entity';
import { type AssertRowKeys } from './_shared/rows';
import { TrekRepository } from './_shared/trek-repository';
export class FileLinksRepository extends TrekRepository<FileLinks> {}
/** A bare `file_links` row — every scalar column of the entity, incl. its five `persist(false)` relation mirrors. */
export interface FileLinkRow {
id: number;
file_id: number;
reservation_id: number | null;
assignment_id: number | null;
place_id: number | null;
created_at: string | null;
budget_item_id: number | null;
}
const _fileLinkRowKeys: AssertRowKeys<FileLinkRow, FileLinks> = true;
/** FL27's joined projection (`getFileLinks`). */
export interface FileLinkWithReservationRow extends FileLinkRow {
reservation_title: string | null;
}
/** FL8's narrow batch projection (`listFiles`'s link lookup). */
export interface FileLinkTargetRow {
file_id: number;
reservation_id: number | null;
place_id: number | null;
budget_item_id: number | null;
}
interface FileLinksKyselyDB {
file_links: FileLinkRow;
reservations: { id: number; title: string };
}
/**
* The insert-only shape — `id`/`created_at` are autoincrement/`DEFAULT
* CURRENT_TIMESTAMP` and omitted from `.values()` below (the legacy
* statement's own column list omits them too, matching this exactly), so a
* SEPARATE interface from {@link FileLinksKyselyDB} rather than making those
* two columns optional there — that would also loosen every SELECT method's
* inferred `id`/`created_at` to `T | undefined`, which is wrong for a row
* that unquestionably has both once read back (the `ReservationJoinKyselyDB`
* / `ReservationRestampKyselyDB` precedent, `Reservations.repository.ts`:
* one purpose-shaped Kysely interface per statement, not one reused
* everywhere).
*/
interface FileLinksWriteKyselyDB {
file_links: {
file_id: number;
reservation_id: number | null;
assignment_id: number | null;
place_id: number | null;
budget_item_id: number | null;
};
}
/**
* `file_links` — the many-to-many between a `trip_files` row and the
* reservation/assignment/place/budget-expense it points at. Kysely
* throughout, same reasoning as `TripFilesRepository`'s class docstring
* (five `persist(false)` mirrors on this entity too), and because the
* `INSERT OR IGNORE` write below has no `em.upsert`-expressible single
* conflict target (four independent partial-unique indexes, only one of
* which any given insert touches) — Kysely's target-less `ON CONFLICT DO
* NOTHING` is the exact SQLite equivalent of `OR IGNORE`, ignoring
* whichever constraint the row happens to violate, matching the legacy
* statement precisely (`Tags.repository.ts#insertIgnore`'s explicit-target
* precedent is the WRONG shape here — that pivot has exactly one unique key).
*/
export class FileLinksRepository extends TrekRepository<FileLinks> {
/**
* FL8 (`FilesService.listFiles`'s link batch) — `SELECT file_id,
* reservation_id, place_id, budget_item_id FROM file_links WHERE file_id
* IN (dynamic)`. Empty `file_ids` short-circuits before any query,
* matching the legacy service's own guard.
*/
async listForFiles(file_ids: number[]): Promise<FileLinkTargetRow[]> {
if (file_ids.length === 0) return [];
return await this.kysely<FileLinksKyselyDB>()
.selectFrom('file_links')
.select(['file_id', 'reservation_id', 'place_id', 'budget_item_id'])
.where('file_id', 'in', file_ids)
.execute();
}
/**
* FL10/FL13/FL24 (`createFile`'s conditional insert, `updateFile`'s
* conditional insert, `createFileLink`) — `INSERT OR IGNORE INTO
* file_links (file_id, reservation_id, assignment_id, place_id,
* budget_item_id) VALUES (?×5)`, the same statement shape at all three
* legacy sites (FL10/FL13 pass only `budget_item_id`; FL24 passes any
* combination the caller supplies) — one method, not three.
*/
async insertIgnore(row: { file_id: number; reservation_id?: number | null; assignment_id?: number | null; place_id?: number | null; budget_item_id?: number | null }): Promise<void> {
await this.kysely<FileLinksWriteKyselyDB>()
.insertInto('file_links')
.values({
file_id: row.file_id,
reservation_id: row.reservation_id ?? null,
assignment_id: row.assignment_id ?? null,
place_id: row.place_id ?? null,
budget_item_id: row.budget_item_id ?? null,
})
.onConflict((oc) => oc.doNothing())
.execute();
}
/** FL14 (`FilesService.updateFile`'s conditional clear) — `DELETE FROM file_links WHERE file_id = ? AND budget_item_id IS NOT NULL`. */
async clearBudgetLink(file_id: number): Promise<void> {
await this.kysely<FileLinksKyselyDB>()
.deleteFrom('file_links')
.where('file_id', '=', file_id)
.where('budget_item_id', 'is not', null)
.execute();
}
/** FL26 (`FilesService.deleteFileLink`) — `DELETE FROM file_links WHERE id = ? AND file_id = ?`. */
async deleteById(id: number, file_id: number): Promise<void> {
await this.kysely<FileLinksKyselyDB>()
.deleteFrom('file_links')
.where('id', '=', id)
.where('file_id', '=', file_id)
.execute();
}
/** FL25 (`createFileLink`'s bare re-select) — `SELECT * FROM file_links WHERE file_id = ?`. */
async listForFile(file_id: number): Promise<FileLinkRow[]> {
return await this.kysely<FileLinksKyselyDB>().selectFrom('file_links').selectAll().where('file_id', '=', file_id).execute();
}
/** FL27 (`FilesService.getFileLinks`) — `SELECT fl.*, r.title as reservation_title FROM file_links fl LEFT JOIN reservations r ON fl.reservation_id = r.id WHERE fl.file_id = ?`. */
async listForFileWithReservationTitle(file_id: number): Promise<FileLinkWithReservationRow[]> {
return await this.kysely<FileLinksKyselyDB>()
.selectFrom('file_links as fl')
.leftJoin('reservations as r', 'r.id', 'fl.reservation_id')
.selectAll('fl')
.select(['r.title as reservation_title'])
.where('fl.file_id', '=', file_id)
.execute();
}
}
@@ -892,4 +892,24 @@ export class PlacesRepository extends TrekRepository<Places> {
.where({ id })
.execute<{ name: string } | undefined>('get', false);
}
// ---------------------------------------------------------------------------
// Plan 3e Task 1 (`FilesService.findForeignLinkTarget`, R12) — additive,
// append-only per that task's own file-ownership rule.
// ---------------------------------------------------------------------------
/**
* FL2 (`FilesService.findForeignLinkTarget`'s place branch) — `SELECT 1
* FROM places WHERE id = ? AND trip_id = ?`, re-expressed as "what trip
* does this row belong to" (R12, the same shape as
* `ReservationsRepository.findTripId`'s docstring).
*/
async findTripId(id: number): Promise<number | undefined> {
const platform = this.getEntityManager().getPlatform();
const row = await this.qb('p')
.select([columnRef(platform, 'p.trip_id').as('trip_id')])
.where({ id })
.execute<{ trip_id: number } | undefined>('get', false);
return row?.trip_id;
}
}
@@ -1439,6 +1439,30 @@ export class ReservationsRepository extends TrekRepository<Reservations> {
.execute();
return rows as PublicApiUnplannedPlaceRow[];
}
// ---------------------------------------------------------------------------
// Plan 3e Task 1 (`FilesService.findForeignLinkTarget`, R12) — additive,
// append-only per that task's own file-ownership rule.
// ---------------------------------------------------------------------------
/**
* FL1 (`FilesService.findForeignLinkTarget`'s reservation branch) —
* `SELECT 1 FROM reservations WHERE id = ? AND trip_id = ?`, re-expressed
* as "what trip does this row belong to" (R12: one method per target
* table, no dynamic identifier dispatch — the RS21/RS23/RS25 D4 precedent)
* so the caller compares the returned `trip_id` against its own, already
* `toRowId`-parsed value instead of binding two ids into the query itself.
* `trip_id` is a `persist(false)` mirror of the `trip` relation —
* `columnRef`, not a bare select (the program-wide trap).
*/
async findTripId(id: number): Promise<number | undefined> {
const platform = this.getEntityManager().getPlatform();
const row = await this.qb('r')
.select([columnRef(platform, 'r.trip_id').as('trip_id')])
.where({ id })
.execute<{ trip_id: number } | undefined>('get', false);
return row?.trip_id;
}
}
/** `SELECT *` — every scalar column of `Reservations`, RS35's shape. */
@@ -1,4 +1,209 @@
import { currentTimestamp } from '../dialect/sql-functions';
import type { TripFiles } from '../entities/TripFiles.entity';
import { type AssertRowKeys } from './_shared/rows';
import { TrekRepository } from './_shared/trek-repository';
export class TripFilesRepository extends TrekRepository<TripFiles> {}
/**
* A bare `trip_files` row — every scalar column of the entity, incl. the SIX
* `persist(false)` relation mirrors (`trip_id`, `place_id`, `reservation_id`,
* `note_id`, `uploaded_by`, `message_id` — the trap the class docstring
* below names). Kysely's `selectAll()` reads real DB columns directly, so it
* carries every one of these without the bare-QB-`.select([...])` drop a
* `columnRef` would otherwise be needed to avoid.
*/
export interface TripFileRow {
id: number;
trip_id: number;
place_id: number | null;
reservation_id: number | null;
filename: string;
original_name: string;
file_size: number | null;
mime_type: string | null;
description: string | null;
created_at: string | null;
note_id: number | null;
uploaded_by: number | null;
starred: number | null;
deleted_at: string | null;
message_id: number | null;
}
const _tripFileRowKeys: AssertRowKeys<TripFileRow, TripFiles> = true;
/** FILE_SELECT's joined projection (FL7/FL11/FL15/FL17/FL20). */
export interface TripFileJoinRow extends TripFileRow {
reservation_title: string | null;
uploaded_by_name: string | null;
uploaded_by_avatar: string | null;
}
interface TripFilesKyselyDB {
trip_files: TripFileRow;
reservations: { id: number; title: string };
users: { id: number; username: string; avatar: string | null };
}
/**
* `trip_files` — trip attachments (uploads, expense receipts). Every read
* below goes through Kysely rather than the QueryBuilder: this entity has
* SIX `persist(false)` mirror columns (see {@link TripFileRow}'s docstring),
* and a bare QB `.select([...])` silently drops any of them it names without
* `columnRef` (the program-wide trap `RoadtripDayBoundariesRepository`
* documents) — Kysely's `selectAll()`/typed column list reads the real DB
* columns directly and has no such gap, so it is the simpler, uniformly-safe
* choice for a row this wide rather than six `columnRef` calls repeated
* across five read methods.
*/
export class TripFilesRepository extends TrekRepository<TripFiles> {
/** FILE_SELECT (`:22-27`) — `trip_files f LEFT JOIN reservations r ON f.reservation_id = r.id LEFT JOIN users u ON f.uploaded_by = u.id`. */
private joinedQuery() {
return this.kysely<TripFilesKyselyDB>()
.selectFrom('trip_files as f')
.leftJoin('reservations as r', 'r.id', 'f.reservation_id')
.leftJoin('users as u', 'u.id', 'f.uploaded_by')
.selectAll('f')
.select(['r.title as reservation_title', 'u.username as uploaded_by_name', 'u.avatar as uploaded_by_avatar']);
}
/**
* FL5 (`FilesService.getFileById`) — `SELECT * FROM trip_files WHERE id = ?
* AND trip_id = ?`, the trip-scoping guard reused by budget/collab reads
* (BG29/44, CB21) and by `permanentDeleteFile`/`softDeleteFile`/`remove`'s
* pre-image reads.
*/
async findInTrip(id: number, trip_id: number): Promise<TripFileRow | undefined> {
return await this.kysely<TripFilesKyselyDB>()
.selectFrom('trip_files')
.selectAll()
.where('id', '=', id)
.where('trip_id', '=', trip_id)
.executeTakeFirst();
}
/** FL6 (`FilesService.getDeletedFile`) — same as {@link findInTrip} plus `AND deleted_at IS NOT NULL`, the trash-scoped variant. */
async findDeletedInTrip(id: number, trip_id: number): Promise<TripFileRow | undefined> {
return await this.kysely<TripFilesKyselyDB>()
.selectFrom('trip_files')
.selectAll()
.where('id', '=', id)
.where('trip_id', '=', trip_id)
.where('deleted_at', 'is not', null)
.executeTakeFirst();
}
/**
* FL7 (`FilesService.listFiles`) — `` FILE_SELECT WHERE f.trip_id = ? AND
* f.message_id IS NULL AND f.deleted_at IS (NOT) NULL ORDER BY f.starred
* DESC, f.created_at DESC ``. `showTrash` toggles the `deleted_at` arm
* (D4: two WHERE shapes, one method, matching the legacy ternary).
*/
async listForTrip(trip_id: number, showTrash: boolean): Promise<TripFileJoinRow[]> {
let q = this.joinedQuery().where('f.trip_id', '=', trip_id).where('f.message_id', 'is', null);
q = showTrash ? q.where('f.deleted_at', 'is not', null) : q.where('f.deleted_at', 'is', null);
return await q.orderBy('f.starred', 'desc').orderBy('f.created_at', 'desc').execute();
}
/**
* FL11/FL15/FL17/FL20 — `` FILE_SELECT WHERE f.id = ? `` , the SAME
* re-select text run after `createFile`/`updateFile`/`toggleStarred`/
* `restoreFile`. No `trip_id` filter (matching the legacy statement — the
* caller already knows `id` is in-scope by construction, having just
* written it).
*/
async findByIdWithJoins(id: number): Promise<TripFileJoinRow | undefined> {
return await this.joinedQuery().where('f.id', '=', id).executeTakeFirst();
}
/** FL22 (`FilesService.emptyTrash`'s read half) — `SELECT * FROM trip_files WHERE trip_id = ? AND deleted_at IS NOT NULL`. */
async listTrashed(trip_id: number): Promise<TripFileRow[]> {
return await this.kysely<TripFilesKyselyDB>()
.selectFrom('trip_files')
.selectAll()
.where('trip_id', '=', trip_id)
.where('deleted_at', 'is not', null)
.execute();
}
/**
* FL9 (`FilesService.createFile`) — `INSERT INTO trip_files (trip_id,
* place_id, reservation_id, filename, original_name, file_size, mime_type,
* description, uploaded_by) VALUES (?×9)`. Relation properties
* (`trip`/`place`/`reservation`/`uploadedByRef`), not their `persist(false)`
* `_id` mirrors — MikroORM accepts a raw scalar id for a `Ref`-typed
* property (`InviteTokensRepository.insertInvite`'s precedent).
* `uploadedByRef` is the entity's own name for the `uploaded_by`
* `joinColumn` relation (`TripFiles.entity.ts`). `trip_id: number |
* string` — the caller's own trip id, already vouched for by the
* guard/access check upstream, written (never compared) here; SQLite's
* own INTEGER-affinity conversion on INSERT normalises either shape the
* same way `DayAccommodationsRepository.insertStay`'s `trip_id` does, so
* no `toRowId` gate is needed on a write-only value (rule 21 governs the
* read-then-write seam, not every scalar a write ever carries).
*/
async insertFile(row: {
trip_id: number | string;
place_id: number | null;
reservation_id: number | null;
filename: string;
original_name: string;
file_size: number;
mime_type: string;
description: string | null;
uploaded_by: number;
}): Promise<number> {
return await this.insert({
trip: row.trip_id,
place: row.place_id,
reservation: row.reservation_id,
filename: row.filename,
original_name: row.original_name,
file_size: row.file_size,
mime_type: row.mime_type,
description: row.description,
uploadedByRef: row.uploaded_by,
});
}
/**
* FL12 (`FilesService.updateFile`) — `UPDATE trip_files SET description =
* ?, place_id = ?, reservation_id = ? WHERE id = ?`. The caller resolves
* the `!== undefined ? (x || null) : current.x` presence sentinel BEFORE
* calling this (matching the legacy inline ternaries) — this method just
* writes the three final values verbatim.
*/
async updateFile(id: number, write: { description: string | null; place_id: number | null; reservation_id: number | null }): Promise<void> {
await this.nativeUpdate({ id }, { description: write.description, place: write.place_id, reservation: write.reservation_id });
}
/** FL16 (`FilesService.toggleStarred`) — `UPDATE trip_files SET starred = ? WHERE id = ?`. */
async setStarred(id: number, starred: number): Promise<void> {
await this.nativeUpdate({ id }, { starred });
}
/** FL18 (`FilesService.softDeleteFile`) — `UPDATE trip_files SET deleted_at = CURRENT_TIMESTAMP WHERE id = ?`. */
async softDelete(id: number): Promise<void> {
const platform = this.getEntityManager().getPlatform();
await this.nativeUpdate({ id }, { deleted_at: currentTimestamp(platform) });
}
/** FL19 (`FilesService.restoreFile`) — `UPDATE trip_files SET deleted_at = NULL WHERE id = ?`. */
async restore(id: number): Promise<void> {
await this.nativeUpdate({ id }, { deleted_at: null });
}
/** FL21 (`FilesService.permanentDeleteFile`) — `DELETE FROM trip_files WHERE id = ?`, run after a successful (or idempotent-missing) storage delete. */
async deleteById(id: number): Promise<void> {
await this.nativeDelete({ id });
}
/**
* FL23 (`FilesService.emptyTrash`'s bulk delete) — `DELETE FROM trip_files
* WHERE id IN (dynamic)`, only for the ids whose storage delete already
* succeeded. `$in` for the dynamic list (rule 23) — no interpolated SQL.
*/
async deleteMany(ids: number[]): Promise<void> {
if (ids.length === 0) return;
await this.nativeDelete({ id: { $in: ids } });
}
}
@@ -1,11 +1,16 @@
import { Module } from '@nestjs/common';
import { MikroOrmModule } from '@mikro-orm/nestjs';
import { AllowedFileTypesService } from './allowed-file-types.service';
import { AppSettings } from '../../db/entities/AppSettings.entity';
/**
* A leaf so both upload paths can reach the extension list without importing
* each other's domain. DatabaseModule is @Global, so this imports nothing.
* each other's domain. `MikroOrmModule.forFeature([AppSettings])` registers
* `AppSettingsRepository` for `AllowedFileTypesService`'s `@InjectRepository`
* (FL29 — the existing 3a repository, this table is not this plan's own).
*/
@Module({
imports: [MikroOrmModule.forFeature([AppSettings])],
providers: [AllowedFileTypesService],
exports: [AllowedFileTypesService],
})
@@ -1,5 +1,7 @@
import { Injectable } from '@nestjs/common';
import { DatabaseService } from '../database/database.service';
import { InjectRepository } from '@mikro-orm/nestjs';
import { AppSettings } from '../../db/entities/AppSettings.entity';
import type { AppSettingsRepository } from '../../db/repositories/AppSettings.repository';
import { DEFAULT_ALLOWED_EXTENSIONS } from './files.constants';
/**
@@ -10,18 +12,20 @@ import { DEFAULT_ALLOWED_EXTENSIONS } from './files.constants';
* thing an upload's fileFilter needs from the container, and it is a live read:
* an admin changing the list in settings applies to the next upload, with no
* invalidation wiring.
*
* FL29 — `SELECT value FROM app_settings WHERE key = 'allowed_file_types'`,
* now `AppSettingsRepository.getValue` (the existing 3a repository, per the
* plan's R12/§5 note — this table is not this plan's own).
*/
@Injectable()
export class AllowedFileTypesService {
constructor(private readonly db: DatabaseService) {}
constructor(@InjectRepository(AppSettings) private readonly appSettings: AppSettingsRepository) {}
/** Comma-separated, as the admin panel stores it. `*` means anything. */
async get(): Promise<string> {
try {
const row = this.db.get<{ value: string }>(
"SELECT value FROM app_settings WHERE key = 'allowed_file_types'",
);
return row?.value || DEFAULT_ALLOWED_EXTENSIONS;
const value = await this.appSettings.getValue('allowed_file_types');
return value || DEFAULT_ALLOWED_EXTENSIONS;
} catch {
return DEFAULT_ALLOWED_EXTENSIONS;
}
+15
View File
@@ -1,4 +1,5 @@
import { Module } from '@nestjs/common';
import { MikroOrmModule } from '@mikro-orm/nestjs';
import { FilesController } from './files.controller';
import { FilesDownloadController } from './files-download.controller';
import { FilesService } from './files.service';
@@ -19,6 +20,13 @@ import { StorageService } from '../storage/storage.service';
import { buildStorageUploadOptions } from '../storage/storage-upload.factory';
import { filesUploadFileFilter } from './files.controller';
import { MAX_VIDEO_SIZE } from './files.constants';
import { TripFiles } from '../../db/entities/TripFiles.entity';
import { FileLinks } from '../../db/entities/FileLinks.entity';
import { Reservations } from '../../db/entities/Reservations.entity';
import { Places } from '../../db/entities/Places.entity';
import { DayAssignments } from '../../db/entities/DayAssignments.entity';
import { BudgetItems } from '../../db/entities/BudgetItems.entity';
import { Users } from '../../db/entities/Users.entity';
@Module({
imports: [
@@ -36,6 +44,13 @@ import { MAX_VIDEO_SIZE } from './files.constants';
}),
}),
StorageModule,
// TripFiles/FileLinks are this domain's own tables (Plan 3e Task 1).
// Reservations/Places/DayAssignments/BudgetItems/Users are owned
// elsewhere — registered here only for `findForeignLinkTarget`'s
// `findTripId` reads and FL28's `UsersRepository.getEmail` (the entity
// classes only, never the owning module — the `AccommodationsDomainModule`
// precedent).
MikroOrmModule.forFeature([TripFiles, FileLinks, Reservations, Places, DayAssignments, BudgetItems, Users]),
// AuthModule + McpSharedModule feed FilesMcp's demo and RBAC guards. Neither is
// @Global, and AuthModule reaches this domain only through the leaf
// AllowedFileTypesModule, so importing it here stays cycle-free.
+7 -4
View File
@@ -1,13 +1,15 @@
import pathMod from 'node:path';
import { randomUUID } from 'node:crypto';
import { Readable } from 'node:stream';
import { InjectRepository } from '@mikro-orm/nestjs';
import { PluginController, PluginMethod } from '../plugins/host/rpc-kit/decorators';
import { PluginGuards } from '../plugins/host/plugin-guards.service';
import { BadParams, ForbiddenResource } from '../plugins/host/rpc-errors';
import { asPayload, num } from '../plugins/host/rpc-params';
import type { PluginRpcContext } from '../plugins/host/rpc-kit/types';
import { RealtimeService } from '../realtime/realtime.service';
import { DatabaseService } from '../database/database.service';
import { Users } from '../../db/entities/Users.entity';
import type { UsersRepository } from '../../db/repositories/Users.repository';
import { readEnv } from '../../app-config';
import { isDemoEmail } from '../common/demo';
import { BLOCKED_EXTENSIONS } from './files.constants';
@@ -48,7 +50,7 @@ export class FilesRpc {
constructor(
private readonly files: FilesService,
private readonly realtime: RealtimeService,
private readonly db: DatabaseService,
@InjectRepository(Users) private readonly users: UsersRepository,
private readonly guards: PluginGuards,
private readonly storage: StorageService,
) {}
@@ -110,8 +112,9 @@ export class FilesRpc {
// demo instance, not even through a plugin's db:write:files. The email is only
// resolved when demo mode is actually on, so self-hosted installs pay nothing.
if (readEnv().demo.enabled) {
const uploader = this.db.prepare('SELECT email FROM users WHERE id = ?').get(actingUserId) as { email?: string } | undefined;
if (isDemoEmail(uploader?.email)) throw new ForbiddenResource('Uploads are disabled in demo mode.');
// FL28 — `SELECT email FROM users WHERE id = ?`, now `UsersRepository.getEmail`.
const email = await this.users.getEmail(actingUserId);
if (isDemoEmail(email)) throw new ForbiddenResource('Uploads are disabled in demo mode.');
}
const original = pathMod.basename(input.name);
const ext = pathMod.extname(original).toLowerCase();
+177 -108
View File
@@ -1,4 +1,5 @@
import { Injectable } from '@nestjs/common';
import { InjectRepository } from '@mikro-orm/nestjs';
import path from 'path';
import type { Readable } from 'node:stream';
import type { Request } from 'express';
@@ -10,23 +11,29 @@ import { EphemeralTokenService } from '../auth/ephemeral-token.service';
import { verifyJwtAndLoadUser } from '../auth/jwt-verify';
import { EntityManager } from '@mikro-orm/core';
import { Users } from '../../db/entities/Users.entity';
import type { User, TripFile } from '../../types';
import type { User } from '../../types';
import { DatabaseService, type TripAccess } from '../database/database.service';
import { DEFAULT_ALLOWED_EXTENSIONS } from './files.constants';
import { UnitOfWork } from '../database/unit-of-work';
import { toRowId } from '../common/row-id';
import { StorageService } from '../storage/storage.service';
import { StorageNotFoundError, StorageInvalidKeyError, type ObjectStat } from '../storage/storage.types';
import { TripFiles } from '../../db/entities/TripFiles.entity';
import type { TripFilesRepository, TripFileRow, TripFileJoinRow } from '../../db/repositories/TripFiles.repository';
import { FileLinks } from '../../db/entities/FileLinks.entity';
import type { FileLinksRepository, FileLinkTargetRow } from '../../db/repositories/FileLinks.repository';
import { Reservations } from '../../db/entities/Reservations.entity';
import type { ReservationsRepository } from '../../db/repositories/Reservations.repository';
import { Places } from '../../db/entities/Places.entity';
import type { PlacesRepository } from '../../db/repositories/Places.repository';
import { DayAssignments } from '../../db/entities/DayAssignments.entity';
import type { DayAssignmentsRepository } from '../../db/repositories/DayAssignments.repository';
import { BudgetItems } from '../../db/entities/BudgetItems.entity';
import type { BudgetItemsRepository } from '../../db/repositories/BudgetItems.repository';
type Trip = TripAccess;
type FilePermission = 'file_upload' | 'file_edit' | 'file_delete';
const FILE_SELECT = `
SELECT f.*, r.title as reservation_title, u.username as uploaded_by_name, u.avatar as uploaded_by_avatar
FROM trip_files f
LEFT JOIN reservations r ON f.reservation_id = r.id
LEFT JOIN users u ON f.uploaded_by = u.id
`;
function formatFile(file: TripFile & { uploaded_by_avatar?: string | null }) {
function formatFile(file: TripFileJoinRow) {
const tripId = file.trip_id;
return {
...file,
@@ -57,21 +64,52 @@ export class FileContentError extends Error {
}
}
/**
* A truthy, non-canonical link-target id is narrowed to `null` (rule 15's
* accepted deviation, applied everywhere a foreign-key id from the wire
* reaches a typed repository write): the legacy statements bound the raw
* value straight into `WHERE id = ? AND trip_id = ?`/an INSERT column and
* let SQLite's loose affinity or a constraint failure decide, matching
* `|| null` on falsy. A falsy value (`0`/`''`/`null`/`undefined`) always
* meant "no link" and still does; a truthy value that isn't a canonical
* decimal id now also resolves to "no link" instead of whatever SQLite's
* affinity would have matched or thrown on.
*/
function coerceLinkId(value: string | number | null | undefined): number | null {
if (!value) return null;
return toRowId(value);
}
/**
* File domain service — owns the file SQL (moved 1:1 from the legacy
* services/fileService.ts: identical statements, the `||` falsy-coercion
* defaults, the post-write FILE_SELECT re-selects, the dynamic IN batches and
* the unlink-first delete semantics). Trip access goes through the injected
* DatabaseService's canAccessTrip (the legacy verifyTripAccess re-export was a
* plain wrapper over the same helper); the file_* permissions, path-resolution
* guard, download-token auth and WebSocket broadcasts are unchanged. The
* load-time constants live in files.constants.ts; the admin allowed-types
* live-read is AllowedFileTypesService (the single query owner since the
* storage slice-2 consolidation).
* the unlink-first delete semantics; Plan 3e Task 1 moved the SQL itself onto
* `TripFilesRepository`/`FileLinksRepository`). Trip access goes through the
* injected DatabaseService's canAccessTrip (the legacy verifyTripAccess
* re-export was a plain wrapper over the same helper); the file_*
* permissions, path-resolution guard, download-token auth and WebSocket
* broadcasts are unchanged. The load-time constants live in files.constants.ts;
* the admin allowed-types live-read is AllowedFileTypesService (the single
* query owner since the storage slice-2 consolidation).
*
* Two deliberate post-migration fixes over the legacy behavior: createFileLink
* no longer swallows insert errors, and updateFile coerces an empty-string
* description to NULL exactly like createFile does.
* Two deliberate post-migration fixes over the legacy behavior, carried
* unchanged from the pre-repository version: createFileLink no longer
* swallows insert errors, and updateFile coerces an empty-string description
* to NULL exactly like createFile does.
*
* **R2 (Plan 3e, flagged behaviour change — failure atomicity):** `files`
* was the one domain in this cluster with ZERO `uow.transactional` calls
* despite three genuinely multi-statement writes (`createFile`+its
* conditional `file_links` insert, `updateFile`+its conditional
* `file_links` insert/delete, `emptyTrash`'s DB-only bulk delete). Every
* sibling domain already carries this fix from its own 2026-08 fold; files
* never got it. `createFile`, `updateFile` and `emptyTrash`'s bulk delete
* now run inside `uow.transactional` — a failed second statement no longer
* leaves an orphaned `trip_files` row with no matching `file_links` row (or
* vice versa). The storage I/O in `permanentDeleteFile`/`emptyTrash` stays
* OUTSIDE every transactional body (program rule 24); `emptyTrash`'s read
* (the trashed-file list) also stays outside, matching the DB-only-body rule.
*/
@Injectable()
export class FilesService {
@@ -86,6 +124,13 @@ export class FilesService {
// kept uniform with the guards rather than adding a one-off
// MikroOrmModule.forFeature([Users]) to FilesModule for this single call.
private readonly em: EntityManager,
private readonly uow: UnitOfWork,
@InjectRepository(TripFiles) private readonly tripFilesRepo: TripFilesRepository,
@InjectRepository(FileLinks) private readonly fileLinksRepo: FileLinksRepository,
@InjectRepository(Reservations) private readonly reservationsRepo: ReservationsRepository,
@InjectRepository(Places) private readonly placesRepo: PlacesRepository,
@InjectRepository(DayAssignments) private readonly dayAssignmentsRepo: DayAssignmentsRepository,
@InjectRepository(BudgetItems) private readonly budgetItemsRepo: BudgetItemsRepository,
) {}
async verifyTripAccess(tripId: string | number, userId: number) {
@@ -142,22 +187,39 @@ export class FilesService {
* (or a file_link) at trip B's reservation id and read the title back. Returns
* the first field that escapes `tripId`, or null when every supplied id belongs
* to the trip. Absent / null / zero ids are ignored (they clear the link).
*
* FL1-4 (`findForeignLinkTarget`'s four-table dispatch) are each now one
* `findTripId(id)` method on the target's OWN repository (R12 — no
* interpolated/dynamic table-name dispatch): the id is parsed once via
* `toRowId` here and compared against the row's own `trip_id` — a
* non-canonical id or a row in a different trip both read as "foreign"
* (rule 15's accepted narrowing: a non-canonical id can never have been
* produced by our own client).
*/
async findForeignLinkTarget(
tripId: string | number,
opts: { reservation_id?: string | number | null; assignment_id?: string | number | null; place_id?: string | number | null; budget_item_id?: string | number | null }
): Promise<'reservation_id' | 'assignment_id' | 'place_id' | 'budget_item_id' | null> {
if (opts.reservation_id && !this.db.get('SELECT 1 FROM reservations WHERE id = ? AND trip_id = ?', opts.reservation_id, tripId)) {
return 'reservation_id';
const tripIdNum = toRowId(tripId);
if (opts.reservation_id) {
const idNum = toRowId(opts.reservation_id);
const rowTripId = idNum === null ? undefined : await this.reservationsRepo.findTripId(idNum);
if (rowTripId === undefined || rowTripId !== tripIdNum) return 'reservation_id';
}
if (opts.place_id && !this.db.get('SELECT 1 FROM places WHERE id = ? AND trip_id = ?', opts.place_id, tripId)) {
return 'place_id';
if (opts.place_id) {
const idNum = toRowId(opts.place_id);
const rowTripId = idNum === null ? undefined : await this.placesRepo.findTripId(idNum);
if (rowTripId === undefined || rowTripId !== tripIdNum) return 'place_id';
}
if (opts.assignment_id && !this.db.get('SELECT 1 FROM day_assignments a JOIN days d ON a.day_id = d.id WHERE a.id = ? AND d.trip_id = ?', opts.assignment_id, tripId)) {
return 'assignment_id';
if (opts.assignment_id) {
const idNum = toRowId(opts.assignment_id);
const rowTripId = idNum === null ? undefined : await this.dayAssignmentsRepo.findTripId(idNum);
if (rowTripId === undefined || rowTripId !== tripIdNum) return 'assignment_id';
}
if (opts.budget_item_id && !this.db.get('SELECT 1 FROM budget_items WHERE id = ? AND trip_id = ?', opts.budget_item_id, tripId)) {
return 'budget_item_id';
if (opts.budget_item_id) {
const idNum = toRowId(opts.budget_item_id);
const rowTripId = idNum === null ? undefined : await this.budgetItemsRepo.findTripId(idNum);
if (rowTripId === undefined || rowTripId !== tripIdNum) return 'budget_item_id';
}
return null;
}
@@ -166,12 +228,18 @@ export class FilesService {
// CRUD
// ---------------------------------------------------------------------------
async getFileById(id: string | number, tripId: string | number): Promise<TripFile | undefined> {
return this.db.get<TripFile>('SELECT * FROM trip_files WHERE id = ? AND trip_id = ?', id, tripId);
async getFileById(id: string | number, tripId: string | number): Promise<TripFileRow | undefined> {
const idNum = toRowId(id);
const tripIdNum = toRowId(tripId);
if (idNum === null || tripIdNum === null) return undefined;
return await this.tripFilesRepo.findInTrip(idNum, tripIdNum);
}
async getDeletedFile(id: string | number, tripId: string | number): Promise<TripFile | undefined> {
return this.db.get<TripFile>('SELECT * FROM trip_files WHERE id = ? AND trip_id = ? AND deleted_at IS NOT NULL', id, tripId);
async getDeletedFile(id: string | number, tripId: string | number): Promise<TripFileRow | undefined> {
const idNum = toRowId(id);
const tripIdNum = toRowId(tripId);
if (idNum === null || tripIdNum === null) return undefined;
return await this.tripFilesRepo.findDeletedInTrip(idNum, tripIdNum);
}
/**
@@ -233,20 +301,15 @@ export class FilesService {
}
async listFiles(tripId: string | number, showTrash: boolean) {
const where = showTrash
? 'f.trip_id = ? AND f.deleted_at IS NOT NULL AND f.message_id IS NULL'
: 'f.trip_id = ? AND f.deleted_at IS NULL AND f.message_id IS NULL';
const files = this.db.all<TripFile>(`${FILE_SELECT} WHERE ${where} ORDER BY f.starred DESC, f.created_at DESC`, tripId);
const tripIdNum = toRowId(tripId) ?? -1;
const files = await this.tripFilesRepo.listForTrip(tripIdNum, showTrash);
const fileIds = files.map(f => f.id);
const linksMap: Record<number, FileLink[]> = {};
if (fileIds.length > 0) {
const placeholders = fileIds.map(() => '?').join(',');
const links = this.db.all<FileLink>(`SELECT file_id, reservation_id, place_id, budget_item_id FROM file_links WHERE file_id IN (${placeholders})`, ...fileIds);
for (const link of links) {
if (!linksMap[link.file_id]) linksMap[link.file_id] = [];
linksMap[link.file_id].push(link);
}
const links = await this.fileLinksRepo.listForFiles(fileIds);
const linksMap: Record<number, FileLinkTargetRow[]> = {};
for (const link of links) {
if (!linksMap[link.file_id]) linksMap[link.file_id] = [];
linksMap[link.file_id].push(link);
}
return files.map(f => {
@@ -260,84 +323,87 @@ export class FilesService {
});
}
/** R2: `createFile`'s `trip_files` insert and its conditional `file_links` insert (FL9+FL10) run inside one transaction. */
async createFile(
tripId: string | number,
file: { filename: string; originalname: string; size: number; mimetype: string },
uploadedBy: number,
opts: { place_id?: string | number | null; reservation_id?: string | number | null; budget_item_id?: string | number | null; description?: string | null }
) {
const result = this.db.run(`
INSERT INTO trip_files (trip_id, place_id, reservation_id, filename, original_name, file_size, mime_type, description, uploaded_by)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)
`,
tripId,
opts.place_id || null,
opts.reservation_id || null,
file.filename,
file.originalname,
file.size,
file.mimetype,
opts.description || null,
uploadedBy
);
const newId = await this.uow.transactional(async () => {
const id = await this.tripFilesRepo.insertFile({
trip_id: tripId,
place_id: coerceLinkId(opts.place_id),
reservation_id: coerceLinkId(opts.reservation_id),
filename: file.filename,
original_name: file.originalname,
file_size: file.size,
mime_type: file.mimetype,
description: opts.description || null,
uploaded_by: uploadedBy,
});
if (opts.budget_item_id) {
this.db.run('INSERT OR IGNORE INTO file_links (file_id, budget_item_id) VALUES (?, ?)', result.lastInsertRowid, opts.budget_item_id);
}
const budgetItemId = coerceLinkId(opts.budget_item_id);
if (budgetItemId) {
await this.fileLinksRepo.insertIgnore({ file_id: id, budget_item_id: budgetItemId });
}
return id;
});
const created = this.db.get<TripFile>(`${FILE_SELECT} WHERE f.id = ?`, result.lastInsertRowid)!;
return formatFile(created);
const created = await this.tripFilesRepo.findByIdWithJoins(newId);
return formatFile(created!);
}
/** R2: `updateFile`'s field update and its conditional `file_links` insert/delete (FL12+FL13+FL14) run inside one transaction. */
async updateFile(
id: string | number,
current: TripFile,
current: TripFileRow,
updates: { description?: string; place_id?: string | number | null; reservation_id?: string | number | null; budget_item_id?: string | number | null }
) {
this.db.run(`
UPDATE trip_files SET
description = ?,
place_id = ?,
reservation_id = ?
WHERE id = ?
`,
updates.description !== undefined ? (updates.description || null) : current.description,
updates.place_id !== undefined ? (updates.place_id || null) : current.place_id,
updates.reservation_id !== undefined ? (updates.reservation_id || null) : current.reservation_id,
id
);
const idNum = toRowId(id) ?? -1;
if (updates.budget_item_id !== undefined) {
if (updates.budget_item_id) {
this.db.run('INSERT OR IGNORE INTO file_links (file_id, budget_item_id) VALUES (?, ?)', id, updates.budget_item_id);
} else {
this.db.run('DELETE FROM file_links WHERE file_id = ? AND budget_item_id IS NOT NULL', id);
await this.uow.transactional(async () => {
await this.tripFilesRepo.updateFile(idNum, {
description: updates.description !== undefined ? (updates.description || null) : (current.description ?? null),
place_id: updates.place_id !== undefined ? coerceLinkId(updates.place_id) : (current.place_id ?? null),
reservation_id: updates.reservation_id !== undefined ? coerceLinkId(updates.reservation_id) : (current.reservation_id ?? null),
});
if (updates.budget_item_id !== undefined) {
const budgetItemId = coerceLinkId(updates.budget_item_id);
if (budgetItemId) {
await this.fileLinksRepo.insertIgnore({ file_id: idNum, budget_item_id: budgetItemId });
} else {
await this.fileLinksRepo.clearBudgetLink(idNum);
}
}
}
});
const updated = this.db.get<TripFile>(`${FILE_SELECT} WHERE f.id = ?`, id)!;
return formatFile(updated);
const updated = await this.tripFilesRepo.findByIdWithJoins(idNum);
return formatFile(updated!);
}
async toggleStarred(id: string | number, currentStarred: number | undefined) {
const idNum = toRowId(id) ?? -1;
const newStarred = currentStarred ? 0 : 1;
this.db.run('UPDATE trip_files SET starred = ? WHERE id = ?', newStarred, id);
await this.tripFilesRepo.setStarred(idNum, newStarred);
const updated = this.db.get<TripFile>(`${FILE_SELECT} WHERE f.id = ?`, id)!;
return formatFile(updated);
const updated = await this.tripFilesRepo.findByIdWithJoins(idNum);
return formatFile(updated!);
}
async softDeleteFile(id: string | number) {
this.db.run('UPDATE trip_files SET deleted_at = CURRENT_TIMESTAMP WHERE id = ?', id);
await this.tripFilesRepo.softDelete(toRowId(id) ?? -1);
}
async restoreFile(id: string | number) {
this.db.run('UPDATE trip_files SET deleted_at = NULL WHERE id = ?', id);
const restored = this.db.get<TripFile>(`${FILE_SELECT} WHERE f.id = ?`, id)!;
return formatFile(restored);
const idNum = toRowId(id) ?? -1;
await this.tripFilesRepo.restore(idNum);
const restored = await this.tripFilesRepo.findByIdWithJoins(idNum);
return formatFile(restored!);
}
async permanentDeleteFile(file: TripFile): Promise<void> {
async permanentDeleteFile(file: TripFileRow): Promise<void> {
// 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
@@ -348,11 +414,13 @@ export class FilesService {
console.error(`[files] unlink failed for ${file.filename}, keeping DB row:`, e);
throw e;
}
this.db.run('DELETE FROM trip_files WHERE id = ?', file.id);
await this.tripFilesRepo.deleteById(file.id);
}
/** R2: the storage-delete loop stays outside any transaction (rule 24); only the final bulk `trip_files` delete (FL23) runs inside one. */
async emptyTrash(tripId: string | number): Promise<number> {
const trashed = this.db.all<TripFile>('SELECT * FROM trip_files WHERE trip_id = ? AND deleted_at IS NOT NULL', tripId);
const tripIdNum = toRowId(tripId) ?? -1;
const trashed = await this.tripFilesRepo.listTrashed(tripIdNum);
// Collect successful IDs separately so we only DELETE rows whose disk
// content was actually removed — failing unlinks keep their DB row
// and a retry via the single-file delete path can try again.
@@ -360,14 +428,15 @@ export class FilesService {
await Promise.all(trashed.map(async (file) => {
try {
await this.storage.delete('files', path.basename(file.filename));
successfullyUnlinked.push(Number(file.id));
successfullyUnlinked.push(file.id);
} catch (e) {
console.error(`[files] unlink failed for ${file.filename}, keeping DB row:`, e);
}
}));
if (successfullyUnlinked.length > 0) {
const placeholders = successfullyUnlinked.map(() => '?').join(',');
this.db.run(`DELETE FROM trip_files WHERE id IN (${placeholders})`, ...successfullyUnlinked);
await this.uow.transactional(async () => {
await this.tripFilesRepo.deleteMany(successfullyUnlinked);
});
}
return successfullyUnlinked.length;
}
@@ -383,22 +452,22 @@ export class FilesService {
fileId: string | number,
opts: { reservation_id?: string | number | null; assignment_id?: string | number | null; place_id?: string | number | null; budget_item_id?: string | number | null }
) {
this.db.run('INSERT OR IGNORE INTO file_links (file_id, reservation_id, assignment_id, place_id, budget_item_id) VALUES (?, ?, ?, ?, ?)',
fileId, opts.reservation_id || null, opts.assignment_id || null, opts.place_id || null, opts.budget_item_id || null
);
return this.db.all('SELECT * FROM file_links WHERE file_id = ?', fileId);
const idNum = toRowId(fileId) ?? -1;
await this.fileLinksRepo.insertIgnore({
file_id: idNum,
reservation_id: coerceLinkId(opts.reservation_id),
assignment_id: coerceLinkId(opts.assignment_id),
place_id: coerceLinkId(opts.place_id),
budget_item_id: coerceLinkId(opts.budget_item_id),
});
return await this.fileLinksRepo.listForFile(idNum);
}
async deleteFileLink(linkId: string | number, fileId: string | number) {
this.db.run('DELETE FROM file_links WHERE id = ? AND file_id = ?', linkId, fileId);
await this.fileLinksRepo.deleteById(toRowId(linkId) ?? -1, toRowId(fileId) ?? -1);
}
async getFileLinks(fileId: string | number) {
return this.db.all(`
SELECT fl.*, r.title as reservation_title
FROM file_links fl
LEFT JOIN reservations r ON fl.reservation_id = r.id
WHERE fl.file_id = ?
`, fileId);
return await this.fileLinksRepo.listForFileWithReservationTitle(toRowId(fileId) ?? -1);
}
}
+40
View File
@@ -0,0 +1,40 @@
import type Database from 'better-sqlite3';
import { sharedTestOrm } from './test-uow';
import { TripFiles } from '../../src/db/entities/TripFiles.entity';
import type { TripFilesRepository } from '../../src/db/repositories/TripFiles.repository';
import { FileLinks } from '../../src/db/entities/FileLinks.entity';
import type { FileLinksRepository } from '../../src/db/repositories/FileLinks.repository';
import { BudgetItems } from '../../src/db/entities/BudgetItems.entity';
import type { BudgetItemsRepository } from '../../src/db/repositories/BudgetItems.repository';
/**
* Plan 3e Task 1 (`FilesService`) test-only repository factories, bound to a
* suite's own better-sqlite3 handle via the SAME memoised `sharedTestOrm`
* `test-uow.ts`'s helpers use — required so a repository built here and a
* `UnitOfWork` built from `createTestUnitOfWork` (that file) resolve the
* IDENTICAL context-resolving `EntityManager` (a repository from a second,
* independent ORM instance would write outside the open transaction and
* deadlock on Kysely's connection mutex — `test-uow.ts`'s own docstring).
*
* A NEW file rather than an addition to `test-uow.ts`: that file is Plan 3e
* Task 6's named file set in this window (`tests/helpers/{mcp-test-
* controllers,plugin-host,test-uow}.ts`), and `sharedTestOrm`/`t.repo(...)`
* are exported precisely so a later task's tests don't need to edit it to
* get a repository this way. `ReservationsRepository`/`PlacesRepository`/
* `DayAssignmentsRepository`/`UsersRepository`/`AppSettingsRepository`
* already have their own factories in `test-uow.ts` (`createTestReservationsRepo`
* etc.) and are reused directly from there — only `TripFilesRepository`,
* `FileLinksRepository` and `BudgetItemsRepository` are new here.
*/
export function createTestTripFilesRepo(db: Database.Database): Promise<TripFilesRepository> {
return sharedTestOrm(db).then((t) => t.repo(TripFiles));
}
export function createTestFileLinksRepo(db: Database.Database): Promise<FileLinksRepository> {
return sharedTestOrm(db).then((t) => t.repo(FileLinks));
}
/** `BudgetItemsRepository` — Task 2 (budget) owns every method appended to the production file; this task only needs `findTripId` (R12). */
export function createTestBudgetItemsRepo(db: Database.Database): Promise<BudgetItemsRepository> {
return sharedTestOrm(db).then((t) => t.repo(BudgetItems));
}
+13 -5
View File
@@ -20,6 +20,7 @@ import { FilesModule } from '../../../src/nest/files/files.module';
import { FilesService } from '../../../src/nest/files/files.service';
import type { RealtimeService } from '../../../src/nest/realtime/realtime.service';
import type { DatabaseService } from '../../../src/nest/database/database.service';
import type { UsersRepository } from '../../../src/db/repositories/Users.repository';
import type { PermissionsService } from '../../../src/nest/permissions/permissions.service';
import type { AddonsService } from '../../../src/nest/addons/addons.service';
import type { StorageService } from '../../../src/nest/storage/storage.service';
@@ -47,8 +48,11 @@ function build(opts: { file?: Record<string, unknown> | undefined; foreign?: str
} as unknown as FilesService & Record<string, ReturnType<typeof vi.fn>>;
const db = {
canAccessTrip: vi.fn(async (tripId: number, userId: number) => (tripId === 1 && userId === 42 ? { id: 1, user_id: 42 } : undefined)),
prepare: vi.fn(() => ({ get: () => ({ role: 'user', email: 'real@example.test' }) })),
// Still needed by PluginGuards' own role lookup (requireTripEdit) — unrelated to FL28.
prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })),
} as unknown as DatabaseService;
// FL28 — `SELECT email FROM users WHERE id = ?`, now `UsersRepository.getEmail`.
const usersRepo = { getEmail: vi.fn(async () => 'real@example.test') } as unknown as UsersRepository;
const permissions = {
checkPermission: vi.fn((action: string) => (opts.allow ? opts.allow(action) : true)),
} as unknown as PermissionsService;
@@ -66,7 +70,7 @@ function build(opts: { file?: Record<string, unknown> | undefined; foreign?: str
// keep exercising that code rather than a stub of it.
(files as unknown as { readContent: FilesService['readContent'] }).readContent =
FilesService.prototype.readContent.bind({ getFileById: files.getFileById, storage } as unknown as FilesService);
const rpc = new FilesRpc(files, realtime, db, guards, storage);
const rpc = new FilesRpc(files, realtime, usersRepo, guards, storage);
const host = (...grants: string[]) => new PluginRpcHost('p', new Set(grants), makeDeps(), createTestPluginRegistry([rpc]));
return { files, realtime, permissions, storage, host };
}
@@ -255,10 +259,10 @@ describe('FilesRpc writes', () => {
process.env.DEMO_MODE = 'true';
try {
const f = build();
// The demo guard resolves the uploader's email; user 9 is the demo account.
const db = {
canAccessTrip: vi.fn(async () => ({ id: 1, user_id: 42 })),
prepare: vi.fn(() => ({ get: (id: number) => (id === 9 ? { role: 'user', email: 'demo@trek.app' } : { role: 'user', email: 'real@example.test' }) })),
// Still needed by PluginGuards' own role lookup — unrelated to FL28.
prepare: vi.fn(() => ({ get: () => ({ role: 'user' }) })),
} as unknown as DatabaseService;
const guards = new PluginGuards(
db,
@@ -270,7 +274,11 @@ describe('FilesRpc writes', () => {
createFile: vi.fn(() => ({ id: 130 })),
} as unknown as FilesService;
const storage = { getStream: vi.fn(), put: vi.fn(async () => undefined) } as unknown as StorageService;
const rpc = new FilesRpc(files, { broadcast: vi.fn() } as unknown as RealtimeService, db, guards, storage);
// The demo guard resolves the uploader's email; user 9 is the demo account.
const usersRepo = {
getEmail: vi.fn(async (id: number) => (id === 9 ? 'demo@trek.app' : 'real@example.test')),
} as unknown as UsersRepository;
const rpc = new FilesRpc(files, { broadcast: vi.fn() } as unknown as RealtimeService, usersRepo, guards, storage);
const host = new PluginRpcHost('p', new Set(['db:write:files']), makeDeps(), createTestPluginRegistry([rpc]));
const input = { name: 'a.pdf', content_base64: b64('x') };
const denied = (await host.dispatch(req('files.create', { tripId: 1, input }), 9)) as RpcError;
+174 -15
View File
@@ -57,9 +57,12 @@ import type { Request } from 'express';
import { createTables } from '../../../src/db/schema';
import { runMigrations } from '../../../src/db/migrations';
import { resetTestDb } from '../../helpers/test-db';
import { createUser, createTrip, addTripMember, createPlace, createReservation, createDay, createDayAssignment, setAppSetting } from '../../helpers/factories';
import { createUser, createTrip, addTripMember, createPlace, createReservation, createDay, createDayAssignment, setAppSetting, createCollabNote } from '../../helpers/factories';
import { DatabaseService, type TripAccess } from '../../../src/nest/database/database.service';
import { sharedTestOrm } from '../../helpers/test-uow';
import { sharedTestOrm, createTestUnitOfWork, createTestAppSettingsRepo, createTestReservationsRepo, createTestPlacesRepo, createTestDayAssignmentsRepo } from '../../helpers/test-uow';
import { createTestTripFilesRepo, createTestFileLinksRepo, createTestBudgetItemsRepo } from '../../helpers/files-repos';
import type { TripFilesRepository } from '../../../src/db/repositories/TripFiles.repository';
import type { FileLinksRepository } from '../../../src/db/repositories/FileLinks.repository';
import type { PermissionsService } from '../../../src/nest/permissions/permissions.service';
import { FilesService } from '../../../src/nest/files/files.service';
import { AllowedFileTypesService } from '../../../src/nest/files/allowed-file-types.service';
@@ -72,7 +75,7 @@ import {
isVideoMime,
isVideoExtension,
} from '../../../src/nest/files/files.constants';
import type { TripFile, User } from '../../../src/types';
import type { User } from '../../../src/types';
import { RealtimeService } from '../../../src/nest/realtime/realtime.service';
import { EphemeralTokenService } from '../../../src/nest/auth/ephemeral-token.service';
import type { EntityManager } from '@mikro-orm/core';
@@ -90,19 +93,48 @@ const storageStub = { delete: storageDelete } as unknown as import('../../../src
// `Users` specifically.
const getRepository = vi.fn(() => ({}));
const emStub = { getRepository } as unknown as EntityManager;
const svc = new FilesService(new DatabaseService(testDb), permissionsStub, new RealtimeService(), new EphemeralTokenService(), storageStub, emStub);
// Constructed in `beforeAll` (async — the repositories below resolve through
// `sharedTestOrm`), not at module load: Plan 3e Task 1 adds the `UnitOfWork`
// and the five repositories `FilesService` now needs. `tripFilesRepo`/
// `fileLinksRepo` are kept as named bindings (not only inside `svc`) so the
// R2 rollback tests below can spy on them directly, the same shape
// `reservations.service.test.ts`'s `RESV-FIX-001/002` already use.
let svc: FilesService;
let tripFilesRepo: TripFilesRepository;
let fileLinksRepo: FileLinksRepository;
beforeAll(() => {
createTables(testDb);
runMigrations(testDb);
});
beforeAll(async () => {
tripFilesRepo = await createTestTripFilesRepo(testDb);
fileLinksRepo = await createTestFileLinksRepo(testDb);
svc = new FilesService(
new DatabaseService(testDb),
permissionsStub,
new RealtimeService(),
new EphemeralTokenService(),
storageStub,
emStub,
await createTestUnitOfWork(testDb),
tripFilesRepo,
fileLinksRepo,
await createTestReservationsRepo(testDb),
await createTestPlacesRepo(testDb),
await createTestDayAssignmentsRepo(testDb),
await createTestBudgetItemsRepo(testDb),
);
});
beforeEach(async () => {
resetTestDb(testDb);
vi.clearAllMocks();
// Plan 3c Task 0b: `svc`'s `DatabaseService` is constructed at module load,
// before any `beforeAll`/`beforeEach` can resolve a real `EntityManager` —
// spied directly on that instance, routed to a real `DatabaseService` built
// Plan 3c Task 0b: `svc`'s `DatabaseService` is constructed in `beforeAll`,
// before any `beforeEach` can resolve a real `EntityManager` — spied
// directly on that instance, routed to a real `DatabaseService` built
// with one. Re-applied every test (not `beforeAll`) because `afterEach`
// below `vi.restoreAllMocks()`s it away after each one.
const real = new DatabaseService(testDb, (await sharedTestOrm(testDb)).em);
@@ -191,20 +223,20 @@ describe('files.constants', () => {
describe('AllowedFileTypesService.get', () => {
it('FILE-SVC-006: returns the app_settings value when set', async () => {
setAppSetting(testDb, 'allowed_file_types', 'pdf,txt');
expect(await new AllowedFileTypesService(new DatabaseService(testDb)).get()).toBe('pdf,txt');
expect(await new AllowedFileTypesService(await createTestAppSettingsRepo(testDb)).get()).toBe('pdf,txt');
});
it('FILE-SVC-007: returns the default when the row is absent', async () => {
expect(await new AllowedFileTypesService(new DatabaseService(testDb)).get()).toBe(DEFAULT_ALLOWED_EXTENSIONS);
expect(await new AllowedFileTypesService(await createTestAppSettingsRepo(testDb)).get()).toBe(DEFAULT_ALLOWED_EXTENSIONS);
});
it('FILE-SVC-008: returns the default for an empty value (|| coercion, not ??)', async () => {
setAppSetting(testDb, 'allowed_file_types', '');
expect(await new AllowedFileTypesService(new DatabaseService(testDb)).get()).toBe(DEFAULT_ALLOWED_EXTENSIONS);
expect(await new AllowedFileTypesService(await createTestAppSettingsRepo(testDb)).get()).toBe(DEFAULT_ALLOWED_EXTENSIONS);
});
it('FILE-SVC-009: returns the default when the query throws (no app_settings table)', async () => {
expect(await new AllowedFileTypesService(new DatabaseService(bareDb)).get()).toBe(DEFAULT_ALLOWED_EXTENSIONS);
expect(await new AllowedFileTypesService(await createTestAppSettingsRepo(bareDb)).get()).toBe(DEFAULT_ALLOWED_EXTENSIONS);
});
});
@@ -359,6 +391,15 @@ describe('createFile', () => {
expect(file.file_size).toBe(99);
expect(file.uploaded_by).toBe(user.id);
});
it('FILE-SVC-060 (R2): a failing file_links insert rolls back the trip_files row too', async () => {
const { user, trip } = seedTrip();
const item = Number(testDb.prepare('INSERT INTO budget_items (trip_id, name) VALUES (?, ?)').run(trip.id, 'Dinner').lastInsertRowid);
const spy = vi.spyOn(fileLinksRepo, 'insertIgnore').mockRejectedValueOnce(new Error('boom'));
await expect(makeFile(trip.id, user.id, {}, { budget_item_id: item })).rejects.toThrow('boom');
expect(testDb.prepare('SELECT COUNT(*) c FROM trip_files WHERE trip_id = ?').get(trip.id)).toEqual({ c: 0 });
spy.mockRestore();
});
});
describe('updateFile', () => {
@@ -383,6 +424,20 @@ describe('updateFile', () => {
expect(updated.place_id).toBeNull();
expect(updated.reservation_id).toBeNull();
});
it('FILE-SVC-061 (R2): a failing file_links swap rolls back the description/place/reservation update too', async () => {
const { user, trip } = seedTrip();
const item = Number(testDb.prepare('INSERT INTO budget_items (trip_id, name) VALUES (?, ?)').run(trip.id, 'Dinner').lastInsertRowid);
const place = createPlace(testDb, trip.id);
const file = await makeFile(trip.id, user.id, {}, { description: 'old' });
const current = (await svc.getFileById(file.id, trip.id))!;
const spy = vi.spyOn(fileLinksRepo, 'insertIgnore').mockRejectedValueOnce(new Error('boom'));
await expect(svc.updateFile(file.id, current, { description: 'new', place_id: String(place.id), budget_item_id: item })).rejects.toThrow('boom');
const row = testDb.prepare('SELECT description, place_id FROM trip_files WHERE id = ?').get(file.id) as Record<string, unknown>;
expect(row.description).toBe('old');
expect(row.place_id).toBeNull();
spy.mockRestore();
});
});
describe('toggleStarred / softDeleteFile / restoreFile', () => {
@@ -420,7 +475,7 @@ describe('permanentDeleteFile', () => {
const { user, trip } = seedTrip();
const file = await makeFile(trip.id, user.id, { filename: 'on-disk.pdf' });
storageDelete.mockResolvedValue(undefined);
await svc.permanentDeleteFile(await svc.getFileById(file.id, trip.id) as TripFile);
await svc.permanentDeleteFile((await svc.getFileById(file.id, trip.id))!);
expect(storageDelete).toHaveBeenCalledWith('files', 'on-disk.pdf');
expect(await svc.getFileById(file.id, trip.id)).toBeUndefined();
});
@@ -431,7 +486,7 @@ describe('permanentDeleteFile', () => {
const boom = new Error('EACCES');
storageDelete.mockRejectedValue(boom);
const err = vi.spyOn(console, 'error').mockImplementation(() => {});
await expect(svc.permanentDeleteFile(await svc.getFileById(file.id, trip.id) as TripFile)).rejects.toThrow('EACCES');
await expect(svc.permanentDeleteFile((await svc.getFileById(file.id, trip.id))!)).rejects.toThrow('EACCES');
expect(err).toHaveBeenCalledWith('[files] unlink failed for stuck.pdf, keeping DB row:', boom);
expect(await svc.getFileById(file.id, trip.id)).toBeDefined();
});
@@ -475,6 +530,17 @@ describe('emptyTrash', () => {
await expect(svc.emptyTrash(trip.id)).resolves.toBe(0);
expect(storageDelete).not.toHaveBeenCalled();
});
it('FILE-SVC-062 (R2): a failing bulk delete leaves every trashed row in place', async () => {
const { user, trip } = seedTrip();
const a = await makeFile(trip.id, user.id, { filename: 'a.pdf' });
await svc.softDeleteFile(a.id);
storageDelete.mockResolvedValue(undefined);
const spy = vi.spyOn(tripFilesRepo, 'deleteMany').mockRejectedValueOnce(new Error('boom'));
await expect(svc.emptyTrash(trip.id)).rejects.toThrow('boom');
expect(testDb.prepare('SELECT COUNT(*) c FROM trip_files WHERE id = ?').get(a.id)).toEqual({ c: 1 });
spy.mockRestore();
});
});
// ── findForeignLinkTarget ─────────────────────────────────────────────────────
@@ -552,9 +618,22 @@ describe('createFileLink / deleteFileLink / getFileLinks', () => {
});
it('FILE-SVC-030: an insert error propagates (post-migration fix: no silent swallow)', async () => {
// Plan 3e Task 1 deviation: the legacy raw statement bound whatever JS
// value the caller passed straight into the driver, so a non-primitive
// (an object) reached better-sqlite3's bind and threw there. The write
// is now typed `number | null` end to end (`FileLinksRepository
// .insertIgnore`), so a value that shape can never reach the repository
// at all — the `toRowId`-backed `coerceLinkId` narrows it to `null`
// (no link) instead, the same accepted narrowing every other
// non-canonical id gets. What the "no silent swallow" fix still means is
// tested directly against the repository call: a genuine insert failure
// (any error, not a type mismatch this layer now prevents) propagates
// rather than being caught and turned into a success-shaped links list.
const { user, trip } = seedTrip();
const file = await makeFile(trip.id, user.id);
await expect(svc.createFileLink(file.id, { reservation_id: { bad: true } as never })).rejects.toThrow();
const spy = vi.spyOn(fileLinksRepo, 'insertIgnore').mockRejectedValueOnce(new Error('boom'));
await expect(svc.createFileLink(file.id, { reservation_id: 1 })).rejects.toThrow('boom');
spy.mockRestore();
});
it('FILE-SVC-031: deleteFileLink is scoped to (id AND file_id)', async () => {
@@ -571,6 +650,86 @@ describe('createFileLink / deleteFileLink / getFileLinks', () => {
});
});
// ── Plan 3e Task 1 — full-key parity on the fully-seeded read models ──────────
// Rule 19: one `toEqual(<legacy statement run raw on the same seeded rows>)`
// per converted read model, on rows with every optional column populated
// (incl. all six `trip_files` `persist(false)` mirrors) and both trashed and
// live rows.
describe('read-model parity: raw SQL vs. the repository-backed reads', () => {
it('FILE-SVC-070: getFileById (FL5) is byte-identical to `SELECT * FROM trip_files WHERE id = ? AND trip_id = ?`, all six persist(false) mirrors set', async () => {
const { user, trip } = seedTrip();
const reservation = createReservation(testDb, trip.id);
const place = createPlace(testDb, trip.id);
const note = createCollabNote(testDb, trip.id, user.id);
const messageId = Number(
testDb.prepare('INSERT INTO collab_messages (trip_id, user_id, text) VALUES (?, ?, ?)').run(trip.id, user.id, 'hi').lastInsertRowid,
);
// Raw insert, not svc.createFile: note_id/message_id are set by OTHER
// domains (collab), never by FilesService itself — this seeds every
// persist(false) mirror at once to prove the Kysely `selectAll()` read
// carries all six, the trap the class docstring names.
const inserted = testDb.prepare(`
INSERT INTO trip_files (trip_id, place_id, reservation_id, filename, original_name, file_size, mime_type, description, note_id, uploaded_by, starred, message_id)
VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
`).run(trip.id, place.id, reservation.id, 'a.pdf', 'A.pdf', 10, 'application/pdf', 'a note', note.id, user.id, 1, messageId);
const id = Number(inserted.lastInsertRowid);
const legacy = testDb.prepare('SELECT * FROM trip_files WHERE id = ? AND trip_id = ?').get(id, trip.id);
expect(await svc.getFileById(id, trip.id)).toEqual(legacy);
});
it('FILE-SVC-071: listFiles (FL7, FILE_SELECT) is byte-identical to the legacy joined SELECT, live and trashed alike, with reservation+place+budget_item links all present', async () => {
const { user, trip } = seedTrip();
const reservation = createReservation(testDb, trip.id, { title: 'Night train' });
const place = createPlace(testDb, trip.id);
const item = Number(testDb.prepare('INSERT INTO budget_items (trip_id, name) VALUES (?, ?)').run(trip.id, 'Dinner').lastInsertRowid);
// `linked_*_ids` come from `file_links` rows (FL8), a SEPARATE mechanism
// from `trip_files.reservation_id`/`place_id` (FILE_SELECT's own join) —
// one `createFileLink` call can attach several targets to the same file
// at once (the legacy statement writes all four columns in one INSERT).
const live = await makeFile(trip.id, user.id);
await svc.createFileLink(live.id, { reservation_id: String(reservation.id), place_id: String(place.id), budget_item_id: item });
const trashed = await makeFile(trip.id, user.id, { filename: 'gone.pdf' });
await svc.softDeleteFile(trashed.id);
const FILE_SELECT = `
SELECT f.*, r.title as reservation_title, u.username as uploaded_by_name, u.avatar as uploaded_by_avatar
FROM trip_files f
LEFT JOIN reservations r ON f.reservation_id = r.id
LEFT JOIN users u ON f.uploaded_by = u.id
`;
const legacyLive = testDb.prepare(`${FILE_SELECT} WHERE f.id = ?`).get(live.id) as Record<string, unknown>;
const legacyTrashed = testDb.prepare(`${FILE_SELECT} WHERE f.id = ?`).get(trashed.id) as Record<string, unknown>;
const activeRow = (await svc.listFiles(trip.id, false) as Record<string, unknown>[]).find((f) => f.id === live.id)!;
const pickedActive = Object.fromEntries(Object.keys(legacyLive).map((k) => [k, activeRow[k]]));
expect(pickedActive).toEqual(legacyLive);
expect(activeRow.linked_reservation_ids).toEqual([reservation.id]);
expect(activeRow.linked_place_ids).toEqual([place.id]);
expect(activeRow.linked_budget_item_ids).toEqual([item]);
const trashedRow = (await svc.listFiles(trip.id, true) as Record<string, unknown>[]).find((f) => f.id === trashed.id)!;
const pickedTrashed = Object.fromEntries(Object.keys(legacyTrashed).map((k) => [k, trashedRow[k]]));
expect(pickedTrashed).toEqual(legacyTrashed);
});
it('FILE-SVC-072: getFileLinks (FL27) is byte-identical to the legacy joined SELECT', async () => {
const { user, trip } = seedTrip();
const reservation = createReservation(testDb, trip.id, { title: 'Ferry' });
const file = await makeFile(trip.id, user.id);
await svc.createFileLink(file.id, { reservation_id: String(reservation.id) });
const legacy = testDb.prepare(`
SELECT fl.*, r.title as reservation_title
FROM file_links fl
LEFT JOIN reservations r ON fl.reservation_id = r.id
WHERE fl.file_id = ?
`).all(file.id);
expect(await svc.getFileLinks(file.id)).toEqual(legacy);
});
});
// ── download auth & glue ──────────────────────────────────────────────────────
describe('authenticateDownload', () => {
@@ -648,7 +807,7 @@ describe('verifyTripAccess / can / files.bridge', () => {
// list moved to a leaf service the multer factories inject, and the property
// that matters is unchanged: an admin editing the list in settings applies
// to the next upload, with no invalidation wiring.
const allowed = new AllowedFileTypesService(new DatabaseService(testDb));
const allowed = new AllowedFileTypesService(await createTestAppSettingsRepo(testDb));
setAppSetting(testDb, 'allowed_file_types', 'md,markdown');
expect(await allowed.get()).toBe('md,markdown');
});