Query results are untyped, so strict: true stops at the database boundary #159

New Issue

2026-08-24T12:19:36-05:00

bermudalamb commented

2026-08-24 12:19:36 -05:00

pg types result.rows as any[]. Every row this application reads therefore enters a strict: true codebase as any, and everything derived from it is unchecked from that point on — rows[0].price_cents, a field renamed in a migration, a column dropped from a SELECT. The compiler has nothing to say about any of it.

Measured on main:


`pool.query<T>(...)` with a row type	1
`pool.query(...)` total	89
`client.query(...)` total	96
`rows[0]` reads	36
Row interfaces in the codebase	1 (`CustomerRow`)

Roughly 184 query sites, one of them typed. Concentrated in a handful of files:

43  routes/customers.ts
29  routes/cartCheckout.ts
25  routes/admin.ts
19  routes/cart.ts
19  routes/adminCustomers.ts
18  routes/shippingAddresses.ts

The TypeScript hygiene here is otherwise good — strict: true on both workspaces, no enum, two defensible non-null assertions, no @ts-ignore anywhere. This is the one place the type system is switched off, and it is the boundary where the data actually comes from.

Why this blocks #101

#101 wants noUncheckedIndexedAccess, which turns rows[0] into T | undefined and demands the check. Enabled today it would bite on almost nothing, because rows[0] is already any and any indexes to any. The violation count would be small, would suggest the flag was cheap, and would change completely once the rows have real types.

That issue currently says it waits on "#65 stage 3". #65 is closed and turned out to be Import convention and avoidable any, which had no stages and never included a typing programme — it delivered CustomerRow and the PoolClient annotations in cartCheckout.ts and nothing more. So #101's dependency is real but points at nothing. This is that dependency.

Where the types belong

itemSelect.ts is the precedent worth following. It already centralises the item SELECT text precisely because two callers need the same shape and a third one-to-many relation would otherwise multiply rows — but nothing types what those SELECTs return. The row type belongs next to the SELECT that produces it, so the two cannot drift: a column added to PUBLIC_ITEM_SELECT and not to its type is then a compile error rather than a field that is silently any.

That argues against one big types.ts of table-shaped interfaces. A table interface describes the table; what a route needs is the shape of its projection, and those differ deliberately — PUBLIC_ITEM_SELECT omits paypal_order_id and reserved_until because the storefront has no business seeing them. Typing rows as the full table would quietly re-admit exactly the columns that select was written to exclude.

Scope

Row types for the projections actually selected, defined beside their SELECT
Applied to pool.query and client.query alike. The client.query calls are the transaction paths in cartCheckout.ts, where a mistake is most expensive and where any currently hides a client/pool mix-up
CustomerRow already exists and moves to wherever the convention lands
Landing in stages by file rather than as one 184-site change, since the point is to be reviewable

Deliberately not in scope

Generating types from the schema. pg-to-ts, Prisma introspection and friends solve this properly and bring a build step, a dependency, and a generated artifact to keep in step with node-pg-migrate. That is a bigger commitment than this repository has taken elsewhere, and it should be a decision rather than a side effect of wanting rows[0] checked. Recorded here so it is not re-litigated in review.

Turning on noUncheckedIndexedAccess. That is #101, and it comes after.

Acceptance criteria

Every .query(...) call in backend/src carries a row type, or is explicitly exempted with a reason
Row types live beside the SELECT they describe rather than in one table-shaped module
A column removed from a shared SELECT and not from its type fails the build, demonstrated rather than assumed
No new dependency and no generated code
#101's dependency is re-pointed at this issue and the stale #65 reference removed

`pg` types `result.rows` as `any[]`. Every row this application reads therefore enters a `strict: true` codebase as `any`, and everything derived from it is unchecked from that point on — `rows[0].price_cents`, a field renamed in a migration, a column dropped from a SELECT. The compiler has nothing to say about any of it. Measured on `main`: | | | | --- | --- | | `pool.query<T>(...)` with a row type | **1** | | `pool.query(...)` total | 89 | | `client.query(...)` total | 96 | | `rows[0]` reads | 36 | | Row interfaces in the codebase | **1** (`CustomerRow`) | Roughly 184 query sites, one of them typed. Concentrated in a handful of files: ``` 43 routes/customers.ts 29 routes/cartCheckout.ts 25 routes/admin.ts 19 routes/cart.ts 19 routes/adminCustomers.ts 18 routes/shippingAddresses.ts ``` The TypeScript hygiene here is otherwise good — `strict: true` on both workspaces, no `enum`, two defensible non-null assertions, no `@ts-ignore` anywhere. This is the one place the type system is switched off, and it is the boundary where the data actually comes from. ## Why this blocks #101 #101 wants `noUncheckedIndexedAccess`, which turns `rows[0]` into `T | undefined` and demands the check. Enabled today it would bite on almost nothing, because `rows[0]` is already `any` and `any` indexes to `any`. The violation count would be small, would suggest the flag was cheap, and would change completely once the rows have real types. That issue currently says it waits on "#65 stage 3". #65 is closed and turned out to be *Import convention and avoidable `any`*, which had no stages and never included a typing programme — it delivered `CustomerRow` and the `PoolClient` annotations in `cartCheckout.ts` and nothing more. So #101's dependency is real but points at nothing. This is that dependency. ## Where the types belong `itemSelect.ts` is the precedent worth following. It already centralises the item SELECT text precisely because two callers need the same shape and a third one-to-many relation would otherwise multiply rows — but nothing types what those SELECTs return. **The row type belongs next to the SELECT that produces it**, so the two cannot drift: a column added to `PUBLIC_ITEM_SELECT` and not to its type is then a compile error rather than a field that is silently `any`. That argues against one big `types.ts` of table-shaped interfaces. A table interface describes the table; what a route needs is the shape of *its* projection, and those differ deliberately — `PUBLIC_ITEM_SELECT` omits `paypal_order_id` and `reserved_until` because the storefront has no business seeing them. Typing rows as the full table would quietly re-admit exactly the columns that select was written to exclude. ## Scope - Row types for the projections actually selected, defined beside their SELECT - Applied to `pool.query` and `client.query` alike. The `client.query` calls are the transaction paths in `cartCheckout.ts`, where a mistake is most expensive and where `any` currently hides a client/pool mix-up - `CustomerRow` already exists and moves to wherever the convention lands - Landing in stages by file rather than as one 184-site change, since the point is to be reviewable ## Deliberately not in scope **Generating types from the schema.** `pg-to-ts`, Prisma introspection and friends solve this properly and bring a build step, a dependency, and a generated artifact to keep in step with `node-pg-migrate`. That is a bigger commitment than this repository has taken elsewhere, and it should be a decision rather than a side effect of wanting `rows[0]` checked. Recorded here so it is not re-litigated in review. **Turning on `noUncheckedIndexedAccess`.** That is #101, and it comes after. ## Acceptance criteria - Every `.query(...)` call in `backend/src` carries a row type, or is explicitly exempted with a reason - Row types live beside the SELECT they describe rather than in one table-shaped module - A column removed from a shared SELECT and not from its type fails the build, demonstrated rather than assumed - No new dependency and no generated code - #101's dependency is re-pointed at this issue and the stale #65 reference removed

