bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

spike(db): finish the Drizzle conversion of the dynamic queries, or reconsider the builder #297

New Issue
Closed
opened 2026-09-04 12:22:29 -05:00 by bermudalamb · 2 comments
bermudalamb commented 2026-09-04 12:22:29 -05:00
Owner
Copy Link
Copy Source

#294 removed interpolation from seven query call sites by making each fixed-shape query a named constant. That was cheap and needed no dependency, and it did nothing for the two queries that genuinely build SQL at run time:

  • backend/src/routes/admin.ts:146 — `${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`

Those are the only places S2077 has a real point, because where is composed at run time by buildItemFilterSql. They are safe today, and the argument for why is written out at admin.ts:132-143: the clause fragments are string literals in itemFilters.ts, the only interpolations inside them are placeholder indices ($${next}), and every value goes in params. That is a sound argument, and it is still an argument — something a reader has to follow and a future edit could quietly break.

The thing that makes this worth doing now

backend/src/db-drizzle/itemFilters.drizzle.ts already exists and targets exactly this. From the #216 spike:

Deliberately the hardest thing 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. If this cannot be said cleanly, nothing else in the conversion matters.

And on the property that matters here:

Note what that template does with ${filters.categoryIds}: it emits a BIND PARAMETER, not text. That is the difference from a plain JS template literal, and it is the whole of the #202 invariant expressed by the type system rather than by a comment — there is no way to spell "interpolate this as text".

Nothing imports it. So the hard part may already be solved and sitting unused.

Kysely

Raised as an option, and it is a fair one — it is a good library and its fluent API is closer to LINQ-to-SQL than Drizzle's schema-DSL style, which matters if that shape is easier to read and review here.

The honest counterweight is that adopting it would mean a third way of writing queries in this codebase: raw pg, Drizzle, and Kysely. Drizzle is already a dependency (drizzle-orm ^0.45.2, drizzle-kit ^0.31.10), db.ts shares its pool, adminCategories.ts is fully converted, and drizzleSchema.integration.test.ts guards the generated mirror against drift. So the real comparison is not "a builder versus string interpolation" — it is "finish what is started versus switch", and switching needs a reason beyond syntax preference.

What to establish

  • Does the existing spike actually work? Run buildItemFilterDrizzle against the same cases itemFilters' tests cover and see whether the SQL and the results match. If it does not, that is the answer about Drizzle for this problem.
  • What the two dynamic call sites look like converted, in both builders, side by side. Small enough to write twice.
  • Whether either removes the S2077 hotspot or merely moves it. A builder that still emits sql\`` templates for the recursive CTE may not.
  • What #219 decided about migrations. drizzle.config.ts still says "It is a queries-only mirror today" and "If #219 ever chooses generated migrations, they also land in out, and this will need splitting then." A builder decision that ignores the migration story is half a decision.
  • Cost of a third dialect if Kysely wins: whether Drizzle then gets removed, and what that does to adminCategories.ts and the mirror guard.

Not in scope

Converting anything. This produces a recommendation and the evidence for it — a short document under docs/, like the rembg evaluation — not a migration.

Refs #294, #216, #217, #219

#294 removed interpolation from seven query call sites by making each fixed-shape query a named constant. That was cheap and needed no dependency, and it did nothing for the two queries that genuinely build SQL at run time: - `backend/src/routes/admin.ts:146` — `` `${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` `` Those are the only places S2077 has a real point, because `where` is composed at run time by `buildItemFilterSql`. They are safe today, and the argument for why is written out at `admin.ts:132-143`: the clause fragments are string literals in `itemFilters.ts`, the only interpolations inside them are placeholder *indices* (`$${next}`), and every value goes in `params`. That is a sound argument, and it is still an argument — something a reader has to follow and a future edit could quietly break. ## The thing that makes this worth doing now **`backend/src/db-drizzle/itemFilters.drizzle.ts` already exists** and targets exactly this. From the #216 spike: > Deliberately the hardest thing 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. If this cannot be said cleanly, nothing else in the conversion matters. And on the property that matters here: > Note what that template does with `${filters.categoryIds}`: it emits a BIND PARAMETER, not text. That is the difference from a plain JS template literal, and it is the whole of the #202 invariant expressed by the type system rather than by a comment — there is no way to spell "interpolate this as text". Nothing imports it. So the hard part may already be solved and sitting unused. ## Kysely Raised as an option, and it is a fair one — it is a good library and its fluent API is closer to LINQ-to-SQL than Drizzle's schema-DSL style, which matters if that shape is easier to read and review here. The honest counterweight is that adopting it would mean a **third** way of writing queries in this codebase: raw `pg`, Drizzle, and Kysely. Drizzle is already a dependency (`drizzle-orm ^0.45.2`, `drizzle-kit ^0.31.10`), `db.ts` shares its pool, `adminCategories.ts` is fully converted, and `drizzleSchema.integration.test.ts` guards the generated mirror against drift. So the real comparison is not "a builder versus string interpolation" — it is "finish what is started versus switch", and switching needs a reason beyond syntax preference. ## What to establish - **Does the existing spike actually work?** Run `buildItemFilterDrizzle` against the same cases `itemFilters`' tests cover and see whether the SQL and the results match. If it does not, that is the answer about Drizzle for this problem. - **What the two dynamic call sites look like converted**, in both builders, side by side. Small enough to write twice. - **Whether either removes the S2077 hotspot** or merely moves it. A builder that still emits `sql\`\`` templates for the recursive CTE may not. - **What #219 decided about migrations.** `drizzle.config.ts` still says *"It is a queries-only mirror today"* and *"If #219 ever chooses generated migrations, they also land in `out`, and this will need splitting then."* A builder decision that ignores the migration story is half a decision. - **Cost of a third dialect** if Kysely wins: whether Drizzle then gets removed, and what that does to `adminCategories.ts` and the mirror guard. ## Not in scope Converting anything. This produces a recommendation and the evidence for it — a short document under `docs/`, like the rembg evaluation — not a migration. Refs #294, #216, #217, #219
bermudalamb commented 2026-09-04 14:52:12 -05:00
Author
Owner
Copy Link
Copy Source

