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.
This commit is contained in:
Maurice
2026-10-08 12:52:00 +02:00
parent 5a5349b817
commit a198dde5d2
2 changed files with 72 additions and 10 deletions
+1 -1
View File
@@ -49,7 +49,7 @@ Nest owns everything. Every domain is a DI module under `src/nest/<domain>/` (`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). - **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. - **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. - **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<string, unknown>` + ad-hoc checks. Also verify every referenced id exists and belongs to the same trip. - **Every write endpoint validates** through the Zod pipe with a schema from `@trek/shared` — no `Record<string, unknown>` + 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). - **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. - **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.
@@ -29,6 +29,7 @@ interface ForeignKeyRow {
interface IndexListRow { interface IndexListRow {
name: string; name: string;
partial: number;
} }
interface IndexInfoRow { 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)); 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. */ /** Every index of a table, and its primary key, as ordered column lists. */
function indexedPrefixes(db: Database.Database, table: string): string[][] { function indexedPrefixes(db: Database.Database, table: string): IndexPrefix[] {
const lists: string[][] = []; const lists: IndexPrefix[] = [];
for (const { name } of db.prepare(`PRAGMA index_list("${table}")`).all() as IndexListRow[]) { 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[]) const cols = (db.prepare(`PRAGMA index_info("${name}")`).all() as IndexInfoRow[])
.sort((a, b) => a.seqno - b.seqno) .sort((a, b) => a.seqno - b.seqno)
// An expression column has no name and ends the usable prefix. // 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; if (c === null) break;
usable.push(c); usable.push(c);
} }
lists.push(usable); lists.push({ cols: usable, notNull });
} }
const pk = (db.prepare(`PRAGMA table_info("${table}")`).all() as TableInfoRow[]) const pk = (db.prepare(`PRAGMA table_info("${table}")`).all() as TableInfoRow[])
.filter((c) => c.pk > 0) .filter((c) => c.pk > 0)
.sort((a, b) => a.pk - b.pk) .sort((a, b) => a.pk - b.pk)
.map((c) => c.name); .map((c) => c.name);
if (pk.length) lists.push(pk); if (pk.length) lists.push({ cols: pk, notNull: [] });
return lists; 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(','); 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[] { 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)'); db.exec('CREATE INDEX idx_child_note_parent ON child(note, parent_id)');
expect(unindexed(db)).toContain('child.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)'); db.exec('CREATE INDEX idx_pair_yx ON pair(y, x, z)');
expect(unindexed(db)).toEqual([]); expect(unindexed(db)).toEqual([]);
} finally { } finally {
db.close(); 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();
});
}); });