From 5312e0fa4e155eea59497f5e91be723e279c7a97 Mon Sep 17 00:00:00 2001 From: Malloo <26630797+MA2153@users.noreply.github.com> Date: Mon, 3 Aug 2026 18:11:03 +0300 Subject: [PATCH 1/5] fix(core): batch taxonomy term counts under D1's compound-SELECT limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fetchVisibleTermCounts built one UNION ALL branch per declared collection. Cloudflare D1 sets SQLITE_LIMIT_COMPOUND_SELECT to 5 (SQLite's own default is 500), so a taxonomy declaring six or more collections produced SQL the backend rejected outright. The counts decorate the admin term list, so the whole list 500'd and rendered empty while the terms themselves were intact. Batch the branches into groups of SQL_COMPOUND_SELECT_LIMIT and sum the resulting maps. Taxonomies at or below the ceiling still issue exactly one query, so nothing on the logged-out render path regresses. The missing-ec_*- table fallback moves per batch so an absent table only degrades its own batch. The ceiling was measured against a live D1: five UNION ALL branches compile, six fail with "too many terms in compound SELECT". better-sqlite3 offers no way to lower the limit, so the regression test imposes it at prepare() — where SQLite raises it too — with D1's error text. handleTermList's bare catch now logs the original error; reconstructing the generated SQL by hand was the only way to diagnose this. Closes #2330 Co-Authored-By: Claude Opus 5 --- ...ix-taxonomy-term-counts-compound-select.md | 5 + packages/core/src/api/handlers/taxonomies.ts | 3 +- packages/core/src/taxonomies/term-counts.ts | 67 +++++++++----- packages/core/src/utils/chunks.ts | 8 ++ .../tests/unit/taxonomies/term-counts.test.ts | 92 ++++++++++++++++++- packages/core/tests/utils/test-db.ts | 34 +++++++ 6 files changed, 186 insertions(+), 23 deletions(-) create mode 100644 .changeset/fix-taxonomy-term-counts-compound-select.md 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/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/taxonomies/term-counts.ts b/packages/core/src/taxonomies/term-counts.ts index 3cbf5f513f..61df3d5386 100644 --- a/packages/core/src/taxonomies/term-counts.ts +++ b/packages/core/src/taxonomies/term-counts.ts @@ -11,8 +11,8 @@ * 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, up to the backend's compound-SELECT ceiling. */ import type { Kysely } from "kysely"; @@ -21,6 +21,7 @@ import { sql } from "kysely"; import { buildStatusCondition } from "../database/dialect-helpers.js"; import type { Database } from "../database/types.js"; import { validateIdentifier } from "../database/validate.js"; +import { chunks, SQL_COMPOUND_SELECT_LIMIT } 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), or one per SQL_COMPOUND_SELECT_LIMIT collections beyond the + * point where a single statement can carry them all — D1 rejects a compound + * SELECT with more terms than that, so a taxonomy declaring enough + * collections 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 batches = await Promise.all( + chunks(unique, SQL_COMPOUND_SELECT_LIMIT).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/src/utils/chunks.ts b/packages/core/src/utils/chunks.ts index 9ff9f0f408..30422e9f8c 100644 --- a/packages/core/src/utils/chunks.ts +++ b/packages/core/src/utils/chunks.ts @@ -15,3 +15,11 @@ export function chunks(arr: T[], size: number): T[][] { /** Conservative default chunk size for SQL IN clauses (well within D1's limit). */ export const SQL_BATCH_SIZE = 50; + +/** + * Maximum number of terms one compound SELECT (`UNION ALL`, `INTERSECT`, + * `EXCEPT`) may have. SQLite's own default is 500, but Cloudflare D1 sets + * SQLITE_LIMIT_COMPOUND_SELECT to 5 and rejects anything larger with + * "too many terms in compound SELECT". Split into separate statements past it. + */ +export const SQL_COMPOUND_SELECT_LIMIT = 5; diff --git a/packages/core/tests/unit/taxonomies/term-counts.test.ts b/packages/core/tests/unit/taxonomies/term-counts.test.ts index 55704d739d..228bc27698 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,87 @@ describeEachDialect("visible term counts (#581)", (dialect) => { expect(counts.size).toBe(0); }); }); + +/** + * #2330: counts were built as one UNION ALL branch per declared collection. + * Past D1's compound-SELECT ceiling the statement is rejected outright, and + * because the counts decorate the admin term list, the whole list 500s — the + * taxonomy becomes unmanageable while its terms are perfectly intact. + */ +describe("visible term counts past the compound-SELECT ceiling (#2330)", () => { + let db: Kysely; + + beforeEach(async () => { + db = await setupTestDatabaseWithCompoundSelectLimit(); + }); + + 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 () => { + 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); + + 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 () => { + 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); + }); +}); diff --git a/packages/core/tests/utils/test-db.ts b/packages/core/tests/utils/test-db.ts index 12c101fbef..88e826ddfc 100644 --- a/packages/core/tests/utils/test-db.ts +++ b/packages/core/tests/utils/test-db.ts @@ -121,6 +121,40 @@ 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; + +/** + * Test database that enforces D1's 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. The ceiling + * is imposed when a statement is prepared — where SQLite itself raises it — + * with D1's error text, so code that inspects the message behaves the same. + */ +export async function setupTestDatabaseWithCompoundSelectLimit( + limit = D1_COMPOUND_SELECT_LIMIT, +): Promise> { + resetSchemaCachesForTests(); + const sqlite = new Database(":memory:"); + const prepare = sqlite.prepare.bind(sqlite); + sqlite.prepare = ((source: string) => { + const terms = source.split(/\b(?:UNION|INTERSECT|EXCEPT)\b/i).length; + if (terms > limit) { + throw new Error("too many terms in compound SELECT: SQLITE_ERROR"); + } + return prepare(source); + }) as typeof sqlite.prepare; + + const db = new Kysely({ dialect: new SqliteDialect({ database: sqlite }) }); + await runMigrations(db); + return db; +} + // --------------------------------------------------------------------------- // PostgreSQL helpers // --------------------------------------------------------------------------- From 40fae2000e53d7559b40d4d9e4b601ae6202798b Mon Sep 17 00:00:00 2001 From: Malloo <26630797+MA2153@users.noreply.github.com> Date: Mon, 3 Aug 2026 22:37:40 +0300 Subject: [PATCH 2/5] fix(core): drop issue references from term-count test block Comments are evergreen; the compound-SELECT background belongs in the commit message and PR description, not in the test file. Co-Authored-By: Claude Opus 5 --- packages/core/tests/unit/taxonomies/term-counts.test.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/core/tests/unit/taxonomies/term-counts.test.ts b/packages/core/tests/unit/taxonomies/term-counts.test.ts index 228bc27698..b53293536c 100644 --- a/packages/core/tests/unit/taxonomies/term-counts.test.ts +++ b/packages/core/tests/unit/taxonomies/term-counts.test.ts @@ -332,13 +332,7 @@ describeEachDialect("visible term counts (#581)", (dialect) => { }); }); -/** - * #2330: counts were built as one UNION ALL branch per declared collection. - * Past D1's compound-SELECT ceiling the statement is rejected outright, and - * because the counts decorate the admin term list, the whole list 500s — the - * taxonomy becomes unmanageable while its terms are perfectly intact. - */ -describe("visible term counts past the compound-SELECT ceiling (#2330)", () => { +describe("visible term counts past the compound-SELECT ceiling", () => { let db: Kysely; beforeEach(async () => { From f00555b6d2116bf97a59db50a041ec03ccd7ca44 Mon Sep 17 00:00:00 2001 From: Malloo <26630797+MA2153@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:10:48 +0300 Subject: [PATCH 3/5] fix(core): scope the compound-SELECT split to backends that cap it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batching the term-count branches was unconditional, so every database paid for a limit only D1 has: a taxonomy with six collections went from one query to two on Node/SQLite and Postgres, on both the admin and public paths. The ceiling now comes from the Kysely adapter. compoundSelectLimit(db) reads it off the adapter and returns null when none is declared, which is every backend except D1 — those keep counting all collections in one statement at any collection count. The three D1 dialects share a D1Adapter base declaring compoundSelectLimit = 5. The limit is a property of the SQLite build behind the dialect rather than of the SQL flavour, so detectDialect()'s sqlite | postgres answer cannot carry it; SQL_COMPOUND_SELECT_LIMIT is gone from utils/chunks.ts. setupTestDatabaseWithCompoundSelectLimit() now models either kind of backend and records prepared statements, so the new test asserts the single statement directly: it fails against the previous commit. Co-Authored-By: Claude Opus 5 --- packages/cloudflare/src/db/coalescing-d1.ts | 4 +- packages/cloudflare/src/db/d1-dialect.ts | 25 ++++++++- .../cloudflare/tests/db/d1-dialect.test.ts | 23 +++++++- packages/core/src/database/dialect-helpers.ts | 26 +++++++++ packages/core/src/index.ts | 1 + packages/core/src/taxonomies/term-counts.ts | 22 ++++---- packages/core/src/utils/chunks.ts | 8 --- .../tests/unit/taxonomies/term-counts.test.ts | 26 +++++++-- packages/core/tests/utils/test-db.ts | 54 +++++++++++++++---- 9 files changed, 153 insertions(+), 36 deletions(-) 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..b2d56d8f11 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 (#2330)", () => { + 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/database/dialect-helpers.ts b/packages/core/src/database/dialect-helpers.ts index 905629660d..0cb7fe93db 100644 --- a/packages/core/src/database/dialect-helpers.ts +++ b/packages/core/src/database/dialect-helpers.ts @@ -37,6 +37,32 @@ 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 { + 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. + */ +// 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 && typeof adapter.compoundSelectLimit === "number") { + return adapter.compoundSelectLimit; + } + return null; +} + /** * 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 61df3d5386..a7ca92a900 100644 --- a/packages/core/src/taxonomies/term-counts.ts +++ b/packages/core/src/taxonomies/term-counts.ts @@ -12,16 +12,16 @@ * * The public render path is latency-sensitive on D1, so per-collection counts * are combined with UNION ALL — one query per taxonomy, never one per - * collection, up to the backend's compound-SELECT ceiling. + * 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, SQL_COMPOUND_SELECT_LIMIT } from "../utils/chunks.js"; +import { chunks } from "../utils/chunks.js"; import { isMissingTableError } from "../utils/db-errors.js"; interface CountRow { @@ -118,11 +118,11 @@ async function runBatch( * rather than a throw. * * One database round-trip for the whole taxonomy (UNION ALL across - * collections), or one per SQL_COMPOUND_SELECT_LIMIT collections beyond the - * point where a single statement can carry them all — D1 rejects a compound - * SELECT with more terms than that, so a taxonomy declaring enough - * collections would otherwise take down every path that shows counts. - * Per-collection sums are commutative, so batching cannot change the total. + * 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 @@ -137,9 +137,9 @@ export async function fetchVisibleTermCounts( for (const collection of unique) validateIdentifier(collection, "collection slug"); if (unique.length === 0) return new Map(); - const batches = await Promise.all( - chunks(unique, SQL_COMPOUND_SELECT_LIMIT).map((batch) => runBatch(db, taxonomyName, batch)), - ); + const limit = compoundSelectLimit(db); + const batched = limit === null ? [unique] : chunks(unique, limit); + const batches = await Promise.all(batched.map((batch) => runBatch(db, taxonomyName, batch))); const counts = new Map(); for (const batch of batches) addCounts(counts, batch); diff --git a/packages/core/src/utils/chunks.ts b/packages/core/src/utils/chunks.ts index 30422e9f8c..9ff9f0f408 100644 --- a/packages/core/src/utils/chunks.ts +++ b/packages/core/src/utils/chunks.ts @@ -15,11 +15,3 @@ export function chunks(arr: T[], size: number): T[][] { /** Conservative default chunk size for SQL IN clauses (well within D1's limit). */ export const SQL_BATCH_SIZE = 50; - -/** - * Maximum number of terms one compound SELECT (`UNION ALL`, `INTERSECT`, - * `EXCEPT`) may have. SQLite's own default is 500, but Cloudflare D1 sets - * SQLITE_LIMIT_COMPOUND_SELECT to 5 and rejects anything larger with - * "too many terms in compound SELECT". Split into separate statements past it. - */ -export const SQL_COMPOUND_SELECT_LIMIT = 5; diff --git a/packages/core/tests/unit/taxonomies/term-counts.test.ts b/packages/core/tests/unit/taxonomies/term-counts.test.ts index b53293536c..b8f6e69634 100644 --- a/packages/core/tests/unit/taxonomies/term-counts.test.ts +++ b/packages/core/tests/unit/taxonomies/term-counts.test.ts @@ -334,10 +334,17 @@ describeEachDialect("visible term counts (#581)", (dialect) => { describe("visible term counts past the compound-SELECT ceiling", () => { let db: Kysely; + let statements: string[]; - beforeEach(async () => { - db = await setupTestDatabaseWithCompoundSelectLimit(); - }); + /** 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); @@ -390,11 +397,13 @@ describe("visible term counts past the compound-SELECT ceiling", () => { } 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); @@ -402,10 +411,21 @@ describe("visible term counts past the compound-SELECT ceiling", () => { }); 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 88e826ddfc..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"; @@ -128,31 +129,64 @@ export async function teardownTestDatabase(db: Kysely): Promise< */ 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 that enforces D1's compound-SELECT ceiling. + * 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. The ceiling - * is imposed when a statement is prepared — where SQLite itself raises it — - * with D1's error text, so code that inspects the message behaves the same. + * 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 = D1_COMPOUND_SELECT_LIMIT, -): Promise> { + 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 (terms > limit) { + if (limit !== null && terms > limit) { throw new Error("too many terms in compound SELECT: SQLITE_ERROR"); } return prepare(source); }) as typeof sqlite.prepare; - const db = new Kysely({ dialect: new SqliteDialect({ database: sqlite }) }); + 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; + return { db, statements }; } // --------------------------------------------------------------------------- From 6ea6f77c825fd326b59765655219d35e3ae63650 Mon Sep 17 00:00:00 2001 From: MA2153 <26630797+MA2153@users.noreply.github.com> Date: Wed, 5 Aug 2026 15:21:00 +0300 Subject: [PATCH 4/5] Update packages/cloudflare/tests/db/d1-dialect.test.ts Co-authored-by: emdashbot[bot] <273199577+emdashbot[bot]@users.noreply.github.com> --- packages/cloudflare/tests/db/d1-dialect.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cloudflare/tests/db/d1-dialect.test.ts b/packages/cloudflare/tests/db/d1-dialect.test.ts index b2d56d8f11..cf366d0a97 100644 --- a/packages/cloudflare/tests/db/d1-dialect.test.ts +++ b/packages/cloudflare/tests/db/d1-dialect.test.ts @@ -115,7 +115,7 @@ describe("EmDashD1Dialect (session path keeps the mutex)", () => { }); }); -describe("D1 compound-SELECT ceiling (#2330)", () => { +describe("D1 compound-SELECT ceiling", () => { it.each([ ["raw binding", RawBindingD1Dialect], ["session", EmDashD1Dialect], From 060499c923df18905988d75bb049d7594a561150 Mon Sep 17 00:00:00 2001 From: Malloo <26630797+MA2153@users.noreply.github.com> Date: Wed, 5 Aug 2026 16:27:39 +0300 Subject: [PATCH 5/5] fix(core): reject a malformed compoundSelectLimit at the adapter probe The ceiling is read off a third-party Kysely adapter by duck-typing, so core cannot assume it is well-formed. Only a positive integer batches correctly: chunks() never advances its cursor on 0 or a negative, so fetchVisibleTermCounts hangs instead of erroring; a fractional limit truncates on the second slice bound and repeats an element across batches, double-counting it; NaN produces a single empty batch and a UNION with no terms. Every one of those fails silently or not at all, which is the failure mode this branch exists to remove. A malformed declaration now throws where the message can name the adapter class. Co-Authored-By: Claude Opus 5 --- packages/core/src/database/dialect-helpers.ts | 18 +++++-- .../database/compound-select-limit.test.ts | 53 +++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) create mode 100644 packages/core/tests/unit/database/compound-select-limit.test.ts diff --git a/packages/core/src/database/dialect-helpers.ts b/packages/core/src/database/dialect-helpers.ts index 0cb7fe93db..41eb5e3240 100644 --- a/packages/core/src/database/dialect-helpers.ts +++ b/packages/core/src/database/dialect-helpers.ts @@ -45,6 +45,7 @@ export function isPostgres(db: Kysely): boolean { * 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; } @@ -53,14 +54,25 @@ export interface CompoundSelectLimitedAdapter { * 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 && typeof adapter.compoundSelectLimit === "number") { - return adapter.compoundSelectLimit; + 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 null; + return limit; } /** 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/); + }); +});