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

refactor(db): swap the query builder from Drizzle to Kysely (#305)

Browse Source 
One commit, because a main that carries both builders is one where the next person converting a query has to guess which to reach for, and where two generated mirrors of one database can disagree. There was nothing to stage anyway: one file used the builder.

The safety property that motivated adopting a builder at all is untouched, and was never the thing being traded. A value interpolated into a sql template becomes a bind parameter in either library, so #202's invariant stays a property of the type system and #180's hotspots retire either way. What changes is the three ways the old library made it easy to be quietly wrong, each verified in #297 against the SQL actually emitted: an array interpolating as a placeholder list unless every site remembered sql.param(), a column reference inside a raw fragment silently losing its table so a correlated subquery correlated with itself, and a camelCase mirror that had to be mapped back at every select or the JSON contract changed with no test noticing.

CATEGORY_COLUMNS stops being a translation layer and becomes what it looks like — four column names four selects share. The generated types carry parent_id and sort_order because kysely-codegen emits the database's own names, so there is nothing left to map and nothing left to get wrong by forgetting to.

The drift guard survives the swap rather than being rewritten, and loses its library name in the process: it is schemaMirror.integration.test.ts now, so the next such change renames nothing. It also got stricter for free. The Drizzle version had to match each column two ways and its own comment called that deliberately loose; a generated Kysely interface spells the database's name verbatim as a bare key, so one exact match is the whole rule and snakeToCamel is gone.

isUniqueViolation keeps accepting both error shapes and now has a test behind it. Kysely uses the pg driver directly and should leave the SQLSTATE on err.code, but "should" is the word that turned two 409s into 500s when the last conversion moved it to err.cause.code with nothing failing to compile.

Migrations are untouched. #219 stands, they remain hand-written node-pg-migrate files, and Kysely has no generator to refuse.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
bermudalamb
2026-09-04 16:08:47 -05:00
co-authored by Claude Opus 5
parent 22c51b6af7
commit c71b11e05e
12 changed files with 567 additions and 1903 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/adminCategories.ts
+75 -78
View File
@@ -1,38 +1,28 @@
import { Router, Request, Response } from 'express';
import { eq, inArray, sql } from 'drizzle-orm';
import { sql } from 'kysely';
import { db, requireRow } from '../db';
import { categories, items } from '../db-drizzle/schema';
import { asyncRoute } from '../asyncRoute';
/**
* The first file converted to Drizzle (#218), chosen because it is awkward
* rather than because it is easy — nine sites including a recursive CTE and an
* array match. See src/db-drizzle/CONVENTIONS.md.
* The one file using the builder (#218, reconverted for Kysely in #305), chosen
* because it is awkward rather than because it is easy — a recursive CTE, a
* correlated count, and an array match.
*
* The pool is still available and most of the application still uses it. This
* is one file moving, not a cutover.
* is one file converted, not a cutover. See src/db-kysely/CONVENTIONS.md.
*/
/**
* The response shape, written once.
* The four columns this API answers with, named once.
*
* The generated mirror names columns in camelCase — `parentId`, `sortOrder` —
* and this API answers in snake_case, which the admin frontend reads. So the
* mapping is explicit here rather than implicit anywhere: selecting the table
* directly would silently change the JSON contract, and no test that checks
* status codes would catch it.
*
* It also answers the question #218 asked. The row type is inferred from this
* object rather than hand-declared beside the query, so the interfaces that used
* to sit at the top of this file are gone and cannot drift from what is
* selected.
* Under Drizzle this was a mapping — `{ parent_id: categories.parentId }` — and
* it existed because the generated mirror was camelCase while this API answers
* snake_case, so selecting the table directly changed the JSON contract with no
* test noticing. The generated types now carry the database's own names, so
* there is nothing left to translate and this is just a list of columns four
* selects happen to share.
*/
const CATEGORY_COLUMNS = {
id: categories.id,
name: categories.name,
parent_id: categories.parentId,
sort_order: categories.sortOrder
};
const CATEGORY_COLUMNS = ['id', 'name', 'parent_id', 'sort_order'] as const;
const router = Router();
@@ -43,11 +33,13 @@ const UNIQUE_VIOLATION = '23505';
/**
* Whether a thrown error is that unique violation.
*
* Drizzle wraps driver errors, so the SQLSTATE that used to sit on `err.code`
* now sits on `err.cause.code`. The old check still compiled and simply never
* matched, turning two 409s into 500s — a conversion hazard with no type error
* and no failing build behind it, only two integration tests. Both shapes are
* accepted so this keeps working either side of a conversion. See #218.
* Both shapes are accepted deliberately. Drizzle wrapped driver errors, moving
* this SQLSTATE from `err.code` to `err.cause.code`, and the check that only
* looked at `err.code` still compiled, never matched, and turned two 409s into
* 500s — a conversion hazard with no type error behind it. Kysely uses the `pg`
* driver directly and is expected to leave it on `err.code`, but "expected" is
* the word that caused the bug last time, so the tolerant check stays and an
* integration test proves the 409 rather than assuming it. See #218, #305.
*/
function isUniqueViolation(err: unknown): boolean {
const direct = (err as { code?: string }).code;
@@ -59,12 +51,11 @@ function isUniqueViolation(err: unknown): boolean {
* Walks down from a node, collecting it and every descendant. Used both for
* cycle detection on reparent and for reporting the blast radius of a delete.
*
* Still a `sql` template. Drizzle has `$with()` for CTEs, but this one is
* recursive and is consumed in two different shapes, and expressing it through
* the builder bought nothing over the SQL that is already correct and reviewed.
* The important part is that `${id}` here is a bind parameter, not text — there
* is no way to spell string interpolation in this template by accident, which is
* the property the whole adoption is for.
* Still a `sql` template: the CTE is recursive and is consumed in two different
* shapes, and expressing it through the builder buys nothing over SQL that is
* already correct and reviewed. The important part is that `${id}` is a bind
* parameter, not text — there is no way to spell string interpolation in this
* template by accident, which is the property the whole adoption is for.
*/
const subtreeOf = (id: number) => sql`
WITH RECURSIVE subtree AS (
@@ -89,28 +80,32 @@ function readParentId(value: unknown): number | null | undefined {
}
async function parentExists(id: number): Promise<boolean> {
const rows = await db
.select({ id: categories.id })
.from(categories)
.where(eq(categories.id, id))
.limit(1);
return rows.length > 0;
const row = await db
.selectFrom('categories')
.select('id')
.where('id', '=', id)
.executeTakeFirst();
return row !== undefined;
}
router.get('/', asyncRoute(async (_req: Request, res: Response) => {
const rows = await db
.select({
...CATEGORY_COLUMNS,
// Written as literal SQL, NOT with ${items.categoryId} and
// ${categories.id}. Drizzle renders a column reference inside a sql
// template UNQUALIFIED — those two produced `WHERE "category_id" = "id"`,
// which Postgres resolved against items for both sides and answered with
// a plausible wrong number rather than an error. There are no values to
// bind in this fragment, so literal text is the honest form. See #218.
item_count: sql<number>`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)`
})
.from(categories)
.orderBy(categories.sortOrder, sql`lower(categories.name)`);
.selectFrom('categories')
.select(CATEGORY_COLUMNS)
// Literal text rather than interpolated column references, and here that is
// a free choice rather than a workaround: the fragment binds no values, so
// there is nothing to parameterize. Under Drizzle this had to be literal,
// because interpolating the columns rendered them unqualified and Postgres
// resolved both sides against items, answering with a plausible wrong
// number rather than an error (#218).
.select(
sql<number>`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)`.as(
'item_count'
)
)
.orderBy('sort_order')
.orderBy(sql`lower(categories.name)`)
.execute();
res.json(rows);
}));
@@ -134,9 +129,10 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
try {
const rows = await db
.insert(categories)
.values({ name, parentId: parent, sortOrder })
.returning(CATEGORY_COLUMNS);
.insertInto('categories')
.values({ name, parent_id: parent, sort_order: sortOrder })
.returning(CATEGORY_COLUMNS)
.execute();
res.status(201).json({ ...requireRow(rows, 'the category INSERT'), item_count: 0 });
} catch (err) {
@@ -175,9 +171,9 @@ async function resolveParentId(
// Moving a node beneath itself or one of its own descendants would detach
// that whole branch from the tree into an unreachable cycle.
const cycle = await db.execute(
sql`${subtreeOf(id)} SELECT 1 FROM subtree WHERE id = ${parsed}`
);
const cycle = await sql<{ found: number }>`
${subtreeOf(id)} SELECT 1 AS found FROM subtree WHERE id = ${parsed}
`.execute(db);
if (cycle.rows.length) {
return { error: 'a category cannot be moved beneath itself' };
}
@@ -187,13 +183,12 @@ async function resolveParentId(
router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
const id = Number(req.params.id);
const existing = await db
const current = await db
.selectFrom('categories')
.select(CATEGORY_COLUMNS)
.from(categories)
.where(eq(categories.id, id))
.limit(1);
.where('id', '=', id)
.executeTakeFirst();
const current = existing[0];
if (!current) {
return res.status(404).json({ error: 'not found' });
}
@@ -219,10 +214,11 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
try {
const rows = await db
.update(categories)
.set({ name, parentId: parent, sortOrder })
.where(eq(categories.id, id))
.returning(CATEGORY_COLUMNS);
.updateTable('categories')
.set({ name, parent_id: parent, sort_order: sortOrder })
.where('id', '=', id)
.returning(CATEGORY_COLUMNS)
.execute();
res.json(requireRow(rows, 'the category UPDATE'));
} catch (err) {
@@ -236,27 +232,28 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
router.delete('/:id', asyncRoute(async (req: Request, res: Response) => {
const id = Number(req.params.id);
const subtree = await db.execute<{ id: number }>(
sql`${subtreeOf(id)} SELECT id FROM subtree`
);
const subtree = await sql<{ id: number }>`
${subtreeOf(id)} SELECT id FROM subtree
`.execute(db);
if (!subtree.rows.length) {
return res.status(404).json({ error: 'not found' });
}
const ids = subtree.rows.map((row) => row.id);
// inArray rather than the ANY(...::int[]) this replaced, which sidesteps the
// array trap in CONVENTIONS.md entirely: there is no template to forget
// sql.param() in. The builder emits the placeholder list itself and it is
// correct by construction.
// `in` rather than the ANY(...::int[]) this replaced. Kysely emits the
// placeholder list itself, so it is correct by construction and there is no
// template to forget anything in. `ids` is never empty — the length check
// above returned already if it were.
const affected = await db
.select({ n: sql<number>`COUNT(*)::int` })
.from(items)
.where(inArray(items.categoryId, ids));
.selectFrom('items')
.select(sql<number>`COUNT(*)::int`.as('n'))
.where('category_id', 'in', ids)
.execute();
// The FK cascade takes the descendants; items fall back to NULL rather than
// being deleted along with their category.
await db.delete(categories).where(eq(categories.id, id));
await db.deleteFrom('categories').where('id', '=', id).execute();
res.json({
deleted_categories: ids.length,