diff --git a/.changeset/deleted-field-stranded-drafts.md b/.changeset/deleted-field-stranded-drafts.md new file mode 100644 index 0000000000..54f3f2a2d7 --- /dev/null +++ b/.changeset/deleted-field-stranded-drafts.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes an entry with a pending draft becoming unsaveable after one of its fields is deleted. The draft revision stores the whole `data`, so the deleted field's value stayed in it and every read handed it back; writing that data back was then refused with `unknown field on collection`, 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. An update now drops a key the collection has no field for when the entry already stores it, and a saved entry is written without those keys, so it sheds them. A key the entry does not already store is still reported as an unknown field. This applies to every content write, including those made from a plugin or a sandboxed plugin bridge. diff --git a/packages/core/src/content/known-fields.ts b/packages/core/src/content/known-fields.ts new file mode 100644 index 0000000000..8fcc3c9a45 --- /dev/null +++ b/packages/core/src/content/known-fields.ts @@ -0,0 +1,44 @@ +/** + * Reconcile an entry's keys with the fields its collection currently declares. + * + * A draft revision stores the whole `data` as JSON, so deleting a field leaves the old + * value inside the revision. Every content-write surface needs the same two rules to keep + * that from stranding the entry, so they live here rather than in one caller. + */ + +/** Keys EmDash reserves (`_slug` and the like). No field declares them. */ +function isReservedKey(key: string): boolean { + return key.startsWith("_"); +} + +/** + * The entry data without the keys the collection has no field for. + * + * Reserved keys are kept. + */ +export function keepKnownFields( + data: Record, + knownFieldSlugs: ReadonlySet, +): Record { + const result: Record = {}; + for (const [key, value] of Object.entries(data)) { + if (isReservedKey(key) || knownFieldSlugs.has(key)) result[key] = value; + } + return result; +} + +/** + * Incoming keys with no matching field that the entry already stores. + * + * Dropping these lets a read-then-write save succeed after the field was deleted. A key the + * entry has never stored is not returned, so a genuinely unknown field is still reported. + */ +export function staleStoredKeys( + data: Record, + stored: Record, + knownFieldSlugs: ReadonlySet, +): string[] { + return Object.keys(data).filter( + (key) => !isReservedKey(key) && !knownFieldSlugs.has(key) && Object.hasOwn(stored, key), + ); +} diff --git a/packages/core/src/database/repositories/content.ts b/packages/core/src/database/repositories/content.ts index 90b3ee16b9..d3e4a4f674 100644 --- a/packages/core/src/database/repositories/content.ts +++ b/packages/core/src/database/repositories/content.ts @@ -2,6 +2,7 @@ import { sql, type Kysely } from "kysely"; import { ulid } from "ulidx"; import type { ContentFieldFilterValue, ContentFieldFilters } from "../../content-list-query.js"; +import { keepKnownFields, staleStoredKeys } from "../../content/known-fields.js"; import { normalizeExplicitDatetime } from "../../datetime-normalization.js"; import { invalidateCollectionCache } from "../../object-cache/index.js"; import { isIndexableFieldType, type FieldType } from "../../schema/types.js"; @@ -963,16 +964,36 @@ export class ContentRepository { return this.update(type, id, { ...input, data }); } - const fieldSlugs = new Set(collectionRows.map((row) => row.fieldSlug).filter(Boolean)); - for (const field of Object.keys(data)) { + const fieldSlugs = new Set( + collectionRows.map((row) => row.fieldSlug).filter((slug): slug is string => Boolean(slug)), + ); + + const revisionRepo = new RevisionRepository(this.db); + let existing = await this.findById(type, id); + + // A key with no field is rejected unless the entry already stores it: deleting a field + // leaves its value in the draft revision, which holds the whole `data`, so a caller that + // reads an entry and writes it back would otherwise be refused for a key it never chose + // to send. Those keys are dropped here and shed from the merge below. + let incoming = data; + if (existing) { + let stored: Record = existing.data ?? {}; + if (existing.draftRevisionId) { + const draft = await revisionRepo.findById(existing.draftRevisionId); + if (draft?.data) stored = draft.data; + } + const stale = staleStoredKeys(data, stored, fieldSlugs); + if (stale.length > 0) { + incoming = { ...data }; + for (const key of stale) delete incoming[key]; + } + } + for (const field of Object.keys(incoming)) { if (!fieldSlugs.has(field)) { throw new EmDashValidationError(`Unknown field '${field}' in collection '${type}'`); } } - const revisionRepo = new RevisionRepository(this.db); - let existing = await this.findById(type, id); - for (let attempt = 0; existing && attempt < MAX_DRAFT_STAGE_ATTEMPTS; attempt++) { let baseData = existing.data; if (existing.draftRevisionId) { @@ -980,7 +1001,7 @@ export class ContentRepository { if (draft) baseData = draft.data; } - const mergedData = { ...baseData, ...data }; + const mergedData = keepKnownFields({ ...baseData, ...incoming }, fieldSlugs); if (stagedSlug !== undefined) mergedData._slug = stagedSlug; const revision = await revisionRepo.create({ collection: type, diff --git a/packages/core/src/emdash-runtime.ts b/packages/core/src/emdash-runtime.ts index 1abbe27249..9d2743502a 100644 --- a/packages/core/src/emdash-runtime.ts +++ b/packages/core/src/emdash-runtime.ts @@ -30,6 +30,7 @@ import type { EmDashManifest } from "./astro/types.js"; import { getAuthMode } from "./auth/mode.js"; import { getTrustedProxyHeaders } from "./auth/trusted-proxy.js"; import type { ContentFieldFilters } from "./content-list-query.js"; +import { keepKnownFields, staleStoredKeys } from "./content/known-fields.js"; import { isSqlite } from "./database/dialect-helpers.js"; import { kyselyLogOption } from "./database/instrumentation.js"; import { @@ -94,6 +95,7 @@ import type { } from "./plugins/types.js"; import { recordSchedulerHeartbeatSafely } from "./scheduler-health.js"; import { primeRegisteredCollections } from "./schema/collection-slugs-cache.js"; +import type { CollectionWithFields } from "./schema/types.js"; import { isMissingTableError } from "./utils/db-errors.js"; import { hashString } from "./utils/hash.js"; import { createInitLock, type InitLock, initWithLock } from "./utils/init-lock.js"; @@ -3054,6 +3056,13 @@ export class EmDashRuntime { } const { _rev: _discardedRev, actor: _discardedActor, ...bodyWithoutRev } = body; + // Loaded once and threaded through normalization, the stale-key drop and the draft + // merge below: each of those needs the field list and the registry does not cache. + const collectionInfo = bodyWithoutRev.data + ? await this.schemaRegistry.getCollectionWithFields(collection).catch(() => null) + : null; + const knownFieldSlugs = new Set((collectionInfo?.fields ?? []).map((f) => f.slug)); + // Run beforeSave hooks if data is provided let processedData = bodyWithoutRev.data; if (bodyWithoutRev.data) { @@ -3073,7 +3082,18 @@ export class EmDashRuntime { } // Normalize media fields (fill dimensions, storageKey, etc.) - processedData = await this.normalizeMediaFields(collection, processedData!); + processedData = await this.normalizeMediaFields(collection, processedData!, collectionInfo); + + // 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. + if (collectionInfo?.fields) { + processedData = await this.dropUnknownKeysAlreadyStored( + processedData, + resolvedItem, + knownFieldSlugs, + ); + } // Validate field-level shape BEFORE the draft-revision write so // invalid updates can't silently land in revision history. @@ -3095,7 +3115,6 @@ export class EmDashRuntime { let usesDraftRevisions = false; let draftStorageChanged = false; if (processedData) { - const collectionInfo = await this.schemaRegistry.getCollectionWithFields(collection); if (collectionInfo?.supports?.includes("revisions")) { usesDraftRevisions = true; const revisionRepo = new RevisionRepository(this.db); @@ -3110,7 +3129,12 @@ export class EmDashRuntime { baseData = existing.data; } - const mergedData = { ...baseData, ...processedData }; + // Written without the keys the collection has no field for, so an entry + // carrying a deleted field's value sheds it on its next save instead of + // carrying it through every revision that follows. + const mergedData = collectionInfo?.fields + ? keepKnownFields({ ...baseData, ...processedData }, knownFieldSlugs) + : { ...baseData, ...processedData }; if (bodyWithoutRev.slug !== undefined) { mergedData._slug = bodyWithoutRev.slug; } @@ -4152,6 +4176,37 @@ export class EmDashRuntime { return undefined; } + /** + * 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. + */ + private async dropUnknownKeysAlreadyStored( + data: Record, + existing: { data: Record; draftRevisionId?: string | null } | null, + knownFieldSlugs: ReadonlySet, + ): Promise> { + if (!existing) return data; + + let stored: Record = existing.data ?? {}; + if (existing.draftRevisionId) { + const draft = await new RevisionRepository(this.db) + .findById(existing.draftRevisionId) + .catch(() => null); + if (draft?.data) stored = draft.data; + } + + const stale = staleStoredKeys(data, stored, knownFieldSlugs); + if (stale.length === 0) return data; + + const result = { ...data }; + for (const key of stale) delete result[key]; + return result; + } + /** * Normalize image/file fields in content data. * Fills missing dimensions, storageKey, mimeType, and filename from providers. @@ -4159,12 +4214,15 @@ export class EmDashRuntime { private async normalizeMediaFields( collection: string, data: Record, + preloaded?: CollectionWithFields | null, ): Promise> { - let collectionInfo; - try { - collectionInfo = await this.schemaRegistry.getCollectionWithFields(collection); - } catch { - return data; + let collectionInfo = preloaded; + if (collectionInfo === undefined) { + try { + collectionInfo = await this.schemaRegistry.getCollectionWithFields(collection); + } catch { + return data; + } } if (!collectionInfo?.fields) return data; diff --git a/packages/core/tests/integration/content/deleted-field-stranded-entry.test.ts b/packages/core/tests/integration/content/deleted-field-stranded-entry.test.ts new file mode 100644 index 0000000000..ac8b005e1a --- /dev/null +++ b/packages/core/tests/integration/content/deleted-field-stranded-entry.test.ts @@ -0,0 +1,126 @@ +/** + * 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. + */ +import { afterEach, beforeEach, describe, expect } from "vitest"; + +import { ContentRepository } from "../../../src/database/repositories/content.js"; +import type { EmDashRuntime } from "../../../src/emdash-runtime.js"; +import { SchemaRegistry } from "../../../src/schema/registry.js"; +import { createTestRuntime } from "../../utils/mcp-runtime.js"; +import { + describeEachDialect, + setupForDialect, + teardownForDialect, + type DialectTestContext, +} from "../../utils/test-db.js"; + +describeEachDialect( + "an entry with a pending draft when one of its fields is deleted", + (dialect) => { + let ctx: DialectTestContext; + let runtime: EmDashRuntime; + let id: string; + + beforeEach(async () => { + ctx = await setupForDialect(dialect); + const registry = new SchemaRegistry(ctx.db); + await registry.createCollection({ slug: "posts", label: "Posts" }); + await registry.createField("posts", { slug: "title", label: "Title", type: "string" }); + await registry.createField("posts", { slug: "hits", label: "Hits", type: "integer" }); + runtime = createTestRuntime(ctx.db); + + const created = await runtime.handleContentCreate("posts", { + slug: "p1", + data: { title: "p1", hits: 42 }, + status: "published", + }); + if (!created.success) throw new Error("setup: create failed"); + id = created.data.item.id; + + // A pending draft, as an editor leaves behind by saving without publishing. + const draft = await runtime.handleContentUpdate("posts", id, { + data: { title: "p1 draft", hits: 43 }, + }); + if (!draft.success) throw new Error("setup: draft update failed"); + + await registry.deleteField("posts", "hits"); + }); + + afterEach(async () => { + await teardownForDialect(ctx); + }); + + it("saves the data it just read, and sheds the deleted field", async () => { + const got = await runtime.handleContentGet("posts", id); + expect(got.success).toBe(true); + if (!got.success) return; + const data = { ...(got.data.item.data as Record), title: "edited" }; + expect(data.hits).toBe(43); // the draft still carries it, which is what a client sends back + + const saved = await runtime.handleContentUpdate("posts", id, { data }); + expect(saved.success).toBe(true); + if (!saved.success) return; + expect(saved.data.item.data).toMatchObject({ title: "edited" }); + expect(Object.hasOwn(saved.data.item.data as Record, "hits")).toBe(false); + }); + + it("accepts an explicit null for the deleted field, and refuses it once the entry is clean", async () => { + const cleared = await runtime.handleContentUpdate("posts", id, { + data: { title: "edited", hits: null }, + }); + expect(cleared.success).toBe(true); + + // The entry no longer stores the key, so sending it again is an unknown field like any other. + const again = await runtime.handleContentUpdate("posts", id, { + data: { title: "edited twice", hits: null }, + }); + expect(again.success).toBe(false); + }); + + it("drops the value from the merged data even when the key is omitted", async () => { + const saved = await runtime.handleContentUpdate("posts", id, { data: { title: "edited" } }); + expect(saved.success).toBe(true); + if (!saved.success) return; + expect(Object.hasOwn(saved.data.item.data as Record, "hits")).toBe(false); + }); + + it("still refuses a key the entry does not already store", async () => { + const saved = await runtime.handleContentUpdate("posts", id, { + data: { title: "edited", titel: "typo" }, + }); + expect(saved.success).toBe(false); + if (saved.success) return; + expect(saved.error.message).toContain("unknown field on collection"); + }); + + // The plugin context and both sandbox bridges write through the repository rather than + // the runtime handler, so the same entry has to be saveable from there too. + describe("through ContentRepository.updateDraftAware", () => { + it("saves the data it just read, and sheds the deleted field", async () => { + const repo = new ContentRepository(ctx.db); + const got = await runtime.handleContentGet("posts", id); + expect(got.success).toBe(true); + if (!got.success) return; + const read = got.data.item.data as Record; + expect(read.hits).toBe(43); // the draft still carries it, which is what a caller sends back + + const updated = await repo.updateDraftAware("posts", id, { + data: { ...read, title: "edited" }, + }); + expect(updated.data).toMatchObject({ title: "edited" }); + expect(Object.hasOwn(updated.data, "hits")).toBe(false); + }); + + it("still refuses a key the entry does not already store", async () => { + const repo = new ContentRepository(ctx.db); + await expect( + repo.updateDraftAware("posts", id, { data: { title: "edited", titel: "typo" } }), + ).rejects.toThrow(/Unknown field/); + }); + }); + }, +);