2026-09-04 17:47:42 -05:00
1 changed files with 91 additions and 0 deletions
@@ -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<SqlBool>`, 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<T>` 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<AdminItemRow>()` 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<SqlBool>[]
                        ▼
 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<SqlBool>` template, recursive CTE unchanged, `${ids}` now one bind parameter |
 | tags AND-match | `sql<SqlBool>` 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.