From 385bab35fe8cefee488fe0ea20296179bea92518 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sat, 12 Sep 2026 11:01:26 -0400 Subject: [PATCH] fix(persistence): recompute the cached average rating inside ReassignAnnotation Moving annotation rows always changes the new item's rating population, so the recompute belongs with the move rather than at each call site. Covers the album reassign in the remap and the two scanner sites, and replaces the explicit call ReassignReferences was making. Album was the worse case: rate an album, move its files, and 'missing fix' handed the rating to an album still caching an average of 0. --- core/maintenance.go | 3 ++- persistence/mediafile_repository.go | 4 ---- persistence/sql_annotations.go | 7 +++++-- persistence/sql_annotations_test.go | 10 ++++++++++ 4 files changed, 17 insertions(+), 7 deletions(-) diff --git a/core/maintenance.go b/core/maintenance.go index 13c94a621..56c0ac18d 100644 --- a/core/maintenance.go +++ b/core/maintenance.go @@ -119,7 +119,8 @@ func (s *maintenanceService) RemapMissingFile(ctx context.Context, missingID, ta return err } - // Stats are refreshed synchronously, unlike deleteMissing, so the CLI sees them before it exits + // Stats are refreshed synchronously, unlike deleteMissing, so the CLI sees them before it exits. + // album/artist play count aggregates are not recalculated here; they are refreshed by the next scan. if _, err := s.ds.Artist(ctx).RefreshStats(true); err != nil { log.Error(ctx, "Error refreshing artist stats after remapping missing file", err) } diff --git a/persistence/mediafile_repository.go b/persistence/mediafile_repository.go index 406afbce3..935a64bb9 100644 --- a/persistence/mediafile_repository.go +++ b/persistence/mediafile_repository.go @@ -404,10 +404,6 @@ func (r *mediaFileRepository) ReassignReferences(prevID, newID string) error { if err := r.ReassignAnnotation(prevID, newID); err != nil { return fmt.Errorf("reassigning annotations: %w", err) } - // The merged rows change the rating population, so the cached average no longer matches - if err := r.updateAvgRating(newID); err != nil { - return fmt.Errorf("recomputing average rating: %w", err) - } if err := r.reassignBookmark(prevID, newID); err != nil { return fmt.Errorf("reassigning bookmarks: %w", err) } diff --git a/persistence/sql_annotations.go b/persistence/sql_annotations.go index 46616f5c9..27445b886 100644 --- a/persistence/sql_annotations.go +++ b/persistence/sql_annotations.go @@ -188,8 +188,11 @@ func (r sqlRepository) ReassignAnnotation(prevID string, newID string) error { // OR IGNORE keeps newID's own row where a user annotated both, instead of aborting the whole statement upd := Expr("update or ignore "+annotationTable+" set item_id = ? where item_type = ? and item_id = ?", newID, r.tableName, prevID) - _, err := r.executeSQL(upd) - return err + if _, err := r.executeSQL(upd); err != nil { + return err + } + // The moved rows change newID's rating population, so its cached average no longer matches + return r.updateAvgRating(newID) } func (r sqlRepository) cleanAnnotations() error { diff --git a/persistence/sql_annotations_test.go b/persistence/sql_annotations_test.go index 05e84fcc7..79ad3c152 100644 --- a/persistence/sql_annotations_test.go +++ b/persistence/sql_annotations_test.go @@ -55,6 +55,16 @@ var _ = Describe("Annotation Filters", func() { Expect(got.Rating).To(Equal(4)) }) + It("recomputes the new item's cached average rating", func() { + Expect(albumRepo.SetRating(4, prev.ID)).To(Succeed()) + + Expect(albumRepo.ReassignAnnotation(prev.ID, next.ID)).To(Succeed()) + + got, err := albumRepo.Get(next.ID) + Expect(err).ToNot(HaveOccurred()) + Expect(got.AverageRating).To(Equal(4.0)) + }) + It("keeps the new item's annotation when both exist", func() { Expect(albumRepo.SetRating(4, prev.ID)).To(Succeed()) Expect(albumRepo.SetRating(2, next.ID)).To(Succeed())