fix(seed): skip content whose slug collides with a trashed entry - #1814
masonjames wants to merge 4 commits into
Conversation
applySeed's existence check uses the live-rows-only findBySlug, but the content tables' UNIQUE(slug, locale) constraint also covers trashed rows — so re-applying a seed against a database where a seeded entry had been moved to the trash crashed with a raw SQLite constraint error instead of honoring onConflict. Hit in the wild via the dev setup bypass, which re-applies the seed on every call. Trashed collisions now skip in every non-error mode — deliberately deleted content is never resurrected or overwritten — and error mode reports a clear conflict message.
🦋 Changeset detectedLatest commit: 15e6270 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
area/core
size/M
review/needs-review
@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 fixes a real, well-explained bug: applySeed only checks live rows for slug conflicts, so re-applying a seed after an entry was moved to trash crashes on the UNIQUE(slug, locale) constraint. The approach is correct for EmDash — it keeps the unique index intact, respects the deliberate-deletion semantics of the trash, and preserves the existing live-row behavior unchanged.
The implementation is clean on the safety front: the lookup uses the existing parameterized findBySlugIncludingTrashed, filters by locale as required, and the changeset is present. Test coverage is good for the three onConflict modes.
I did not run the test suite or linters (no shell), so I’m treating the author’s tooling results as unverified.
One concrete issue: when a trashed collision is skipped, the code adds the trashed row’s id to seedIdMap. That id later flows into translationOf resolution and menu/$ref resolution. ContentRepository.create resolves translationOf via findById, which filters deleted_at IS NULL, so a trashed id causes EmDashValidationError("Translation source content not found") and aborts the whole seed apply. Even for plain $ref or menu refs, pointing at a deleted row is a broken reference. Since the PR’s goal is to stop seed re-applies from crashing on trashed content, this mapping undermines that goal for seeds with references/translations. The fix is to not map the skipped trashed entry to seed IDs at all, which makes downstream references behave like any other unresolved seed reference.
I’d also like to see a regression test covering a translationOf (or $ref/menu ref) pointing at a trashed-collision entry, so this edge case stays fixed.
Review follow-up: recording the trashed row's id in seedIdMap made the skipped entry a resolution target for translationOf, $ref, and menu refs. translationOf resolves through the live-only findById, so a sibling entry translating the skipped one crashed the whole apply with 'Translation source content not found' — the same failure class this fix exists to prevent. Skipped trashed entries now behave like any other unresolved seed reference; regression test covers the translationOf path.
github-actions
Bot
added
review/needs-rereview
|
I'm kind've in two minds here. I think this is a valid fix, but I also think we need to think a bit more about what trashing a post means for the slug. I'm not sure it should collide with a live post. That would mean either changing the slug when something is trashed (simple, but hacky) or changing the unique key to be a composite of the slug and trashed status. I'd be interested in people's thoughts on that. |
|
FWIW, WordPress hit exactly this (#11863 — open for six years) and shipped the hacky option in 4.5: trash appends __trashed (ref), untrash reclaims the stored slug, and pre-existing trashed posts get renamed lazily when a new post wants their slug (ref). Stable for a decade since. The follow-up bugs (#40588, #44805) were all restore semantics, so we could design for that up front. Rename-on-trash is structurally cheaper: UNIQUE(slug, locale) is a table constraint in every ec_* table's DDL, so a composite key means rebuilding every content table on D1 — and restore needs live-collision handling either way. A deterministic suffix needs no stored state: strip it and re-run slug generation. Either way existing databases have unsuffixed trashed rows regardless, which is why WP kept the lazy path. Happy to open a Discussion for more eyeballs. |
|
Yeah, a discussion would be good. |
|
This PR has been inactive for 14 days. It will be closed automatically in 7 days if there is no further activity. If you're still working on this, please push an update or leave a comment. |
…-slug-collision # Conflicts: # packages/core/src/seed/apply.ts
|
Discussion post created here: #2412 |
2 participants
What does this PR do?
Re-applying a seed against a database where a seeded entry has been moved to the trash crashes with a raw SQLite error instead of honoring
onConflict:applySeed's content existence check usesContentRepository.findBySlug, which filtersdeleted_at IS NULL— but the content tables'UNIQUE(slug, locale)constraint covers trashed rows too. So a trashed entry is invisible to the conflict check, the insert fires, and the constraint throws. Hit in the wild via the dev setup bypass (/_emdash/api/setup/dev-bypass), which re-applies the seed on every call: on our site, a dozen seeded projects had been moved to the trash, and the bypass 500'd every time.Fix: when the live lookup misses, check
findBySlugIncludingTrashed. A trashed collision now:skipmode (counted inresult.content.skipped, with a console warning naming the entry),updatemode too — a trashed row is a deliberate deletion, and a seed re-apply must not resurrect or overwrite it,Conflict: ... already exists (in trash)inerrormode instead of the raw constraint error.The live-row path is untouched (the extra lookup only runs when the live check misses). Three new tests pin the semantics per mode; the
skip/updatetests fail onmainwith the exact production error.Issue: none filed — happy to open one retroactively if you prefer tracking it.
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runemdash, patch)AI-generated code disclosure
Screenshots / test output
mainbefore the fix:SqliteError: UNIQUE constraint failed: ec_posts.slug, ec_posts.locale(skip/update modes) and the raw constraint error surfacing inerrormodevitest run tests/unit/seed/→ 129 passed (8 files)pnpm lint:json | jq '.diagnostics | length'→0pnpm format→ ranemdash, patch)