fix(core): batch taxonomy term counts under D1's compound-SELECT limit - #2331
Conversation
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 emdash-cms#2330 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 060499c The changes in this PR will be included in the next version bump. This PR includes changesets to release 17 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
Bot
added
area/core
size/L
review/needs-review
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-verification
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
![emdashbot[bot]](https://avatars.githubusercontent.com/in/3255304?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR correctly addresses the D1 compound-SELECT ceiling: batching UNION ALL branches at the measured limit of 5 keeps the single-query fast path for taxonomies with ≤5 collections while avoiding the D1 error for larger ones, and moving the missing-table fallback inside each batch preserves the graceful-degradation behavior. I read the changed implementation, the new test harness, callers (handleTermList/handleTermGet and the request-cached getVisibleTermCounts), SQL construction, and checked against AGENTS.md conventions.
The logic is sound, the regression tests cover the reported failure (six collections) and the missing-table-across-batches case, and the changeset is user-facing and properly scoped. The only blocker-ish issue is a comment-discipline violation: the new test block references the closed issue #2330 in both a JSDoc comment and the describe() string, which AGENTS.md explicitly forbids. Everything else is clean.
Overlapping PRsThis PR modifies files that are also changed by other open PRs:
This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
github-actions
Bot
added
review/awaiting-author
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 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
The approach is sound: batching a taxonomy's collections at D1's measured compound-SELECT limit of 5 keeps the single-query fast path for the common ≤5-collection case, while letting larger taxonomies survive. Moving the missing-table fallback per batch preserves the existing graceful-degradation behavior without one absent collection poisoning the whole count. This re-read checks the author addressed the prior review note and re-scrutinizes the changed code against AGENTS.md.
What's fixed versus the last review:
- The
#2330issue reference is gone: the JSDoc block above the newdescribewas deleted, and thedescribestring now reads visible term counts past the compound-SELECT ceiling without an issue number.
Fresh checks on the current diff:
term-counts.tschunks bySQL_COMPOUND_SELECT_LIMIT(5) and runs batches concurrently; per-group sums are commutative, so partitioning cannot change totals.- The missing-
ec_*-table fallback is now scoped to each batch, verified by the new regression test for the missing table falling beyond the first batch. handleTermListnow logs the original server-side error before returning a safe envelope; this is diagnostics only and does not leak to clients.- SQL construction still uses Kysely's tagged templates and
sql.ref(); no raw interpolation, no new identifier validation gaps. - The public render path calls
fetchVisibleTermCountsthrough the request-cachedgetVisibleTermCounts, which already keys on the sorted collection scope; the only hot-path change is that an invalid case (>5 collections) now works instead of 500ing. - Changeset is present, present-tense, and user-facing.
- No new comments reference issues/PRs/review threads; the remaining
#581references are pre-existing and were not introduced by this change.
I found nothing else that needs fixing.
github-actions
Bot
added
review/approved
khoinguyenpham04
left a comment
khoinguyenpham04
left a comment
There was a problem hiding this comment.
could we avoid applying d1’s five-collection limit to every database? i tested six collections end to end: node/sqlite goes from one query to two on both admin and public pages. d1 needs this split, but other databases do not.
plz keep the single query where the limit doesn’t apply.
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 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
The approach is still sound: reading the compound-SELECT ceiling from the adapter keeps the single-query fast path for most taxonomies and lets larger ones survive on D1, while the per-batch missing-table fallback preserves graceful degradation. The new server-side log in handleTermList stays internal and does not leak to clients.
This is a re-review after the prior approval. The core test file's #2330 reference is gone as promised, and I re-checked all changed files against AGENTS.md. I found only one consistency slip: the Cloudflare test file still labels its new describe block with #2330. A describe label is not a code comment, but it is persistent narrative that will show up in test output, and leaving it contradicts the comment-discipline cleanup you just applied in the core test file. No logic bugs, SQL safety issues, or logged-out query-count regressions remain.
Co-authored-by: emdashbot[bot] <273199577+emdashbot[bot]@users.noreply.github.com>
|
@khoinguyenpham04 done, it is now isolated per adapter. could you re-check? |
There was a problem hiding this comment.
The approach remains sound: reading the compound-SELECT ceiling from the Kysely adapter keeps the single-statement fast path for most taxonomies while splitting only when a backend like D1 declares a low limit. Batching is commutative for per-collection sums, and moving the missing-ec_*-table fallback into each batch preserves graceful degradation without letting one absent table kill the whole count. The added console.error in handleTermList logs server-side without leaking to clients.
This is a re-review after the prior round. The only outstanding item then was a #2330 reference in the Cloudflare test file's new describe block; that reference is gone — the block now reads describe("D1 compound-SELECT ceiling", ...). I also re-read all changed files against AGENTS.md conventions and checked:
- SQL safety:
collectionBranchstill validates collection slugs withvalidateIdentifierbefore interpolating them intosql.ref(ec_${collection}), and the module uses Kysely parameter binding for values. - Logged-out query counts: the change is in the admin term-list handler; no new query is added to a public route, and the request-cached public path keeps its existing behavior.
- Logic:
chunks(unique, limit)withlimit === 5yields one batch for ≤5 collections and multiple batches for more, matching the D1 measurement.Promise.allacross independent SELECT batches is safe. - Error handling:
runBatchretries only on missing-table errors and rethrows everything else; non-missing errors still surface as a 500 through the existingcatch. - Comments / changeset: no new issue references in changed hunks; the changeset is a concise, user-facing patch note.
No logic bugs, regressions, security issues, or convention violations remain in the diff. LGTM.
github-actions
Bot
added
review/approved
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 <noreply@anthropic.com>
github-actions
Bot
added
review/needs-rereview
There was a problem hiding this comment.
Re-review: LGTM
This is a clean follow-up to the prior review. The core approach—reading the D1 compound-SELECT ceiling from the Kysely adapter so only D1 pays the batching cost—remains sound.
What's fixed since the prior review:
- Comment discipline. The
#2330reference in the newdescribeblock is gone; the block is now named after what it tests, with no issue references in changed code. - Unconditional batching on non-D1 backends.
compoundSelectLimit(db)returnsnullfor any adapter that doesn't declare a ceiling, so Node/SQLite, libSQL and Postgres still issue a single statement regardless of collection count. A new test asserts the single-statement path directly. - Malformed ceiling validation.
compoundSelectLimitnow rejects0, negatives,NaN,Infinity, fractions and non-numbers, and the error names the offending adapter class. A dedicated unit test covers all the malformed cases.
What I checked:
- SQL safety. Collection slugs are still
validateIdentifier'd beforesql.ref(ec_${collection}); values stay parameter-bound. - Logged-out query counts. The split only happens on adapters that declare a ceiling, and only when a taxonomy has more collections than that ceiling. For the common ≤5-collection case on D1, it's still one query. Public render consumers go through the existing request-cached wrapper.
- Correctness of batching. Sums are per translation group and commutative;
runBatchkeeps the missing-ec_*fallback inside each batch so an absent table only degrades its batch. - D1 dialect coverage. All three D1 dialects (raw binding, session, coalescing) extend the shared
D1Adapterand exposecompoundSelectLimit = 5. - AGENTS.md conventions. The changeset correctly uses a single
emdashpatch (cloudflare is in the samefixedgroup). No new admin UI strings; no logged-out hot-path regressions; comment references are gone.
No logic bugs, security issues, regressions or convention violations remain.
|
Thanks! @MA2153 |
2 participants
What does this PR do?
fetchVisibleTermCountsbuilt oneUNION ALLbranch per declared collection. Cloudflare D1 setsSQLITE_LIMIT_COMPOUND_SELECTto 5 — SQLite's own default is 500 — so a taxonomy declaring six or more collections produced SQL the backend rejected outright. Because the counts decorate the admin term list, the failure took the whole list down:GET /_emdash/api/taxonomies/:name/termsreturned 500 and the admin rendered an empty list for a taxonomy whose terms were perfectly intact.The ceiling is 5, measured against a live D1 rather than inferred: with the same query shape and only the branch count varying, five
UNION ALLbranches compile and six fail withtoo many terms in compound SELECT. That also settles #895, where the dashboard hit this at 9 arms and was fixed by fan-out without anyone bisecting the real threshold.The fix splits the collections into batches the backend can carry, runs the batches concurrently, and sums the resulting maps — per-collection sums are commutative, so partitioning cannot change a total. The missing-
ec_*-table fallback moves per batch, so one absent table only degrades its own batch instead of the whole computation.The ceiling belongs to the adapter, not to core.
compoundSelectLimit(db)(database/dialect-helpers.ts) reads it off the Kysely adapter and returnsnullwhen the backend declares none — which is every backend except D1, so Node/SQLite, libSQL and Postgres keep counting every collection in a single statement no matter how many there are.@emdash-cms/cloudflare's three D1 dialects (raw binding, session, coalescing) share aD1Adapterbase that declarescompoundSelectLimit = 5. The limit is a property of the SQLite build behind the dialect rather than of the SQL flavour, which is whydetectDialect()'ssqlite | postgresanswer can't carry it.On D1 the batch size is the measured 5, not a more conservative 4: a taxonomy at or below the ceiling still issues exactly one query, so nothing on the logged-out render path regresses. 4 would push every 5-collection taxonomy from one query to two. The perf fixture's taxonomies declare one collection each, so the query-count snapshots are unchanged.
handleTermList's barecatchnow logs the original error. Diagnosing this required reconstructing the generated SQL by hand, because nothing anywhere recorded why the list failed.Closes #2330
Notes on the issue's other two suggestions
Deliberately not included, happy to be overruled:
isMissingTableErrorpredicate. With the ceiling gone, the only errors a wider predicate would newly swallow are genuine ones.handleTermListto counts-free terms on a count failure. It trades a loud 500 for silently wrong-looking admin data, and the failure that motivated it no longer exists. That reads like a maintainer's call about how much decoration is allowed to fail, not something to slip into a bug fix.Testing
TDD: the regression test failed with exactly the reported error at
runCountsbefore the fix.better-sqlite3 uses SQLite's upstream default of 500 and offers no way to lower it, so a query shape D1 rejects runs happily in tests.
setupTestDatabaseWithCompoundSelectLimit(limit)stands in for either kind of backend: given a number its dialect declares the ceiling the way D1's does andprepare()rejects statements past it — where SQLite raises the error too — with D1's error text; givennullit declares nothing, like better-sqlite3 itself. It also hands back every prepared statement, so a test can assert how many round trips the count took.Three cases, all at
LIMIT + 1collections:handleTermListreturns them (the reported symptom), in 2 statements;ec_*table beyond the first batch is still skipped. Mutation-checked by restricting the fallback to the first batch — the test fails as intended.On the
@emdash-cms/cloudflareside, each of the three D1 dialects is asserted to declare the ceiling on its adapter — an adapter that drops it silently sends D1 compound SELECTs it rejects.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) —packages/coreunit suite (3448 passed), plus the taxonomy, database, utils and loader suites andpackages/cloudflare'stests/dbre-run after the adapter changepnpm formathas been runAI-generated code disclosure
Screenshots / test output
Before the fix, with the D1 ceiling imposed on the test database:
After, with the ceiling now read off the adapter —
packages/coretaxonomies (unit + integration) andpackages/cloudflaretests/db:Review response
Comment discipline —
#2330in the test file (40fae20). Fixed, and the review is right on both counts. The JSDoc block above the newdescribeis deleted outright rather than reworded: it was narrative about how the bug presented, which is exactly what the commit message and this description are for. Thedescribename is nowvisible term counts past the compound-SELECT ceiling— it already said what the block covers without the number. Nothing else in the diff carries an issue reference; the#581interm-counts.ts:2is pre-existing and untouched.Targeted suite re-run after the edit:
tests/unit/taxonomies/term-counts.test.ts— 10 passed.D1's ceiling was applied to every database — six collections went from one query to two on Node/SQLite. Correct, and the measurement matches: batching was unconditional, so every backend paid for a limit only D1 has. The ceiling now comes from the adapter (
compoundSelectLimit(db), described above), so a backend that declares nothing runs the counts as a single statement at any collection count — Node/SQLite, libSQL and Postgres included — and only D1 splits. The new test asserts the single statement directly rather than trusting the shape: it fails on the previous commit.Durable Object SQLite (
DOSqlDialect,PreviewDODialect) deliberately declares nothing. D1 may well share the build, but I have no measurement for a DO the way I do for D1, and guessing here would cost the extra query on a backend that may not need it. Happy to add it if you know the answer.Why the changeset is still a single
emdashpatch. The batching and the adapter lookup both live in core;@emdash-cms/cloudflarecontributes one declared constant. Since.changeset/config.jsonputs the two packages in the samefixedgroup, listingemdashalone already versions cloudflare in lockstep — adding it to the changeset would only copy the same note into a second changelog without changing what anyone installs.compoundSelectLimitisn't validated —0/negative loop forever,NaN/fractions build invalid batches (060499c). Agreed, and the reasoning holds for every one of those values; the probe now requires a positive integer and throws naming the adapter class otherwise. The ceiling is read off a third-party Kysely adapter by duck-typing, so core has no compile-time guarantee about what it gets, and each malformed value fails silently rather than loudly:0or negative:chunks()never advancesi, sofetchVisibleTermCountshangs — worse than the 500 this PR set out to fix.2.5):slice()truncates both bounds, so batch 2 starts at index 2 after batch 1 ended there — an element lands in two batches and its count is added twice. Silently wrong numbers, no error anywhere.NaN: one empty batch, so the UNION has no terms and the statement is invalid SQL.Declared-but-not-a-number (
"5") throws too rather than falling through tonull. Silently ignoring a ceiling an adapter meant to declare is exactly the failure this PR exists to remove — it would send D1 the compound SELECTs it rejects, which is the reported bug with an extra step.Throwing is the right severity here: this is a static property of the dialect, so it is wrong on the first request or never, and it can only be fixed by the person who wrote the adapter. A silent
nullwould hide that from them.New unit test (
tests/unit/database/compound-select-limit.test.ts) coversnullfor an adapter declaring nothing, pass-through for5and1, and a throw for0,-1,NaN,Infinity,2.5,"5"andnull— 8 of its 10 cases fail on the previous commit.packages/coretaxonomies + database andpackages/cloudflaretests/dbre-run clean.🤖 Generated with Claude Code