Answered, and the answer is the second branch of the title: reconsider the builder. Decision doc on feature/297-kysely-vs-drizzle at docs/superpowers/specs/2026-09-04-kysely-vs-drizzle.md, with the spike beside it.

It was not answered from documentation. Kysely 0.28.17 was run against the same query #216 used to judge Drizzle — buildItemFilterSql, with six optional clauses composed at run time, a recursive CTE, an ANY(...::int[]) tag match with a count equality, and array parameters — and the doc quotes the SQL it actually emitted. Three findings, all of which are properties of Drizzle rather than of type-safe query building, and two of which are the silent kind.

An array interpolates as one bind parameter with no sql.param() ceremony, so the trap CONVENTIONS.md calls "the rule that will bite you" does not exist. A column reference inside a raw fragment is the text you wrote, so the correlated-subquery rewrite that returned a quietly wrong count in #218 cannot happen. And the generated types carry the database's own snake_case names, so the explicit column mapping that exists to stop a select silently changing the JSON contract is not needed at all.

The property that motivated the whole exercise is identical either way: a hostile value lands in the parameters, not the SQL text, so #202's invariant becomes a type-system property and #180's hotspots retire regardless of which library wins.

What decided it is how little is actually built — one converted file, adminCategories.ts, three calls, against 238 raw query sites.

Q&A from the discussion

Q: I like Drizzle for the migration management and prefer Kysely for the better LINQ-to-SQL syntax. Can we work them that way?

The premise needed correcting: Drizzle is not doing migration management here. drizzle.config.ts is pull-only and says so in its own comment — "It is a queries-only mirror today". Migrations are nine hand-written node-pg-migrate files, and migrate:up/down/create never touch Drizzle. #219 measured drizzle-kit generate and refused it, because one nullable column emitted six statements including three index rebuilds, generated SQL carries none of the prose these migrations are mostly made of, and data migrations cannot be generated at all.

So what Drizzle actually provides is codegen — introspecting the live database into a typed mirror — and kysely-codegen does the identical job. The split asked for already exists; only the labels were off. Hand-written migrations stay, and they were never Drizzle's.

Keeping drizzle-kit purely for pull was considered and rejected: it emits pgTable(...) definitions that Kysely cannot consume, and bridging them through $inferSelect would carry the mirror's camelCase into the query layer, reintroducing the mapping problem that is one of the three reasons to move. Two codegen tools, two mirrors, two drift tests, and a third of the benefit gone.

Decision: node-pg-migrate for schema, kysely-codegen for types, Kysely for queries, Drizzle removed. #219 is untouched. The conversion is separate work with its own issue.

