feat(subsonic): Implement OpenSubsonic topSongsByArtistId extension (#5853)

* implement topSongsByArtistId extension

* do not look up by artist name if empty

---------

Co-authored-by: Deluan Quintão <deluan@navidrome.org>
This commit is contained in:
Kendall Garner
2026-08-02 09:39:51 -07:00
committed by GitHub
parent 810b14ed57
commit b40b41584a
8 changed files with 143 additions and 59 deletions

View File

@@ -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) {

View File

@@ -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,

View File

@@ -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))

View File

@@ -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
}

View File

@@ -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
}

View File

@@ -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() {

View File

@@ -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{

View File

@@ -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}}),
))
})
})