fix(core): batch taxonomy term counts under D1's compound-SELECT limit - #2331
fix(core): batch taxonomy term counts under D1's compound-SELECT limit#2331MA2153 wants to merge 2 commits into
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: 40fae20 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
1 participant
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 batches collections into groups of
SQL_COMPOUND_SELECT_LIMIT(new constant next toSQL_BATCH_SIZEinutils/chunks.ts), 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.Chunk size is 5, not a more conservative 4, on purpose: 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()imposes the ceiling when a statement is prepared — where SQLite raises it too — with D1's error text.Two cases:
LIMIT + 1collections, andhandleTermListreturns them (the reported symptom);ec_*table beyond the first batch is still skipped. Mutation-checked by restricting the fallback to the first batch — the test fails as intended.Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change) — fullpackages/coreunit + integration suite, 4993 passedpnpm formathas been runAI-generated code disclosure
Screenshots / test output
Before the fix, with the D1 ceiling imposed on the test database:
After:
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.🤖 Generated with Claude Code