search: don't assume a title column exists in searchable collections#1246
search: don't assume a title column exists in searchable collections#1246mvanhorn wants to merge 3 commits into
Conversation
The search query and suggestion SQL hardcoded c.title, so searching a collection whose schema has no title field failed with D1_ERROR: no such column: title and broke the whole multi-collection search. Detect whether the collection actually has a title field (via the fields registry, with identifier validation) and select it only when present: NULL in the main search projection and COALESCE(slug, id) for suggestions, dropping the title IS NOT NULL filter when there is no title field. Adds integration tests with a title-less searchable collection. Closes emdash-cms#1178
🦋 Changeset detectedLatest commit: c04d512 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/blocks
@emdash-cms/cloudflare
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@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-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Query-count snapshot changes4 routes changed, total Δ +4 queries. SQLite
D1
Comparing snapshot files between base and head. Updated automatically on each push. |
There was a problem hiding this comment.
Review: fix/1178-mcp-search-d1-column
Approach judgment: This is the right fix for the right problem. Search and suggestions assumed every collection had a title column, which broke multi-collection search when any searchable collection lacked one. Detecting the field via _emdash_fields and conditionally selecting c.title, NULL, or COALESCE(c.slug, c.id) is a sound, minimally invasive approach.
What I checked:
- Full diff of
packages/core/src/search/query.tsand the two new integration tests. - SQL safety: no raw interpolation of variables; identifiers are validated with
validateIdentifier(). The newcollectionHasFieldhelper uses the Kysely query builder correctly. - Content-table conventions: locale filters are preserved,
deleted_atchecks are preserved, no locale filtering issues. - Cross-cutting concerns: verified there are no other hardcoded
c.titlereferences in search/MCP code. - Edge cases:
titlemay be null even when the field exists, sorow.title ?? undefinedremains correct. TheCOALESCE(c.slug, c.id)fallback in suggestions guarantees a non-null string sinceidis a required primary key. - Tests: both the MCP search and suggestions integration tests cover titleless collections in multi-collection scenarios, which directly reproduces the reported bug.
- Changeset: present (
emdash: patch).
Headline conclusion: The implementation is clean, safe, and well-tested. I found no logic bugs, regressions, security issues, or AGENTS.md convention violations.
One non-blocking observation: collectionHasField adds an extra query per collection in loops that already query _emdash_collections for search config. In the existing architecture this is acceptable, but future work could batch or cache field metadata if search latency becomes a concern.
|
You could avoid the extra query by updating If you want to improve things further in a follow-up, you could put all of the metadata queries (getSearchConfig, getSearchableFields, ftsTableExists at least) into a new |
ascorbic
left a comment
There was a problem hiding this comment.
Avoid the extra hot-path query
What does this PR do?
Search and suggestions hardcoded
c.titlein the generated SQL, so searching a collection whose schema has notitlefield failed withD1_ERROR: no such column: titleand broke multi-collection search entirely.The query now detects whether the collection actually defines a
titlefield (via the fields registry, with identifier validation) and only selects it when present:NULLin the main search projection andCOALESCE(slug, id)for suggestions, dropping thetitle IS NOT NULLfilter when there is no title field.Closes #1178
Type of change
Checklist
pnpm typecheckpassespnpm lintpasses (changed files)pnpm testpasses (targeted tests for my change: search + mcp/search, 13 passing)pnpm formathas been runemdash: patch)AI-generated code disclosure
Screenshots / test output