From 8b5e50a1aa3e3af75f3f7ec865a689934e98a581 Mon Sep 17 00:00:00 2001 From: Deluan Date: Sun, 24 May 2026 19:42:04 -0300 Subject: [PATCH] feat(scanner): mirror album re-PID flow for artists When PID.Artist spec changes, existing artist IDs differ from newly-computed ones. Without remapping, all annotations (starred, ratings, play counts) become orphaned. This change builds an artistIDMap during phase 1 and reassigns annotations on persist, mirroring the existing albumIDMap flow. The migration logic is gated on the previous spec being non-empty and different from the current spec, so the default "name" spec (byte-identical to the historical hardcoded artistID) triggers no remapping on upgrade. --- model/metadata/persistent_ids.go | 6 ++++ scanner/folder_entry.go | 18 ++++++----- scanner/phase_1_folders.go | 54 +++++++++++++++++++++++++++----- scanner/phase_1_folders_test.go | 49 +++++++++++++++++++++++++++++ tests/mock_artist_repo.go | 20 ++++++++++-- 5 files changed, 128 insertions(+), 19 deletions(-) create mode 100644 scanner/phase_1_folders_test.go diff --git a/model/metadata/persistent_ids.go b/model/metadata/persistent_ids.go index c3a3682a5..168e1081c 100644 --- a/model/metadata/persistent_ids.go +++ b/model/metadata/persistent_ids.go @@ -124,6 +124,12 @@ func computeArtistPID(p model.Participant, spec string, hash hashFunc) string { return hash(pid) } +// ComputeArtistPID is the exported entry point for callers outside this package +// (e.g. the scanner) that need to compute an artist PID under a specific spec. +func ComputeArtistPID(p model.Participant, spec string) string { + return computeArtistPID(p, spec, id.NewHash) +} + func getArtistPIDAttr(p model.Participant, attr string, hash hashFunc) string { switch strings.TrimSpace(strings.ToLower(attr)) { case "name": diff --git a/scanner/folder_entry.go b/scanner/folder_entry.go index c7cc88ee1..6a26a44bb 100644 --- a/scanner/folder_entry.go +++ b/scanner/folder_entry.go @@ -17,14 +17,15 @@ import ( func newFolderEntry(job *scanJob, id, path string, updTime time.Time, hash string) *folderEntry { f := &folderEntry{ - id: id, - job: job, - path: path, - audioFiles: make(map[string]fs.DirEntry), - imageFiles: make(map[string]fs.DirEntry), - albumIDMap: make(map[string]string), - updTime: updTime, - prevHash: hash, + id: id, + job: job, + path: path, + audioFiles: make(map[string]fs.DirEntry), + imageFiles: make(map[string]fs.DirEntry), + albumIDMap: make(map[string]string), + artistIDMap: make(map[string]string), + updTime: updTime, + prevHash: hash, } return f } @@ -46,6 +47,7 @@ type folderEntry struct { albums model.Albums albumIDMap map[string]string artists model.Artists + artistIDMap map[string]string tags model.TagList missingTracks []*model.MediaFile } diff --git a/scanner/phase_1_folders.go b/scanner/phase_1_folders.go index 38967832c..8aee6af25 100644 --- a/scanner/phase_1_folders.go +++ b/scanner/phase_1_folders.go @@ -123,11 +123,12 @@ func (j *scanJob) createFolderEntry(path string) *folderEntry { // The phaseFolders struct implements the phase interface, providing methods to produce // folder entries, process folders, persist changes to the database, and log the results. type phaseFolders struct { - jobs []*scanJob - ds model.DataStore - ctx context.Context - state *scanState - prevAlbumPIDConf string + jobs []*scanJob + ds model.DataStore + ctx context.Context + state *scanState + prevAlbumPIDConf string + prevArtistPIDConf string } func (p *phaseFolders) description() string { @@ -141,6 +142,10 @@ func (p *phaseFolders) producer() ppl.Producer[*folderEntry] { if err != nil { return fmt.Errorf("getting album PID conf: %w", err) } + p.prevArtistPIDConf, err = p.ds.Property(p.ctx).DefaultGet(consts.PIDArtistKey, "") + if err != nil { + return fmt.Errorf("getting artist PID conf: %w", err) + } // TODO Parallelize multiple job when we have multiple libraries var total int64 @@ -298,6 +303,21 @@ func (p *phaseFolders) loadTagsFromFiles(entry *folderEntry, toImport map[string if prevAlbumID != track.AlbumID && !ok { entry.albumIDMap[track.AlbumID] = prevAlbumID } + + // Build artistIDMap: for each participant on this track, if the previous + // spec produces a different ID than the current one, record the mapping. + if p.prevArtistPIDConf != "" && p.prevArtistPIDConf != conf.Server.PID.Artist { + for _, list := range track.Participants { + for _, part := range list { + prevID := metadata.ComputeArtistPID(part, p.prevArtistPIDConf) + if prevID != part.ID { + if _, already := entry.artistIDMap[part.ID]; !already { + entry.artistIDMap[part.ID] = prevID + } + } + } + } + } } } entry.tracks = tracks @@ -359,9 +379,7 @@ func (p *phaseFolders) persistChanges(entry *folderEntry) (*folderEntry, error) // Save all new/modified artists to DB. Their information will be incomplete, but they will be refreshed later for i := range entry.artists { - err = artistRepo.Put(&entry.artists[i], "name", - "mbz_artist_id", "sort_artist_name", "order_artist_name", "full_text", "updated_at") - if err != nil { + if err = p.persistArtist(artistRepo, &entry.artists[i], entry.artistIDMap); err != nil { log.Error(p.ctx, "Scanner: Error persisting artist to DB", "folder", entry.path, "artist", entry.artists[i].Name, err) return err } @@ -462,6 +480,26 @@ func (p *phaseFolders) persistAlbum(repo model.AlbumRepository, a *model.Album, return nil } +// persistArtist persists the given artist and reassigns annotations from the +// previous artist ID (if recorded in idMap). Mirrors persistAlbum. +func (p *phaseFolders) persistArtist(repo model.ArtistRepository, a *model.Artist, idMap map[string]string) error { + if err := repo.Put(a, "name", "mbz_artist_id", "sort_artist_name", + "order_artist_name", "full_text", "updated_at"); err != nil { + return fmt.Errorf("persisting artist %s: %w", a.ID, err) + } + prevID := idMap[a.ID] + if prevID == "" { + return nil + } + log.Trace(p.ctx, "Reassigning artist annotations", "from", prevID, "to", a.ID, "artist", a.Name) + if err := repo.ReassignAnnotation(prevID, a.ID); err != nil { + log.Warn(p.ctx, "Scanner: Could not reassign artist annotations", "from", prevID, "to", a.ID, "artist", a.Name, err) + p.state.sendWarning(fmt.Sprintf("Could not reassign artist annotations from %s to %s ('%s'): %v", prevID, a.ID, a.Name, err)) + } + delete(idMap, a.ID) + return nil +} + func (p *phaseFolders) logFolder(entry *folderEntry) (*folderEntry, error) { logCall := log.Info if entry.isEmpty() { diff --git a/scanner/phase_1_folders_test.go b/scanner/phase_1_folders_test.go new file mode 100644 index 000000000..f76b63bcb --- /dev/null +++ b/scanner/phase_1_folders_test.go @@ -0,0 +1,49 @@ +package scanner + +import ( + "context" + + "github.com/navidrome/navidrome/conf/configtest" + "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/tests" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("phaseFolders", func() { + Describe("Artist re-PID migration", func() { + var ( + artRepo *tests.MockArtistRepo + p *phaseFolders + ) + + BeforeEach(func() { + DeferCleanup(configtest.SetupConfig()) + artRepo = tests.CreateMockArtistRepo() + if artRepo.ReassignAnnotationCalls == nil { + artRepo.ReassignAnnotationCalls = map[string]string{} + } + p = &phaseFolders{ctx: context.Background(), state: &scanState{}} + }) + + It("calls ReassignAnnotation when persisting an artist whose ID changed", func() { + oldID := "old-id" + newID := "new-id" + idMap := map[string]string{newID: oldID} + artist := model.Artist{ID: newID, Name: "Foo"} + + Expect(p.persistArtist(artRepo, &artist, idMap)).To(Succeed()) + Expect(artRepo.ReassignAnnotationCalls).To(HaveKeyWithValue(oldID, newID)) + // Mapping should be removed after successful reassignment + Expect(idMap).ToNot(HaveKey(newID)) + }) + + It("does not call ReassignAnnotation when no mapping exists", func() { + idMap := map[string]string{} + artist := model.Artist{ID: "some-id", Name: "Foo"} + + Expect(p.persistArtist(artRepo, &artist, idMap)).To(Succeed()) + Expect(artRepo.ReassignAnnotationCalls).To(BeEmpty()) + }) + }) +}) diff --git a/tests/mock_artist_repo.go b/tests/mock_artist_repo.go index b7a6fb811..950fea7de 100644 --- a/tests/mock_artist_repo.go +++ b/tests/mock_artist_repo.go @@ -16,9 +16,10 @@ func CreateMockArtistRepo() *MockArtistRepo { type MockArtistRepo struct { model.ArtistRepository - Data map[string]*model.Artist - Err bool - Options model.QueryOptions + Data map[string]*model.Artist + Err bool + Options model.QueryOptions + ReassignAnnotationCalls map[string]string // prevID -> newID } func (m *MockArtistRepo) SetError(err bool) { @@ -157,4 +158,17 @@ func (m *MockArtistRepo) Search(q string, options ...model.QueryOptions) (model. return allArtists, err } +// ReassignAnnotation reassigns annotations from one artist to another +func (m *MockArtistRepo) ReassignAnnotation(prevID string, newID string) error { + if m.Err { + return errors.New("unexpected error") + } + // Mock implementation - track the reassignment calls + if m.ReassignAnnotationCalls == nil { + m.ReassignAnnotationCalls = make(map[string]string) + } + m.ReassignAnnotationCalls[prevID] = newID + return nil +} + var _ model.ArtistRepository = (*MockArtistRepo)(nil)