Files
redefined-designs/backend/src/itemFilters.ts
T
bermudalambandClaude Opus 5 528e49bf49
Linting / lint (pull_request) Successful in 3m16s
SonarQube Analysis / sonarqube (pull_request) Successful in 24m19s
fix(tests): close the route guard's factory hole and two regex warnings (#307)
Three of the items on the cleanup issue, and the first is the one that mattered.

routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed.

Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone.

The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added.

The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it.

An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-09-04 17:58:43 -05:00

343 lines
14 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.
//
// The empty list is spelled out rather than left to `in`, which would emit
// `in ()` — a Postgres syntax error, where `= ANY` on an empty array was
// valid and matched nothing. Unreachable through parseItemFilters, which
// refuses a list that names nothing, but the obvious alternative is wrong
// in the opposite direction: dropping the clause entirely would make an
// empty status filter match *every* status, where the behaviour this
// replaced matched none. See #307.
clauses.push(
filters.status.length ? eb('i.status', 'in', filters.status) : sql<SqlBool>`false`
);
}
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;
}