From 7ca55eaa1a6bedb0de3cfe85d0019ba17d1cfa20 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Fri, 4 Sep 2026 14:23:22 -0500 Subject: [PATCH] docs(db): weigh Kysely against the Drizzle decision (#297) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The question is not whether Kysely is good, it is whether it is enough better for this codebase to reverse a decision already made in #216 and partly built in #217. That is a higher bar than being the nicer library, so this answers it against the same target #216 used: buildItemFilterSql, with six optional clauses composed at run time, a recursive CTE, an ANY(...::int[]) tag match with a count equality, and array parameters. Kysely compiles without a connection, so the document quotes the SQL it actually emitted rather than a reading of its documentation. Three of the four hazards that src/db-drizzle/CONVENTIONS.md exists to warn about turn out to be properties of Drizzle rather than of type-safe query building, and two of them are the silent kind. An array interpolates as one bind parameter with no sql.param() ceremony, so the trap that document 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 unchanged: a hostile value lands in the parameters either way, so #202's invariant becomes a type-system property and #180's hotspots retire either way. What decides it is how little is actually built. One file is converted — adminCategories.ts, three calls — against 238 raw query sites, and the generated mirror and its drift test are things any builder needs an equivalent of. The recommendation is to switch now, while the cost is reconverting one file and rewriting a conventions document that gets substantially shorter. The counter-argument is recorded rather than hidden: Drizzle is more widely used, and #219's migration reasoning was measured against drizzle-kit specifically. That reasoning survives, because losing the prose and being unable to express data migrations are true of any generator, and Kysely simply has nothing to refuse. Co-Authored-By: Claude Opus 5 --- .../specs/2026-09-04-kysely-spike.ts | 132 ++++++++++++++++++ .../specs/2026-09-04-kysely-vs-drizzle.md | 121 ++++++++++++++++ 2 files changed, 253 insertions(+) create mode 100644 docs/superpowers/specs/2026-09-04-kysely-spike.ts create mode 100644 docs/superpowers/specs/2026-09-04-kysely-vs-drizzle.md diff --git a/docs/superpowers/specs/2026-09-04-kysely-spike.ts b/docs/superpowers/specs/2026-09-04-kysely-spike.ts new file mode 100644 index 0000000..699da2f --- /dev/null +++ b/docs/superpowers/specs/2026-09-04-kysely-spike.ts @@ -0,0 +1,132 @@ +// Kysely spike for #297. Deliberately the same target #216 set for Drizzle: +// buildItemFilterSql — 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. +// +// Nothing here connects to a database. Kysely compiles to { sql, parameters } +// without a connection, which is the whole point: the questions being asked are +// about what SQL comes out and where the values end up. + +import { Kysely, PostgresDialect, sql, SqlBool, expressionBuilder } from 'kysely'; + +// What kysely-codegen would generate, hand-written for the four tables this +// query touches. Note the names: snake_case, exactly as the database spells +// them and exactly as these APIs answer. +interface DB { + items: { + id: number; + category_id: number | null; + price_cents: number; + status: string; + }; + item_tags: { item_id: number; tag_id: number }; + categories: { id: number; parent_id: number | null }; + favorites: { customer_id: number; item_id: number }; +} + +const db = new Kysely({ + dialect: new PostgresDialect({ pool: {} as never }) +}); + +interface SpikeFilters { + categoryIds: number[]; + tagIds: number[]; + minPriceCents: number | null; + maxPriceCents: number | null; + status: string[] | null; + favoritesOnly: boolean; +} + +function buildItemFilterKysely(filters: SpikeFilters, favoritesCustomerId: number | null) { + const eb = expressionBuilder(); + const clauses = []; + + // The recursive CTE, inside an IN (...) subquery. + // + // ${filters.categoryIds} emits a BIND PARAMETER, and — the question this + // spike exists to answer — it emits ONE parameter for the whole array, not a + // placeholder list. No sql.param() equivalent is needed. + if (filters.categoryIds.length) { + clauses.push(sql`items.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 + )`); + } + + // AND, not OR: the item must carry every selected tag. + if (filters.tagIds.length) { + clauses.push(sql`( + SELECT COUNT(*) FROM item_tags it + WHERE it.item_id = items.id AND it.tag_id = ANY(${filters.tagIds}::int[]) + ) = ${filters.tagIds.length}`); + } + + if (filters.minPriceCents !== null) clauses.push(eb('items.price_cents', '>=', filters.minPriceCents)); + if (filters.maxPriceCents !== null) clauses.push(eb('items.price_cents', '<=', filters.maxPriceCents)); + if (filters.status !== null) clauses.push(eb('items.status', 'in', filters.status)); + + if (filters.favoritesOnly && favoritesCustomerId !== null) { + clauses.push( + eb.exists( + eb + .selectFrom('favorites') + .select('favorites.item_id') + .whereRef('favorites.item_id', '=', 'items.id') + .where('favorites.customer_id', '=', favoritesCustomerId) + ) + ); + } + + return clauses; +} + +function show(label: string, compiled: { sql: string; parameters: readonly unknown[] }) { + console.log(`\n=== ${label} ===`); + console.log(compiled.sql.replace(/\s+/g, ' ').trim()); + console.log('parameters:', JSON.stringify(compiled.parameters)); +} + +const filters: SpikeFilters = { + categoryIds: [3, 7], + tagIds: [11, 12], + minPriceCents: 1000, + maxPriceCents: 50000, + status: ['available', 'reserved'], + favoritesOnly: true +}; + +const query = db + .selectFrom('items') + .select(['items.id', 'items.status', 'items.price_cents']) + .where((eb) => eb.and(buildItemFilterKysely(filters, 42))); + +show('every clause at once', query.compile()); + +// Question 2: what does a hostile value do? #202's invariant is that only +// placeholder indices may reach the SQL text. +const hostile = db + .selectFrom('items') + .select('items.id') + .where('items.status', '=', "1); DROP TABLE items; --"); +show('hostile value in a status filter', hostile.compile()); + +// Question 3: the trap that cost #218 a silently wrong count in Drizzle — a +// correlated subquery referencing a column of the outer table. +const correlated = db + .selectFrom('categories') + .select([ + 'categories.id', + sql`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)`.as('item_count') + ]); +show('correlated subquery', correlated.compile()); + +// Question 4: does an empty array still produce one parameter? +const emptyish = db + .selectFrom('items') + .select('items.id') + .where(sql`items.id = ANY(${[] as number[]}::int[])`); +show('empty array', emptyish.compile()); diff --git a/docs/superpowers/specs/2026-09-04-kysely-vs-drizzle.md b/docs/superpowers/specs/2026-09-04-kysely-vs-drizzle.md new file mode 100644 index 0000000..18b31a2 --- /dev/null +++ b/docs/superpowers/specs/2026-09-04-kysely-vs-drizzle.md @@ -0,0 +1,121 @@ +# Kysely against the Drizzle decision + +**Issue:** #297. Revisits #216, which chose Drizzle, and #217/#219, which landed it. + +The question is not "is Kysely good". It is whether Kysely is enough better, for this codebase specifically, to reverse a decision that is already made and partly built. That is a higher bar than being the nicer library, and this document answers it against the same target #216 used. + +## What is actually built today + +Worth stating precisely, because the answer turns on it. + +| Piece | State | +|---|---| +| `drizzle-orm` 0.45, `drizzle-kit` 0.31 | Installed | +| `src/db-drizzle/schema.ts`, `relations.ts` | Generated mirror of the migrations | +| `src/db-drizzle/itemFilters.drizzle.ts` | The #216 spike. Never imported by anything. | +| `src/db-drizzle/CONVENTIONS.md` | Written, and mostly a list of traps | +| `drizzleSchema.integration.test.ts` | Guards the mirror against drift | +| `src/routes/adminCategories.ts` | **The only converted file.** Three `db.` calls. | + +Against **238** `pool.query` / `client.query` call sites in `src/`. + +So the sunk cost is one converted file, a generated mirror that any query builder needs an equivalent of, and a drift test whose rationale is library-independent. That is a materially smaller commitment than "we have adopted Drizzle" suggests, and it is why this question is worth asking now rather than never. + +## The spike + +Kysely 0.28.17, in a scratch directory, against the same query #216 used to judge Drizzle. The spike is kept beside this document as `2026-09-04-kysely-spike.ts`; it is not part of any build and needs `kysely` installed to run, which is why it lives here rather than in `backend/src`. The query it expresses is: `buildItemFilterSql` — 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. Kysely compiles without a connection, so what follows is the SQL it actually emitted, not a reading of its documentation. + +Every clause at once: + +```sql +select "items"."id", "items"."status", "items"."price_cents" from "items" where ( + items.category_id IN ( WITH RECURSIVE subtree AS ( + SELECT id FROM categories WHERE id = ANY($1::int[]) + UNION ALL SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id + ) SELECT id FROM subtree ) + and ( SELECT COUNT(*) FROM item_tags it + WHERE it.item_id = items.id AND it.tag_id = ANY($2::int[]) ) = $3 + and "items"."price_cents" >= $4 and "items"."price_cents" <= $5 + and "items"."status" in ($6, $7) + and exists (select "favorites"."item_id" from "favorites" + where "favorites"."item_id" = "items"."id" and "favorites"."customer_id" = $8)) +``` +``` +parameters: [[3,7],[11,12],2,1000,50000,"available","reserved",42] +``` + +The query is expressible, and it reads about as well as the Drizzle version. That was expected. What matters is the three things underneath it. + +### 1. An array is one parameter, with no ceremony + +`ANY($1::int[])`, parameter `[3,7]`. Written as a plain `${filters.categoryIds}` interpolation. + +This is the trap CONVENTIONS.md calls "the rule that will bite you", and in Drizzle it is real: `${array}` emits a **placeholder list**, producing `ANY(($1, $2)::int[])`, which is invalid Postgres. The remedy is to remember `sql.param()` at every array site, and the wrong form type-checks and reads correctly. The document's own assessment is that "across 187 call sites this is exactly the shape of defect that passes review and breaks in production". + +In Kysely the trap does not exist. An empty array behaves too — `ANY($1::int[])` with `[[]]`. + +### 2. The silently-wrong-data trap does not exist either + +CONVENTIONS.md's worst entry, because it produces valid SQL and quiet corruption: Drizzle renders a column reference inside a `sql` template **without its table**, so a correlated subquery silently correlates with itself. It cost #218 a count that returned 1 where 2 was correct, caught only because an integration test asserted a value. + +Kysely emitted the correlated subquery exactly as written: + +```sql +(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id) as "item_count" +``` + +There is no rewriting to be surprised by, because column references in a raw fragment are text you wrote and qualified yourself. + +### 3. The snake_case mapping layer disappears + +CONVENTIONS.md requires every converted select to map columns explicitly — `{ parent_id: categories.parentId }` — because Drizzle's mirror is camelCase while these APIs answer snake_case, and selecting the table directly "changes the JSON contract silently, and no test asserting status codes notices". + +Kysely's generated types use the database's own names. `items.price_cents` is `items.price_cents` in the type, in the query, and in the response. The mapping step, and the class of silent contract break it exists to prevent, are both gone. + +### What is unchanged + +The property that motivated the whole exercise holds identically. A hostile status value: + +```sql +select "items"."id" from "items" where "items"."status" = $1 +``` +``` +parameters: ["1); DROP TABLE items; --"] +``` + +`${value}` is a bind parameter, never text, and the escape hatch that looks like a plain template literal does not behave like one. #202's invariant becomes a property of the type system either way, and #180's S2077 hotspots retire either way. Kysely is not better here; it is equal, which is the point — the strongest argument for the original decision is not weakened by changing library. + +## The comparison that matters + +| | Drizzle | Kysely | +|---|---|---| +| Values parameterized by default | Yes | Yes | +| Arrays | `sql.param()` required; wrong form is invalid SQL at run time | One parameter, no ceremony | +| Columns in a raw fragment | Silently unqualified — valid SQL, wrong data | Text as written | +| Generated types' naming | camelCase; explicit mapping required at every select | The database's own names | +| Schema mirror | `drizzle-kit pull`, manual refresh, drift test needed | `kysely-codegen`, manual refresh, drift test needed | +| Migrations | Generation exists and had to be refused in #219 | No generation to refuse | +| Coexists with raw `pg` on one pool | Yes | Yes — takes a `pg` Pool directly | +| Shape | ORM with a query-builder mode | Query builder only | + +Three of the four hazards CONVENTIONS.md exists to warn about are properties of Drizzle, not of type-safe query building. Two of them are the silent kind. + +There is also a smaller thing worth naming because it is what prompted the question: Kysely reads more like LINQ-to-SQL — `.selectFrom().select().where()` chaining over the database's own column names. That is a preference, not an argument, and it does not carry weight on its own. It happens to point the same way as the evidence. + +## Recommendation + +**Switch to Kysely, now, while one file is converted.** + +The decision in #216 was right about the thing it was deciding — that a type-safe builder should replace hand-assembled SQL, and that the safety property is real rather than relocated. Nothing here disturbs that. What #216 could not know is that Drizzle's own conventions document would end up being mostly a list of ways to be quietly wrong, two of which produce working code and bad data. + +The cost of switching is small and knowable: reconvert `adminCategories.ts` (three calls), replace the generated mirror and repoint the drift test, rewrite CONVENTIONS.md — which gets substantially shorter, since three of its four warnings stop applying. The cost of not switching is paid 237 more times, in a codebase where the failure mode is a review that passes. + +The honest counter-argument, recorded rather than hidden: Drizzle is more widely used, and #219's reasoning about hand-written migrations was measured against `drizzle-kit` specifically. That second point survives the change — the reasoning was that generated migrations lose the prose and cannot express data migrations, which is true of any generator, and Kysely simply has nothing to refuse. + +## Out of scope + +**Converting anything.** This is the decision; the conversion is separate work with its own issue, and it stays file-by-file with both drivers on one pool either way. + +**Revisiting #219.** Migrations stay hand-written in `node-pg-migrate`. Nothing here touches that. + +**Removing Drizzle before Kysely replaces it.** If this is accepted, the mirror, the drift test and `adminCategories.ts` move together in one change, so `main` is never half-converted between two builders.