From 11609d13d153c8f004ac3228e5e828b4cc97937e Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 17 Oct 2026 15:56:24 -0400 Subject: [PATCH] refactor(annotations): one LWW decision core + one tombstone gate + typed queue decoders MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The consolidation half of H3 — shared LOGIC with thin per-type remains: - lwwDecision: the three compareIncoming* functions were the same skeleton (client timestamp -> strictly-newer wins; no timestamp -> any content difference wins) drifting apart only in per-type equality and the schema-driven fallback timestamp (bookmarks carry no updated_at). The decision now lives in one function; each compare keeps only its equality expression. - idTargetOwnership + tombstoneWins: the id-targeted resolution blocks (HighlightID/BookmarkID/NoteID) shared the ownership check and the deleted-row gate — now one gate each instead of three copies of the resurrect semantics. - queue.go: syncNote/syncHighlight/syncBookmark decoded device payloads through three 10-key if-type-assertion chains; typed map accessors (mapStr/mapF64/mapI32/mapDeviceSyncData) replace them — same missing-key-is-zero semantics, declarative field lists. The progress decoder is untouched (H1 territory, separately tested). Per-type remains are the things that genuinely cannot merge: request structs, dedup triples, the per-table sqlc param builders, and the KOReader/Kobo payload adapters. Pins, H1/H3 tests, both unit suites green. --- internal/sync/annotations.go | 121 +++++++++++++------------- internal/sync/queue.go | 160 +++++++++++++---------------------- 2 files changed, 120 insertions(+), 161 deletions(-) diff --git a/internal/sync/annotations.go b/internal/sync/annotations.go index 571cc4c1..2ad0f12b 100644 --- a/internal/sync/annotations.go +++ b/internal/sync/annotations.go @@ -95,6 +95,22 @@ type SaveHighlightResult struct { Conflict bool } +// idTargetOwnership verifies an id-targeted edit's ownership: the URL row +// must belong to the requesting user and media item. +func idTargetOwnership(rowUser, rowItem, reqUser, reqItem pgtype.UUID, kind string, id pgtype.UUID) error { + if rowUser != reqUser || rowItem != reqItem { + return fmt.Errorf("%s %s belongs to another user or media item", kind, id) + } + return nil +} + +// tombstoneWins is the shared tombstone gate for id-targeted edits: a +// deleted row stays deleted unless the edit is newer than the tombstone +// (a deliberate re-create — the LWW update then resurrects the row). +func tombstoneWins(deleted bool, deletedAt, lastModifiedAt pgtype.Timestamptz, modifiedAt time.Time) bool { + return deleted && !incomingNewerThanTombstone(modifiedAt, deletedAt, lastModifiedAt) +} + func (s *AnnotationService) SaveHighlight(ctx context.Context, req SaveHighlightRequest) (*SaveHighlightResult, error) { dedupKey := req.DedupKey @@ -108,18 +124,14 @@ func (s *AnnotationService) SaveHighlight(ctx context.Context, req SaveHighlight return nil, fmt.Errorf("query highlight by id: %w", idErr) } if idErr == nil { - if byID.UserID != req.UserID || byID.MediaItemID != req.MediaItemID { - return nil, fmt.Errorf("highlight %s belongs to another user or media item", req.HighlightID) + if err := idTargetOwnership(byID.UserID, byID.MediaItemID, req.UserID, req.MediaItemID, "highlight", req.HighlightID); err != nil { + return nil, err } if dedupKey == "" { dedupKey = byID.DedupKey.String } - if byID.Deleted.Bool { - if !incomingNewerThanTombstone(req.ModifiedAt, byID.DeletedAt, byID.LastModifiedAt) { - return &SaveHighlightResult{Highlight: byID, Outcome: SaveOutcomeDeleted}, nil - } - // Newer than the tombstone: a deliberate re-create. Resurrect - // via the LWW update (which clears deleted/deleted_at). + if tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) { + return &SaveHighlightResult{Highlight: byID, Outcome: SaveOutcomeDeleted}, nil } return s.applyLWW(ctx, req, byID, dedupKey) } @@ -275,22 +287,14 @@ func (s *AnnotationService) applyLWW( } func (s *AnnotationService) compareIncoming(req SaveHighlightRequest, existing database.MediaHighlights) (incomingNewer bool, contentChanged bool) { - if req.ModifiedAt.IsZero() { - contentSame := strings.EqualFold(req.SelectionText, existing.SelectionText) && - textEq(req.Color, existing.Color) && - textEq(req.NoteText, existing.NoteText) && - floatEq(req.PercentageStart, existing.PercentageStart) && - floatEq(req.PercentageEnd, existing.PercentageEnd) && - locatorRefresher(req.EpubcfiStart, existing.EpubcfiStart) && - locatorRefresher(req.EpubcfiEnd, existing.EpubcfiEnd) - return !contentSame, !contentSame - } - - existingMod := existing.LastModifiedAt - if !existingMod.Valid { - existingMod = existing.UpdatedAt - } - return req.ModifiedAt.After(existingMod.Time), true + contentSame := strings.EqualFold(req.SelectionText, existing.SelectionText) && + textEq(req.Color, existing.Color) && + textEq(req.NoteText, existing.NoteText) && + floatEq(req.PercentageStart, existing.PercentageStart) && + floatEq(req.PercentageEnd, existing.PercentageEnd) && + locatorRefresher(req.EpubcfiStart, existing.EpubcfiStart) && + locatorRefresher(req.EpubcfiEnd, existing.EpubcfiEnd) + return lwwDecision(req.ModifiedAt, existing.LastModifiedAt, existing.UpdatedAt, contentSame) } // locatorRefresher reports whether an incoming locator leaves the stored @@ -551,8 +555,8 @@ func (s *AnnotationService) SaveNote(ctx context.Context, req SaveNoteRequest) ( return nil, fmt.Errorf("query note by id: %w", idErr) } if idErr == nil { - if byID.UserID != req.UserID || byID.MediaItemID != req.MediaItemID { - return nil, fmt.Errorf("note %s belongs to another user or media item", req.NoteID) + if err := idTargetOwnership(byID.UserID, byID.MediaItemID, req.UserID, req.MediaItemID, "note", req.NoteID); err != nil { + return nil, err } if dedupKey == "" { dedupKey = byID.DedupKey.String @@ -575,12 +579,8 @@ func (s *AnnotationService) SaveNote(ctx context.Context, req SaveNoteRequest) ( if req.ParagraphReference == 0 { req.ParagraphReference = byID.ParagraphReference.Int32 } - if byID.Deleted.Bool { - if !incomingNewerThanTombstone(req.ModifiedAt, byID.DeletedAt, byID.LastModifiedAt) { - return &SaveNoteResult{Note: byID, Outcome: SaveOutcomeDeleted}, nil - } - // Newer than the tombstone: a deliberate re-create. Resurrect - // via the LWW update (which clears deleted/deleted_at). + if tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) { + return &SaveNoteResult{Note: byID, Outcome: SaveOutcomeDeleted}, nil } return s.applyNoteLWW(ctx, req, byID, dedupKey) } @@ -692,20 +692,28 @@ func (s *AnnotationService) applyNoteLWW(ctx context.Context, req SaveNoteReques return &SaveNoteResult{Note: note, Outcome: SaveOutcomeUpdated, Conflict: conflict}, nil } -func (s *AnnotationService) compareIncomingNote(req SaveNoteRequest, existing database.MediaNotes) (incomingNewer bool, contentChanged bool) { - if req.ModifiedAt.IsZero() { - contentSame := strings.EqualFold(req.Content, existing.Content) && - textEq(req.Position, existing.Position) +// lwwDecision is the shared LWW decision core for all three annotation +// stacks: with a client timestamp, strictly-newer wins (and content is +// treated as changed — the update re-lands everything); without one, any +// content difference is "newer" and identical content resolves to skip. +// The fallback timestamp is schema-driven per stack (rows with an +// updated_at fall back to it; bookmarks only carry created_at). +func lwwDecision(modifiedAt time.Time, existingLastMod, existingFallback pgtype.Timestamptz, contentSame bool) (incomingNewer bool, contentChanged bool) { + if modifiedAt.IsZero() { return !contentSame, !contentSame } - existingMod := existing.LastModifiedAt + existingMod := existingLastMod if !existingMod.Valid { - existingMod = existing.UpdatedAt + existingMod = existingFallback } - if !existingMod.Valid { - return true, true - } - return req.ModifiedAt.After(existingMod.Time), true + return modifiedAt.After(existingMod.Time), true +} + +// compareIncomingNote is compareIncoming for the note stack. +func (s *AnnotationService) compareIncomingNote(req SaveNoteRequest, existing database.MediaNotes) (incomingNewer bool, contentChanged bool) { + contentSame := strings.EqualFold(req.Content, existing.Content) && + textEq(req.Position, existing.Position) + return lwwDecision(req.ModifiedAt, existing.LastModifiedAt, existing.UpdatedAt, contentSame) } func (s *AnnotationService) TombstoneNoteByID(ctx context.Context, id, userID pgtype.UUID) error { @@ -764,8 +772,8 @@ func (s *AnnotationService) SaveBookmark(ctx context.Context, req SaveBookmarkRe return nil, fmt.Errorf("query bookmark by id: %w", idErr) } if idErr == nil { - if byID.UserID != req.UserID || byID.MediaItemID != req.MediaItemID { - return nil, fmt.Errorf("bookmark %s belongs to another user or media item", req.BookmarkID) + if err := idTargetOwnership(byID.UserID, byID.MediaItemID, req.UserID, req.MediaItemID, "bookmark", req.BookmarkID); err != nil { + return nil, err } if dedupKey == "" { dedupKey = byID.DedupKey.String @@ -788,12 +796,8 @@ func (s *AnnotationService) SaveBookmark(ctx context.Context, req SaveBookmarkRe if req.ChapterReference == 0 { req.ChapterReference = byID.ChapterReference.Int32 } - if byID.Deleted.Bool { - if !incomingNewerThanTombstone(req.ModifiedAt, byID.DeletedAt, byID.LastModifiedAt) { - return &SaveBookmarkResult{Bookmark: byID, Outcome: SaveOutcomeDeleted}, nil - } - // Newer than the tombstone: a deliberate re-create. Resurrect - // via the LWW update (which clears deleted/deleted_at). + if tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) { + return &SaveBookmarkResult{Bookmark: byID, Outcome: SaveOutcomeDeleted}, nil } return s.applyBookmarkLWW(ctx, req, byID, dedupKey) } @@ -923,18 +927,11 @@ func (s *AnnotationService) applyBookmarkLWW(ctx context.Context, req SaveBookma } func (s *AnnotationService) compareIncomingBookmark(req SaveBookmarkRequest, existing database.MediaBookmarks) (incomingNewer bool, contentChanged bool) { - if req.ModifiedAt.IsZero() { - contentSame := strings.EqualFold(req.Title, existing.Title) && - textEq(req.Notes, existing.Notes) && - locatorRefresher(req.CFIPosition, existing.CfiPosition) && - locatorRefresher(req.Position, existing.Position) - return !contentSame, !contentSame - } - existingMod := existing.LastModifiedAt - if !existingMod.Valid { - existingMod = existing.CreatedAt - } - return req.ModifiedAt.After(existingMod.Time), true + contentSame := strings.EqualFold(req.Title, existing.Title) && + textEq(req.Notes, existing.Notes) && + locatorRefresher(req.CFIPosition, existing.CfiPosition) && + locatorRefresher(req.Position, existing.Position) + return lwwDecision(req.ModifiedAt, existing.LastModifiedAt, existing.CreatedAt, contentSame) } func (s *AnnotationService) TombstoneBookmarkByID(ctx context.Context, userID, bookmarkID pgtype.UUID, source string) error { diff --git a/internal/sync/queue.go b/internal/sync/queue.go index 992af511..15b86f24 100644 --- a/internal/sync/queue.go +++ b/internal/sync/queue.go @@ -457,32 +457,42 @@ func (p *SyncQueueProcessor) syncProgress(ctx context.Context, userID pgtype.UUI return err } +// queue-map accessors: syncData entries arrive as JSON-decoded +// interface{} values; a missing key or wrong type is simply the zero +// value (the decoders only carry fields the device actually pushed). +func mapStr(m map[string]interface{}, key string) string { + v, _ := m[key].(string) + return v +} + +func mapF64(m map[string]interface{}, key string) float64 { + v, _ := m[key].(float64) + return v +} + +func mapI32(m map[string]interface{}, key string) int32 { + return int32(mapF64(m, key)) +} + +func mapDeviceSyncData(m map[string]interface{}) json.RawMessage { + if v, ok := m["device_sync_data"].(map[string]interface{}); ok { + raw, _ := json.Marshal(v) + return raw + } + return nil +} + func (p *SyncQueueProcessor) syncNote(ctx context.Context, userID pgtype.UUID, mediaItemID pgtype.UUID, syncData map[string]interface{}) error { req := SaveNoteRequest{ - MediaItemID: mediaItemID, - UserID: userID, - } - - if v, ok := syncData["content"].(string); ok { - req.Content = v - } - if v, ok := syncData["position"].(string); ok { - req.Position = v - } - if v, ok := syncData["source"].(string); ok { - req.Source = v - } - if v, ok := syncData["epubcfi_location"].(string); ok { - req.EpubcfiLocation = v - } - if v, ok := syncData["percentage_location"].(float64); ok { - req.PercentageLocation = v - } - if v, ok := syncData["chapter_reference"].(float64); ok { - req.ChapterReference = int32(v) - } - if v, ok := syncData["device_sync_data"].(map[string]interface{}); ok { - req.DeviceSyncData, _ = json.Marshal(v) + MediaItemID: mediaItemID, + UserID: userID, + Content: mapStr(syncData, "content"), + Position: mapStr(syncData, "position"), + Source: mapStr(syncData, "source"), + EpubcfiLocation: mapStr(syncData, "epubcfi_location"), + PercentageLocation: mapF64(syncData, "percentage_location"), + ChapterReference: mapI32(syncData, "chapter_reference"), + DeviceSyncData: mapDeviceSyncData(syncData), } _, err := p.annotationSvc.SaveNote(ctx, req) @@ -491,45 +501,20 @@ func (p *SyncQueueProcessor) syncNote(ctx context.Context, userID pgtype.UUID, m func (p *SyncQueueProcessor) syncHighlight(ctx context.Context, userID pgtype.UUID, mediaItemID pgtype.UUID, syncData map[string]interface{}) error { req := SaveHighlightRequest{ - MediaItemID: mediaItemID, - UserID: userID, - } - - if v, ok := syncData["selection_text"].(string); ok { - req.SelectionText = v - } - if v, ok := syncData["start_position"].(string); ok { - req.StartPosition = v - } - if v, ok := syncData["end_position"].(string); ok { - req.EndPosition = v - } - if v, ok := syncData["color"].(string); ok { - req.Color = v - } - if v, ok := syncData["note_text"].(string); ok { - req.NoteText = v - } - if v, ok := syncData["source"].(string); ok { - req.Source = v - } - if v, ok := syncData["epubcfi_start"].(string); ok { - req.EpubcfiStart = v - } - if v, ok := syncData["epubcfi_end"].(string); ok { - req.EpubcfiEnd = v - } - if v, ok := syncData["percentage_start"].(float64); ok { - req.PercentageStart = v - } - if v, ok := syncData["percentage_end"].(float64); ok { - req.PercentageEnd = v - } - if v, ok := syncData["chapter_reference"].(float64); ok { - req.ChapterReference = int32(v) - } - if v, ok := syncData["device_sync_data"].(map[string]interface{}); ok { - req.DeviceSyncData, _ = json.Marshal(v) + MediaItemID: mediaItemID, + UserID: userID, + SelectionText: mapStr(syncData, "selection_text"), + StartPosition: mapStr(syncData, "start_position"), + EndPosition: mapStr(syncData, "end_position"), + Color: mapStr(syncData, "color"), + NoteText: mapStr(syncData, "note_text"), + PercentageStart: mapF64(syncData, "percentage_start"), + PercentageEnd: mapF64(syncData, "percentage_end"), + EpubcfiStart: mapStr(syncData, "epubcfi_start"), + EpubcfiEnd: mapStr(syncData, "epubcfi_end"), + ChapterReference: mapI32(syncData, "chapter_reference"), + Source: mapStr(syncData, "source"), + DeviceSyncData: mapDeviceSyncData(syncData), } _, err := p.annotationSvc.SaveHighlight(ctx, req) @@ -538,42 +523,19 @@ func (p *SyncQueueProcessor) syncHighlight(ctx context.Context, userID pgtype.UU func (p *SyncQueueProcessor) syncBookmark(ctx context.Context, userID pgtype.UUID, mediaItemID pgtype.UUID, syncData map[string]interface{}) error { req := SaveBookmarkRequest{ - MediaItemID: mediaItemID, - UserID: userID, - } - - if v, ok := syncData["title"].(string); ok { - req.Title = v - } - if v, ok := syncData["position"].(string); ok { - req.Position = v - } - if v, ok := syncData["notes"].(string); ok { - req.Notes = v - } - if v, ok := syncData["source"].(string); ok { - req.Source = v - } - if v, ok := syncData["cfi_position"].(string); ok { - req.CFIPosition = v - } - if v, ok := syncData["epubcfi_location"].(string); ok { - req.EpubcfiLocation = v - } - if v, ok := syncData["percentage_loc"].(float64); ok { - req.PercentageLoc = v - } - if v, ok := syncData["page_number"].(float64); ok { - req.PageNumber = int32(v) - } - if v, ok := syncData["chapter_number"].(float64); ok { - req.ChapterNumber = int32(v) - } - if v, ok := syncData["chapter_reference"].(float64); ok { - req.ChapterReference = int32(v) - } - if v, ok := syncData["device_sync_data"].(map[string]interface{}); ok { - req.DeviceSyncData, _ = json.Marshal(v) + MediaItemID: mediaItemID, + UserID: userID, + Title: mapStr(syncData, "title"), + Position: mapStr(syncData, "position"), + Notes: mapStr(syncData, "notes"), + Source: mapStr(syncData, "source"), + CFIPosition: mapStr(syncData, "cfi_position"), + EpubcfiLocation: mapStr(syncData, "epubcfi_location"), + PercentageLoc: mapF64(syncData, "percentage_loc"), + PageNumber: mapI32(syncData, "page_number"), + ChapterNumber: mapI32(syncData, "chapter_number"), + ChapterReference: mapI32(syncData, "chapter_reference"), + DeviceSyncData: mapDeviceSyncData(syncData), } _, err := p.annotationSvc.SaveBookmark(ctx, req)