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>
This commit is contained in:
@@ -0,0 +1,80 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user