diff --git a/server/CLAUDE.md b/server/CLAUDE.md index f7198bd82..305c7adeb 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -46,7 +46,7 @@ Nest owns everything. Every domain is a DI module under `src/nest//` (`c - **Thin controller → injectable provider.** Never resurrect a plain function-module service; don't grow god services — split a domain into several modules when it outgrows one class (trips and auth already are). - **Inject, don't reach for module globals.** No imports of the global `db` proxy from `nest/` layers at all any more — the last production importer converted onto a repository, and the export now survives only so ~190 test files' `vi.mock('…/db/database', …)` factories keep typechecking; no new code depending on the `ws` singleton or event-sink globals. There is no `DatabaseService` to inject either — take the repository(ies) you need plus `UnitOfWork` for transactions. Modules must be importable without side effects. - **Transactions are not optional.** Multi-statement writes go in `await this.uow.transactional(async () => …)` (`UnitOfWork` from `nest/database`, provided by the global `OrmModule`); never hand-roll `BEGIN`/`COMMIT` and never call `db.transaction()` (the Phase 1 async sweep removes it). Every DB-touching method is `async`; `@typescript-eslint/no-floating-promises` and `no-misused-promises` are errors, so an unawaited call or a `Promise` in a condition fails lint. Bind values with `?`; identifiers only via a literal allow-list. -- **Migrations are append-only.** A migration is identified by its class name in `mikro_orm_migrations`, and files sort by the timestamp in that name — never rename or delete one that has shipped, and never edit its body; add a new one. No env interpolation in SQL. (The old positional `schema_version` array and the hand-kept `db/migrations.ts` test builder are both gone; the numbered migrations are the only schema history, for tests too.) +- **Migrations are append-only.** A migration is identified by its class name in `mikro_orm_migrations`, and files sort by the timestamp in that name — never rename or delete one that has shipped, and never edit its body; add a new one. No env interpolation in SQL. (The old positional `schema_version` array and the hand-kept `db/migrations.ts` test builder are both gone; the numbered migrations are the only schema history, for tests too.) A new foreign key comes with an index that leads with its column(s), or deleting the parent scans the child table; `tests/unit/db/foreign-key-indexes.test.ts` fails on one without. - **Every write endpoint validates** through the Zod pipe with a schema from `@trek/shared` — no `Record` + ad-hoc checks. Also verify every referenced id exists and belongs to the same trip. - **Every outbound `fetch`** gets a timeout, a response-size cap and boundary validation — no `as`-casting provider responses, no silent `catch → null`. User-influenced URLs go through the SSRF guard. Never fall back to another user's API key. - **Fail closed.** Unrecognized flag values mean OFF; sandbox/permission opt-outs refuse in production; never pick a less-safe path based on the *absence* of a file. GET handlers must not write. diff --git a/server/tests/unit/db/foreign-key-indexes.test.ts b/server/tests/unit/db/foreign-key-indexes.test.ts new file mode 100644 index 000000000..1aff9e4ef --- /dev/null +++ b/server/tests/unit/db/foreign-key-indexes.test.ts @@ -0,0 +1,139 @@ +import type Database from 'better-sqlite3'; +import { afterAll, describe, expect, it } from 'vitest'; +import { createSnapshotTestDb } from '../../helpers/db-mock'; + +/** + * Migration20200101042200 added 86 indexes by hand so that deleting a trip or a + * user stops scanning every child table while it holds the one connection. A + * hand-written list is a one-off, though: the next table with a trip_id or a + * user_id foreign key would bring the full scans back without a signal, and + * gen:entities regenerates entities without an @Index just as happily. + * + * This runs against the database the migrations build (the same snapshot every + * test opens) and requires that the columns of every foreign key lead some + * index or the primary key, in order, so SQLite can find the child rows of a + * parent by seeking instead of scanning. A foreign key that deliberately goes + * without one is listed in UNINDEXED_ALLOWED with the reason. + */ + +/** `table.column` (comma-joined for a composite key) that may stay unindexed, each with its reason. */ +const UNINDEXED_ALLOWED: Record = {}; + +interface ForeignKeyRow { + id: number; + seq: number; + table: string; + from: string; +} + +interface IndexListRow { + name: string; +} + +interface IndexInfoRow { + seqno: number; + name: string | null; +} + +interface TableInfoRow { + name: string; + pk: number; +} + +const testDb = createSnapshotTestDb(); + +afterAll(() => { + testDb.close(); +}); + +function tables(db: Database.Database): string[] { + const rows = db.prepare(`SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%'`).all() as { + name: string; + }[]; + return rows.map((r) => r.name).sort(); +} + +/** Each foreign key of a table as its ordered child columns. */ +function foreignKeys(db: Database.Database, table: string): string[][] { + const rows = db.prepare(`PRAGMA foreign_key_list("${table}")`).all() as ForeignKeyRow[]; + const byId = new Map(); + for (const row of rows) byId.set(row.id, [...(byId.get(row.id) ?? []), row]); + return [...byId.values()].map((group) => group.sort((a, b) => a.seq - b.seq).map((r) => r.from)); +} + +/** Every index of a table, and its primary key, as ordered column lists. */ +function indexedPrefixes(db: Database.Database, table: string): string[][] { + const lists: string[][] = []; + for (const { name } of db.prepare(`PRAGMA index_list("${table}")`).all() as IndexListRow[]) { + const cols = (db.prepare(`PRAGMA index_info("${name}")`).all() as IndexInfoRow[]) + .sort((a, b) => a.seqno - b.seqno) + // An expression column has no name and ends the usable prefix. + .map((c) => c.name); + const usable: string[] = []; + for (const c of cols) { + if (c === null) break; + usable.push(c); + } + lists.push(usable); + } + const pk = (db.prepare(`PRAGMA table_info("${table}")`).all() as TableInfoRow[]) + .filter((c) => c.pk > 0) + .sort((a, b) => a.pk - b.pk) + .map((c) => c.name); + if (pk.length) lists.push(pk); + return lists; +} + +/** True when the foreign key's columns, in any order, are the leading columns of one index. */ +function isLedByIndex(fk: string[], indexes: string[][]): boolean { + const want = [...fk].sort().join(','); + return indexes.some((cols) => cols.length >= fk.length && [...cols.slice(0, fk.length)].sort().join(',') === want); +} + +function unindexed(db: Database.Database): string[] { + const missing: string[] = []; + for (const table of tables(db)) { + const indexes = indexedPrefixes(db, table); + for (const fk of foreignKeys(db, table)) { + if (!isLedByIndex(fk, indexes)) missing.push(`${table}.${fk.join(',')}`); + } + } + return missing; +} + +describe('foreign key indexes', () => { + it('FKIDX-001: every foreign key in the migrated schema leads an index or the primary key', () => { + const missing = unindexed(testDb).filter((key) => !(key in UNINDEXED_ALLOWED)); + expect(missing, 'add a CREATE INDEX for each in a new migration, and an @Index on the entity').toEqual([]); + }); + + it('FKIDX-002: UNINDEXED_ALLOWED holds only keys that exist and are still unindexed', () => { + const now = new Set(unindexed(testDb)); + expect(Object.keys(UNINDEXED_ALLOWED).filter((key) => !now.has(key))).toEqual([]); + }); + + it('FKIDX-003: the check fails on an unindexed foreign key and passes once it is indexed', () => { + const Sqlite = testDb.constructor as unknown as new (path: string) => Database.Database; + const db = new Sqlite(':memory:'); + try { + db.exec(` + CREATE TABLE parent (id INTEGER PRIMARY KEY, a INTEGER, b INTEGER, UNIQUE (a, b)); + CREATE TABLE child (id INTEGER PRIMARY KEY, parent_id INTEGER REFERENCES parent(id), note TEXT); + CREATE TABLE pair (x INTEGER, y INTEGER, z INTEGER, FOREIGN KEY (x, y) REFERENCES parent(a, b)); + CREATE TABLE owned (parent_id INTEGER REFERENCES parent(id), slot INTEGER, PRIMARY KEY (parent_id, slot)); + `); + // owned is covered by its primary key; child and pair are not. + expect(unindexed(db)).toEqual(['child.parent_id', 'pair.x,y']); + + // An index that holds the column second does not help a lookup by it. + db.exec('CREATE INDEX idx_child_note_parent ON child(note, parent_id)'); + expect(unindexed(db)).toContain('child.parent_id'); + + db.exec('CREATE INDEX idx_child_parent ON child(parent_id)'); + db.exec('CREATE INDEX idx_pair_yx ON pair(y, x, z)'); + expect(unindexed(db)).toEqual([]); + } finally { + db.close(); + } + }); +});