diff --git a/backend/src/routes/adminCategories.ts b/backend/src/routes/adminCategories.ts index a8d66da..bb392d0 100644 --- a/backend/src/routes/adminCategories.ts +++ b/backend/src/routes/adminCategories.ts @@ -1,35 +1,38 @@ import { Router, Request, Response } from 'express'; -import { pool, requireRow } from '../db'; +import { eq, inArray, sql } from 'drizzle-orm'; +import { db, requireRow } from '../db'; +import { categories, items } from '../db-drizzle/schema'; import { asyncRoute } from '../asyncRoute'; -interface CategoryRow { - id: number; - name: string; - parent_id: number | null; - sort_order: number; -} - -/** The tree adds a usage count, cast to int so it arrives as a number. */ -interface CategoryListRow extends CategoryRow { - item_count: number; -} - -interface IdRow { - id: number; -} - -interface CountRow { - n: number; -} +/** + * The first file converted to Drizzle (#218), chosen because it is awkward + * rather than because it is easy — nine sites including a recursive CTE and an + * array match. See src/db-drizzle/CONVENTIONS.md. + * + * The pool is still available and most of the application still uses it. This + * is one file moving, not a cutover. + */ /** - * `SELECT 1 ...`, used only for `.length`. The column has no name of its own — - * Postgres calls it `?column?` — so the shape is an index signature rather than - * a field, and nothing reads a value out of it. + * The response shape, written once. + * + * The generated mirror names columns in camelCase — `parentId`, `sortOrder` — + * and this API answers in snake_case, which the admin frontend reads. So the + * mapping is explicit here rather than implicit anywhere: selecting the table + * directly would silently change the JSON contract, and no test that checks + * status codes would catch it. + * + * It also answers the question #218 asked. The row type is inferred from this + * object rather than hand-declared beside the query, so the interfaces that used + * to sit at the top of this file are gone and cannot drift from what is + * selected. */ -interface ExistsProbe { - [column: string]: number; -} +const CATEGORY_COLUMNS = { + id: categories.id, + name: categories.name, + parent_id: categories.parentId, + sort_order: categories.sortOrder +}; const router = Router(); @@ -37,11 +40,35 @@ const router = Router(); // stop siblings sharing a name. const UNIQUE_VIOLATION = '23505'; -// 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. -const SUBTREE_CTE = ` +/** + * Whether a thrown error is that unique violation. + * + * Drizzle wraps driver errors, so the SQLSTATE that used to sit on `err.code` + * now sits on `err.cause.code`. The old check still compiled and simply never + * matched, turning two 409s into 500s — a conversion hazard with no type error + * and no failing build behind it, only two integration tests. Both shapes are + * accepted so this keeps working either side of a conversion. See #218. + */ +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. Drizzle has `$with()` for CTEs, but this one is + * recursive and is consumed in two different shapes, and expressing it through + * the builder bought nothing over the SQL that is already correct and reviewed. + * The important part is that `${id}` here is a bind parameter, not text — there + * is no way to spell string interpolation in this template by accident, which is + * the property the whole adoption is for. + */ +const subtreeOf = (id: number) => sql` WITH RECURSIVE subtree AS ( - SELECT id FROM categories WHERE id = $1 + SELECT id FROM categories WHERE id = ${id} UNION ALL SELECT c.id FROM categories c JOIN subtree s ON c.parent_id = s.id )`; @@ -62,17 +89,29 @@ function readParentId(value: unknown): number | null | undefined { } async function parentExists(id: number): Promise { - const { rows } = await pool.query(`SELECT 1 FROM categories WHERE id = $1`, [id]); + const rows = await db + .select({ id: categories.id }) + .from(categories) + .where(eq(categories.id, id)) + .limit(1); return rows.length > 0; } router.get('/', asyncRoute(async (_req: Request, res: Response) => { - const { rows } = await pool.query( - `SELECT c.id, c.name, c.parent_id, c.sort_order, - (SELECT COUNT(*)::int FROM items i WHERE i.category_id = c.id) AS item_count - FROM categories c - ORDER BY c.sort_order, lower(c.name)` - ); + const rows = await db + .select({ + ...CATEGORY_COLUMNS, + // Written as literal SQL, NOT with ${items.categoryId} and + // ${categories.id}. Drizzle renders a column reference inside a sql + // template UNQUALIFIED — those two produced `WHERE "category_id" = "id"`, + // which Postgres resolved against items for both sides and answered with + // a plausible wrong number rather than an error. There are no values to + // bind in this fragment, so literal text is the honest form. See #218. + item_count: sql`(SELECT COUNT(*)::int FROM items WHERE items.category_id = categories.id)` + }) + .from(categories) + .orderBy(categories.sortOrder, sql`lower(categories.name)`); + res.json(rows); })); @@ -94,14 +133,14 @@ router.post('/', asyncRoute(async (req: Request, res: Response) => { const sortOrder = Number.isSafeInteger(req.body.sort_order) ? req.body.sort_order : 0; try { - const { rows } = await pool.query( - `INSERT INTO categories (name, parent_id, sort_order) VALUES ($1, $2, $3) - RETURNING id, name, parent_id, sort_order`, - [name, parent, sortOrder] - ); - res.status(201).json({ ...rows[0], item_count: 0 }); + const rows = await db + .insert(categories) + .values({ name, parentId: parent, sortOrder }) + .returning(CATEGORY_COLUMNS); + + res.status(201).json({ ...requireRow(rows, 'the category INSERT'), item_count: 0 }); } catch (err) { - if ((err as { code?: string }).code === UNIQUE_VIOLATION) { + if (isUniqueViolation(err)) { return res.status(409).json({ error: 'a category with that name already exists here' }); } throw err; @@ -136,11 +175,10 @@ async function resolveParentId( // Moving a node beneath itself or one of its own descendants would detach // that whole branch from the tree into an unreachable cycle. - const { rows: cycle } = await pool.query( - `${SUBTREE_CTE} SELECT 1 FROM subtree WHERE id = $2`, - [id, parsed] + const cycle = await db.execute( + sql`${subtreeOf(id)} SELECT 1 FROM subtree WHERE id = ${parsed}` ); - if (cycle.length) { + if (cycle.rows.length) { return { error: 'a category cannot be moved beneath itself' }; } @@ -149,12 +187,18 @@ async function resolveParentId( router.put('/:id', asyncRoute(async (req: Request, res: Response) => { const id = Number(req.params.id); - const existing = await pool.query(`SELECT id, name, parent_id, sort_order FROM categories WHERE id = $1`, [id]); - if (!existing.rows.length) { + const existing = await db + .select(CATEGORY_COLUMNS) + .from(categories) + .where(eq(categories.id, id)) + .limit(1); + + const current = existing[0]; + if (!current) { return res.status(404).json({ error: 'not found' }); } - let name = existing.rows[0].name; + let name = current.name; if (req.body.name !== undefined) { const parsed = readName(req.body.name); if (!parsed) { @@ -163,7 +207,7 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => { name = parsed; } - const resolved = await resolveParentId(req.body.parent_id, id, existing.rows[0].parent_id); + const resolved = await resolveParentId(req.body.parent_id, id, current.parent_id); if ('error' in resolved) { return res.status(400).json({ error: resolved.error }); } @@ -171,17 +215,18 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => { const sortOrder = Number.isSafeInteger(req.body.sort_order) ? req.body.sort_order - : existing.rows[0].sort_order; + : current.sort_order; try { - const { rows } = await pool.query( - `UPDATE categories SET name = $1, parent_id = $2, sort_order = $3 WHERE id = $4 - RETURNING id, name, parent_id, sort_order`, - [name, parent, sortOrder, id] - ); - res.json(rows[0]); + const rows = await db + .update(categories) + .set({ name, parentId: parent, sortOrder }) + .where(eq(categories.id, id)) + .returning(CATEGORY_COLUMNS); + + res.json(requireRow(rows, 'the category UPDATE')); } catch (err) { - if ((err as { code?: string }).code === UNIQUE_VIOLATION) { + if (isUniqueViolation(err)) { return res.status(409).json({ error: 'a category with that name already exists here' }); } throw err; @@ -190,22 +235,33 @@ router.put('/:id', asyncRoute(async (req: Request, res: Response) => { router.delete('/:id', asyncRoute(async (req: Request, res: Response) => { const id = Number(req.params.id); - const { rows: subtree } = await pool.query(`${SUBTREE_CTE} SELECT id FROM subtree`, [id]); - if (!subtree.length) { + + const subtree = await db.execute<{ id: number }>( + sql`${subtreeOf(id)} SELECT id FROM subtree` + ); + if (!subtree.rows.length) { return res.status(404).json({ error: 'not found' }); } - const ids = subtree.map((row: { id: number }) => row.id); - const { rows: affected } = await pool.query( - `SELECT COUNT(*)::int AS n FROM items WHERE category_id = ANY($1::int[])`, - [ids] - ); + const ids = subtree.rows.map((row) => row.id); + + // inArray rather than the ANY(...::int[]) this replaced, which sidesteps the + // array trap in CONVENTIONS.md entirely: there is no template to forget + // sql.param() in. The builder emits the placeholder list itself and it is + // correct by construction. + const affected = await db + .select({ n: sql`COUNT(*)::int` }) + .from(items) + .where(inArray(items.categoryId, ids)); // The FK cascade takes the descendants; items fall back to NULL rather than // being deleted along with their category. - await pool.query(`DELETE FROM categories WHERE id = $1`, [id]); + await db.delete(categories).where(eq(categories.id, id)); - res.json({ deleted_categories: ids.length, uncategorized_items: requireRow(affected, 'the affected-items COUNT').n }); + res.json({ + deleted_categories: ids.length, + uncategorized_items: requireRow(affected, 'the affected-items COUNT').n + }); })); export default router;