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.