Answered, and the answer is the second branch of the title: reconsider the builder. Decision doc on `feature/297-kysely-vs-drizzle` at `docs/superpowers/specs/2026-09-04-kysely-vs-drizzle.md`, with the spike beside it. It was not answered from documentation. Kysely 0.28.17 was run against the same query #216 used to judge Drizzle — `buildItemFilterSql`, with six optional clauses composed at run time, a recursive CTE, an `ANY(...::int[])` tag match with a count equality, and array parameters — and the doc quotes the SQL it actually emitted. Three findings, all of which are properties of Drizzle rather than of type-safe query building, and two of which are the silent kind. An array interpolates as one bind parameter with no `sql.param()` ceremony, so the trap `CONVENTIONS.md` calls "the rule that will bite you" does not exist. A column reference inside a raw fragment is the text you wrote, so the correlated-subquery rewrite that returned a quietly wrong count in #218 cannot happen. And the generated types carry the database's own snake_case names, so the explicit column mapping that exists to stop a select silently changing the JSON contract is not needed at all. The property that motivated the whole exercise is identical either way: a hostile value lands in the parameters, not the SQL text, so #202's invariant becomes a type-system property and #180's hotspots retire regardless of which library wins. What decided it is how little is actually built — one converted file, `adminCategories.ts`, three calls, against 238 raw query sites. ## Q&A from the discussion **Q: I like Drizzle for the migration management and prefer Kysely for the better LINQ-to-SQL syntax. Can we work them that way?** The premise needed correcting: Drizzle is not doing migration management here. `drizzle.config.ts` is `pull`-only and says so in its own comment — "It is a queries-only mirror today". Migrations are nine hand-written `node-pg-migrate` files, and `migrate:up/down/create` never touch Drizzle. #219 measured `drizzle-kit generate` and refused it, because one nullable column emitted six statements including three index rebuilds, generated SQL carries none of the prose these migrations are mostly made of, and data migrations cannot be generated at all. So what Drizzle actually provides is codegen — introspecting the live database into a typed mirror — and `kysely-codegen` does the identical job. The split asked for already exists; only the labels were off. Hand-written migrations stay, and they were never Drizzle's. Keeping `drizzle-kit` purely for `pull` was considered and rejected: it emits `pgTable(...)` definitions that Kysely cannot consume, and bridging them through `$inferSelect` would carry the mirror's camelCase into the query layer, reintroducing the mapping problem that is one of the three reasons to move. Two codegen tools, two mirrors, two drift tests, and a third of the benefit gone. **Decision:** `node-pg-migrate` for schema, `kysely-codegen` for types, Kysely for queries, Drizzle removed. #219 is untouched. The conversion is separate work with its own issue.
bermudalamb commented 2026-09-04 16:11:02 -05:00
Author
Owner
Copy Link
Copy Source

Merged in #304, and the conversion it decided on is merged in #306. Closing manually — the Closes #297 line was in the commit body, but the rebase-merge rewrote the SHAs and Gitea did not pick it up.

Answered as the second branch of the title: reconsider the builder. main now has node-pg-migrate for the schema, kysely-codegen for types, Kysely for queries, and no Drizzle anywhere — no dependency, no lockfile entry, no import, no config, no directory. #219 is untouched.

Verified on main after the merge: backend build clean, lint 0 errors, unit 466/466, integration 445/445.

Two things worth recording for whoever picks up the remaining conversion.

The drift guard is real, and that was demonstrated rather than assumed. schemaMirror.integration.test.ts was initially ported with a column check that built one flat set of every column name in the generated file, which would have passed a migration adding created_at — declared on fourteen of the eighteen tables — to a table that lacked it. It is now keyed per table through the DB interface, and it was proved to fail by deleting carts.customer_id from the mirror and watching it name exactly that column.

The other one is the class of bug this whole exercise is about. eslint.config.mjs still listed the two deleted src/db-drizzle/ files in its ignores and did not list the file that replaced them, so the generated mirror was being linted for the first time — which the config's own comment explains is exactly what not to do, because #261 hand-fixed a warning in generated output and #217's regeneration put it straight back. Nothing failed; it just quietly started doing the wrong thing. Worth checking the same way after any future generator change.

Merged in #304, and the conversion it decided on is merged in #306. Closing manually — the `Closes #297` line was in the commit body, but the rebase-merge rewrote the SHAs and Gitea did not pick it up. Answered as the second branch of the title: reconsider the builder. `main` now has `node-pg-migrate` for the schema, `kysely-codegen` for types, Kysely for queries, and no Drizzle anywhere — no dependency, no lockfile entry, no import, no config, no directory. #219 is untouched. Verified on `main` after the merge: backend build clean, lint 0 errors, unit 466/466, integration 445/445. Two things worth recording for whoever picks up the remaining conversion. The drift guard is real, and that was demonstrated rather than assumed. `schemaMirror.integration.test.ts` was initially ported with a column check that built one flat set of every column name in the generated file, which would have passed a migration adding `created_at` — declared on fourteen of the eighteen tables — to a table that lacked it. It is now keyed per table through the `DB` interface, and it was proved to fail by deleting `carts.customer_id` from the mirror and watching it name exactly that column. The other one is the class of bug this whole exercise is about. `eslint.config.mjs` still listed the two deleted `src/db-drizzle/` files in its ignores and did not list the file that replaced them, so the generated mirror was being linted for the first time — which the config's own comment explains is exactly what not to do, because #261 hand-fixed a warning in generated output and #217's regeneration put it straight back. Nothing failed; it just quietly started doing the wrong thing. Worth checking the same way after any future generator change.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#297