redefined-designs/docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md

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.