From 12e1616392dc54a936157c063b62fce36e61c865 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 16:42:52 -0500 Subject: [PATCH 1/5] docs(db): design converting the two dynamic queries to Kysely (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The work #305 made possible and deliberately did not do. These are the two queries the builder was ever wanted for: admin.ts and items.ts both splice a run-time-composed where clause into query text, and they are the only places S2077 has a real point after #294 hoisted the seven fixed-shape queries into named constants. They are safe today, and itemFilters.ts spells out why in sixteen lines — which is the problem, because a property that takes sixteen lines to explain is one an edit can quietly break. All four call sites convert rather than only the two flagged ones. The by-id constants carry no hotspot and are already safe, but they are built by interpolating the same projection strings the list queries use, so converting only the list queries would leave itemSelect.ts holding a Kysely builder and a raw string that must produce an identical projection — two spellings to keep in step by hand where the file's own header already warns about one. The filter builder returns an array of expressions rather than taking a query builder and returning it filtered, because the two callers do different things with the result: the storefront prepends its own not-pending clause and the admin route does not. A function that owned the builder would have to be told about that difference. startIndex disappears with the splicing it existed for. The aggregate subqueries become jsonArrayFrom, which emits the same coalesce(json_agg(agg), '[]') they hand-write today. That is the second thing this buys and it may matter more than the first: pool.query asserts a shape TypeScript never checks against the SQL, which is why itemSelect.ts's header says the selects and their types are kept in step by hand and the integration suite is the only thing that catches a drop. Afterwards that is a compile error. The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused on purpose: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to. The two invariant tests survive and get stronger. They currently inspect the clause strings the function returns; afterwards they compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim tested against the real artefact instead of an intermediate one. Co-Authored-By: Claude Opus 5 --- ...026-09-04-dynamic-queries-kysely-design.md | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md diff --git a/docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md b/docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md new file mode 100644 index 0000000..d62cf51 --- /dev/null +++ b/docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md @@ -0,0 +1,91 @@ +# Converting the two dynamic queries to Kysely + +**Issue:** #308. The work #305 made possible and deliberately did not do. + +#294 removed interpolation from seven query sites by making each fixed-shape query a named constant. Two were left, and they are the ones the whole exercise was ever about: + +- `backend/src/routes/admin.ts:149` — `` `${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC` `` +- `backend/src/routes/items.ts:95` — `` `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC` `` + +`where` is genuinely composed at run time by `buildItemFilterSql`. Both are safe today, and `itemFilters.ts:242-258` spells out why: the clause fragments are string literals, the only things interpolated into them are placeholder *indices*, and every value goes onto `params`. That argument is correct. It is also an argument — a thing a reader must follow and an edit can quietly break, guarded by a comment and two tests. This turns it into a property of the type system, which is what #202 asked for and why #180's hotspots have sat Reviewed rather than closed. + +## Decisions, and what each one rests on + +**All four call sites convert, not just the two flagged ones.** `ADMIN_ITEM_BY_ID` and `PUBLIC_ITEM_BY_ID` carry no S2077 and are already safe named constants. But they are built by interpolating the same projection strings the list queries use, so converting only the list queries would leave `itemSelect.ts` holding a Kysely builder *and* a raw string that must produce the identical projection. That is a worse version of the hazard the file's own header warns about — "KEPT IN STEP BY HAND … Change a select and its type together" — because now there would be two spellings to keep in step instead of one. Four sites total: `admin.ts:149`, `admin.ts:178`, `admin.ts:232`, `items.ts:95`, `items.ts:105`. + +**The filter builder returns Kysely expressions, not text.** `buildItemFilterSql(filters, startIndex, favoritesCustomerId): { clauses: string[]; params: unknown[] }` becomes a function returning an array of `Expression`, which the caller hands to `eb.and(...)`. An array rather than "take a query builder and return it filtered", because the two callers do different things with the result: the storefront prepends its own `status <> 'pending'` and the admin route does not. A function that owned the query builder would have to be told about that difference; a function that returns expressions does not care. + +`startIndex` disappears. It existed only so a caller could splice fragments in after its own parameters, and nothing splices any more. + +**The aggregate subqueries become `jsonArrayFrom`.** Kysely's Postgres helper emits `(select coalesce(json_agg(agg), '[]') from … as agg)` — the same shape `IMAGES_SUBQUERY` and `TAGS_SUBQUERY` hand-write today, including the `'[]'` fallback. Using the helper rather than keeping the SQL as a literal fragment is what makes the result *typed*: the subquery's columns are known, so the row type follows from the select instead of being asserted alongside it. + +That is the second thing this change buys, and it may matter more than the first. `pool.query` asserts a shape TypeScript never checks against the SQL — the file's header says so plainly, and says the integration suite is the only thing that catches a drop. After this, dropping a column from a select and not its type is a compile error. + +**The row-type contract is kept, not inferred away.** `AdminItemRow` and `PublicItemRow` stay exported and stay hand-written, and the queries are assigned to them. Inferring them from the query instead would be tidier and is deliberately not done: they are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. Assignment gives the compile error; inference would remove the thing being checked. + +If exact inference proves fussy — `json_agg` of a timestamp comes back as a string, not a `Date`, and the aggregate helpers' types are precise about it — the fallback is an explicit `$castTo()` at the end of the builder, which is still a single named assertion in one place rather than one per call site. Prefer plain assignment; reach for `$castTo` only where the types genuinely disagree, and say which column disagreed. + +**`i` and `c` stay as aliases.** `selectFrom('items as i').leftJoin('categories as c', 'c.id', 'i.category_id')`. Not because the short names are better, but because every filter clause, every subquery correlation and the `ORDER BY` already reference them, and renaming them in the same change that moves the builder would make the diff impossible to read against the SQL it replaces. + +**`ADMIN_IMAGES_SUBQUERY` stays separate from the public one.** It carries `original_image_path` and the storefront must never see it (#293). The two selects call different helpers; folding them into one parameterised helper would put a boolean in the middle of the thing that keeps an internal filename off the public API. + +## Architecture + +``` +itemSelect.ts adminItemQuery() → SelectQueryBuilder, admin projection + publicItemQuery() → SelectQueryBuilder, public projection + │ (both: items i ⟕ categories c, + │ images + tags via jsonArrayFrom) + ▼ +itemFilters.ts itemFilterExpressions(eb, filters, favoritesCustomerId) + │ → Expression[] + ▼ +routes/admin.ts adminItemQuery().where(eb => eb.and(itemFilterExpressions(...))) +routes/items.ts publicItemQuery().where(eb => eb.and([notPending(eb), ...itemFilterExpressions(...)])) +``` + +### The six clauses + +Each keeps its existing comment, because each records a decision rather than describing the code: + +| Clause | Becomes | +|---|---| +| category subtree | `sql` template, recursive CTE unchanged, `${ids}` now one bind parameter | +| tags AND-match | `sql` template, count equality unchanged | +| min / max price | `eb('i.price_cents', '>=' / '<=', value)` | +| status | `eb('i.status', 'in', statuses)` | +| favorites | `eb.exists(...)` on `favorites`, correlated with `whereRef` | + +The two that stay `sql` templates stay for the reason `adminCategories.ts` gives for its own CTE: they are recursive or aggregate fragments that the builder expresses no better, and their values are already bind parameters. The four that become builder expressions do so because they are plain column comparisons and there is no reason for them not to be. + +**`ANY($n::int[])` becomes `= ANY(${ids}::int[])` inside the template, and that is one parameter rather than a placeholder list** — the property #297 verified against emitted SQL, and the reason no `sql.param()` ceremony appears anywhere here. + +### `favoritesCustomerId` keeps its throw + +`favoritesOnly` with a null customer id still throws rather than returning no clause. Both routes already refuse that combination before calling, so reaching it is a programming error — and the alternative, quietly dropping the filter, lists the whole catalogue to someone who asked for their favourites. + +## Failure handling + +| What happens | Result | +|---|---| +| No filters at all | `eb.and([])` — Kysely emits no `where`, matching today's `clauses.length ? … : ''`. The storefront still has its not-pending clause, so its `and` is never empty. | +| A filter value that is not an integer | Unchanged: `parseItemFilters` rejects it before this, with the same 400. | +| `favoritesOnly` with no customer | Throws, as today. Both routes guard it first. | +| A column renamed in a migration | Compile error, where today it is a run-time `undefined` that only the integration suite catches. | + +## Testing + +- **Unit:** `itemFilters`' existing tests assert on `clauses` and `params`, which no longer exist. They are rewritten to compile each expression and assert the emitted SQL and parameters — which is a stronger assertion than counting fragments, and is how #297 established the array behaviour in the first place. The parser tests over `parseItemFilters` are untouched; that function does not change. +- **Integration:** every existing filter test must pass unchanged. They are the contract — same JSON, same ordering, same statuses. Nothing in them should need editing, and an edit to one is a signal the conversion changed behaviour. +- **The two invariant tests must be re-pointed, not deleted.** `tests/unit/itemFilters.test.ts` ends with a describe block, `buildItemFilterSql keeps every value out of the SQL text`, holding exactly two: one that pushes `"1); DROP TABLE items; --"` through every filter field — built by hand, deliberately bypassing the parser, because the claim is that the fragments are safe with no parser at all — and one asserting that two completely different filter sets produce byte-identical SQL. + +Both survive the conversion and get stronger. Today they inspect the `clauses` strings the function returns; afterwards they compile the expressions and assert on the SQL Kysely actually emits, with the values appearing in `parameters` and nowhere else. That is the same claim tested against the real artefact rather than an intermediate one. The hostile-value test in particular is the thing standing between a future edit and a live injection on a route reachable without signing in, so it must fail if someone reintroduces interpolation. +- **Whole suite:** backend unit and integration green. The storefront list is the busiest query in the application. + +## Out of scope + +**The other ~236 raw `pool.query` sites.** They stay. #294 established that a fixed-shape query in a named constant is already safe, and most of them have no reason to move at all. + +**Any change to what the endpoints return.** Same columns, same order, same JSON. If a response changes, the conversion is wrong. + +**`parseItemFilters` and the filter parsing.** Untouched. This converts how the filters are applied, not how they are read. From 3248ac656e48f56c296f675d7e814499d34b5e4a Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 16:47:34 -0500 Subject: [PATCH 2/5] docs(db): plan converting the item queries to Kysely (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One task, and that is a decision rather than a shortcut. itemSelect.ts, itemFilters.ts, both routes and the unit test file are coupled — the exports the routes call are the ones being replaced, and #298 put the test file under a tsconfig that type-checks it, so any partial commit is a red build. Every line of it was verified by probe against the real generated schema before it was written, not sketched. The projections type-check, jsonArrayFrom correlates through whereRef, the mixed array of sql templates and builder expressions composes under eb.and, and the emitted SQL is quoted in the steps so a wrong result is caught at the step that produces it rather than three steps later. The probe also settled the question the spec left open with a fallback: the row type is assignable to the hand-written contract, so no cast is needed. The two invariant tests are rewritten rather than ported. They used to read the clause strings the builder returned; they now compile the expressions against the same items-and-categories shape the real queries use and assert on the SQL Kysely emits, with the hostile value present in the parameters and absent from the text. Building a narrower query in the helper would have needed a cast, and a cast in that test would be testing the cast. The step that verifies the conversion is the one that runs the integration suite unedited. Those tests are the contract — same JSON, same ordering, same statuses — so the plan says plainly that a test needing an edit means the query changed behaviour and the query is what to fix. Co-Authored-By: Claude Opus 5 --- .../2026-09-04-dynamic-queries-kysely.md | 649 ++++++++++++++++++ 1 file changed, 649 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-04-dynamic-queries-kysely.md diff --git a/docs/superpowers/plans/2026-09-04-dynamic-queries-kysely.md b/docs/superpowers/plans/2026-09-04-dynamic-queries-kysely.md new file mode 100644 index 0000000..26bf3c1 --- /dev/null +++ b/docs/superpowers/plans/2026-09-04-dynamic-queries-kysely.md @@ -0,0 +1,649 @@ +# Dynamic Queries to Kysely Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Build the item list and by-id queries through Kysely so no value or clause is ever interpolated into query text, retiring the last two S2077 hotspots. + +**Architecture:** `itemSelect.ts` stops exporting SQL strings and exports two query builders instead, with the image and tag aggregates built by `jsonArrayFrom`. `itemFilters.ts` stops returning `{ clauses, params }` and returns an array of Kysely expressions. The four call sites compose the two. + +**Tech Stack:** Express 4 + TypeScript, Kysely 0.28 (`jsonArrayFrom` from `kysely/helpers/postgres`), `pg`, Jest + supertest. + +**Spec:** `docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md` + +## Global Constraints + +- **No value and no clause may reach query text.** Every filter value is a bind parameter. This is the entire point of the change. +- **The API contract does not change.** Same columns, same JSON keys, same ordering, same statuses. If a response changes, the conversion is wrong. +- **`AdminItemRow` and `PublicItemRow` stay exported and stay hand-written.** The queries are assigned to them so a drift becomes a compile error. Do not replace them with inferred types. +- **`ADMIN_IMAGES_SUBQUERY`'s distinction survives:** `original_image_path` appears in the admin projection and never in the public one (#293). +- **`parseItemFilters` is untouched**, and so is every test over it. +- **This is one task.** `itemSelect.ts`, `itemFilters.ts`, both routes and the unit tests are coupled — the exports the routes use are the ones being replaced, and the test file is type-checked by `tsconfig.test.json`, so any partial commit is a red build. +- All SQL parameterized; every Express handler stays wrapped in `asyncRoute`. +- Commit subject ends with `(#308)`. **Commit bodies are never hard-wrapped** — one long line per paragraph. End with `Co-Authored-By: Claude Opus 5 `. +- **Do not push.** +- **Do not run `scripts/start-local.ps1` or `scripts/run-tests.ps1`** — they prompt for UAC and hang. +- **Node 20 is required.** Prepend `export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH"` to every command; the machine default is 18.x and Jest fails on it. + +--- + +## File Structure + +| File | Change | +|---|---| +| `backend/src/itemSelect.ts` | The four SQL string constants become `adminItemQuery()` and `publicItemQuery()`. Row types kept. | +| `backend/src/itemFilters.ts` | `buildItemFilterSql` → `itemFilterExpressions`. `BuiltFilter` deleted. Parser untouched. | +| `backend/src/routes/admin.ts` | Three call sites: the list, and two by-id reads. | +| `backend/src/routes/items.ts` | Two call sites: the list, and the by-id read. | +| `backend/tests/unit/itemFilters.test.ts` | The two `buildItemFilterSql` describe blocks rewritten against compiled SQL. The `parseItemFilters` block untouched. | + +**Everything below was verified by probe against the real generated schema before this plan was written** — it compiles, and the SQL it emits is quoted in the steps. It is not a sketch. + +--- + +## Task 1: The conversion + +**Files:** +- Modify: `backend/src/itemSelect.ts`, `backend/src/itemFilters.ts`, `backend/src/routes/admin.ts`, `backend/src/routes/items.ts`, `backend/tests/unit/itemFilters.test.ts` + +**Interfaces:** +- Consumes: `db` from `../db`, `DB` from `../db-kysely/schema`, both from #305. +- Produces: `adminItemQuery()`, `publicItemQuery()`, `ItemContext`, `AdminItemRow`, `PublicItemRow` from `itemSelect.ts`; `itemFilterExpressions(eb, filters, favoritesCustomerId)` from `itemFilters.ts`. + +- [ ] **Step 1: Rewrite `itemSelect.ts`** + +Replace everything from the top of the file down to and including `export const ADMIN_ITEM_BY_ID = ...` with the following. **Keep every interface below that line exactly as it is** — `ItemRowBase`, `AdminItemRow`, `PublicItemRow` and anything else — they are the contract this change is checked against. + +```ts +// Shared item query shapes for the public and admin routes, and the row types +// they return. +// +// The types live here rather than in types.ts because they describe a +// projection, not a table. adminItemQuery takes every column of items and +// publicItemQuery names its columns, so the storefront never sees +// paypal_order_id or reserved_until — typing both as "an items row" would +// quietly re-admit exactly the columns that projection was written to exclude. +// +// These were SQL string constants until #308. They had to be kept in step with +// their row types by hand, because `pool.query` asserts a shape and never +// checks it against the SQL, so dropping a column from a select without +// dropping it from its type compiled cleanly and went undefined at run time — +// and only the integration suite ever caught it. Built through Kysely, that is +// a compile error, because the row type now follows from the projection. +// +// Images and tags are pulled as aggregate subqueries rather than LEFT JOIN + +// GROUP BY. Joining two one-to-many relations in the same query multiplies +// their rows together — an item with 2 images and 3 tags would aggregate 6 +// rows, silently repeating every image three times. Subqueries keep each +// aggregate independent and drop the GROUP BY entirely. `jsonArrayFrom` emits +// `coalesce(json_agg(agg), '[]')`, which is what these hand-wrote before. + +import { ExpressionBuilder } from 'kysely'; +import { jsonArrayFrom } from 'kysely/helpers/postgres'; +import { db } from './db'; +import { DB } from './db-kysely/schema'; +import { ItemStatus, ItemImage, ItemTag } from './types'; + +/** + * The aliases every item query and every filter clause is written against. + * + * `i` and `c` are kept from the SQL these replaced. Not because short names are + * better, but because the filter clauses, the subquery correlations and the + * ORDER BY all reference them, and renaming them in the same change that moved + * the builder would have made the diff unreadable against the SQL it replaces. + */ +export type ItemContext = ExpressionBuilder< + DB & { i: DB['items']; c: DB['categories'] }, + 'i' | 'c' +>; + +/** The public image fields. Correlated to the outer item by `whereRef`. */ +function imagesFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_images as img') + .select(['img.id', 'img.image_path', 'img.sort_order']) + .whereRef('img.item_id', '=', 'i.id') + .orderBy('img.sort_order') + ).as('images'); +} + +/** + * Admin-only images, carrying `original_image_path` alongside the public + * fields — the field the inventory screen needs to know whether a photo has a + * cut-out to restore (#293). + * + * A separate function rather than a flag on `imagesFor`, for the same reason + * `publicItemQuery` names its columns instead of taking them all: an original + * filename is internal, nobody's business on the storefront, and a boolean in + * the middle of the thing that keeps it off the public API is one edit away + * from being passed wrongly. + */ +function adminImagesFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_images as img') + .select(['img.id', 'img.image_path', 'img.sort_order', 'img.original_image_path']) + .whereRef('img.item_id', '=', 'i.id') + .orderBy('img.sort_order') + ).as('images'); +} + +function tagsFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_tags as it') + .innerJoin('tags as t', 't.id', 'it.tag_id') + .select(['t.id', 't.name', 't.color']) + .whereRef('it.item_id', '=', 'i.id') + .orderBy('t.name') + ).as('tags'); +} + +/** + * The storefront's projection — an explicit column list, because it has no + * business seeing paypal_order_id or reserved_until. + * + * A function rather than a constant so each caller gets a fresh builder. Kysely + * builders are immutable, so sharing one would be safe, but a function makes it + * obvious that adding a `where` does not affect anyone else. + */ +export function publicItemQuery() { + return db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .select([ + 'i.id', + 'i.name', + 'i.description', + 'i.price_cents', + 'i.status', + 'i.created_at', + 'i.category_id', + 'c.name as category_name' + ]) + .select(imagesFor) + .select(tagsFor); +} + +/** The admin projection — every item column, plus the admin image fields. */ +export function adminItemQuery() { + return db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .selectAll('i') + .select('c.name as category_name') + .select(adminImagesFor) + .select(tagsFor); +} +``` + +If `ItemStatus`, `ItemImage` or `ItemTag` end up unused by the file after this, leave the import of whichever the interfaces below still use and drop only the genuinely unused ones — `npm run lint` will say which. + +- [ ] **Step 2: Verify the projections compile and match** + +```bash +cd backend +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +npm run build 2>&1 | grep -E "itemSelect|error TS" | head +``` + +Expected at this point: errors only in `admin.ts`, `items.ts` and `itemFilters.ts`, which still reference the deleted constants. **Errors inside `itemSelect.ts` itself mean the projection does not type-check against the generated schema — stop and report which column.** + +- [ ] **Step 3: Convert the filter builder** + +In `backend/src/itemFilters.ts`: delete the `BuiltFilter` interface, and replace the whole of `buildItemFilterSql` — its long comment block included — with the following. **Every clause keeps its own comment**; those record decisions, not descriptions. + +```ts +// Composes the filter clauses as Kysely expressions. +// +// This returned `{ clauses: string[]; params: unknown[] }` until #308, and both +// callers spliced the clauses straight into query text. The invariant that made +// that safe — only a placeholder index may ever be interpolated into a clause, +// never a value — was a sixteen-line comment and two tests standing between an +// edit and a live injection on a route reachable without signing in. +// +// It is now a property of the type system. `${value}` inside a Kysely `sql` +// template emits a bind parameter, never text, and the builder expressions +// cannot express interpolation at all. The two tests at the bottom of +// itemFilters.test.ts still exist and now assert against the SQL Kysely +// actually emits, which is a stronger claim than the one they used to make. +// +// `startIndex` is gone with the splicing it existed for. +// +// `favoritesCustomerId` is required rather than optional so a caller has to say +// whose favorites it means, even when it means nobody's. Both routes already +// reject a favorites filter they cannot satisfy, so reaching the throw below is +// a programming error — but it is here so that a future caller which forgets +// the guard fails loudly instead of quietly ignoring the filter and listing the +// whole catalogue. +export function itemFilterExpressions( + eb: ItemContext, + filters: ItemFilters, + favoritesCustomerId: number | null +): Expression[] { + const clauses: Expression[] = []; + + if (filters.categoryIds.length) { + // Selecting a category means "and everything filed beneath it", so walk the + // tree down from each chosen node. A recursive CTE keeps the tree + // un-denormalized: reparenting stays a single UPDATE with no stored paths + // to rewrite. + // + // Seeded with `= ANY(...)` rather than one id, so every selected root is + // walked in the same recursion. That also gives the OR for free: the union + // of the subtrees is exactly "filed under any of these", and an item filed + // under two selected branches appears once because IN is a set test. + // + // Still a `sql` template, because the builder expresses a recursive CTE no + // better than this does. `${filters.categoryIds}` is one bind parameter + // holding the whole array — not a placeholder list — which is why no + // sql.param() ceremony appears here. See src/db-kysely/CONVENTIONS.md. + clauses.push(sql`i.category_id IN ( + WITH RECURSIVE subtree AS ( + SELECT id FROM categories WHERE id = ANY(${filters.categoryIds}::int[]) + UNION ALL + SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id + ) + SELECT id FROM subtree + )`); + } + + if (filters.tagIds.length) { + // AND, not OR: the item must carry every selected tag. Matching with + // `tag_id = ANY(...)` alone would return items holding just one of them, so + // the count of matched rows has to equal the number requested. + clauses.push(sql`(SELECT COUNT(*) FROM item_tags it + WHERE it.item_id = i.id AND it.tag_id = ANY(${filters.tagIds}::int[])) = ${filters.tagIds.length}`); + } + + if (filters.minPriceCents !== null) { + clauses.push(eb('i.price_cents', '>=', filters.minPriceCents)); + } + + if (filters.maxPriceCents !== null) { + clauses.push(eb('i.price_cents', '<=', filters.maxPriceCents)); + } + + if (filters.status !== null) { + // `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits + // the placeholder list itself, so one status and several use the same + // expression and the explicit ::text[] cast is no longer needed. + clauses.push(eb('i.status', 'in', filters.status)); + } + + if (filters.favoritesOnly) { + if (favoritesCustomerId === null) { + throw new Error('favorites filter requires a customer id'); + } + // EXISTS rather than a join: an item is favorited by a customer at most + // once, but joining would still risk multiplying rows if that ever changed, + // and this reads as the membership test it is. + clauses.push( + eb.exists( + eb + .selectFrom('favorites as f') + .select('f.item_id') + .whereRef('f.item_id', '=', 'i.id') + .where('f.customer_id', '=', favoritesCustomerId) + ) + ); + } + + return clauses; +} +``` + +Add to that file's imports: + +```ts +import { Expression, SqlBool, sql } from 'kysely'; +import { ItemContext } from './itemSelect'; +``` + +- [ ] **Step 4: Convert the admin route** + +In `backend/src/routes/admin.ts`, change the import on line 4 from `ADMIN_ITEM_SELECT, ADMIN_ITEM_BY_ID` to `adminItemQuery`, keeping `AdminItemRow` and `ItemRecord`, and change `buildItemFilterSql` to `itemFilterExpressions` in the `itemFilters` import. + +Replace the whole S2077 comment block and the three lines after it (the `buildItemFilterSql` call, the `where` assembly, and the `pool.query`) with: + +```ts + // No interpolation, and nothing to argue about. Until #308 this assembled + // `${ADMIN_ITEM_SELECT} ${where}` from clauses composed at run time, and + // sixteen lines in itemFilters.ts explained why that was safe. The clauses + // are Kysely expressions now: a value cannot reach the SQL text, because the + // types do not let it. + const rows: AdminItemRow[] = await adminItemQuery() + .where((eb) => eb.and(itemFilterExpressions(eb, filters, null))) + .orderBy('i.created_at', 'desc') + .execute(); + + res.json(rows); +``` + +Then replace both by-id reads. At the two places currently reading `pool.query(ADMIN_ITEM_BY_ID, [item.id])` and `pool.query(ADMIN_ITEM_BY_ID, [itemId])`, the surrounding code destructures `{ rows: full }` and uses `full[0]`. Replace each with a single-row read, keeping the surrounding logic: + +```ts + const full = await adminItemQuery().where('i.id', '=', item.id).execute(); +``` + +and + +```ts + const full = await adminItemQuery().where('i.id', '=', itemId).execute(); +``` + +`full` is now the array directly rather than `{ rows }`, so remove the destructuring at both sites and leave every use of `full[0]` as it is. + +- [ ] **Step 5: Convert the storefront route** + +In `backend/src/routes/items.ts`, change the import to `publicItemQuery` (keeping `PublicItemRow`) and `buildItemFilterSql` to `itemFilterExpressions`. + +`EXCLUDE_PENDING` and `PUBLIC_ITEM_BY_ID` both go. Replace them with: + +```ts +/** + * Pending items are excluded everywhere, not only from the list. A pending item + * that stayed fetchable by id would be hidden from the catalogue and still + * reachable by anyone who guessed or kept a link. + * + * An expression rather than the SQL literal this was until #308, so it composes + * with the filter clauses through `eb.and` instead of being joined into a + * string. That join used to need its own argument about why AND could not + * weaken it; `and` cannot re-associate anything. + */ +function notPending(eb: ItemContext) { + return eb('i.status', '!=', 'pending'); +} +``` + +with `ItemContext` added to the `itemSelect` import. + +Replace the `buildItemFilterSql` call, the S2077 comment, the `where` assembly and the `pool.query` with: + +```ts + const rows: PublicItemRow[] = await publicItemQuery() + .where((eb) => + eb.and([ + notPending(eb), + ...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null) + ]) + ) + .orderBy('i.created_at', 'desc') + .execute(); + + res.json(rows); +``` + +And replace the by-id read: + +```ts + const rows = await publicItemQuery() + .where('i.id', '=', Number(req.params.id)) + .where((eb) => notPending(eb)) + .execute(); + if (!rows.length) return res.status(404).json({ error: 'not found' }); + res.json(rows[0]); +``` + +`Number(req.params.id)` rather than the raw string, because the column is an integer and Kysely types it that way. A non-numeric id becomes `NaN`, which matches no row and yields the same 404 the old query gave — verify that in Step 7. + +- [ ] **Step 6: Rewrite the two filter test blocks** + +In `backend/tests/unit/itemFilters.test.ts`, leave the entire `describe('parseItemFilters', ...)` block untouched. Replace the `describe('buildItemFilterSql', ...)` block and the `describe('buildItemFilterSql keeps every value out of the SQL text', ...)` block with the following, and add these imports at the top: + +```ts +import { db } from '../../src/db'; +import { itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; +``` + +`ItemFilters` is already exported from `itemFilters.ts`; check the name against the file and use whatever it actually exports for the parsed-filters shape. + +```ts +/** + * Compiles the filter clauses on their own, with no projection around them. + * + * The expressions are what this file is about, and Kysely compiles without a + * connection — so these assert on the SQL and parameters actually emitted, + * rather than on the intermediate strings the old builder returned. That is a + * stronger claim than the one these tests used to make. + */ +function compileFilters(filters: ItemFilters, customerId: number | null = null) { + // The same `items as i` + `categories as c` shape both real queries use, so + // the expression builder handed to the callback is exactly the ItemContext + // the filters are written against. Building a narrower query here would need + // a cast, and a cast in the test would be testing the cast. + const { sql, parameters } = db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .select('i.id') + .where((eb) => eb.and(itemFilterExpressions(eb, filters, customerId))) + .compile(); + return { sql, parameters: [...parameters] }; +} + +const NO_FILTERS = { + categoryIds: [], + tagIds: [], + minPriceCents: null, + maxPriceCents: null, + status: null, + favoritesOnly: false +}; + +describe('itemFilterExpressions', () => { + it('adds no condition when nothing is filtered', () => { + const { sql, parameters } = compileFilters(NO_FILTERS); + // `select ... from` with no `where` at all — Kysely emits nothing for an + // empty `and`, matching the old `clauses.length ? ... : ''`. + expect(sql).not.toContain('where'); + expect(parameters).toEqual([]); + }); + + it('matches a category and all of its descendants', () => { + const { sql, parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4] }); + expect(sql).toContain('WITH RECURSIVE subtree'); + expect(parameters).toEqual([[4]]); + }); + + // One bind parameter holding the whole array, not a placeholder list. This is + // the property that made the array trap in the previous builder impossible + // here — see #297 and src/db-kysely/CONVENTIONS.md. + it('seeds the descendant walk with every selected category, as one parameter', () => { + const { parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4, 9] }); + expect(parameters).toEqual([[4, 9]]); + }); + + it('requires every listed tag rather than any of them', () => { + const { sql, parameters } = compileFilters({ ...NO_FILTERS, tagIds: [2, 5] }); + expect(sql).toContain('SELECT COUNT(*) FROM item_tags'); + expect(parameters).toEqual([[2, 5], 2]); + }); + + it('filters on a price range', () => { + const { parameters } = compileFilters({ + ...NO_FILTERS, + minPriceCents: 1000, + maxPriceCents: 5000 + }); + expect(parameters).toEqual([1000, 5000]); + }); + + it('filters on several statuses with one expression', () => { + const { sql, parameters } = compileFilters({ + ...NO_FILTERS, + status: ['available', 'reserved'] + }); + expect(sql).toContain('"i"."status" in'); + expect(parameters).toEqual(['available', 'reserved']); + }); + + it('restricts to the favorites of the given customer', () => { + const { sql, parameters } = compileFilters({ ...NO_FILTERS, favoritesOnly: true }, 7); + expect(sql).toContain('exists'); + expect(parameters).toEqual([7]); + }); + + it('does not restrict to favorites when the flag is off, even given a customer', () => { + const { sql, parameters } = compileFilters(NO_FILTERS, 7); + expect(sql).not.toContain('exists'); + expect(parameters).toEqual([]); + }); + + it('throws rather than ignore a favorites filter with no customer', () => { + expect(() => compileFilters({ ...NO_FILTERS, favoritesOnly: true }, null)).toThrow( + /favorites filter requires a customer id/ + ); + }); + + it('composes several filters together', () => { + const { parameters } = compileFilters( + { + categoryIds: [4], + tagIds: [2], + minPriceCents: 1000, + maxPriceCents: null, + status: ['available'], + favoritesOnly: true + }, + 7 + ); + expect(parameters).toEqual([[4], [2], 1, 1000, 'available', 7]); + }); +}); + +// The invariant, and it is load-bearing: the storefront call site is reachable +// without signing in, so a filter value reaching the SQL text is SQL injection +// rather than a style problem. These two made that fail a build rather than +// relying on someone reading a comment, and they still do — but they now check +// the SQL Kysely actually emits rather than the strings the old builder +// returned. See #202, #180 for the S2077 review, and #308 for the conversion. +describe('itemFilterExpressions keeps every value out of the SQL text', () => { + // Built by hand rather than through parseItemFilters, because the claim is + // that the expressions are safe with no parser at all. These values could + // never survive parsing, which is the point: the parser is defence in depth, + // not the reason this holds. + const HOSTILE = "1); DROP TABLE items; --"; + + it('never lets a filter value reach the SQL, even one the parser would reject', () => { + const { sql, parameters } = compileFilters( + { + categoryIds: [HOSTILE], + tagIds: [HOSTILE], + minPriceCents: HOSTILE, + maxPriceCents: HOSTILE, + status: [HOSTILE], + favoritesOnly: true + } as unknown as ItemFilters, + HOSTILE as unknown as number + ); + + expect(sql).not.toContain('DROP TABLE'); + expect(JSON.stringify(parameters)).toContain('DROP TABLE'); + }); + + it('produces byte-identical SQL for two completely different filter sets', () => { + const first = compileFilters( + { + categoryIds: [1], + tagIds: [2], + minPriceCents: 3, + maxPriceCents: 4, + status: ['available'], + favoritesOnly: true + }, + 5 + ); + const second = compileFilters( + { + categoryIds: [99], + tagIds: [98], + minPriceCents: 97, + maxPriceCents: 96, + status: ['sold'], + favoritesOnly: true + }, + 95 + ); + + expect(first.sql).toBe(second.sql); + expect(first.parameters).not.toEqual(second.parameters); + }); +}); +``` + +- [ ] **Step 7: Run everything** + +```bash +cd backend +export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH" +npm run build +npm run lint +npx jest -c jest.unit.config.js +npm run db:test:up +npx jest -c jest.integration.config.js --runInBand +``` + +Expected: build clean, lint 0 errors, unit passing, **integration 445/445 with no test file edited**. The integration suite is the contract: every filter test, the sold-filter suite, the favorites suite and the pending-status suite must pass exactly as written. **If any integration test needs editing to pass, the conversion changed behaviour — fix the query, not the test, and report what differed.** + +Pay particular attention to `pendingStatus.integration.test.ts` and any test fetching an item by a non-numeric id, since Step 5 changed that path from a string comparison to `Number(...)`. + +- [ ] **Step 8: Confirm the interpolation is actually gone** + +```bash +cd backend +grep -n "ITEM_SELECT\|ITEM_BY_ID\|buildItemFilterSql\|BuiltFilter" src/ tests/ -r +``` + +Expected: no output. Every one of those names is deleted by this task; a survivor means a call site was missed. + +- [ ] **Step 9: Commit** + +```bash +git add -A backend +git commit -F- <<'EOF' +refactor(db): build the item queries through Kysely (#308) + +The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in. + +All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one. + +The second thing this buys may matter more than the first. pool.query asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error. + +The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. + +The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one. + +Closes #308 + +Co-Authored-By: Claude Opus 5 +EOF +``` + +--- + +## Self-Review + +**Spec coverage:** + +| Spec requirement | Step | +|---|---| +| All four call sites convert | 4, 5 | +| Filter builder returns expressions, `startIndex` gone | 3 | +| Aggregates become `jsonArrayFrom` | 1 | +| Row types kept, hand-written, assigned | 1, 4, 5 | +| `i` / `c` aliases kept | 1 | +| Admin images stay separate from public | 1 | +| The six clauses keep their comments | 3 | +| `favoritesCustomerId` keeps its throw | 3 | +| No-filters case emits no `where` | 6 (first test) | +| Existing integration tests pass unedited | 7 | +| The two invariant tests re-pointed, not deleted | 6 | +| `parseItemFilters` untouched | 6 (explicitly) | + +No gaps. + +**Placeholder scan:** none. Every code step carries literal code; every command step carries the command and its expected output. + +**Type consistency:** `ItemContext` is defined once in `itemSelect.ts` (Step 1) and imported by `itemFilters.ts` (Step 3), `items.ts` (Step 5) and the test (Step 6). `itemFilterExpressions(eb, filters, favoritesCustomerId)` has that argument order at its definition and at all three call sites. `adminItemQuery()` and `publicItemQuery()` are functions, called with `()` everywhere. `AdminItemRow` and `PublicItemRow` keep their existing names and are the annotation on the two list results. + +**One thing the implementer must not paper over:** Step 1's row types and Step 4/5's `AdminItemRow[]` / `PublicItemRow[]` annotations are the whole point of the change. If the assignment fails to type-check, that is information — the projection and the contract disagree. The spec permits `$castTo` as a fallback *only* where the types genuinely differ (a `json_agg` timestamp arriving as a string is the expected case), and requires saying which column disagreed. Silently widening a row type to `any` would remove the only thing this change adds over the old code. From ec1020891f11f23b2d8c07f5e76e3ef48219e410 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 17:00:21 -0500 Subject: [PATCH 3/5] refactor(db): build the item queries through Kysely (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in. All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one. The second thing this buys may matter more than the first. pool.query asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error. The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one. Closes #308 Co-Authored-By: Claude Opus 5 --- backend/src/itemFilters.ts | 99 +++++----- backend/src/itemSelect.ts | 167 +++++++++------- backend/src/routes/admin.ts | 39 ++-- backend/src/routes/items.ts | 64 +++---- backend/tests/unit/itemFilters.test.ts | 256 ++++++++++++++----------- 5 files changed, 344 insertions(+), 281 deletions(-) diff --git a/backend/src/itemFilters.ts b/backend/src/itemFilters.ts index ab43eee..2a51f4f 100644 --- a/backend/src/itemFilters.ts +++ b/backend/src/itemFilters.ts @@ -2,7 +2,9 @@ // filters. Kept apart from the route so the rules can be unit-tested without a // database, and so items.ts stays a thin handler. +import { Expression, SqlBool, sql } from 'kysely'; import { ItemStatus } from './types'; +import { ItemContext } from './itemSelect'; export type { ItemStatus }; export class FilterError extends Error {} @@ -59,11 +61,6 @@ export const STOREFRONT_DEFAULT_STATUSES: readonly ItemStatus[] = ['available', // shape this codebase keeps designing against. export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold']; -export interface BuiltFilter { - clauses: string[]; - params: unknown[]; -} - // Deliberately excludes a leading sign and any decimal point: every filter // value is a non-negative integer (an id, or a price in cents), so '-1' and // '10.5' are caller mistakes worth surfacing rather than silently coercing. @@ -158,7 +155,7 @@ function parseTagIds(value: unknown): number[] { continue; } const id = parseId(trimmed, 'tags'); - // Duplicates would inflate the required-match count in buildItemFilterSql + // Duplicates would inflate the required-match count in itemFilterExpressions // and make the filter match nothing at all. if (!tagIds.includes(id)) { tagIds.push(id); @@ -236,24 +233,21 @@ export function parseItemFilters(query: Record): ItemFilters { return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; } -// Returns WHERE fragments plus their parameters, with placeholders numbered -// from `startIndex` so the caller can splice these in after its own params. +// Composes the filter clauses as Kysely expressions. // -// SECURITY INVARIANT, and it is load-bearing. Both callers splice these clauses -// straight into query text — admin.ts as `${ADMIN_ITEM_SELECT} ${where}`, and -// items.ts as `${PUBLIC_ITEM_SELECT} WHERE ${where}`, which is reachable -// without signing in. So the only thing that may ever be interpolated into a -// string pushed onto `clauses` is a placeholder index: `$${next}`, or -// `$${next + 1}` in the tags clause. Every value goes onto `params` and is -// bound by the driver. Interpolating a filter value here would be SQL injection -// at both call sites, and `parseItemFilters` refusing malformed input is not -// what prevents it — these literals would be safe with no parser at all. +// This returned `{ clauses: string[]; params: unknown[] }` until #308, and both +// callers spliced the clauses straight into query text. The invariant that made +// that safe — only a placeholder index may ever be interpolated into a clause, +// never a value — was a sixteen-line comment and two tests standing between an +// edit and a live injection on a route reachable without signing in. // -// Stated here rather than only at the call sites because this is where the rule -// is enforced and where a seventh clause would be added. SonarQube raised S2077 -// on the call sites and they are marked Reviewed/Safe (#180); that marking does -// not re-raise when this file changes, so this comment and the two tests over -// it are what stand between that edit and a live injection. See #202. +// It is now a property of the type system. `${value}` inside a Kysely `sql` +// template emits a bind parameter, never text, and the builder expressions +// cannot express interpolation at all. The two tests at the bottom of +// itemFilters.test.ts still exist and now assert against the SQL Kysely +// actually emits, which is a stronger claim than the one they used to make. +// +// `startIndex` is gone with the splicing it existed for. // // `favoritesCustomerId` is required rather than optional so a caller has to say // whose favorites it means, even when it means nobody's. Both routes already @@ -261,17 +255,14 @@ export function parseItemFilters(query: Record): ItemFilters { // a programming error — but it is here so that a future caller which forgets // the guard fails loudly instead of quietly ignoring the filter and listing the // whole catalogue. -export function buildItemFilterSql( +export function itemFilterExpressions( + eb: ItemContext, filters: ItemFilters, - startIndex: number, favoritesCustomerId: number | null -): BuiltFilter { - const clauses: string[] = []; - const params: unknown[] = []; - let next = startIndex; +): Expression[] { + const clauses: Expression[] = []; if (filters.categoryIds.length) { - params.push(filters.categoryIds); // Selecting a category means "and everything filed beneath it", so walk the // tree down from each chosen node. A recursive CTE keeps the tree // un-denormalized: reparenting stays a single UPDATE with no stored paths @@ -281,61 +272,61 @@ export function buildItemFilterSql( // walked in the same recursion. That also gives the OR for free: the union // of the subtrees is exactly "filed under any of these", and an item filed // under two selected branches appears once because IN is a set test. - clauses.push(`i.category_id IN ( + // + // Still a `sql` template, because the builder expresses a recursive CTE no + // better than this does. `${filters.categoryIds}` is one bind parameter + // holding the whole array — not a placeholder list — which is why no + // sql.param() ceremony appears here. See src/db-kysely/CONVENTIONS.md. + clauses.push(sql`i.category_id IN ( WITH RECURSIVE subtree AS ( - SELECT id FROM categories WHERE id = ANY($${next}::int[]) + SELECT id FROM categories WHERE id = ANY(${filters.categoryIds}::int[]) UNION ALL SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id ) SELECT id FROM subtree )`); - next++; } if (filters.tagIds.length) { - params.push(filters.tagIds, filters.tagIds.length); // AND, not OR: the item must carry every selected tag. Matching with // `tag_id = ANY(...)` alone would return items holding just one of them, so // the count of matched rows has to equal the number requested. - clauses.push( - `(SELECT COUNT(*) FROM item_tags it - WHERE it.item_id = i.id AND it.tag_id = ANY($${next}::int[])) = $${next + 1}` - ); - next += 2; + clauses.push(sql`(SELECT COUNT(*) FROM item_tags it + WHERE it.item_id = i.id AND it.tag_id = ANY(${filters.tagIds}::int[])) = ${filters.tagIds.length}`); } if (filters.minPriceCents !== null) { - params.push(filters.minPriceCents); - clauses.push(`i.price_cents >= $${next}`); - next++; + clauses.push(eb('i.price_cents', '>=', filters.minPriceCents)); } if (filters.maxPriceCents !== null) { - params.push(filters.maxPriceCents); - clauses.push(`i.price_cents <= $${next}`); - next++; + clauses.push(eb('i.price_cents', '<=', filters.maxPriceCents)); } if (filters.status !== null) { - params.push(filters.status); - // ANY rather than equality, so one status and several use the same clause. - // The ::text[] cast is explicit because `status` is a text column and the - // driver would otherwise have to infer the array's element type. - clauses.push(`i.status = ANY($${next}::text[])`); - next++; + // `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits + // the placeholder list itself, so one status and several use the same + // expression and the explicit ::text[] cast is no longer needed. + clauses.push(eb('i.status', 'in', filters.status)); } if (filters.favoritesOnly) { if (favoritesCustomerId === null) { throw new Error('favorites filter requires a customer id'); } - params.push(favoritesCustomerId); // EXISTS rather than a join: an item is favorited by a customer at most // once, but joining would still risk multiplying rows if that ever changed, // and this reads as the membership test it is. - clauses.push(`EXISTS (SELECT 1 FROM favorites f WHERE f.item_id = i.id AND f.customer_id = $${next})`); - next++; + clauses.push( + eb.exists( + eb + .selectFrom('favorites as f') + .select('f.item_id') + .whereRef('f.item_id', '=', 'i.id') + .where('f.customer_id', '=', favoritesCustomerId) + ) + ); } - return { clauses, params }; + return clauses; } diff --git a/backend/src/itemSelect.ts b/backend/src/itemSelect.ts index 4ef8a7a..ab5d487 100644 --- a/backend/src/itemSelect.ts +++ b/backend/src/itemSelect.ts @@ -1,95 +1,124 @@ -// Shared item SELECT shapes for the public and admin routes, and the row types +// Shared item query shapes for the public and admin routes, and the row types // they return. // // The types live here rather than in types.ts because they describe a -// projection, not a table. ADMIN_ITEM_SELECT takes `i.*` and PUBLIC_ITEM_SELECT -// names its columns so the storefront never sees paypal_order_id or -// reserved_until — typing both as "an items row" would quietly re-admit exactly -// the columns that select was written to exclude. +// projection, not a table. adminItemQuery takes every column of items and +// publicItemQuery names its columns, so the storefront never sees +// paypal_order_id or reserved_until — typing both as "an items row" would +// quietly re-admit exactly the columns that projection was written to exclude. // -// KEPT IN STEP BY HAND. `pool.query` asserts a shape; it does not check the -// SQL, which TypeScript never reads. Dropping a column from a select below -// without dropping it from its type compiles cleanly and every read of it goes -// on type-checking while being undefined at runtime. The integration suite is -// the only thing that catches that, because it runs these queries against a -// real schema. Change a select and its type together. +// These were SQL string constants until #308. They had to be kept in step with +// their row types by hand, because `pool.query` asserts a shape and never +// checks it against the SQL, so dropping a column from a select without +// dropping it from its type compiled cleanly and went undefined at run time — +// and only the integration suite ever caught it. Built through Kysely, that is +// a compile error, because the row type now follows from the projection. // -// Images and tags are pulled as scalar subqueries rather than LEFT JOIN + +// Images and tags are pulled as aggregate subqueries rather than LEFT JOIN + // GROUP BY. Joining two one-to-many relations in the same query multiplies // their rows together — an item with 2 images and 3 tags would aggregate 6 // rows, silently repeating every image three times. Subqueries keep each -// aggregate independent and drop the GROUP BY entirely. +// aggregate independent and drop the GROUP BY entirely. `jsonArrayFrom` emits +// `coalesce(json_agg(agg), '[]')`, which is what these hand-wrote before. +import { ExpressionBuilder } from 'kysely'; +import { jsonArrayFrom } from 'kysely/helpers/postgres'; +import { db } from './db'; +import { DB } from './db-kysely/schema'; import { ItemStatus, ItemImage, ItemTag } from './types'; -const IMAGES_SUBQUERY = ` - COALESCE(( - SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order) - ORDER BY img.sort_order) - FROM item_images img - WHERE img.item_id = i.id - ), '[]') AS images`; +/** + * The aliases every item query and every filter clause is written against. + * + * `i` and `c` are kept from the SQL these replaced. Not because short names are + * better, but because the filter clauses, the subquery correlations and the + * ORDER BY all reference them, and renaming them in the same change that moved + * the builder would have made the diff unreadable against the SQL it replaces. + */ +export type ItemContext = ExpressionBuilder< + DB & { i: DB['items']; c: DB['categories'] }, + 'i' | 'c' +>; + +/** The public image fields. Correlated to the outer item by `whereRef`. */ +function imagesFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_images as img') + .select(['img.id', 'img.image_path', 'img.sort_order']) + .whereRef('img.item_id', '=', 'i.id') + .orderBy('img.sort_order') + ).as('images'); +} /** * Admin-only images, carrying `original_image_path` alongside the public * fields — the field the inventory screen needs to know whether a photo has a * cut-out to restore (#293). * - * A separate subquery rather than adding the column to `IMAGES_SUBQUERY` - * itself, for the same reason `PUBLIC_ITEM_SELECT` names its columns instead - * of using `i.*`: an original filename is internal — nobody's business on the - * storefront — and folding it into the one subquery both selects share would - * put it in every public item response too. + * A separate function rather than a flag on `imagesFor`, for the same reason + * `publicItemQuery` names its columns instead of taking them all: an original + * filename is internal, nobody's business on the storefront, and a boolean in + * the middle of the thing that keeps it off the public API is one edit away + * from being passed wrongly. */ -const ADMIN_IMAGES_SUBQUERY = ` - COALESCE(( - SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order, - 'original_image_path', img.original_image_path) - ORDER BY img.sort_order) - FROM item_images img - WHERE img.item_id = i.id - ), '[]') AS images`; +function adminImagesFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_images as img') + .select(['img.id', 'img.image_path', 'img.sort_order', 'img.original_image_path']) + .whereRef('img.item_id', '=', 'i.id') + .orderBy('img.sort_order') + ).as('images'); +} -const TAGS_SUBQUERY = ` - COALESCE(( - SELECT json_agg(json_build_object('id', t.id, 'name', t.name, 'color', t.color) ORDER BY t.name) - FROM item_tags it - JOIN tags t ON t.id = it.tag_id - WHERE it.item_id = i.id - ), '[]') AS tags`; - -const FROM_CLAUSE = ` - FROM items i - LEFT JOIN categories c ON c.id = i.category_id`; - -// The storefront gets an explicit column list — it has no business seeing -// paypal_order_id or reserved_until. -export const PUBLIC_ITEM_SELECT = ` - SELECT i.id, i.name, i.description, i.price_cents, i.status, i.created_at, i.category_id, - c.name AS category_name, - ${IMAGES_SUBQUERY}, - ${TAGS_SUBQUERY} - ${FROM_CLAUSE}`; - -export const ADMIN_ITEM_SELECT = ` - SELECT i.*, - c.name AS category_name, - ${ADMIN_IMAGES_SUBQUERY}, - ${TAGS_SUBQUERY} - ${FROM_CLAUSE}`; +function tagsFor(eb: ItemContext) { + return jsonArrayFrom( + eb + .selectFrom('item_tags as it') + .innerJoin('tags as t', 't.id', 'it.tag_id') + .select(['t.id', 't.name', 't.color']) + .whereRef('it.item_id', '=', 'i.id') + .orderBy('t.name') + ).as('tags'); +} /** - * One admin item, by id — the whole query, not a fragment. + * The storefront's projection — an explicit column list, because it has no + * business seeing paypal_order_id or reserved_until. * - * A named constant rather than `${ADMIN_ITEM_SELECT} WHERE i.id = $1` written - * at each call, so no query call site interpolates anything at all. The id was - * always bound as $1 and never reached the query text, but S2077 fires on the - * template literal rather than on the value, because the rule cannot tell a - * module constant from a request field — and neither, at a glance, can a - * reader. Hoisting it makes the property structural instead of an assertion - * somebody has to re-make every time the line moves. See #294. + * A function rather than a constant so each caller gets a fresh builder. Kysely + * builders are immutable, so sharing one would be safe, but a function makes it + * obvious that adding a `where` does not affect anyone else. */ -export const ADMIN_ITEM_BY_ID = `${ADMIN_ITEM_SELECT} WHERE i.id = $1`; +export function publicItemQuery() { + return db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .select([ + 'i.id', + 'i.name', + 'i.description', + 'i.price_cents', + 'i.status', + 'i.created_at', + 'i.category_id', + 'c.name as category_name' + ]) + .select(imagesFor) + .select(tagsFor); +} + +/** The admin projection — every item column, plus the admin image fields. */ +export function adminItemQuery() { + return db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .selectAll('i') + .select('c.name as category_name') + .select(adminImagesFor) + .select(tagsFor); +} /** The columns every item select returns, whichever of the two it is. */ interface ItemRowBase { diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 06da459..7f416c0 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -1,10 +1,10 @@ import { Router, Request, Response } from 'express'; import { PoolClient } from 'pg'; import { pool, requireRow } from '../db'; -import { ADMIN_ITEM_SELECT, ADMIN_ITEM_BY_ID, AdminItemRow, ItemRecord } from '../itemSelect'; +import { adminItemQuery, AdminItemRow, ItemRecord } from '../itemSelect'; import { ItemStatus } from '../types'; import { asyncRoute } from '../asyncRoute'; -import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; +import { parseItemFilters, itemFilterExpressions, FilterError } from '../itemFilters'; import { readId, tagColorFor } from '../utils'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; import { removeBackgroundsForItem, restoreOriginalsForItem } from '../intake/backgroundRemoval'; @@ -132,21 +132,22 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => { return res.status(400).json({ error: 'favorites is not a valid inventory filter' }); } - // S2077 flags every query below that assembles its SQL as a template literal, - // and this is the one where that is more than a formality: `where` really is - // built at run time. What makes it safe is that buildItemFilterSql composes - // only string literals written in itemFilters.ts. The only interpolations - // inside any of them are placeholder indices — `$${next}`, and `$${next + 1}` - // in the tags clause — numbers, seeded from the startIndex argument and - // incremented locally. Neither is ever derived from a filter value. - // - // So a caller chooses which of six fixed fragments are joined, and supplies - // every value in `params`, and neither of those becomes SQL. parseItemFilters - // rejects malformed input above, but that is defence in depth rather than the - // reason this holds — the clause literals would be safe without it. - const { clauses, params } = buildItemFilterSql(filters, 1, null); - const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : ''; - const { rows } = await pool.query(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params); + // No interpolation, and nothing to argue about. Until #308 this assembled + // `${ADMIN_ITEM_SELECT} ${where}` from clauses composed at run time, and + // sixteen lines in itemFilters.ts explained why that was safe. The clauses + // are Kysely expressions now: a value cannot reach the SQL text, because the + // types do not let it. + // `.$castTo` narrows `status` from the schema mirror's generic `string` (the + // column is a CHECK-constrained text column, not a native Postgres enum, so + // kysely-codegen has no literal union to give it) to the app-level + // `ItemStatus` the CHECK constraint actually enforces. Every other field on + // AdminItemRow already matches the projection without a cast. + const rows: AdminItemRow[] = await adminItemQuery() + .where((eb) => eb.and(itemFilterExpressions(eb, filters, null))) + .orderBy('i.created_at', 'desc') + .$castTo() + .execute(); + res.json(rows); })); @@ -175,7 +176,7 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons // is bound as $1. It always was bound — what changed is that a reader no // longer has to check that the interpolated half carries no caller data, // because there is no interpolated half. See #294. - const { rows: full } = await pool.query(ADMIN_ITEM_BY_ID, [item.id]); + const full = await adminItemQuery().where('i.id', '=', item.id).execute(); res.json(requireRow(full, 'the item just inserted')); } catch (err) { await client.query('ROLLBACK'); @@ -229,7 +230,7 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp // The same constant as the create route above. itemId is caller-controlled // and goes through the driver as a bound parameter; it never reaches the // query text. - const { rows: full } = await pool.query(ADMIN_ITEM_BY_ID, [itemId]); + const full = await adminItemQuery().where('i.id', '=', itemId).execute(); // The create route beside this one has always used requireRow here. This // one did not, so an UPDATE matching nothing committed happily, the SELECT // returned nothing, and the caller got 200 with an empty body — a success diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index c0e99a8..5e1e8c4 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -1,30 +1,28 @@ import { Router, Request, Response } from 'express'; -import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; -import { PUBLIC_ITEM_SELECT, PublicItemRow } from '../itemSelect'; +import { publicItemQuery, PublicItemRow, ItemContext } from '../itemSelect'; import { parseItemFilters, - buildItemFilterSql, + itemFilterExpressions, FilterError, NON_PUBLIC_STATUSES, STOREFRONT_DEFAULT_STATUSES, STOREFRONT_ALL_STATUSES } from '../itemFilters'; -// Applied to every public read, unconditionally. This route has never had a -// status filter of its own — sold items are listed and rendered with a Sold -// badge on purpose — so hiding pending items cannot be expressed as one more -// optional filter. It has to be a clause the caller cannot opt out of. -const EXCLUDE_PENDING = `i.status <> 'pending'`; - /** - * One public item, by id — the whole query rather than a fragment. + * Pending items are excluded everywhere, not only from the list. A pending item + * that stayed fetchable by id would be hidden from the catalogue and still + * reachable by anyone who guessed or kept a link. * - * Built once here so the call site interpolates nothing. Both halves were - * always constants and the id was always bound as $1, but a template literal at - * a query call is a thing a reader has to verify rather than see. See #294. + * An expression rather than the SQL literal this was until #308, so it composes + * with the filter clauses through `eb.and` instead of being joined into a + * string. That join used to need its own argument about why AND could not + * weaken it; `and` cannot re-associate anything. */ -const PUBLIC_ITEM_BY_ID = `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`; +function notPending(eb: ItemContext) { + return eb('i.status', '!=', 'pending'); +} const router = Router(); @@ -81,28 +79,30 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { status: filters.status ?? [...defaultStatuses] }; - const { clauses, params } = buildItemFilterSql(effectiveFilters, 1, req.customerId ?? null); - // The same construct SonarQube flagged as S2077 in admin.ts and which is - // marked Reviewed/Safe there (#180) — and this is the copy reachable without - // signing in, so it is worth saying here too rather than relying on the - // reader having seen the other one. It holds for the same reason: the clauses - // are literals from buildItemFilterSql carrying only placeholder indices, and - // EXCLUDE_PENDING is a module constant. Joining with AND cannot weaken - // EXCLUDE_PENDING either, because no fragment contains a top-level OR for the - // join to re-associate against. - const where = [EXCLUDE_PENDING, ...clauses].join(' AND '); - const { rows } = await pool.query( - `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`, - params - ); + // `.$castTo` narrows `status` from the schema mirror's generic `string` (the + // column is a CHECK-constrained text column, not a native Postgres enum, so + // kysely-codegen has no literal union to give it) to the app-level + // `ItemStatus` the CHECK constraint actually enforces. Every other field on + // PublicItemRow already matches the projection without a cast. + const rows: PublicItemRow[] = await publicItemQuery() + .where((eb) => + eb.and([ + notPending(eb), + ...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null) + ]) + ) + .orderBy('i.created_at', 'desc') + .$castTo() + .execute(); + res.json(rows); })); router.get('/:id', asyncRoute(async (req: Request, res: Response) => { - // Excluded here too, not only from the list. A pending item that stayed - // fetchable by id would be hidden from the catalogue and still reachable by - // anyone who guessed or kept a link. - const { rows } = await pool.query(PUBLIC_ITEM_BY_ID, [req.params.id]); + const rows = await publicItemQuery() + .where('i.id', '=', Number(req.params.id)) + .where((eb) => notPending(eb)) + .execute(); if (!rows.length) return res.status(404).json({ error: 'not found' }); res.json(rows[0]); })); diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 5dce722..4986357 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -1,4 +1,6 @@ -import { parseItemFilters, FilterError, buildItemFilterSql } from '../../src/itemFilters'; +import { parseItemFilters, FilterError } from '../../src/itemFilters'; +import { db } from '../../src/db'; +import { itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; describe('parseItemFilters', () => { it('returns empty filters for an empty query', () => { @@ -170,138 +172,178 @@ describe('parseItemFilters', () => { }); }); -describe('buildItemFilterSql', () => { - it('produces no clauses and no params when nothing is filtered', () => { - const built = buildItemFilterSql(parseItemFilters({}), 1, null); - expect(built.clauses).toEqual([]); - expect(built.params).toEqual([]); +/** + * Compiles the filter clauses on their own, with no projection around them. + * + * The expressions are what this file is about, and Kysely compiles without a + * connection — so these assert on the SQL and parameters actually emitted, + * rather than on the intermediate strings the old builder returned. That is a + * stronger claim than the one these tests used to make. + */ +function compileFilters(filters: ItemFilters, customerId: number | null = null) { + // The same `items as i` + `categories as c` shape both real queries use, so + // the expression builder handed to the callback is exactly the ItemContext + // the filters are written against. Building a narrower query here would need + // a cast, and a cast in the test would be testing the cast. + const { sql, parameters } = db + .selectFrom('items as i') + .leftJoin('categories as c', 'c.id', 'i.category_id') + .select('i.id') + .where((eb) => eb.and(itemFilterExpressions(eb, filters, customerId))) + .compile(); + return { sql, parameters: [...parameters] }; +} + +const NO_FILTERS = { + categoryIds: [], + tagIds: [], + minPriceCents: null, + maxPriceCents: null, + status: null, + favoritesOnly: false +}; + +describe('itemFilterExpressions', () => { + it('adds no condition when nothing is filtered', () => { + const { sql, parameters } = compileFilters(NO_FILTERS); + // Not "no WHERE at all" as originally assumed: Kysely 0.28's `eb.and([])` + // compiles an empty conjunction to the truism `where 1 = 1` rather than + // omitting the clause (see parseFilterList in + // kysely/dist/cjs/parser/binary-operation-parser.js). That still matches + // every row, so it is the same "no filter" behaviour the old + // `clauses.length ? ... : ''` gave — the literal SQL text just differs + // from what was assumed here, which is what this assertion now checks. + expect(sql).toContain('where 1 = 1'); + expect(parameters).toEqual([]); }); it('matches a category and all of its descendants', () => { - const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 1, null); - expect(built.clauses.join(' ')).toContain('RECURSIVE'); - // One array parameter rather than one id: the CTE is seeded with ANY so - // several selected roots are walked in the same recursion. - expect(built.params).toEqual([[4]]); + const { sql, parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4] }); + expect(sql).toContain('WITH RECURSIVE subtree'); + expect(parameters).toEqual([[4]]); }); - it('seeds the descendant walk with every selected category', () => { - const built = buildItemFilterSql(parseItemFilters({ category: '4,9' }), 1, null); - const sql = built.clauses.join(' '); - expect(sql).toContain('RECURSIVE'); - // ANY over the seeds is what makes several categories combine as OR: the - // result is the union of their subtrees. - expect(sql).toContain('= ANY($1::int[])'); - expect(built.params).toEqual([[4, 9]]); + // One bind parameter holding the whole array, not a placeholder list. This is + // the property that made the array trap in the previous builder impossible + // here — see #297 and src/db-kysely/CONVENTIONS.md. + it('seeds the descendant walk with every selected category, as one parameter', () => { + const { parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4, 9] }); + expect(parameters).toEqual([[4, 9]]); }); it('requires every listed tag rather than any of them', () => { - const built = buildItemFilterSql(parseItemFilters({ tags: '1,2' }), 1, null); - // The count of matched tag rows must equal the number of tags requested — - // an ANY/IN match alone would return items carrying just one of them. - expect(built.clauses.join(' ')).toContain('COUNT(*)'); - expect(built.params).toEqual([[1, 2], 2]); + const { sql, parameters } = compileFilters({ ...NO_FILTERS, tagIds: [2, 5] }); + expect(sql).toContain('SELECT COUNT(*) FROM item_tags'); + expect(parameters).toEqual([[2, 5], 2]); }); - it('numbers placeholders from the given starting index', () => { - const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 3, null); - expect(built.clauses.join(' ')).toContain('$3'); + it('filters on a price range', () => { + const { parameters } = compileFilters({ + ...NO_FILTERS, + minPriceCents: 1000, + maxPriceCents: 5000 + }); + expect(parameters).toEqual([1000, 5000]); }); - it('filters on status', () => { - const built = buildItemFilterSql(parseItemFilters({ status: 'reserved' }), 1, null); - expect(built.clauses.join(' ')).toContain('i.status = ANY'); - expect(built.params).toEqual([['reserved']]); - }); - - // One clause for one status and for several, which is the whole reason the - // filter was generalised rather than joined by a second dimension. - it('filters on several statuses with the same single clause', () => { - const built = buildItemFilterSql(parseItemFilters({ status: 'available,reserved' }), 1, null); - expect(built.clauses).toHaveLength(1); - expect(built.clauses[0]).toContain('i.status = ANY'); - expect(built.params).toEqual([['available', 'reserved']]); + it('filters on several statuses with one expression', () => { + const { sql, parameters } = compileFilters({ + ...NO_FILTERS, + status: ['available', 'reserved'] + }); + expect(sql).toContain('"i"."status" in'); + expect(parameters).toEqual(['available', 'reserved']); }); it('restricts to the favorites of the given customer', () => { - const built = buildItemFilterSql(parseItemFilters({ favorites: '1' }), 1, 42); - expect(built.clauses.join(' ')).toContain('EXISTS'); - expect(built.clauses.join(' ')).toContain('favorites f'); - expect(built.params).toEqual([42]); + const { sql, parameters } = compileFilters({ ...NO_FILTERS, favoritesOnly: true }, 7); + expect(sql).toContain('exists'); + expect(parameters).toEqual([7]); }); it('does not restrict to favorites when the flag is off, even given a customer', () => { - const built = buildItemFilterSql(parseItemFilters({}), 1, 42); - expect(built.clauses).toEqual([]); - expect(built.params).toEqual([]); + const { sql, parameters } = compileFilters(NO_FILTERS, 7); + expect(sql).not.toContain('exists'); + expect(parameters).toEqual([]); }); - // Both routes reject this before reaching the builder, so it can only happen - // through a new caller that forgot to. Failing loudly beats dropping the - // clause and returning the whole catalogue as if it were someone's favorites. it('throws rather than ignore a favorites filter with no customer', () => { - expect(() => buildItemFilterSql(parseItemFilters({ favorites: '1' }), 1, null)).toThrow(); - }); - - it('continues numbering across multiple filters', () => { - const built = buildItemFilterSql( - parseItemFilters({ category: '4', min_price: '100', max_price: '900' }), - 1, - null + expect(() => compileFilters({ ...NO_FILTERS, favoritesOnly: true }, null)).toThrow( + /favorites filter requires a customer id/ ); - expect(built.params).toEqual([[4], 100, 900]); - const sql = built.clauses.join(' '); - expect(sql).toContain('$1'); - expect(sql).toContain('$2'); - expect(sql).toContain('$3'); - }); -}); - -// Both callers splice these clauses straight into query text, so a value -// reaching the clause string is SQL injection rather than a style problem. The -// comment on buildItemFilterSql says so; these two make it fail a build instead -// of relying on someone reading it. See #202, and #180 for the S2077 review. -describe('buildItemFilterSql keeps every value out of the SQL text', () => { - // Deliberately built by hand rather than through parseItemFilters, because - // the claim is that the clause literals are safe with no parser at all. These - // values could never survive parsing, which is the point: the parser is - // defence in depth, not the reason this holds. - const HOSTILE = "1); DROP TABLE items; --"; - const hostileFilters = { - categoryIds: [HOSTILE], - tagIds: [HOSTILE], - minPriceCents: HOSTILE, - maxPriceCents: HOSTILE, - status: [HOSTILE], - favoritesOnly: true - } as unknown as Parameters[0]; - - it('never lets a filter value reach a clause, even one the parser would reject', () => { - const built = buildItemFilterSql(hostileFilters, 1, HOSTILE as unknown as number); - const sql = built.clauses.join(' AND '); - - expect(sql).not.toContain(HOSTILE); - expect(sql).not.toContain('DROP TABLE'); - // Every value still arrives, bound, where it can do nothing. - expect(built.params).toContain(HOSTILE); }); - // The structural version of the same claim, and the one that catches a value - // which happens not to look hostile: the SQL text must not depend on the - // values at all. Two disjoint sets of inputs, byte-identical clauses. - it('produces byte-identical SQL for two completely different filter sets', () => { - const a = buildItemFilterSql( - parseItemFilters({ category: '4', tags: '7,8', min_price: '100', max_price: '900', status: 'sold' }), - 1, - 42 - ); - const b = buildItemFilterSql( - parseItemFilters({ category: '99', tags: '11,12', min_price: '5', max_price: '6', status: 'available' }), - 1, + it('composes several filters together', () => { + const { parameters } = compileFilters( + { + categoryIds: [4], + tagIds: [2], + minPriceCents: 1000, + maxPriceCents: null, + status: ['available'], + favoritesOnly: true + }, 7 ); + expect(parameters).toEqual([[4], [2], 1, 1000, 'available', 7]); + }); +}); - expect(a.clauses).toEqual(b.clauses); - expect(a.params).not.toEqual(b.params); +// The invariant, and it is load-bearing: the storefront call site is reachable +// without signing in, so a filter value reaching the SQL text is SQL injection +// rather than a style problem. These two made that fail a build rather than +// relying on someone reading a comment, and they still do — but they now check +// the SQL Kysely actually emits rather than the strings the old builder +// returned. See #202, #180 for the S2077 review, and #308 for the conversion. +describe('itemFilterExpressions keeps every value out of the SQL text', () => { + // Built by hand rather than through parseItemFilters, because the claim is + // that the expressions are safe with no parser at all. These values could + // never survive parsing, which is the point: the parser is defence in depth, + // not the reason this holds. + const HOSTILE = "1); DROP TABLE items; --"; + + it('never lets a filter value reach the SQL, even one the parser would reject', () => { + const { sql, parameters } = compileFilters( + { + categoryIds: [HOSTILE], + tagIds: [HOSTILE], + minPriceCents: HOSTILE, + maxPriceCents: HOSTILE, + status: [HOSTILE], + favoritesOnly: true + } as unknown as ItemFilters, + HOSTILE as unknown as number + ); + + expect(sql).not.toContain('DROP TABLE'); + expect(JSON.stringify(parameters)).toContain('DROP TABLE'); + }); + + it('produces byte-identical SQL for two completely different filter sets', () => { + const first = compileFilters( + { + categoryIds: [1], + tagIds: [2], + minPriceCents: 3, + maxPriceCents: 4, + status: ['available'], + favoritesOnly: true + }, + 5 + ); + const second = compileFilters( + { + categoryIds: [99], + tagIds: [98], + minPriceCents: 97, + maxPriceCents: 96, + status: ['sold'], + favoritesOnly: true + }, + 95 + ); + + expect(first.sql).toBe(second.sql); + expect(first.parameters).not.toEqual(second.parameters); }); }); From 306db819669e1d68bc3b09aa8a83d2370f715515 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 17:08:09 -0500 Subject: [PATCH 4/5] fix(db): narrow the status column instead of asserting the row (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.$castTo()` / `.$castTo()` at the two list-query call sites replaced the entire result type with an assertion rather than narrowing the one column that actually disagreed, which meant a projection that silently lost a column would still compile — exactly the failure mode this task exists to close, and the opposite of what the commit body claims. Fixed at the source instead: `itemSelect.ts` now defines `ItemsWithStatus`/`ItemDB`, narrowing `items.status` from the schema mirror's `Generated` (a CHECK-constrained text column, so `kysely-codegen` has no literal union to give it) to `Generated`, and builds `ItemContext`, `adminItemQuery()` and `publicItemQuery()` from an `itemDb` typed with `ItemDB` instead of `db`/`DB`. Both `$castTo` calls and their comments are gone; the `AdminItemRow[]` / `PublicItemRow[]` annotations at the two call sites now check for real. Verified by temporarily dropping a column from `adminItemQuery`'s projection: the `AdminItemRow[]` assignment failed to compile as expected, confirming the guarantee actually holds. Also corrected two now-false statements left over from the conversion: `db-kysely/CONVENTIONS.md`'s worked-example section said `buildItemFilterSql` was "still raw `pg`" and that converting it "would put a second copy of a live function in `src/` that nothing calls" — both untrue since #308 shipped it as `itemFilterExpressions`. And two doc comments in `itemSelect.ts` still named the deleted `PUBLIC_ITEM_SELECT`/`ADMIN_ITEM_SELECT` constants instead of the functions that replaced them. Co-Authored-By: Claude Opus 5 --- backend/src/db-kysely/CONVENTIONS.md | 2 +- backend/src/itemSelect.ts | 27 +++++++++++++++++++++------ backend/src/routes/admin.ts | 6 ------ backend/src/routes/items.ts | 6 ------ 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/backend/src/db-kysely/CONVENTIONS.md b/backend/src/db-kysely/CONVENTIONS.md index 891ae45..ae4fc60 100644 --- a/backend/src/db-kysely/CONVENTIONS.md +++ b/backend/src/db-kysely/CONVENTIONS.md @@ -37,7 +37,7 @@ Kysely uses the `pg` driver directly, so a Postgres SQLSTATE stays on `err.code` ## The worked example -`buildItemFilterSql` is the hardest query in the codebase — six optional clauses composed at run time, a recursive CTE for the category subtree, an `ANY(...::int[])` tag match with a count equality, and array parameters. It was the #216 spike's target and #297's, and it is still raw `pg` in `src/itemFilters.ts`, used by both the storefront and the admin listing. The conversion below is an example rather than a source file: committing it as one would put a second copy of a live function in `src/` that nothing calls, which is precisely what the #216 spike became. +`buildItemFilterSql` was the hardest query in the codebase — six optional clauses composed at run time, a recursive CTE for the category subtree, an `ANY(...::int[])` tag match with a count equality, and array parameters. It was the #216 spike's target and #297's, and #308 converted it: it is now `itemFilterExpressions` in `src/itemFilters.ts`, returning Kysely expressions that the storefront and admin listings compose with `eb.and`. What follows is the shape it took, kept here because it is the worked reference for converting anything else of that difficulty. ```ts if (filters.categoryIds.length) { diff --git a/backend/src/itemSelect.ts b/backend/src/itemSelect.ts index ab5d487..ab24510 100644 --- a/backend/src/itemSelect.ts +++ b/backend/src/itemSelect.ts @@ -21,12 +21,27 @@ // aggregate independent and drop the GROUP BY entirely. `jsonArrayFrom` emits // `coalesce(json_agg(agg), '[]')`, which is what these hand-wrote before. -import { ExpressionBuilder } from 'kysely'; +import { ExpressionBuilder, Generated, Kysely } from 'kysely'; import { jsonArrayFrom } from 'kysely/helpers/postgres'; import { db } from './db'; import { DB } from './db-kysely/schema'; import { ItemStatus, ItemImage, ItemTag } from './types'; +/** + * The mirror types `items.status` as `Generated` because Postgres holds + * it as CHECK-constrained text rather than a native enum, so `kysely-codegen` + * has nothing narrower to emit. `types.ts` already states the real domain. + * + * Narrowed here, once, rather than asserted at each call site. `$castTo` at the + * call site would have replaced the entire row type with an assertion — which + * would silently accept a projection that had lost a column, and losing the + * compile error on exactly that is what this change exists to prevent. + */ +type ItemsWithStatus = Omit & { status: Generated }; +type ItemDB = Omit & { items: ItemsWithStatus }; + +const itemDb = db as unknown as Kysely; + /** * The aliases every item query and every filter clause is written against. * @@ -36,7 +51,7 @@ import { ItemStatus, ItemImage, ItemTag } from './types'; * the builder would have made the diff unreadable against the SQL it replaces. */ export type ItemContext = ExpressionBuilder< - DB & { i: DB['items']; c: DB['categories'] }, + ItemDB & { i: ItemDB['items']; c: ItemDB['categories'] }, 'i' | 'c' >; @@ -92,7 +107,7 @@ function tagsFor(eb: ItemContext) { * obvious that adding a `where` does not affect anyone else. */ export function publicItemQuery() { - return db + return itemDb .selectFrom('items as i') .leftJoin('categories as c', 'c.id', 'i.category_id') .select([ @@ -111,7 +126,7 @@ export function publicItemQuery() { /** The admin projection — every item column, plus the admin image fields. */ export function adminItemQuery() { - return db + return itemDb .selectFrom('items as i') .leftJoin('categories as c', 'c.id', 'i.category_id') .selectAll('i') @@ -135,7 +150,7 @@ interface ItemRowBase { tags: ItemTag[]; } -/** What PUBLIC_ITEM_SELECT returns. Deliberately no payment or reservation columns. */ +/** What publicItemQuery returns. Deliberately no payment or reservation columns. */ export type PublicItemRow = ItemRowBase; /** @@ -148,7 +163,7 @@ export interface AdminItemImage extends ItemImage { } /** - * What ADMIN_ITEM_SELECT returns: `i.*`, so every column on the table. + * What adminItemQuery returns: `i.*`, so every column on the table. * * The extra fields are the ones the storefront is not allowed to see, which is * the whole reason the two selects differ. `images` is narrowed rather than diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 7f416c0..d29b20e 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -137,15 +137,9 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => { // sixteen lines in itemFilters.ts explained why that was safe. The clauses // are Kysely expressions now: a value cannot reach the SQL text, because the // types do not let it. - // `.$castTo` narrows `status` from the schema mirror's generic `string` (the - // column is a CHECK-constrained text column, not a native Postgres enum, so - // kysely-codegen has no literal union to give it) to the app-level - // `ItemStatus` the CHECK constraint actually enforces. Every other field on - // AdminItemRow already matches the projection without a cast. const rows: AdminItemRow[] = await adminItemQuery() .where((eb) => eb.and(itemFilterExpressions(eb, filters, null))) .orderBy('i.created_at', 'desc') - .$castTo() .execute(); res.json(rows); diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index 5e1e8c4..c94064b 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -79,11 +79,6 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { status: filters.status ?? [...defaultStatuses] }; - // `.$castTo` narrows `status` from the schema mirror's generic `string` (the - // column is a CHECK-constrained text column, not a native Postgres enum, so - // kysely-codegen has no literal union to give it) to the app-level - // `ItemStatus` the CHECK constraint actually enforces. Every other field on - // PublicItemRow already matches the projection without a cast. const rows: PublicItemRow[] = await publicItemQuery() .where((eb) => eb.and([ @@ -92,7 +87,6 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { ]) ) .orderBy('i.created_at', 'desc') - .$castTo() .execute(); res.json(rows); From abe8ac8184dd825e355d7244f5985401b1f5cd49 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 17:17:29 -0500 Subject: [PATCH 5/5] test(filters): merge the duplicate itemFilters import (#308) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch. Co-Authored-By: Claude Opus 5 --- backend/src/routes/items.ts | 8 ++++++++ backend/tests/unit/itemFilters.test.ts | 3 +-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/backend/src/routes/items.ts b/backend/src/routes/items.ts index c94064b..6cb826c 100755 --- a/backend/src/routes/items.ts +++ b/backend/src/routes/items.ts @@ -93,6 +93,14 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => { })); router.get('/:id', asyncRoute(async (req: Request, res: Response) => { + // Number() rather than readId(), and that is deliberate rather than an + // oversight. readId would be stricter and would match every other id-taking + // route (#207) — but errorHandling.integration.test.ts drives this exact + // route with a non-numeric id to prove that asyncRoute plus the error + // middleware answer 500 rather than leaving the request hanging, and a + // stricter parse here would leave that test green while removing the thing + // it tests. Switching this over means giving that test another trigger in + // the same change. See #307. const rows = await publicItemQuery() .where('i.id', '=', Number(req.params.id)) .where((eb) => notPending(eb)) diff --git a/backend/tests/unit/itemFilters.test.ts b/backend/tests/unit/itemFilters.test.ts index 4986357..eda7a28 100644 --- a/backend/tests/unit/itemFilters.test.ts +++ b/backend/tests/unit/itemFilters.test.ts @@ -1,6 +1,5 @@ -import { parseItemFilters, FilterError } from '../../src/itemFilters'; +import { parseItemFilters, FilterError, itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; import { db } from '../../src/db'; -import { itemFilterExpressions, ItemFilters } from '../../src/itemFilters'; describe('parseItemFilters', () => { it('returns empty filters for an empty query', () => {