diff --git a/.changeset/fix-taxonomy-term-counts-compound-select.md b/.changeset/fix-taxonomy-term-counts-compound-select.md new file mode 100644 index 0000000000..e03f87be65 --- /dev/null +++ b/.changeset/fix-taxonomy-term-counts-compound-select.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes the taxonomy term list returning a 500 and rendering empty on Cloudflare D1 when a taxonomy declares six or more collections. diff --git a/packages/cloudflare/src/db/coalescing-d1.ts b/packages/cloudflare/src/db/coalescing-d1.ts index 748fdea52b..e88a44a497 100644 --- a/packages/cloudflare/src/db/coalescing-d1.ts +++ b/packages/cloudflare/src/db/coalescing-d1.ts @@ -21,7 +21,7 @@ import { } from "kysely"; import type { D1DialectConfig } from "kysely-d1"; -import { EmDashD1Dialect } from "./d1-dialect.js"; +import { D1Adapter, EmDashD1Dialect } from "./d1-dialect.js"; /** * Statements safe to coalesce: plain SELECTs. Deliberately conservative — @@ -284,7 +284,7 @@ export class CoalescingD1Driver implements Driver { * `executeQuery` calls — that is the whole point — so report `true`. * Transactions are rejected by the driver regardless. */ -class CoalescingD1Adapter extends SqliteAdapter { +class CoalescingD1Adapter extends D1Adapter { override get supportsMultipleConnections(): boolean { return true; } diff --git a/packages/cloudflare/src/db/d1-dialect.ts b/packages/cloudflare/src/db/d1-dialect.ts index 60549a9e00..5417421390 100644 --- a/packages/cloudflare/src/db/d1-dialect.ts +++ b/packages/cloudflare/src/db/d1-dialect.ts @@ -6,12 +6,31 @@ * d1.ts, and without pulling cloudflare:workers into test environments. */ +import type { CompoundSelectLimitedAdapter } from "emdash"; import type { DatabaseIntrospector, Kysely } from "kysely"; import { SqliteAdapter } from "kysely"; import { D1Dialect } from "kysely-d1"; import { D1Introspector } from "./d1-introspector.js"; +/** + * Terms D1 allows in one compound SELECT (`UNION ALL`, `INTERSECT`, `EXCEPT`). + * D1 sets SQLITE_LIMIT_COMPOUND_SELECT to 5 where SQLite's upstream default is + * 500; a sixth branch is rejected with "too many terms in compound SELECT". + * Measured against a live D1. + */ +export const D1_COMPOUND_SELECT_LIMIT = 5; + +/** + * Base adapter for every D1-backed dialect. Declares the compound-SELECT + * ceiling, which core reads off the adapter to split statements that would + * exceed it; a dialect that overrides `createAdapter()` without extending this + * silently sends D1 compound SELECTs it rejects. + */ +export class D1Adapter extends SqliteAdapter implements CompoundSelectLimitedAdapter { + readonly compoundSelectLimit = D1_COMPOUND_SELECT_LIMIT; +} + /** * Adapter for the raw-binding (non-session) D1 dialect only. * @@ -38,7 +57,7 @@ import { D1Introspector } from "./d1-introspector.js"; * single-in-flight op chain — see CoalescingD1Connection — but the plain * session path has no such replacement, so it must keep the mutex.) */ -class RawBindingD1Adapter extends SqliteAdapter { +class RawBindingD1Adapter extends D1Adapter { override get supportsMultipleConnections(): boolean { return true; } @@ -51,6 +70,10 @@ class RawBindingD1Adapter extends SqliteAdapter { * cross-join with pragma_table_info() that D1 doesn't allow. */ export class EmDashD1Dialect extends D1Dialect { + override createAdapter(): SqliteAdapter { + return new D1Adapter(); + } + override createIntrospector(db: Kysely): DatabaseIntrospector { return new D1Introspector(db); } diff --git a/packages/cloudflare/tests/db/d1-dialect.test.ts b/packages/cloudflare/tests/db/d1-dialect.test.ts index 6b1cccde58..cf366d0a97 100644 --- a/packages/cloudflare/tests/db/d1-dialect.test.ts +++ b/packages/cloudflare/tests/db/d1-dialect.test.ts @@ -1,7 +1,12 @@ import { CompiledQuery, Kysely } from "kysely"; import { describe, expect, it } from "vitest"; -import { EmDashD1Dialect, RawBindingD1Dialect } from "../../src/db/d1-dialect.js"; +import { CoalescingD1Dialect } from "../../src/db/coalescing-d1.js"; +import { + D1_COMPOUND_SELECT_LIMIT, + EmDashD1Dialect, + RawBindingD1Dialect, +} from "../../src/db/d1-dialect.js"; /** * Regression tests for #2040: with the default D1 config the singleton @@ -110,6 +115,22 @@ describe("EmDashD1Dialect (session path keeps the mutex)", () => { }); }); +describe("D1 compound-SELECT ceiling", () => { + it.each([ + ["raw binding", RawBindingD1Dialect], + ["session", EmDashD1Dialect], + ["coalescing", CoalescingD1Dialect], + ])("declares the ceiling on the %s adapter so core splits statements", (_name, Dialect) => { + const { database } = createMockD1(); + const adapter = new Dialect({ database }).createAdapter(); + + // Core reads the ceiling off the adapter and leaves undeclaring backends + // on a single statement, so an adapter that drops it sends D1 compound + // SELECTs it rejects outright. + expect(adapter).toHaveProperty("compoundSelectLimit", D1_COMPOUND_SELECT_LIMIT); + }); +}); + describe("D1 write results", () => { it.each([ ["raw binding", RawBindingD1Dialect], diff --git a/packages/core/src/api/handlers/taxonomies.ts b/packages/core/src/api/handlers/taxonomies.ts index 27852274c3..cdcae960e0 100644 --- a/packages/core/src/api/handlers/taxonomies.ts +++ b/packages/core/src/api/handlers/taxonomies.ts @@ -423,7 +423,8 @@ export async function handleTermList( const isHierarchical = lookup.def.hierarchical === 1; const result = isHierarchical ? buildTree(termData) : termData; return { success: true, data: { terms: result } }; - } catch { + } catch (error) { + console.error("[taxonomies] term list failed:", error); return { success: false, error: { code: "TERM_LIST_ERROR", message: "Failed to list terms" }, diff --git a/packages/core/src/database/dialect-helpers.ts b/packages/core/src/database/dialect-helpers.ts index 905629660d..41eb5e3240 100644 --- a/packages/core/src/database/dialect-helpers.ts +++ b/packages/core/src/database/dialect-helpers.ts @@ -37,6 +37,44 @@ export function isPostgres(db: Kysely): boolean { return detectDialect(db) === "postgres"; } +/** + * Declared by an adapter whose backend caps the number of terms in a compound + * SELECT (`UNION ALL`, `INTERSECT`, `EXCEPT`). SQLite's own + * SQLITE_LIMIT_COMPOUND_SELECT default is 500 — high enough that no query + * EmDash builds approaches it — but Cloudflare D1 sets it to 5 and rejects + * anything larger with "too many terms in compound SELECT". + */ +export interface CompoundSelectLimitedAdapter { + /** Maximum terms per compound SELECT. Must be a positive integer. */ + readonly compoundSelectLimit: number; +} + +/** + * The backend's compound-SELECT ceiling, or null when it has none worth + * splitting statements for. Only the adapter knows: the limit is a property of + * the SQLite build behind the dialect, not of the SQL flavour, so two "sqlite" + * dialects can answer differently. + * + * A declared ceiling must be a positive integer — callers batch by it, and + * every other value silently misbehaves rather than failing: 0 and negatives + * never advance the batch cursor, fractions overlap batches and double-count, + * NaN yields an empty batch. A malformed declaration throws here, where the + * message can name the adapter. + */ +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- accepts any Kysely instance +export function compoundSelectLimit(db: Kysely): number | null { + const adapter: object = db.getExecutor().adapter; + if (!("compoundSelectLimit" in adapter)) return null; + + const limit: unknown = adapter.compoundSelectLimit; + if (typeof limit !== "number" || !Number.isInteger(limit) || limit < 1) { + throw new Error( + `${adapter.constructor.name} declares compoundSelectLimit ${String(limit)}; it must be a positive integer.`, + ); + } + return limit; +} + /** * Default timestamp expression for column defaults. * Wrapped in parens for use in CREATE TABLE ... DEFAULT (...). diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 488fe2789f..4392244aaa 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -28,6 +28,7 @@ export type { FindManyResult, } from "./database/repositories/index.js"; export type { MediaItem, CreateMediaInput } from "./database/repositories/media.js"; +export type { CompoundSelectLimitedAdapter } from "./database/dialect-helpers.js"; // Fields export { portableText, image, file, reference } from "./fields/index.js"; diff --git a/packages/core/src/taxonomies/term-counts.ts b/packages/core/src/taxonomies/term-counts.ts index 3cbf5f513f..a7ca92a900 100644 --- a/packages/core/src/taxonomies/term-counts.ts +++ b/packages/core/src/taxonomies/term-counts.ts @@ -11,16 +11,17 @@ * visibility at query time rather than trusting a literal status value. * * The public render path is latency-sensitive on D1, so per-collection counts - * are combined into a single round-trip with UNION ALL — one query per - * taxonomy, never one per collection. + * are combined with UNION ALL — one query per taxonomy, never one per + * collection, split only where the backend caps compound-SELECT terms. */ import type { Kysely } from "kysely"; import { sql } from "kysely"; -import { buildStatusCondition } from "../database/dialect-helpers.js"; +import { buildStatusCondition, compoundSelectLimit } from "../database/dialect-helpers.js"; import type { Database } from "../database/types.js"; import { validateIdentifier } from "../database/validate.js"; +import { chunks } from "../utils/chunks.js"; import { isMissingTableError } from "../utils/db-errors.js"; interface CountRow { @@ -76,6 +77,36 @@ async function runCounts( return counts; } +function addCounts(into: Map, from: Map): void { + for (const [group, count] of from) into.set(group, (into.get(group) ?? 0) + count); +} + +/** + * Counts for one batch of collections, degrading to a query per collection + * when a declared collection has no ec_* table so the rest still contribute. + */ +async function runBatch( + db: Kysely, + taxonomyName: string, + collections: string[], +): Promise> { + try { + return await runCounts(db, taxonomyName, collections); + } catch (error) { + if (!isMissingTableError(error)) throw error; + } + + const counts = new Map(); + for (const collection of collections) { + try { + addCounts(counts, await runCounts(db, taxonomyName, [collection])); + } catch (error) { + if (!isMissingTableError(error)) throw error; + } + } + return counts; +} + /** * Count publicly-visible term assignments for one taxonomy, keyed by the * term's translation_group (what `content_taxonomies.taxonomy_id` stores). @@ -87,9 +118,15 @@ async function runCounts( * rather than a throw. * * One database round-trip for the whole taxonomy (UNION ALL across - * collections). Callers on the public render path should go through the - * request-cached wrapper in `taxonomies/index.ts` so a page rendering both the - * widget and a term detail shares one computation. + * collections). On a backend that caps compound-SELECT terms — D1 allows five + * — the branches are split into one statement per batch, since a taxonomy + * declaring more collections than that would otherwise take down every path + * that shows counts. Per-collection sums are commutative, so batching cannot + * change the total. + * + * Callers on the public render path should go through the request-cached + * wrapper in `taxonomies/index.ts` so a page rendering both the widget and a + * term detail shares one computation. */ export async function fetchVisibleTermCounts( db: Kysely, @@ -100,23 +137,11 @@ export async function fetchVisibleTermCounts( for (const collection of unique) validateIdentifier(collection, "collection slug"); if (unique.length === 0) return new Map(); - try { - return await runCounts(db, taxonomyName, unique); - } catch (error) { - if (!isMissingTableError(error)) throw error; - } + const limit = compoundSelectLimit(db); + const batched = limit === null ? [unique] : chunks(unique, limit); + const batches = await Promise.all(batched.map((batch) => runBatch(db, taxonomyName, batch))); - // A declared collection has no ec_* table — retry per collection so the - // existing tables still contribute (still scheduled-aware + deleted_at). const counts = new Map(); - for (const collection of unique) { - try { - for (const [group, count] of await runCounts(db, taxonomyName, [collection])) { - counts.set(group, (counts.get(group) ?? 0) + count); - } - } catch (error) { - if (!isMissingTableError(error)) throw error; - } - } + for (const batch of batches) addCounts(counts, batch); return counts; } diff --git a/packages/core/tests/unit/database/compound-select-limit.test.ts b/packages/core/tests/unit/database/compound-select-limit.test.ts new file mode 100644 index 0000000000..c50e21d5a2 --- /dev/null +++ b/packages/core/tests/unit/database/compound-select-limit.test.ts @@ -0,0 +1,53 @@ +import type { Kysely as KyselyType } from "kysely"; +import { Kysely, SqliteAdapter, SqliteDialect } from "kysely"; +import { describe, expect, it } from "vitest"; + +import { compoundSelectLimit } from "../../../src/database/dialect-helpers.js"; + +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- adapter probe takes any instance +type AnyDb = KyselyType; + +function stockDb(): AnyDb { + return new Kysely({ dialect: new SqliteDialect({ database: {} as never }) }); +} + +function dbDeclaring(limit: unknown): AnyDb { + class DeclaringAdapter extends SqliteAdapter { + readonly compoundSelectLimit = limit; + } + class DeclaringDialect extends SqliteDialect { + override createAdapter(): SqliteAdapter { + return new DeclaringAdapter(); + } + } + return new Kysely({ dialect: new DeclaringDialect({ database: {} as never }) }); +} + +describe("compoundSelectLimit", () => { + it("returns null for an adapter that declares no ceiling", () => { + expect(compoundSelectLimit(stockDb())).toBeNull(); + }); + + it("returns the declared ceiling", () => { + expect(compoundSelectLimit(dbDeclaring(5))).toBe(5); + expect(compoundSelectLimit(dbDeclaring(1))).toBe(1); + }); + + it.each([ + ["zero", 0], + ["negative", -1], + ["NaN", Number.NaN], + ["Infinity", Number.POSITIVE_INFINITY], + ["fractional", 2.5], + ["a numeric string", "5"], + ["null", null], + ])("throws for a %s ceiling instead of returning it", (_label, limit) => { + expect(() => compoundSelectLimit(dbDeclaring(limit))).toThrow( + /compoundSelectLimit.*positive integer/s, + ); + }); + + it("names the offending adapter so the dialect can be found", () => { + expect(() => compoundSelectLimit(dbDeclaring(0))).toThrow(/DeclaringAdapter/); + }); +}); diff --git a/packages/core/tests/unit/taxonomies/term-counts.test.ts b/packages/core/tests/unit/taxonomies/term-counts.test.ts index 55704d739d..b8f6e69634 100644 --- a/packages/core/tests/unit/taxonomies/term-counts.test.ts +++ b/packages/core/tests/unit/taxonomies/term-counts.test.ts @@ -6,19 +6,25 @@ * declared collections. */ +import type { Kysely } from "kysely"; import { sql } from "kysely"; import { ulid } from "ulidx"; -import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { handleTermGet, handleTermList } from "../../../src/api/handlers/taxonomies.js"; import { ContentRepository } from "../../../src/database/repositories/content.js"; import { TaxonomyRepository } from "../../../src/database/repositories/taxonomy.js"; +import type { Database as DatabaseSchema } from "../../../src/database/types.js"; import { runWithContext } from "../../../src/request-context.js"; +import { SchemaRegistry } from "../../../src/schema/registry.js"; import { fetchVisibleTermCounts } from "../../../src/taxonomies/term-counts.js"; import { + D1_COMPOUND_SELECT_LIMIT, describeEachDialect, setupForDialectWithCollections, + setupTestDatabaseWithCompoundSelectLimit, teardownForDialect, + teardownTestDatabase, type DialectTestContext, } from "../../utils/test-db.js"; @@ -325,3 +331,101 @@ describeEachDialect("visible term counts (#581)", (dialect) => { expect(counts.size).toBe(0); }); }); + +describe("visible term counts past the compound-SELECT ceiling", () => { + let db: Kysely; + let statements: string[]; + + /** Back the test by a database that declares `limit` (null: no ceiling). */ + async function useDatabase(limit: number | null): Promise { + ({ db, statements } = await setupTestDatabaseWithCompoundSelectLimit(limit)); + } + + /** How many statements the count query took; its subquery alias is unique to it. */ + function countStatements(): number { + return statements.filter((source) => source.includes("per_collection")).length; + } + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + /** + * Declare `collections` on a taxonomy, create a table and one published, + * term-tagged entry for each of `existing`, and return the term. + */ + async function seedTaxonomy(collections: string[], existing: string[]) { + const registry = new SchemaRegistry(db); + const contentRepo = new ContentRepository(db); + const taxRepo = new TaxonomyRepository(db); + + for (const slug of existing) { + await registry.createCollection({ slug, label: slug, labelSingular: slug }); + await registry.createField(slug, { slug: "title", label: "Title", type: "string" }); + } + + const defId = ulid(); + await db + .insertInto("_emdash_taxonomy_defs") + .values({ + id: defId, + name: "topic", + label: "Topics", + label_singular: null, + hierarchical: 0, + collections: JSON.stringify(collections), + locale: "en", + translation_group: defId, + }) + .execute(); + + const term = await taxRepo.create({ name: "topic", slug: "science", label: "Science" }); + for (const slug of existing) { + const entry = await contentRepo.create({ + type: slug, + slug: `${slug}-entry`, + status: "published", + data: { title: slug }, + }); + await taxRepo.attachToEntry(slug, entry.id, term.id); + } + return term; + } + + function collectionSlugs(count: number): string[] { + return Array.from({ length: count }, (_, i) => `coll_${String(i)}`); + } + + it("aggregates every declared collection when there are more than one statement can carry", async () => { + await useDatabase(D1_COMPOUND_SELECT_LIMIT); + const slugs = collectionSlugs(D1_COMPOUND_SELECT_LIMIT + 1); + const term = await seedTaxonomy(slugs, slugs); + + const counts = await fetchVisibleTermCounts(db, "topic", slugs); + expect(counts.get(term.translationGroup ?? term.id)).toBe(slugs.length); + expect(countStatements()).toBe(2); + + const list = await handleTermList(db, "topic"); + if (!list.success) throw new Error(list.error.code); + expect(list.data.terms[0]!.count).toBe(slugs.length); + }); + + it("still skips a missing ec_* table when it falls beyond the first batch", async () => { + await useDatabase(D1_COMPOUND_SELECT_LIMIT); + const existing = collectionSlugs(D1_COMPOUND_SELECT_LIMIT); + const term = await seedTaxonomy([...existing, "ghost"], existing); + + const counts = await fetchVisibleTermCounts(db, "topic", [...existing, "ghost"]); + expect(counts.get(term.translationGroup ?? term.id)).toBe(existing.length); + }); + + it("takes a single statement on a backend that declares no ceiling", async () => { + await useDatabase(null); + const slugs = collectionSlugs(D1_COMPOUND_SELECT_LIMIT + 1); + const term = await seedTaxonomy(slugs, slugs); + + const counts = await fetchVisibleTermCounts(db, "topic", slugs); + expect(counts.get(term.translationGroup ?? term.id)).toBe(slugs.length); + expect(countStatements()).toBe(1); + }); +}); diff --git a/packages/core/tests/utils/test-db.ts b/packages/core/tests/utils/test-db.ts index 12c101fbef..c5dace36a9 100644 --- a/packages/core/tests/utils/test-db.ts +++ b/packages/core/tests/utils/test-db.ts @@ -1,7 +1,8 @@ import { randomUUID } from "node:crypto"; import Database from "better-sqlite3"; -import { Kysely, SqliteDialect } from "kysely"; +import type { SqliteDialectConfig } from "kysely"; +import { Kysely, SqliteAdapter, SqliteDialect } from "kysely"; import { Pool } from "pg"; import { describe } from "vitest"; @@ -121,6 +122,73 @@ export async function teardownTestDatabase(db: Kysely): Promise< await db.destroy(); } +/** + * Number of terms Cloudflare D1 allows in a compound SELECT + * (SQLITE_LIMIT_COMPOUND_SELECT). Measured against a real D1: five + * `UNION ALL` branches compile, six are rejected. + */ +export const D1_COMPOUND_SELECT_LIMIT = 5; + +class LimitedCompoundSelectAdapter extends SqliteAdapter { + constructor(readonly compoundSelectLimit: number) { + super(); + } +} + +class LimitedCompoundSelectDialect extends SqliteDialect { + readonly #limit: number; + + constructor(config: SqliteDialectConfig, limit: number) { + super(config); + this.#limit = limit; + } + + override createAdapter(): SqliteAdapter { + return new LimitedCompoundSelectAdapter(this.#limit); + } +} + +export interface CompoundSelectTestDatabase { + db: Kysely; + /** Every statement prepared against the database, in order. */ + statements: string[]; +} + +/** + * Test database standing in for a backend with — or without — a + * compound-SELECT ceiling. + * + * better-sqlite3 uses SQLite's upstream default of 500 and offers no way to + * lower it, so query shapes that D1 rejects run happily in tests. Pass a + * number and the dialect declares the ceiling the way the D1 dialect does, + * while prepare() rejects statements past it — where SQLite itself raises the + * error — with D1's error text, so code that inspects the message behaves the + * same. Pass null for a backend that imposes no ceiling. + */ +export async function setupTestDatabaseWithCompoundSelectLimit( + limit: number | null = D1_COMPOUND_SELECT_LIMIT, +): Promise { + resetSchemaCachesForTests(); + const sqlite = new Database(":memory:"); + const statements: string[] = []; + const prepare = sqlite.prepare.bind(sqlite); + sqlite.prepare = ((source: string) => { + statements.push(source); + const terms = source.split(/\b(?:UNION|INTERSECT|EXCEPT)\b/i).length; + if (limit !== null && terms > limit) { + throw new Error("too many terms in compound SELECT: SQLITE_ERROR"); + } + return prepare(source); + }) as typeof sqlite.prepare; + + const config = { database: sqlite }; + const dialect = + limit === null ? new SqliteDialect(config) : new LimitedCompoundSelectDialect(config, limit); + const db = new Kysely({ dialect }); + await runMigrations(db); + return { db, statements }; +} + // --------------------------------------------------------------------------- // PostgreSQL helpers // ---------------------------------------------------------------------------