2026-09-04 14:40:34 -05:00
2 changed files with 253 additions and 0 deletions
@@ -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<DB>({
  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<DB, 'items'>();
  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<SqlBool>`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<SqlBool>`(
      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<number>`(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<SqlBool>`items.id = ANY(${[] as number[]}::int[])`);
 show('empty array', emptyish.compile());
@@ -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.