From a17fe4739b14084e92350fb2ad5f145786541ef3 Mon Sep 17 00:00:00 2001 From: ssavutu Date: Fri, 28 Aug 2026 01:35:53 -0400 Subject: [PATCH 1/2] Stop a second article with the same title becoming the first one Creating an article whose title already existed filed a second row on the same slug. POST derived the slug from the title with no uniqueness check, and only the dashboard's quick-draft path deduped -- client-side, with a probe loop the full editor's New Article form never ran. Every screen then addressed articles by slug alone, so opening the new article loaded the older one, and the autosave that followed wrote over both: UPDATE ... WHERE `slug` = ? matches every duplicate, and clientFoundRows reports the match count, so nothing looked wrong. Production carried two letters in exactly that state, one of them with its body replaced by a letter from 2022. Reserve the slug server-side on create. The MySQL named lock is held on the candidate rather than on the stem it was derived from: two creates can reach the same candidate from different stems -- one titled "Foo" that becomes foo-2, one whose slug is literally foo-2 -- and a lock on the stem would let both insert it. The response now carries the id and the slug that were actually written, because the server may not have stored the one it was sent. Address articles by id everywhere else. The editor routes carry /articles/:id/:slug/edit, the write endpoints accept ?id=, and a request that arrives with a slug alone resolves to a single row -- preferring the caller's archive state, then the lowest id -- before it reads, locks or writes. One rule now governs the edit lease, the mutation lock, the excerpt derivation and the public detail read, so the row a request locks is the row it reads and the row it writes. Legacy /articles/:slug links keep working and resolve the same way. Renaming a slug onto one another article holds answers 409 rather than quietly recreating the collision. Two deadlocks surfaced while testing this, both the same shape: a handler waiting on the connection pool while holding part of it. Create kept its dedicated slug-lock connection across the pooled writes that follow the insert, and the detail read kept its result rows open across the author lookup. Neither is reachable at the production pool size; both hang outright at the single connection the integration tests use, which is why the create path had no working test. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WuLVSbPvAr2o4m7CqzLEVu --- frontend/src/App.tsx | 2 + frontend/src/pages/DashboardPage.tsx | 15 +- frontend/src/pages/articleView.tsx | 14 +- frontend/src/pages/commentsView.tsx | 2 +- frontend/src/pages/editArticleView.test.tsx | 25 +- frontend/src/pages/editArticleView.tsx | 43 +- frontend/src/pages/seoView.tsx | 2 +- server/docs/docs.go | 23 +- server/docs/swagger.json | 23 +- server/docs/swagger.yaml | 16 + .../internal/database/article_categories.go | 6 - server/internal/database/crud.go | 6 +- server/internal/database/http_models.go | 105 ++++ server/internal/handlers/article_edit_lock.go | 35 +- .../article_patch_integration_test.go | 320 +++++++++++- server/internal/handlers/handlers.go | 472 +++++++++++++++--- server/internal/handlers/handlers_test.go | 13 +- server/internal/models/api_responses.go | 5 + 18 files changed, 999 insertions(+), 128 deletions(-) diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 91bcdff..81b539a 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -120,8 +120,10 @@ export default function App() { } /> } /> + } /> } /> } /> + } /> } /> } /> } /> diff --git a/frontend/src/pages/DashboardPage.tsx b/frontend/src/pages/DashboardPage.tsx index 8b3c5d5..2a72e67 100644 --- a/frontend/src/pages/DashboardPage.tsx +++ b/frontend/src/pages/DashboardPage.tsx @@ -35,6 +35,9 @@ interface RecentArticle { published_date: string | null } +const editArticlePath = (article: Pick) => + `/articles/${encodeURIComponent(String(article.id))}/${encodeURIComponent(article.slug)}/edit` + interface ApiStats { totalArticles: number | null publishedArticles: number | null @@ -244,9 +247,11 @@ export default function DashboardPage() { throw new Error(`Save failed (${response.status})`) } + const created = (await response.json().catch(() => null)) as { id?: number | string; slug?: string } | null + const createdSlug = created?.slug || slug let confirmed = false for (let attempt = 0; attempt < 5; attempt += 1) { - const verifyResponse = await apiFetch(`/v1/articles/${encodeURIComponent(slug)}`) + const verifyResponse = await apiFetch(`/v1/articles/${encodeURIComponent(createdSlug)}`) if (verifyResponse.ok) { confirmed = true break @@ -270,7 +275,11 @@ export default function DashboardPage() { } } - navigate(`/articles/${encodeURIComponent(slug)}/edit`) + if (created?.id !== undefined && created.id !== null) { + navigate(`/articles/${encodeURIComponent(String(created.id))}/${encodeURIComponent(createdSlug)}/edit`) + } else { + navigate(`/articles/${encodeURIComponent(createdSlug)}/edit`) + } } catch (err) { const message = err instanceof Error ? err.message : "Unable to save draft." setDraftError(message) @@ -436,7 +445,7 @@ export default function DashboardPage() { navigate(`/articles/${article.slug}/edit`)} + onClick={() => navigate(editArticlePath(article))} > {article.title} diff --git a/frontend/src/pages/articleView.tsx b/frontend/src/pages/articleView.tsx index e3828f8..2e986ff 100644 --- a/frontend/src/pages/articleView.tsx +++ b/frontend/src/pages/articleView.tsx @@ -488,10 +488,10 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article const effectiveTotalCount = Math.max(totalArticleCount, (page * pageSize) + articles.length) const totalPages = Math.max(1, Math.ceil(effectiveTotalCount / pageSize)) const listLabel = pageTitle.toLowerCase() - const editPathForSlug = (slug: string) => + const editPathForArticle = (item: ArticleItem) => fixedType === "developing-stories" - ? `/developing-stories/${encodeURIComponent(slug)}/edit` - : `/articles/${encodeURIComponent(slug)}/edit` + ? `/developing-stories/${encodeURIComponent(item.id)}/${encodeURIComponent(item.slug ?? "")}/edit` + : `/articles/${encodeURIComponent(item.id)}/${encodeURIComponent(item.slug ?? "")}/edit` // subsectionRows walks the section, then each subsection on the trail, and // keeps the levels that actually have children to offer. @@ -562,7 +562,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article setDeleteError(null) setDeletingArticleId(item.id) try { - const response = await apiFetch(`/v1/articles/${encodeURIComponent(item.slug)}`, { + const response = await apiFetch(`/v1/articles/${encodeURIComponent(item.slug)}?id=${encodeURIComponent(item.id)}`, { method: "DELETE", }) if (!response.ok) { @@ -589,7 +589,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article setDeleteError(null) setDeletingArticleId(item.id) try { - const response = await apiFetch(`/v1/articles/${encodeURIComponent(item.slug)}/restore`, { + const response = await apiFetch(`/v1/articles/${encodeURIComponent(item.slug)}/restore?id=${encodeURIComponent(item.id)}`, { method: "PATCH", }) if (!response.ok) { @@ -830,7 +830,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article {item.title} @@ -883,7 +883,7 @@ function ArticleView({ pageTitle = "Articles", fixedType, excludeType }: Article disabled={!item.slug} onClick={() => { if (!item.slug) return - navigate(editPathForSlug(item.slug)) + navigate(editPathForArticle(item)) }} title={item.slug ? "Edit" : "Edit unavailable"} type="button" diff --git a/frontend/src/pages/commentsView.tsx b/frontend/src/pages/commentsView.tsx index 9a3061b..2288915 100644 --- a/frontend/src/pages/commentsView.tsx +++ b/frontend/src/pages/commentsView.tsx @@ -454,7 +454,7 @@ export default function CommentsView() { {comments.map((comment) => { const actionsDisabled = busyCommentId !== null const hasMappedArticle = comment.article_slug && comment.article_id > 0 - const articlePath = hasMappedArticle ? `/articles/${encodeURIComponent(comment.article_slug)}/edit` : "" + const articlePath = hasMappedArticle ? `/articles/${encodeURIComponent(String(comment.article_id))}/${encodeURIComponent(comment.article_slug)}/edit` : "" const publicPath = comment.article_slug ? `${siteUrl}/article/${comment.article_slug}` : "" return ( diff --git a/frontend/src/pages/editArticleView.test.tsx b/frontend/src/pages/editArticleView.test.tsx index 76ba91d..e21303c 100644 --- a/frontend/src/pages/editArticleView.test.tsx +++ b/frontend/src/pages/editArticleView.test.tsx @@ -105,12 +105,15 @@ vi.mock("../components/SeoAnalysis", () => ({ default: () => null })) const patchCalls = () => apiCalls.filter((call) => call.method === "PATCH") -const renderEditor = async () => { +const renderEditor = async ( + initialEntry = `/articles/${ARTICLE_SLUG}/edit`, + routePath = "/articles/:slug/edit", +) => { const user = userEvent.setup({ advanceTimers: vi.advanceTimersByTime }) render( - + - } /> + } /> , ) @@ -168,6 +171,22 @@ describe("EditArticleView autosave", () => { expect(patches[0].body?.content).toContain("and more") }) + it("loads and saves by id when the editor route carries one", async () => { + const user = await renderEditor( + `/articles/42/${ARTICLE_SLUG}/edit`, + "/articles/:id/:slug/edit", + ) + + expect(apiCalls.some((call) => call.method === "GET" && call.url === `/v1/articles/${ARTICLE_SLUG}?id=42`)).toBe(true) + + await user.type(screen.getByTestId("article-body"), " and more") + await waitOutAutosave() + + const patches = patchCalls() + expect(patches).toHaveLength(1) + expect(patches[0].url).toBe(`/v1/articles/${ARTICLE_SLUG}?id=42`) + }) + it("does not unpublish a live article when the editor only selects Draft", async () => { articleStatus = "published" articlePublishedDate = "2025-03-04T15:30:00Z" diff --git a/frontend/src/pages/editArticleView.tsx b/frontend/src/pages/editArticleView.tsx index f95f08a..3ffd6f3 100644 --- a/frontend/src/pages/editArticleView.tsx +++ b/frontend/src/pages/editArticleView.tsx @@ -269,8 +269,11 @@ const clearArticleListCache = () => { function EditArticleView() { const navigate = useNavigate() const apiFetch = useApiFetch() - const { slug: rawSlug } = useParams<{ slug: string }>() + const { id: rawID, slug: rawSlug } = useParams<{ id?: string; slug: string }>() const slug = useMemo(() => (rawSlug ? decodeURIComponent(rawSlug) : ""), [rawSlug]) + const articleID = useMemo(() => (rawID && /^\d+$/.test(rawID) ? rawID : ""), [rawID]) + const articleQuery = articleID ? `?id=${encodeURIComponent(articleID)}` : "" + const articleApiPath = slug ? `/v1/articles/${encodeURIComponent(slug)}${articleQuery}` : "" const isNew = !rawSlug const [slugInput, setSlugInput] = useState("") @@ -423,7 +426,7 @@ function EditArticleView() { setError(null) setSuccessMessage(null) try { - const response = await apiFetch(`/v1/articles/${encodeURIComponent(slug)}`) + const response = await apiFetch(articleApiPath) if (!response.ok) { throw new Error(await readErrorMessage(response, `Could not load article (${response.status})`)) } @@ -496,7 +499,7 @@ function EditArticleView() { return () => { cancelled = true } - }, [apiFetch, slug, isNew]) + }, [apiFetch, articleApiPath, slug, isNew]) // Try to claim an advisory edit lock while this article is open. If someone // else already holds it we surface who, block editing, and keep re-checking @@ -507,7 +510,7 @@ function EditArticleView() { if (isNew || !slug) return setLockChecking(true) try { - const response = await apiFetch(`/v1/articles/${encodeURIComponent(slug)}/edit-lock`, { method: "PUT" }) + const response = await apiFetch(`/v1/articles/${encodeURIComponent(slug)}/edit-lock${articleQuery}`, { method: "PUT" }) if (response.status === 409) { const payload = (await response.json().catch(() => null)) as { holder_name?: string } | null setLockedBy(payload?.holder_name?.trim() || "another editor") @@ -520,7 +523,7 @@ function EditArticleView() { } finally { setLockChecking(false) } - }, [apiFetch, slug, isNew]) + }, [apiFetch, articleQuery, slug, isNew]) useEffect(() => { if (isNew || !slug) return @@ -532,7 +535,7 @@ function EditArticleView() { const release = () => { if (released) return released = true - void apiFetch(`/v1/articles/${encodeURIComponent(slug)}/edit-lock`, { + void apiFetch(`/v1/articles/${encodeURIComponent(slug)}/edit-lock${articleQuery}`, { method: "DELETE", keepalive: true, }).catch(() => {}) @@ -544,7 +547,7 @@ function EditArticleView() { window.removeEventListener("beforeunload", release) release() } - }, [apiFetch, slug, isNew, acquireLock]) + }, [apiFetch, articleQuery, slug, isNew, acquireLock]) useEffect(() => { let cancelled = false @@ -817,11 +820,21 @@ function EditArticleView() { body: JSON.stringify(createPayload), }) if (!response.ok) { - throw new Error(`Create failed (${response.status})`) + throw new Error(await readErrorMessage(response, `Create failed (${response.status})`)) } clearArticleListCache() setSuccessMessage("Article created.") - navigate("/articles") + // The server owns the slug: a title or slug another article already uses + // comes back with a suffix. Land on the new article rather than the list + // so the editor is looking at the slug that was actually stored. + const created = (await response.json().catch(() => null)) as { id?: number; slug?: string } | null + if (created?.id && created.slug) { + navigate(`/articles/${encodeURIComponent(String(created.id))}/${encodeURIComponent(created.slug)}/edit`, { + replace: true, + }) + } else { + navigate("/articles") + } return } @@ -856,7 +869,7 @@ function EditArticleView() { noindex: noIndex, } - const response = await apiFetch(`/v1/articles/${encodeURIComponent(slug)}`, { + const response = await apiFetch(articleApiPath, { method: "PATCH", headers: { "Content-Type": "application/json", @@ -864,7 +877,10 @@ function EditArticleView() { body: JSON.stringify(payload), }) if (!response.ok) { - throw new Error(`Save failed (${response.status})`) + // A rename onto a slug another article holds comes back 409 with a + // message worth showing: the generic text would read as a save that + // failed for no reason. + throw new Error(await readErrorMessage(response, `Save failed (${response.status})`)) } if (nextTiming) { setPublishTiming(nextTiming) @@ -886,7 +902,8 @@ function EditArticleView() { // The route still points at the old slug, which no longer resolves, so // move the editor onto the new one before anything refetches. setPendingSlug(null) - navigate(`/articles/${encodeURIComponent(slugToSave)}/edit`, { replace: true }) + const basePath = articleID ? `/articles/${encodeURIComponent(articleID)}` : "/articles" + navigate(`${basePath}/${encodeURIComponent(slugToSave)}/edit`, { replace: true }) } } catch (err) { const message = err instanceof Error ? err.message : "Unable to save article." @@ -918,7 +935,7 @@ function EditArticleView() { }, AUTOSAVE_DELAY_MS) return () => window.clearTimeout(timer) - }, [articleSnapshot, isAutoSaving, isLoading, isNew, isSaving, lockedBy, selectedAuthorIds, selectedCategorySlugs]) + }, [articleApiPath, articleID, articleSnapshot, isAutoSaving, isLoading, isNew, isSaving, lockedBy, selectedAuthorIds, selectedCategorySlugs]) const inputClass ="w-full px-3 py-2 rounded-lg border border-border bg-background text-sm text-foreground placeholder:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-primary/40 focus:border-primary transition" const selectClass = "w-full px-3 py-2 rounded-lg border border-border bg-background text-sm text-foreground focus:outline-none focus:ring-2 focus:ring-primary/40 focus:border-primary transition" diff --git a/frontend/src/pages/seoView.tsx b/frontend/src/pages/seoView.tsx index b1d1d43..7ee97d5 100644 --- a/frontend/src/pages/seoView.tsx +++ b/frontend/src/pages/seoView.tsx @@ -213,7 +213,7 @@ export default function SeoView() {