From a198dde5d20f5b67613ea8bd090fcbb3941dff88 Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 12:52:00 +0200 Subject: [PATCH] test(server): stop counting a filtered partial index as covering a foreign key The foreign key index check took every index from PRAGMA index_list as a usable prefix, partial ones included. An index like CREATE INDEX ix ON c(p_id) WHERE state = 'open' would satisfy FKIDX-001, yet the cascade lookup WHERE p_id = ? still scans the table, which is the regression the check exists to catch. A partial index now counts only when its predicate is IS NOT NULL on the key's own columns, the one form SQLite can prove from p_id = ?. Nothing in today's schema depends on a partial index alone, so the result on the migrated snapshot is unchanged. --- server/CLAUDE.md | 2 +- .../tests/unit/db/foreign-key-indexes.test.ts | 80 ++++++++++++++++--- 2 files changed, 72 insertions(+), 10 deletions(-) diff --git a/server/CLAUDE.md b/server/CLAUDE.md index 83531e336..a386a7201 100644 --- a/server/CLAUDE.md +++ b/server/CLAUDE.md @@ -49,7 +49,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. In `src/db/repositories/` and `src/db/seeders/`, ESLint refuses raw SQL in any spelling: `raw()`, the `sql` tag, and SQL text handed to `execute`/`run`/`all`/`get`. Build the statement with the query builder; `MaintenanceRepository`, `DemoRepository` and the six existing seeders are the named exceptions in `eslint.config.mjs`, and `tests/unit/eslint-rules/restricted-syntax-selectors.test.ts` pins how many SQL strings each still holds, so they can only lose SQL, never gain it. -- **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. +- **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 (a partial index counts only when its WHERE clause is `IS NOT NULL` on those columns); `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. ESLint requires an init with a `signal` on `fetch`, `safeFetch`, `safeFetchFollow` and `safeFetchAdminConfigured` alike, or a literal `timeoutMs` in the options object of `safeFetch`/`safeFetchFollow`; an init spelled `undefined`, `null` or `void 0` counts as none (`safeFetchLlm` takes `LLM_TIMEOUT_MS` inside). Behind that, the guarded wrappers (`utils/ssrfGuard.ts`) cap every body at `DEFAULT_MAX_RESPONSE_BYTES` (64 MB) and, for a call without a signal, wait `DEFAULT_RESPONSE_TIMEOUT_MS` (30 s) for the headers. Pass a tighter `maxBytes` when you buffer a small answer, `timeoutMs` for a deadline over the whole exchange, and `maxBytes: null` only where the body is streamed on or counted by the caller (`pipeAsset`, the doc-sync transfers, the Ollama pull). - **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 index 309655279..a17fe491d 100644 --- a/server/tests/unit/db/foreign-key-indexes.test.ts +++ b/server/tests/unit/db/foreign-key-indexes.test.ts @@ -29,6 +29,7 @@ interface ForeignKeyRow { interface IndexListRow { name: string; + partial: number; } interface IndexInfoRow { @@ -62,10 +63,43 @@ function foreignKeys(db: Database.Database, table: string): string[][] { return [...byId.values()].map((group) => group.sort((a, b) => a.seq - b.seq).map((r) => r.from)); } +/** The leading columns of an index, and the columns its WHERE clause requires to be non-null. */ +interface IndexPrefix { + cols: string[]; + notNull: string[]; +} + +/** + * The columns a partial index's predicate requires to be non-null, or null when + * the predicate says anything else. SQLite proves `col = ?` implies `col IS NOT + * NULL`, so only such an index serves the parent's cascade lookup; one filtered + * on another predicate (`WHERE state = 'open'`) leaves the lookup scanning. + */ +function notNullPredicate(sql: string | null): string[] | null { + const where = /\bWHERE\b([\s\S]*)$/i.exec(sql ?? ''); + if (!where) return null; + const cols: string[] = []; + for (const term of where[1].trim().split(/\s+AND\s+/i)) { + const m = /^\(?\s*["`[]?(\w+)["`\]]?\s+IS\s+NOT\s+NULL\s*\)?$/i.exec(term.trim()); + if (!m) return null; + cols.push(m[1]); + } + return cols; +} + /** 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[]) { +function indexedPrefixes(db: Database.Database, table: string): IndexPrefix[] { + const lists: IndexPrefix[] = []; + for (const { name, partial } of db.prepare(`PRAGMA index_list("${table}")`).all() as IndexListRow[]) { + let notNull: string[] = []; + if (partial) { + const row = db.prepare(`SELECT sql FROM sqlite_master WHERE type = 'index' AND name = ?`).get(name) as + { sql: string | null } | undefined; + const required = notNullPredicate(row?.sql ?? null); + // A predicate other than IS NOT NULL on key columns hides rows from the lookup. + if (required === null) continue; + notNull = required; + } 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. @@ -75,20 +109,28 @@ function indexedPrefixes(db: Database.Database, table: string): string[][] { if (c === null) break; usable.push(c); } - lists.push(usable); + lists.push({ cols: usable, notNull }); } 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); + if (pk.length) lists.push({ cols: pk, notNull: [] }); 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 { +/** + * True when the foreign key's columns, in any order, are the leading columns of + * one index, and a partial index filters only on those columns being non-null. + */ +function isLedByIndex(fk: string[], indexes: IndexPrefix[]): boolean { const want = [...fk].sort().join(','); - return indexes.some((cols) => cols.length >= fk.length && [...cols.slice(0, fk.length)].sort().join(',') === want); + return indexes.some( + ({ cols, notNull }) => + cols.length >= fk.length && + [...cols.slice(0, fk.length)].sort().join(',') === want && + notNull.every((c) => fk.includes(c)), + ); } function unindexed(db: Database.Database): string[] { @@ -130,11 +172,31 @@ describe('foreign key indexes', () => { 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)'); + // A partial index on another predicate leaves the cascade lookup scanning. + db.exec(`CREATE INDEX idx_child_parent_open ON child(parent_id) WHERE note = 'open'`); + expect(db.prepare('EXPLAIN QUERY PLAN SELECT 1 FROM child WHERE parent_id = ?').get(1)).toMatchObject({ + detail: 'SCAN child', + }); + expect(unindexed(db)).toContain('child.parent_id'); + + // One filtered only on the key being non-null still serves `parent_id = ?`. + db.exec('CREATE INDEX idx_child_parent ON child(parent_id) WHERE parent_id IS NOT NULL'); + expect( + (db.prepare('EXPLAIN QUERY PLAN SELECT 1 FROM child WHERE parent_id = ?').get(1) as { detail: string }).detail, + ).toContain('idx_child_parent'); db.exec('CREATE INDEX idx_pair_yx ON pair(y, x, z)'); expect(unindexed(db)).toEqual([]); } finally { db.close(); } }); + + it('FKIDX-004: a partial index counts only when its predicate is IS NOT NULL on its columns', () => { + expect(notNullPredicate('CREATE INDEX i ON c(p_id) WHERE p_id IS NOT NULL')).toEqual(['p_id']); + expect(notNullPredicate('CREATE INDEX i ON c(a, b) WHERE "a" IS NOT NULL AND (b IS NOT NULL)')).toEqual(['a', 'b']); + expect(notNullPredicate("CREATE INDEX i ON c(p_id) WHERE state = 'open'")).toBeNull(); + expect(notNullPredicate('CREATE INDEX i ON c(p_id) WHERE p_id IS NOT NULL OR x = 1')).toBeNull(); + expect(notNullPredicate('CREATE INDEX i ON c(p_id) WHERE p_id IS NULL')).toBeNull(); + expect(notNullPredicate(null)).toBeNull(); + }); });