docs(db): plan the Kysely swap (#305)
Two tasks. The first is the whole swap in one commit — dependencies, generated types, db.ts, the reconverted file, the ported drift test and the new 409 assertion — because splitting it would put a commit on the branch where the build is broken or both builders are present, and neither is a state worth being able to bisect to. The second is the conventions document, which touches no code and is much shorter than the one it replaces. The plan carries the converted adminCategories.ts in full rather than describing it, and names the two places the conversion could silently change behaviour: the four selects must keep answering id, name, parent_id, sort_order and item_count, and the unique-violation catch must keep producing a 409. The existing category integration suite is the gate on the first, and a new test is the gate on the second. Three expected outputs are written down so a wrong one is caught at the step rather than three steps later. Codegen must report 18 tables, not 19 — 19 means pgmigrations leaked past the exclude flag. The generated Categories interface must spell parent_id and sort_order, because camelCase there means --camel-case got turned on and the mapping layer this swap removes has come straight back. And the integration count should rise by exactly one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -0,0 +1,704 @@
|
||||
# Kysely Swap Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Replace Drizzle with Kysely as the backend's query builder, in one commit that leaves no trace of Drizzle behind.
|
||||
|
||||
**Architecture:** `node-pg-migrate` keeps owning the schema. `kysely-codegen` replaces `drizzle-kit pull` as the source of generated types, `src/db-kysely/` replaces `src/db-drizzle/`, `db` in `src/db.ts` becomes a `Kysely` instance over the same `pg` Pool, and `adminCategories.ts` — the one converted file — is reconverted.
|
||||
|
||||
**Tech Stack:** Express 4 + TypeScript, `pg`, Kysely 0.28, kysely-codegen 0.20, Jest + supertest.
|
||||
|
||||
**Spec:** `docs/superpowers/specs/2026-09-04-kysely-swap-design.md`
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- **Drizzle and Kysely must not both be present at the end of Task 1.** `drizzle-orm`, `drizzle-kit`, `drizzle.config.ts` and `src/db-drizzle/` are all gone by then. `grep -ri drizzle backend/src backend/package.json` returns nothing.
|
||||
- **Kysely takes the existing `pg` Pool.** `new PostgresDialect({ pool })` using the `pool` already exported from `src/db.ts`. Never let Kysely create its own — a transaction on one pool is invisible to the other and the configured limits would silently double.
|
||||
- **`node-pg-migrate` is untouched.** No migration is written, generated, or altered. #219 stands.
|
||||
- **The API contract does not change.** `adminCategories.ts` must answer byte-identical JSON: `id`, `name`, `parent_id`, `sort_order`, `item_count`. The existing integration suite is the gate.
|
||||
- **Do not use `--camel-case`.** kysely-codegen offers it; using it would reintroduce the mapping layer this swap removes.
|
||||
- **All SQL is parameterized.** Never interpolate a value into a query string.
|
||||
- **Every Express route handler stays wrapped in `asyncRoute`.**
|
||||
- Commit subjects end with `(#305)`. **Commit bodies are never hard-wrapped** — one long line per paragraph. End every body with `Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`.
|
||||
- **Do not push.** Commit locally only.
|
||||
- **Do not run `scripts/start-local.ps1` or `scripts/run-tests.ps1`** — they prompt for UAC and hang.
|
||||
- **Node 20 is required.** The shell default is 18.x and Jest fails on it. Prepend `export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH"` to every command.
|
||||
- Integration tests need the test database: `npm run db:test:up` from `backend`, reachable at `postgres://redefined_test:redefined_test@localhost:55432/redefined_test`.
|
||||
|
||||
---
|
||||
|
||||
## File Structure
|
||||
|
||||
| File | Change |
|
||||
|---|---|
|
||||
| `backend/package.json` | Remove `drizzle-orm`, `drizzle-kit`. Add `kysely`, `kysely-codegen`. Add the `db:types` script. |
|
||||
| `backend/drizzle.config.ts` | Delete. |
|
||||
| `backend/src/db-drizzle/` | Delete the whole directory. |
|
||||
| `backend/src/db-kysely/schema.ts` | Create — generated by `npm run db:types`, never hand-edited. |
|
||||
| `backend/src/db.ts` | `db` becomes a `Kysely<DB>` over the same pool. |
|
||||
| `backend/src/routes/adminCategories.ts` | Reconverted. |
|
||||
| `backend/tests/integration/drizzleSchema.integration.test.ts` | Rename to `schemaMirror.integration.test.ts` and port. |
|
||||
| `backend/tests/integration/categoriesTags.integration.test.ts` | Add the duplicate-name 409 assertion. |
|
||||
| `backend/src/db-kysely/CONVENTIONS.md` | Task 2. Replaces `db-drizzle/CONVENTIONS.md`. |
|
||||
|
||||
---
|
||||
|
||||
## Task 1: The swap
|
||||
|
||||
**Files:**
|
||||
- Modify: `backend/package.json`, `backend/src/db.ts`, `backend/src/routes/adminCategories.ts`, `backend/tests/integration/categoriesTags.integration.test.ts`
|
||||
- Create: `backend/src/db-kysely/schema.ts` (generated)
|
||||
- Delete: `backend/drizzle.config.ts`, `backend/src/db-drizzle/` (all four files)
|
||||
- Rename: `backend/tests/integration/drizzleSchema.integration.test.ts` → `backend/tests/integration/schemaMirror.integration.test.ts`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: nothing from earlier tasks.
|
||||
- Produces: `export const db: Kysely<DB>` from `backend/src/db.ts`, and `export interface DB` from `backend/src/db-kysely/schema.ts`. Task 2 documents both.
|
||||
|
||||
- [ ] **Step 1: Swap the dependencies**
|
||||
|
||||
From `backend`:
|
||||
|
||||
```bash
|
||||
npm uninstall drizzle-orm drizzle-kit
|
||||
npm install kysely
|
||||
npm install --save-dev kysely-codegen
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Add the codegen script**
|
||||
|
||||
In `backend/package.json`, add to `scripts`, immediately after `"migrate:create"`:
|
||||
|
||||
```json
|
||||
"db:types": "kysely-codegen --dialect postgres --exclude-pattern pgmigrations --url \"env(KYSELY_DATABASE_URL)\" --out-file src/db-kysely/schema.ts"
|
||||
```
|
||||
|
||||
`env(...)` is kysely-codegen's own syntax for naming an environment variable, so the variable name lives in the script rather than being interpolated by a shell — which keeps the command identical on Windows and Linux. `--exclude-pattern pgmigrations` replaces `tablesFilter: ['!pgmigrations']` from the deleted `drizzle.config.ts`.
|
||||
|
||||
- [ ] **Step 3: Generate the schema**
|
||||
|
||||
Bring the test database up if it is not already, then generate:
|
||||
|
||||
```bash
|
||||
npm run db:test:up
|
||||
KYSELY_DATABASE_URL=postgres://redefined_test:redefined_test@localhost:55432/redefined_test npm run db:types
|
||||
```
|
||||
|
||||
Expected: `✓ Introspected 18 tables and generated src/db-kysely/schema.ts`. **18, not 19** — if it says 19, `pgmigrations` leaked in and the `--exclude-pattern` flag is wrong.
|
||||
|
||||
Confirm the shape is what the rest of this task assumes:
|
||||
|
||||
```bash
|
||||
grep -A20 "export interface DB" src/db-kysely/schema.ts
|
||||
grep -A6 "export interface Categories" src/db-kysely/schema.ts
|
||||
```
|
||||
|
||||
Expected: `DB` maps snake_case table names to interfaces, and `Categories` declares `id: Generated<number>`, `name: string`, `parent_id: number | null`, `sort_order: Generated<number>`. Column names are snake_case — that is the point of the swap, and `--camel-case` must not be used.
|
||||
|
||||
- [ ] **Step 4: Delete Drizzle**
|
||||
|
||||
```bash
|
||||
rm backend/drizzle.config.ts
|
||||
rm -r backend/src/db-drizzle
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Point `db.ts` at Kysely**
|
||||
|
||||
In `backend/src/db.ts`, replace the two Drizzle imports and the `db` export. The `pool` export and `requireRow` are unchanged.
|
||||
|
||||
Imports become:
|
||||
|
||||
```ts
|
||||
import { Pool } from 'pg';
|
||||
import { Kysely, PostgresDialect } from 'kysely';
|
||||
import type { DB } from './db-kysely/schema';
|
||||
```
|
||||
|
||||
And the `db` export, replacing the existing one and its comment:
|
||||
|
||||
```ts
|
||||
/**
|
||||
* Kysely over the same pool, alongside `pool` rather than instead of it.
|
||||
*
|
||||
* Both have to work at once: the conversion is file by file across 238 call
|
||||
* sites, so for a long time most queries will still be raw `pg` and the two
|
||||
* must share one set of connections. Handing Kysely the existing pool rather
|
||||
* than letting it open its own is what makes that true — otherwise a
|
||||
* transaction started on one would be invisible to the other, and the pool
|
||||
* limits would silently double.
|
||||
*
|
||||
* The value of this over raw `pg` is not brevity. In a Kysely `sql` template
|
||||
* `${value}` emits a bind parameter, never text, so there is no way to spell
|
||||
* "interpolate this as SQL" by accident. That makes the #202 invariant
|
||||
* structural instead of a comment plus two mutation tests, and it is the main
|
||||
* reason a builder is here at all.
|
||||
*
|
||||
* Kysely rather than Drizzle since #305. The safety property above was true of
|
||||
* both; what decided it is that three of the four hazards in the old
|
||||
* CONVENTIONS.md — an array needing sql.param(), a column reference silently
|
||||
* losing its table inside a raw fragment, and a camelCase mirror that had to be
|
||||
* mapped back at every select — were properties of Drizzle rather than of
|
||||
* type-safe query building. See #297 for the SQL each one actually emitted.
|
||||
*/
|
||||
export const db = new Kysely<DB>({ dialect: new PostgresDialect({ pool }) });
|
||||
```
|
||||
|
||||
- [ ] **Step 6: Reconvert `adminCategories.ts`**
|
||||
|
||||
Replace the whole file with:
|
||||
|
||||
```ts
|
||||
import { Router, Request, Response } from 'express';
|
||||
import { sql } from 'kysely';
|
||||
import { db, requireRow } from '../db';
|
||||
import { asyncRoute } from '../asyncRoute';
|
||||
|
||||
/**
|
||||
* 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 converted, not a cutover. See src/db-kysely/CONVENTIONS.md.
|
||||
*/
|
||||
|
||||
/**
|
||||
* The four columns this API answers with, named once.
|
||||
*
|
||||
* 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', 'name', 'parent_id', 'sort_order'] as const;
|
||||
|
||||
const router = Router();
|
||||
|
||||
// Postgres unique-violation SQLSTATE — raised by the two partial indexes that
|
||||
// stop siblings sharing a name.
|
||||
const UNIQUE_VIOLATION = '23505';
|
||||
|
||||
/**
|
||||
* Whether a thrown error is that unique violation.
|
||||
*
|
||||
* 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;
|
||||
const wrapped = (err as { cause?: { code?: string } }).cause?.code;
|
||||
return direct === UNIQUE_VIOLATION || wrapped === UNIQUE_VIOLATION;
|
||||
}
|
||||
|
||||
/**
|
||||
* 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: 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 (
|
||||
SELECT id FROM categories WHERE id = ${id}
|
||||
UNION ALL
|
||||
SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id
|
||||
)`;
|
||||
|
||||
function readName(value: unknown): string | null {
|
||||
if (typeof value !== 'string') return null;
|
||||
const trimmed = value.trim();
|
||||
return trimmed === '' ? null : trimmed;
|
||||
}
|
||||
|
||||
// Distinguishes "not supplied" from "explicitly cleared to root".
|
||||
function readParentId(value: unknown): number | null | undefined {
|
||||
if (value === undefined) return undefined;
|
||||
if (value === null || value === '') return null;
|
||||
const parsed = typeof value === 'number' ? value : Number(value);
|
||||
if (!Number.isSafeInteger(parsed) || parsed < 1) return undefined;
|
||||
return parsed;
|
||||
}
|
||||
|
||||
async function parentExists(id: number): Promise<boolean> {
|
||||
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
|
||||
.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);
|
||||
}));
|
||||
|
||||
router.post('/', asyncRoute(async (req: Request, res: Response) => {
|
||||
const name = readName(req.body.name);
|
||||
if (!name) {
|
||||
return res.status(400).json({ error: 'name is required' });
|
||||
}
|
||||
|
||||
const parentId = readParentId(req.body.parent_id);
|
||||
if (parentId === undefined && req.body.parent_id !== undefined) {
|
||||
return res.status(400).json({ error: 'invalid parent_id' });
|
||||
}
|
||||
const parent = parentId ?? null;
|
||||
if (parent !== null && !(await parentExists(parent))) {
|
||||
return res.status(400).json({ error: 'parent category does not exist' });
|
||||
}
|
||||
|
||||
const sortOrder = Number.isSafeInteger(req.body.sort_order) ? req.body.sort_order : 0;
|
||||
|
||||
try {
|
||||
const rows = await db
|
||||
.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) {
|
||||
if (isUniqueViolation(err)) {
|
||||
return res.status(409).json({ error: 'a category with that name already exists here' });
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}));
|
||||
|
||||
// Works out what parent_id an update should land on. Absent means "leave it
|
||||
// alone", so the current value is echoed back rather than treated as a clear.
|
||||
// Returns the refusal instead of sending it, keeping the response the
|
||||
// handler's business and the two ways a parent can be invalid out of its body.
|
||||
type ParentResolution = { error: string } | { parent: number | null };
|
||||
|
||||
async function resolveParentId(
|
||||
submitted: unknown,
|
||||
id: number,
|
||||
current: number | null
|
||||
): Promise<ParentResolution> {
|
||||
if (submitted === undefined) {
|
||||
return { parent: current };
|
||||
}
|
||||
|
||||
const parsed = readParentId(submitted);
|
||||
if (parsed === undefined) {
|
||||
return { error: 'invalid parent_id' };
|
||||
}
|
||||
if (parsed === null) {
|
||||
return { parent: null };
|
||||
}
|
||||
if (!(await parentExists(parsed))) {
|
||||
return { error: 'parent category does not exist' };
|
||||
}
|
||||
|
||||
// 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 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' };
|
||||
}
|
||||
|
||||
return { parent: parsed };
|
||||
}
|
||||
|
||||
router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
|
||||
const id = Number(req.params.id);
|
||||
const current = await db
|
||||
.selectFrom('categories')
|
||||
.select(CATEGORY_COLUMNS)
|
||||
.where('id', '=', id)
|
||||
.executeTakeFirst();
|
||||
|
||||
if (!current) {
|
||||
return res.status(404).json({ error: 'not found' });
|
||||
}
|
||||
|
||||
let name = current.name;
|
||||
if (req.body.name !== undefined) {
|
||||
const parsed = readName(req.body.name);
|
||||
if (!parsed) {
|
||||
return res.status(400).json({ error: 'name is required' });
|
||||
}
|
||||
name = parsed;
|
||||
}
|
||||
|
||||
const resolved = await resolveParentId(req.body.parent_id, id, current.parent_id);
|
||||
if ('error' in resolved) {
|
||||
return res.status(400).json({ error: resolved.error });
|
||||
}
|
||||
const parent = resolved.parent;
|
||||
|
||||
const sortOrder = Number.isSafeInteger(req.body.sort_order)
|
||||
? req.body.sort_order
|
||||
: current.sort_order;
|
||||
|
||||
try {
|
||||
const rows = await db
|
||||
.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) {
|
||||
if (isUniqueViolation(err)) {
|
||||
return res.status(409).json({ error: 'a category with that name already exists here' });
|
||||
}
|
||||
throw err;
|
||||
}
|
||||
}));
|
||||
|
||||
router.delete('/:id', asyncRoute(async (req: Request, res: Response) => {
|
||||
const id = Number(req.params.id);
|
||||
|
||||
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);
|
||||
|
||||
// `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
|
||||
.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.deleteFrom('categories').where('id', '=', id).execute();
|
||||
|
||||
res.json({
|
||||
deleted_categories: ids.length,
|
||||
uncategorized_items: requireRow(affected, 'the affected-items COUNT').n
|
||||
});
|
||||
}));
|
||||
|
||||
export default router;
|
||||
```
|
||||
|
||||
- [ ] **Step 7: Port the drift test**
|
||||
|
||||
Rename the file and replace its two parsing helpers. Keep every doc comment that explains *why* the test exists — the history in it is the reason it is trusted.
|
||||
|
||||
```bash
|
||||
git mv backend/tests/integration/drizzleSchema.integration.test.ts backend/tests/integration/schemaMirror.integration.test.ts
|
||||
```
|
||||
|
||||
In the renamed file: change the `readFileSync` path from `'db-drizzle', 'schema.ts'` to `'db-kysely', 'schema.ts'`, rename the describe block from `'the Drizzle schema mirror'` to `'the generated schema mirror'`, and make these three replacements.
|
||||
|
||||
`mirroredTables` — the table names now live in the `DB` interface rather than in `pgTable(...)` calls:
|
||||
|
||||
```ts
|
||||
/**
|
||||
* Every table name the generated mirror declares.
|
||||
*
|
||||
* Read from the `DB` interface, which is the one place kysely-codegen lists
|
||||
* them all, mapping the database's own snake_case name to the interface for
|
||||
* that table. Parsing the file as text rather than importing it is deliberate
|
||||
* and unchanged from the Drizzle version: these are TypeScript types, erased at
|
||||
* run time, so there is nothing to import and inspect.
|
||||
*/
|
||||
function mirroredTables(): string[] {
|
||||
const block = /export interface DB \{([^}]*)\}/.exec(SCHEMA)?.[1] ?? '';
|
||||
return [...block.matchAll(/^\s*([a-z_]+):/gm)].map((m) => m[1]!).sort();
|
||||
}
|
||||
```
|
||||
|
||||
The `pgmigrations` test's comment changes to name the new flag:
|
||||
|
||||
```ts
|
||||
// pgmigrations is node-pg-migrate's bookkeeping, excluded by
|
||||
// --exclude-pattern in the db:types script. A regeneration without that flag
|
||||
// would quietly put it back.
|
||||
```
|
||||
|
||||
And the column check gets stricter, losing the two-way match and the `snakeToCamel` helper entirely — **delete that function**:
|
||||
|
||||
```ts
|
||||
// Columns, not just tables: a migration that adds a column to a table the
|
||||
// mirror already knows about is the likelier drift, and the one a table-level
|
||||
// check would wave through.
|
||||
//
|
||||
// One exact match, where the Drizzle version needed two and called itself
|
||||
// "deliberately loose" for it. kysely-codegen emits the database's own name
|
||||
// as a bare interface key, so ` column_name:` at the start of a line is the
|
||||
// whole of the naming rule and there is nothing to re-implement.
|
||||
it('declares every column of every table it mirrors', async () => {
|
||||
const { rows } = await pool.query<{ table_name: string; column_name: string }>(
|
||||
`SELECT table_name, column_name FROM information_schema.columns
|
||||
WHERE table_schema = 'public' AND table_name <> 'pgmigrations'
|
||||
ORDER BY table_name, column_name`
|
||||
);
|
||||
|
||||
const declared = new Set(
|
||||
[...SCHEMA.matchAll(/^\s{2}([a-z_]+):/gm)].map((m) => m[1]!)
|
||||
);
|
||||
const missing = rows
|
||||
.filter((row) => !declared.has(row.column_name))
|
||||
.map((row) => `${row.table_name}.${row.column_name}`);
|
||||
|
||||
expect(missing).toEqual([]);
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 8: Prove the 409 still happens**
|
||||
|
||||
In `backend/tests/integration/categoriesTags.integration.test.ts`, add this test inside the existing describe block that covers category creation. If the file's existing tests use a helper to create a category, use it rather than the raw request below; otherwise this shape is correct as written.
|
||||
|
||||
```ts
|
||||
// The conversion hazard from #218, asserted rather than assumed. Drizzle
|
||||
// wrapped driver errors and moved the unique-violation SQLSTATE from
|
||||
// err.code to err.cause.code, which turned this 409 into a 500 with nothing
|
||||
// failing to compile. #305 changed builder again, so this proves where the
|
||||
// code actually lands rather than trusting that Kysely leaves it alone.
|
||||
it('refuses a duplicate sibling name with 409, not 500', async () => {
|
||||
const first = await request(app).post('/api/admin/categories').send({ name: 'Duplicate me' });
|
||||
expect(first.status).toBe(201);
|
||||
|
||||
const second = await request(app).post('/api/admin/categories').send({ name: 'Duplicate me' });
|
||||
|
||||
expect(second.status).toBe(409);
|
||||
expect(second.body.error).toMatch(/already exists/);
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 9: Verify no Drizzle survives**
|
||||
|
||||
```bash
|
||||
grep -ri drizzle backend/src backend/tests backend/package.json backend/package-lock.json --include='*.ts' --include='*.json' -l
|
||||
```
|
||||
|
||||
Expected: no output from `backend/src` or `backend/tests`. `package-lock.json` may still list transitive entries removed by npm — if `drizzle` appears there, run `npm install` once more from `backend` and re-check. `backend/drizzle.config.ts` and `backend/src/db-drizzle/` must not exist.
|
||||
|
||||
- [ ] **Step 10: Run everything**
|
||||
|
||||
```bash
|
||||
cd backend
|
||||
npm run build
|
||||
npm run lint
|
||||
npx jest -c jest.unit.config.js
|
||||
npx jest -c jest.integration.config.js --runInBand
|
||||
```
|
||||
|
||||
Expected: build clean, lint 0 errors, 466 unit tests passing, 445 integration tests passing (444 before, plus the new 409 assertion). If the category suite fails, the JSON contract changed — that is the failure this task exists to prevent, so fix the query rather than the test.
|
||||
|
||||
- [ ] **Step 11: Commit**
|
||||
|
||||
```bash
|
||||
git add -A backend
|
||||
git commit -F- <<'EOF'
|
||||
refactor(db): swap the query builder from Drizzle to Kysely (#305)
|
||||
|
||||
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>
|
||||
EOF
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: The conventions document
|
||||
|
||||
**Files:**
|
||||
- Create: `backend/src/db-kysely/CONVENTIONS.md`
|
||||
|
||||
**Interfaces:**
|
||||
- Consumes: `db` from `backend/src/db.ts` and `DB` from `backend/src/db-kysely/schema.ts` (Task 1). This task writes no code.
|
||||
- Produces: nothing code depends on.
|
||||
|
||||
The old document was mostly a list of ways to be quietly wrong, and three of its four warnings no longer apply. The replacement is shorter for that reason — do not pad it back out, and do not carry across a warning that is no longer true.
|
||||
|
||||
- [ ] **Step 1: Write the document**
|
||||
|
||||
Create `backend/src/db-kysely/CONVENTIONS.md`:
|
||||
|
||||
````markdown
|
||||
# Kysely conventions
|
||||
|
||||
Decided in #216, rebuilt on Kysely in #305 for the reasons in #297. Read this before converting a query.
|
||||
|
||||
## What is in this directory
|
||||
|
||||
| File | Owner |
|
||||
|---|---|
|
||||
| `schema.ts` | **Generated.** `kysely-codegen` output. Do not hand-edit. |
|
||||
| `CONVENTIONS.md` | This file. |
|
||||
|
||||
`backend/migrations` owns the schema. `schema.ts` is a read-only mirror of it, and refreshing that mirror is a manual step:
|
||||
|
||||
```bash
|
||||
KYSELY_DATABASE_URL=postgres://user:pass@localhost:PORT/db npm run db:types
|
||||
```
|
||||
|
||||
Run it against a database with every migration applied, after writing a migration. `schemaMirror.integration.test.ts` fails when the mirror and the database disagree, on tables or on columns.
|
||||
|
||||
That test exists because the drift is silent and already happened: the mirror sat missing `item_drafts` and `upload_links` from the moment #222 landed until #217, and nobody had reason to look. A stale mirror is worse than none — row types are inferred from it, so a converted query type-checks against a schema the database does not have and fails at run time on a column that does not exist.
|
||||
|
||||
## The reason this is worth doing
|
||||
|
||||
`${value}` in a Kysely `sql` template emits a bind parameter, never text. There is no way to spell "interpolate this value as SQL" by accident: the escape hatch that looks like a plain template literal does not behave like one. Passing `"1); DROP TABLE items; --"` as a status value puts it in the parameters, not in the SQL.
|
||||
|
||||
That makes the #202 invariant — only placeholder indices may be interpolated into a clause — a property of the type system rather than a comment guarded by two mutation tests, and it retires #180's three S2077 hotspots rather than leaving them reviewed and watched.
|
||||
|
||||
## Both drivers run at once
|
||||
|
||||
`db` and `pool` are exported from `src/db.ts` and share one pool, deliberately. Conversion is file by file across 238 sites, so most queries will be raw `pg` for a long time and the two must not open separate connection pools — a transaction on one would be invisible to the other, and the configured limits would silently double.
|
||||
|
||||
Column names need no translation. The generated types carry the database's own snake_case, which is also what these APIs answer with, so a select names the columns it wants and the JSON comes out right. Do not turn on kysely-codegen's `--camel-case`: it would reintroduce a mapping layer whose failure mode is a silently changed response that no status-code test catches.
|
||||
|
||||
## Driver errors are not wrapped
|
||||
|
||||
Kysely uses the `pg` driver directly, so a Postgres SQLSTATE stays on `err.code`. This is worth stating only because it was not true before: Drizzle wrapped driver errors and moved the code to `err.cause.code`, so a `catch` keyed on it still compiled, never matched, and turned a handled 409 into a 500. `adminCategories.ts` has `isUniqueViolation`, which accepts both shapes and has an integration test behind it. Reuse that pattern, and keep the test.
|
||||
|
||||
## The worked example
|
||||
|
||||
`buildItemFilterSql` is the hardest query in the codebase — 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. It was the #216 spike's target and #297's, and it is here rather than in a source file because nothing imports it:
|
||||
|
||||
```ts
|
||||
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
|
||||
)`);
|
||||
}
|
||||
|
||||
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.status !== null) clauses.push(eb('items.status', 'in', filters.status));
|
||||
```
|
||||
|
||||
Two things in there are worth pointing at, because both were traps in the previous library and are not traps here.
|
||||
|
||||
`${filters.categoryIds}` emits **one** bind parameter holding the whole array — `ANY($1::int[])` — rather than a placeholder list. Drizzle emitted `ANY(($1, $2)::int[])`, which is invalid Postgres, unless every array site remembered `sql.param()`.
|
||||
|
||||
The column references inside those templates are text you wrote and qualified yourself, so `it.item_id = items.id` means what it says. Drizzle rendered an interpolated column reference without its table, so a correlated subquery silently correlated with itself — valid SQL, quietly wrong data, and the reason #218 got a count of 1 where 2 was correct.
|
||||
|
||||
That second one is why a converted query containing a correlated subquery or a self-join still deserves a test asserting **values** rather than a status code. The library no longer makes the mistake for you; writing the wrong column name in a raw fragment is still your own to make.
|
||||
|
||||
## Migrations stay hand-written
|
||||
|
||||
Decided in **#219** and unchanged by #305: `node-pg-migrate` keeps the schema, the builder is for queries only.
|
||||
|
||||
Three reasons, all measured rather than assumed. `drizzle-kit generate` could not diff expression indexes, so adding one nullable column emitted six statements — three `DROP INDEX` and three `CREATE UNIQUE INDEX` alongside the `ALTER` — and those rebuilds take real locks on a large table. Our migrations are mostly prose, and generated SQL carries none of it: a rule that every generated migration is annotated before merge is a rule that holds for three migrations and then quietly stops, with the failure invisible because the migration still works. And data migrations cannot be generated at all, so anything touching existing rows stays hand-written regardless.
|
||||
|
||||
The first of those was specific to `drizzle-kit`. The other two are true of any generator, which is why the decision survives the change of library — and Kysely, which ships no generator anyone was asking us to use, has nothing to refuse.
|
||||
|
||||
The workflow: write the migration by hand, then run `npm run db:types` to refresh the mirror. `schemaMirror.integration.test.ts` fails if you forget.
|
||||
````
|
||||
|
||||
- [ ] **Step 2: Check the links it makes are real**
|
||||
|
||||
```bash
|
||||
cd backend
|
||||
test -f src/db-kysely/schema.ts && echo "schema present"
|
||||
grep -n "db:types" package.json
|
||||
grep -n "isUniqueViolation" src/routes/adminCategories.ts
|
||||
test -f tests/integration/schemaMirror.integration.test.ts && echo "drift test present"
|
||||
```
|
||||
|
||||
Expected: all four confirm. Every path and script name this document names must exist, because a conventions file that points at something gone is worse than none.
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add backend/src/db-kysely/CONVENTIONS.md
|
||||
git commit -F- <<'EOF'
|
||||
docs(db): write the Kysely conventions (#305)
|
||||
|
||||
Replaces the Drizzle conventions, and is much shorter, because three of that document's four warnings described the library rather than the practice and stopped being true when the library changed. An array is one bind parameter with no ceremony, a column reference in a raw fragment is the text you wrote, and the generated names are the database's own so nothing needs mapping back.
|
||||
|
||||
What survives is what was never about Drizzle. The mirror is generated and refreshing it is manual, so the drift test is the thing that catches forgetting — and it exists because the drift already happened once and nobody noticed for a week. Both drivers share one pool, because a transaction on a second pool would be invisible to the first and the limits would silently double. Migrations stay hand-written, and the reasoning survives the change of library: only the expression-index complaint was specific to drizzle-kit, while losing the prose and being unable to express data migrations are true of any generator.
|
||||
|
||||
One warning is genuinely new, and it is the inverse of an old one: driver errors are no longer wrapped, so a SQLSTATE sits on err.code again. That is worth stating precisely because it was not true before, and the last time it moved it turned a handled 409 into a 500 with nothing failing to compile.
|
||||
|
||||
The worked example moved into this file rather than staying a source file nothing imports. It is documentation, and it was only ever documentation.
|
||||
|
||||
Closes #305
|
||||
|
||||
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||||
EOF
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Self-Review
|
||||
|
||||
**Spec coverage:**
|
||||
|
||||
| Spec requirement | Task |
|
||||
|---|---|
|
||||
| Both builders must not coexist | 1 (steps 1, 4, 9) |
|
||||
| Kysely takes the existing `pg` Pool | 1 (step 5) |
|
||||
| `src/db-drizzle/` becomes `src/db-kysely/` | 1 (steps 3, 4) |
|
||||
| The worked example is not a source file | 2 (inside CONVENTIONS.md) |
|
||||
| Drift test survives, renamed and stricter | 1 (step 7) |
|
||||
| `CATEGORY_COLUMNS` stops being a mapping | 1 (step 6) |
|
||||
| `isUniqueViolation` keeps both shapes, gains a test | 1 (steps 6, 8) |
|
||||
| Codegen command and `--exclude-pattern` | 1 (steps 2, 3) |
|
||||
| `--camel-case` must not be used | Global constraints; 1 (step 3); 2 |
|
||||
| Migrations untouched | Global constraints; 2 |
|
||||
| Existing category suite passes unchanged | 1 (step 10) |
|
||||
| Whole backend suite green | 1 (step 10) |
|
||||
|
||||
No gaps.
|
||||
|
||||
**Placeholder scan:** none. Every code step carries literal code; every command step carries the literal command and its expected output.
|
||||
|
||||
**Type consistency:** `DB` is generated in step 3 and imported in step 5 under that exact name. `db` is exported from `src/db.ts` (step 5) and imported by `adminCategories.ts` (step 6) and used by the `sql` fragments' `.execute(db)`. `CATEGORY_COLUMNS` is a `readonly ['id','name','parent_id','sort_order']` throughout step 6, passed to `.select()` and `.returning()` in all four places. `requireRow` keeps its existing `(rows: T[], what: string) => T` signature and is only ever handed `.execute()` results, which are arrays — never `executeTakeFirst()`, which returns `T | undefined` and is branched on directly instead.
|
||||
Reference in New Issue
Block a user