diff --git a/core/artwork/e2e/helpers_test.go b/core/artwork/e2e/helpers_test.go index e3abca097..ddc926cec 100644 --- a/core/artwork/e2e/helpers_test.go +++ b/core/artwork/e2e/helpers_test.go @@ -171,7 +171,7 @@ func (n *noopProvider) UpdateArtistInfo(context.Context, string, int, bool) (*mo func (n *noopProvider) SimilarSongs(context.Context, string, int) (model.MediaFiles, error) { return nil, nil } -func (n *noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, error) { +func (n *noopProvider) TopSongs(context.Context, string, string, int) (model.MediaFiles, error) { return nil, nil } func (n *noopProvider) ArtistImage(context.Context, string) (*url.URL, error) { diff --git a/core/external/provider.go b/core/external/provider.go index 459e8a205..869841e06 100644 --- a/core/external/provider.go +++ b/core/external/provider.go @@ -34,7 +34,7 @@ type Provider interface { UpdateAlbumInfo(ctx context.Context, id string) (*model.Album, error) UpdateArtistInfo(ctx context.Context, id string, count int, includeNotPresent bool) (*model.Artist, error) SimilarSongs(ctx context.Context, id string, count int) (model.MediaFiles, error) - TopSongs(ctx context.Context, artist string, count int) (model.MediaFiles, error) + TopSongs(ctx context.Context, artist, artistId string, count int) (model.MediaFiles, error) ArtistImage(ctx context.Context, id string) (*url.URL, error) AlbumImage(ctx context.Context, id string) (*url.URL, error) } @@ -440,11 +440,16 @@ func (e *provider) AlbumImage(ctx context.Context, id string) (*url.URL, error) return url.Parse(img.URL) } -func (e *provider) TopSongs(ctx context.Context, artistName string, count int) (model.MediaFiles, error) { - artist, err := e.findArtistByName(ctx, artistName) +func (e *provider) TopSongs(ctx context.Context, artistName, id string, count int) (model.MediaFiles, error) { + artist, err := e.findArtist(ctx, artistName, id) if err != nil { - log.Error(ctx, "Artist not found", "name", artistName, err) - return nil, nil + if errors.Is(err, model.ErrNotFound) { + log.Error(ctx, "Artist not found", "name", artistName, "id", id, err) + return nil, nil + } + + log.Error(ctx, "Failure occurred when trying to fetch artist", "name", artistName, "id", id, err) + return nil, err } songs, err := e.getMatchingTopSongs(ctx, e.ag, artist, count) @@ -713,7 +718,24 @@ func (e *provider) loadArtistsByName(ctx context.Context, similar []agents.Artis return matches, nil } -func (e *provider) findArtistByName(ctx context.Context, artistName string) (*auxArtist, error) { +func (e *provider) findArtist(ctx context.Context, artistName, id string) (*auxArtist, error) { + if id != "" { + artist, err := e.ds.Artist(ctx).Get(id) + if err == nil { + return &auxArtist{Artist: *artist}, nil + } + + if errors.Is(err, model.ErrNotFound) { + log.Warn(ctx, "Could not find artist by id", "id", id, err) + } else { + return nil, err + } + } + + if artistName == "" { + return nil, model.ErrNotFound + } + artists, err := e.ds.Artist(ctx).GetAll(model.QueryOptions{ Filters: squirrel.Like{"artist.name": artistName}, Max: 1, diff --git a/core/external/provider_topsongs_test.go b/core/external/provider_topsongs_test.go index 86f9110e2..795a7ff74 100644 --- a/core/external/provider_topsongs_test.go +++ b/core/external/provider_topsongs_test.go @@ -65,7 +65,7 @@ var _ = Describe("Provider - TopSongs", func() { 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", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -76,6 +76,59 @@ var _ = Describe("Provider - TopSongs", func() { mediaFileRepo.AssertExpectations(GinkgoT()) }) + Context("artist id and/or name", func() { + artist1 := model.Artist{ID: "artist-1", Name: "Artist One", MbzArtistID: "mbid-artist-1"} + + BeforeEach(func() { + agentSongs := []agents.Song{{Name: "Song One", MBID: "mbid-song-1"}} + ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 2).Return(agentSongs, nil).Once() + + 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() + }) + + It("returns top songs for a known artist by id only", func() { + artistRepo.On("Get", "artist-1").Return(&artist1, nil).Once() + + songs, err := p.TopSongs(ctx, "", "artist-1", 2) + + 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()) + }) + + It("returns top songs for a known artist by id, falling back to name", func() { + artistRepo.On("Get", "fake-id").Return(nil, model.ErrNotFound).Once() + artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{artist1}, nil).Once() + + songs, err := p.TopSongs(ctx, "Artist One", "fake-id", 2) + + 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()) + }) + }) + + It("bails if lookup by id returns a fatal error", func() { + artistRepo.On("Get", "artist-1").Return(nil, model.ErrInvalidAuth).Once() + songs, err := p.TopSongs(ctx, "Artist One", "artist-1", 2) + Expect(songs).To(BeEmpty()) + Expect(err).To(HaveOccurred()) + }) + + It("bails if lookup by name returns a fatal error", func() { + artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(nil, model.ErrInvalidAuth).Once() + songs, err := p.TopSongs(ctx, "Artist One", "", 2) + Expect(songs).To(BeEmpty()) + Expect(err).To(HaveOccurred()) + }) + It("backfills name and MBID onto an unnamed primary credit (the queried artist) and matches", func() { 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) @@ -96,7 +149,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{track}, nil) - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -116,7 +169,8 @@ var _ = Describe("Provider - TopSongs", func() { ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 1).Return(agentSongs, nil).Once() // Library track is credited to Artist One (the queried artist) under a same title. If the - // queried MBID were wrongly stamped onto the "Artist Two" credit, that mismatched name+MBID + // queried MBID were wrongly stamped onto the "Artist Two" credit, that mismatched name+MBID artistRepo.On("Get", "fake-id").Return(nil, model.ErrNotFound).Once() + // could mis-resolve. With the guard, "Artist Two" stays MBID-less and does not match One's track. track := model.MediaFile{ ID: "one-track", Title: "Collab Song", ArtistID: "artist-1", @@ -126,7 +180,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{track}, nil) - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) // "Artist Two" (named, MBID-less, not in the library) does not resolve to One's track. @@ -137,7 +191,7 @@ var _ = Describe("Provider - TopSongs", func() { // Mock artist not found artistRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.Artists{}, nil).Once() - songs, err := p.TopSongs(ctx, "Unknown Artist", 5) + songs, err := p.TopSongs(ctx, "Unknown Artist", "", 5) Expect(err).ToNot(HaveOccurred()) // TopSongs returns nil error if artist not found Expect(songs).To(BeNil()) @@ -154,7 +208,7 @@ var _ = Describe("Provider - TopSongs", func() { agentErr := errors.New("agent error") ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 5).Return(nil, agentErr).Once() - songs, err := p.TopSongs(ctx, "Artist One", 5) + songs, err := p.TopSongs(ctx, "Artist One", "", 5) Expect(err).To(MatchError(agentErr)) Expect(songs).To(BeNil()) @@ -170,7 +224,7 @@ var _ = Describe("Provider - TopSongs", func() { // Mock agent ErrNotFound ag.On("GetArtistTopSongs", ctx, "artist-1", "Artist One", "mbid-artist-1", 5).Return(nil, agents.ErrNotFound).Once() - songs, err := p.TopSongs(ctx, "Artist One", 5) + songs, err := p.TopSongs(ctx, "Artist One", "", 5) Expect(err).To(MatchError(model.ErrNotFound)) Expect(songs).To(BeNil()) @@ -191,7 +245,7 @@ var _ = Describe("Provider - TopSongs", func() { 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() - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -220,7 +274,7 @@ var _ = Describe("Provider - TopSongs", func() { 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 track-fetch for song2: no match - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -242,7 +296,7 @@ var _ = Describe("Provider - TopSongs", func() { ag.On("GetArtistTopSongs", canceledCtx, "artist-1", "Artist One", "mbid-artist-1", 5).Return(nil, context.Canceled).Once() cancel() // Cancel the context before calling - songs, err := p.TopSongs(canceledCtx, "Artist One", 5) + songs, err := p.TopSongs(canceledCtx, "Artist One", "", 5) Expect(err).To(MatchError(context.Canceled)) Expect(songs).To(BeNil()) @@ -276,7 +330,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -313,7 +367,7 @@ var _ = Describe("Provider - TopSongs", func() { } mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -341,7 +395,7 @@ var _ = Describe("Provider - TopSongs", func() { 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) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) @@ -369,7 +423,7 @@ var _ = Describe("Provider - TopSongs", func() { song2 := model.MediaFile{ID: "song-2", Title: "Song Two", ArtistID: "artist-1"} mediaFileRepo.On("GetAll", mock.AnythingOfType("model.QueryOptions")).Return(model.MediaFiles{song1, song2}, nil).Once() - songs, err := p.TopSongs(ctx, "Artist One", 2) + songs, err := p.TopSongs(ctx, "Artist One", "", 2) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(2)) @@ -397,7 +451,7 @@ var _ = Describe("Provider - TopSongs", func() { 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() - songs, err := p.TopSongs(ctx, "Artist One", 1) + songs, err := p.TopSongs(ctx, "Artist One", "", 1) Expect(err).ToNot(HaveOccurred()) Expect(songs).To(HaveLen(1)) diff --git a/server/subsonic/browsing.go b/server/subsonic/browsing.go index f6a7047c4..59d267321 100644 --- a/server/subsonic/browsing.go +++ b/server/subsonic/browsing.go @@ -381,13 +381,14 @@ func (api *Router) GetSimilarSongs2(r *http.Request) (*responses.Subsonic, error func (api *Router) GetTopSongs(r *http.Request) (*responses.Subsonic, error) { ctx := r.Context() p := req.Params(r) + id, idErr := p.String("id") artist, err := p.String("artist") - if err != nil { + if err != nil && idErr != nil { return nil, err } count := p.IntOr("count", 50) - songs, err := api.provider.TopSongs(ctx, artist, count) + songs, err := api.provider.TopSongs(ctx, artist, id, count) if err != nil && !errors.Is(err, model.ErrNotFound) { return nil, err } diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 4e9039d68..97d7fb40a 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -353,7 +353,7 @@ func (n noopProvider) SimilarSongs(context.Context, string, int) (model.MediaFil return nil, nil } -func (n noopProvider) TopSongs(context.Context, string, int) (model.MediaFiles, error) { +func (n noopProvider) TopSongs(context.Context, string, string, int) (model.MediaFiles, error) { return nil, nil } diff --git a/server/subsonic/e2e/subsonic_browsing_test.go b/server/subsonic/e2e/subsonic_browsing_test.go index 55aeb8e9e..992f9e0fb 100644 --- a/server/subsonic/e2e/subsonic_browsing_test.go +++ b/server/subsonic/e2e/subsonic_browsing_test.go @@ -13,6 +13,15 @@ var _ = Describe("Browsing Endpoints", func() { setupTestDB() }) + getBeatlesId := func() string { + artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ + Filters: squirrel.Eq{"name": "The Beatles"}, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(artists).ToNot(BeEmpty()) + return artists[0].ID + } + Describe("getMusicFolders", func() { It("returns the configured music library", func() { resp := doReq("getMusicFolders") @@ -85,12 +94,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getMusicDirectory", func() { It("returns an artist directory with its albums as children", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID + beatlesID := getBeatlesId() resp := doReq("getMusicDirectory", "id", beatlesID) @@ -125,12 +129,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getArtist", func() { It("returns artist with albums in ID3 format", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID + beatlesID := getBeatlesId() resp := doReq("getArtist", "id", beatlesID) @@ -141,12 +140,7 @@ var _ = Describe("Browsing Endpoints", func() { }) It("returns album names for the artist", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID + beatlesID := getBeatlesId() resp := doReq("getArtist", "id", beatlesID) @@ -381,13 +375,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getArtistInfo", func() { It("returns artist info for a valid artist", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID - + beatlesID := getBeatlesId() resp := doReq("getArtistInfo", "id", beatlesID) Expect(resp.Status).To(Equal(responses.StatusOK)) @@ -397,13 +385,7 @@ var _ = Describe("Browsing Endpoints", func() { Describe("getArtistInfo2", func() { It("returns artist info2 for a valid artist", func() { - artists, err := ds.Artist(ctx).GetAll(model.QueryOptions{ - Filters: squirrel.Eq{"name": "The Beatles"}, - }) - Expect(err).ToNot(HaveOccurred()) - Expect(artists).ToNot(BeEmpty()) - beatlesID := artists[0].ID - + beatlesID := getBeatlesId() resp := doReq("getArtistInfo2", "id", beatlesID) Expect(resp.Status).To(Equal(responses.StatusOK)) @@ -426,6 +408,28 @@ var _ = Describe("Browsing Endpoints", func() { Expect(resp.TopSongs).ToNot(BeNil()) Expect(resp.TopSongs.Song).To(BeEmpty()) }) + + It("returns an error if no arguments are specified", func() { + resp := doReq("getTopSongs") + Expect(resp.Status).To(Equal(responses.StatusFailed)) + Expect(resp.TopSongs).To(BeNil()) + }) + + It("returns a response only by id", func() { + beatlesID := getBeatlesId() + resp := doReq("getTopSongs", "id", beatlesID) + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TopSongs).ToNot(BeNil()) + }) + + It("returns a response when both id and name are included", func() { + beatlesID := getBeatlesId() + resp := doReq("getTopSongs", "id", beatlesID, "artist", "The Beatles") + + Expect(resp.Status).To(Equal(responses.StatusOK)) + Expect(resp.TopSongs).ToNot(BeNil()) + }) }) Describe("getSimilarSongs", func() { diff --git a/server/subsonic/opensubsonic.go b/server/subsonic/opensubsonic.go index 97b3cafcc..2b2a31bf3 100644 --- a/server/subsonic/opensubsonic.go +++ b/server/subsonic/opensubsonic.go @@ -15,6 +15,7 @@ func (api *Router) GetOpenSubsonicExtensions(_ *http.Request) (*responses.Subson {Name: "indexBasedQueue", Versions: []int32{1}}, {Name: "transcoding", Versions: []int32{1}}, {Name: "playbackReport", Versions: []int32{1}}, + {Name: "topSongsByArtistId", Versions: []int32{1}}, } if api.sonic != nil && api.sonic.HasProvider() { extensions = append(extensions, responses.OpenSubsonicExtension{ diff --git a/server/subsonic/opensubsonic_test.go b/server/subsonic/opensubsonic_test.go index e4217303f..2615a652d 100644 --- a/server/subsonic/opensubsonic_test.go +++ b/server/subsonic/opensubsonic_test.go @@ -55,13 +55,14 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { err := json.Unmarshal(w.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( - HaveLen(6), + HaveLen(7), ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), ContainElement(responses.OpenSubsonicExtension{Name: "indexBasedQueue", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), )) Expect(*response.Subsonic.OpenSubsonicExtensions).NotTo( ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), @@ -85,7 +86,7 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { err := json.Unmarshal(w.Body.Bytes(), &response) Expect(err).NotTo(HaveOccurred()) Expect(*response.Subsonic.OpenSubsonicExtensions).To(SatisfyAll( - HaveLen(7), + HaveLen(8), ContainElement(responses.OpenSubsonicExtension{Name: "transcodeOffset", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "formPost", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "songLyrics", Versions: []int32{1, 2}}), @@ -93,6 +94,7 @@ var _ = Describe("GetOpenSubsonicExtensions", func() { ContainElement(responses.OpenSubsonicExtension{Name: "transcoding", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "playbackReport", Versions: []int32{1}}), ContainElement(responses.OpenSubsonicExtension{Name: "sonicSimilarity", Versions: []int32{1}}), + ContainElement(responses.OpenSubsonicExtension{Name: "topSongsByArtistId", Versions: []int32{1}}), )) }) })