fix(annotations): LWW applies coalesce omitted fields — partial payloads no longer erase stored locators
Caught by the live round-trip sweep: a dedup-key re-save (a device echo or a client re-POSTing a bookmark it already had) carried no locator fields, and the full-column sync update wrote zeroes — which pgFloat8/pgInt4 turn into NULLs — erasing the stored percentage/page/cfi on every echo. The highlight stack had string coalescing for exactly this reason (its locatorRefresher doctrine comment); the numeric columns and the two cloned stacks never got it. One primitive now carries the whole doctrine: coalesce[T comparable] (zero value -> stored value), applied at the single write point of each LWW apply — highlight (percentages, color, note_text, chapter_reference added to its existing string coalescing), bookmark (page/chapter/percentage/epubcfi/chapter_reference added to its cfi/position pair), note (all locators). The redundant pre-coalescing in the id-targeted resolution blocks is removed — the apply is the one coalesce point, so dedup-path and id-path edits behave identically. Verified live on the rebuilt container: dedup re-save and legacy 3-field PUTs preserve every stored locator on all three stacks; an explicitly-sent field still lands. Pins, H1/H3 tests, unit suites green.
This commit is contained in:
John O'Keefe committed
1 parent
11609d13d1
commit
edb4e60679
1 file changed
+39
-69
@@ -95,6 +95,18 @@ type SaveHighlightResult struct {
|
||||
Conflict bool
|
||||
}
|
||||
|
||||
// coalesce returns fallback when v is the zero value — the LWW-apply
|
||||
// "an omitted field preserves the stored value" primitive. Zeroes matter
|
||||
// here because pgFloat8/pgInt4 turn them into NULLs: an uncoalesced
|
||||
// partial payload would erase stored locators on every re-save.
|
||||
func coalesce[T comparable](v, fallback T) T {
|
||||
var zero T
|
||||
if v == zero {
|
||||
return fallback
|
||||
}
|
||||
return v
|
||||
}
|
||||
|
||||
// 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 {
|
||||
@@ -238,35 +250,23 @@ func (s *AnnotationService) applyLWW(
|
||||
// xpointer) and may carry no CFI either: keep the stored ones so
|
||||
// device-native serve-back and round-trip identity survive a web-side
|
||||
// note/color edit instead of being wiped to empty.
|
||||
startPosition := req.StartPosition
|
||||
if startPosition == "" {
|
||||
startPosition = existing.StartPosition.String
|
||||
}
|
||||
endPosition := req.EndPosition
|
||||
if endPosition == "" {
|
||||
endPosition = existing.EndPosition.String
|
||||
}
|
||||
epubcfiStart := req.EpubcfiStart
|
||||
if epubcfiStart == "" {
|
||||
epubcfiStart = existing.EpubcfiStart.String
|
||||
}
|
||||
epubcfiEnd := req.EpubcfiEnd
|
||||
if epubcfiEnd == "" {
|
||||
epubcfiEnd = existing.EpubcfiEnd.String
|
||||
}
|
||||
startPosition := coalesce(req.StartPosition, existing.StartPosition.String)
|
||||
endPosition := coalesce(req.EndPosition, existing.EndPosition.String)
|
||||
epubcfiStart := coalesce(req.EpubcfiStart, existing.EpubcfiStart.String)
|
||||
epubcfiEnd := coalesce(req.EpubcfiEnd, existing.EpubcfiEnd.String)
|
||||
|
||||
highlight, err := s.db.UpdateMediaHighlightForSync(ctx, database.UpdateMediaHighlightForSyncParams{
|
||||
ID: existing.ID,
|
||||
SelectionText: req.SelectionText,
|
||||
StartPosition: pgText(startPosition),
|
||||
EndPosition: pgText(endPosition),
|
||||
Color: pgText(req.Color),
|
||||
NoteText: pgText(req.NoteText),
|
||||
PercentageStart: pgFloat8(req.PercentageStart),
|
||||
PercentageEnd: pgFloat8(req.PercentageEnd),
|
||||
Color: pgText(coalesce(req.Color, existing.Color.String)),
|
||||
NoteText: pgText(coalesce(req.NoteText, existing.NoteText.String)),
|
||||
PercentageStart: pgFloat8(coalesce(req.PercentageStart, existing.PercentageStart.Float64)),
|
||||
PercentageEnd: pgFloat8(coalesce(req.PercentageEnd, existing.PercentageEnd.Float64)),
|
||||
EpubcfiStart: pgText(epubcfiStart),
|
||||
EpubcfiEnd: pgText(epubcfiEnd),
|
||||
ChapterReference: pgInt4(req.ChapterReference),
|
||||
ChapterReference: pgInt4(coalesce(req.ChapterReference, existing.ChapterReference.Int32)),
|
||||
LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true},
|
||||
LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""},
|
||||
DeviceSyncData: deviceData,
|
||||
@@ -561,24 +561,6 @@ func (s *AnnotationService) SaveNote(ctx context.Context, req SaveNoteRequest) (
|
||||
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 tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) {
|
||||
return &SaveNoteResult{Note: byID, Outcome: SaveOutcomeDeleted}, nil
|
||||
}
|
||||
@@ -663,17 +645,21 @@ func (s *AnnotationService) applyNoteLWW(ctx context.Context, req SaveNoteReques
|
||||
|
||||
deviceData := mergeDeviceSyncData(existing.DeviceSyncData, req.Source, req.DeviceSyncData)
|
||||
|
||||
// Coalesce omitted fields with the stored row (same doctrine as the
|
||||
// highlight apply): a partial payload — a web PUT carrying only
|
||||
// content/position, a device echo without percentages — must not
|
||||
// wipe the locators it didn't mention (see coalesce).
|
||||
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),
|
||||
CharacterStart: pgInt4(req.CharacterStart),
|
||||
CharacterEnd: pgInt4(req.CharacterEnd),
|
||||
EpubcfiLocation: pgText(req.EpubcfiLocation),
|
||||
ChapterReference: pgInt4(req.ChapterReference),
|
||||
ParagraphReference: pgInt4(req.ParagraphReference),
|
||||
Position: pgText(coalesce(req.Position, existing.Position.String)),
|
||||
PercentageLocation: pgFloat8(coalesce(req.PercentageLocation, existing.PercentageLocation.Float64)),
|
||||
CharacterStart: pgInt4(coalesce(req.CharacterStart, existing.CharacterStart.Int32)),
|
||||
CharacterEnd: pgInt4(coalesce(req.CharacterEnd, existing.CharacterEnd.Int32)),
|
||||
EpubcfiLocation: pgText(coalesce(req.EpubcfiLocation, existing.EpubcfiLocation.String)),
|
||||
ChapterReference: pgInt4(coalesce(req.ChapterReference, existing.ChapterReference.Int32)),
|
||||
ParagraphReference: pgInt4(coalesce(req.ParagraphReference, existing.ParagraphReference.Int32)),
|
||||
LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true},
|
||||
LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""},
|
||||
DeviceSyncData: deviceData,
|
||||
@@ -778,24 +764,6 @@ func (s *AnnotationService) SaveBookmark(ctx context.Context, req SaveBookmarkRe
|
||||
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 tombstoneWins(byID.Deleted.Bool, byID.DeletedAt, byID.LastModifiedAt, req.ModifiedAt) {
|
||||
return &SaveBookmarkResult{Bookmark: byID, Outcome: SaveOutcomeDeleted}, nil
|
||||
}
|
||||
@@ -899,15 +867,17 @@ func (s *AnnotationService) applyBookmarkLWW(ctx context.Context, req SaveBookma
|
||||
|
||||
bm, err := s.db.UpdateMediaBookmarkForSync(ctx, database.UpdateMediaBookmarkForSyncParams{
|
||||
ID: existing.ID,
|
||||
PageNumber: pgInt4(req.PageNumber),
|
||||
ChapterNumber: pgInt4(req.ChapterNumber),
|
||||
PageNumber: pgInt4(coalesce(req.PageNumber, existing.PageNumber.Int32)),
|
||||
ChapterNumber: pgInt4(coalesce(req.ChapterNumber, existing.ChapterNumber.Int32)),
|
||||
CfiPosition: pgText(cfiPosition),
|
||||
Title: req.Title,
|
||||
Position: pgText(position),
|
||||
Notes: pgText(req.Notes),
|
||||
PercentageLocation: pgFloat8(req.PercentageLoc),
|
||||
EpubcfiLocation: pgText(req.EpubcfiLocation),
|
||||
ChapterReference: pgInt4(req.ChapterReference),
|
||||
// Locator fields a partial payload omits must survive: zeroes
|
||||
// become NULLs (pgFloat8/pgInt4) and would erase stored values.
|
||||
PercentageLocation: pgFloat8(coalesce(req.PercentageLoc, existing.PercentageLocation.Float64)),
|
||||
EpubcfiLocation: pgText(coalesce(req.EpubcfiLocation, existing.EpubcfiLocation.String)),
|
||||
ChapterReference: pgInt4(coalesce(req.ChapterReference, existing.ChapterReference.Int32)),
|
||||
LastModifiedAt: pgtype.Timestamptz{Time: modifiedAt, Valid: true},
|
||||
LastModifiedSource: pgtype.Text{String: req.Source, Valid: req.Source != ""},
|
||||
DeviceSyncData: deviceData,
|
||||
|
||||
Reference in new issue
Block a user