diff --git a/cmd/server/tests/contract_pins_test.go b/cmd/server/tests/contract_pins_test.go index 7eda7468..d9bf1449 100644 --- a/cmd/server/tests/contract_pins_test.go +++ b/cmd/server/tests/contract_pins_test.go @@ -1218,3 +1218,91 @@ func TestContractPin_AnnotationCRUDBookmark(t *testing.T) { }, _WEBBookmarkRowKeys) } + +// TestH3_BookmarkPUTPreservesLocators locks the H3 PUT routing: the web +// bookmark PUT lands through the sync service as an id-targeted LWW edit. +// Fields the PUT omits must be COALESCED with the stored row (the old +// plain query never touched them; a naive full-column sync write would +// have zeroed them), fields it sends must land, and the row's +// source/modified_at bookkeeping must update — the plain query bypassed +// it entirely. +func TestH3_BookmarkPUTPreservesLocators(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + setup := setupTestServer(t) + fx := createContractFixture(t, setup) + url := annotationURL(setup.Server.URL, fx.itemID, "bookmarks", "") + + create := doReq(t, "POST", url, map[string]interface{}{ + "title": "H3 bookmark", + "position": "p1", + "cfi_position": "epubcfi(/6/4/2:30)", + "page_number": 12, + "chapter_number": 3, + "percentage": 0.25, + "chapter_reference": 4, + }, setup.RegularToken) + defer create.Body.Close() + require.Equal(t, http.StatusCreated, create.StatusCode) + created := decodeJSON(t, create) + id, _ := created["id"].(string) + require.NotEmpty(t, id) + + // PUT carrying ONLY the legacy fields: locators must survive. + put := doReq(t, "PUT", annotationURL(setup.Server.URL, fx.itemID, "bookmarks", id), map[string]interface{}{ + "title": "H3 bookmark renamed", + "notes": "edited", + }, setup.RegularToken) + defer put.Body.Close() + raw, _ := io.ReadAll(put.Body) + require.Equal(t, http.StatusOK, put.StatusCode, "body: %s", raw) + var row map[string]interface{} + require.NoError(t, json.Unmarshal(raw, &row)) + require.Equal(t, "H3 bookmark renamed", getStringField(t, row, "title")) + require.Equal(t, "epubcfi(/6/4/2:30)", getStringField(t, row, "cfi_position"), + "an omitted locator must be coalesced, not zeroed") + require.Equal(t, 0.25, getFloatField(t, row, "percentage_location"), + "an omitted locator must be coalesced, not zeroed") + require.Equal(t, float64(12), getFloatField(t, row, "page_number")) + require.Equal(t, "web", getStringField(t, row, "last_modified_source"), + "the sync-routed PUT must maintain source bookkeeping") + + // PUT carrying a locator explicitly: it lands. + put2 := doReq(t, "PUT", annotationURL(setup.Server.URL, fx.itemID, "bookmarks", id), map[string]interface{}{ + "title": "H3 bookmark renamed", + "notes": "edited", + "percentage": 0.5, + "cfi_position": "epubcfi(/6/4/2:44)", + }, setup.RegularToken) + defer put2.Body.Close() + raw2, _ := io.ReadAll(put2.Body) + require.Equal(t, http.StatusOK, put2.StatusCode, "body: %s", raw2) + var row2 map[string]interface{} + require.NoError(t, json.Unmarshal(raw2, &row2)) + require.Equal(t, 0.5, getFloatField(t, row2, "percentage_location")) + require.Equal(t, "epubcfi(/6/4/2:44)", getStringField(t, row2, "cfi_position")) + // ...and the fields omitted THIS time coalesce again (page survived). + require.Equal(t, float64(12), getFloatField(t, row2, "page_number")) + + // The note PUT carries its content edit through the service too. + noteURL := annotationURL(setup.Server.URL, fx.itemID, "notes", "") + noteCreate := doReq(t, "POST", noteURL, map[string]interface{}{ + "content": "H3 note", "position": "p9", + }, setup.RegularToken) + defer noteCreate.Body.Close() + require.Equal(t, http.StatusCreated, noteCreate.StatusCode) + note := decodeJSON(t, noteCreate) + noteID, _ := note["id"].(string) + notePut := doReq(t, "PUT", annotationURL(setup.Server.URL, fx.itemID, "notes", noteID), map[string]interface{}{ + "content": "H3 note edited", "position": "p9b", + }, setup.RegularToken) + defer notePut.Body.Close() + raw3, _ := io.ReadAll(notePut.Body) + require.Equal(t, http.StatusOK, notePut.StatusCode, "body: %s", raw3) + var noteRow map[string]interface{} + require.NoError(t, json.Unmarshal(raw3, ¬eRow)) + require.Equal(t, "H3 note edited", getStringField(t, noteRow, "content")) + require.Equal(t, "web", getStringField(t, noteRow, "last_modified_source"), + "the note PUT must maintain source bookkeeping via the sync service") +} diff --git a/docs/contract_pins.md b/docs/contract_pins.md index 6b62dca0..76872742 100644 --- a/docs/contract_pins.md +++ b/docs/contract_pins.md @@ -74,3 +74,16 @@ cover_image_path, has_conflict, format_group, badge`. ("update note: no rows in result set") — the LWW update could never land. Fixed in the same commit (server-side fix, freely allowed); the pins are its regression net. +- **H3 (PUT fixes routed through the sync service)** — `PUT + /api/media-items/:id/bookmarks/:bookmarkId` now parses ADDITIONAL + optional request keys mirroring its create sibling: + `cfi_position, page_number, chapter_number, percentage, + chapter_reference` (all zero-value-preserving: omitted keys coalesce + with the stored row inside the service, so legacy clients that send + only title/notes/position keep every stored locator — verified by + TestH3_BookmarkPUTPreservesLocators). The PUT lands as an id-targeted + LWW edit under the row's stored dedup key (BookmarkID/NoteID mirror + SaveHighlightRequest.HighlightID), so `last_modified_source` / + `last_modified_at` are now maintained. Note PUT unchanged on the wire + (content+position) but routed through the service the same way. + Response shapes unchanged (same row models — pins green). diff --git a/internal/handlers/media.go b/internal/handlers/media.go index ae0dd766..aea610c4 100644 --- a/internal/handlers/media.go +++ b/internal/handlers/media.go @@ -169,6 +169,14 @@ type UpdateMediaBookmarkRequest struct { Title string `json:"title" validate:"required,min=1,max=255"` Notes string `json:"notes" validate:"max=100000"` Position string `json:"position" validate:"max=100"` + // Optional locator fields (H3): the create endpoint has always + // accepted these; the PUT ignored them until it was routed through + // the sync service. Zero-valued fields preserve the stored values. + CfiPosition string `json:"cfi_position" validate:"max=255"` + PageNumber int32 `json:"page_number"` + ChapterNumber int32 `json:"chapter_number"` + Percentage float64 `json:"percentage"` + ChapterReference int32 `json:"chapter_reference"` } type MediaHandler struct { @@ -1875,6 +1883,12 @@ func (mh *MediaHandler) UpdateMediaNote(c *echo.Context) error { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid user"}) } + mediaID := c.Param("id") + mediaUUID, err := uuid.Parse(mediaID) + if err != nil { + return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid media item id"}) + } + var req UpdateMediaNoteRequest if err := c.Bind(&req); err != nil { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid request"}) @@ -1883,17 +1897,24 @@ func (mh *MediaHandler) UpdateMediaNote(c *echo.Context) error { return c.JSON(http.StatusBadRequest, map[string]string{"error": err.Error()}) } - note, err := mh.db.UpdateMediaNote(c.Request().Context(), database.UpdateMediaNoteParams{ - ID: pgtype.UUID{Bytes: noteUUID, Valid: true}, - Content: req.Content, - Position: pgtype.Text{String: req.Position, Valid: req.Position != ""}, - UserID: pgtype.UUID{Bytes: userUUID, Valid: true}, + // Through the sync service (H3): the PUT lands as a proper LWW edit + // targeted by id (locator columns coalesced with the stored row), so + // source/modified_at bookkeeping is maintained — the plain query + // bypassed it entirely. + result, err := mh.annotationSvc.SaveNote(c.Request().Context(), wsync.SaveNoteRequest{ + MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true}, + UserID: pgtype.UUID{Bytes: userUUID, Valid: true}, + Content: req.Content, + Position: req.Position, + Source: "web", + ModifiedAt: time.Now(), + NoteID: pgtype.UUID{Bytes: noteUUID, Valid: true}, }) if err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) } - return c.JSON(http.StatusOK, note) + return c.JSON(http.StatusOK, result.Note) } // DeleteMediaNote handles DELETE /api/media-items/:id/notes/:noteId @@ -2178,12 +2199,16 @@ func (mh *MediaHandler) UpdateMediaBookmark(c *echo.Context) error { if err != nil { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid user"}) } - bookmarkID := c.Param("bookmarkId") bookmarkUUID, err := uuid.Parse(bookmarkID) if err != nil { return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid bookmark id"}) } + mediaID := c.Param("id") + mediaUUID, err := uuid.Parse(mediaID) + if err != nil { + return c.JSON(http.StatusBadRequest, map[string]string{"error": "invalid media item id"}) + } var req UpdateMediaBookmarkRequest if err := c.Bind(&req); err != nil { @@ -2193,18 +2218,30 @@ func (mh *MediaHandler) UpdateMediaBookmark(c *echo.Context) error { return c.JSON(http.StatusBadRequest, map[string]string{"error": err.Error()}) } - bookmark, err := mh.db.UpdateMediaBookmark(c.Request().Context(), database.UpdateMediaBookmarkParams{ - ID: pgtype.UUID{Bytes: bookmarkUUID, Valid: true}, - Title: req.Title, - Notes: pgtype.Text{String: req.Notes, Valid: req.Notes != ""}, - Position: pgtype.Text{String: req.Position, Valid: req.Position != ""}, - UserID: pgtype.UUID{Bytes: userUUID, Valid: true}, + // Through the sync service (H3): the PUT now lands as a proper LWW + // edit — source/modified_at bookkeeping maintained, locator fields + // carried (coalesced with the stored row for omitted ones), and the + // row targeted by id so an edited title can't miss its dedup key. + result, err := mh.annotationSvc.SaveBookmark(c.Request().Context(), wsync.SaveBookmarkRequest{ + MediaItemID: pgtype.UUID{Bytes: mediaUUID, Valid: true}, + UserID: pgtype.UUID{Bytes: userUUID, Valid: true}, + Title: req.Title, + Position: req.Position, + Notes: req.Notes, + PageNumber: req.PageNumber, + ChapterNumber: req.ChapterNumber, + CFIPosition: req.CfiPosition, + PercentageLoc: req.Percentage, + ChapterReference: req.ChapterReference, + Source: "web", + ModifiedAt: time.Now(), + BookmarkID: pgtype.UUID{Bytes: bookmarkUUID, Valid: true}, }) if err != nil { return c.JSON(http.StatusInternalServerError, map[string]string{"error": err.Error()}) } - return c.JSON(http.StatusOK, bookmark) + return c.JSON(http.StatusOK, result.Bookmark) } // DeleteMediaBookmark handles DELETE /api/media-items/:id/bookmarks/:bookmarkId diff --git a/internal/sync/annotations.go b/internal/sync/annotations.go index f062b38b..571cc4c1 100644 --- a/internal/sync/annotations.go +++ b/internal/sync/annotations.go @@ -521,6 +521,10 @@ type SaveNoteRequest struct { ModifiedAt time.Time DeviceSyncData []byte DedupKey string // overrides the computed key for device echoes + // NoteID, when valid, targets that exact row (web PUTs edit by id): + // the save LWWs against it under its stored dedup key, mirroring + // SaveHighlightRequest.HighlightID. + NoteID pgtype.UUID } type SaveNoteResult struct { @@ -535,6 +539,54 @@ func (s *AnnotationService) SaveNote(ctx context.Context, req SaveNoteRequest) ( } dedupKey := req.DedupKey + + // Web edits arrive with the row id from the URL: resolve by id and LWW + // against that row under its stored key (same doctrine as + // SaveHighlight's HighlightID path). Locator columns the note PUT + // does not carry are coalesced from the stored row so the full-column + // sync update cannot zero them. + if req.NoteID.Valid { + byID, idErr := s.db.GetMediaNote(ctx, req.NoteID) + if idErr != nil && !errors.Is(idErr, pgx.ErrNoRows) { + 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 dedupKey == "" { + dedupKey = byID.DedupKey.String + } + if req.EpubcfiLocation == "" { + req.EpubcfiLocation = byID.EpubcfiLocation.String + } + if req.PercentageLocation == 0 { + req.PercentageLocation = byID.PercentageLocation.Float64 + } + if req.CharacterStart == 0 { + req.CharacterStart = byID.CharacterStart.Int32 + } + if req.CharacterEnd == 0 { + req.CharacterEnd = byID.CharacterEnd.Int32 + } + if req.ChapterReference == 0 { + req.ChapterReference = byID.ChapterReference.Int32 + } + 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). + } + return s.applyNoteLWW(ctx, req, byID, dedupKey) + } + // No row with that id: fall through to identity-based resolution. + } + if dedupKey == "" { dedupKey = ComputeDedupKey(req.Content, req.EpubcfiLocation, req.Position) } @@ -684,6 +736,11 @@ type SaveBookmarkRequest struct { // DedupKey overrides the computed key for device echoes (see // SaveHighlightRequest). DedupKey string + // BookmarkID, when valid, targets that exact row (web PUTs edit by + // id): the save LWWs against it under its stored dedup key, mirroring + // SaveHighlightRequest.HighlightID — an edit must not re-derive + // identity from (possibly edited) content. + BookmarkID pgtype.UUID } type SaveBookmarkResult struct { @@ -694,6 +751,55 @@ type SaveBookmarkResult struct { func (s *AnnotationService) SaveBookmark(ctx context.Context, req SaveBookmarkRequest) (*SaveBookmarkResult, error) { dedupKey := req.DedupKey + + // Web edits arrive with the row id from the URL: resolve by id and LWW + // against that row under its stored key (same doctrine as + // SaveHighlight's HighlightID path). Fields the edit does not carry + // are coalesced from the stored row — the web PUT predates the + // locator columns and omits them; zeroing them on update would be + // data loss. + if req.BookmarkID.Valid { + byID, idErr := s.db.GetMediaBookmark(ctx, req.BookmarkID) + if idErr != nil && !errors.Is(idErr, pgx.ErrNoRows) { + 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 dedupKey == "" { + dedupKey = byID.DedupKey.String + } + if req.CFIPosition == "" { + req.CFIPosition = byID.CfiPosition.String + } + if req.EpubcfiLocation == "" { + req.EpubcfiLocation = byID.EpubcfiLocation.String + } + if req.PercentageLoc == 0 { + req.PercentageLoc = byID.PercentageLocation.Float64 + } + if req.PageNumber == 0 { + req.PageNumber = byID.PageNumber.Int32 + } + if req.ChapterNumber == 0 { + req.ChapterNumber = byID.ChapterNumber.Int32 + } + 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). + } + return s.applyBookmarkLWW(ctx, req, byID, dedupKey) + } + // No row with that id: fall through to identity-based resolution. + } + if dedupKey == "" { dedupKey = ComputeDedupKey(req.Title, req.EpubcfiLocation, req.Position) }