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>
This commit is contained in:
Claude Opus 5
+45
-54
@@ -2,7 +2,9 @@
|
||||
// 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 {}
|
||||
@@ -59,11 +61,6 @@ export const STOREFRONT_DEFAULT_STATUSES: readonly ItemStatus[] = ['available',
|
||||
// shape this codebase keeps designing against.
|
||||
export const STOREFRONT_ALL_STATUSES: readonly ItemStatus[] = ['available', 'reserved', 'sold'];
|
||||
|
||||
export interface BuiltFilter {
|
||||
clauses: string[];
|
||||
params: unknown[];
|
||||
}
|
||||
|
||||
// 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.
|
||||
@@ -158,7 +155,7 @@ function parseTagIds(value: unknown): number[] {
|
||||
continue;
|
||||
}
|
||||
const id = parseId(trimmed, 'tags');
|
||||
// Duplicates would inflate the required-match count in buildItemFilterSql
|
||||
// Duplicates would inflate the required-match count in itemFilterExpressions
|
||||
// and make the filter match nothing at all.
|
||||
if (!tagIds.includes(id)) {
|
||||
tagIds.push(id);
|
||||
@@ -236,24 +233,21 @@ export function parseItemFilters(query: Record<string, unknown>): ItemFilters {
|
||||
return { categoryIds, tagIds, minPriceCents, maxPriceCents, status, favoritesOnly };
|
||||
}
|
||||
|
||||
// Returns WHERE fragments plus their parameters, with placeholders numbered
|
||||
// from `startIndex` so the caller can splice these in after its own params.
|
||||
// Composes the filter clauses as Kysely expressions.
|
||||
//
|
||||
// SECURITY INVARIANT, and it is load-bearing. Both callers splice these clauses
|
||||
// straight into query text — admin.ts as `${ADMIN_ITEM_SELECT} ${where}`, and
|
||||
// items.ts as `${PUBLIC_ITEM_SELECT} WHERE ${where}`, which is reachable
|
||||
// without signing in. So the only thing that may ever be interpolated into a
|
||||
// string pushed onto `clauses` is a placeholder index: `$${next}`, or
|
||||
// `$${next + 1}` in the tags clause. Every value goes onto `params` and is
|
||||
// bound by the driver. Interpolating a filter value here would be SQL injection
|
||||
// at both call sites, and `parseItemFilters` refusing malformed input is not
|
||||
// what prevents it — these literals would be safe with no parser at all.
|
||||
// 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.
|
||||
//
|
||||
// Stated here rather than only at the call sites because this is where the rule
|
||||
// is enforced and where a seventh clause would be added. SonarQube raised S2077
|
||||
// on the call sites and they are marked Reviewed/Safe (#180); that marking does
|
||||
// not re-raise when this file changes, so this comment and the two tests over
|
||||
// it are what stand between that edit and a live injection. See #202.
|
||||
// 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
|
||||
@@ -261,17 +255,14 @@ export function parseItemFilters(query: Record<string, unknown>): ItemFilters {
|
||||
// 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 buildItemFilterSql(
|
||||
export function itemFilterExpressions(
|
||||
eb: ItemContext,
|
||||
filters: ItemFilters,
|
||||
startIndex: number,
|
||||
favoritesCustomerId: number | null
|
||||
): BuiltFilter {
|
||||
const clauses: string[] = [];
|
||||
const params: unknown[] = [];
|
||||
let next = startIndex;
|
||||
): Expression<SqlBool>[] {
|
||||
const clauses: Expression<SqlBool>[] = [];
|
||||
|
||||
if (filters.categoryIds.length) {
|
||||
params.push(filters.categoryIds);
|
||||
// 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
|
||||
@@ -281,61 +272,61 @@ export function buildItemFilterSql(
|
||||
// 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.
|
||||
clauses.push(`i.category_id IN (
|
||||
//
|
||||
// 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($${next}::int[])
|
||||
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
|
||||
)`);
|
||||
next++;
|
||||
}
|
||||
|
||||
if (filters.tagIds.length) {
|
||||
params.push(filters.tagIds, 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(
|
||||
`(SELECT COUNT(*) FROM item_tags it
|
||||
WHERE it.item_id = i.id AND it.tag_id = ANY($${next}::int[])) = $${next + 1}`
|
||||
);
|
||||
next += 2;
|
||||
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) {
|
||||
params.push(filters.minPriceCents);
|
||||
clauses.push(`i.price_cents >= $${next}`);
|
||||
next++;
|
||||
clauses.push(eb('i.price_cents', '>=', filters.minPriceCents));
|
||||
}
|
||||
|
||||
if (filters.maxPriceCents !== null) {
|
||||
params.push(filters.maxPriceCents);
|
||||
clauses.push(`i.price_cents <= $${next}`);
|
||||
next++;
|
||||
clauses.push(eb('i.price_cents', '<=', filters.maxPriceCents));
|
||||
}
|
||||
|
||||
if (filters.status !== null) {
|
||||
params.push(filters.status);
|
||||
// ANY rather than equality, so one status and several use the same clause.
|
||||
// The ::text[] cast is explicit because `status` is a text column and the
|
||||
// driver would otherwise have to infer the array's element type.
|
||||
clauses.push(`i.status = ANY($${next}::text[])`);
|
||||
next++;
|
||||
// `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');
|
||||
}
|
||||
params.push(favoritesCustomerId);
|
||||
// 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(`EXISTS (SELECT 1 FROM favorites f WHERE f.item_id = i.id AND f.customer_id = $${next})`);
|
||||
next++;
|
||||
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, params };
|
||||
return clauses;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user