From 9508fb8e86e25dc191064b33a76adc5a4216f55a Mon Sep 17 00:00:00 2001 From: MechaCat02 Date: Sat, 11 Jul 2026 14:08:27 +0200 Subject: [PATCH] fix: make bookmark add idempotent instead of 409 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-adding an already-bookmarked manga returned 409, which the UI surfaced as a false "Could not add bookmark" toast — collections are idempotent, bookmarks weren't. On a unique violation, fetch and return the existing bookmark, and answer 200 (vs 201 for a fresh insert). The frontend toggle already treats a successful POST as done, so the false toast disappears with no client change. Co-Authored-By: Claude Opus 4.8 (1M context) --- backend/Cargo.lock | 2 +- backend/Cargo.toml | 2 +- backend/src/api/bookmarks.rs | 10 +++++++-- backend/src/repo/bookmark.rs | 37 +++++++++++++++++++++++++++++----- backend/tests/api_bookmarks.rs | 31 +++++++++++++++++++++------- frontend/package.json | 2 +- 6 files changed, 67 insertions(+), 17 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index 93679d3..a8a0910 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4" [[package]] name = "mangalord" -version = "0.124.20" +version = "0.124.21" dependencies = [ "anyhow", "argon2", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 52f3cb0..66fb869 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mangalord" -version = "0.124.20" +version = "0.124.21" edition = "2021" default-run = "mangalord" diff --git a/backend/src/api/bookmarks.rs b/backend/src/api/bookmarks.rs index 1f424f3..90d3bda 100644 --- a/backend/src/api/bookmarks.rs +++ b/backend/src/api/bookmarks.rs @@ -72,7 +72,7 @@ async fn create( } } - let bookmark = repo::bookmark::create( + let (bookmark, created) = repo::bookmark::create( &state.db, user.id, input.manga_id, @@ -103,7 +103,13 @@ async fn create( } }); - Ok((StatusCode::CREATED, Json(bookmark))) + // 201 for a fresh bookmark, 200 when it already existed (idempotent add). + let status = if created { + StatusCode::CREATED + } else { + StatusCode::OK + }; + Ok((status, Json(bookmark))) } async fn delete_one( diff --git a/backend/src/repo/bookmark.rs b/backend/src/repo/bookmark.rs index 21dbb0b..3064e93 100644 --- a/backend/src/repo/bookmark.rs +++ b/backend/src/repo/bookmark.rs @@ -6,13 +6,23 @@ use uuid::Uuid; use crate::domain::{Bookmark, BookmarkSummary}; use crate::error::{AppError, AppResult}; +/// Add a bookmark, idempotently. Returns the bookmark plus whether it was +/// newly created (`true`) or already existed (`false`), so the handler can +/// answer 201 vs 200. Re-adding an existing bookmark is a no-op success rather +/// than a 409 — matching the idempotent collection semantics, so the UI doesn't +/// show a false "Could not add bookmark" toast when the manga is already saved. +/// +/// Uniqueness is per `(user_id, manga_id, chapter_id)` — enforced by the 0001 +/// constraint for chapter-level rows and the 0004 partial index for manga-level +/// (NULL chapter) rows. `page` is not part of the key, so the existing row is +/// returned unchanged (its page is not overwritten). pub async fn create( pool: &PgPool, user_id: Uuid, manga_id: Uuid, chapter_id: Option, page: Option, -) -> AppResult { +) -> AppResult<(Bookmark, bool)> { let result = sqlx::query_as::<_, Bookmark>( r#" INSERT INTO bookmarks (user_id, manga_id, chapter_id, page) @@ -28,10 +38,27 @@ pub async fn create( .await; match result { - Ok(b) => Ok(b), - Err(sqlx::Error::Database(ref db_err)) if db_err.is_unique_violation() => Err( - AppError::Conflict("bookmark already exists for this manga/chapter".into()), - ), + Ok(b) => Ok((b, true)), + Err(sqlx::Error::Database(ref db_err)) if db_err.is_unique_violation() => { + // A bookmark for this (user, manga, chapter) already exists — fetch + // and return it. `IS NOT DISTINCT FROM` matches a NULL chapter_id + // (manga-level bookmark) as well as a concrete one, covering both + // uniqueness paths with a single lookup. + let existing = sqlx::query_as::<_, Bookmark>( + r#" + SELECT id, user_id, manga_id, chapter_id, page, created_at + FROM bookmarks + WHERE user_id = $1 AND manga_id = $2 + AND chapter_id IS NOT DISTINCT FROM $3 + "#, + ) + .bind(user_id) + .bind(manga_id) + .bind(chapter_id) + .fetch_one(pool) + .await?; + Ok((existing, false)) + } Err(e) => Err(AppError::Database(e)), } } diff --git a/backend/tests/api_bookmarks.rs b/backend/tests/api_bookmarks.rs index 5a2e6a3..65491b6 100644 --- a/backend/tests/api_bookmarks.rs +++ b/backend/tests/api_bookmarks.rs @@ -52,7 +52,7 @@ async fn create_then_list_returns_only_own(pool: PgPool) { } #[sqlx::test(migrations = "./migrations")] -async fn create_returns_409_on_duplicate_manga_level(pool: PgPool) { +async fn create_is_idempotent_on_duplicate_manga_level(pool: PgPool) { let h = common::harness(pool); let (_, cookie) = common::register_user(&h.app).await; let manga_id = common::seed_manga_via_api(&h.app, &cookie, "Berserk").await; @@ -64,12 +64,26 @@ async fn create_returns_409_on_duplicate_manga_level(pool: PgPool) { &cookie, ) }; + // First add creates (201); re-adding is an idempotent no-op (200) that + // returns the SAME bookmark rather than a 409 — collections behave this way + // too, so the UI shouldn't surface a false "Could not add bookmark" error. let first = h.app.clone().oneshot(make()).await.unwrap(); assert_eq!(first.status(), StatusCode::CREATED); - let second = h.app.oneshot(make()).await.unwrap(); - assert_eq!(second.status(), StatusCode::CONFLICT); - let body = common::body_json(second).await; - assert_eq!(body["error"]["code"], "conflict"); + let first_body = common::body_json(first).await; + + let second = h.app.clone().oneshot(make()).await.unwrap(); + assert_eq!(second.status(), StatusCode::OK); + let second_body = common::body_json(second).await; + assert_eq!(second_body["id"], first_body["id"], "same bookmark returned"); + + // Exactly one row exists. + let list = h + .app + .oneshot(common::get_with_cookie("/api/v1/me/bookmarks", &cookie)) + .await + .unwrap(); + let body = common::body_json(list).await; + assert_eq!(body["items"].as_array().unwrap().len(), 1); } #[sqlx::test(migrations = "./migrations")] @@ -225,13 +239,16 @@ async fn concurrent_manga_bookmarks_serialised_by_unique_index(pool: PgPool) { let (s1, s2) = tokio::join!(f1, f2); let statuses = [s1.unwrap(), s2.unwrap()]; + // The unique index serialises the two inserts: one wins with 201, the other + // hits the violation and — now idempotent — returns the existing bookmark + // with 200 rather than a 409. assert!( statuses.contains(&StatusCode::CREATED), "expected one winner with 201, got {statuses:?}" ); assert!( - statuses.contains(&StatusCode::CONFLICT), - "expected one loser with 409 (the partial unique index), got {statuses:?}" + statuses.contains(&StatusCode::OK), + "expected the loser to return 200 (idempotent), got {statuses:?}" ); } diff --git a/frontend/package.json b/frontend/package.json index b7a8469..c6d614c 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "mangalord-frontend", - "version": "0.124.20", + "version": "0.124.21", "private": true, "type": "module", "scripts": {