diff --git a/docs/superpowers/specs/2026-09-04-kysely-swap-design.md b/docs/superpowers/specs/2026-09-04-kysely-swap-design.md new file mode 100644 index 0000000..ecba55a --- /dev/null +++ b/docs/superpowers/specs/2026-09-04-kysely-swap-design.md @@ -0,0 +1,84 @@ +# Swapping the query builder to Kysely + +**Issue:** #305. Carries out the decision recorded in #297, which re-opened the choice made in #216 and landed in #217/#218. + +Nothing here reopens #219. Migrations stay hand-written in `node-pg-migrate`, which is what they already are — Drizzle was never doing them. + +## What is actually changing + +One converted file, three calls, against 238 raw `pool.query` sites. That is the whole reason this is worth doing now rather than never: the commitment to Drizzle is far smaller than "we adopted Drizzle" suggests, and every month it grows. + +The safety property that motivated the adoption is unchanged and is not the thing being traded. In both libraries a value interpolated into a `sql` template becomes a bind parameter, never text, so #202's invariant stays a property of the type system and #180's S2077 hotspots retire either way. What changes is the three ways Drizzle makes it easy to be quietly wrong, verified in #297 against emitted SQL: an array needing `sql.param()` or producing invalid Postgres, a column reference inside a raw fragment silently losing its table, and a camelCase mirror that forces an explicit column map at every select or the JSON contract changes without a test noticing. + +## Decisions, and what each one rests on + +**Both builders must not coexist at any commit.** The swap lands as one change. A `main` that carries Drizzle and Kysely together, even briefly, is a `main` where the next person converting a query has to guess which one to reach for, and where two generated mirrors of one database can disagree. There is nothing to stage here — one file uses the builder. + +**Kysely takes the existing `pg` Pool.** Exactly as Drizzle does today, and for the same reason, which has not weakened: the conversion stays file by file, so most queries will be raw `pg` for a long time and the two must share one set of connections. A separate pool would make a transaction on one invisible to the other and would silently double the configured limits. + +**`src/db-drizzle/` becomes `src/db-kysely/`.** A rename rather than a new directory beside it, because the old one has no reason to survive the commit that empties it. + +**The worked example does not come across as a source file.** `itemFilters.drizzle.ts` was kept from #216 as the worked example and was never imported by anything — dead code in `src/` that only documentation justified. Its replacement lives inside `CONVENTIONS.md` as a fenced block, which is where a worked example belongs, and #297's spike stays in `docs/` as the record of how the decision was reached. This is a small improvement the swap makes free; it is not a change of intent. + +**The drift test survives the swap and loses its rename.** `drizzleSchema.integration.test.ts` becomes `schemaMirror.integration.test.ts` — named for what it guards rather than for the library that happens to generate the mirror, so the next such change renames nothing. The drift it exists for is library-independent and already happened once: the mirror sat missing `item_drafts` and `upload_links` from #222 until #217 and nothing noticed for a week. + +It also gets **stricter for free**. The Drizzle version had to check each column two ways — the bare camelCase key or an explicit string argument — and its own comment calls that "deliberately loose". kysely-codegen emits the database's names verbatim as bare keys, so the check becomes one exact match and the `snakeToCamel` helper goes away. + +**`CATEGORY_COLUMNS` stops being a mapping.** It exists as a mapping solely because Drizzle's mirror is camelCase while the API answers snake_case; its comment says selecting the table directly "would silently change the JSON contract, and no test that checks status codes would catch it". With generated types carrying `parent_id` and `sort_order`, there is nothing left to translate. What remains is a plain list of column names, shared by the four selects that want the same four columns — worth keeping for the ordinary reason any repeated literal is, but no longer a translation layer with a silent failure mode behind it. That is the clearest single illustration of what the swap buys, so the reconverted file should show it. + +**`isUniqueViolation` keeps accepting both error shapes, and gains a test that proves which one arrives.** Drizzle wraps driver errors, moving the SQLSTATE from `err.code` to `err.cause.code`; the old check compiled, never matched, and turned two 409s into 500s — a hazard with no type error behind it. Kysely uses the `pg` driver directly and is expected to leave the code where it was, but "expected" is exactly the word that made this a bug last time. The tolerant check stays, and an integration test asserts a duplicate sibling name still answers 409 rather than 500. + +## Architecture + +``` +backend/migrations node-pg-migrate, hand-written. Owns the schema. + │ + ▼ npm run db:types (manual, after every migration) +src/db-kysely/schema.ts Generated. kysely-codegen. Read-only mirror. + │ + ├─ schemaMirror.integration.test.ts fails when mirror and database disagree + │ + ▼ +src/db.ts export const db = new Kysely({ dialect: new PostgresDialect({ pool }) }) + │ ▲ + │ the same pool ┘ + ▼ `pool` still exports +src/routes/adminCategories.ts the one converted file +``` + +### Codegen + +``` +KYSELY_DATABASE_URL=postgres://user:pass@localhost:PORT/db npm run db:types +``` + +The env-var name mirrors `DRIZZLE_DATABASE_URL`'s reasoning: credentials come from the environment, and the name says which tool wants it so it is not mistaken for something the application reads. `kysely-codegen` accepts `--url env(KYSELY_DATABASE_URL)`, so the variable name is fixed in the script rather than interpolated by a shell, which keeps the command identical on Windows and Linux. + +`--exclude-pattern pgmigrations` replaces `tablesFilter: ['!pgmigrations']`. It is node-pg-migrate's bookkeeping and has no business in a generated model of the application's schema; the drift test asserts it stays out, because a regeneration that dropped the flag would quietly put it back. + +Run it against a database with every migration applied, **after** writing a migration. The drift test is what catches forgetting. + +## Failure handling + +| What happens | Result | +|---|---| +| A migration adds a table, nobody regenerates | `schemaMirror.integration.test.ts` fails naming the table. | +| A migration adds a column, nobody regenerates | Same test fails naming `table.column`. The likelier drift, and the one a table-level check waves through. | +| Someone regenerates without the exclude flag | The test fails on `pgmigrations`. | +| The mirror names something the database does not have | The test fails. A migration was rolled back without regenerating. | +| Kysely surfaces the unique violation on `err.cause.code` after all | `isUniqueViolation` already accepts it, and the new integration test proves the 409 rather than assuming it. | + +## Testing + +- **Integration:** the existing category suite must pass unchanged — it is the contract this file answers, and the whole point is that the JSON is byte-identical afterwards. Plus the duplicate-name 409 assertion described above. +- **Schema mirror:** the four drift cases, ported to the new generated shape and tightened to an exact column match. +- **Unit:** none needed. There is no new pure logic; `requireRow` is untouched. +- **Whole suite:** backend unit and integration both green, because a builder swap that changes a shared `db.ts` can break something nowhere near the diff. + +## Out of scope + +**Converting anything beyond `adminCategories.ts`.** The remaining 238 sites stay raw `pg`, file by file, under their own issue. This change makes the next conversion possible; it does not perform it. + +**Migrations.** #219 stands untouched, and Kysely has no migration generator to refuse. + +**`CamelCasePlugin`.** kysely-codegen offers `--camel-case` and using it would reintroduce precisely the mapping problem this swap removes.