refactor(server): optimize top songs lookup (#4189)

* optimize top songs lookup * Optimize title matching queries * refactor: simplify top songs matching * improve error handling and logging in track loading functions Signed-off-by: Deluan <deluan@navidrome.org> * test: add cases for fallback to title matching and combined MBID/title matching Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org>
2025-12-23 23:18:05 -05:00 · 2025-06-08 11:44:44 -04:00
parent 844966df89
commit bc733540f9
3 changed files with 166 additions and 49 deletions
--- a/core/external/provider.go
+++ b/core/external/provider.go
@@ -3,6 +3,7 @@ package external
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/url"
 	"sort"
 	"strings"
@@ -400,20 +401,21 @@ func (e *provider) TopSongs(ctx context.Context, artistName string, count int) (
 func (e *provider) getMatchingTopSongs(ctx context.Context, agent agents.ArtistTopSongsRetriever, artist *auxArtist, count int) (model.MediaFiles, error) {
 	songs, err := agent.GetArtistTopSongs(ctx, artist.ID, artist.Name, artist.MbzArtistID, count)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to get top songs for artist %s: %w", artist.Name, err)
 	}

-	var mfs model.MediaFiles
-	for _, t := range songs {
-		mf, err := e.findMatchingTrack(ctx, t.MBID, artist.ID, t.Name)
-		if err != nil {
-			continue
-		}
-		mfs = append(mfs, *mf)
-		if len(mfs) == count {
-			break
-		}
+	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)
+	if err != nil {
+		return nil, fmt.Errorf("failed to load tracks by title: %w", err)
+	}
+
+	log.Trace(ctx, "Top Songs loaded", "name", artist.Name, "numSongs", len(songs), "numMBIDMatches", len(mbidMatches), "numTitleMatches", len(titleMatches))
+	mfs := e.selectTopSongs(songs, mbidMatches, titleMatches, count)
+
 	if len(mfs) == 0 {
 		log.Debug(ctx, "No matching top songs found", "name", artist.Name)
 	} else {
@@ -423,35 +425,94 @@ func (e *provider) getMatchingTopSongs(ctx context.Context, agent agents.ArtistT
 	return mfs, nil
 }

-func (e *provider) findMatchingTrack(ctx context.Context, mbid string, artistID, title string) (*model.MediaFile, error) {
-	if mbid != "" {
-		mfs, err := e.ds.MediaFile(ctx).GetAll(model.QueryOptions{
-			Filters: squirrel.And{
-				squirrel.Eq{"mbz_recording_id": mbid},
-				squirrel.Eq{"missing": false},
-			},
-		})
-		if err == nil && len(mfs) > 0 {
-			return &mfs[0], nil
+func (e *provider) loadTracksByMBID(ctx context.Context, songs []agents.Song) (map[string]model.MediaFile, error) {
+	var mbids []string
+	for _, s := range songs {
+		if s.MBID != "" {
+			mbids = append(mbids, s.MBID)
 		}
-		return e.findMatchingTrack(ctx, "", artistID, title)
 	}
-	mfs, err := e.ds.MediaFile(ctx).GetAll(model.QueryOptions{
+	matches := map[string]model.MediaFile{}
+	if len(mbids) == 0 {
+		return matches, nil
+	}
+	res, err := e.ds.MediaFile(ctx).GetAll(model.QueryOptions{
+		Filters: squirrel.And{
+			squirrel.Eq{"mbz_recording_id": mbids},
+			squirrel.Eq{"missing": false},
+		},
+	})
+	if err != nil {
+		return matches, err
+	}
+	for _, mf := range res {
+		if id := mf.MbzRecordingID; id != "" {
+			if _, ok := matches[id]; !ok {
+				matches[id] = mf
+			}
+		}
+	}
+	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) {
+	titleMap := map[string]string{}
+	for _, s := range songs {
+		if s.MBID != "" && mbidMatches[s.MBID].ID != "" {
+			continue
+		}
+		sanitized := str.SanitizeFieldForSorting(s.Name)
+		titleMap[sanitized] = s.Name
+	}
+	matches := map[string]model.MediaFile{}
+	if len(titleMap) == 0 {
+		return matches, nil
+	}
+	titleFilters := squirrel.Or{}
+	for sanitized := range titleMap {
+		titleFilters = append(titleFilters, squirrel.Like{"order_title": sanitized})
+	}
+
+	res, err := e.ds.MediaFile(ctx).GetAll(model.QueryOptions{
 		Filters: squirrel.And{
 			squirrel.Or{
-				squirrel.Eq{"artist_id": artistID},
-				squirrel.Eq{"album_artist_id": artistID},
+				squirrel.Eq{"artist_id": artist.ID},
+				squirrel.Eq{"album_artist_id": artist.ID},
 			},
-			squirrel.Like{"order_title": str.SanitizeFieldForSorting(title)},
+			titleFilters,
 			squirrel.Eq{"missing": false},
 		},
 		Sort: "starred desc, rating desc, year asc, compilation asc ",
-		Max:  1,
 	})
-	if err != nil || len(mfs) == 0 {
-		return nil, model.ErrNotFound
+	if err != nil {
+		return matches, err
 	}
-	return &mfs[0], nil
+	for _, mf := range res {
+		sanitized := str.SanitizeFieldForSorting(mf.Title)
+		if _, ok := matches[sanitized]; !ok {
+			matches[sanitized] = mf
+		}
+	}
+	return matches, nil
+}
+
+func (e *provider) selectTopSongs(songs []agents.Song, byMBID, byTitle map[string]model.MediaFile, count int) model.MediaFiles {
+	var mfs model.MediaFiles
+	for _, t := range songs {
+		if len(mfs) == count {
+			break
+		}
+		if t.MBID != "" {
+			if mf, ok := byMBID[t.MBID]; ok {
+				mfs = append(mfs, mf)
+				continue
+			}
+		}
+		if mf, ok := byTitle[str.SanitizeFieldForSorting(t.Name)]; ok {
+			mfs = append(mfs, mf)
+		}
+	}
+	return mfs
 }

 func (e *provider) callGetURL(ctx context.Context, agent agents.ArtistURLRetriever, artist *auxArtist) {
--- a/core/external/provider_similarsongs_test.go
+++ b/core/external/provider_similarsongs_test.go
@@ -50,9 +50,9 @@ var _ = Describe("Provider - SimilarSongs", func() {
 	It("returns similar songs from main artist and similar artists", func() {
 		artist1 := model.Artist{ID: "artist-1", Name: "Artist One"}
 		similarArtist := model.Artist{ID: "artist-3", Name: "Similar Artist"}
-		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"}
-		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"}
-		song3 := model.MediaFile{ID: "song-3", Title: "Song Three", ArtistID: "artist-3"}
+		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-1"}
+		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "mbid-2"}
+		song3 := model.MediaFile{ID: "song-3", Title: "Song Three", ArtistID: "artist-3", MbzRecordingID: "mbid-3"}

 		artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe()
 		artistRepo.On("Get", "artist-3").Return(&similarArtist, nil).Maybe()
@@ -82,9 +82,8 @@ var _ = Describe("Provider - SimilarSongs", func() {
 				{Name: "Song Three", MBID: "mbid-3"},
 			}, nil).Once()

-		mediaFileRepo.FindByMBID("mbid-1", song1)
-		mediaFileRepo.FindByMBID("mbid-2", song2)
-		mediaFileRepo.FindByMBID("mbid-3", song3)
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once()
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song3}, nil).Once()

 		songs, err := provider.SimilarSongs(ctx, "artist-1", 3)

@@ -111,7 +110,7 @@ var _ = Describe("Provider - SimilarSongs", func() {

 	It("returns songs from main artist when GetSimilarArtists returns error", func() {
 		artist1 := model.Artist{ID: "artist-1", Name: "Artist One"}
-		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"}
+		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-1"}

 		artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe()
 		artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
@@ -130,7 +129,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
 				{Name: "Song One", MBID: "mbid-1"},
 			}, nil).Once()

-		mediaFileRepo.FindByMBID("mbid-1", song1)
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once()

 		songs, err := provider.SimilarSongs(ctx, "artist-1", 5)

@@ -165,8 +164,8 @@ var _ = Describe("Provider - SimilarSongs", func() {

 	It("respects count parameter", func() {
 		artist1 := model.Artist{ID: "artist-1", Name: "Artist One"}
-		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1"}
-		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"}
+		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-1"}
+		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "mbid-2"}

 		artistRepo.On("Get", "artist-1").Return(&artist1, nil).Maybe()
 		artistRepo.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
@@ -186,8 +185,7 @@ var _ = Describe("Provider - SimilarSongs", func() {
 				{Name: "Song Two", MBID: "mbid-2"},
 			}, nil).Once()

-		mediaFileRepo.FindByMBID("mbid-1", song1)
-		mediaFileRepo.FindByMBID("mbid-2", song2)
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once()

 		songs, err := provider.SimilarSongs(ctx, "artist-1", 1)

--- a/core/external/provider_topsongs_test.go
+++ b/core/external/provider_topsongs_test.go
@@ -58,11 +58,10 @@ var _ = Describe("Provider - TopSongs", func() {
 		}
 		ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once()

-		// Mock finding matching tracks
+		// Mock finding matching tracks (both returned in a single query)
 		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1"}
 		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "mbid-song-2"}
-		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once()
-		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song2}, nil).Once()
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once()

 		songs, err := p.TopSongs(ctx, "Artist One", 2)

@@ -155,11 +154,10 @@ var _ = Describe("Provider - TopSongs", func() {
 		}
 		ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once()

-		// Mock finding matching tracks (only find song 1)
+		// Mock finding matching tracks (only find song 1 on bulk query)
 		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()
-		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once() // For mbid-song-2 (fails)
-		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once() // For title fallback (fails)
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once() // bulk MBID query
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{}, nil).Once()      // title fallback for song2

 		songs, err := p.TopSongs(ctx, "Artist One", 2)

@@ -190,4 +188,64 @@ var _ = Describe("Provider - TopSongs", func() {
 		artistRepo.AssertExpectations(GinkgoT())
 		ag.AssertExpectations(GinkgoT())
 	})
+
+	It("falls back to title matching when MbzRecordingID is missing", 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 songs that have NO MBID (empty string)
+		agentSongs := []agents.Song{
+			{Name: "Song One", MBID: ""}, // No MBID, should fall back to title matching
+			{Name: "Song Two", MBID: ""}, // No MBID, should fall back to title matching
+		}
+		ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once()
+
+		// Since there are no MBIDs, loadTracksByMBID should not make any database call
+		// loadTracksByTitle should make a database call for title matching
+		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "", OrderTitle: "song one"}
+		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "", OrderTitle: "song two"}
+		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("combines MBID and title matching when some songs have missing MbzRecordingID", 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 mixed MBID availability
+		agentSongs := []agents.Song{
+			{Name: "Song One", MBID: "mbid-song-1"}, // Has MBID, should match by MBID
+			{Name: "Song Two", MBID: ""},            // No MBID, should fall back to title matching
+		}
+		ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once()
+
+		// Mock the MBID query (finds song1 by MBID)
+		song1 := model.MediaFile{ID: "song-1", Title: "Song One", ArtistID: "artist-1", MbzRecordingID: "mbid-song-1", OrderTitle: "song one"}
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1}, nil).Once()
+
+		// Mock the title fallback query (finds song2 by title)
+		song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1", MbzRecordingID: "", OrderTitle: "song two"}
+		mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{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")) // Found by MBID
+		Expect(songs[1].ID).To(Equal("song-2")) // Found by title
+		artistRepo.AssertExpectations(GinkgoT())
+		ag.AssertExpectations(GinkgoT())
+		mediaFileRepo.AssertExpectations(GinkgoT())
+	})
 })