Merge pull request 'docs(db): weigh Kysely against the Drizzle decision (#297)' (#304) from feature/297-kysely-vs-drizzle into main
Reviewed-on: #304
This commit was merged in pull request #304.
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user