Files
redefined-designs/backend/src/itemFilters.ts
T
bermudalambandClaude Opus 5 ec1020891f 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>
2026-09-04 17:00:21 -05:00

333 lines
13 KiB
TypeScript

// Parsing and SQL construction for the storefront's category / tag / price
// filters. Kept apart from the route so the rules can be unit-tested without a
// database, and so items.ts stays a thin handler.
import { Expression, SqlBool, sql } from 'kysely';
import { ItemStatus } from './types';
import { ItemContext } from './itemSelect';
export type { ItemStatus };
export class FilterError extends Error {}
export interface ItemFilters {
// Several categories, combined as OR — a customer picking Furniture and
// Decor wants both, not the empty intersection. Deliberately the opposite of
// how tagIds combine below, which is AND; the two controls say so in the UI
// rather than leaving it to be discovered.
//
// Each selected id still expands to its descendants, so choosing two parents
// means "anything filed under either of them".
categoryIds: number[];
tagIds: number[];
minPriceCents: number | null;
maxPriceCents: number | null;
// Several statuses rather than one, because the control this exists to serve
// is not a status filter. "Not sold" is available-or-reserved on the
// storefront and available-or-reserved-or-pending in the admin, so it cannot
// be expressed as equality against a single value. A single-status filter is
// still expressible: it arrives as a list of one, which is how the admin's
// old `?status=sold` keeps working unchanged.
//
// Null means the caller expressed no preference, which is distinct from
// asking for every status — the storefront turns the first into its default
// and the second into an explicit list.
status: ItemStatus[] | null;
// Storefront only: "just the items I have favorited". Which customer that
// means is not part of the parsed filter — it comes from the session at build
// time, so a query string can never name someone else's favorites.
favoritesOnly: boolean;
}
// Matched exactly, not case-insensitively: `items.status` only ever holds these
// lowercase values, so accepting 'Reserved' would quietly return nothing rather
// than reporting that the filter was wrong.
const ITEM_STATUSES: readonly string[] = ['pending', 'available', 'reserved', 'sold'];
// Storefront-invalid statuses. This parser is shared with the admin routes,
// where filtering by 'pending' is exactly the point, so the public routes have
// to refuse it themselves rather than the parser refusing it for everyone.
export const NON_PUBLIC_STATUSES: readonly ItemStatus[] = ['pending'];
// What the storefront lists when the caller expressed no preference. Named here
// rather than implied by the absence of a parameter, because the absence is now
// meaningful: before this change no status meant every status, and afterwards it
// means these two. Anything reading a shared link from before will get the new
// meaning, which is the accepted cost of the default changing.
export const STOREFRONT_DEFAULT_STATUSES: readonly ItemStatus[] = ['available', 'reserved'];
// What "All" can mean on the storefront, which is not all of them. Pending items
// are excluded from every public read unconditionally, so a filter labelled All
// must not promise the fourth — a label that delivers less than it says is the
// shape this codebase keeps designing against.
export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold'];
// 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
// '10.5' are caller mistakes worth surfacing rather than silently coercing.
const NON_NEGATIVE_INTEGER = /^\d+$/;
// Accepts both spellings because these URLs get hand-edited and shared, but
// nothing else: '?favorites=yes' is a mistake worth reporting rather than
// treating as either on or off.
const TRUE_VALUES: readonly string[] = ['1', 'true'];
const FALSE_VALUES: readonly string[] = ['0', 'false'];
function singleValue(value: unknown, name: string): string | null {
if (value === undefined || value === null) {
return null;
}
// Express parses `?category=1&category=2` into an array. Picking one silently
// would make a malformed link look like it worked, so refuse it instead.
if (Array.isArray(value)) {
throw new FilterError(`${name} may only be given once`);
}
if (typeof value !== 'string') {
throw new FilterError(`invalid ${name}`);
}
return value;
}
function parseNonNegativeInteger(raw: string, name: string): number {
if (!NON_NEGATIVE_INTEGER.test(raw)) {
throw new FilterError(`invalid ${name}`);
}
const parsed = Number(raw);
if (!Number.isSafeInteger(parsed)) {
throw new FilterError(`invalid ${name}`);
}
return parsed;
}
function parseId(raw: string, name: string): number {
const parsed = parseNonNegativeInteger(raw, name);
if (parsed < 1) {
throw new FilterError(`invalid ${name}`);
}
return parsed;
}
function parsePrice(value: unknown, name: string): number | null {
const raw = singleValue(value, name);
if (raw === null || raw === '') {
return null;
}
return parseNonNegativeInteger(raw, name);
}
/**
* Comma-separated, the same shape `tags` and `status` already use.
*
* The parameter keeps its singular name so that every `?category=1` link,
* bookmark and shared URL written before this went multi-valued still parses —
* as a list of one, needing no alias and leaving no way to give the filter
* twice with different meanings.
*/
function parseCategoryIds(value: unknown): number[] {
const raw = singleValue(value, 'category');
const categoryIds: number[] = [];
if (!raw) {
return categoryIds;
}
for (const part of raw.split(',')) {
const trimmed = part.trim();
if (trimmed === '') {
continue;
}
const id = parseId(trimmed, 'category');
// Duplicates are harmless to the OR below, but they would show twice in
// any caller that renders the parsed filter back.
if (!categoryIds.includes(id)) {
categoryIds.push(id);
}
}
return categoryIds;
}
function parseTagIds(value: unknown): number[] {
const raw = singleValue(value, 'tags');
const tagIds: number[] = [];
if (!raw) {
return tagIds;
}
for (const part of raw.split(',')) {
const trimmed = part.trim();
if (trimmed === '') {
continue;
}
const id = parseId(trimmed, 'tags');
// Duplicates would inflate the required-match count in itemFilterExpressions
// and make the filter match nothing at all.
if (!tagIds.includes(id)) {
tagIds.push(id);
}
}
return tagIds;
}
// Comma-separated, matching how `tags` already works, so the two multi-value
// parameters in this parser read the same way in a URL.
//
// An unrecognised name is refused rather than dropped. Silently ignoring one
// would turn `?status=available,sold_out` into "available only" — narrower than
// what was asked for, and indistinguishable from a filter that worked.
function parseStatus(value: unknown): ItemStatus[] | null {
const raw = singleValue(value, 'status');
if (raw === null || raw === '') {
return null;
}
const statuses: ItemStatus[] = [];
for (const part of raw.split(',')) {
const trimmed = part.trim();
if (trimmed === '') {
continue;
}
if (!ITEM_STATUSES.includes(trimmed)) {
throw new FilterError('invalid status');
}
// Duplicates are harmless in `= ANY(...)`, but removing them keeps the
// parsed filter a faithful description of what was asked for.
if (!statuses.includes(trimmed as ItemStatus)) {
statuses.push(trimmed as ItemStatus);
}
}
// `?status=,,` asked for something and named nothing. Returning null would
// silently mean "no status filter", which on the storefront now means the
// default rather than everything — a different answer from the one requested.
if (statuses.length === 0) {
throw new FilterError('invalid status');
}
return statuses;
}
function parseFavoritesOnly(value: unknown): boolean {
const raw = singleValue(value, 'favorites');
if (raw === null || raw === '') {
return false;
}
if (TRUE_VALUES.includes(raw)) {
return true;
}
if (FALSE_VALUES.includes(raw)) {
return false;
}
throw new FilterError('invalid favorites');
}
// The per-field parsing lives in the helpers above; what stays here is the
// order they run in and the one rule that spans two fields. Order is
// deliberate and observable: a query wrong in two ways reports the first
// field, so moving these lines around changes which error a caller sees.
export function parseItemFilters(query: Record<string, unknown>): ItemFilters {
const categoryIds = parseCategoryIds(query.category);
const tagIds = parseTagIds(query.tags);
const minPriceCents = parsePrice(query.min_price, 'min_price');
const maxPriceCents = parsePrice(query.max_price, 'max_price');
if (minPriceCents !== null && maxPriceCents !== null && minPriceCents > maxPriceCents) {
throw new FilterError('min_price may not exceed max_price');
}
const status = parseStatus(query.status);
const favoritesOnly = parseFavoritesOnly(query.favorites);
return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly };
}
// 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;
}