diff --git a/core/external/provider.go b/core/external/provider.go index f27ded11..c23d1edd 100644 --- 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) { diff --git a/core/external/provider_similarsongs_test.go b/core/external/provider_similarsongs_test.go index fd622746..e7b3cee1 100644 --- 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) diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index 4ce7911d..443be36d 100644 --- 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()) + }) })