bermudalamb/redefined-designs
1
0
Fork 0
Code Issues 2 Pull Requests 1 Actions Packages Projects 3 Releases Wiki Activity

feat(db): convert routes/adminCategories.ts to Drizzle, and measure the real per-site cost #218

New Issue
Closed
opened 2026-08-28 15:17:24 -05:00 by bermudalamb · 1 comment
bermudalamb commented 2026-08-28 15:17:24 -05:00
Owner
Copy Link
Copy Source

Second step of the Drizzle adoption decided in #216, and the one that produces the number the rest of the plan depends on.

This absorbs step 4 of the spike, which was not completed — Tinqer was added to that comparison partway through and took its budget. So the current per-site estimate is an extrapolation from one function and should not be trusted. This issue replaces it with a measurement.

Why this file

routes/adminCategories.ts has 9 query sites and contains SUBTREE_CTE, the recursive category walk. It is small enough to finish in one sitting and awkward enough to be representative — a file of plain CRUD would produce a flattering number that does not generalise to customers.ts (43 sites) or cartCheckout.ts (30).

Scope

Convert all 9 sites, end to end, with the integration suite green throughout. The pool stays available; this is one file moving, not a cutover.

What to record

  • Actual time per site, split between the mechanical ones and the awkward ones. This is the deliverable.
  • Whether the hand-declared row interfaces can be derived from the schema rather than maintained beside it. itemSelect.ts carries a comment stating it is "KEPT IN STEP BY HAND" and that the integration suite is the only thing catching drift — closing that is a large part of the value, and this file is where it first gets tested.
  • Whether strict and noUncheckedIndexedAccess hold without spraying non-null assertions. If inferred row types need ! everywhere, the type safety is nominal and that changes the recommendation.
  • Anything that needed a sql escape hatch, and whether sql.param() was needed for arrays (see the convention issue).

Done when

The file is converted, both suites pass, and this issue carries a measured per-site cost plus a revised estimate for the remaining ~178 sites — enough for a go or no-go on the rest.

Refs #216

Second step of the Drizzle adoption decided in #216, and the one that produces the number the rest of the plan depends on. This absorbs step 4 of the spike, which was **not completed** — Tinqer was added to that comparison partway through and took its budget. So the current per-site estimate is an extrapolation from one function and should not be trusted. This issue replaces it with a measurement. ## Why this file `routes/adminCategories.ts` has 9 query sites and contains `SUBTREE_CTE`, the recursive category walk. It is small enough to finish in one sitting and awkward enough to be representative — a file of plain CRUD would produce a flattering number that does not generalise to `customers.ts` (43 sites) or `cartCheckout.ts` (30). ## Scope Convert all 9 sites, end to end, with the integration suite green throughout. The `pool` stays available; this is one file moving, not a cutover. ## What to record - **Actual time per site**, split between the mechanical ones and the awkward ones. This is the deliverable. - Whether the hand-declared row interfaces can be **derived from the schema** rather than maintained beside it. `itemSelect.ts` carries a comment stating it is "KEPT IN STEP BY HAND" and that the integration suite is the only thing catching drift — closing that is a large part of the value, and this file is where it first gets tested. - Whether `strict` and `noUncheckedIndexedAccess` hold without spraying non-null assertions. If inferred row types need `!` everywhere, the type safety is nominal and that changes the recommendation. - Anything that needed a `sql` escape hatch, and whether `sql.param()` was needed for arrays (see the convention issue). ## Done when The file is converted, both suites pass, and this issue carries a measured per-site cost plus a revised estimate for the remaining ~178 sites — enough for a go or no-go on the rest. Refs #216
bermudalamb commented 2026-09-02 09:03:38 -05:00
Author
Owner
Copy Link
Copy Source

Converted, all nine sites, both suites green — 382 unit and 345 integration.

The measurement

Sites Kind Cost
5 Mechanical — SELECT by id, INSERT ... RETURNING, UPDATE ... RETURNING, DELETE, an existence probe a few minutes each, essentially transcription
2 Recursive CTE, consumed two different ways stayed a sql template; the builder's $with() bought nothing over SQL that was already correct and reviewed
1 ANY($1::int[]) cheaper than before — inArray removes the trap rather than working around it
1 Correlated subquery with a cast the expensive one, for the reason below

Wall-clock for the file was well under an hour including the two defects. Extrapolating to the remaining ~178 sites: the mechanical majority is minutes each, and the cost is concentrated entirely in the awkward minority. The spike's 15–30 minutes for awkward sites holds; what it missed is that the awkward ones can be silently wrong rather than merely slow.

The finding that matters more than the number

Drizzle renders a column reference inside a sql template unqualified. This:

sql<number>`(SELECT COUNT(*)::int FROM ${items} WHERE ${items.categoryId} = ${categories.id})`

generates:

(SELECT COUNT(*)::int FROM "items" WHERE "category_id" = "id")

Postgres resolves both sides against items, so the correlated subquery correlates with itself. It type-checks, reads correctly, executes without error, and returns a plausible wrong number — the item count came back 1 where it should have been 2.

That is worse than the sql.param() array trap already in CONVENTIONS.md, because the array trap produces invalid SQL and fails loudly. This one produces valid SQL and quietly wrong data. Any converted query with a correlated subquery or a self-join needs a test that asserts values, not just a status code.

