feat(content): filter indexed custom fields - #2213
Conversation
🦋 Changeset detectedLatest commit: d64fc47 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
review/needs-review
Scope checkThis PR changes 1,474 lines across 47 files. Large PRs are harder to review and more likely to be closed without review. If this scope is intentional, no action needed. A maintainer will review it. If not, please consider splitting this into smaller PRs. See CONTRIBUTING.md for contribution guidelines. |
@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 adds a coherent, architecture-aligned implementation of indexed custom-field filtering: it stores filterable fields as real columns, validates identifiers before they reach SQL, parameterizes values, and adds coverage across repository, handler, client, plugin, and MCP surfaces. The sorting/index plumbing and admin list-column work are related but distinct concerns, so the main judgement call is whether the PR should be split or retitled.
What I checked:
- SQL safety: dynamic column refs use
sql.ref(...)aftervalidateIdentifier, filter values are parameterized,inlists are built withsql.join, and the partial expression index design is correct. No injection vectors found. - Authorization/logged-out paths:
fieldFiltersflows throughhandleContentListand plugincontent:read, both authenticated; public loader filters via the pre-existing loader path are untouched. No new anonymous hot-path queries. - Locale filtering: content-table list queries already filter by
localewhen supplied; indexed field values are per-row, so this stays correct. - Index discipline:
createFieldIndexmatches thedeleted_at IS NULLfilter used by list/count queries. - API envelope: responses remain
{ items, total, nextCursor? }.
Blocking issues found: none. Issues to fix before merge:
- A new repository test asserts
new Set(...).toHaveLength(5), which will fail because Sets havesize, notlength. - The new admin list-column formatter emits hard-coded
"—","✓", and", ", and formats dates with the browser default locale instead of Lingui — violating the AGENTS.md localization rule.
Lower-priority suggestions:
- The PR title says "filtering" but the diff ships three separate changesets/features (filtering, sorting/indexes, list columns). Consider splitting or updating the title/description per scope discipline.
- Seed validation checks
admin.listColumnsshape but not that the slugs are declared fields in the collection. createFieldIndexcould be idempotent withIF NOT EXISTSto survive partial creates or re-runs.
Overall direction is solid; the test and localization items should be addressed.
| fields[field.slug] = entry; | ||
| } | ||
|
|
||
| const configuredListColumns = collection.admin?.listColumns ?? []; |
There was a problem hiding this comment.
[suggestion] The PR title and main changeset are about indexed custom-field filtering, but this hunk and the related schema/admin code introduce a third concern: collection-configured admin list columns (and the diff also ships a separate sorting/index feature). Per AGENTS.md scope discipline, unrelated/semi-related features should not ride along on a single PR. Consider either splitting this into focused PRs or updating the title/description/changeset set so reviewers can evaluate each feature on its own.
There was a problem hiding this comment.
This PR sits at the top of a four-PR stack, so its diff necessarily contains 2194 and 2212. Each capability is reviewed in its own PR; the stack order is in the description. Leaving this thread open since it is a judgement call rather than something the code can resolve.
80b9da5 to
abd8982
Compare
github-actions
Bot
added
review/needs-rereview
e318139 to
e00e9db
Compare
e00e9db to
3947dda
Compare
3947dda to
1afb45e
Compare
51754bf to
754981b
Compare
754981b to
3c4bbf5
Compare
There was a problem hiding this comment.
This re-review focuses on the new indexed custom-field filtering path in PR #2213. The approach is sound and fits EmDash’s architecture: filters are validated in the REST query schema, resolved against _emdash_fields with the indexed flag, applied as parameterized SQL through sql.ref()/Kysely fragments, and flow through the existing authenticated content:read route and plugin content:read capability. Test coverage for scalar, in, range, null, budget, injection, cursor pagination, and query-plan index usage is comprehensive.
What I checked:
- SQL safety: dynamic column identifiers are validated with
validateIdentifierand emitted viasql.ref(...); values,INlists, and cursor tuple members are parameterized. - Authorization:
fieldFiltersis only accepted by authenticated handlers (content:readroute, plugincontent:read) and cannot override the subscriberstatus=publishedguard. - Locale filtering: field filters apply per row;
localefiltering infindManyis unchanged, so translated indexed fields are still filtered within the requested locale. - Cursor pagination: null/ present tuple ordering and row-value cursor comparisons line up with the
(isPresent, field, id)index key. - API envelope: responses still return
{ items, total, nextCursor? }. - Logged-out hot path: no new anonymous-route queries are introduced by the filtering feature.
- AGENTS.md conventions: code is formatted with tabs, changesets are present, and no reviewer-facing comments were added.
One code regression remains: resolveFieldFilters resolves field metadata before confirming the collection exists, so a request to a non-existent collection that includes fieldFilters returns a misleading VALIDATION_ERROR instead of the COLLECTION_NOT_FOUND returned for the same request without filters. This breaks the handler’s existing error contract and should be aligned (for example, by checking _emdash_collections existence when metadata is empty, or by validating the collection before resolving filters).
One non-code process item is also still open: the author has explicitly noted that the Discussion linked in the PR description covers admin.listColumns rather than indexed filtering, so the maintainer-approved Discussion requirement for this feature is still pending. That is outside the code review but affects whether the feature should merge.
The prior review’s two concrete issues (test Set.size assertions and localized admin custom-cell formatting) are resolved.
|
|
||
| const rows = await this.db | ||
| .selectFrom("_emdash_fields as field") | ||
| .innerJoin("_emdash_collections as collection", "collection.id", "field.collection_id") | ||
| .where("collection.slug", "=", type) | ||
| .where("field.slug", "in", fields) | ||
| .where("field.indexed", "=", 1) | ||
| .select(["field.slug", "field.type"]) | ||
| .execute(); | ||
| const metadata = new Map(rows.map((row) => [row.slug, row.type as FieldType])); | ||
|
|
||
| const normalized = fields.map((field) => { | ||
| const fieldType = metadata.get(field); | ||
| if (!fieldType || !isIndexableFieldType(fieldType)) { | ||
| throw new EmDashValidationError( | ||
| `Cannot filter by field "${field}". Custom fields must be indexed before filtering.`, |
There was a problem hiding this comment.
[needs fixing] resolveFieldFilters fetches field metadata from _emdash_collections/_emdash_fields before the backing ec_* table is touched. If the collection does not exist, the join returns no rows and the loop throws Cannot filter by field "...". Custom fields must be indexed before filtering. The handleContentList catch block maps that EmDashValidationError to VALIDATION_ERROR, but the same request without fieldFilters (or any content-list request to a real collection) would hit the missing ec_* table and return COLLECTION_NOT_FOUND. That makes the error code depend on whether filters are supplied, which is a contract regression.
Check collection existence when field metadata is missing, and surface the same COLLECTION_NOT_FOUND error the rest of the handler uses. For example:
| const rows = await this.db | |
| .selectFrom("_emdash_fields as field") | |
| .innerJoin("_emdash_collections as collection", "collection.id", "field.collection_id") | |
| .where("collection.slug", "=", type) | |
| .where("field.slug", "in", fields) | |
| .where("field.indexed", "=", 1) | |
| .select(["field.slug", "field.type"]) | |
| .execute(); | |
| const metadata = new Map(rows.map((row) => [row.slug, row.type as FieldType])); | |
| const normalized = fields.map((field) => { | |
| const fieldType = metadata.get(field); | |
| if (!fieldType || !isIndexableFieldType(fieldType)) { | |
| throw new EmDashValidationError( | |
| `Cannot filter by field "${field}". Custom fields must be indexed before filtering.`, | |
| const normalized = fields.map(async (field) => { | |
| const fieldType = metadata.get(field); | |
| if (!fieldType || !isIndexableFieldType(fieldType)) { | |
| const collection = await this.db | |
| .selectFrom("_emdash_collections") | |
| .where("slug", "=", type) | |
| .select("id") | |
| .executeTakeFirst(); | |
| if (!collection) { | |
| throw new CollectionNotFoundError(type); | |
| } | |
| throw new EmDashValidationError( | |
| `Cannot filter by field "${field}". Custom fields must be indexed before filtering.`, | |
| ); | |
| } | |
| return this.normalizeFieldFilter(field, fieldType, resolvedFilters[field]); | |
| }); |
This needs a matching CollectionNotFoundError (or reuse of the existing missing-table mapping) and a handler catch that converts it to COLLECTION_NOT_FOUND. Please also add a test covering a list request with fieldFilters against a missing collection.
`findMany` races the page query against the count query. A collection whose table is missing rejects both, and `Promise.all` returns on the first while the other stays in flight holding a pooled connection. A Postgres pool destroyed inside that window never finishes closing. This predates the branch. The missing-collection regression test added here is the first to reach the path on Postgres, so the suite cannot pass without it.
There was a problem hiding this comment.
The indexed custom-field filtering approach remains sound and fits EmDash’s architecture: filters are validated in the REST query schema, resolved against _emdash_fields with indexed = 1, applied as parameterized SQL through sql.ref() / raw boolean expressions, and only exposed on authenticated content-list paths (REST, plugin content:read, and internal handlers). Cursor pagination, null ordering, and index-backed query plans line up with the (isPresent, field, id) index key, and tests cover scalar, in, range, null, budget, injection, cursor stability, and query-plan index usage.
This re-review confirms that the prior review’s concrete code regression is fixed: resolveFieldFilters now checks collection existence when metadata is empty, so requests to non-existent collections return COLLECTION_NOT_FOUND instead of a misleading VALIDATION_ERROR. The two resolved prior items (test Set.size assertions and localized admin custom-cell formatting) also look correct.
New code issues found:
- Residual error-contract edge case. If a request to a non-existent collection includes
fieldFilterswith an invalid field identifier, the identifier validation inresolveFieldFiltersstill throws before the collection-existence fallback, so the handler returnsVALIDATION_ERRORinstead of matching the no-filtersCOLLECTION_NOT_FOUND. The fix is to move identifier validation after the collection-existence fallback. - AGENTS.md comment-discipline violations. Several newly-added comments justify decisions, narrate rejected alternatives, or address the reviewer rather than explaining a non-obvious invariant. These should be deleted.
The maintainer-approved Discussion requirement for indexed filtering (distinct from the existing Discussion for admin.listColumns) remains pending as a non-code process item.
| } | ||
| for (const field of fields) { | ||
| try { | ||
| validateIdentifier(field, "content filter field"); | ||
| } catch { | ||
| throw new EmDashValidationError(`Invalid content filter field: ${field}`); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[needs fixing] Identifier validation runs before the collection-existence fallback. For a request to a non-existent collection that also includes an invalid field name (e.g. {"x y": ...}), this throws EmDashValidationError before the fallback can detect the missing collection, so the handler returns VALIDATION_ERROR instead of the COLLECTION_NOT_FOUND returned for the same request without filters. That is a residual break in the existing error contract.
Move the identifier validation loop after the metadata / collection-existence check so missing collections are detected first:
| } | |
| for (const field of fields) { | |
| try { | |
| validateIdentifier(field, "content filter field"); | |
| } catch { | |
| throw new EmDashValidationError(`Invalid content filter field: ${field}`); | |
| } | |
| } | |
| const rows = await this.db | |
| .selectFrom("_emdash_fields as field") | |
| .innerJoin("_emdash_collections as collection", "collection.id", "field.collection_id") | |
| .where("collection.slug", "=", type) | |
| .where("field.slug", "in", fields) | |
| .where("field.indexed", "=", 1) | |
| .select(["field.slug", "field.type"]) | |
| .execute(); | |
| const metadata = new Map(rows.map((row) => [row.slug, row.type as FieldType])); | |
| if (metadata.size === 0) { | |
| const collection = await this.db | |
| .selectFrom("_emdash_collections") | |
| .where("slug", "=", type) | |
| .select("id") | |
| .executeTakeFirst(); | |
| if (!collection) return []; | |
| } | |
| for (const field of fields) { | |
| try { | |
| validateIdentifier(field, "content filter field"); | |
| } catch { | |
| throw new EmDashValidationError(`Invalid content filter field: ${field}`); | |
| } | |
| } |
| // A missing collection produces the same empty result as an unindexed | ||
| // field, and reporting it as a filter problem would hide the real cause: | ||
| // the same request without `fieldFilters` answers COLLECTION_NOT_FOUND. | ||
| // Returning no filters lets the query reach its missing backing table, | ||
| // which is where that error is raised. |
There was a problem hiding this comment.
[needs fixing] This block is a reviewer-facing justification for the fallback. AGENTS.md prohibits comments that justify decisions or narrate rejected alternatives. The test below (reports a missing collection the same way with and without field filters) already documents the contract. Delete the comment.
| // A missing collection produces the same empty result as an unindexed | |
| // field, and reporting it as a filter problem would hide the real cause: | |
| // the same request without `fieldFilters` answers COLLECTION_NOT_FOUND. | |
| // Returning no filters lets the query reach its missing backing table, | |
| // which is where that error is raised. | |
| const metadata = new Map(rows.map((row) => [row.slug, row.type as FieldType])); | |
| if (metadata.size === 0) { | |
| const collection = await this.db | |
| .selectFrom("_emdash_collections") | |
| .where("slug", "=", type) | |
| .select("id") | |
| .executeTakeFirst(); | |
| if (!collection) return []; | |
| } |
| // `false`, not `undefined`: the server reads an absent `indexed` as | ||
| // "keep the stored value", so switching an indexed field to a type | ||
| // that cannot be indexed would keep the flag set and then fail its | ||
| // own validation. The Indexed switch is hidden for those types, so | ||
| // the editor could not clear it either. |
There was a problem hiding this comment.
[needs fixing] This comment justifies why the editor sends false rather than undefined. AGENTS.md treats justification comments as violations. The code indexed: isIndexableType ? indexed : false is clear enough; delete the comment.
| // `false`, not `undefined`: the server reads an absent `indexed` as | |
| // "keep the stored value", so switching an indexed field to a type | |
| // that cannot be indexed would keep the flag set and then fail its | |
| // own validation. The Indexed switch is hidden for those types, so | |
| // the editor could not clear it either. | |
| searchable: isSearchableType ? searchable : undefined, | |
| indexed: isIndexableType ? indexed : false, | |
| validation: Object.keys(validation).length > 0 ? validation : null, |
| // The filters resolve against `_emdash_fields`, which exists, so an | ||
| // unknown collection would otherwise surface as a filter problem and | ||
| // hide the real cause. |
There was a problem hiding this comment.
[needs fixing] The test name already explains what is being asserted (reports a missing collection the same way with and without field filters). The inline comment restates the rationale and is reviewer-facing narration. Delete it.
| // The filters resolve against `_emdash_fields`, which exists, so an | |
| // unknown collection would otherwise surface as a filter problem and | |
| // hide the real cause. | |
| it("reports a missing collection the same way with and without field filters", async () => { | |
| const withFilters = await handleContentList(ctx.db, "ghosts", { | |
| fieldFilters: { priority: "urgent" }, | |
| }); |
| // The admin has to send `indexed: false` here. Omitting it means | ||
| // "keep the stored value", which would leave the flag set on a type | ||
| // that rejects it, and the update would fail its own validation with | ||
| // no way for the editor to clear the flag first. |
There was a problem hiding this comment.
[needs fixing] This comment narrates the setup rationale and a rejected alternative (Omitting it means ...). The test name already describes the behavior (drops the index when an indexed field moves to a type that cannot carry one). Delete the comment.
| // The admin has to send `indexed: false` here. Omitting it means | |
| // "keep the stored value", which would leave the flag set on a type | |
| // that rejects it, and the update would fail its own validation with | |
| // no way for the editor to clear the flag first. | |
| it("drops the index when an indexed field moves to a type that cannot carry one", async () => { | |
| await registry.createField("posts", { | |
| slug: "summary", | |
| label: "Summary", | |
| type: "string", | |
| indexed: true, | |
| }); |
| // The dialog overlay blocks Playwright's actionability check, so submit | ||
| // through the DOM node the way gallery-detail-panel.test.tsx does. |
There was a problem hiding this comment.
[needs fixing] This comment narrates a workaround, references another test file, and explains a rejected approach. AGENTS.md discourages narrative comments that address the reviewer. The helper name save is self-explanatory; delete the comment.
| // The dialog overlay blocks Playwright's actionability check, so submit | |
| // through the DOM node the way gallery-detail-panel.test.tsx does. | |
| describe("indexed flag", () => { | |
| const save = async (screen: Awaited<ReturnType<typeof render>>) => { | |
| const button = screen.getByRole("button", { name: "Update Field" }); | |
| await expect.element(button).toBeEnabled(); | |
| button.element().click(); | |
| }; |
Identifier validation ran first, so a request naming both a collection that does not exist and an invalid filter field answered VALIDATION_ERROR while the same request without filters answered COLLECTION_NOT_FOUND. The field names reach the lookup as bound parameters, so deferring validation past the collection check keeps the query parameterized. Drop four comments that justify decisions rather than record an invariant.
Three more comments narrate a workaround, name another test file, or defend an omission. The remaining two record a footgun a reader would otherwise trip over: a stranded pool connection, and the bind-parameter budget behind a batch size.
2 participants
What does this PR do?
Adds typed, server-backed filtering for custom fields that are explicitly marked
indexed.The content list REST API, core client, runtime, and plugin content access contract accept AND-combined
fieldFilters. Supported conditions include exact scalar matches, missing values, membership throughin, and inclusive or exclusive numeric or textual ranges. Field names and values are schema-validated, bounded, and resolved only against indexed fields.This lets plugins and admin experiences filter complete result sets by metadata such as ticket state, priority, workflow status, or SEO score without browser-side filtering or full-table scans.
This PR is stacked on #2212, which supplies the indexed-field storage and validation contract. Both PRs can be reviewed at the same time, but #2212 should merge first; GitHub will then narrow this PR to its unique filtering commit automatically.
Addresses the structured filtering portion of #2179.
Discussion: #1717
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runadmin.listColumns(feat(admin): show configured fields in content lists #2194); no Discussion has been opened for indexed filtering yetAI-generated code disclosure
Screenshots / test output
Validated on EmDash 0.32.0 (base
776d65f7) with Node 24.16.0 and pnpm 11.9.0 on macOS. The workspace has all six stacked contributions applied, so these are the integrated totals rather than a per-PR subset:CI runs each PR on its own branch.