From 91e7f7b5c9526f13e64c2478273c1cb9cade4ba1 Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Sun, 29 Jun 2025 16:19:29 +0000 Subject: [PATCH] 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 | 2 ++ core/agents/agents.go | 14 +++++++++-- core/agents/agents_test.go | 12 ++++++++++ core/external/provider.go | 18 +++++++++++--- core/external/provider_topsongs_test.go | 31 +++++++++++++++++++++---- 5 files changed, 68 insertions(+), 9 deletions(-) diff --git a/conf/configuration.go b/conf/configuration.go index 7ea16bf4b..258e3727f 100644 --- a/conf/configuration.go +++ b/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() { diff --git a/core/agents/agents.go b/core/agents/agents.go index bfffb84b6..efa9f383d 100644 --- a/core/agents/agents.go +++ b/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 diff --git a/core/agents/agents_test.go b/core/agents/agents_test.go index 13583a4de..0732d43ef 100644 --- a/core/agents/agents_test.go +++ b/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() { diff --git a/core/external/provider.go b/core/external/provider.go index 1b5a2dab4..8e9a458c1 100644 --- a/core/external/provider.go +++ b/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 + } } } diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index 443be36dd..5a5a25714 100644 --- a/core/external/provider_topsongs_test.go +++ b/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()) + }) })