Second, quieter one: the driver's error code moves. Drizzle wraps errors, so a SQLSTATE that sat on err.code now sits on err.cause.code. The existing 23505 check compiled, never matched, and turned two 409s into 500s. Every catch keyed on a Postgres error code has to be revisited on conversion, and nothing about the types says so.

Both were caught by the integration suite and only by the integration suite. Neither produced a type error, a lint warning, or a failing build.

The questions the issue asked

Can row interfaces be derived from the schema? Yes, and it is the clearest win. CategoryRow, CategoryListRow, IdRow, CountRow and ExistsProbe are all gone; one CATEGORY_COLUMNS object is written once and the row type is inferred from it. That closes the drift itemSelect.ts documents as "KEPT IN STEP BY HAND".

With one caveat that will apply to every conversion: the mirror names columns in camelCase and these APIs answer in snake_case, so the mapping must be explicit. Selecting the table directly would have silently changed the JSON contract the admin frontend reads, and no test checking status codes would have noticed.

Do strict and noUncheckedIndexedAccess hold? Yes, with no non-null assertions added. requireRow covers the RETURNING rows and the existing lookup destructures and branches exactly as it did.

Escape hatches needed? Two: the recursive CTE, and the correlated subquery — the latter written as literal SQL text rather than with interpolated column references, for the reason above. sql.param() was not needed, because inArray removed the only array site.

Recommendation

Go, with a condition. The conversion is faithful, the types are better, and the injection property is real. But convert a file at a time behind a green integration suite, and treat any query containing a correlated subquery, a self-join, or a SQLSTATE-keyed catch as needing a value-asserting test written before the conversion — those are the two shapes that fail silently.

Do not convert files that lack integration coverage until they have it. The type system did not catch either defect and will not catch the next one.

Closes #218

Converted, all nine sites, both suites green — 382 unit and 345 integration. ## The measurement | Sites | Kind | Cost | |---|---|---| | 5 | Mechanical — `SELECT` by id, `INSERT ... RETURNING`, `UPDATE ... RETURNING`, `DELETE`, an existence probe | a few minutes each, essentially transcription | | 2 | Recursive CTE, consumed two different ways | stayed a `sql` template; the builder's `$with()` bought nothing over SQL that was already correct and reviewed | | 1 | `ANY($1::int[])` | **cheaper than before** — `inArray` removes the trap rather than working around it | | 1 | Correlated subquery with a cast | the expensive one, for the reason below | Wall-clock for the file was well under an hour including the two defects. **Extrapolating to the remaining ~178 sites: the mechanical majority is minutes each, and the cost is concentrated entirely in the awkward minority.** The spike's 15–30 minutes for awkward sites holds; what it missed is that the awkward ones can be silently wrong rather than merely slow. ## The finding that matters more than the number **Drizzle renders a column reference inside a `sql` template unqualified.** This: ```ts sql<number>`(SELECT COUNT(*)::int FROM ${items} WHERE ${items.categoryId} = ${categories.id})` ``` generates: ```sql (SELECT COUNT(*)::int FROM "items" WHERE "category_id" = "id") ``` Postgres resolves both sides against `items`, so the correlated subquery correlates with itself. It type-checks, reads correctly, executes without error, and returns a **plausible wrong number** — the item count came back 1 where it should have been 2. That is worse than the `sql.param()` array trap already in CONVENTIONS.md, because the array trap produces invalid SQL and fails loudly. This one produces valid SQL and quietly wrong data. Any converted query with a correlated subquery or a self-join needs a test that asserts *values*, not just a status code. **Second, quieter one: the driver's error code moves.** Drizzle wraps errors, so a SQLSTATE that sat on `err.code` now sits on `err.cause.code`. The existing `23505` check compiled, never matched, and turned two 409s into 500s. Every `catch` keyed on a Postgres error code has to be revisited on conversion, and nothing about the types says so. Both were caught by the integration suite and only by the integration suite. Neither produced a type error, a lint warning, or a failing build. ## The questions the issue asked **Can row interfaces be derived from the schema?** Yes, and it is the clearest win. `CategoryRow`, `CategoryListRow`, `IdRow`, `CountRow` and `ExistsProbe` are all gone; one `CATEGORY_COLUMNS` object is written once and the row type is inferred from it. That closes the drift `itemSelect.ts` documents as "KEPT IN STEP BY HAND". With one caveat that will apply to every conversion: the mirror names columns in camelCase and these APIs answer in snake_case, so the mapping must be **explicit**. Selecting the table directly would have silently changed the JSON contract the admin frontend reads, and no test checking status codes would have noticed. **Do `strict` and `noUncheckedIndexedAccess` hold?** Yes, with no non-null assertions added. `requireRow` covers the `RETURNING` rows and the existing lookup destructures and branches exactly as it did. **Escape hatches needed?** Two: the recursive CTE, and the correlated subquery — the latter written as literal SQL text rather than with interpolated column references, for the reason above. `sql.param()` was not needed, because `inArray` removed the only array site. ## Recommendation **Go, with a condition.** The conversion is faithful, the types are better, and the injection property is real. But convert a file at a time behind a green integration suite, and treat any query containing a correlated subquery, a self-join, or a SQLSTATE-keyed `catch` as needing a value-asserting test written *before* the conversion — those are the two shapes that fail silently. Do not convert files that lack integration coverage until they have it. The type system did not catch either defect and will not catch the next one. Closes #218
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
calamb (Chelsea Lamb) bermudalamb (Thom Lamb)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#218