docs(db): weigh Kysely against the Drizzle decision (#297) #311

Closed
bermudalamb wants to merge 3 commits from feature/297-kysely-vs-drizzle into main
26 changed files with 2643 additions and 2282 deletions
Showing only changes of commit a14b3eafb8 - Show all commits
-9
View File
@@ -21,15 +21,6 @@ backend/unit-results.json
backend/integration-results.json backend/integration-results.json
frontend/playwright-results.json frontend/playwright-results.json
# drizzle-kit pull writes the schema mirror into backend/src/db-drizzle (see
# backend/drizzle.config.ts), but `out` is also where it would put generated
# migrations and their journal. This project's migration history is
# backend/migrations — hand-written, and mostly prose. #219 has not chosen
# otherwise, so a stray 0000_*.sql in src/ is at best noise and at worst
# mistaken for real migration history. Keep the mirror, drop the rest.
backend/src/db-drizzle/*.sql
backend/src/db-drizzle/meta/
# Where an end-to-end run against the throwaway database writes its uploads. # Where an end-to-end run against the throwaway database writes its uploads.
# Disposable with the database it belongs to (#186). # Disposable with the database it belongs to (#186).
backend/.e2e-uploads/ backend/.e2e-uploads/
-31
View File
@@ -1,31 +0,0 @@
import { defineConfig } from 'drizzle-kit';
// Spike configuration (#216). Credentials come from the environment rather than
// this file — the same rule the rest of the repo follows, and this one points at
// a developer's local database, not a deployed one.
//
// DRIZZLE_DATABASE_URL=postgres://redefined_local:redefined_local@localhost:55500/redefined_local
export default defineConfig({
dialect: 'postgresql',
schema: './src/db-drizzle/schema.ts',
// `drizzle-kit pull` writes its output here, so this points at the directory
// the application actually imports from. The spike pulled into ./drizzle and
// copied the file into src/ by hand, and that copy drifted exactly as
// predicted — not because anyone re-pulled, but because #222 added
// item_drafts and upload_links and the mirror was never refreshed. Nothing
// noticed for a week. Pulling in place removes the copy step that made that
// possible. See #217.
//
// If #219 ever chooses generated migrations, they also land in `out`, and
// this will need splitting then. It is a queries-only mirror today.
out: './src/db-drizzle',
// pgmigrations is node-pg-migrate's own bookkeeping. It is not part of the
// application's schema and has no business in a generated model of it.
tablesFilter: ['!pgmigrations'],
dbCredentials: {
url: process.env.DRIZZLE_DATABASE_URL ?? ''
}
});
+6 -7
View File
@@ -30,18 +30,17 @@ const advisory = (config) => ({
}); });
export default tseslint.config( export default tseslint.config(
// src/db-drizzle/schema.ts and relations.ts are `drizzle-kit pull` output, not // src/db-kysely/schema.ts is `kysely-codegen` output, not written by anyone
// written by anyone here. #261 hand-fixed an unused-parameter warning in the // here. #261 hand-fixed an unused-parameter warning in the equivalent Drizzle
// schema and #217's re-pull put it straight back, which is the whole argument: // file and #217's regeneration put it straight back, which is the whole
// linting generated code buys a fix that the next regeneration undoes. The // argument: linting generated code buys a fix that the next regeneration
// hand-written files in that directory are still linted. // undoes. The hand-written files in that directory are still linted.
{ {
ignores: [ ignores: [
'dist/**', 'dist/**',
'coverage/**', 'coverage/**',
'eslint.config.mjs', 'eslint.config.mjs',
'src/db-drizzle/schema.ts', 'src/db-kysely/schema.ts'
'src/db-drizzle/relations.ts'
] ]
}, },
+198 -1094
View File
File diff suppressed because it is too large Load Diff
+4 -3
View File
@@ -26,16 +26,17 @@
"db:test:down": "docker compose -f docker-compose.test.yml down -v", "db:test:down": "docker compose -f docker-compose.test.yml down -v",
"migrate:up": "node migrate.js up", "migrate:up": "node migrate.js up",
"migrate:down": "node migrate.js down", "migrate:down": "node migrate.js down",
"migrate:create": "node-pg-migrate create --migration-file-language js" "migrate:create": "node-pg-migrate create --migration-file-language js",
"db:types": "kysely-codegen --dialect postgres --exclude-pattern pgmigrations --url \"env(KYSELY_DATABASE_URL)\" --out-file src/db-kysely/schema.ts"
}, },
"dependencies": { "dependencies": {
"@anthropic-ai/sdk": "^0.122.0", "@anthropic-ai/sdk": "^0.122.0",
"@types/markdown-it": "^14.2.0", "@types/markdown-it": "^14.2.0",
"bcryptjs": "^2.4.3", "bcryptjs": "^2.4.3",
"cookie-parser": "^1.4.6", "cookie-parser": "^1.4.6",
"drizzle-orm": "^0.45.2",
"express": "^4.19.2", "express": "^4.19.2",
"express-rate-limit": "^8.6.2", "express-rate-limit": "^8.6.2",
"kysely": "^0.28.17",
"markdown-it": "^15.0.0", "markdown-it": "^15.0.0",
"multer": "^1.4.5-lts.1", "multer": "^1.4.5-lts.1",
"node-cron": "^3.0.3", "node-cron": "^3.0.3",
@@ -57,11 +58,11 @@
"@types/nodemailer": "^6.4.15", "@types/nodemailer": "^6.4.15",
"@types/pg": "^8.11.6", "@types/pg": "^8.11.6",
"@types/supertest": "^6.0.2", "@types/supertest": "^6.0.2",
"drizzle-kit": "^0.31.10",
"eslint": "^9.39.5", "eslint": "^9.39.5",
"eslint-plugin-sonarjs": "^4.2.0", "eslint-plugin-sonarjs": "^4.2.0",
"globals": "^17.11.0", "globals": "^17.11.0",
"jest": "^29.7.0", "jest": "^29.7.0",
"kysely-codegen": "^0.20.0",
"supertest": "^7.0.0", "supertest": "^7.0.0",
"ts-jest": "^29.2.4", "ts-jest": "^29.2.4",
"tsx": "^4.16.5", "tsx": "^4.16.5",
-78
View File
@@ -1,78 +0,0 @@
# Drizzle conventions
Decided in #216, landed in #217. Read this before converting a query.
## What is in this directory
| File | Owner |
|---|---|
| `schema.ts` | **Generated.** `drizzle-kit pull` output. Do not hand-edit. |
| `relations.ts` | **Generated.** Same. |
| `itemFilters.drizzle.ts` | Hand-written. The #216 spike's conversion of `buildItemFilterSql`, kept as the worked example. |
| `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
DRIZZLE_DATABASE_URL=postgres://user:pass@localhost:PORT/db npx drizzle-kit pull
```
Run it against a database with every migration applied, after writing a migration. `drizzle-kit pull` also emits `0000_*.sql` and `meta/` into this directory because `out` serves both purposes; both are gitignored, because this project's migration history is `backend/migrations` and a stray SQL file here would at best be noise and at worst be mistaken for real history. Whether that stays true is #219.
`drizzleSchema.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, because the spike had copied it into `src/` by hand and nobody had reason to look. A stale mirror is worse than none — Drizzle infers row types 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 rule that will bite you hardest: columns in a `sql` template are unqualified
Drizzle renders a column reference inside a `sql` template **without its table**.
```ts
// WRONG. Generates: (SELECT COUNT(*)::int FROM "items" WHERE "category_id" = "id")
// Postgres resolves both sides against items, so the subquery correlates with
// itself and returns a plausible wrong number.
sql`(SELECT COUNT(*)::int FROM ${items} WHERE ${items.categoryId} = ${categories.id})`
// RIGHT. Literal text, which is honest here because the fragment binds no values.
sql`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)`
```
This is worse than the array trap below, because the array trap produces invalid SQL and fails loudly. This produces **valid SQL and quietly wrong data** — it type-checks, reads correctly, and executes without error. It was found in #218 only because an integration test asserted the count was 2 and got 1.
So: any converted query containing a correlated subquery or a self-join needs a test asserting **values**, not just a status code. Write that test before converting.
## The other one: a driver error code moves
Drizzle wraps driver errors. A Postgres SQLSTATE that sat on `err.code` sits on `err.cause.code` after conversion, so a `catch` keyed on it still compiles, never matches, and turns a handled 409 into a 500. `adminCategories.ts` has `isUniqueViolation`, which accepts both shapes; reuse that pattern. Revisit every SQLSTATE-keyed catch when converting a file.
## The rule that will bite you: arrays
In a Drizzle `sql` template, an array interpolates as a **placeholder list**, not as one array parameter.
```ts
// WRONG. Emits ANY(($1, $2)::int[]), which is invalid Postgres.
sql`... WHERE id = ANY(${filters.categoryIds}::int[])`
// RIGHT. Emits ANY($1::int[]).
sql`... WHERE id = ANY(${sql.param(filters.categoryIds)}::int[])`
```
The wrong form type-checks, reads correctly, and fails at run time. Nothing warns. Across 187 call sites this is exactly the shape of defect that passes review and breaks in production, so `sql.param()` is required for every array and any converted query taking one needs a test that actually executes it.
## The reason this is worth doing
`${value}` in a Drizzle `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 and 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. It is the strongest argument for the adoption, and the spike confirmed it is real rather than relocated.
## Both drivers run at once
Column names differ, and the difference is load-bearing. The mirror is camelCase (`parentId`, `sortOrder`); these APIs answer in snake_case, which the admin frontend reads. So a select must map explicitly — `{ parent_id: categories.parentId }` — rather than selecting the table. Selecting the table directly changes the JSON contract silently, and no test asserting status codes notices. `adminCategories.ts` writes that mapping once as `CATEGORY_COLUMNS` and infers the row type from it, which is also how the hand-declared row interfaces are retired.
`db` and `pool` are exported from `src/db.ts` and share one pool, deliberately. Conversion is file by file across 187 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.
## Migrations stay hand-written
Decided in **#219**: `node-pg-migrate` keeps the schema, Drizzle is for queries only. Do not start generating migrations as a side effect of converting a query.
Three reasons, all measured rather than assumed. `drizzle-kit generate` cannot 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 workflow: write the migration by hand, then run `drizzle-kit pull` to refresh the mirror. `drizzleSchema.integration.test.ts` fails if you forget.
@@ -1,74 +0,0 @@
// Spike (#216): buildItemFilterSql expressed with Drizzle.
//
// Deliberately the hardest thing 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. If this cannot be said
// cleanly, nothing else in the conversion matters.
import { SQL, and, gte, lte, sql, inArray, exists } from 'drizzle-orm';
import { items, itemTags, favorites, categories } from './schema';
export interface SpikeFilters {
categoryIds: number[];
tagIds: number[];
minPriceCents: number | null;
maxPriceCents: number | null;
status: string[] | null;
favoritesOnly: boolean;
}
export function buildItemFilterDrizzle(
filters: SpikeFilters,
favoritesCustomerId: number | null
): SQL[] {
const clauses: SQL[] = [];
// The recursive CTE. Drizzle's $with() builds statement-level CTEs; this one
// has to sit inside an IN (...) subquery, so it stays a sql`` template.
//
// Note what that template does with ${filters.categoryIds}: it emits a BIND
// PARAMETER, not text. That is the difference from a plain JS template
// literal, and it is the whole of the #202 invariant expressed by the type
// system rather than by a comment — there is no way to spell "interpolate
// this value as SQL text" by accident.
if (filters.categoryIds.length) {
clauses.push(sql`${items.categoryId} IN (
WITH RECURSIVE subtree AS (
SELECT id FROM ${categories} WHERE id = ANY(${sql.param(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, so the count of
// matched rows has to equal the number requested.
if (filters.tagIds.length) {
clauses.push(sql`(
SELECT COUNT(*) FROM ${itemTags} it
WHERE it.item_id = ${items.id} AND it.tag_id = ANY(${sql.param(filters.tagIds)}::int[])
) = ${filters.tagIds.length}`);
}
if (filters.minPriceCents !== null) clauses.push(gte(items.priceCents, filters.minPriceCents));
if (filters.maxPriceCents !== null) clauses.push(lte(items.priceCents, filters.maxPriceCents));
// inArray replaces `= ANY($n::text[])`. Drizzle emits an IN list of binds.
if (filters.status !== null) clauses.push(inArray(items.status, filters.status));
if (filters.favoritesOnly) {
if (favoritesCustomerId === null) throw new Error('favorites filter requires a customer id');
clauses.push(
exists(
sql`(SELECT 1 FROM ${favorites} f WHERE f.item_id = ${items.id} AND f.customer_id = ${favoritesCustomerId})`
)
);
}
return clauses;
}
export function combine(clauses: SQL[]): SQL | undefined {
return clauses.length ? and(...clauses) : undefined;
}
-171
View File
@@ -1,171 +0,0 @@
import { relations } from "drizzle-orm/relations";
import { categories, items, itemImages, customers, customerSessions, customerTokens, carts, cartItems, shippingAddresses, checkouts, orders, itemDrafts, uploadLinks, itemTags, tags, checkoutItems, favorites } from "./schema";
export const itemsRelations = relations(items, ({one, many}) => ({
category: one(categories, {
fields: [items.categoryId],
references: [categories.id]
}),
itemImages: many(itemImages),
cartItems: many(cartItems),
orders: many(orders),
itemDrafts: many(itemDrafts),
itemTags: many(itemTags),
checkoutItems: many(checkoutItems),
favorites: many(favorites),
}));
export const categoriesRelations = relations(categories, ({one, many}) => ({
items: many(items),
category: one(categories, {
fields: [categories.parentId],
references: [categories.id],
relationName: "categories_parentId_categories_id"
}),
categories: many(categories, {
relationName: "categories_parentId_categories_id"
}),
itemDrafts: many(itemDrafts),
}));
export const itemImagesRelations = relations(itemImages, ({one}) => ({
item: one(items, {
fields: [itemImages.itemId],
references: [items.id]
}),
}));
export const customerSessionsRelations = relations(customerSessions, ({one}) => ({
customer: one(customers, {
fields: [customerSessions.customerId],
references: [customers.id]
}),
}));
export const customersRelations = relations(customers, ({many}) => ({
customerSessions: many(customerSessions),
customerTokens: many(customerTokens),
carts: many(carts),
shippingAddresses: many(shippingAddresses),
checkouts: many(checkouts),
orders: many(orders),
favorites: many(favorites),
}));
export const customerTokensRelations = relations(customerTokens, ({one}) => ({
customer: one(customers, {
fields: [customerTokens.customerId],
references: [customers.id]
}),
}));
export const cartsRelations = relations(carts, ({one, many}) => ({
customer: one(customers, {
fields: [carts.customerId],
references: [customers.id]
}),
cartItems: many(cartItems),
}));
export const cartItemsRelations = relations(cartItems, ({one}) => ({
cart: one(carts, {
fields: [cartItems.cartId],
references: [carts.id]
}),
item: one(items, {
fields: [cartItems.itemId],
references: [items.id]
}),
}));
export const shippingAddressesRelations = relations(shippingAddresses, ({one, many}) => ({
customer: one(customers, {
fields: [shippingAddresses.customerId],
references: [customers.id]
}),
checkouts: many(checkouts),
}));
export const checkoutsRelations = relations(checkouts, ({one, many}) => ({
customer: one(customers, {
fields: [checkouts.customerId],
references: [customers.id]
}),
shippingAddress: one(shippingAddresses, {
fields: [checkouts.shippingAddressId],
references: [shippingAddresses.id]
}),
orders: many(orders),
checkoutItems: many(checkoutItems),
}));
export const ordersRelations = relations(orders, ({one}) => ({
item: one(items, {
fields: [orders.itemId],
references: [items.id]
}),
customer: one(customers, {
fields: [orders.customerId],
references: [customers.id]
}),
checkout: one(checkouts, {
fields: [orders.checkoutId],
references: [checkouts.id]
}),
}));
export const itemDraftsRelations = relations(itemDrafts, ({one}) => ({
item: one(items, {
fields: [itemDrafts.itemId],
references: [items.id]
}),
uploadLink: one(uploadLinks, {
fields: [itemDrafts.uploadLinkId],
references: [uploadLinks.id]
}),
category: one(categories, {
fields: [itemDrafts.aiCategoryId],
references: [categories.id]
}),
}));
export const uploadLinksRelations = relations(uploadLinks, ({many}) => ({
itemDrafts: many(itemDrafts),
}));
export const itemTagsRelations = relations(itemTags, ({one}) => ({
item: one(items, {
fields: [itemTags.itemId],
references: [items.id]
}),
tag: one(tags, {
fields: [itemTags.tagId],
references: [tags.id]
}),
}));
export const tagsRelations = relations(tags, ({many}) => ({
itemTags: many(itemTags),
}));
export const checkoutItemsRelations = relations(checkoutItems, ({one}) => ({
checkout: one(checkouts, {
fields: [checkoutItems.checkoutId],
references: [checkouts.id]
}),
item: one(items, {
fields: [checkoutItems.itemId],
references: [items.id]
}),
}));
export const favoritesRelations = relations(favorites, ({one}) => ({
customer: one(customers, {
fields: [favorites.customerId],
references: [customers.id]
}),
item: one(items, {
fields: [favorites.itemId],
references: [items.id]
}),
}));
-341
View File
@@ -1,341 +0,0 @@
import { pgTable, index, foreignKey, serial, text, integer, timestamp, unique, boolean, jsonb, uniqueIndex, primaryKey, pgSequence } from "drizzle-orm/pg-core"
import { sql } from "drizzle-orm"
export const pgmigrationsIdSeq = pgSequence("pgmigrations_id_seq", { startWith: "1", increment: "1", minValue: "1", maxValue: "2147483647", cache: "1", cycle: false })
export const items = pgTable("items", {
id: serial().primaryKey().notNull(),
name: text().notNull(),
description: text(),
priceCents: integer("price_cents").default(8000).notNull(),
status: text().default('pending').notNull(),
reservedUntil: timestamp("reserved_until", { withTimezone: true, mode: 'string' }),
soldAt: timestamp("sold_at", { withTimezone: true, mode: 'string' }),
paypalOrderId: text("paypal_order_id"),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
categoryId: integer("category_id"),
}, (table) => [
index("items_category_id_idx").using("btree", table.categoryId.asc().nullsLast().op("int4_ops")),
foreignKey({
columns: [table.categoryId],
foreignColumns: [categories.id],
name: "items_category_id_fkey"
}).onDelete("set null"),
]);
export const itemImages = pgTable("item_images", {
id: serial().primaryKey().notNull(),
itemId: integer("item_id").notNull(),
imagePath: text("image_path").notNull(),
sortOrder: integer("sort_order").default(0).notNull(),
originalImagePath: text("original_image_path"),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "item_images_item_id_fkey"
}).onDelete("cascade"),
]);
export const customerSessions = pgTable("customer_sessions", {
token: text().primaryKey().notNull(),
customerId: integer("customer_id").notNull(),
expiresAt: timestamp("expires_at", { withTimezone: true, mode: 'string' }).notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "customer_sessions_customer_id_fkey"
}).onDelete("cascade"),
]);
export const customerTokens = pgTable("customer_tokens", {
token: text().primaryKey().notNull(),
customerId: integer("customer_id").notNull(),
kind: text().notNull(),
expiresAt: timestamp("expires_at", { withTimezone: true, mode: 'string' }).notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "customer_tokens_customer_id_fkey"
}).onDelete("cascade"),
]);
export const adminSettings = pgTable("admin_settings", {
key: text().primaryKey().notNull(),
value: text().notNull(),
updatedAt: timestamp("updated_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
});
export const customers = pgTable("customers", {
id: serial().primaryKey().notNull(),
email: text().notNull(),
passwordHash: text("password_hash").notNull(),
emailVerified: boolean("email_verified").default(false).notNull(),
marketingConsent: boolean("marketing_consent").default(false).notNull(),
marketingConsentAt: timestamp("marketing_consent_at", { withTimezone: true, mode: 'string' }),
marketingConsentText: text("marketing_consent_text"),
unsubscribeToken: text("unsubscribe_token").notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
disabledAt: timestamp("disabled_at", { withTimezone: true, mode: 'string' }),
favoriteAlerts: boolean("favorite_alerts").default(false).notNull(),
favoriteAlertsAt: timestamp("favorite_alerts_at", { withTimezone: true, mode: 'string' }),
favoriteAlertsText: text("favorite_alerts_text"),
firstName: text("first_name"),
lastName: text("last_name"),
}, (table) => [
index("customers_disabled_at_idx").using("btree", table.disabledAt.asc().nullsLast().op("timestamptz_ops")).where(sql`(disabled_at IS NOT NULL)`),
unique("customers_email_key").on(table.email),
unique("customers_unsubscribe_token_key").on(table.unsubscribeToken),
]);
export const carts = pgTable("carts", {
id: serial().primaryKey().notNull(),
customerId: integer("customer_id").notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
updatedAt: timestamp("updated_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "carts_customer_id_fkey"
}).onDelete("cascade"),
unique("carts_customer_id_key").on(table.customerId),
]);
export const cartItems = pgTable("cart_items", {
id: serial().primaryKey().notNull(),
cartId: integer("cart_id").notNull(),
itemId: integer("item_id").notNull(),
addedAt: timestamp("added_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
expiresAt: timestamp("expires_at", { withTimezone: true, mode: 'string' }).notNull(),
lastReminderSentAt: timestamp("last_reminder_sent_at", { withTimezone: true, mode: 'string' }),
}, (table) => [
foreignKey({
columns: [table.cartId],
foreignColumns: [carts.id],
name: "cart_items_cart_id_fkey"
}).onDelete("cascade"),
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "cart_items_item_id_fkey"
}).onDelete("cascade"),
unique("cart_items_item_id_key").on(table.itemId),
]);
export const shippingAddresses = pgTable("shipping_addresses", {
id: serial().primaryKey().notNull(),
customerId: integer("customer_id").notNull(),
fullName: text("full_name").notNull(),
addressLine1: text("address_line1").notNull(),
addressLine2: text("address_line2"),
city: text().notNull(),
state: text().notNull(),
postalCode: text("postal_code").notNull(),
country: text().default('US').notNull(),
isDefault: boolean("is_default").default(false).notNull(),
uspsValidated: boolean("usps_validated").default(false).notNull(),
uspsStandardized: jsonb("usps_standardized"),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "shipping_addresses_customer_id_fkey"
}).onDelete("cascade"),
]);
export const checkouts = pgTable("checkouts", {
id: serial().primaryKey().notNull(),
customerId: integer("customer_id"),
shippingAddressId: integer("shipping_address_id"),
processor: text().notNull(),
processorOrderId: text("processor_order_id"),
amountCents: integer("amount_cents"),
status: text().default('pending').notNull(),
rawEvent: jsonb("raw_event"),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "checkouts_customer_id_fkey"
}).onDelete("set null"),
foreignKey({
columns: [table.shippingAddressId],
foreignColumns: [shippingAddresses.id],
name: "checkouts_shipping_address_id_fkey"
}).onDelete("set null"),
]);
export const orders = pgTable("orders", {
id: serial().primaryKey().notNull(),
itemId: integer("item_id"),
customerId: integer("customer_id"),
checkoutId: integer("checkout_id"),
processor: text().notNull(),
processorOrderId: text("processor_order_id"),
amountCents: integer("amount_cents"),
status: text(),
rawEvent: jsonb("raw_event"),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "orders_item_id_fkey"
}),
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "orders_customer_id_fkey"
}).onDelete("set null"),
foreignKey({
columns: [table.checkoutId],
foreignColumns: [checkouts.id],
name: "orders_checkout_id_fkey"
}).onDelete("set null"),
]);
export const categories = pgTable("categories", {
id: serial().primaryKey().notNull(),
name: text().notNull(),
parentId: integer("parent_id"),
sortOrder: integer("sort_order").default(0).notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
uniqueIndex("categories_child_name_uniq").using("btree", sql`parent_id`, sql`lower(name)`).where(sql`(parent_id IS NOT NULL)`),
index("categories_parent_id_idx").using("btree", table.parentId.asc().nullsLast().op("int4_ops")),
uniqueIndex("categories_root_name_uniq").using("btree", sql`lower(name)`).where(sql`(parent_id IS NULL)`),
foreignKey({
columns: [table.parentId],
foreignColumns: [table.id],
name: "categories_parent_id_fkey"
}).onDelete("cascade"),
]);
export const tags = pgTable("tags", {
id: serial().primaryKey().notNull(),
name: text().notNull(),
color: text().notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
uniqueIndex("tags_name_uniq").using("btree", sql`lower(name)`),
]);
export const itemDrafts = pgTable("item_drafts", {
id: serial().primaryKey().notNull(),
itemId: integer("item_id").notNull(),
uploadLinkId: integer("upload_link_id"),
submitterNote: text("submitter_note"),
removeBackground: boolean("remove_background").default(true).notNull(),
state: text().default('queued').notNull(),
attempts: integer().default(0).notNull(),
model: text(),
aiName: text("ai_name"),
aiDescription: text("ai_description"),
aiCategoryId: integer("ai_category_id"),
aiTagNames: text("ai_tag_names").array(),
aiSuggestedPriceCents: integer("ai_suggested_price_cents"),
priceSource: text("price_source").default('default').notNull(),
aiError: text("ai_error"),
inputTokens: integer("input_tokens"),
outputTokens: integer("output_tokens"),
costMicros: integer("cost_micros"),
draftedAt: timestamp("drafted_at", { withTimezone: true, mode: 'string' }),
reviewedAt: timestamp("reviewed_at", { withTimezone: true, mode: 'string' }),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
index("item_drafts_state_idx").using("btree", table.state.asc().nullsLast().op("text_ops")),
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "item_drafts_item_id_fkey"
}).onDelete("cascade"),
foreignKey({
columns: [table.uploadLinkId],
foreignColumns: [uploadLinks.id],
name: "item_drafts_upload_link_id_fkey"
}).onDelete("set null"),
foreignKey({
columns: [table.aiCategoryId],
foreignColumns: [categories.id],
name: "item_drafts_ai_category_id_fkey"
}).onDelete("set null"),
unique("item_drafts_item_id_key").on(table.itemId),
]);
export const uploadLinks = pgTable("upload_links", {
id: serial().primaryKey().notNull(),
label: text().notNull(),
tokenHash: text("token_hash").notNull(),
revokedAt: timestamp("revoked_at", { withTimezone: true, mode: 'string' }),
submissionCount: integer("submission_count").default(0).notNull(),
maxSubmissions: integer("max_submissions"),
lastUsedAt: timestamp("last_used_at", { withTimezone: true, mode: 'string' }),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
contactEmail: text("contact_email"),
}, (table) => [
unique("upload_links_token_hash_key").on(table.tokenHash),
]);
export const itemTags = pgTable("item_tags", {
itemId: integer("item_id").notNull(),
tagId: integer("tag_id").notNull(),
}, (table) => [
index("item_tags_tag_id_idx").using("btree", table.tagId.asc().nullsLast().op("int4_ops")),
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "item_tags_item_id_fkey"
}).onDelete("cascade"),
foreignKey({
columns: [table.tagId],
foreignColumns: [tags.id],
name: "item_tags_tag_id_fkey"
}).onDelete("cascade"),
primaryKey({ columns: [table.itemId, table.tagId], name: "item_tags_pkey"}),
]);
export const checkoutItems = pgTable("checkout_items", {
checkoutId: integer("checkout_id").notNull(),
itemId: integer("item_id").notNull(),
priceCents: integer("price_cents").notNull(),
}, (table) => [
foreignKey({
columns: [table.checkoutId],
foreignColumns: [checkouts.id],
name: "checkout_items_checkout_id_fkey"
}).onDelete("cascade"),
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "checkout_items_item_id_fkey"
}).onDelete("cascade"),
primaryKey({ columns: [table.checkoutId, table.itemId], name: "checkout_items_pkey"}),
]);
export const favorites = pgTable("favorites", {
customerId: integer("customer_id").notNull(),
itemId: integer("item_id").notNull(),
createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(),
}, (table) => [
index("favorites_item_id_idx").using("btree", table.itemId.asc().nullsLast().op("int4_ops")),
foreignKey({
columns: [table.customerId],
foreignColumns: [customers.id],
name: "favorites_customer_id_fkey"
}).onDelete("cascade"),
foreignKey({
columns: [table.itemId],
foreignColumns: [items.id],
name: "favorites_item_id_fkey"
}).onDelete("cascade"),
primaryKey({ columns: [table.customerId, table.itemId], name: "favorites_pkey"}),
]);
+80
View File
@@ -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` was 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 #308 converted it: it is now `itemFilterExpressions` in `src/itemFilters.ts`, returning Kysely expressions that the storefront and admin listings compose with `eb.and`. What follows is the shape it took, kept here because it is the worked reference for converting anything else of that difficulty.
```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.
+231
View File
@@ -0,0 +1,231 @@
/**
* This file was generated by kysely-codegen.
* Please do not edit it manually.
*/
import type { ColumnType } from "kysely";
export type Generated<T> = T extends ColumnType<infer S, infer I, infer U>
? ColumnType<S, I | undefined, U>
: ColumnType<T, T | undefined, T>;
export type Json = JsonValue;
export type JsonArray = JsonValue[];
export type JsonObject = {
[x: string]: JsonValue | undefined;
};
export type JsonPrimitive = boolean | number | string | null;
export type JsonValue = JsonArray | JsonObject | JsonPrimitive;
export type Timestamp = ColumnType<Date, Date | string, Date | string>;
export interface AdminSettings {
key: string;
updated_at: Generated<Timestamp>;
value: string;
}
export interface CartItems {
added_at: Generated<Timestamp>;
cart_id: number;
expires_at: Timestamp;
id: Generated<number>;
item_id: number;
last_reminder_sent_at: Timestamp | null;
}
export interface Carts {
created_at: Generated<Timestamp>;
customer_id: number;
id: Generated<number>;
updated_at: Generated<Timestamp>;
}
export interface Categories {
created_at: Generated<Timestamp>;
id: Generated<number>;
name: string;
parent_id: number | null;
sort_order: Generated<number>;
}
export interface CheckoutItems {
checkout_id: number;
item_id: number;
price_cents: number;
}
export interface Checkouts {
amount_cents: number | null;
created_at: Generated<Timestamp>;
customer_id: number | null;
id: Generated<number>;
processor: string;
processor_order_id: string | null;
raw_event: Json | null;
shipping_address_id: number | null;
status: Generated<string>;
}
export interface Customers {
created_at: Generated<Timestamp>;
disabled_at: Timestamp | null;
email: string;
email_verified: Generated<boolean>;
favorite_alerts: Generated<boolean>;
favorite_alerts_at: Timestamp | null;
favorite_alerts_text: string | null;
first_name: string | null;
id: Generated<number>;
last_name: string | null;
marketing_consent: Generated<boolean>;
marketing_consent_at: Timestamp | null;
marketing_consent_text: string | null;
password_hash: string;
unsubscribe_token: string;
}
export interface CustomerSessions {
created_at: Generated<Timestamp>;
customer_id: number;
expires_at: Timestamp;
token: string;
}
export interface CustomerTokens {
created_at: Generated<Timestamp>;
customer_id: number;
expires_at: Timestamp;
kind: string;
token: string;
}
export interface Favorites {
created_at: Generated<Timestamp>;
customer_id: number;
item_id: number;
}
export interface ItemDrafts {
ai_category_id: number | null;
ai_description: string | null;
ai_error: string | null;
ai_name: string | null;
ai_suggested_price_cents: number | null;
ai_tag_names: string[] | null;
attempts: Generated<number>;
cost_micros: number | null;
created_at: Generated<Timestamp>;
drafted_at: Timestamp | null;
id: Generated<number>;
input_tokens: number | null;
item_id: number;
model: string | null;
output_tokens: number | null;
price_source: Generated<string>;
remove_background: Generated<boolean>;
reviewed_at: Timestamp | null;
state: Generated<string>;
submitter_note: string | null;
upload_link_id: number | null;
}
export interface ItemImages {
created_at: Generated<Timestamp>;
id: Generated<number>;
image_path: string;
item_id: number;
original_image_path: string | null;
sort_order: Generated<number>;
}
export interface Items {
category_id: number | null;
created_at: Generated<Timestamp>;
description: string | null;
id: Generated<number>;
name: string;
paypal_order_id: string | null;
price_cents: Generated<number>;
reserved_until: Timestamp | null;
sold_at: Timestamp | null;
status: Generated<string>;
}
export interface ItemTags {
item_id: number;
tag_id: number;
}
export interface Orders {
amount_cents: number | null;
checkout_id: number | null;
created_at: Generated<Timestamp>;
customer_id: number | null;
id: Generated<number>;
item_id: number | null;
processor: string;
processor_order_id: string | null;
raw_event: Json | null;
status: string | null;
}
export interface ShippingAddresses {
address_line1: string;
address_line2: string | null;
city: string;
country: Generated<string>;
created_at: Generated<Timestamp>;
customer_id: number;
full_name: string;
id: Generated<number>;
is_default: Generated<boolean>;
postal_code: string;
state: string;
usps_standardized: Json | null;
usps_validated: Generated<boolean>;
}
export interface Tags {
color: string;
created_at: Generated<Timestamp>;
id: Generated<number>;
name: string;
}
export interface UploadLinks {
contact_email: string | null;
created_at: Generated<Timestamp>;
id: Generated<number>;
label: string;
last_used_at: Timestamp | null;
max_submissions: number | null;
revoked_at: Timestamp | null;
submission_count: Generated<number>;
token_hash: string;
}
export interface DB {
admin_settings: AdminSettings;
cart_items: CartItems;
carts: Carts;
categories: Categories;
checkout_items: CheckoutItems;
checkouts: Checkouts;
customer_sessions: CustomerSessions;
customer_tokens: CustomerTokens;
customers: Customers;
favorites: Favorites;
item_drafts: ItemDrafts;
item_images: ItemImages;
item_tags: ItemTags;
items: Items;
orders: Orders;
shipping_addresses: ShippingAddresses;
tags: Tags;
upload_links: UploadLinks;
}
+20 -16
View File
@@ -1,6 +1,6 @@
import { Pool } from 'pg'; import { Pool } from 'pg';
import { drizzle } from 'drizzle-orm/node-postgres'; import { Kysely, PostgresDialect } from 'kysely';
import * as schema from './db-drizzle/schema'; import type { DB } from './db-kysely/schema';
export const pool = new Pool({ export const pool = new Pool({
host: process.env.PGHOST, host: process.env.PGHOST,
@@ -11,25 +11,29 @@ export const pool = new Pool({
}); });
/** /**
* Drizzle over the same pool, alongside `pool` rather than instead of it. * Kysely over the same pool, alongside `pool` rather than instead of it.
* *
* Both have to work at once: the conversion decided in #216 is file by file * Both have to work at once: the conversion is file by file across 238 call
* across 187 call sites, so for a long time most queries will still be raw `pg` * sites, so for a long time most queries will still be raw `pg` and the two
* and the two must share one set of connections. Handing drizzle the existing * must share one set of connections. Handing Kysely the existing pool rather
* pool rather than letting it open its own is what makes that true — otherwise * than letting it open its own is what makes that true — otherwise a
* a transaction started on one would be invisible to the other, and the pool * transaction started on one would be invisible to the other, and the pool
* limits would silently double. * limits would silently double.
* *
* The value of this over raw `pg` is not brevity. In a Drizzle `sql` template * 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 * `${value}` emits a bind parameter, never text, so there is no way to spell
* spell "interpolate this as SQL" by accident — the escape hatch that looks * "interpolate this as SQL" by accident. That makes the #202 invariant
* like a plain template literal does not behave like one. That makes the #202 * structural instead of a comment plus two mutation tests, and it is the main
* invariant structural instead of a comment plus two mutation tests, and it is * reason a builder is here at all.
* the main reason this adoption is worth doing.
* *
* The trap that goes with it is arrays. See db-drizzle/CONVENTIONS.md. * 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 = drizzle(pool, { schema }); export const db = new Kysely<DB>({ dialect: new PostgresDialect({ pool }) });
/** /**
* The single row a query is guaranteed to have returned. * The single row a query is guaranteed to have returned.
+45 -54
View File
@@ -2,7 +2,9 @@
// filters. Kept apart from the route so the rules can be unit-tested without a // filters. Kept apart from the route so the rules can be unit-tested without a
// database, and so items.ts stays a thin handler. // database, and so items.ts stays a thin handler.
import { Expression, SqlBool, sql } from 'kysely';
import { ItemStatus } from './types'; import { ItemStatus } from './types';
import { ItemContext } from './itemSelect';
export type { ItemStatus }; export type { ItemStatus };
export class FilterError extends Error {} export class FilterError extends Error {}
@@ -59,11 +61,6 @@ export const STOREFRONT_DEFAULT_STATUSES: readonly ItemStatus[] = ['available',
// shape this codebase keeps designing against. // shape this codebase keeps designing against.
export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold']; export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold'];
export interface BuiltFilter {
clauses: string[];
params: unknown[];
}
// Deliberately excludes a leading sign and any decimal point: every filter // Deliberately excludes a leading sign and any decimal point: every filter
// value is a non-negative integer (an id, or a price in cents), so '-1' and // value is a non-negative integer (an id, or a price in cents), so '-1' and
// '10.5' are caller mistakes worth surfacing rather than silently coercing. // '10.5' are caller mistakes worth surfacing rather than silently coercing.
@@ -158,7 +155,7 @@ function parseTagIds(value: unknown): number[] {
continue; continue;
} }
const id = parseId(trimmed, 'tags'); const id = parseId(trimmed, 'tags');
// Duplicates would inflate the required-match count in buildItemFilterSql // Duplicates would inflate the required-match count in itemFilterExpressions
// and make the filter match nothing at all. // and make the filter match nothing at all.
if (!tagIds.includes(id)) { if (!tagIds.includes(id)) {
tagIds.push(id); tagIds.push(id);
@@ -236,24 +233,21 @@ export function parseItemFilters(query: Record<string, unknown>): ItemFilters {
return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly }; return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly };
} }
// Returns WHERE fragments plus their parameters, with placeholders numbered // Composes the filter clauses as Kysely expressions.
// from `startIndex` so the caller can splice these in after its own params.
// //
// SECURITY INVARIANT, and it is load-bearing. Both callers splice these clauses // This returned `{ clauses: string[]; params: unknown[] }` until #308, and both
// straight into query text — admin.ts as `${ADMIN_ITEM_SELECT} ${where}`, and // callers spliced the clauses straight into query text. The invariant that made
// items.ts as `${PUBLIC_ITEM_SELECT} WHERE ${where}`, which is reachable // that safe — only a placeholder index may ever be interpolated into a clause,
// without signing in. So the only thing that may ever be interpolated into a // never a value — was a sixteen-line comment and two tests standing between an
// string pushed onto `clauses` is a placeholder index: `$${next}`, or // edit and a live injection on a route reachable without signing in.
// `$${next + 1}` in the tags clause. Every value goes onto `params` and is
// bound by the driver. Interpolating a filter value here would be SQL injection
// at both call sites, and `parseItemFilters` refusing malformed input is not
// what prevents it — these literals would be safe with no parser at all.
// //
// Stated here rather than only at the call sites because this is where the rule // It is now a property of the type system. `${value}` inside a Kysely `sql`
// is enforced and where a seventh clause would be added. SonarQube raised S2077 // template emits a bind parameter, never text, and the builder expressions
// on the call sites and they are marked Reviewed/Safe (#180); that marking does // cannot express interpolation at all. The two tests at the bottom of
// not re-raise when this file changes, so this comment and the two tests over // itemFilters.test.ts still exist and now assert against the SQL Kysely
// it are what stand between that edit and a live injection. See #202. // actually emits, which is a stronger claim than the one they used to make.
//
// `startIndex` is gone with the splicing it existed for.
// //
// `favoritesCustomerId` is required rather than optional so a caller has to say // `favoritesCustomerId` is required rather than optional so a caller has to say
// whose favorites it means, even when it means nobody's. Both routes already // whose favorites it means, even when it means nobody's. Both routes already
@@ -261,17 +255,14 @@ export function parseItemFilters(query: Record<string, unknown>): ItemFilters {
// a programming error — but it is here so that a future caller which forgets // a programming error — but it is here so that a future caller which forgets
// the guard fails loudly instead of quietly ignoring the filter and listing the // the guard fails loudly instead of quietly ignoring the filter and listing the
// whole catalogue. // whole catalogue.
export function buildItemFilterSql( export function itemFilterExpressions(
eb: ItemContext,
filters: ItemFilters, filters: ItemFilters,
startIndex: number,
favoritesCustomerId: number | null favoritesCustomerId: number | null
): BuiltFilter { ): Expression<SqlBool>[] {
const clauses: string[] = []; const clauses: Expression<SqlBool>[] = [];
const params: unknown[] = [];
let next = startIndex;
if (filters.categoryIds.length) { if (filters.categoryIds.length) {
params.push(filters.categoryIds);
// Selecting a category means "and everything filed beneath it", so walk the // Selecting a category means "and everything filed beneath it", so walk the
// tree down from each chosen node. A recursive CTE keeps the tree // tree down from each chosen node. A recursive CTE keeps the tree
// un-denormalized: reparenting stays a single UPDATE with no stored paths // un-denormalized: reparenting stays a single UPDATE with no stored paths
@@ -281,61 +272,61 @@ export function buildItemFilterSql(
// walked in the same recursion. That also gives the OR for free: the union // walked in the same recursion. That also gives the OR for free: the union
// of the subtrees is exactly "filed under any of these", and an item filed // of the subtrees is exactly "filed under any of these", and an item filed
// under two selected branches appears once because IN is a set test. // under two selected branches appears once because IN is a set test.
clauses.push(`i.category_id IN ( //
// Still a `sql` template, because the builder expresses a recursive CTE no
// better than this does. `${filters.categoryIds}` is one bind parameter
// holding the whole array — not a placeholder list — which is why no
// sql.param() ceremony appears here. See src/db-kysely/CONVENTIONS.md.
clauses.push(sql<SqlBool>`i.category_id IN (
WITH RECURSIVE subtree AS ( WITH RECURSIVE subtree AS (
SELECT id FROM categories WHERE id = ANY($${next}::int[]) SELECT id FROM categories WHERE id = ANY(${filters.categoryIds}::int[])
UNION ALL UNION ALL
SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id
) )
SELECT id FROM subtree SELECT id FROM subtree
)`); )`);
next++;
} }
if (filters.tagIds.length) { if (filters.tagIds.length) {
params.push(filters.tagIds, filters.tagIds.length);
// AND, not OR: the item must carry every selected tag. Matching with // AND, not OR: the item must carry every selected tag. Matching with
// `tag_id = ANY(...)` alone would return items holding just one of them, so // `tag_id = ANY(...)` alone would return items holding just one of them, so
// the count of matched rows has to equal the number requested. // the count of matched rows has to equal the number requested.
clauses.push( clauses.push(sql<SqlBool>`(SELECT COUNT(*) FROM item_tags it
`(SELECT COUNT(*) FROM item_tags it WHERE it.item_id = i.id AND it.tag_id = ANY(${filters.tagIds}::int[])) = ${filters.tagIds.length}`);
WHERE it.item_id = i.id AND it.tag_id = ANY($${next}::int[])) = $${next + 1}`
);
next += 2;
} }
if (filters.minPriceCents !== null) { if (filters.minPriceCents !== null) {
params.push(filters.minPriceCents); clauses.push(eb('i.price_cents', '>=', filters.minPriceCents));
clauses.push(`i.price_cents >= $${next}`);
next++;
} }
if (filters.maxPriceCents !== null) { if (filters.maxPriceCents !== null) {
params.push(filters.maxPriceCents); clauses.push(eb('i.price_cents', '<=', filters.maxPriceCents));
clauses.push(`i.price_cents <= $${next}`);
next++;
} }
if (filters.status !== null) { if (filters.status !== null) {
params.push(filters.status); // `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits
// ANY rather than equality, so one status and several use the same clause. // the placeholder list itself, so one status and several use the same
// The ::text[] cast is explicit because `status` is a text column and the // expression and the explicit ::text[] cast is no longer needed.
// driver would otherwise have to infer the array's element type. clauses.push(eb('i.status', 'in', filters.status));
clauses.push(`i.status = ANY($${next}::text[])`);
next++;
} }
if (filters.favoritesOnly) { if (filters.favoritesOnly) {
if (favoritesCustomerId === null) { if (favoritesCustomerId === null) {
throw new Error('favorites filter requires a customer id'); throw new Error('favorites filter requires a customer id');
} }
params.push(favoritesCustomerId);
// EXISTS rather than a join: an item is favorited by a customer at most // EXISTS rather than a join: an item is favorited by a customer at most
// once, but joining would still risk multiplying rows if that ever changed, // once, but joining would still risk multiplying rows if that ever changed,
// and this reads as the membership test it is. // and this reads as the membership test it is.
clauses.push(`EXISTS (SELECT 1 FROM favorites f WHERE f.item_id = i.id AND f.customer_id = $${next})`); clauses.push(
next++; eb.exists(
eb
.selectFrom('favorites as f')
.select('f.item_id')
.whereRef('f.item_id', '=', 'i.id')
.where('f.customer_id', '=', favoritesCustomerId)
)
);
} }
return { clauses, params }; return clauses;
} }
+115 -71
View File
@@ -1,95 +1,139 @@
// Shared item SELECT shapes for the public and admin routes, and the row types // Shared item query shapes for the public and admin routes, and the row types
// they return. // they return.
// //
// The types live here rather than in types.ts because they describe a // The types live here rather than in types.ts because they describe a
// projection, not a table. ADMIN_ITEM_SELECT takes `i.*` and PUBLIC_ITEM_SELECT // projection, not a table. adminItemQuery takes every column of items and
// names its columns so the storefront never sees paypal_order_id or // publicItemQuery names its columns, so the storefront never sees
// reserved_until — typing both as "an items row" would quietly re-admit exactly // paypal_order_id or reserved_until — typing both as "an items row" would
// the columns that select was written to exclude. // quietly re-admit exactly the columns that projection was written to exclude.
// //
// KEPT IN STEP BY HAND. `pool.query<T>` asserts a shape; it does not check the // These were SQL string constants until #308. They had to be kept in step with
// SQL, which TypeScript never reads. Dropping a column from a select below // their row types by hand, because `pool.query<T>` asserts a shape and never
// without dropping it from its type compiles cleanly and every read of it goes // checks it against the SQL, so dropping a column from a select without
// on type-checking while being undefined at runtime. The integration suite is // dropping it from its type compiled cleanly and went undefined at run time —
// the only thing that catches that, because it runs these queries against a // and only the integration suite ever caught it. Built through Kysely, that is
// real schema. Change a select and its type together. // a compile error, because the row type now follows from the projection.
// //
// Images and tags are pulled as scalar subqueries rather than LEFT JOIN + // Images and tags are pulled as aggregate subqueries rather than LEFT JOIN +
// GROUP BY. Joining two one-to-many relations in the same query multiplies // GROUP BY. Joining two one-to-many relations in the same query multiplies
// their rows together — an item with 2 images and 3 tags would aggregate 6 // their rows together — an item with 2 images and 3 tags would aggregate 6
// rows, silently repeating every image three times. Subqueries keep each // rows, silently repeating every image three times. Subqueries keep each
// aggregate independent and drop the GROUP BY entirely. // aggregate independent and drop the GROUP BY entirely. `jsonArrayFrom` emits
// `coalesce(json_agg(agg), '[]')`, which is what these hand-wrote before.
import { ExpressionBuilder, Generated, Kysely } from 'kysely';
import { jsonArrayFrom } from 'kysely/helpers/postgres';
import { db } from './db';
import { DB } from './db-kysely/schema';
import { ItemStatus, ItemImage, ItemTag } from './types'; import { ItemStatus, ItemImage, ItemTag } from './types';
const IMAGES_SUBQUERY = ` /**
COALESCE(( * The mirror types `items.status` as `Generated<string>` because Postgres holds
SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order) * it as CHECK-constrained text rather than a native enum, so `kysely-codegen`
ORDER BY img.sort_order) * has nothing narrower to emit. `types.ts` already states the real domain.
FROM item_images img *
WHERE img.item_id = i.id * Narrowed here, once, rather than asserted at each call site. `$castTo` at the
), '[]') AS images`; * call site would have replaced the entire row type with an assertion — which
* would silently accept a projection that had lost a column, and losing the
* compile error on exactly that is what this change exists to prevent.
*/
type ItemsWithStatus = Omit<DB['items'], 'status'> & { status: Generated<ItemStatus> };
type ItemDB = Omit<DB, 'items'> & { items: ItemsWithStatus };
const itemDb = db as unknown as Kysely<ItemDB>;
/**
* The aliases every item query and every filter clause is written against.
*
* `i` and `c` are kept from the SQL these replaced. Not because short names are
* better, but because the filter clauses, the subquery correlations and the
* ORDER BY all reference them, and renaming them in the same change that moved
* the builder would have made the diff unreadable against the SQL it replaces.
*/
export type ItemContext = ExpressionBuilder<
ItemDB & { i: ItemDB['items']; c: ItemDB['categories'] },
'i' | 'c'
>;
/** The public image fields. Correlated to the outer item by `whereRef`. */
function imagesFor(eb: ItemContext) {
return jsonArrayFrom(
eb
.selectFrom('item_images as img')
.select(['img.id', 'img.image_path', 'img.sort_order'])
.whereRef('img.item_id', '=', 'i.id')
.orderBy('img.sort_order')
).as('images');
}
/** /**
* Admin-only images, carrying `original_image_path` alongside the public * Admin-only images, carrying `original_image_path` alongside the public
* fields — the field the inventory screen needs to know whether a photo has a * fields — the field the inventory screen needs to know whether a photo has a
* cut-out to restore (#293). * cut-out to restore (#293).
* *
* A separate subquery rather than adding the column to `IMAGES_SUBQUERY` * A separate function rather than a flag on `imagesFor`, for the same reason
* itself, for the same reason `PUBLIC_ITEM_SELECT` names its columns instead * `publicItemQuery` names its columns instead of taking them all: an original
* of using `i.*`: an original filename is internal nobody's business on the * filename is internal, nobody's business on the storefront, and a boolean in
* storefront — and folding it into the one subquery both selects share would * the middle of the thing that keeps it off the public API is one edit away
* put it in every public item response too. * from being passed wrongly.
*/ */
const ADMIN_IMAGES_SUBQUERY = ` function adminImagesFor(eb: ItemContext) {
COALESCE(( return jsonArrayFrom(
SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path, 'sort_order', img.sort_order, eb
'original_image_path', img.original_image_path) .selectFrom('item_images as img')
ORDER BY img.sort_order) .select(['img.id', 'img.image_path', 'img.sort_order', 'img.original_image_path'])
FROM item_images img .whereRef('img.item_id', '=', 'i.id')
WHERE img.item_id = i.id .orderBy('img.sort_order')
), '[]') AS images`; ).as('images');
}
const TAGS_SUBQUERY = ` function tagsFor(eb: ItemContext) {
COALESCE(( return jsonArrayFrom(
SELECT json_agg(json_build_object('id', t.id, 'name', t.name, 'color', t.color) ORDER BY t.name) eb
FROM item_tags it .selectFrom('item_tags as it')
JOIN tags t ON t.id = it.tag_id .innerJoin('tags as t', 't.id', 'it.tag_id')
WHERE it.item_id = i.id .select(['t.id', 't.name', 't.color'])
), '[]') AS tags`; .whereRef('it.item_id', '=', 'i.id')
.orderBy('t.name')
const FROM_CLAUSE = ` ).as('tags');
FROM items i }
LEFT JOIN categories c ON c.id = i.category_id`;
// The storefront gets an explicit column list — it has no business seeing
// paypal_order_id or reserved_until.
export const PUBLIC_ITEM_SELECT = `
SELECT i.id, i.name, i.description, i.price_cents, i.status, i.created_at, i.category_id,
c.name AS category_name,
${IMAGES_SUBQUERY},
${TAGS_SUBQUERY}
${FROM_CLAUSE}`;
export const ADMIN_ITEM_SELECT = `
SELECT i.*,
c.name AS category_name,
${ADMIN_IMAGES_SUBQUERY},
${TAGS_SUBQUERY}
${FROM_CLAUSE}`;
/** /**
* One admin item, by id — the whole query, not a fragment. * The storefront's projection — an explicit column list, because it has no
* business seeing paypal_order_id or reserved_until.
* *
* A named constant rather than `${ADMIN_ITEM_SELECT} WHERE i.id = $1` written * A function rather than a constant so each caller gets a fresh builder. Kysely
* at each call, so no query call site interpolates anything at all. The id was * builders are immutable, so sharing one would be safe, but a function makes it
* always bound as $1 and never reached the query text, but S2077 fires on the * obvious that adding a `where` does not affect anyone else.
* template literal rather than on the value, because the rule cannot tell a
* module constant from a request field — and neither, at a glance, can a
* reader. Hoisting it makes the property structural instead of an assertion
* somebody has to re-make every time the line moves. See #294.
*/ */
export const ADMIN_ITEM_BY_ID = `${ADMIN_ITEM_SELECT} WHERE i.id = $1`; export function publicItemQuery() {
return itemDb
.selectFrom('items as i')
.leftJoin('categories as c', 'c.id', 'i.category_id')
.select([
'i.id',
'i.name',
'i.description',
'i.price_cents',
'i.status',
'i.created_at',
'i.category_id',
'c.name as category_name'
])
.select(imagesFor)
.select(tagsFor);
}
/** The admin projection — every item column, plus the admin image fields. */
export function adminItemQuery() {
return itemDb
.selectFrom('items as i')
.leftJoin('categories as c', 'c.id', 'i.category_id')
.selectAll('i')
.select('c.name as category_name')
.select(adminImagesFor)
.select(tagsFor);
}
/** The columns every item select returns, whichever of the two it is. */ /** The columns every item select returns, whichever of the two it is. */
interface ItemRowBase { interface ItemRowBase {
@@ -106,7 +150,7 @@ interface ItemRowBase {
tags: ItemTag[]; tags: ItemTag[];
} }
/** What PUBLIC_ITEM_SELECT returns. Deliberately no payment or reservation columns. */ /** What publicItemQuery returns. Deliberately no payment or reservation columns. */
export type PublicItemRow = ItemRowBase; export type PublicItemRow = ItemRowBase;
/** /**
@@ -119,7 +163,7 @@ export interface AdminItemImage extends ItemImage {
} }
/** /**
* What ADMIN_ITEM_SELECT returns: `i.*`, so every column on the table. * What adminItemQuery returns: `i.*`, so every column on the table.
* *
* The extra fields are the ones the storefront is not allowed to see, which is * The extra fields are the ones the storefront is not allowed to see, which is
* the whole reason the two selects differ. `images` is narrowed rather than * the whole reason the two selects differ. `images` is narrowed rather than
+14 -19
View File
@@ -1,10 +1,10 @@
import { Router, Request, Response } from 'express'; import { Router, Request, Response } from 'express';
import { PoolClient } from 'pg'; import { PoolClient } from 'pg';
import { pool, requireRow } from '../db'; import { pool, requireRow } from '../db';
import { ADMIN_ITEM_SELECT, ADMIN_ITEM_BY_ID, AdminItemRow, ItemRecord } from '../itemSelect'; import { adminItemQuery, AdminItemRow, ItemRecord } from '../itemSelect';
import { ItemStatus } from '../types'; import { ItemStatus } from '../types';
import { asyncRoute } from '../asyncRoute'; import { asyncRoute } from '../asyncRoute';
import { parseItemFilters, buildItemFilterSql, FilterError } from '../itemFilters'; import { parseItemFilters, itemFilterExpressions, FilterError } from '../itemFilters';
import { readId, tagColorFor } from '../utils'; import { readId, tagColorFor } from '../utils';
import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts'; import { notifyFavoritersOfSale, notifyFavoritersOfRemoval, collectFavoriteRecipients } from '../favoriteAlerts';
import { removeBackgroundsForItem, restoreOriginalsForItem } from '../intake/backgroundRemoval'; import { removeBackgroundsForItem, restoreOriginalsForItem } from '../intake/backgroundRemoval';
@@ -132,21 +132,16 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => {
return res.status(400).json({ error: 'favorites is not a valid inventory filter' }); return res.status(400).json({ error: 'favorites is not a valid inventory filter' });
} }
// S2077 flags every query below that assembles its SQL as a template literal, // No interpolation, and nothing to argue about. Until #308 this assembled
// and this is the one where that is more than a formality: `where` really is // `${ADMIN_ITEM_SELECT} ${where}` from clauses composed at run time, and
// built at run time. What makes it safe is that buildItemFilterSql composes // sixteen lines in itemFilters.ts explained why that was safe. The clauses
// only string literals written in itemFilters.ts. The only interpolations // are Kysely expressions now: a value cannot reach the SQL text, because the
// inside any of them are placeholder indices — `$${next}`, and `$${next + 1}` // types do not let it.
// in the tags clause — numbers, seeded from the startIndex argument and const rows: AdminItemRow[] = await adminItemQuery()
// incremented locally. Neither is ever derived from a filter value. .where((eb) => eb.and(itemFilterExpressions(eb, filters, null)))
// .orderBy('i.created_at', 'desc')
// So a caller chooses which of six fixed fragments are joined, and supplies .execute();
// every value in `params`, and neither of those becomes SQL. parseItemFilters
// rejects malformed input above, but that is defence in depth rather than the
// reason this holds — the clause literals would be safe without it.
const { clauses, params } = buildItemFilterSql(filters, 1, null);
const where = clauses.length ? `WHERE ${clauses.join(' AND ')}` : '';
const { rows } = await pool.query<AdminItemRow>(`${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC`, params);
res.json(rows); res.json(rows);
})); }));
@@ -175,7 +170,7 @@ router.post('/items', uploadImages, asyncRoute(async (req: Request, res: Respons
// is bound as $1. It always was bound — what changed is that a reader no // is bound as $1. It always was bound — what changed is that a reader no
// longer has to check that the interpolated half carries no caller data, // longer has to check that the interpolated half carries no caller data,
// because there is no interpolated half. See #294. // because there is no interpolated half. See #294.
const { rows: full } = await pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [item.id]); const full = await adminItemQuery().where('i.id', '=', item.id).execute();
res.json(requireRow(full, 'the item just inserted')); res.json(requireRow(full, 'the item just inserted'));
} catch (err) { } catch (err) {
await client.query('ROLLBACK'); await client.query('ROLLBACK');
@@ -229,7 +224,7 @@ router.put('/items/:id', uploadImages, asyncRoute(async (req: Request, res: Resp
// The same constant as the create route above. itemId is caller-controlled // The same constant as the create route above. itemId is caller-controlled
// and goes through the driver as a bound parameter; it never reaches the // and goes through the driver as a bound parameter; it never reaches the
// query text. // query text.
const { rows: full } = await pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [itemId]); const full = await adminItemQuery().where('i.id', '=', itemId).execute();
// The create route beside this one has always used requireRow here. This // The create route beside this one has always used requireRow here. This
// one did not, so an UPDATE matching nothing committed happily, the SELECT // one did not, so an UPDATE matching nothing committed happily, the SELECT
// returned nothing, and the caller got 200 with an empty body — a success // returned nothing, and the caller got 200 with an empty body — a success
+75 -78
View File
@@ -1,38 +1,28 @@
import { Router, Request, Response } from 'express'; import { Router, Request, Response } from 'express';
import { eq, inArray, sql } from 'drizzle-orm'; import { sql } from 'kysely';
import { db, requireRow } from '../db'; import { db, requireRow } from '../db';
import { categories, items } from '../db-drizzle/schema';
import { asyncRoute } from '../asyncRoute'; import { asyncRoute } from '../asyncRoute';
/** /**
* The first file converted to Drizzle (#218), chosen because it is awkward * The one file using the builder (#218, reconverted for Kysely in #305), chosen
* rather than because it is easy — nine sites including a recursive CTE and an * because it is awkward rather than because it is easy — a recursive CTE, a
* array match. See src/db-drizzle/CONVENTIONS.md. * correlated count, and an array match.
* *
* The pool is still available and most of the application still uses it. This * 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` — * Under Drizzle this was a mapping — `{ parent_id: categories.parentId }` — and
* and this API answers in snake_case, which the admin frontend reads. So the * it existed because the generated mirror was camelCase while this API answers
* mapping is explicit here rather than implicit anywhere: selecting the table * snake_case, so selecting the table directly changed the JSON contract with no
* directly would silently change the JSON contract, and no test that checks * test noticing. The generated types now carry the database's own names, so
* status codes would catch it. * there is nothing left to translate and this is just a list of columns four
* * selects happen to share.
* 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.
*/ */
const CATEGORY_COLUMNS = { const CATEGORY_COLUMNS = ['id', 'name', 'parent_id', 'sort_order'] as const;
id: categories.id,
name: categories.name,
parent_id: categories.parentId,
sort_order: categories.sortOrder
};
const router = Router(); const router = Router();
@@ -43,11 +33,13 @@ const UNIQUE_VIOLATION = '23505';
/** /**
* Whether a thrown error is that unique violation. * Whether a thrown error is that unique violation.
* *
* Drizzle wraps driver errors, so the SQLSTATE that used to sit on `err.code` * Both shapes are accepted deliberately. Drizzle wrapped driver errors, moving
* now sits on `err.cause.code`. The old check still compiled and simply never * this SQLSTATE from `err.code` to `err.cause.code`, and the check that only
* matched, turning two 409s into 500s — a conversion hazard with no type error * looked at `err.code` still compiled, never matched, and turned two 409s into
* and no failing build behind it, only two integration tests. Both shapes are * 500s — a conversion hazard with no type error behind it. Kysely uses the `pg`
* accepted so this keeps working either side of a conversion. See #218. * 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 { function isUniqueViolation(err: unknown): boolean {
const direct = (err as { code?: string }).code; 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 * 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. * 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 * Still a `sql` template: the CTE is recursive and is consumed in two different
* recursive and is consumed in two different shapes, and expressing it through * shapes, and expressing it through the builder buys nothing over SQL that is
* the builder bought nothing over the SQL that is already correct and reviewed. * already correct and reviewed. The important part is that `${id}` is a bind
* The important part is that `${id}` here is a bind parameter, not text — there * parameter, not text — there is no way to spell string interpolation in this
* is no way to spell string interpolation in this template by accident, which is * template by accident, which is the property the whole adoption is for.
* the property the whole adoption is for.
*/ */
const subtreeOf = (id: number) => sql` const subtreeOf = (id: number) => sql`
WITH RECURSIVE subtree AS ( WITH RECURSIVE subtree AS (
@@ -89,28 +80,32 @@ function readParentId(value: unknown): number | null | undefined {
} }
async function parentExists(id: number): Promise<boolean> { async function parentExists(id: number): Promise<boolean> {
const rows = await db const row = await db
.select({ id: categories.id }) .selectFrom('categories')
.from(categories) .select('id')
.where(eq(categories.id, id)) .where('id', '=', id)
.limit(1); .executeTakeFirst();
return rows.length > 0; return row !== undefined;
} }
router.get('/', asyncRoute(async (_req: Request, res: Response) => { router.get('/', asyncRoute(async (_req: Request, res: Response) => {
const rows = await db const rows = await db
.select({ .selectFrom('categories')
...CATEGORY_COLUMNS, .select(CATEGORY_COLUMNS)
// Written as literal SQL, NOT with ${items.categoryId} and // Literal text rather than interpolated column references, and here that is
// ${categories.id}. Drizzle renders a column reference inside a sql // a free choice rather than a workaround: the fragment binds no values, so
// template UNQUALIFIED — those two produced `WHERE "category_id" = "id"`, // there is nothing to parameterize. Under Drizzle this had to be literal,
// which Postgres resolved against items for both sides and answered with // because interpolating the columns rendered them unqualified and Postgres
// a plausible wrong number rather than an error. There are no values to // resolved both sides against items, answering with a plausible wrong
// bind in this fragment, so literal text is the honest form. See #218. // number rather than an error (#218).
item_count: sql<number>`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)` .select(
}) sql<number>`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)`.as(
.from(categories) 'item_count'
.orderBy(categories.sortOrder, sql`lower(categories.name)`); )
)
.orderBy('sort_order')
.orderBy(sql`lower(categories.name)`)
.execute();
res.json(rows); res.json(rows);
})); }));
@@ -134,9 +129,10 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => {
try { try {
const rows = await db const rows = await db
.insert(categories) .insertInto('categories')
.values({ name, parentId: parent, sortOrder }) .values({ name, parent_id: parent, sort_order: sortOrder })
.returning(CATEGORY_COLUMNS); .returning(CATEGORY_COLUMNS)
.execute();
res.status(201).json({ ...requireRow(rows, 'the category INSERT'), item_count: 0 }); res.status(201).json({ ...requireRow(rows, 'the category INSERT'), item_count: 0 });
} catch (err) { } catch (err) {
@@ -175,9 +171,9 @@ async function resolveParentId(
// Moving a node beneath itself or one of its own descendants would detach // Moving a node beneath itself or one of its own descendants would detach
// that whole branch from the tree into an unreachable cycle. // that whole branch from the tree into an unreachable cycle.
const cycle = await db.execute( const cycle = await sql<{ found: number }>`
sql`${subtreeOf(id)} SELECT 1 FROM subtree WHERE id = ${parsed}` ${subtreeOf(id)} SELECT 1 AS found FROM subtree WHERE id = ${parsed}
); `.execute(db);
if (cycle.rows.length) { if (cycle.rows.length) {
return { error: 'a category cannot be moved beneath itself' }; 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) => { router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
const id = Number(req.params.id); const id = Number(req.params.id);
const existing = await db const current = await db
.selectFrom('categories')
.select(CATEGORY_COLUMNS) .select(CATEGORY_COLUMNS)
.from(categories) .where('id', '=', id)
.where(eq(categories.id, id)) .executeTakeFirst();
.limit(1);
const current = existing[0];
if (!current) { if (!current) {
return res.status(404).json({ error: 'not found' }); return res.status(404).json({ error: 'not found' });
} }
@@ -219,10 +214,11 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
try { try {
const rows = await db const rows = await db
.update(categories) .updateTable('categories')
.set({ name, parentId: parent, sortOrder }) .set({ name, parent_id: parent, sort_order: sortOrder })
.where(eq(categories.id, id)) .where('id', '=', id)
.returning(CATEGORY_COLUMNS); .returning(CATEGORY_COLUMNS)
.execute();
res.json(requireRow(rows, 'the category UPDATE')); res.json(requireRow(rows, 'the category UPDATE'));
} catch (err) { } catch (err) {
@@ -236,27 +232,28 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => {
router.delete('/:id', asyncRoute(async (req: Request, res: Response) => { router.delete('/:id', asyncRoute(async (req: Request, res: Response) => {
const id = Number(req.params.id); const id = Number(req.params.id);
const subtree = await db.execute<{ id: number }>( const subtree = await sql<{ id: number }>`
sql`${subtreeOf(id)} SELECT id FROM subtree` ${subtreeOf(id)} SELECT id FROM subtree
); `.execute(db);
if (!subtree.rows.length) { if (!subtree.rows.length) {
return res.status(404).json({ error: 'not found' }); return res.status(404).json({ error: 'not found' });
} }
const ids = subtree.rows.map((row) => row.id); const ids = subtree.rows.map((row) => row.id);
// inArray rather than the ANY(...::int[]) this replaced, which sidesteps the // `in` rather than the ANY(...::int[]) this replaced. Kysely emits the
// array trap in CONVENTIONS.md entirely: there is no template to forget // placeholder list itself, so it is correct by construction and there is no
// sql.param() in. The builder emits the placeholder list itself and it is // template to forget anything in. `ids` is never empty — the length check
// correct by construction. // above returned already if it were.
const affected = await db const affected = await db
.select({ n: sql<number>`COUNT(*)::int` }) .selectFrom('items')
.from(items) .select(sql<number>`COUNT(*)::int`.as('n'))
.where(inArray(items.categoryId, ids)); .where('category_id', 'in', ids)
.execute();
// The FK cascade takes the descendants; items fall back to NULL rather than // The FK cascade takes the descendants; items fall back to NULL rather than
// being deleted along with their category. // 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({ res.json({
deleted_categories: ids.length, deleted_categories: ids.length,
+34 -32
View File
@@ -1,30 +1,28 @@
import { Router, Request, Response } from 'express'; import { Router, Request, Response } from 'express';
import { pool } from '../db';
import { asyncRoute } from '../asyncRoute'; import { asyncRoute } from '../asyncRoute';
import { PUBLIC_ITEM_SELECT, PublicItemRow } from '../itemSelect'; import { publicItemQuery, PublicItemRow, ItemContext } from '../itemSelect';
import { import {
parseItemFilters, parseItemFilters,
buildItemFilterSql, itemFilterExpressions,
FilterError, FilterError,
NON_PUBLIC_STATUSES, NON_PUBLIC_STATUSES,
STOREFRONT_DEFAULT_STATUSES, STOREFRONT_DEFAULT_STATUSES,
STOREFRONT_ALL_STATUSES STOREFRONT_ALL_STATUSES
} from '../itemFilters'; } from '../itemFilters';
// Applied to every public read, unconditionally. This route has never had a
// status filter of its own — sold items are listed and rendered with a Sold
// badge on purpose — so hiding pending items cannot be expressed as one more
// optional filter. It has to be a clause the caller cannot opt out of.
const EXCLUDE_PENDING = `i.status <> 'pending'`;
/** /**
* One public item, by id — the whole query rather than a fragment. * Pending items are excluded everywhere, not only from the list. A pending item
* that stayed fetchable by id would be hidden from the catalogue and still
* reachable by anyone who guessed or kept a link.
* *
* Built once here so the call site interpolates nothing. Both halves were * An expression rather than the SQL literal this was until #308, so it composes
* always constants and the id was always bound as $1, but a template literal at * with the filter clauses through `eb.and` instead of being joined into a
* a query call is a thing a reader has to verify rather than see. See #294. * string. That join used to need its own argument about why AND could not
* weaken it; `and` cannot re-associate anything.
*/ */
const PUBLIC_ITEM_BY_ID = `${PUBLIC_ITEM_SELECT} WHERE i.id = $1 AND ${EXCLUDE_PENDING}`; function notPending(eb: ItemContext) {
return eb('i.status', '!=', 'pending');
}
const router = Router(); const router = Router();
@@ -81,28 +79,32 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => {
status: filters.status ?? [...defaultStatuses] status: filters.status ?? [...defaultStatuses]
}; };
const { clauses, params } = buildItemFilterSql(effectiveFilters, 1, req.customerId ?? null); const rows: PublicItemRow[] = await publicItemQuery()
// The same construct SonarQube flagged as S2077 in admin.ts and which is .where((eb) =>
// marked Reviewed/Safe there (#180) — and this is the copy reachable without eb.and([
// signing in, so it is worth saying here too rather than relying on the notPending(eb),
// reader having seen the other one. It holds for the same reason: the clauses ...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null)
// are literals from buildItemFilterSql carrying only placeholder indices, and ])
// EXCLUDE_PENDING is a module constant. Joining with AND cannot weaken )
// EXCLUDE_PENDING either, because no fragment contains a top-level OR for the .orderBy('i.created_at', 'desc')
// join to re-associate against. .execute();
const where = [EXCLUDE_PENDING, ...clauses].join(' AND ');
const { rows } = await pool.query<PublicItemRow>(
`${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC`,
params
);
res.json(rows); res.json(rows);
})); }));
router.get('/:id', asyncRoute(async (req: Request, res: Response) => { router.get('/:id', asyncRoute(async (req: Request, res: Response) => {
// Excluded here too, not only from the list. A pending item that stayed // Number() rather than readId(), and that is deliberate rather than an
// fetchable by id would be hidden from the catalogue and still reachable by // oversight. readId would be stricter and would match every other id-taking
// anyone who guessed or kept a link. // route (#207) — but errorHandling.integration.test.ts drives this exact
const { rows } = await pool.query<PublicItemRow>(PUBLIC_ITEM_BY_ID, [req.params.id]); // route with a non-numeric id to prove that asyncRoute plus the error
// middleware answer 500 rather than leaving the request hanging, and a
// stricter parse here would leave that test green while removing the thing
// it tests. Switching this over means giving that test another trigger in
// the same change. See #307.
const rows = await publicItemQuery()
.where('i.id', '=', Number(req.params.id))
.where((eb) => notPending(eb))
.execute();
if (!rows.length) return res.status(404).json({ error: 'not found' }); if (!rows.length) return res.status(404).json({ error: 'not found' });
res.json(rows[0]); res.json(rows[0]);
})); }));
@@ -158,6 +158,20 @@ describe('admin categories', () => {
expect(res.status).toBe(200); expect(res.status).toBe(200);
expect(res.body.category_id).toBeNull(); expect(res.body.category_id).toBeNull();
}); });
// 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 () => {
await createCategory('Duplicate me');
const res = await request(app).post('/api/admin/categories').send({ name: 'Duplicate me' });
expect(res.status).toBe(409);
expect(res.body.error).toMatch(/already exists/);
});
}); });
describe('admin tags', () => { describe('admin tags', () => {
@@ -1,95 +0,0 @@
import { readFileSync } from 'fs';
import path from 'path';
import { pool } from '../../src/db';
import { closeDb } from './setup/testDb';
afterAll(async () => {
await pool.end();
await closeDb();
});
const SCHEMA = readFileSync(
path.join(__dirname, '..', '..', 'src', 'db-drizzle', 'schema.ts'),
'utf8'
);
/** Every table name the generated mirror declares. */
function mirroredTables(): string[] {
return [...SCHEMA.matchAll(/pgTable\("([a-z_]+)"/g)].map((m) => m[1]!).sort();
}
async function liveTables(): Promise<string[]> {
const { rows } = await pool.query<{ table_name: string }>(
`SELECT table_name FROM information_schema.tables
WHERE table_schema = 'public' AND table_type = 'BASE TABLE'
AND table_name <> 'pgmigrations'
ORDER BY table_name`
);
return rows.map((r) => r.table_name);
}
/**
* The guard for #217.
*
* `src/db-drizzle/schema.ts` is generated by `drizzle-kit pull` and is a
* read-only mirror of the real schema, which `backend/migrations` owns. Nothing
* makes anyone re-pull after writing a migration, and that is not hypothetical:
* the mirror sat missing `item_drafts` and `upload_links` from the moment #222
* landed until #217, because it had been copied into src/ by hand and nobody
* had reason to look at it.
*
* A stale mirror is worse than no mirror. Drizzle infers row types from it, so
* a converted query would type-check against a schema the database does not
* have and fail at run time with a column that does not exist — the exact class
* of drift the adoption was meant to close.
*/
describe('the Drizzle schema mirror', () => {
it('declares every table the migrations create', async () => {
const live = await liveTables();
const mirrored = mirroredTables();
const missing = live.filter((name) => !mirrored.includes(name));
expect(missing).toEqual([]);
});
it('declares no table the database does not have', async () => {
const live = await liveTables();
const mirrored = mirroredTables();
const extra = mirrored.filter((name) => !live.includes(name));
expect(extra).toEqual([]);
});
// pgmigrations is node-pg-migrate's bookkeeping, excluded by tablesFilter in
// drizzle.config.ts. A re-pull without that filter would quietly put it back.
it('excludes node-pg-migrate bookkeeping', () => {
expect(mirroredTables()).not.toContain('pgmigrations');
});
// 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.
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`
);
// The mirror names a column either as a bare camelCase key or, when the
// database name differs, as an explicit string argument. Checking the
// snake_case name appears anywhere in the file is deliberately loose: it
// catches a column the mirror has never heard of, which is the failure that
// matters, without re-implementing drizzle-kit's naming rules.
const missing = rows
.filter((row) => !SCHEMA.includes(`"${row.column_name}"`))
.filter((row) => !SCHEMA.includes(snakeToCamel(row.column_name)))
.map((row) => `${row.table_name}.${row.column_name}`);
expect(missing).toEqual([]);
});
});
function snakeToCamel(name: string): string {
return name.replace(/_([a-z])/g, (_, c: string) => c.toUpperCase());
}
@@ -0,0 +1,125 @@
import { readFileSync } from 'fs';
import path from 'path';
import { pool } from '../../src/db';
import { closeDb } from './setup/testDb';
afterAll(async () => {
await pool.end();
await closeDb();
});
const SCHEMA = readFileSync(
path.join(__dirname, '..', '..', 'src', 'db-kysely', 'schema.ts'),
'utf8'
);
/**
* Every column the mirror declares, keyed by the database's own table name.
*
* The `DB` interface maps a table name to the interface that declares that
* table's columns, so the two have to be read together. A flat set of every
* column name appearing anywhere in the file is not the same assertion and is
* far weaker than it looks: seventeen column names are declared on more than
* one table and `created_at` is on fourteen of the eighteen, so a migration
* adding one of those to a table that lacks it would pass without the mirror
* knowing anything about it.
*
* Parsed as text rather than imported, because these are TypeScript types and
* are erased at run time — there is nothing to import and inspect.
*/
function mirroredColumns(): Map<string, Set<string>> {
const block = /export interface DB \{([^}]*)\}/.exec(SCHEMA)?.[1] ?? '';
const byTable = new Map<string, Set<string>>();
for (const [, table, iface] of block.matchAll(/^\s*([a-z0-9_]+):\s*([A-Za-z0-9_]+);/gm)) {
const declaration =
new RegExp(`export interface ${iface!} \\{([^}]*)\\}`).exec(SCHEMA)?.[1] ?? '';
byTable.set(
table!,
new Set([...declaration.matchAll(/^\s{2}([a-z0-9_]+):/gm)].map((m) => m[1]!))
);
}
return byTable;
}
/** Every table name the generated mirror declares. */
function mirroredTables(): string[] {
return [...mirroredColumns().keys()].sort();
}
async function liveTables(): Promise<string[]> {
const { rows } = await pool.query<{ table_name: string }>(
`SELECT table_name FROM information_schema.tables
WHERE table_schema = 'public' AND table_type = 'BASE TABLE'
AND table_name <> 'pgmigrations'
ORDER BY table_name`
);
return rows.map((r) => r.table_name);
}
/**
* The guard for #217.
*
* `src/db-kysely/schema.ts` is generated by `npm run db:types` and is a
* read-only mirror of the real schema, which `backend/migrations` owns. Nothing
* makes anyone regenerate after writing a migration, and that is not
* hypothetical: the mirror sat missing `item_drafts` and `upload_links` from
* the moment #222 landed until #217, because it had been copied into src/ by
* hand and nobody had reason to look at it.
*
* A stale mirror is worse than no mirror. Row types are inferred from it, so a
* converted query would type-check against a schema the database does not have
* and fail at run time with a column that does not exist — the exact class of
* drift the adoption was meant to close.
*
* The generator changed in #305 and this test did not, because the drift it
* guards is a property of generating a mirror at all rather than of any
* library.
*/
describe('the generated schema mirror', () => {
it('declares every table the migrations create', async () => {
const live = await liveTables();
const mirrored = mirroredTables();
const missing = live.filter((name) => !mirrored.includes(name));
expect(missing).toEqual([]);
});
it('declares no table the database does not have', async () => {
const live = await liveTables();
const mirrored = mirroredTables();
const extra = mirrored.filter((name) => !live.includes(name));
expect(extra).toEqual([]);
});
// 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.
it('excludes node-pg-migrate bookkeeping', () => {
expect(mirroredTables()).not.toContain('pgmigrations');
});
// 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 per table, where the Drizzle version needed two matches and
// no table at all, and called itself "deliberately loose" for it.
// kysely-codegen emits the database's own name as a bare interface key and
// the DB interface says which interface belongs to which table, so the pair
// 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 columns = mirroredColumns();
const missing = rows
.filter((row) => !columns.get(row.table_name)?.has(row.column_name))
.map((row) => `${row.table_name}.${row.column_name}`);
expect(missing).toEqual([]);
});
});
+148 -107
View File
@@ -1,4 +1,5 @@
import { parseItemFilters, FilterError, buildItemFilterSql } from '../../src/itemFilters'; import { parseItemFilters, FilterError, itemFilterExpressions, ItemFilters } from '../../src/itemFilters';
import { db } from '../../src/db';
describe('parseItemFilters', () => { describe('parseItemFilters', () => {
it('returns empty filters for an empty query', () => { it('returns empty filters for an empty query', () => {
@@ -170,138 +171,178 @@ describe('parseItemFilters', () => {
}); });
}); });
describe('buildItemFilterSql', () => { /**
it('produces no clauses and no params when nothing is filtered', () => { * Compiles the filter clauses on their own, with no projection around them.
const built = buildItemFilterSql(parseItemFilters({}), 1, null); *
expect(built.clauses).toEqual([]); * The expressions are what this file is about, and Kysely compiles without a
expect(built.params).toEqual([]); * connection — so these assert on the SQL and parameters actually emitted,
* rather than on the intermediate strings the old builder returned. That is a
* stronger claim than the one these tests used to make.
*/
function compileFilters(filters: ItemFilters, customerId: number | null = null) {
// The same `items as i` + `categories as c` shape both real queries use, so
// the expression builder handed to the callback is exactly the ItemContext
// the filters are written against. Building a narrower query here would need
// a cast, and a cast in the test would be testing the cast.
const { sql, parameters } = db
.selectFrom('items as i')
.leftJoin('categories as c', 'c.id', 'i.category_id')
.select('i.id')
.where((eb) => eb.and(itemFilterExpressions(eb, filters, customerId)))
.compile();
return { sql, parameters: [...parameters] };
}
const NO_FILTERS = {
categoryIds: [],
tagIds: [],
minPriceCents: null,
maxPriceCents: null,
status: null,
favoritesOnly: false
};
describe('itemFilterExpressions', () => {
it('adds no condition when nothing is filtered', () => {
const { sql, parameters } = compileFilters(NO_FILTERS);
// Not "no WHERE at all" as originally assumed: Kysely 0.28's `eb.and([])`
// compiles an empty conjunction to the truism `where 1 = 1` rather than
// omitting the clause (see parseFilterList in
// kysely/dist/cjs/parser/binary-operation-parser.js). That still matches
// every row, so it is the same "no filter" behaviour the old
// `clauses.length ? ... : ''` gave — the literal SQL text just differs
// from what was assumed here, which is what this assertion now checks.
expect(sql).toContain('where 1 = 1');
expect(parameters).toEqual([]);
}); });
it('matches a category and all of its descendants', () => { it('matches a category and all of its descendants', () => {
const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 1, null); const { sql, parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4] });
expect(built.clauses.join(' ')).toContain('RECURSIVE'); expect(sql).toContain('WITH RECURSIVE subtree');
// One array parameter rather than one id: the CTE is seeded with ANY so expect(parameters).toEqual([[4]]);
// several selected roots are walked in the same recursion.
expect(built.params).toEqual([[4]]);
}); });
it('seeds the descendant walk with every selected category', () => { // One bind parameter holding the whole array, not a placeholder list. This is
const built = buildItemFilterSql(parseItemFilters({ category: '4,9' }), 1, null); // the property that made the array trap in the previous builder impossible
const sql = built.clauses.join(' '); // here — see #297 and src/db-kysely/CONVENTIONS.md.
expect(sql).toContain('RECURSIVE'); it('seeds the descendant walk with every selected category, as one parameter', () => {
// ANY over the seeds is what makes several categories combine as OR: the const { parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4, 9] });
// result is the union of their subtrees. expect(parameters).toEqual([[4, 9]]);
expect(sql).toContain('= ANY($1::int[])');
expect(built.params).toEqual([[4, 9]]);
}); });
it('requires every listed tag rather than any of them', () => { it('requires every listed tag rather than any of them', () => {
const built = buildItemFilterSql(parseItemFilters({ tags: '1,2' }), 1, null); const { sql, parameters } = compileFilters({ ...NO_FILTERS, tagIds: [2, 5] });
// The count of matched tag rows must equal the number of tags requested — expect(sql).toContain('SELECT COUNT(*) FROM item_tags');
// an ANY/IN match alone would return items carrying just one of them. expect(parameters).toEqual([[2, 5], 2]);
expect(built.clauses.join(' ')).toContain('COUNT(*)');
expect(built.params).toEqual([[1, 2], 2]);
}); });
it('numbers placeholders from the given starting index', () => { it('filters on a price range', () => {
const built = buildItemFilterSql(parseItemFilters({ category: '4' }), 3, null); const { parameters } = compileFilters({
expect(built.clauses.join(' ')).toContain('$3'); ...NO_FILTERS,
minPriceCents: 1000,
maxPriceCents: 5000
});
expect(parameters).toEqual([1000, 5000]);
}); });
it('filters on status', () => { it('filters on several statuses with one expression', () => {
const built = buildItemFilterSql(parseItemFilters({ status: 'reserved' }), 1, null); const { sql, parameters } = compileFilters({
expect(built.clauses.join(' ')).toContain('i.status = ANY'); ...NO_FILTERS,
expect(built.params).toEqual([['reserved']]); status: ['available', 'reserved']
}); });
expect(sql).toContain('"i"."status" in');
// One clause for one status and for several, which is the whole reason the expect(parameters).toEqual(['available', 'reserved']);
// filter was generalised rather than joined by a second dimension.
it('filters on several statuses with the same single clause', () => {
const built = buildItemFilterSql(parseItemFilters({ status: 'available,reserved' }), 1, null);
expect(built.clauses).toHaveLength(1);
expect(built.clauses[0]).toContain('i.status = ANY');
expect(built.params).toEqual([['available', 'reserved']]);
}); });
it('restricts to the favorites of the given customer', () => { it('restricts to the favorites of the given customer', () => {
const built = buildItemFilterSql(parseItemFilters({ favorites: '1' }), 1, 42); const { sql, parameters } = compileFilters({ ...NO_FILTERS, favoritesOnly: true }, 7);
expect(built.clauses.join(' ')).toContain('EXISTS'); expect(sql).toContain('exists');
expect(built.clauses.join(' ')).toContain('favorites f'); expect(parameters).toEqual([7]);
expect(built.params).toEqual([42]);
}); });
it('does not restrict to favorites when the flag is off, even given a customer', () => { it('does not restrict to favorites when the flag is off, even given a customer', () => {
const built = buildItemFilterSql(parseItemFilters({}), 1, 42); const { sql, parameters } = compileFilters(NO_FILTERS, 7);
expect(built.clauses).toEqual([]); expect(sql).not.toContain('exists');
expect(built.params).toEqual([]); expect(parameters).toEqual([]);
}); });
// Both routes reject this before reaching the builder, so it can only happen
// through a new caller that forgot to. Failing loudly beats dropping the
// clause and returning the whole catalogue as if it were someone's favorites.
it('throws rather than ignore a favorites filter with no customer', () => { it('throws rather than ignore a favorites filter with no customer', () => {
expect(() => buildItemFilterSql(parseItemFilters({ favorites: '1' }), 1, null)).toThrow(); expect(() => compileFilters({ ...NO_FILTERS, favoritesOnly: true }, null)).toThrow(
}); /favorites filter requires a customer id/
it('continues numbering across multiple filters', () => {
const built = buildItemFilterSql(
parseItemFilters({ category: '4', min_price: '100', max_price: '900' }),
1,
null
); );
expect(built.params).toEqual([[4], 100, 900]);
const sql = built.clauses.join(' ');
expect(sql).toContain('$1');
expect(sql).toContain('$2');
expect(sql).toContain('$3');
});
});
// Both callers splice these clauses straight into query text, so a value
// reaching the clause string is SQL injection rather than a style problem. The
// comment on buildItemFilterSql says so; these two make it fail a build instead
// of relying on someone reading it. See #202, and #180 for the S2077 review.
describe('buildItemFilterSql keeps every value out of the SQL text', () => {
// Deliberately built by hand rather than through parseItemFilters, because
// the claim is that the clause literals are safe with no parser at all. These
// values could never survive parsing, which is the point: the parser is
// defence in depth, not the reason this holds.
const HOSTILE = "1); DROP TABLE items; --";
const hostileFilters = {
categoryIds: [HOSTILE],
tagIds: [HOSTILE],
minPriceCents: HOSTILE,
maxPriceCents: HOSTILE,
status: [HOSTILE],
favoritesOnly: true
} as unknown as Parameters<typeof buildItemFilterSql>[0];
it('never lets a filter value reach a clause, even one the parser would reject', () => {
const built = buildItemFilterSql(hostileFilters, 1, HOSTILE as unknown as number);
const sql = built.clauses.join(' AND ');
expect(sql).not.toContain(HOSTILE);
expect(sql).not.toContain('DROP TABLE');
// Every value still arrives, bound, where it can do nothing.
expect(built.params).toContain(HOSTILE);
}); });
// The structural version of the same claim, and the one that catches a value it('composes several filters together', () => {
// which happens not to look hostile: the SQL text must not depend on the const { parameters } = compileFilters(
// values at all. Two disjoint sets of inputs, byte-identical clauses. {
it('produces byte-identical SQL for two completely different filter sets', () => { categoryIds: [4],
const a = buildItemFilterSql( tagIds: [2],
parseItemFilters({ category: '4', tags: '7,8', min_price: '100', max_price: '900', status: 'sold' }), minPriceCents: 1000,
1, maxPriceCents: null,
42 status: ['available'],
); favoritesOnly: true
const b = buildItemFilterSql( },
parseItemFilters({ category: '99', tags: '11,12', min_price: '5', max_price: '6', status: 'available' }),
1,
7 7
); );
expect(parameters).toEqual([[4], [2], 1, 1000, 'available', 7]);
});
});
expect(a.clauses).toEqual(b.clauses); // The invariant, and it is load-bearing: the storefront call site is reachable
expect(a.params).not.toEqual(b.params); // without signing in, so a filter value reaching the SQL text is SQL injection
// rather than a style problem. These two made that fail a build rather than
// relying on someone reading a comment, and they still do — but they now check
// the SQL Kysely actually emits rather than the strings the old builder
// returned. See #202, #180 for the S2077 review, and #308 for the conversion.
describe('itemFilterExpressions keeps every value out of the SQL text', () => {
// Built by hand rather than through parseItemFilters, because the claim is
// that the expressions are safe with no parser at all. These values could
// never survive parsing, which is the point: the parser is defence in depth,
// not the reason this holds.
const HOSTILE = "1); DROP TABLE items; --";
it('never lets a filter value reach the SQL, even one the parser would reject', () => {
const { sql, parameters } = compileFilters(
{
categoryIds: [HOSTILE],
tagIds: [HOSTILE],
minPriceCents: HOSTILE,
maxPriceCents: HOSTILE,
status: [HOSTILE],
favoritesOnly: true
} as unknown as ItemFilters,
HOSTILE as unknown as number
);
expect(sql).not.toContain('DROP TABLE');
expect(JSON.stringify(parameters)).toContain('DROP TABLE');
});
it('produces byte-identical SQL for two completely different filter sets', () => {
const first = compileFilters(
{
categoryIds: [1],
tagIds: [2],
minPriceCents: 3,
maxPriceCents: 4,
status: ['available'],
favoritesOnly: true
},
5
);
const second = compileFilters(
{
categoryIds: [99],
tagIds: [98],
minPriceCents: 97,
maxPriceCents: 96,
status: ['sold'],
favoritesOnly: true
},
95
);
expect(first.sql).toBe(second.sql);
expect(first.parameters).not.toEqual(second.parameters);
}); });
}); });
@@ -0,0 +1,649 @@
# Dynamic Queries to Kysely 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:** Build the item list and by-id queries through Kysely so no value or clause is ever interpolated into query text, retiring the last two S2077 hotspots.
**Architecture:** `itemSelect.ts` stops exporting SQL strings and exports two query builders instead, with the image and tag aggregates built by `jsonArrayFrom`. `itemFilters.ts` stops returning `{ clauses, params }` and returns an array of Kysely expressions. The four call sites compose the two.
**Tech Stack:** Express 4 + TypeScript, Kysely 0.28 (`jsonArrayFrom` from `kysely/helpers/postgres`), `pg`, Jest + supertest.
**Spec:** `docs/superpowers/specs/2026-09-04-dynamic-queries-kysely-design.md`
## Global Constraints
- **No value and no clause may reach query text.** Every filter value is a bind parameter. This is the entire point of the change.
- **The API contract does not change.** Same columns, same JSON keys, same ordering, same statuses. If a response changes, the conversion is wrong.
- **`AdminItemRow` and `PublicItemRow` stay exported and stay hand-written.** The queries are assigned to them so a drift becomes a compile error. Do not replace them with inferred types.
- **`ADMIN_IMAGES_SUBQUERY`'s distinction survives:** `original_image_path` appears in the admin projection and never in the public one (#293).
- **`parseItemFilters` is untouched**, and so is every test over it.
- **This is one task.** `itemSelect.ts`, `itemFilters.ts`, both routes and the unit tests are coupled — the exports the routes use are the ones being replaced, and the test file is type-checked by `tsconfig.test.json`, so any partial commit is a red build.
- All SQL parameterized; every Express handler stays wrapped in `asyncRoute`.
- Commit subject ends with `(#308)`. **Commit bodies are never hard-wrapped** — one long line per paragraph. End with `Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>`.
- **Do not push.**
- **Do not run `scripts/start-local.ps1` or `scripts/run-tests.ps1`** — they prompt for UAC and hang.
- **Node 20 is required.** Prepend `export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH"` to every command; the machine default is 18.x and Jest fails on it.
---
## File Structure
| File | Change |
|---|---|
| `backend/src/itemSelect.ts` | The four SQL string constants become `adminItemQuery()` and `publicItemQuery()`. Row types kept. |
| `backend/src/itemFilters.ts` | `buildItemFilterSql``itemFilterExpressions`. `BuiltFilter` deleted. Parser untouched. |
| `backend/src/routes/admin.ts` | Three call sites: the list, and two by-id reads. |
| `backend/src/routes/items.ts` | Two call sites: the list, and the by-id read. |
| `backend/tests/unit/itemFilters.test.ts` | The two `buildItemFilterSql` describe blocks rewritten against compiled SQL. The `parseItemFilters` block untouched. |
**Everything below was verified by probe against the real generated schema before this plan was written** — it compiles, and the SQL it emits is quoted in the steps. It is not a sketch.
---
## Task 1: The conversion
**Files:**
- Modify: `backend/src/itemSelect.ts`, `backend/src/itemFilters.ts`, `backend/src/routes/admin.ts`, `backend/src/routes/items.ts`, `backend/tests/unit/itemFilters.test.ts`
**Interfaces:**
- Consumes: `db` from `../db`, `DB` from `../db-kysely/schema`, both from #305.
- Produces: `adminItemQuery()`, `publicItemQuery()`, `ItemContext`, `AdminItemRow`, `PublicItemRow` from `itemSelect.ts`; `itemFilterExpressions(eb, filters, favoritesCustomerId)` from `itemFilters.ts`.
- [ ] **Step 1: Rewrite `itemSelect.ts`**
Replace everything from the top of the file down to and including `export const ADMIN_ITEM_BY_ID = ...` with the following. **Keep every interface below that line exactly as it is**`ItemRowBase`, `AdminItemRow`, `PublicItemRow` and anything else — they are the contract this change is checked against.
```ts
// Shared item query shapes for the public and admin routes, and the row types
// they return.
//
// The types live here rather than in types.ts because they describe a
// projection, not a table. adminItemQuery takes every column of items and
// publicItemQuery names its columns, so the storefront never sees
// paypal_order_id or reserved_until — typing both as "an items row" would
// quietly re-admit exactly the columns that projection was written to exclude.
//
// These were SQL string constants until #308. They had to be kept in step with
// their row types by hand, because `pool.query<T>` asserts a shape and never
// checks it against the SQL, so dropping a column from a select without
// dropping it from its type compiled cleanly and went undefined at run time —
// and only the integration suite ever caught it. Built through Kysely, that is
// a compile error, because the row type now follows from the projection.
//
// Images and tags are pulled as aggregate subqueries rather than LEFT JOIN +
// GROUP BY. Joining two one-to-many relations in the same query multiplies
// their rows together — an item with 2 images and 3 tags would aggregate 6
// rows, silently repeating every image three times. Subqueries keep each
// aggregate independent and drop the GROUP BY entirely. `jsonArrayFrom` emits
// `coalesce(json_agg(agg), '[]')`, which is what these hand-wrote before.
import { ExpressionBuilder } from 'kysely';
import { jsonArrayFrom } from 'kysely/helpers/postgres';
import { db } from './db';
import { DB } from './db-kysely/schema';
import { ItemStatus, ItemImage, ItemTag } from './types';
/**
* The aliases every item query and every filter clause is written against.
*
* `i` and `c` are kept from the SQL these replaced. Not because short names are
* better, but because the filter clauses, the subquery correlations and the
* ORDER BY all reference them, and renaming them in the same change that moved
* the builder would have made the diff unreadable against the SQL it replaces.
*/
export type ItemContext = ExpressionBuilder<
DB & { i: DB['items']; c: DB['categories'] },
'i' | 'c'
>;
/** The public image fields. Correlated to the outer item by `whereRef`. */
function imagesFor(eb: ItemContext) {
return jsonArrayFrom(
eb
.selectFrom('item_images as img')
.select(['img.id', 'img.image_path', 'img.sort_order'])
.whereRef('img.item_id', '=', 'i.id')
.orderBy('img.sort_order')
).as('images');
}
/**
* Admin-only images, carrying `original_image_path` alongside the public
* fields — the field the inventory screen needs to know whether a photo has a
* cut-out to restore (#293).
*
* A separate function rather than a flag on `imagesFor`, for the same reason
* `publicItemQuery` names its columns instead of taking them all: an original
* filename is internal, nobody's business on the storefront, and a boolean in
* the middle of the thing that keeps it off the public API is one edit away
* from being passed wrongly.
*/
function adminImagesFor(eb: ItemContext) {
return jsonArrayFrom(
eb
.selectFrom('item_images as img')
.select(['img.id', 'img.image_path', 'img.sort_order', 'img.original_image_path'])
.whereRef('img.item_id', '=', 'i.id')
.orderBy('img.sort_order')
).as('images');
}
function tagsFor(eb: ItemContext) {
return jsonArrayFrom(
eb
.selectFrom('item_tags as it')
.innerJoin('tags as t', 't.id', 'it.tag_id')
.select(['t.id', 't.name', 't.color'])
.whereRef('it.item_id', '=', 'i.id')
.orderBy('t.name')
).as('tags');
}
/**
* The storefront's projection — an explicit column list, because it has no
* business seeing paypal_order_id or reserved_until.
*
* A function rather than a constant so each caller gets a fresh builder. Kysely
* builders are immutable, so sharing one would be safe, but a function makes it
* obvious that adding a `where` does not affect anyone else.
*/
export function publicItemQuery() {
return db
.selectFrom('items as i')
.leftJoin('categories as c', 'c.id', 'i.category_id')
.select([
'i.id',
'i.name',
'i.description',
'i.price_cents',
'i.status',
'i.created_at',
'i.category_id',
'c.name as category_name'
])
.select(imagesFor)
.select(tagsFor);
}
/** The admin projection — every item column, plus the admin image fields. */
export function adminItemQuery() {
return db
.selectFrom('items as i')
.leftJoin('categories as c', 'c.id', 'i.category_id')
.selectAll('i')
.select('c.name as category_name')
.select(adminImagesFor)
.select(tagsFor);
}
```
If `ItemStatus`, `ItemImage` or `ItemTag` end up unused by the file after this, leave the import of whichever the interfaces below still use and drop only the genuinely unused ones — `npm run lint` will say which.
- [ ] **Step 2: Verify the projections compile and match**
```bash
cd backend
export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH"
npm run build 2>&1 | grep -E "itemSelect|error TS" | head
```
Expected at this point: errors only in `admin.ts`, `items.ts` and `itemFilters.ts`, which still reference the deleted constants. **Errors inside `itemSelect.ts` itself mean the projection does not type-check against the generated schema — stop and report which column.**
- [ ] **Step 3: Convert the filter builder**
In `backend/src/itemFilters.ts`: delete the `BuiltFilter` interface, and replace the whole of `buildItemFilterSql` — its long comment block included — with the following. **Every clause keeps its own comment**; those record decisions, not descriptions.
```ts
// Composes the filter clauses as Kysely expressions.
//
// This returned `{ clauses: string[]; params: unknown[] }` until #308, and both
// callers spliced the clauses straight into query text. The invariant that made
// that safe — only a placeholder index may ever be interpolated into a clause,
// never a value — was a sixteen-line comment and two tests standing between an
// edit and a live injection on a route reachable without signing in.
//
// It is now a property of the type system. `${value}` inside a Kysely `sql`
// template emits a bind parameter, never text, and the builder expressions
// cannot express interpolation at all. The two tests at the bottom of
// itemFilters.test.ts still exist and now assert against the SQL Kysely
// actually emits, which is a stronger claim than the one they used to make.
//
// `startIndex` is gone with the splicing it existed for.
//
// `favoritesCustomerId` is required rather than optional so a caller has to say
// whose favorites it means, even when it means nobody's. Both routes already
// reject a favorites filter they cannot satisfy, so reaching the throw below is
// a programming error — but it is here so that a future caller which forgets
// the guard fails loudly instead of quietly ignoring the filter and listing the
// whole catalogue.
export function itemFilterExpressions(
eb: ItemContext,
filters: ItemFilters,
favoritesCustomerId: number | null
): Expression<SqlBool>[] {
const clauses: Expression<SqlBool>[] = [];
if (filters.categoryIds.length) {
// Selecting a category means "and everything filed beneath it", so walk the
// tree down from each chosen node. A recursive CTE keeps the tree
// un-denormalized: reparenting stays a single UPDATE with no stored paths
// to rewrite.
//
// Seeded with `= ANY(...)` rather than one id, so every selected root is
// walked in the same recursion. That also gives the OR for free: the union
// of the subtrees is exactly "filed under any of these", and an item filed
// under two selected branches appears once because IN is a set test.
//
// Still a `sql` template, because the builder expresses a recursive CTE no
// better than this does. `${filters.categoryIds}` is one bind parameter
// holding the whole array — not a placeholder list — which is why no
// sql.param() ceremony appears here. See src/db-kysely/CONVENTIONS.md.
clauses.push(sql<SqlBool>`i.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) {
// AND, not OR: the item must carry every selected tag. Matching with
// `tag_id = ANY(...)` alone would return items holding just one of them, so
// the count of matched rows has to equal the number requested.
clauses.push(sql<SqlBool>`(SELECT COUNT(*) FROM item_tags it
WHERE it.item_id = i.id AND it.tag_id = ANY(${filters.tagIds}::int[])) = ${filters.tagIds.length}`);
}
if (filters.minPriceCents !== null) {
clauses.push(eb('i.price_cents', '>=', filters.minPriceCents));
}
if (filters.maxPriceCents !== null) {
clauses.push(eb('i.price_cents', '<=', filters.maxPriceCents));
}
if (filters.status !== null) {
// `in` replaces the `= ANY($n::text[])` this used to build. Kysely emits
// the placeholder list itself, so one status and several use the same
// expression and the explicit ::text[] cast is no longer needed.
clauses.push(eb('i.status', 'in', filters.status));
}
if (filters.favoritesOnly) {
if (favoritesCustomerId === null) {
throw new Error('favorites filter requires a customer id');
}
// EXISTS rather than a join: an item is favorited by a customer at most
// once, but joining would still risk multiplying rows if that ever changed,
// and this reads as the membership test it is.
clauses.push(
eb.exists(
eb
.selectFrom('favorites as f')
.select('f.item_id')
.whereRef('f.item_id', '=', 'i.id')
.where('f.customer_id', '=', favoritesCustomerId)
)
);
}
return clauses;
}
```
Add to that file's imports:
```ts
import { Expression, SqlBool, sql } from 'kysely';
import { ItemContext } from './itemSelect';
```
- [ ] **Step 4: Convert the admin route**
In `backend/src/routes/admin.ts`, change the import on line 4 from `ADMIN_ITEM_SELECT, ADMIN_ITEM_BY_ID` to `adminItemQuery`, keeping `AdminItemRow` and `ItemRecord`, and change `buildItemFilterSql` to `itemFilterExpressions` in the `itemFilters` import.
Replace the whole S2077 comment block and the three lines after it (the `buildItemFilterSql` call, the `where` assembly, and the `pool.query`) with:
```ts
// No interpolation, and nothing to argue about. Until #308 this assembled
// `${ADMIN_ITEM_SELECT} ${where}` from clauses composed at run time, and
// sixteen lines in itemFilters.ts explained why that was safe. The clauses
// are Kysely expressions now: a value cannot reach the SQL text, because the
// types do not let it.
const rows: AdminItemRow[] = await adminItemQuery()
.where((eb) => eb.and(itemFilterExpressions(eb, filters, null)))
.orderBy('i.created_at', 'desc')
.execute();
res.json(rows);
```
Then replace both by-id reads. At the two places currently reading `pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [item.id])` and `pool.query<AdminItemRow>(ADMIN_ITEM_BY_ID, [itemId])`, the surrounding code destructures `{ rows: full }` and uses `full[0]`. Replace each with a single-row read, keeping the surrounding logic:
```ts
const full = await adminItemQuery().where('i.id', '=', item.id).execute();
```
and
```ts
const full = await adminItemQuery().where('i.id', '=', itemId).execute();
```
`full` is now the array directly rather than `{ rows }`, so remove the destructuring at both sites and leave every use of `full[0]` as it is.
- [ ] **Step 5: Convert the storefront route**
In `backend/src/routes/items.ts`, change the import to `publicItemQuery` (keeping `PublicItemRow`) and `buildItemFilterSql` to `itemFilterExpressions`.
`EXCLUDE_PENDING` and `PUBLIC_ITEM_BY_ID` both go. Replace them with:
```ts
/**
* Pending items are excluded everywhere, not only from the list. A pending item
* that stayed fetchable by id would be hidden from the catalogue and still
* reachable by anyone who guessed or kept a link.
*
* An expression rather than the SQL literal this was until #308, so it composes
* with the filter clauses through `eb.and` instead of being joined into a
* string. That join used to need its own argument about why AND could not
* weaken it; `and` cannot re-associate anything.
*/
function notPending(eb: ItemContext) {
return eb('i.status', '!=', 'pending');
}
```
with `ItemContext` added to the `itemSelect` import.
Replace the `buildItemFilterSql` call, the S2077 comment, the `where` assembly and the `pool.query` with:
```ts
const rows: PublicItemRow[] = await publicItemQuery()
.where((eb) =>
eb.and([
notPending(eb),
...itemFilterExpressions(eb, effectiveFilters, req.customerId ?? null)
])
)
.orderBy('i.created_at', 'desc')
.execute();
res.json(rows);
```
And replace the by-id read:
```ts
const rows = await publicItemQuery()
.where('i.id', '=', Number(req.params.id))
.where((eb) => notPending(eb))
.execute();
if (!rows.length) return res.status(404).json({ error: 'not found' });
res.json(rows[0]);
```
`Number(req.params.id)` rather than the raw string, because the column is an integer and Kysely types it that way. A non-numeric id becomes `NaN`, which matches no row and yields the same 404 the old query gave — verify that in Step 7.
- [ ] **Step 6: Rewrite the two filter test blocks**
In `backend/tests/unit/itemFilters.test.ts`, leave the entire `describe('parseItemFilters', ...)` block untouched. Replace the `describe('buildItemFilterSql', ...)` block and the `describe('buildItemFilterSql keeps every value out of the SQL text', ...)` block with the following, and add these imports at the top:
```ts
import { db } from '../../src/db';
import { itemFilterExpressions, ItemFilters } from '../../src/itemFilters';
```
`ItemFilters` is already exported from `itemFilters.ts`; check the name against the file and use whatever it actually exports for the parsed-filters shape.
```ts
/**
* Compiles the filter clauses on their own, with no projection around them.
*
* The expressions are what this file is about, and Kysely compiles without a
* connection — so these assert on the SQL and parameters actually emitted,
* rather than on the intermediate strings the old builder returned. That is a
* stronger claim than the one these tests used to make.
*/
function compileFilters(filters: ItemFilters, customerId: number | null = null) {
// The same `items as i` + `categories as c` shape both real queries use, so
// the expression builder handed to the callback is exactly the ItemContext
// the filters are written against. Building a narrower query here would need
// a cast, and a cast in the test would be testing the cast.
const { sql, parameters } = db
.selectFrom('items as i')
.leftJoin('categories as c', 'c.id', 'i.category_id')
.select('i.id')
.where((eb) => eb.and(itemFilterExpressions(eb, filters, customerId)))
.compile();
return { sql, parameters: [...parameters] };
}
const NO_FILTERS = {
categoryIds: [],
tagIds: [],
minPriceCents: null,
maxPriceCents: null,
status: null,
favoritesOnly: false
};
describe('itemFilterExpressions', () => {
it('adds no condition when nothing is filtered', () => {
const { sql, parameters } = compileFilters(NO_FILTERS);
// `select ... from` with no `where` at all — Kysely emits nothing for an
// empty `and`, matching the old `clauses.length ? ... : ''`.
expect(sql).not.toContain('where');
expect(parameters).toEqual([]);
});
it('matches a category and all of its descendants', () => {
const { sql, parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4] });
expect(sql).toContain('WITH RECURSIVE subtree');
expect(parameters).toEqual([[4]]);
});
// One bind parameter holding the whole array, not a placeholder list. This is
// the property that made the array trap in the previous builder impossible
// here — see #297 and src/db-kysely/CONVENTIONS.md.
it('seeds the descendant walk with every selected category, as one parameter', () => {
const { parameters } = compileFilters({ ...NO_FILTERS, categoryIds: [4, 9] });
expect(parameters).toEqual([[4, 9]]);
});
it('requires every listed tag rather than any of them', () => {
const { sql, parameters } = compileFilters({ ...NO_FILTERS, tagIds: [2, 5] });
expect(sql).toContain('SELECT COUNT(*) FROM item_tags');
expect(parameters).toEqual([[2, 5], 2]);
});
it('filters on a price range', () => {
const { parameters } = compileFilters({
...NO_FILTERS,
minPriceCents: 1000,
maxPriceCents: 5000
});
expect(parameters).toEqual([1000, 5000]);
});
it('filters on several statuses with one expression', () => {
const { sql, parameters } = compileFilters({
...NO_FILTERS,
status: ['available', 'reserved']
});
expect(sql).toContain('"i"."status" in');
expect(parameters).toEqual(['available', 'reserved']);
});
it('restricts to the favorites of the given customer', () => {
const { sql, parameters } = compileFilters({ ...NO_FILTERS, favoritesOnly: true }, 7);
expect(sql).toContain('exists');
expect(parameters).toEqual([7]);
});
it('does not restrict to favorites when the flag is off, even given a customer', () => {
const { sql, parameters } = compileFilters(NO_FILTERS, 7);
expect(sql).not.toContain('exists');
expect(parameters).toEqual([]);
});
it('throws rather than ignore a favorites filter with no customer', () => {
expect(() => compileFilters({ ...NO_FILTERS, favoritesOnly: true }, null)).toThrow(
/favorites filter requires a customer id/
);
});
it('composes several filters together', () => {
const { parameters } = compileFilters(
{
categoryIds: [4],
tagIds: [2],
minPriceCents: 1000,
maxPriceCents: null,
status: ['available'],
favoritesOnly: true
},
7
);
expect(parameters).toEqual([[4], [2], 1, 1000, 'available', 7]);
});
});
// The invariant, and it is load-bearing: the storefront call site is reachable
// without signing in, so a filter value reaching the SQL text is SQL injection
// rather than a style problem. These two made that fail a build rather than
// relying on someone reading a comment, and they still do — but they now check
// the SQL Kysely actually emits rather than the strings the old builder
// returned. See #202, #180 for the S2077 review, and #308 for the conversion.
describe('itemFilterExpressions keeps every value out of the SQL text', () => {
// Built by hand rather than through parseItemFilters, because the claim is
// that the expressions are safe with no parser at all. These values could
// never survive parsing, which is the point: the parser is defence in depth,
// not the reason this holds.
const HOSTILE = "1); DROP TABLE items; --";
it('never lets a filter value reach the SQL, even one the parser would reject', () => {
const { sql, parameters } = compileFilters(
{
categoryIds: [HOSTILE],
tagIds: [HOSTILE],
minPriceCents: HOSTILE,
maxPriceCents: HOSTILE,
status: [HOSTILE],
favoritesOnly: true
} as unknown as ItemFilters,
HOSTILE as unknown as number
);
expect(sql).not.toContain('DROP TABLE');
expect(JSON.stringify(parameters)).toContain('DROP TABLE');
});
it('produces byte-identical SQL for two completely different filter sets', () => {
const first = compileFilters(
{
categoryIds: [1],
tagIds: [2],
minPriceCents: 3,
maxPriceCents: 4,
status: ['available'],
favoritesOnly: true
},
5
);
const second = compileFilters(
{
categoryIds: [99],
tagIds: [98],
minPriceCents: 97,
maxPriceCents: 96,
status: ['sold'],
favoritesOnly: true
},
95
);
expect(first.sql).toBe(second.sql);
expect(first.parameters).not.toEqual(second.parameters);
});
});
```
- [ ] **Step 7: Run everything**
```bash
cd backend
export PATH="/c/Users/tlamb/AppData/Local/nvm/v20.20.2:$PATH"
npm run build
npm run lint
npx jest -c jest.unit.config.js
npm run db:test:up
npx jest -c jest.integration.config.js --runInBand
```
Expected: build clean, lint 0 errors, unit passing, **integration 445/445 with no test file edited**. The integration suite is the contract: every filter test, the sold-filter suite, the favorites suite and the pending-status suite must pass exactly as written. **If any integration test needs editing to pass, the conversion changed behaviour — fix the query, not the test, and report what differed.**
Pay particular attention to `pendingStatus.integration.test.ts` and any test fetching an item by a non-numeric id, since Step 5 changed that path from a string comparison to `Number(...)`.
- [ ] **Step 8: Confirm the interpolation is actually gone**
```bash
cd backend
grep -n "ITEM_SELECT\|ITEM_BY_ID\|buildItemFilterSql\|BuiltFilter" src/ tests/ -r
```
Expected: no output. Every one of those names is deleted by this task; a survivor means a call site was missed.
- [ ] **Step 9: Commit**
```bash
git add -A backend
git commit -F- <<'EOF'
refactor(db): build the item queries through Kysely (#308)
The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in.
All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one.
The second thing this buys may matter more than the first. pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error.
The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it.
The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one.
Closes #308
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
EOF
```
---
## Self-Review
**Spec coverage:**
| Spec requirement | Step |
|---|---|
| All four call sites convert | 4, 5 |
| Filter builder returns expressions, `startIndex` gone | 3 |
| Aggregates become `jsonArrayFrom` | 1 |
| Row types kept, hand-written, assigned | 1, 4, 5 |
| `i` / `c` aliases kept | 1 |
| Admin images stay separate from public | 1 |
| The six clauses keep their comments | 3 |
| `favoritesCustomerId` keeps its throw | 3 |
| No-filters case emits no `where` | 6 (first test) |
| Existing integration tests pass unedited | 7 |
| The two invariant tests re-pointed, not deleted | 6 |
| `parseItemFilters` untouched | 6 (explicitly) |
No gaps.
**Placeholder scan:** none. Every code step carries literal code; every command step carries the command and its expected output.
**Type consistency:** `ItemContext` is defined once in `itemSelect.ts` (Step 1) and imported by `itemFilters.ts` (Step 3), `items.ts` (Step 5) and the test (Step 6). `itemFilterExpressions(eb, filters, favoritesCustomerId)` has that argument order at its definition and at all three call sites. `adminItemQuery()` and `publicItemQuery()` are functions, called with `()` everywhere. `AdminItemRow` and `PublicItemRow` keep their existing names and are the annotation on the two list results.
**One thing the implementer must not paper over:** Step 1's row types and Step 4/5's `AdminItemRow[]` / `PublicItemRow[]` annotations are the whole point of the change. If the assignment fails to type-check, that is information — the projection and the contract disagree. The spec permits `$castTo` as a fallback *only* where the types genuinely differ (a `json_agg` timestamp arriving as a string is the expected case), and requires saying which column disagreed. Silently widening a row type to `any` would remove the only thing this change adds over the old code.
@@ -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.
@@ -0,0 +1,91 @@
# Converting the two dynamic queries to Kysely
**Issue:** #308. The work #305 made possible and deliberately did not do.
#294 removed interpolation from seven query sites by making each fixed-shape query a named constant. Two were left, and they are the ones the whole exercise was ever about:
- `backend/src/routes/admin.ts:149` — `` `${ADMIN_ITEM_SELECT} ${where} ORDER BY i.created_at DESC` ``
- `backend/src/routes/items.ts:95` — `` `${PUBLIC_ITEM_SELECT} WHERE ${where} ORDER BY i.created_at DESC` ``
`where` is genuinely composed at run time by `buildItemFilterSql`. Both are safe today, and `itemFilters.ts:242-258` spells out why: the clause fragments are string literals, the only things interpolated into them are placeholder *indices*, and every value goes onto `params`. That argument is correct. It is also an argument — a thing a reader must follow and an edit can quietly break, guarded by a comment and two tests. This turns it into a property of the type system, which is what #202 asked for and why #180's hotspots have sat Reviewed rather than closed.
## Decisions, and what each one rests on
**All four call sites convert, not just the two flagged ones.** `ADMIN_ITEM_BY_ID` and `PUBLIC_ITEM_BY_ID` carry no S2077 and are already safe named constants. But they are built by interpolating the same projection strings the list queries use, so converting only the list queries would leave `itemSelect.ts` holding a Kysely builder *and* a raw string that must produce the identical projection. That is a worse version of the hazard the file's own header warns about — "KEPT IN STEP BY HAND … Change a select and its type together" — because now there would be two spellings to keep in step instead of one. Four sites total: `admin.ts:149`, `admin.ts:178`, `admin.ts:232`, `items.ts:95`, `items.ts:105`.
**The filter builder returns Kysely expressions, not text.** `buildItemFilterSql(filters, startIndex, favoritesCustomerId): { clauses: string[]; params: unknown[] }` becomes a function returning an array of `Expression<SqlBool>`, which the caller hands to `eb.and(...)`. An array rather than "take a query builder and return it filtered", because the two callers do different things with the result: the storefront prepends its own `status <> 'pending'` and the admin route does not. A function that owned the query builder would have to be told about that difference; a function that returns expressions does not care.
`startIndex` disappears. It existed only so a caller could splice fragments in after its own parameters, and nothing splices any more.
**The aggregate subqueries become `jsonArrayFrom`.** Kysely's Postgres helper emits `(select coalesce(json_agg(agg), '[]') from … as agg)` — the same shape `IMAGES_SUBQUERY` and `TAGS_SUBQUERY` hand-write today, including the `'[]'` fallback. Using the helper rather than keeping the SQL as a literal fragment is what makes the result *typed*: the subquery's columns are known, so the row type follows from the select instead of being asserted alongside it.
That is the second thing this change buys, and it may matter more than the first. `pool.query<T>` asserts a shape TypeScript never checks against the SQL — the file's header says so plainly, and says the integration suite is the only thing that catches a drop. After this, dropping a column from a select and not its type is a compile error.
**The row-type contract is kept, not inferred away.** `AdminItemRow` and `PublicItemRow` stay exported and stay hand-written, and the queries are assigned to them. Inferring them from the query instead would be tidier and is deliberately not done: they are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. Assignment gives the compile error; inference would remove the thing being checked.
If exact inference proves fussy — `json_agg` of a timestamp comes back as a string, not a `Date`, and the aggregate helpers' types are precise about it — the fallback is an explicit `$castTo<AdminItemRow>()` at the end of the builder, which is still a single named assertion in one place rather than one per call site. Prefer plain assignment; reach for `$castTo` only where the types genuinely disagree, and say which column disagreed.
**`i` and `c` stay as aliases.** `selectFrom('items as i').leftJoin('categories as c', 'c.id', 'i.category_id')`. Not because the short names are better, but because every filter clause, every subquery correlation and the `ORDER BY` already reference them, and renaming them in the same change that moves the builder would make the diff impossible to read against the SQL it replaces.
**`ADMIN_IMAGES_SUBQUERY` stays separate from the public one.** It carries `original_image_path` and the storefront must never see it (#293). The two selects call different helpers; folding them into one parameterised helper would put a boolean in the middle of the thing that keeps an internal filename off the public API.
## Architecture
```
itemSelect.ts adminItemQuery() → SelectQueryBuilder, admin projection
publicItemQuery() → SelectQueryBuilder, public projection
│ (both: items i ⟕ categories c,
│ images + tags via jsonArrayFrom)
itemFilters.ts itemFilterExpressions(eb, filters, favoritesCustomerId)
│ → Expression<SqlBool>[]
routes/admin.ts adminItemQuery().where(eb => eb.and(itemFilterExpressions(...)))
routes/items.ts publicItemQuery().where(eb => eb.and([notPending(eb), ...itemFilterExpressions(...)]))
```
### The six clauses
Each keeps its existing comment, because each records a decision rather than describing the code:
| Clause | Becomes |
|---|---|
| category subtree | `sql<SqlBool>` template, recursive CTE unchanged, `${ids}` now one bind parameter |
| tags AND-match | `sql<SqlBool>` template, count equality unchanged |
| min / max price | `eb('i.price_cents', '>=' / '<=', value)` |
| status | `eb('i.status', 'in', statuses)` |
| favorites | `eb.exists(...)` on `favorites`, correlated with `whereRef` |
The two that stay `sql` templates stay for the reason `adminCategories.ts` gives for its own CTE: they are recursive or aggregate fragments that the builder expresses no better, and their values are already bind parameters. The four that become builder expressions do so because they are plain column comparisons and there is no reason for them not to be.
**`ANY($n::int[])` becomes `= ANY(${ids}::int[])` inside the template, and that is one parameter rather than a placeholder list** — the property #297 verified against emitted SQL, and the reason no `sql.param()` ceremony appears anywhere here.
### `favoritesCustomerId` keeps its throw
`favoritesOnly` with a null customer id still throws rather than returning no clause. Both routes already refuse that combination before calling, so reaching it is a programming error — and the alternative, quietly dropping the filter, lists the whole catalogue to someone who asked for their favourites.
## Failure handling
| What happens | Result |
|---|---|
| No filters at all | `eb.and([])` — Kysely emits no `where`, matching today's `clauses.length ? … : ''`. The storefront still has its not-pending clause, so its `and` is never empty. |
| A filter value that is not an integer | Unchanged: `parseItemFilters` rejects it before this, with the same 400. |
| `favoritesOnly` with no customer | Throws, as today. Both routes guard it first. |
| A column renamed in a migration | Compile error, where today it is a run-time `undefined` that only the integration suite catches. |
## Testing
- **Unit:** `itemFilters`' existing tests assert on `clauses` and `params`, which no longer exist. They are rewritten to compile each expression and assert the emitted SQL and parameters — which is a stronger assertion than counting fragments, and is how #297 established the array behaviour in the first place. The parser tests over `parseItemFilters` are untouched; that function does not change.
- **Integration:** every existing filter test must pass unchanged. They are the contract — same JSON, same ordering, same statuses. Nothing in them should need editing, and an edit to one is a signal the conversion changed behaviour.
- **The two invariant tests must be re-pointed, not deleted.** `tests/unit/itemFilters.test.ts` ends with a describe block, `buildItemFilterSql keeps every value out of the SQL text`, holding exactly two: one that pushes `"1); DROP TABLE items; --"` through every filter field — built by hand, deliberately bypassing the parser, because the claim is that the fragments are safe with no parser at all — and one asserting that two completely different filter sets produce byte-identical SQL.
Both survive the conversion and get stronger. Today they inspect the `clauses` strings the function returns; afterwards they compile the expressions and assert on the SQL Kysely actually emits, with the values appearing in `parameters` and nowhere else. That is the same claim tested against the real artefact rather than an intermediate one. The hostile-value test in particular is the thing standing between a future edit and a live injection on a route reachable without signing in, so it must fail if someone reintroduces interpolation.
- **Whole suite:** backend unit and integration green. The storefront list is the busiest query in the application.
## Out of scope
**The other ~236 raw `pool.query` sites.** They stay. #294 established that a fixed-shape query in a named constant is already safe, and most of them have no reason to move at all.
**Any change to what the endpoints return.** Same columns, same order, same JSON. If a response changes, the conversion is wrong.
**`parseItemFilters` and the filter parsing.** Untouched. This converts how the filters are applied, not how they are read.
@@ -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<DB>({ 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.
+6 -2
View File
@@ -135,8 +135,12 @@ $script:DEFAULT_NODE_VERSION = '18.16.1'
# not free: post-20 syntax and node: APIs pass here and fail in the pipeline, and # not free: post-20 syntax and node: APIs pass here and fail in the pipeline, and
# Node 26 ships an npm that can touch the lockfile in ways CI's npm reads # Node 26 ships an npm that can touch the lockfile in ways CI's npm reads
# differently. The `engines` field in both package.json files records the floor # differently. The `engines` field in both package.json files records the floor
# machine-readably; nothing yet catches "too new for where this ships". See #208 # machine-readably.
# part 4, which is an open decision rather than an oversight. #
# Settled in #208 part 4: the pin stays at 26.7.0 and CI on 20 is the backstop.
# The cost is knowing that "it passed locally" does not mean it ships — a post-20
# API is caught after a push rather than before one. That is accepted rather than
# unnoticed, which is the only reason this paragraph is here.
<# <#
Switches to the pinned version and insists it clears the floor. Switches to the pinned version and insists it clears the floor.