From ff029f1ad5e8c32f5bd11ab5f54844dd94cd3060 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Sat, 17 Oct 2026 15:38:47 -0400 Subject: [PATCH] =?UTF-8?q?fix(annotations):=20note=20LWW=20re-save=20500'?= =?UTF-8?q?d=20=E2=80=94=20UserID=20missing=20from=20the=20sync=20update?= =?UTF-8?q?=20params?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pin-first batch for H3: freeze web annotation CRUD response shapes (create 201 / update 200 / delete 204; full sqlc row models) before the stack consolidation touches anything. The new pins immediately caught a live server bug the 28 unit tests could never see (annotations_test.go is pure-unit: keys and compare logic, no DB): applyNoteLWW built UpdateMediaNoteForSyncParams without UserID, so the query's WHERE id=$1 AND user_id=$13 compared against zero — every dedup-hit note re-save (same content+position) returned 500 "update note: no rows in result set" and the update never landed. The bookmark twin passes UserID and works; the note stack is the clone that dropped the field. Fix: pass UserID in the params (one line). Pins go green and become the regression net; changelog records the new surfaces. --- cmd/server/tests/contract_pins_test.go | 147 ++++++++++++++++++++++++- docs/contract_pins.md | 13 +++ internal/sync/annotations.go | 1 + 3 files changed, 160 insertions(+), 1 deletion(-) diff --git a/cmd/server/tests/contract_pins_test.go b/cmd/server/tests/contract_pins_test.go index 2c0b64ea..7eda7468 100644 --- a/cmd/server/tests/contract_pins_test.go +++ b/cmd/server/tests/contract_pins_test.go @@ -982,7 +982,6 @@ func TestH1_ConflictResolutionPreservesCFIPair(t *testing.T) { // 7. H1 verification — resolving a conflict must preserve unknown fields // --------------------------------------------------------------------------- - // TestH1_ConflictResolutionPreservesAudio is the executable check for the // hardening plan's H1 done-when: a resolution applied via // POST /api/conflicts/:id/resolve (the endpoint the web UI and the @@ -1073,3 +1072,149 @@ func TestH1_ConflictResolutionPreservesAudio(t *testing.T) { } } } + +// --------------------------------------------------------------------------- +// 3b. web annotation CRUD — the shape freeze H3 needs before it consolidates +// --------------------------------------------------------------------------- + +// The three response shapes are the full sqlc row models (annotations are +// richer than their create/PUT request DTOs — the sync columns ride along). +// H3's consolidation + PUT fixes must keep these key sets; response keys +// gained by routing the PUTs through the sync service (e.g. outcome +// markers) are additive and go through the whitelist. +var ( + _WEBBookmarkRowKeys = []string{ + "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", "origin_source", + } + _WebNoteRowKeys = []string{ + "id", "media_item_id", "user_id", "content", "position", "created_at", + "updated_at", "percentage_location", "character_start", + "character_end", "epubcfi_location", "chapter_reference", + "paragraph_reference", "device_sync_data", "dedup_key", + "last_modified_at", "last_modified_source", "deleted", "deleted_at", + } + _WebHighlightRowKeys = []string{ + "id", "media_item_id", "user_id", "selection_text", + "start_position", "end_position", "color", "note_id", "created_at", + "updated_at", "percentage_start", "percentage_end", + "character_start", "character_end", "epubcfi_start", "epubcfi_end", + "chapter_reference", "paragraph_start", "paragraph_end", + "panel_number", "device_sync_data", "dedup_key", + "last_modified_at", "last_modified_source", "note_text", + "deleted", "deleted_at", + } +) + +func annotationURL(serverURL, mediaItemID, kind, id string) string { + if id == "" { + return fmt.Sprintf("%s/api/media-items/%s/%s", serverURL, mediaItemID, kind) + } + return fmt.Sprintf("%s/api/media-items/%s/%s/%s", serverURL, mediaItemID, kind, id) +} + +// pinAnnotationLifecycle walks create -> update -> delete on ONE annotation +// type and freezes every response shape (that's the whole contract the web +// reader and the app rely on for annotations). Every failed status includes +// the response body — pins that fail on status alone are otherwise +// undiagnosable. +func pinAnnotationLifecycle(t *testing.T, setup *TestServerSetup, fx contractFixture, kind string, createBody, updateBody interface{}, rowKeys []string) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + t.Run(kind+" POST response", func(t *testing.T) { + resp := doReq(t, "POST", annotationURL(setup.Server.URL, fx.itemID, kind, ""), createBody, setup.RegularToken) + defer resp.Body.Close() + raw, readErr := io.ReadAll(resp.Body) + require.NoError(t, readErr) + if !assert.Equal(t, http.StatusCreated, resp.StatusCode, "create must stay 201; body: %s", raw) { + t.FailNow() + } + var body map[string]interface{} + require.NoError(t, json.Unmarshal(raw, &body)) + assertResponseShape(t, "annotation "+kind+" create", body, rowKeys, rowKeys) + }) + t.Run(kind+" PUT response", func(t *testing.T) { + resp := doReq(t, "POST", annotationURL(setup.Server.URL, fx.itemID, kind, ""), createBody, setup.RegularToken) + defer resp.Body.Close() + raw0, readErr := io.ReadAll(resp.Body) + require.NoError(t, readErr) + if !assert.Equal(t, http.StatusCreated, resp.StatusCode, "inner create must stay 201; body: %s", raw0) { + t.FailNow() + } + var created map[string]interface{} + require.NoError(t, json.Unmarshal(raw0, &created)) + id, _ := created["id"].(string) + require.NotEmpty(t, id) + + resp2 := doReq(t, "PUT", annotationURL(setup.Server.URL, fx.itemID, kind, id), updateBody, setup.RegularToken) + defer resp2.Body.Close() + raw, readErr := io.ReadAll(resp2.Body) + require.NoError(t, readErr) + if !assert.Equal(t, http.StatusOK, resp2.StatusCode, "update must stay 200; body: %s", raw) { + t.FailNow() + } + var body2 map[string]interface{} + require.NoError(t, json.Unmarshal(raw, &body2)) + assertResponseShape(t, "annotation "+kind+" update", body2, rowKeys, rowKeys) + + t.Run(kind+" DELETE response", func(t *testing.T) { + resp3 := doReq(t, "DELETE", annotationURL(setup.Server.URL, fx.itemID, kind, id), nil, setup.RegularToken) + defer resp3.Body.Close() + require.Equal(t, http.StatusNoContent, resp3.StatusCode, "delete must stay 204") + }) + }) +} + +func TestContractPin_AnnotationCRUDNote(t *testing.T) { + setup := setupTestServer(t) + fx := createContractFixture(t, setup) + pinAnnotationLifecycle(t, setup, fx, "notes", + map[string]interface{}{"content": "pin note", "position": "p1"}, + map[string]interface{}{"content": "pin note 2", "position": "p1b"}, + _WebNoteRowKeys) +} + +func TestContractPin_AnnotationCRUDHighlight(t *testing.T) { + setup := setupTestServer(t) + fx := createContractFixture(t, setup) + pinAnnotationLifecycle(t, setup, fx, "highlights", + map[string]interface{}{ + "selection_text": "pin highlight text", + "epubcfi_start": "epubcfi(/6/4/2:10)", + "epubcfi_end": "epubcfi(/6/4/2:20)", + "color": "#ffff00", + }, + map[string]interface{}{ + "selection_text": "pin highlight text", + "epubcfi_start": "epubcfi(/6/4/2:10)", + "epubcfi_end": "epubcfi(/6/4/2:20)", + "color": "#ff0000", + }, + _WebHighlightRowKeys) +} + +func TestContractPin_AnnotationCRUDBookmark(t *testing.T) { + setup := setupTestServer(t) + fx := createContractFixture(t, setup) + pinAnnotationLifecycle(t, setup, fx, "bookmarks", + map[string]interface{}{ + "title": "pin bookmark", + "position": "p1", + "cfi_position": "epubcfi(/6/4/2:30)", + "page_number": 12, + "chapter_number": 3, + "percentage": 0.25, + "chapter_reference": 4, + }, + map[string]interface{}{ + "title": "pin bookmark 2", + "position": "p1b", + "notes": "updated by pin", + }, + _WEBBookmarkRowKeys) +} diff --git a/docs/contract_pins.md b/docs/contract_pins.md index 43da2f50..6b62dca0 100644 --- a/docs/contract_pins.md +++ b/docs/contract_pins.md @@ -61,3 +61,16 @@ cover_image_path, has_conflict, format_group, badge`. reader.ts:3113-3143 and audio.ts:518-520); keys the web never sent (viewport_x/y, panel_number) merely become honor-able. All other pins unchanged. +- **H3-prep (web annotation CRUD freeze)** — NEW pinned surfaces, captured + pre-refactor per the pin-first doctrine: `POST/PUT/DELETE + /api/media-items/:id/{notes,highlights,bookmarks}` — create 201 / update + 200 / delete 204, response bodies are the full sqlc row models + (MediaNotes 19 keys, MediaBookmarks 20 keys, MediaHighlights 27 keys — + listed in contract_pins_test.go). No keys removed; nothing was pinned + here before. Capturing these immediately caught a LIVE server bug: + `applyNoteLWW` omitted `UserID` from its + `UpdateMediaNoteForSync` params, so every dedup-hit note re-save + (same content+position ⇒ same key) returned 500 + ("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. diff --git a/internal/sync/annotations.go b/internal/sync/annotations.go index 20b0ea12..3d5bcdd7 100644 --- a/internal/sync/annotations.go +++ b/internal/sync/annotations.go @@ -613,6 +613,7 @@ func (s *AnnotationService) applyNoteLWW(ctx context.Context, req SaveNoteReques note, err := s.db.UpdateMediaNoteForSync(ctx, database.UpdateMediaNoteForSyncParams{ ID: existing.ID, + UserID: req.UserID, Content: req.Content, Position: pgText(req.Position), PercentageLocation: pgFloat8(req.PercentageLocation),