mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-30 15:07:56 +01:00
fix(jellyfin): filter the Artists page by role (album artist vs performer)
/Artists and /Artists/AlbumArtists both called the same role-agnostic handler, so Navidrome's per-role artist entries (composers, arrangers, performers) all showed as Album Artists, and the two tabs were identical. Filter /Artists/AlbumArtists to RoleAlbumArtist and /Artists to RoleArtist (and the MusicArtist browse to album artists), via filter.ArtistsByRole. Verified live: Beatles Singles' album-artists dropped 138->5, composers excluded, performers (Billy Preston) show only under /Artists.
This commit is contained in:
parent
58cc14b0fa
commit
0fb520f3d3
6 changed files with 67 additions and 15 deletions
|
|
@ -150,6 +150,23 @@ func ApplyArtistLibraryFilter(opts Options, musicFolderIds []int) Options {
|
|||
return opts
|
||||
}
|
||||
|
||||
// ArtistsByRole restricts an artist query to artists appearing in the given role (album artist,
|
||||
// performer, composer, ...), mirroring the artist repository's own role filter over
|
||||
// library_artist.stats — which selectArtist/CountAll always join. The role name comes from a
|
||||
// model.Role constant so it's trusted; an unknown role is ignored (no filter) as a guard.
|
||||
func ArtistsByRole(opts Options, role model.Role) Options {
|
||||
if _, ok := model.AllRoles[role.String()]; !ok {
|
||||
return opts
|
||||
}
|
||||
roleFilter := Expr("JSON_EXTRACT(library_artist.stats, '$." + role.String() + ".m') IS NOT NULL")
|
||||
if opts.Filters == nil {
|
||||
opts.Filters = roleFilter
|
||||
} else {
|
||||
opts.Filters = And{opts.Filters, roleFilter}
|
||||
}
|
||||
return opts
|
||||
}
|
||||
|
||||
func ByGenre(genre string) Options {
|
||||
return addDefaultFilters(Options{
|
||||
Sort: "name",
|
||||
|
|
|
|||
|
|
@ -76,7 +76,7 @@ func (api *Router) routes() http.Handler {
|
|||
r.Delete("/Users/{userId}/Items/{itemId}/Rating", api.removeRating)
|
||||
|
||||
r.Get("/Artists", api.getArtists)
|
||||
r.Get("/Artists/AlbumArtists", api.getArtists)
|
||||
r.Get("/Artists/AlbumArtists", api.getAlbumArtists)
|
||||
r.Get("/Genres", api.getGenres)
|
||||
r.Get("/MusicGenres", api.getGenres)
|
||||
|
||||
|
|
|
|||
|
|
@ -8,10 +8,21 @@ import (
|
|||
"github.com/navidrome/navidrome/utils/req"
|
||||
)
|
||||
|
||||
// getArtists handles /Artists and /Artists/AlbumArtists. It defaults to the user's
|
||||
// accessible libraries but narrows to a single one when ParentId names a library the
|
||||
// user can access, matching how queryItems treats ParentId as a UserView id.
|
||||
// getArtists handles GET /Artists — performing artists (Finamp's "Artists" tab). getAlbumArtists
|
||||
// handles GET /Artists/AlbumArtists — album artists only ("Album Artists" tab). They're distinct
|
||||
// roles: without this, both listed every participant (composers, arrangers, ...) identically.
|
||||
func (api *Router) getArtists(w http.ResponseWriter, r *http.Request) {
|
||||
api.listArtistsByRole(w, r, model.RoleArtist)
|
||||
}
|
||||
|
||||
func (api *Router) getAlbumArtists(w http.ResponseWriter, r *http.Request) {
|
||||
api.listArtistsByRole(w, r, model.RoleAlbumArtist)
|
||||
}
|
||||
|
||||
// listArtistsByRole is the shared body of the /Artists* handlers. It defaults to the user's
|
||||
// accessible libraries but narrows to a single one when ParentId names a library the user can
|
||||
// access, matching how queryItems treats ParentId as a UserView id.
|
||||
func (api *Router) listArtistsByRole(w http.ResponseWriter, r *http.Request, role model.Role) {
|
||||
ctx := r.Context()
|
||||
p := req.Params(r)
|
||||
opts := model.QueryOptions{Offset: p.IntOr("StartIndex", 0), Max: p.IntOr("Limit", 0)}
|
||||
|
|
@ -19,7 +30,7 @@ func (api *Router) getArtists(w http.ResponseWriter, r *http.Request) {
|
|||
|
||||
scopeIDs, _ := resolveLibraryScope(ctx, dto.DecodeID(p.StringOr("ParentId", "")))
|
||||
|
||||
res, err := api.listArtists(ctx, opts, scopeIDs, p.StringOr("SearchTerm", ""), false)
|
||||
res, err := api.listArtists(ctx, opts, scopeIDs, p.StringOr("SearchTerm", ""), false, role)
|
||||
if err != nil {
|
||||
api.internalError(w, r, err)
|
||||
return
|
||||
|
|
|
|||
|
|
@ -39,7 +39,7 @@ var _ = Describe("Browsing", func() {
|
|||
|
||||
It("lists all songs with Audio type and an AlbumId", func() {
|
||||
q := queryResult(get("/Items?IncludeItemTypes=Audio&Recursive=true"))
|
||||
Expect(q.TotalRecordCount).To(Equal(6))
|
||||
Expect(q.TotalRecordCount).To(Equal(7))
|
||||
for _, it := range q.Items {
|
||||
Expect(it.Type).To(Equal("Audio"))
|
||||
Expect(it.MediaType).To(Equal("Audio"))
|
||||
|
|
@ -148,7 +148,7 @@ var _ = Describe("Browsing", func() {
|
|||
|
||||
It("merges multiple types into one paginated result", func() {
|
||||
q := queryResult(get("/Items?IncludeItemTypes=MusicAlbum,Audio&Recursive=true"))
|
||||
Expect(q.TotalRecordCount).To(Equal(11)) // 5 albums + 6 songs
|
||||
Expect(q.TotalRecordCount).To(Equal(12)) // 5 albums + 7 songs
|
||||
})
|
||||
})
|
||||
|
||||
|
|
@ -207,9 +207,22 @@ var _ = Describe("Browsing", func() {
|
|||
})
|
||||
|
||||
Describe("GET /Artists and /Genres", func() {
|
||||
It("lists artists", func() {
|
||||
q := queryResult(get("/Artists"))
|
||||
Expect(q.TotalRecordCount).To(Equal(4))
|
||||
It("lists album artists only on /Artists/AlbumArtists (excludes performer-only artists)", func() {
|
||||
names := names(queryResult(get("/Artists/AlbumArtists")).Items)
|
||||
Expect(names).To(ConsistOf("The Beatles", "Led Zeppelin", "Miles Davis", "Solo Artist"))
|
||||
Expect(names).ToNot(ContainElement("Featured Guest"))
|
||||
})
|
||||
|
||||
It("lists performing artists on /Artists (includes a track's guest artist)", func() {
|
||||
names := names(queryResult(get("/Artists")).Items)
|
||||
Expect(names).To(ContainElement("Featured Guest"))
|
||||
Expect(names).To(ContainElement("Solo Artist"))
|
||||
})
|
||||
|
||||
It("returns different lists for album artists and performing artists", func() {
|
||||
aa := names(queryResult(get("/Artists/AlbumArtists")).Items)
|
||||
ar := names(queryResult(get("/Artists")).Items)
|
||||
Expect(aa).ToNot(Equal(ar))
|
||||
})
|
||||
|
||||
It("lists genres", func() {
|
||||
|
|
|
|||
|
|
@ -15,9 +15,10 @@
|
|||
// Rock/The Beatles/Help!/ 01 Help! (1965)
|
||||
// Rock/Led Zeppelin/IV/ 01 Stairway To Heaven (1971)
|
||||
// Jazz/Miles Davis/Kind of Blue/01 So What (1959)
|
||||
// Pop/Solo Artist/Singles/ 01 Standalone Track (2020)
|
||||
// Pop/Solo Artist/Singles/ 01 Standalone Track (2020), 02 Duet (artist "Featured Guest")
|
||||
//
|
||||
// Totals: 6 songs, 5 albums, 4 album artists, 3 genres (Rock=4, Jazz=1, Pop=1).
|
||||
// Totals: 7 songs, 5 albums, 4 album artists (+ 1 performer-only "Featured Guest" = 5 artists),
|
||||
// 3 genres (Rock=4, Jazz=1, Pop=2).
|
||||
package e2e
|
||||
|
||||
import (
|
||||
|
|
@ -128,6 +129,9 @@ func buildTestFS() storagetest.FakeFS {
|
|||
"Rock/Led Zeppelin/IV/01 - Stairway To Heaven.mp3": ledZepIV(track(1, "Stairway To Heaven")),
|
||||
"Jazz/Miles Davis/Kind of Blue/01 - So What.mp3": kindOfBlue(track(1, "So What")),
|
||||
"Pop/Solo Artist/Singles/01 - Standalone Track.mp3": singles(track(1, "Standalone Track")),
|
||||
// "Featured Guest" is the track artist here (album artist stays "Solo Artist"), so it's a
|
||||
// performer but not an album artist — lets tests tell /Artists from /Artists/AlbumArtists.
|
||||
"Pop/Solo Artist/Singles/02 - Duet.mp3": singles(track(2, "Duet", _t{"artist": "Featured Guest"})),
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -104,7 +104,8 @@ func (api *Router) queryItemsOfType(ctx context.Context, itemType string, opts m
|
|||
case "Audio":
|
||||
return api.listSongs(ctx, opts, entityParent, artistId, scopeIDs, search, favOnly)
|
||||
case "MusicArtist":
|
||||
return api.listArtists(ctx, opts, scopeIDs, search, favOnly)
|
||||
// The MusicArtist browse hierarchy (UserViews -> artists -> albums) means album artists.
|
||||
return api.listArtists(ctx, opts, scopeIDs, search, favOnly, model.RoleAlbumArtist)
|
||||
case "MusicGenre":
|
||||
return api.listGenres(ctx, opts)
|
||||
case "Playlist":
|
||||
|
|
@ -237,14 +238,19 @@ func (api *Router) listSongs(ctx context.Context, opts model.QueryOptions, paren
|
|||
return result(slice.Map(mfs, dto.SongToBaseItem), int(total), opts.Offset), nil
|
||||
}
|
||||
|
||||
func (api *Router) listArtists(ctx context.Context, opts model.QueryOptions, scopeIDs []int, search string, fav bool) (dto.QueryResult, error) {
|
||||
// listArtists lists artists in the given role — RoleAlbumArtist for the "album artists" views
|
||||
// (/Artists/AlbumArtists, the MusicArtist browse hierarchy) and RoleArtist for performing artists
|
||||
// (/Artists). Without a role filter every participant (composers, arrangers, ...) would show up in
|
||||
// both lists and they'd be identical.
|
||||
func (api *Router) listArtists(ctx context.Context, opts model.QueryOptions, scopeIDs []int, search string, fav bool, role model.Role) (dto.QueryResult, error) {
|
||||
repo := api.ds.Artist(ctx)
|
||||
|
||||
// Artist Search does its own library scoping: it consumes a sole Eq{"library_id": ...} filter
|
||||
// (artists have no library_id column, so it can't be a real WHERE clause) and realizes it as a
|
||||
// search scope. The join-based ApplyArtistLibraryFilter, or any compound filter, would leak
|
||||
// library_artist.library_id into the FTS query and 500. So the search and browse paths build
|
||||
// their filters differently. See persistence/artist_repository.go's searchScope.
|
||||
// their filters differently. See persistence/artist_repository.go's searchScope. Role isn't
|
||||
// applied to search: it's a name lookup, and a compound filter would break the same way.
|
||||
if search != "" {
|
||||
if len(scopeIDs) > 0 {
|
||||
opts.Filters = squirrel.Eq{"library_id": scopeIDs}
|
||||
|
|
@ -261,6 +267,7 @@ func (api *Router) listArtists(ctx context.Context, opts model.QueryOptions, sco
|
|||
} else {
|
||||
opts.Filters = notMissing
|
||||
}
|
||||
opts = filter.ArtistsByRole(opts, role)
|
||||
opts = filter.ApplyArtistLibraryFilter(opts, scopeIDs)
|
||||
artists, err := repo.GetAll(opts)
|
||||
if err != nil {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue