From 27b3dcb69f24adadb63944282a3cd0a2c47810bb Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sun, 30 Aug 2026 20:57:15 -0400 Subject: [PATCH] fix(bookmarks): upsert on title conflict so position upgrades don't 500 Bookmark dedup is keyed on hash(title + position bucket), but the table also enforces UNIQUE(media_item_id, user_id, title). When a client re- saves the same bookmark title with a changed position form - e.g. the Android app upgrading a percentage-only row to an EPUB CFI, or a web and app bookmark landing on the same 'Bookmark at 44%' title - the dedup-key lookup misses and the INSERT violates the title constraint, returning HTTP 500 and failing the sync. A title collision on the same (user, item) is by definition the same bookmark slot, so take the LWW semantics all the way: ON CONFLICT DO UPDATE replaces position/cfi_position/page/chapter/percentage, refreshes dedup_key and timestamps, merges device_sync_data, and - matching UpdateMediaBookmarkForSync - clears deleted/deleted_at so a re-create resurrects a tombstoned title slot instead of leaving an invisible row holding it. Device sync flows are unaffected: KOReader/Kobo pushes that carry their own dedup-key echoes never reach the INSERT, and same-key saves still go through applyBookmarkLWW with its tombstone freshness checks. --- internal/database/db.go | 2 +- internal/database/models.go | 2 +- internal/database/querier.go | 6 +++++- internal/database/queries.sql.go | 24 ++++++++++++++++++++++-- internal/database/queries/queries.sql | 22 +++++++++++++++++++++- 5 files changed, 50 insertions(+), 6 deletions(-) diff --git a/internal/database/db.go b/internal/database/db.go index bdf4241..486aa36 100644 --- a/internal/database/db.go +++ b/internal/database/db.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.30.0 +// sqlc v1.31.1 package database diff --git a/internal/database/models.go b/internal/database/models.go index 1d937f6..0e7affe 100644 --- a/internal/database/models.go +++ b/internal/database/models.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.30.0 +// sqlc v1.31.1 package database diff --git a/internal/database/querier.go b/internal/database/querier.go index 2a54c98..f3aac13 100644 --- a/internal/database/querier.go +++ b/internal/database/querier.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.30.0 +// sqlc v1.31.1 package database @@ -60,6 +60,10 @@ type Querier interface { // Libraries queries CreateLibrary(ctx context.Context, arg CreateLibraryParams) (Libraries, error) CreateMediaBookmark(ctx context.Context, arg CreateMediaBookmarkParams) (MediaBookmarks, error) + // The dedup key can change while the title stays the same (e.g. a client + // upgrading a bookmark from percentage-only to a CFI position), so the + // title UNIQUE constraint races the dedup-key lookup. Upsert on the title + // slot: the conflicting row is by definition the same bookmark. CreateMediaBookmarkFull(ctx context.Context, arg CreateMediaBookmarkFullParams) (MediaBookmarks, error) // Media Highlights queries CreateMediaHighlight(ctx context.Context, arg CreateMediaHighlightParams) (MediaHighlights, error) diff --git a/internal/database/queries.sql.go b/internal/database/queries.sql.go index 1541229..c33c690 100644 --- a/internal/database/queries.sql.go +++ b/internal/database/queries.sql.go @@ -1,6 +1,6 @@ // Code generated by sqlc. DO NOT EDIT. // versions: -// sqlc v1.30.0 +// sqlc v1.31.1 // source: queries.sql package database @@ -683,7 +683,23 @@ INSERT INTO media_bookmarks ( device_sync_data ) VALUES ( $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15 -) RETURNING id, media_item_id, user_id, page_number, chapter_number, cfi_position, title, position, notes, created_at, dedup_key, last_modified_at, last_modified_source, device_sync_data, percentage_location, epubcfi_location, chapter_reference, deleted, deleted_at +) ON CONFLICT (media_item_id, user_id, title) DO UPDATE SET + page_number = EXCLUDED.page_number, + chapter_number = EXCLUDED.chapter_number, + cfi_position = EXCLUDED.cfi_position, + position = EXCLUDED.position, + notes = EXCLUDED.notes, + percentage_location = EXCLUDED.percentage_location, + epubcfi_location = EXCLUDED.epubcfi_location, + chapter_reference = EXCLUDED.chapter_reference, + dedup_key = EXCLUDED.dedup_key, + last_modified_at = EXCLUDED.last_modified_at, + last_modified_source = EXCLUDED.last_modified_source, + device_sync_data = EXCLUDED.device_sync_data, + created_at = created_at, + deleted = FALSE, + deleted_at = NULL +RETURNING id, media_item_id, user_id, page_number, chapter_number, cfi_position, title, position, notes, created_at, dedup_key, last_modified_at, last_modified_source, device_sync_data, percentage_location, epubcfi_location, chapter_reference, deleted, deleted_at ` type CreateMediaBookmarkFullParams struct { @@ -704,6 +720,10 @@ type CreateMediaBookmarkFullParams struct { DeviceSyncData []byte `db:"device_sync_data" json:"device_sync_data"` } +// The dedup key can change while the title stays the same (e.g. a client +// upgrading a bookmark from percentage-only to a CFI position), so the +// title UNIQUE constraint races the dedup-key lookup. Upsert on the title +// slot: the conflicting row is by definition the same bookmark. func (q *Queries) CreateMediaBookmarkFull(ctx context.Context, arg CreateMediaBookmarkFullParams) (MediaBookmarks, error) { row := q.db.QueryRow(ctx, CreateMediaBookmarkFull, arg.MediaItemID, diff --git a/internal/database/queries/queries.sql b/internal/database/queries/queries.sql index 8681a0b..1270536 100644 --- a/internal/database/queries/queries.sql +++ b/internal/database/queries/queries.sql @@ -893,6 +893,10 @@ ORDER BY deleted ASC, deleted_at DESC NULLS LAST LIMIT 1; -- name: CreateMediaBookmarkFull :one +-- The dedup key can change while the title stays the same (e.g. a client +-- upgrading a bookmark from percentage-only to a CFI position), so the +-- title UNIQUE constraint races the dedup-key lookup. Upsert on the title +-- slot: the conflicting row is by definition the same bookmark. INSERT INTO media_bookmarks ( media_item_id, user_id, page_number, chapter_number, cfi_position, title, position, notes, @@ -901,7 +905,23 @@ INSERT INTO media_bookmarks ( device_sync_data ) VALUES ( $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15 -) RETURNING *; +) ON CONFLICT (media_item_id, user_id, title) DO UPDATE SET + page_number = EXCLUDED.page_number, + chapter_number = EXCLUDED.chapter_number, + cfi_position = EXCLUDED.cfi_position, + position = EXCLUDED.position, + notes = EXCLUDED.notes, + percentage_location = EXCLUDED.percentage_location, + epubcfi_location = EXCLUDED.epubcfi_location, + chapter_reference = EXCLUDED.chapter_reference, + dedup_key = EXCLUDED.dedup_key, + last_modified_at = EXCLUDED.last_modified_at, + last_modified_source = EXCLUDED.last_modified_source, + device_sync_data = EXCLUDED.device_sync_data, + created_at = created_at, + deleted = FALSE, + deleted_at = NULL +RETURNING *; -- name: UpdateMediaBookmarkForSync :one UPDATE media_bookmarks SET