diff --git a/.changeset/publish-staged-slug-conflict.md b/.changeset/publish-staged-slug-conflict.md new file mode 100644 index 0000000000..599df40ff6 --- /dev/null +++ b/.changeset/publish-staged-slug-conflict.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes publishing an entry whose staged slug is already used by another entry in the same locale failing with an opaque database error. Publish now returns a validation error naming the conflicting slug and entry, which the admin can show inline, and leaves the live entry untouched. diff --git a/packages/core/src/api/handlers/content.ts b/packages/core/src/api/handlers/content.ts index e4eccc4389..9544e27cea 100644 --- a/packages/core/src/api/handlers/content.ts +++ b/packages/core/src/api/handlers/content.ts @@ -1487,14 +1487,39 @@ export async function handleContentPublish( }; } if (error instanceof EmDashValidationError) { + // The staged-slug pre-check tags its error so it maps to the same + // 409 SLUG_CONFLICT as direct slug edits in create/update. + const details: unknown = error.details; + const isSlugConflict = + typeof details === "object" && + details !== null && + "code" in details && + details.code === "SLUG_CONFLICT"; return { success: false, error: { - code: "VALIDATION_ERROR", + code: isSlugConflict ? "SLUG_CONFLICT" : "VALIDATION_ERROR", message: error.message, }, }; } + // Backstop for the pre-check inside repo.publish(): a concurrent write + // can still take the slug between the check and the UPDATE, in which + // case the `(slug, locale)` unique constraint fires. Same fingerprint + // mapping as create/update — never a raw SQLite error to the client. + const message = error instanceof Error ? error.message.toLowerCase() : ""; + if ( + (message.includes("unique constraint failed") || message.includes("duplicate key")) && + message.includes("slug") + ) { + return { + success: false, + error: { + code: "SLUG_CONFLICT", + message: `The staged slug is already used by another entry in collection '${collection}'`, + }, + }; + } console.error("Content publish error:", error); return { success: false, diff --git a/packages/core/src/database/repositories/content.ts b/packages/core/src/database/repositories/content.ts index a0b17a6014..92c192cb2f 100644 --- a/packages/core/src/database/repositories/content.ts +++ b/packages/core/src/database/repositories/content.ts @@ -1339,16 +1339,39 @@ export class ContentRepository { // so the content table always holds the published version const revision = await revisionRepo.findById(revisionToPublish); if (revision) { - await this.syncDataColumns(type, id, revision.data); + // A staged slug lands on the live column only here — it never + // passes through update(), so its uniqueness was never checked. + // Validate before any write: the `(slug, locale)` unique index + // covers every row (drafts and trashed entries included), and + // letting the constraint fire mid-publish would surface as an + // opaque 500 after draft data already hit the live columns. + // A NULL locale never collides (unique indexes treat NULLs as + // distinct), so the check only applies to localized rows. + const stagedSlug = typeof revision.data._slug === "string" ? revision.data._slug : null; + if (stagedSlug !== null && stagedSlug !== existing.slug && existing.locale !== null) { + const conflict = await this.findBySlugIncludingTrashed(type, stagedSlug, existing.locale); + if (conflict && conflict.id !== id) { + throw new EmDashValidationError( + `Cannot publish: slug '${stagedSlug}' is already used by another entry` + + ` in this collection (id: ${conflict.id}). Choose a different slug.`, + { code: "SLUG_CONFLICT" }, + ); + } + } - // Sync slug from revision if stored there - if (typeof revision.data._slug === "string") { + // Write the slug before the data columns: the slug UPDATE is the + // only statement here that can hit the unique constraint (if a + // concurrent write took the slug after the check above), and on + // D1 there is no transaction to roll back earlier statements. + if (stagedSlug !== null) { await sql` UPDATE ${sql.ref(tableName)} - SET slug = ${revision.data._slug} + SET slug = ${stagedSlug} WHERE id = ${id} `.execute(this.db); } + + await this.syncDataColumns(type, id, revision.data); } if (publishedAt !== undefined) { diff --git a/packages/core/tests/unit/api/content-handlers.test.ts b/packages/core/tests/unit/api/content-handlers.test.ts index d4cca91f74..f8e0093813 100644 --- a/packages/core/tests/unit/api/content-handlers.test.ts +++ b/packages/core/tests/unit/api/content-handlers.test.ts @@ -789,4 +789,34 @@ describe("Content Handlers — slug-change auto-redirect on publish", () => { const redirects = await db.selectFrom("_emdash_redirects").selectAll().execute(); expect(redirects).toHaveLength(0); }); + + // #2034: a staged slug colliding with another entry's (slug, locale) used + // to surface as an opaque 500 (raw D1/SQLite UNIQUE error). It must be the + // same SLUG_CONFLICT (409) as direct slug edits, naming the slug so the + // admin can show it inline. + it("returns SLUG_CONFLICT naming the slug when the staged slug is taken", async () => { + const taken = await handleContentCreate(db, "post", { + data: { title: "Owner" }, + slug: "eleven", + status: "published", + }); + expect(taken.success).toBe(true); + + const created = await handleContentCreate(db, "post", { + data: { title: "Victim" }, + slug: "victim", + status: "published", + }); + expect(created.success).toBe(true); + const id = created.data!.item.id; + + await stageDraftSlugChange("post", id, { title: "Victim" }, "eleven"); + + const published = await handleContentPublish(db, "post", id); + expect(published.success).toBe(false); + if (published.success) return; + expect(published.error.code).toBe("SLUG_CONFLICT"); + expect(published.error.message).toContain("eleven"); + expect(published.error.message).toContain(taken.data!.item.id); + }); }); diff --git a/packages/core/tests/unit/database/repositories/content.test.ts b/packages/core/tests/unit/database/repositories/content.test.ts index 0675773e33..8135cddc18 100644 --- a/packages/core/tests/unit/database/repositories/content.test.ts +++ b/packages/core/tests/unit/database/repositories/content.test.ts @@ -525,6 +525,60 @@ describe("ContentRepository", () => { }); }); + describe("publish() staged slug conflict (#2034)", () => { + /** Stage a slug edit the way the runtime does: as `_slug` in a draft revision. */ + async function stageSlug(entryId: string, data: Record, newSlug: string) { + const revisionRepo = new RevisionRepository(db); + const draft = await revisionRepo.create({ + collection: "post", + entryId, + data: { ...data, _slug: newSlug }, + }); + await repo.setDraftRevision("post", entryId, draft.id); + } + + it("rejects a staged slug already held by another entry with an actionable error", async () => { + await repo.create(createPostFixture({ slug: "taken", status: "published" })); + const post = await repo.create(createPostFixture({ slug: "mine" })); + await stageSlug(post.id, post.data, "taken"); + + const error: unknown = await repo.publish("post", post.id).catch((e: unknown) => e); + + expect(error).toBeInstanceOf(EmDashValidationError); + expect((error as Error).message).toContain("taken"); + // The live row must be untouched — no partial publish. + const after = await repo.findById("post", post.id); + expect(after?.slug).toBe("mine"); + expect(after?.status).toBe("draft"); + }); + + it("rejects a staged slug held by a trashed entry (the unique index still covers it)", async () => { + const trashed = await repo.create(createPostFixture({ slug: "ghost" })); + await repo.delete("post", trashed.id); + const post = await repo.create(createPostFixture({ slug: "mine" })); + await stageSlug(post.id, post.data, "ghost"); + + await expect(repo.publish("post", post.id)).rejects.toThrow(EmDashValidationError); + }); + + it("allows republishing with the entry's own slug staged", async () => { + const post = await repo.create(createPostFixture({ slug: "stable", status: "published" })); + await stageSlug(post.id, post.data, "stable"); + + const published = await repo.publish("post", post.id); + expect(published.slug).toBe("stable"); + }); + + it("allows the same slug in a different locale", async () => { + await repo.create(createPostFixture({ slug: "hallo", locale: "de" })); + const post = await repo.create(createPostFixture({ slug: "mine", locale: "en" })); + await stageSlug(post.id, post.data, "hallo"); + + const published = await repo.publish("post", post.id); + expect(published.slug).toBe("hallo"); + }); + }); + describe("publish() clears schedule", () => { it("should clear scheduled_at when publishing a scheduled draft", async () => { const post = await repo.create(createPostFixture());