fix(server): ensure that similar artists retrieved from provider are no more than limit (#4267)

* fix(provider): ensure that similar artists retreived from provider are no more than limit

* add overlimit multiplier

conf/configuration.go

@@ -128,6 +128,7 @@ type configOptions struct {
 	DevInsightsInitialDelay          time.Duration
 	DevEnablePlayerInsights          bool
 	DevPluginCompilationTimeout      time.Duration
+	DevExternalArtistFetchMultiplier float64
 }
 
 type scannerOptions struct {
@@ -599,6 +600,7 @@ func setViperDefaults() {
 	viper.SetDefault("devinsightsinitialdelay", consts.InsightsInitialDelay)
 	viper.SetDefault("devenableplayerinsights", true)
 	viper.SetDefault("devplugincompilationtimeout", time.Minute)
+	viper.SetDefault("devexternalartistfetchmultiplier", 1.5)
 }
 
 func init() {

core/agents/agents.go

@@ -258,6 +258,8 @@ func (a *Agents) GetArtistBiography(ctx context.Context, id, name, mbid string)
 	return "", ErrNotFound
 }
 
+// GetSimilarArtists returns similar artists by id, name, and/or mbid. Because some artists returned from an enabled
+// agent may not exist in the database, return at most limit * conf.Server.DevExternalArtistFetchMultiplier items.
 func (a *Agents) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]Artist, error) {
 	switch id {
 	case consts.UnknownArtistID:
@@ -265,6 +267,9 @@ func (a *Agents) GetSimilarArtists(ctx context.Context, id, name, mbid string, l
 	case consts.VariousArtistsID:
 		return nil, nil
 	}
+
+	overLimit := int(float64(limit) * conf.Server.DevExternalArtistFetchMultiplier)
+
 	start := time.Now()
 	for _, agentName := range a.getEnabledAgentNames() {
 		ag := a.getAgent(agentName)
@@ -278,7 +283,7 @@ func (a *Agents) GetSimilarArtists(ctx context.Context, id, name, mbid string, l
 		if !ok {
 			continue
 		}
-		similar, err := retriever.GetSimilarArtists(ctx, id, name, mbid, limit)
+		similar, err := retriever.GetSimilarArtists(ctx, id, name, mbid, overLimit)
 		if len(similar) > 0 && err == nil {
 			if log.IsGreaterOrEqualTo(log.LevelTrace) {
 				log.Debug(ctx, "Got Similar Artists", "agent", ag.AgentName(), "artist", name, "similar", similar, "elapsed", time.Since(start))
@@ -320,6 +325,8 @@ func (a *Agents) GetArtistImages(ctx context.Context, id, name, mbid string) ([]
 	return nil, ErrNotFound
 }
 
+// GetArtistTopSongs returns top songs by id, name, and/or mbid. Because some songs returned from an enabled
+// agent may not exist in the database, return at most limit * conf.Server.DevExternalArtistFetchMultiplier items.
 func (a *Agents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]Song, error) {
 	switch id {
 	case consts.UnknownArtistID:
@@ -327,6 +334,9 @@ func (a *Agents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid str
 	case consts.VariousArtistsID:
 		return nil, nil
 	}
+
+	overLimit := int(float64(count) * conf.Server.DevExternalArtistFetchMultiplier)
+
 	start := time.Now()
 	for _, agentName := range a.getEnabledAgentNames() {
 		ag := a.getAgent(agentName)
@@ -340,7 +350,7 @@ func (a *Agents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid str
 		if !ok {
 			continue
 		}
-		songs, err := retriever.GetArtistTopSongs(ctx, id, artistName, mbid, count)
+		songs, err := retriever.GetArtistTopSongs(ctx, id, artistName, mbid, overLimit)
 		if len(songs) > 0 && err == nil {
 			log.Debug(ctx, "Got Top Songs", "agent", ag.AgentName(), "artist", artistName, "songs", songs, "elapsed", time.Since(start))
 			return songs, nil

core/agents/agents_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 
+	"github.com/navidrome/navidrome/conf/configtest"
 	"github.com/navidrome/navidrome/consts"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/tests"
@@ -19,6 +20,7 @@ var _ = Describe("Agents", func() {
 	var ds model.DataStore
 	var mfRepo *tests.MockMediaFileRepo
 	BeforeEach(func() {
+		DeferCleanup(configtest.SetupConfig())
 		ctx, cancel = context.WithCancel(context.Background())
 		mfRepo = tests.CreateMockMediaFileRepo()
 		ds = &tests.MockDataStore{MockedMediaFile: mfRepo}
@@ -240,6 +242,7 @@ var _ = Describe("Agents", func() {
 
 		Describe("GetArtistTopSongs", func() {
 			It("returns on first match", func() {
+				conf.Server.DevExternalArtistFetchMultiplier = 1
 				Expect(ag.GetArtistTopSongs(ctx, "123", "test", "mb123", 2)).To(Equal([]Song{{
 					Name: "A Song",
 					MBID: "mbid444",
@@ -247,6 +250,7 @@ var _ = Describe("Agents", func() {
 				Expect(mock.Args).To(HaveExactElements("123", "test", "mb123", 2))
 			})
 			It("skips the agent if it returns an error", func() {
+				conf.Server.DevExternalArtistFetchMultiplier = 1
 				mock.Err = errors.New("error")
 				_, err := ag.GetArtistTopSongs(ctx, "123", "test", "mb123", 2)
 				Expect(err).To(MatchError(ErrNotFound))
@@ -258,6 +262,14 @@ var _ = Describe("Agents", func() {
 				Expect(err).To(MatchError(ErrNotFound))
 				Expect(mock.Args).To(BeEmpty())
 			})
+			It("fetches with multiplier", func() {
+				conf.Server.DevExternalArtistFetchMultiplier = 2
+				Expect(ag.GetArtistTopSongs(ctx, "123", "test", "mb123", 2)).To(Equal([]Song{{
+					Name: "A Song",
+					MBID: "mbid444",
+				}}))
+				Expect(mock.Args).To(HaveExactElements("123", "test", "mb123", 4))
+			})
 		})
 
 		Describe("GetAlbumInfo", func() {

core/external/provider.go

@@ -560,7 +560,7 @@ func (e *provider) callGetSimilar(ctx context.Context, agent agents.ArtistSimila
 		return
 	}
 	start := time.Now()
-	sa, err := e.mapSimilarArtists(ctx, similar, includeNotPresent)
+	sa, err := e.mapSimilarArtists(ctx, similar, limit, includeNotPresent)
 	log.Debug(ctx, "Mapped Similar Artists", "artist", artist.Name, "numSimilar", len(sa), "elapsed", time.Since(start))
 	if err != nil {
 		return
@@ -568,7 +568,7 @@ func (e *provider) callGetSimilar(ctx context.Context, agent agents.ArtistSimila
 	artist.SimilarArtists = sa
 }
 
-func (e *provider) mapSimilarArtists(ctx context.Context, similar []agents.Artist, includeNotPresent bool) (model.Artists, error) {
+func (e *provider) mapSimilarArtists(ctx context.Context, similar []agents.Artist, limit int, includeNotPresent bool) (model.Artists, error) {
 	var result model.Artists
 	var notPresent []string
 
@@ -591,21 +591,33 @@ func (e *provider) mapSimilarArtists(ctx context.Context, similar []agents.Artis
 		artistMap[artist.Name] = artist
 	}
 
+	count := 0
+
 	// Process the similar artists
 	for _, s := range similar {
 		if artist, found := artistMap[s.Name]; found {
 			result = append(result, artist)
+			count++
+
+			if count >= limit {
+				break
+			}
 		} else {
 			notPresent = append(notPresent, s.Name)
 		}
 	}
 
 	// Then fill up with non-present artists
-	if includeNotPresent {
+	if includeNotPresent && count < limit {
 		for _, s := range notPresent {
 			// Let the ID empty to indicate that the artist is not present in the DB
 			sa := model.Artist{Name: s}
 			result = append(result, sa)
+
+			count++
+			if count >= limit {
+				break
+			}
 		}
 	}
 

core/external/provider_topsongs_test.go

@@ -42,10 +42,6 @@ var _ = Describe("Provider - TopSongs", func() {
 		p = NewProvider(ds, ag)
 	})
 
-	BeforeEach(func() {
-		// Setup expectations in individual tests
-	})
-
 	It("returns top songs for a known artist", func() {
 		// Mock finding the artist
 		artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"}
@@ -248,4 +244,31 @@ var _ = Describe("Provider - TopSongs", func() {
 		ag.AssertExpectations(GinkgoT())
 		mediaFileRepo.AssertExpectations(GinkgoT())
 	})
+
+	It("only returns requested count when provider returns additional items", 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
+		agentSongs := []agents.Song{
+			{Name: "Song One", MBID: "mbid-song-1"},
+			{Name: "Song Two", MBID: "mbid-song-2"},
+		}
+		ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 1).Return(agentSongs, nil).Once()
+
+		// 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, song2}, 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())
+	})
 })