mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-31 07:30:32 +00:00
757ca783d3
* fix(instant-mix): top short mixes up instead of returning what the first source found
SimilarSongs returned the first non-empty source's tracks, however few. For a
thinly-represented artist that meant a 3-track mix no matter the requested count:
the artist agent found nothing, the similar-artists fallback matched 3 library
tracks, and `len(res) > 0` kept seed-track sampling from ever running.
Clients treat that as a failed mix and retry with a bigger limit forever. Finamp
cycles limit 34 through 472 and starts over, ~1 request every 2s indefinitely,
each one re-hitting Last.fm, Deezer and AudioMuse.
Sources are now chained rather than raced: each one tops the mix up until it
holds count tracks, so the agent's picks, the similar-artists fallback and
seed-track sampling all contribute instead of the first one winning outright.
* refactor(external): move similar-songs code to its own file
provider.go held two distinct concerns: artist/album external metadata and the
similar-songs mix pipeline. The mix code was already one contiguous block, and
maxSeeds, maxSimilarSongs and dedupByID were used by nothing else.
Moved SimilarSongs and its helpers to provider_similarsongs.go, matching the
existing provider_similarsongs_test.go. Pure code motion: the moved block is
byte-for-byte unchanged and provider.go has no additions, only deletions.
* fix(instant-mix): dedup before deciding a mix is full
topUp measured res before deduplicating it. Matcher.MatchSongs deliberately
re-emits a library track when the same input song repeats, and the similar-artists
fallback can reach one track through several artists, so len(res) could equal count
while holding fewer unique tracks. That returned a mix with duplicates in it and
stopped the top-up early; the caller then deduplicated and handed back a short mix,
which is the client retry loop this branch set out to fix.
Deduplicate first, so the length check counts what the client will actually receive.
* perf(instant-mix): skip a fallback once the mix is already full
The artist path nested one topUp inside another, so the inner one measured only
similarSongsFallback's own result against the full count. With 49 agent matches and
one fallback match for count=50 the mix was already full, yet seed-track sampling
still ran and fired up to five GetSimilarSongsByTrack calls whose results the outer
topUp then truncated away.
topUp now takes the sources as a variadic list and re-checks the accumulated mix
before each one, so a later, costlier source only runs while the mix is still short.
That also flattens the artist case: the agent, the similar-artists fallback and
seed-track sampling are now three peers in one chain instead of two nested calls.
* fix(instant-mix): count distinct tracks when picking the fallback mix
similarSongsFallback stopped after count picks from the weighted chooser, but a
track can sit in that chooser once per artist listing it in their top songs, and
Pick removes the entry it returns. Repeats therefore consumed pick slots and left
unique candidates stranded, so the batch could come back short of count. On the
track path this is the only source, so that short mix reached the client and kept
the retry loop alive.
Track the ids already picked and keep drawing until count distinct tracks are held
or the chooser is empty.
* fix(instant-mix): match the whole agent response before trimming
MatchSongs was capped at count, and it re-emits a track when the same song repeats,
so [A, A, B] with count=2 returned [A, A] and never reached B. topUp then shrank
that to [A] and, with an empty or overlapping fallback, the mix stayed short even
though B had been available all along. seedMix already matched its full merged set
for this reason; mixFromAgent now does the same and leaves the trim to topUp.
Also drop the capacity hint on the picked-ids map. It was sized from the caller's
count, which CodeQL flags as an allocation sized by user input (go/uncontrolled-
allocation-size). SimilarSongs clamps count to maxSimilarSongs long before this
point, so the hint bought nothing worth the alert.
* refactor(instant-mix): tidy the mix chain and its specs
Quality pass over the new code, no behaviour change:
- topUp: drop the first-vs-last error bookkeeping (the value is only read when the
mix is empty, so the distinction is unobservable) and the redundant nil guard
(dedupByID returns nil for an empty result, so both branches already agreed).
- mixFromAgent: assign through the if-scoped err instead of a second error name.
- Hoist the similar-artists fallback closure written verbatim in two switch arms.
- Use map[string]struct{} in the pick loop, matching dedupByID in the same file.
- Trim three comments back within budget; two restated the line below them and one
carried commit-message rationale.
- Tests: add an ids() helper for the ID assertion repeated seven times, and fold
the track-entity stub block copied into three specs into stubTrackEntity. The
block hard-coded .Twice() on GetEntityByID, which pinned an implementation
detail no spec asserts.
* revert(instant-mix): inline the similar-artists fallback closure again
Hoisting it to a shared artistFallback var moved the call away from the arm that
uses it and saved nothing: each arm reads better spelling out its own sources.
332 lines
9.9 KiB
Go
332 lines
9.9 KiB
Go
package external_test
|
|
|
|
import (
|
|
"context"
|
|
"errors"
|
|
|
|
"github.com/navidrome/navidrome/core/agents"
|
|
"github.com/navidrome/navidrome/model"
|
|
"github.com/navidrome/navidrome/utils/slice"
|
|
"github.com/stretchr/testify/mock"
|
|
)
|
|
|
|
// --- Shared Mock Implementations ---
|
|
|
|
// mockArtistRepo mocks model.ArtistRepository
|
|
type mockArtistRepo struct {
|
|
mock.Mock
|
|
model.ArtistRepository
|
|
}
|
|
|
|
func newMockArtistRepo() *mockArtistRepo {
|
|
return &mockArtistRepo{}
|
|
}
|
|
|
|
// SetData sets up basic Get expectations.
|
|
func (m *mockArtistRepo) SetData(artists model.Artists) {
|
|
for _, a := range artists {
|
|
artistCopy := a
|
|
m.On("Get", artistCopy.ID).Return(&artistCopy, nil)
|
|
}
|
|
}
|
|
|
|
// Get implements model.ArtistRepository.
|
|
func (m *mockArtistRepo) Get(id string) (*model.Artist, error) {
|
|
args := m.Called(id)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(*model.Artist), args.Error(1)
|
|
}
|
|
|
|
// GetAll implements model.ArtistRepository.
|
|
func (m *mockArtistRepo) GetAll(options ...model.QueryOptions) (model.Artists, error) {
|
|
argsSlice := make([]any, len(options))
|
|
for i, v := range options {
|
|
argsSlice[i] = v
|
|
}
|
|
args := m.Called(argsSlice...)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(model.Artists), args.Error(1)
|
|
}
|
|
|
|
// SetError is a helper to set up a generic error for GetAll.
|
|
func (m *mockArtistRepo) SetError(hasError bool) {
|
|
if hasError {
|
|
m.On("GetAll", mock.Anything).Return(nil, errors.New("mock repo error"))
|
|
}
|
|
}
|
|
|
|
// FindByName is a helper to set up a GetAll expectation for finding by name.
|
|
func (m *mockArtistRepo) FindByName(name string, artist model.Artist) {
|
|
m.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
|
|
return opt.Filters != nil
|
|
})).Return(model.Artists{artist}, nil).Once()
|
|
}
|
|
|
|
// mockMediaFileRepo mocks model.MediaFileRepository
|
|
type mockMediaFileRepo struct {
|
|
mock.Mock
|
|
model.MediaFileRepository
|
|
}
|
|
|
|
func newMockMediaFileRepo() *mockMediaFileRepo {
|
|
return &mockMediaFileRepo{}
|
|
}
|
|
|
|
// SetData sets up basic Get expectations.
|
|
func (m *mockMediaFileRepo) SetData(mediaFiles model.MediaFiles) {
|
|
for _, mf := range mediaFiles {
|
|
mfCopy := mf
|
|
m.On("Get", mfCopy.ID).Return(&mfCopy, nil)
|
|
}
|
|
}
|
|
|
|
// Get implements model.MediaFileRepository.
|
|
func (m *mockMediaFileRepo) Get(id string) (*model.MediaFile, error) {
|
|
args := m.Called(id)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(*model.MediaFile), args.Error(1)
|
|
}
|
|
|
|
// GetAllByTags implements model.MediaFileRepository.
|
|
func (m *mockMediaFileRepo) GetAllByTags(_ model.TagName, _ []string, options ...model.QueryOptions) (model.MediaFiles, error) {
|
|
return m.GetAll(options...)
|
|
}
|
|
|
|
// GetAll implements model.MediaFileRepository.
|
|
func (m *mockMediaFileRepo) GetAll(options ...model.QueryOptions) (model.MediaFiles, error) {
|
|
argsSlice := make([]any, len(options))
|
|
for i, v := range options {
|
|
argsSlice[i] = v
|
|
}
|
|
args := m.Called(argsSlice...)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(model.MediaFiles), args.Error(1)
|
|
}
|
|
|
|
// GetRandom implements model.MediaFileRepository.
|
|
func (m *mockMediaFileRepo) GetRandom(options ...model.QueryOptions) (model.MediaFiles, error) {
|
|
argsSlice := make([]any, len(options))
|
|
for i, v := range options {
|
|
argsSlice[i] = v
|
|
}
|
|
args := m.Called(argsSlice...)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(model.MediaFiles), args.Error(1)
|
|
}
|
|
|
|
// SetError is a helper to set up a generic error for GetAll.
|
|
func (m *mockMediaFileRepo) SetError(hasError bool) {
|
|
if hasError {
|
|
m.On("GetAll", mock.Anything).Return(nil, errors.New("mock repo error"))
|
|
}
|
|
}
|
|
|
|
// FindByMBID is a helper to set up a GetAll expectation for finding by MBID.
|
|
func (m *mockMediaFileRepo) FindByMBID(mbid string, mediaFile model.MediaFile) {
|
|
m.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
|
|
return opt.Filters != nil
|
|
})).Return(model.MediaFiles{mediaFile}, nil).Once()
|
|
}
|
|
|
|
// FindByArtistAndTitle is a helper to set up a GetAll expectation for finding by artist/title.
|
|
func (m *mockMediaFileRepo) FindByArtistAndTitle(artistID string, title string, mediaFile model.MediaFile) {
|
|
m.On("GetAll", mock.MatchedBy(func(opt model.QueryOptions) bool {
|
|
return opt.Filters != nil
|
|
})).Return(model.MediaFiles{mediaFile}, nil).Once()
|
|
}
|
|
|
|
// mockAlbumRepo mocks model.AlbumRepository
|
|
type mockAlbumRepo struct {
|
|
mock.Mock
|
|
model.AlbumRepository
|
|
}
|
|
|
|
func newMockAlbumRepo() *mockAlbumRepo {
|
|
return &mockAlbumRepo{}
|
|
}
|
|
|
|
// Get implements model.AlbumRepository.
|
|
func (m *mockAlbumRepo) Get(id string) (*model.Album, error) {
|
|
args := m.Called(id)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(*model.Album), args.Error(1)
|
|
}
|
|
|
|
// GetAll implements model.AlbumRepository.
|
|
func (m *mockAlbumRepo) GetAll(options ...model.QueryOptions) (model.Albums, error) {
|
|
argsSlice := make([]any, len(options))
|
|
for i, v := range options {
|
|
argsSlice[i] = v
|
|
}
|
|
args := m.Called(argsSlice...)
|
|
if args.Get(0) == nil {
|
|
return nil, args.Error(1)
|
|
}
|
|
return args.Get(0).(model.Albums), args.Error(1)
|
|
}
|
|
|
|
// mockSimilarArtistAgent mocks agents implementing ArtistTopSongsRetriever and ArtistSimilarRetriever
|
|
type mockSimilarArtistAgent struct {
|
|
mock.Mock
|
|
agents.Interface // Embed to satisfy methods not explicitly mocked
|
|
}
|
|
|
|
func (m *mockSimilarArtistAgent) AgentName() string {
|
|
return "mockSimilar"
|
|
}
|
|
|
|
func (m *mockSimilarArtistAgent) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) {
|
|
args := m.Called(ctx, id, artistName, mbid, count)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Song), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockSimilarArtistAgent) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) {
|
|
args := m.Called(ctx, id, name, mbid, limit)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Artist), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
// mockAgents mocks the main Agents interface used by Provider
|
|
type mockAgents struct {
|
|
mock.Mock // Embed testify mock
|
|
topSongsAgent agents.ArtistTopSongsRetriever
|
|
similarAgent agents.ArtistSimilarRetriever
|
|
imageAgent agents.ArtistImageRetriever
|
|
albumInfoAgent interface {
|
|
agents.AlbumInfoRetriever
|
|
agents.AlbumImageRetriever
|
|
}
|
|
bioAgent agents.ArtistBiographyRetriever
|
|
mbidAgent agents.ArtistMBIDRetriever
|
|
urlAgent agents.ArtistURLRetriever
|
|
agents.Interface
|
|
}
|
|
|
|
func (m *mockAgents) AgentName() string {
|
|
return "mockCombined"
|
|
}
|
|
|
|
func (m *mockAgents) GetSimilarArtists(ctx context.Context, id, name, mbid string, limit int) ([]agents.Artist, error) {
|
|
if m.similarAgent != nil {
|
|
return m.similarAgent.GetSimilarArtists(ctx, id, name, mbid, limit)
|
|
}
|
|
args := m.Called(ctx, id, name, mbid, limit)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Artist), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetArtistTopSongs(ctx context.Context, id, artistName, mbid string, count int) ([]agents.Song, error) {
|
|
if m.topSongsAgent != nil {
|
|
return m.topSongsAgent.GetArtistTopSongs(ctx, id, artistName, mbid, count)
|
|
}
|
|
args := m.Called(ctx, id, artistName, mbid, count)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Song), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetAlbumInfo(ctx context.Context, name, artist, mbid string) (*agents.AlbumInfo, error) {
|
|
if m.albumInfoAgent != nil {
|
|
return m.albumInfoAgent.GetAlbumInfo(ctx, name, artist, mbid)
|
|
}
|
|
args := m.Called(ctx, name, artist, mbid)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).(*agents.AlbumInfo), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetArtistMBID(ctx context.Context, id string, name string) (string, error) {
|
|
if m.mbidAgent != nil {
|
|
return m.mbidAgent.GetArtistMBID(ctx, id, name)
|
|
}
|
|
args := m.Called(ctx, id, name)
|
|
return args.String(0), args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetArtistURL(ctx context.Context, id, name, mbid string) (string, error) {
|
|
if m.urlAgent != nil {
|
|
return m.urlAgent.GetArtistURL(ctx, id, name, mbid)
|
|
}
|
|
args := m.Called(ctx, id, name, mbid)
|
|
return args.String(0), args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetArtistBiography(ctx context.Context, id, name, mbid string) (string, error) {
|
|
if m.bioAgent != nil {
|
|
return m.bioAgent.GetArtistBiography(ctx, id, name, mbid)
|
|
}
|
|
args := m.Called(ctx, id, name, mbid)
|
|
return args.String(0), args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetArtistImages(ctx context.Context, id, name, mbid string) ([]agents.ExternalImage, error) {
|
|
if m.imageAgent != nil {
|
|
return m.imageAgent.GetArtistImages(ctx, id, name, mbid)
|
|
}
|
|
args := m.Called(ctx, id, name, mbid)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.ExternalImage), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetAlbumImages(ctx context.Context, name, artist, mbid string) ([]agents.ExternalImage, error) {
|
|
if m.albumInfoAgent != nil {
|
|
return m.albumInfoAgent.GetAlbumImages(ctx, name, artist, mbid)
|
|
}
|
|
args := m.Called(ctx, name, artist, mbid)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.ExternalImage), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetSimilarSongsByTrack(ctx context.Context, id, name, artist, mbid string, count int) ([]agents.Song, error) {
|
|
args := m.Called(ctx, id, name, artist, mbid, count)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Song), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetSimilarSongsByAlbum(ctx context.Context, id, name, artist, mbid string, count int) ([]agents.Song, error) {
|
|
args := m.Called(ctx, id, name, artist, mbid, count)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Song), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func (m *mockAgents) GetSimilarSongsByArtist(ctx context.Context, id, name, mbid string, count int) ([]agents.Song, error) {
|
|
args := m.Called(ctx, id, name, mbid, count)
|
|
if args.Get(0) != nil {
|
|
return args.Get(0).([]agents.Song), args.Error(1)
|
|
}
|
|
return nil, args.Error(1)
|
|
}
|
|
|
|
func ids(mfs model.MediaFiles) []string {
|
|
return slice.Map(mfs, func(mf model.MediaFile) string { return mf.ID })
|
|
}
|