bermudalamb referenced this issue

2026-08-24 12:19:59 -05:00

Turn on noUncheckedIndexedAccess once #65 has typed the query results #101

bermudalamb added this to the Code Quality and Hardening 2 project 2026-08-24 12:20:20 -05:00

bermudalamb self-assigned this 2026-08-24 12:20:29 -05:00

bermudalamb commented

2026-08-24 12:20:57 -05:00

Correction to the acceptance criteria before any work starts: one of them cannot be met by the approach this issue chooses, and it was wrong to write it.

A column removed from a shared SELECT and not from its type fails the build, demonstrated rather than assumed

It will not. pool.query<T>(...) is an assertion about the shape, not a check against the SQL. TypeScript never reads the query string, so a column dropped from PUBLIC_ITEM_SELECT while ItemRow still declares it compiles cleanly, and every read of that field keeps type-checking while being undefined at runtime. The type would simply be a lie the compiler now believes.

Catching that genuinely requires the schema-aware tooling this issue rules out — generation from the database, or a query builder that owns both halves. Wanting the criterion does not make the chosen approach able to deliver it.

What typing the rows does buy, which is still worth doing and is what #101 needs:

A misspelled field at a read site becomes a compile error instead of undefined
A rename applied to the type fails at every read site that still uses the old name, so the change is complete rather than mostly complete
rows[0] stops being any, which is the precondition that makes noUncheckedIndexedAccess measure something real
The shape a route actually returns becomes readable without reconstructing it from the SQL

What it does not buy, and should be written into the code rather than assumed by the next reader: the type and the SELECT are kept in step by hand. That is a weaker guarantee than it looks, and the comment beside each shared SELECT should say so, otherwise the type reads as verified when it is asserted.

So that criterion is replaced by:

Each shared SELECT's row type sits beside it with a note that the two are hand-kept, since nothing checks the SQL against the type
The integration suite is what actually catches a SELECT and its type disagreeing, because it runs the real queries against a real schema — worth stating, as it is the only thing that does

Correction to the acceptance criteria before any work starts: one of them cannot be met by the approach this issue chooses, and it was wrong to write it. > A column removed from a shared SELECT and not from its type fails the build, demonstrated rather than assumed It will not. `pool.query<T>(...)` is an assertion about the shape, not a check against the SQL. TypeScript never reads the query string, so a column dropped from `PUBLIC_ITEM_SELECT` while `ItemRow` still declares it compiles cleanly, and every read of that field keeps type-checking while being `undefined` at runtime. The type would simply be a lie the compiler now believes. Catching that genuinely requires the schema-aware tooling this issue rules out — generation from the database, or a query builder that owns both halves. Wanting the criterion does not make the chosen approach able to deliver it. **What typing the rows does buy**, which is still worth doing and is what #101 needs: - A misspelled field at a read site becomes a compile error instead of `undefined` - A rename applied to the type fails at every read site that still uses the old name, so the change is complete rather than mostly complete - `rows[0]` stops being `any`, which is the precondition that makes `noUncheckedIndexedAccess` measure something real - The shape a route actually returns becomes readable without reconstructing it from the SQL **What it does not buy**, and should be written into the code rather than assumed by the next reader: the type and the SELECT are kept in step by hand. That is a weaker guarantee than it looks, and the comment beside each shared SELECT should say so, otherwise the type reads as verified when it is asserted. So that criterion is replaced by: - Each shared SELECT's row type sits beside it with a note that the two are hand-kept, since nothing checks the SQL against the type - The integration suite is what actually catches a SELECT and its type disagreeing, because it runs the real queries against a real schema — worth stating, as it is the only thing that does

bermudalamb referenced this issue from a commit

2026-08-24 13:00:38 -05:00

refactor(backend): type the item query results (#159)

bermudalamb referenced this issue

2026-08-24 13:01:03 -05:00

refactor(backend): type the item query results (#159) #160

bermudalamb added reference feature/159-type-query-results

2026-08-24 13:01:21 -05:00

bermudalamb referenced this issue from a commit

2026-08-24 13:01:51 -05:00

refactor(backend): type the item query results (#159)

bermudalamb referenced this issue from a commit