diff --git a/core/agents/interfaces.go b/core/agents/interfaces.go index e60c6190..054a14c5 100644 --- a/core/agents/interfaces.go +++ b/core/agents/interfaces.go @@ -22,6 +22,7 @@ type AlbumInfo struct { } type Artist struct { + ID string Name string MBID string } @@ -32,6 +33,7 @@ type ExternalImage struct { } type Song struct { + ID string Name string MBID string } diff --git a/core/external/provider.go b/core/external/provider.go index 413c7e0c..032805f7 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -426,17 +426,21 @@ func (e *provider) getMatchingTopSongs(ctx context.Context, agent agents.ArtistT return nil, fmt.Errorf("failed to get top songs for artist %s: %w", artistName, err) } + idMatches, err := e.loadTracksByID(ctx, songs) + if err != nil { + return nil, fmt.Errorf("failed to load tracks by ID: %w", err) + } mbidMatches, err := e.loadTracksByMBID(ctx, songs) if err != nil { return nil, fmt.Errorf("failed to load tracks by MBID: %w", err) } - titleMatches, err := e.loadTracksByTitle(ctx, songs, artist, mbidMatches) + titleMatches, err := e.loadTracksByTitle(ctx, songs, artist, idMatches, mbidMatches) if err != nil { return nil, fmt.Errorf("failed to load tracks by title: %w", err) } - log.Trace(ctx, "Top Songs loaded", "name", artistName, "numSongs", len(songs), "numMBIDMatches", len(mbidMatches), "numTitleMatches", len(titleMatches)) - mfs := e.selectTopSongs(songs, mbidMatches, titleMatches, count) + log.Trace(ctx, "Top Songs loaded", "name", artistName, "numSongs", len(songs), "numIDMatches", len(idMatches), "numMBIDMatches", len(mbidMatches), "numTitleMatches", len(titleMatches)) + mfs := e.selectTopSongs(songs, idMatches, mbidMatches, titleMatches, count) if len(mfs) == 0 { log.Debug(ctx, "No matching top songs found", "name", artistName) @@ -477,9 +481,41 @@ func (e *provider) loadTracksByMBID(ctx context.Context, songs []agents.Song) (m return matches, nil } -func (e *provider) loadTracksByTitle(ctx context.Context, songs []agents.Song, artist *auxArtist, mbidMatches map[string]model.MediaFile) (map[string]model.MediaFile, error) { +func (e *provider) loadTracksByID(ctx context.Context, songs []agents.Song) (map[string]model.MediaFile, error) { + var ids []string + for _, s := range songs { + if s.ID != "" { + ids = append(ids, s.ID) + } + } + matches := map[string]model.MediaFile{} + if len(ids) == 0 { + return matches, nil + } + res, err := e.ds.MediaFile(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.And{ + squirrel.Eq{"id": ids}, + squirrel.Eq{"missing": false}, + }, + }) + if err != nil { + return matches, err + } + for _, mf := range res { + if _, ok := matches[mf.ID]; !ok { + matches[mf.ID] = mf + } + } + return matches, nil +} + +func (e *provider) loadTracksByTitle(ctx context.Context, songs []agents.Song, artist *auxArtist, idMatches, mbidMatches map[string]model.MediaFile) (map[string]model.MediaFile, error) { titleMap := map[string]string{} for _, s := range songs { + // Skip if already matched by ID or MBID + if s.ID != "" && idMatches[s.ID].ID != "" { + continue + } if s.MBID != "" && mbidMatches[s.MBID].ID != "" { continue } @@ -518,18 +554,27 @@ func (e *provider) loadTracksByTitle(ctx context.Context, songs []agents.Song, a return matches, nil } -func (e *provider) selectTopSongs(songs []agents.Song, byMBID, byTitle map[string]model.MediaFile, count int) model.MediaFiles { +func (e *provider) selectTopSongs(songs []agents.Song, byID, byMBID, byTitle map[string]model.MediaFile, count int) model.MediaFiles { var mfs model.MediaFiles for _, t := range songs { if len(mfs) == count { break } + // Try ID match first + if t.ID != "" { + if mf, ok := byID[t.ID]; ok { + mfs = append(mfs, mf) + continue + } + } + // Try MBID match second if t.MBID != "" { if mf, ok := byMBID[t.MBID]; ok { mfs = append(mfs, mf) continue } } + // Fall back to title match if mf, ok := byTitle[str.SanitizeFieldForSorting(t.Name)]; ok { mfs = append(mfs, mf) } @@ -593,36 +638,51 @@ func (e *provider) mapSimilarArtists(ctx context.Context, similar []agents.Artis var result model.Artists var notPresent []string - artistNames := slice.Map(similar, func(artist agents.Artist) string { return artist.Name }) - - // Query all artists at once - clauses := slice.Map(artistNames, func(name string) squirrel.Sqlizer { - return squirrel.Like{"artist.name": name} - }) - artists, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Or(clauses), - }) + // Load artists by ID (highest priority) + idMatches, err := e.loadArtistsByID(ctx, similar) if err != nil { return nil, err } - // Create a map for quick lookup - artistMap := make(map[string]model.Artist) - for _, artist := range artists { - artistMap[artist.Name] = artist + // Load artists by MBID (second priority) + mbidMatches, err := e.loadArtistsByMBID(ctx, similar, idMatches) + if err != nil { + return nil, err + } + + // Load artists by name (lowest priority, fallback) + nameMatches, err := e.loadArtistsByName(ctx, similar, idMatches, mbidMatches) + if err != nil { + return nil, err } count := 0 - // Process the similar artists + // Process the similar artists using priority: ID → MBID → Name for _, s := range similar { - if artist, found := artistMap[s.Name]; found { + if count >= limit { + break + } + // Try ID match first + if s.ID != "" { + if artist, found := idMatches[s.ID]; found { + result = append(result, artist) + count++ + continue + } + } + // Try MBID match second + if s.MBID != "" { + if artist, found := mbidMatches[s.MBID]; found { + result = append(result, artist) + count++ + continue + } + } + // Fall back to name match + if artist, found := nameMatches[s.Name]; found { result = append(result, artist) count++ - - if count >= limit { - break - } } else { notPresent = append(notPresent, s.Name) } @@ -645,6 +705,95 @@ func (e *provider) mapSimilarArtists(ctx context.Context, similar []agents.Artis return result, nil } +func (e *provider) loadArtistsByID(ctx context.Context, similar []agents.Artist) (map[string]model.Artist, error) { + var ids []string + for _, s := range similar { + if s.ID != "" { + ids = append(ids, s.ID) + } + } + matches := map[string]model.Artist{} + if len(ids) == 0 { + return matches, nil + } + res, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"artist.id": ids}, + }) + if err != nil { + return matches, err + } + for _, a := range res { + if _, ok := matches[a.ID]; !ok { + matches[a.ID] = a + } + } + return matches, nil +} + +func (e *provider) loadArtistsByMBID(ctx context.Context, similar []agents.Artist, idMatches map[string]model.Artist) (map[string]model.Artist, error) { + var mbids []string + for _, s := range similar { + // Skip if already matched by ID + if s.ID != "" && idMatches[s.ID].ID != "" { + continue + } + if s.MBID != "" { + mbids = append(mbids, s.MBID) + } + } + matches := map[string]model.Artist{} + if len(mbids) == 0 { + return matches, nil + } + res, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"mbz_artist_id": mbids}, + }) + if err != nil { + return matches, err + } + for _, a := range res { + if id := a.MbzArtistID; id != "" { + if _, ok := matches[id]; !ok { + matches[id] = a + } + } + } + return matches, nil +} + +func (e *provider) loadArtistsByName(ctx context.Context, similar []agents.Artist, idMatches map[string]model.Artist, mbidMatches map[string]model.Artist) (map[string]model.Artist, error) { + var names []string + for _, s := range similar { + // Skip if already matched by ID or MBID + if s.ID != "" && idMatches[s.ID].ID != "" { + continue + } + if s.MBID != "" && mbidMatches[s.MBID].ID != "" { + continue + } + names = append(names, s.Name) + } + matches := map[string]model.Artist{} + if len(names) == 0 { + return matches, nil + } + clauses := slice.Map(names, func(name string) squirrel.Sqlizer { + return squirrel.Like{"artist.name": name} + }) + res, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Or(clauses), + }) + if err != nil { + return matches, err + } + for _, a := range res { + if _, ok := matches[a.Name]; !ok { + matches[a.Name] = a + } + } + return matches, nil +} + func (e *provider) findArtistByName(ctx context.Context, artistName string) (*auxArtist, error) { artists, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ Filters: squirrel.Like{"artist.name": artistName}, diff --git a/core/external/provider_artistradio_test.go b/core/external/provider_artistradio_test.go index 21ea0770..18afede6 100644 --- a/core/external/provider_artistradio_test.go +++ b/core/external/provider_artistradio_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" + "github.com/Masterminds/squirrel" "github.com/navidrome/navidrome/core/agents" . "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/model" @@ -67,8 +68,16 @@ var _ = Describe("Provider - ArtistRadio", func() { mockAgent.On("GetSimilarArtists", mock.Anything, "artist-1", "Artist One", "", 15). Return(similarAgentsResp, nil).Once() + // Mock the three-phase artist lookup: ID (skipped - no IDs), MBID, then Name + // MBID lookup returns empty (no match) artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { - return opt.Max == 0 && opt.Filters != nil + _, ok := opt.Filters.(squirrel.Eq) + return opt.Max == 0 && ok + })).Return(model.Artists{}, nil).Once() + // Name lookup returns the similar artist + artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool { + _, ok := opt.Filters.(squirrel.Or) + return opt.Max == 0 && ok })).Return(model.Artists{similarArtist}, nil).Once() mockAgent.On("GetArtistTopSongs", mock.Anything, "artist-1", "Artist One", "", mock.Anything). diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index 5a5a2571..5a98817e 100644 --- a/core/external/provider_topsongs_test.go +++ b/core/external/provider_topsongs_test.go @@ -271,4 +271,60 @@ var _ = Describe("Provider - TopSongs", func() { ag.AssertExpectations(GinkgoT()) mediaFileRepo.AssertExpectations(GinkgoT()) }) + + It("matches songs by ID first when agent provides IDs", func() { + // Mock finding the artist + artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} + artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + + // Mock agent response with IDs provided (highest priority matching) + // Note: Songs have no MBID to ensure only ID matching is used + agentSongs := []agents.Song{ + {ID: "song-1", Name: "Song One"}, + {ID: "song-2", Name: "Song Two"}, + } + ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once() + + // Mock ID lookup (first query - should match both songs directly) + song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"} + song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"} + mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() + + songs, err := p.TopSongs(ctx, "Artist One", 2) + + Expect(err).ToNot(HaveOccurred()) + Expect(songs).To(HaveLen(2)) + Expect(songs[0].ID).To(Equal("song-1")) + Expect(songs[1].ID).To(Equal("song-2")) + artistRepo.AssertExpectations(GinkgoT()) + ag.AssertExpectations(GinkgoT()) + mediaFileRepo.AssertExpectations(GinkgoT()) + }) + + It("falls back to MBID when ID is not found", func() { + // Mock finding the artist + artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} + artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + + // Mock agent response with ID that won't be found, but MBID that will + agentSongs := []agents.Song{ + {ID: "non-existent-id", Name: "Song One", MBID: "mbid-song-1"}, + } + ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 1).Return(agentSongs, nil).Once() + + // Mock ID lookup - returns empty (ID not found) + mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once() + // Mock MBID lookup - finds the song + song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1"} + mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() + + songs, err := p.TopSongs(ctx, "Artist One", 1) + + Expect(err).ToNot(HaveOccurred()) + Expect(songs).To(HaveLen(1)) + Expect(songs[0].ID).To(Equal("song-1")) + artistRepo.AssertExpectations(GinkgoT()) + ag.AssertExpectations(GinkgoT()) + mediaFileRepo.AssertExpectations(GinkgoT()) + }) }) diff --git a/core/external/provider_updateartistinfo_test.go b/core/external/provider_updateartistinfo_test.go index 9b1e8d86..0c489ead 100644 --- a/core/external/provider_updateartistinfo_test.go +++ b/core/external/provider_updateartistinfo_test.go @@ -226,4 +226,88 @@ var _ = Describe("Provider - UpdateArtistInfo", func() { Expect(updatedArtist.ID).To(Equal("ar-agent-fail")) ag.AssertExpectations(GinkgoT()) }) + + It("matches similar artists by ID first when agent provides IDs", func() { + originalArtist := &model.Artist{ + ID: "ar-id-match", + Name: "ID Match Artist", + } + similarByID := model.Artist{ID: "ar-similar-by-id", Name: "Similar By ID", MbzArtistID: "mbid-similar"} + mockArtistRepo.SetData(model.Artists{*originalArtist, similarByID}) + + // Agent returns similar artist with ID (highest priority matching) + rawSimilar := []agents.Artist{ + {ID: "ar-similar-by-id", Name: "Different Name", MBID: "different-mbid"}, + } + + ag.On("GetArtistMBID", ctx, "ar-id-match", "ID Match Artist").Return("", nil).Once() + ag.On("GetArtistImages", ctx, "ar-id-match", "ID Match Artist", mock.Anything).Return(nil, nil).Maybe() + ag.On("GetArtistBiography", ctx, "ar-id-match", "ID Match Artist", mock.Anything).Return("", nil).Maybe() + ag.On("GetArtistURL", ctx, "ar-id-match", "ID Match Artist", mock.Anything).Return("", nil).Maybe() + ag.On("GetSimilarArtists", ctx, "ar-id-match", "ID Match Artist", mock.Anything, 100).Return(rawSimilar, nil).Once() + + updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-id-match", 10, false) + + Expect(err).NotTo(HaveOccurred()) + Expect(updatedArtist.SimilarArtists).To(HaveLen(1)) + // Should match by ID, not by name or MBID + Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-by-id")) + Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar By ID")) + }) + + It("matches similar artists by MBID when ID is empty", func() { + originalArtist := &model.Artist{ + ID: "ar-mbid-match", + Name: "MBID Match Artist", + } + similarByMBID := model.Artist{ID: "ar-similar-by-mbid", Name: "Similar By MBID", MbzArtistID: "mbid-similar"} + mockArtistRepo.SetData(model.Artists{*originalArtist, similarByMBID}) + + // Agent returns similar artist with only MBID (no ID) + rawSimilar := []agents.Artist{ + {Name: "Different Name", MBID: "mbid-similar"}, + } + + ag.On("GetArtistMBID", ctx, "ar-mbid-match", "MBID Match Artist").Return("", nil).Once() + ag.On("GetArtistImages", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything).Return(nil, nil).Maybe() + ag.On("GetArtistBiography", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything).Return("", nil).Maybe() + ag.On("GetArtistURL", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything).Return("", nil).Maybe() + ag.On("GetSimilarArtists", ctx, "ar-mbid-match", "MBID Match Artist", mock.Anything, 100).Return(rawSimilar, nil).Once() + + updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-mbid-match", 10, false) + + Expect(err).NotTo(HaveOccurred()) + Expect(updatedArtist.SimilarArtists).To(HaveLen(1)) + // Should match by MBID since ID was empty + Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-by-mbid")) + Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar By MBID")) + }) + + It("falls back to name matching when ID and MBID don't match", func() { + originalArtist := &model.Artist{ + ID: "ar-name-match", + Name: "Name Match Artist", + } + similarByName := model.Artist{ID: "ar-similar-by-name", Name: "Similar By Name"} + mockArtistRepo.SetData(model.Artists{*originalArtist, similarByName}) + + // Agent returns similar artist with non-matching ID and MBID + rawSimilar := []agents.Artist{ + {ID: "non-existent-id", Name: "Similar By Name", MBID: "non-existent-mbid"}, + } + + ag.On("GetArtistMBID", ctx, "ar-name-match", "Name Match Artist").Return("", nil).Once() + ag.On("GetArtistImages", ctx, "ar-name-match", "Name Match Artist", mock.Anything).Return(nil, nil).Maybe() + ag.On("GetArtistBiography", ctx, "ar-name-match", "Name Match Artist", mock.Anything).Return("", nil).Maybe() + ag.On("GetArtistURL", ctx, "ar-name-match", "Name Match Artist", mock.Anything).Return("", nil).Maybe() + ag.On("GetSimilarArtists", ctx, "ar-name-match", "Name Match Artist", mock.Anything, 100).Return(rawSimilar, nil).Once() + + updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-name-match", 10, false) + + Expect(err).NotTo(HaveOccurred()) + Expect(updatedArtist.SimilarArtists).To(HaveLen(1)) + // Should fall back to name matching since ID and MBID didn't match + Expect(updatedArtist.SimilarArtists[0].ID).To(Equal("ar-similar-by-name")) + Expect(updatedArtist.SimilarArtists[0].Name).To(Equal("Similar By Name")) + }) })