fix(db): narrow the status column instead of asserting the row (#308)
`.$castTo<AdminItemRow>()` / `.$castTo<PublicItemRow>()` at the two list-query call sites replaced the entire result type with an assertion rather than narrowing the one column that actually disagreed, which meant a projection that silently lost a column would still compile — exactly the failure mode this task exists to close, and the opposite of what the commit body claims. Fixed at the source instead: `itemSelect.ts` now defines `ItemsWithStatus`/`ItemDB`, narrowing `items.status` from the schema mirror's `Generated<string>` (a CHECK-constrained text column, so `kysely-codegen` has no literal union to give it) to `Generated<ItemStatus>`, and builds `ItemContext`, `adminItemQuery()` and `publicItemQuery()` from an `itemDb` typed with `ItemDB` instead of `db`/`DB`. Both `$castTo` calls and their comments are gone; the `AdminItemRow[]` / `PublicItemRow[]` annotations at the two call sites now check for real. Verified by temporarily dropping a column from `adminItemQuery`'s projection: the `AdminItemRow[]` assignment failed to compile as expected, confirming the guarantee actually holds. Also corrected two now-false statements left over from the conversion: `db-kysely/CONVENTIONS.md`'s worked-example section said `buildItemFilterSql` was "still raw `pg`" and that converting it "would put a second copy of a live function in `src/` that nothing calls" — both untrue since #308 shipped it as `itemFilterExpressions`. And two doc comments in `itemSelect.ts` still named the deleted `PUBLIC_ITEM_SELECT`/`ADMIN_ITEM_SELECT` constants instead of the functions that replaced them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -37,7 +37,7 @@ Kysely uses the `pg` driver directly, so a Postgres SQLSTATE stays on `err.code`
|
||||
|
||||
## 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 still raw `pg` in `src/itemFilters.ts`, used by both the storefront and the admin listing. The conversion below is an example rather than a source file: committing it as one would put a second copy of a live function in `src/` that nothing calls, which is precisely what the #216 spike became.
|
||||
`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) {
|
||||
|
||||
@@ -21,12 +21,27 @@
|
||||
// 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 { 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';
|
||||
|
||||
/**
|
||||
* The mirror types `items.status` as `Generated<string>` because Postgres holds
|
||||
* it as CHECK-constrained text rather than a native enum, so `kysely-codegen`
|
||||
* has nothing narrower to emit. `types.ts` already states the real domain.
|
||||
*
|
||||
* Narrowed here, once, rather than asserted at each call site. `$castTo` at the
|
||||
* 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.
|
||||
*
|
||||
@@ -36,7 +51,7 @@ import { ItemStatus, ItemImage, ItemTag } from './types';
|
||||
* the builder would have made the diff unreadable against the SQL it replaces.
|
||||
*/
|
||||
export type ItemContext = ExpressionBuilder<
|
||||
DB & { i: DB['items']; c: DB['categories'] },
|
||||
ItemDB & { i: ItemDB['items']; c: ItemDB['categories'] },
|
||||
'i' | 'c'
|
||||
>;
|
||||
|
||||
@@ -92,7 +107,7 @@ function tagsFor(eb: ItemContext) {
|
||||
* obvious that adding a `where` does not affect anyone else.
|
||||
*/
|
||||
export function publicItemQuery() {
|
||||
return db
|
||||
return itemDb
|
||||
.selectFrom('items as i')
|
||||
.leftJoin('categories as c', 'c.id', 'i.category_id')
|
||||
.select([
|
||||
@@ -111,7 +126,7 @@ export function publicItemQuery() {
|
||||
|
||||
/** The admin projection — every item column, plus the admin image fields. */
|
||||
export function adminItemQuery() {
|
||||
return db
|
||||
return itemDb
|
||||
.selectFrom('items as i')
|
||||
.leftJoin('categories as c', 'c.id', 'i.category_id')
|
||||
.selectAll('i')
|
||||
@@ -135,7 +150,7 @@ interface ItemRowBase {
|
||||
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;
|
||||
|
||||
/**
|
||||
@@ -148,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 whole reason the two selects differ. `images` is narrowed rather than
|
||||
|
||||
@@ -137,15 +137,9 @@ router.get('/items', asyncRoute(async (req: Request, res: Response) => {
|
||||
// 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.
|
||||
// `.$castTo` narrows `status` from the schema mirror's generic `string` (the
|
||||
// column is a CHECK-constrained text column, not a native Postgres enum, so
|
||||
// kysely-codegen has no literal union to give it) to the app-level
|
||||
// `ItemStatus` the CHECK constraint actually enforces. Every other field on
|
||||
// AdminItemRow already matches the projection without a cast.
|
||||
const rows: AdminItemRow[] = await adminItemQuery()
|
||||
.where((eb) => eb.and(itemFilterExpressions(eb, filters, null)))
|
||||
.orderBy('i.created_at', 'desc')
|
||||
.$castTo<AdminItemRow>()
|
||||
.execute();
|
||||
|
||||
res.json(rows);
|
||||
|
||||
@@ -79,11 +79,6 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => {
|
||||
status: filters.status ?? [...defaultStatuses]
|
||||
};
|
||||
|
||||
// `.$castTo` narrows `status` from the schema mirror's generic `string` (the
|
||||
// column is a CHECK-constrained text column, not a native Postgres enum, so
|
||||
// kysely-codegen has no literal union to give it) to the app-level
|
||||
// `ItemStatus` the CHECK constraint actually enforces. Every other field on
|
||||
// PublicItemRow already matches the projection without a cast.
|
||||
const rows: PublicItemRow[] = await publicItemQuery()
|
||||
.where((eb) =>
|
||||
eb.and([
|
||||
@@ -92,7 +87,6 @@ router.get('/', asyncRoute(async (req: Request, res: Response) => {
|
||||
])
|
||||
)
|
||||
.orderBy('i.created_at', 'desc')
|
||||
.$castTo<PublicItemRow>()
|
||||
.execute();
|
||||
|
||||
res.json(rows);
|
||||
|
||||
Reference in New Issue
Block a user