Repository navigation
fix(core): let an entry shed a deleted field's value instead of stranding it - #3142
Conversation
🦋 Changeset detectedLatest commit: 49d6710 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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/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-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@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 fixes a real bug: once a collection field is deleted, the draft revision still carries the old value, so EmDashRuntime.handleContentUpdate rejects it as an unknown field and the editor cannot save its way out. The chosen approach — drop unknown keys the entry already stores before validation, and write merged draft data through keepKnownFields() so future revisions also shed deleted keys — is sound for the runtime path it touches, and the tests cover the primary cases well.
However, the same merge/validation logic still exists in ContentRepository.updateDraftAware, which is used by plugin context and both sandbox bridges (workerd and Cloudflare). That path still throws for a key stranded in a draft revision, so the bug is only partially fixed until the helper is shared or updateDraftAware is updated too.
I also have a few smaller items: the new comments are useful but read like a PR-description/narrative in places; the test header references the issue number; and handleContentUpdate can now load the collection field list three or four times in a single save, which is worth consolidating even though it is an authenticated route.
None of these are security or data-loss blockers, but the留下updateDraftAware gap is a real regression left standing.
Findings
-
[needs fixing]
packages/core/src/database/repositories/content.ts:956-970updateDraftAwarestill validates every incoming key against the live field list and merges{ ...baseData, ...data }without shedding keys whose fields have been deleted. This leaves the same stranding bug alive for every caller that is notEmDashRuntime.handleContentUpdate— specificallypackages/core/src/plugins/context.ts,packages/workerd/src/sandbox/bridge-handler.ts, andpackages/cloudflare/src/sandbox/bridge.ts. The newkeepKnownFieldsfilter should be applied to the merged data here (and already-stored unknown keys should be allowed through the field check), either by sharing the helper or by duplicating the same behavior, so the fix covers every content-write surface. -
[suggestion]
packages/core/src/emdash-runtime.ts:2961-2969normalizeMediaFieldsalready loads the collection field list and discards it, thendropUnknownKeysAlreadyStoredloads it again, thenvalidateContentDataloads it a third time, and the merge further down loads it a fourth time. On an update-with-data this can issue four field-list queries. Consider loadingcollectionInfoonce before normalization and reusing it through the rest of the function, or havingnormalizeMediaFieldsreturn the field list it already fetched. -
[suggestion]
packages/core/src/emdash-runtime.ts:2963-2968The comment explains a non-obvious invariant, but it is written as a bug-report narrative ("would be refused... no body that works"). AGENTS.md asks comments to state the invariant for future readers without restating the PR description. Trim it:
// Drop unknown field keys the entry already stores (e.g. a deleted field // stranded in a draft revision) before validation, while still rejecting // unknown keys the entry has never stored. -
[suggestion]
packages/core/src/emdash-runtime.ts:3971-3979The docstring is useful but longer than necessary and reads like the PR description. Keep the reason someone reading the code needs:
/** * Drop incoming keys that have no matching collection field when the entry * already stores them in its live data or current draft revision. * * Draft revisions keep the full `data` JSON, so deleting a field can strand * the old value; this lets a read-then-write save succeed without allowing * genuinely unknown keys. */ -
[suggestion]
packages/core/tests/integration/content/deleted-field-stranded-entry.test.ts:1-6AGENTS.md says comments should never reference issues, PRs, or review threads — that context belongs in the commit message and PR description, not in code. Drop the issue number from the test header:
/** * Deleting a field can strand a pending draft: the revision stores the whole `data`, * so the deleted field's value stays there and every read hands it back. Writing that * data back was refused as an unknown field, an explicit null was refused too, and * omitting the key was accepted but left the merge carrying it, so no request body * got the entry out of the state. */
emdashbot
Bot
added
review/awaiting-author
|
Thanks, the
Rather than duplicate the behavior I moved it into Two new cases in the integration test cover the repository path: a read-then-write save that sheds the key, and a typo key that is still refused. The first fails against this branch without the Field-list loads (suggestion). You were right about the count, and Comments and test header (suggestions). Taken as written, all three.
|
github-actions
Bot
added
review/needs-rereview
…ding it Deleting a field leaves its value in the entry's draft revision, which stores the whole data. Every read handed the key back, writing it back was refused as an unknown field, an explicit null was refused too, and omitting the key was accepted but left the merge carrying it, so no request body got the entry out. An update now drops a key the collection has no field for when the entry already stores it, and the saved data is written without those keys, so the entry sheds them. A key the entry does not already store is still an unknown field, so a typo still fails.
The first pass only covered EmDashRuntime.handleContentUpdate, so an entry with a stranded key stayed unsaveable through ContentRepository.updateDraftAware, which the plugin context and both sandbox bridges write through. That path now applies the same two rules, and the shared helpers live in one module rather than being duplicated per caller. handleContentUpdate loaded the collection's field list up to four times in a single save; it is now read once and threaded through normalization, the stale-key drop and the draft merge.
5129a8a to
49d6710
Compare
One conflict, in EmDashRuntime.handleContentUpdate. emdash-cms#3142 hoisted the collection load out of the draft-revision block so a single save reads the field list once, and narrowed the block's guard back to `processedData`. This branch had widened that guard to `processedData || references` so a save carrying only a picker change still reaches draft staging. Keep both: the guard stays widened and the local re-load is gone. The hoist condition has to widen to match, since it only loaded the collection when the body carried `data` — a references-only save would otherwise reach the draft merge with a null collection and silently skip revision handling. `keepKnownFields` now also applies to a references-only save's merged data, which is the intended shedding behaviour and leaves reserved `_`-prefixed keys such as `_slug` untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
What does this PR do?
Fixes #3141: an entry with a pending draft cannot be saved again once one of its fields is deleted.
A draft revision stores the whole
dataas JSON, so a deleted field's value stays in it and every read hands the key back. On the way in,validateContentDatarefuses any key that is not a current field, and there is no request body that gets the entry out:hits: unknown field on collection 'posts';nullor"": refused the same way;This takes the validation side of the two fixes offered in the issue, because it also rescues entries that a delete has already stranded. Two changes in
EmDashRuntime.handleContentUpdate:dropUnknownKeysAlreadyStored()removes a key the collection has no field for from the incoming data when the entry already stores it (checking the draft revision when there is one, otherwise the entry). A key the entry does not already store is left alone, so an unknown field is still reported as one and a typo still fails.keepKnownFields(), so a saved entry sheds keys the collection has no field for instead of carrying them through every revision that follows. Reserved_-prefixed keys are kept, and a collection whose fields cannot be read is left alone rather than emptied.I have not touched
handleContentCreate: a new entry has nothing stored to shed, so an unknown key there is exactly what the existing error is for.The alternative in the issue, stripping the field from stored data inside
schema_delete_field, is worth doing as well, and it is not in this PR. It prevents the next delete from stranding anything, but it cannot help an entry stranded by a delete that has already happened.Type of change
Checklist
pnpm typecheckpasses (packages/core)pnpm lintpasses (pnpm lint:json: 0 diagnostics)pnpm testpasses (or targeted tests for my change):tests/integration/contentandtests/unit/fields, 26 files, 220 testspnpm formathas been runemdash: patch)AI-generated code disclosure
Screenshots / test output
Not applicable for UI.
packages/core/tests/integration/content/deleted-field-stranded-entry.test.tssets up a published entry with a pending draft, deletes the field, and then covers the four cases. Againstmainthe first three fail:With the change:
And the suites around it:
Opened as a draft: I have six PRs waiting for review, which I understand to be the cap. Mark it ready whenever suits you, or say if you would rather I close it and reopen it later.