remote/navidrome
1
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-10-08 02:17:25 +02:00
Code Issues Projects Releases Packages Wiki Activity

fix(artwork): do not promote artist folder to album folder (#5856)

Browse source 
* fix(artwork): do not promote artist folder to album folder

* use filesystem join for path instead

* fix(artwork): resolve artist folder for albums with disc subfolders

Dropping the promoted album root from the artist reader's paths fixed the
flat-album case but broke albums whose tracks live in disc subfolders: the
promoted root was what kept the byte-wise longest common prefix on a
directory boundary. Without it, CD1/CD2 siblings share the fragment
"Album/CD", so the artist folder resolved one level too deep and
album/artist.* won over artist.*.

loadAlbumFoldersPaths now collapses each album to its own root before the
paths are compared across albums, so a disc-split album contributes its
album folder rather than each disc folder. Folders no album claims (the
promoted parent) are still returned unchanged, keeping the album, disc and
mediafile readers unaffected. The prefix math moves into commonDir, which
appends a trailing separator so the comparison lands on segment boundaries
- this also fixes sibling folders sharing a name prefix (Album/Album2).

Adds e2e coverage for the case the branch fixes (artist.* with no album/
fallback, which failed before this branch), the disc-subfolder regression,
and a multi-album guard that pins the scope boundary.

* test(artwork): cover artist whose albums are all disc-split

The existing specs cover a single disc-split album and a disc-split album
alongside a flat one, but not an artist where every album is split into
disc subfolders. That layout resolves correctly because the albums diverge
one level above the disc folders, which re-anchors the common prefix - a
property worth pinning so a future change to the path math can't silently
break it.

* fix(artwork): resolve artist folder when albums share a folder

loadArtistFolder decided whether to climb above the common directory by
comparing len(paths) against 2. That count stands in for "how many distinct
album roots are there", and the two diverge when an artist has two albums in
the same folder: the slice holds two identical entries, the climb is skipped,
and the artist folder resolves to the album folder. Deduplicate the roots so
the branch tests the property it means to test.

Replace the includeParent flag with two functions split by audience. The flag
was a caller-identity switch - constant true at the album, disc and mediafile
readers, all of which discard the returned paths, and constant false at the
only caller that reads them. The artist path now has loadArtistAlbumRoots,
which collapses each album to its own root and never consults albumRootParent;
loadAlbumFoldersPaths goes back to taking a single album and always promoting
the parent. Both share the folder load and image aggregation.

This also makes albumRootParent's contract structural. Its guard only rejects
an artist folder when it can find audio outside the album being resolved, so
passing every album of an artist at once made the check meaningless; taking a
single album means it can no longer be called that way.

Drops the per-album grouping and unclaimed-folder reconciliation from the
album path, where the result was discarded, and removes 15 mechanical true
arguments from the tests.

---------

Co-authored-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Kendall Garner 2026-07-30 01:48:42 +00:00 • committed by GitHub
commit 4f29465d27
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 206 additions and 20 deletions
Download patch file Download diff file Expand all files Collapse all files

1
core/artwork/artwork_internal_test.go
View file

@ -202,6 +202,7 @@ var _ = Describe("Artwork", func() {
repoRoot, err := os.Getwd()
Expect(err).ToNot(HaveOccurred())
folderRepo.result = []model.Folder{{
ID: "f1",
LibraryPath: testFileLibPath(repoRoot),
Path: "tests/fixtures/artist/an-album",
ImageFiles: []string{"artist.png"},

95
core/artwork/e2e/artist_test.go
View file

@ -80,6 +80,101 @@ var _ = Describe("Artist artwork resolution", func() {
})
})
When("ArtistArtPriority has no album/ fallback", func() {
// Artist/
// ├── artist.jpg ← must resolve via the artist folder itself
// └── Album/
// └── 01 - Track.mp3
It("still resolves the artist folder and returns artist.*", func() {
conf.Server.ArtistArtPriority = "artist.*"
setLayout(fstest.MapFS{
"Artist/Album/01 - Track.mp3": trackFile(1, "Track", map[string]any{"albumartist": "Artist"}),
"Artist/artist.jpg": imageFile("artist-folder"),
})
scan()
ar := soleArtist()
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
})
})
When("the artist's only album has its tracks in disc subfolders", func() {
// Artist/
// ├── artist.jpg ← wins (artist.* before album/artist.*)
// └── Album/
// ├── artist.jpg
// ├── CD1/01 - Track.mp3
// └── CD2/02 - Track.mp3
It("prefers the artist-folder image over the album-folder one", func() {
conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external"
setLayout(fstest.MapFS{
"Artist/Album/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album"}),
"Artist/Album/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album"}),
"Artist/artist.jpg": imageFile("artist-folder"),
"Artist/Album/artist.jpg": imageFile("album-artist"),
})
scan()
ar := soleArtist()
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
})
})
When("one album has disc subfolders and another sits at artist level", func() {
// Artist/
// ├── artist.jpg ← wins
// ├── Album1/
// │ ├── artist.jpg
// │ ├── CD1/01 - Track.mp3
// │ └── CD2/02 - Track.mp3
// └── Album2/03 - Track.mp3
It("prefers the artist-folder image over the album-folder one", func() {
conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external"
setLayout(fstest.MapFS{
"Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}),
"Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}),
"Artist/Album2/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}),
"Artist/artist.jpg": imageFile("artist-folder"),
"Artist/Album1/artist.jpg": imageFile("album-artist"),
})
scan()
ar := soleArtist()
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
})
})
When("every album of the artist has its tracks in disc subfolders", func() {
// Artist/
// ├── artist.jpg ← wins
// ├── Album1/
// │ ├── artist.jpg
// │ ├── CD1/01 - Track.mp3
// │ └── CD2/02 - Track.mp3
// └── Album2/
// ├── CD1/03 - Track.mp3
// └── CD2/04 - Track.mp3
It("prefers the artist-folder image over the album-folder one", func() {
conf.Server.ArtistArtPriority = "artist.*, album/artist.*, external"
setLayout(fstest.MapFS{
"Artist/Album1/CD1/01 - Track.mp3": trackFile(1, "Track 1", map[string]any{"albumartist": "Artist", "album": "Album1"}),
"Artist/Album1/CD2/02 - Track.mp3": trackFile(2, "Track 2", map[string]any{"albumartist": "Artist", "album": "Album1"}),
"Artist/Album2/CD1/03 - Track.mp3": trackFile(3, "Track 3", map[string]any{"albumartist": "Artist", "album": "Album2"}),
"Artist/Album2/CD2/04 - Track.mp3": trackFile(4, "Track 4", map[string]any{"albumartist": "Artist", "album": "Album2"}),
"Artist/artist.jpg": imageFile("artist-folder"),
"Artist/Album1/artist.jpg": imageFile("album-artist"),
})
scan()
ar := soleArtist()
artID := model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil)
Expect(readArtwork(artID)).To(Equal(imageBytes("artist-folder")))
})
})
When("an artist has an uploaded image and a matching artist.* file", func() {
// <DataFolder>/
// └── artwork/

32
core/artwork/reader_album.go
View file

@ -20,6 +20,7 @@ import (
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/utils"
"github.com/navidrome/navidrome/utils/natural"
"github.com/navidrome/navidrome/utils/slice"
)
type albumArtworkReader struct {
@ -103,17 +104,13 @@ func (a *albumArtworkReader) fromCoverArtPriority(ctx context.Context, ffmpeg ff
return ff
}
func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...model.Album) ([]string, []string, *time.Time, error) {
var folderIDs []string
for _, album := range albums {
folderIDs = append(folderIDs, album.FolderIDs...)
}
folders, err := ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}})
func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, album model.Album) ([]string, []string, *time.Time, error) {
folders, err := loadFolders(ctx, ds, album.FolderIDs)
if err != nil {
return nil, nil, nil, err
}
parent, err := albumRootParent(ctx, ds, folders, folderIDs)
parent, err := albumRootParent(ctx, ds, folders, album.FolderIDs)
if err != nil {
return nil, nil, nil, err
}
@ -121,11 +118,21 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
folders = append(folders, *parent)
}
var paths []string
paths := slice.Map(folders, func(f model.Folder) string { return f.AbsolutePath() })
imgFiles, updatedAt := folderImages(folders)
return paths, imgFiles, &updatedAt, nil
}
func loadFolders(ctx context.Context, ds model.DataStore, folderIDs []string) ([]model.Folder, error) {
return ds.Folder(ctx).GetAll(model.QueryOptions{Filters: squirrel.Eq{"folder.id": folderIDs, "missing": false}})
}
// folderImages collects the folders' image files, sorted so files without
// numeric suffixes win (e.g. cover.jpg over cover.1.jpg).
func folderImages(folders []model.Folder) ([]string, time.Time) {
var imgFiles []string
var updatedAt time.Time
for _, f := range folders {
paths = append(paths, f.AbsolutePath())
if f.ImagesUpdatedAt.After(updatedAt) {
updatedAt = f.ImagesUpdatedAt
}
@ -134,13 +141,8 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
imgFiles = append(imgFiles, path.Join(rel, img))
}
}
// Sort image files to ensure consistent selection of cover art
// This prioritizes files without numeric suffixes (e.g., cover.jpg over cover.1.jpg)
// by comparing base filenames without extensions
slices.SortFunc(imgFiles, compareImageFiles)
return paths, imgFiles, &updatedAt, nil
return imgFiles, updatedAt
}
// albumRootParent returns the common parent of the album's folders when it

27
core/artwork/reader_album_test.go
View file

@ -3,6 +3,7 @@ package artwork
import (
"context"
"errors"
"path/filepath"
"time"
"github.com/navidrome/navidrome/model"
@ -339,6 +340,32 @@ var _ = Describe("Album Artwork Reader", func() {
Expect(repo.getCallCount).To(Equal(1))
})
It("promotes the album root parent into the returned paths", func() {
repo.result = []model.Folder{
{
ID: "folder1",
Path: "Artist",
Name: "Album",
ParentID: "artistFolder",
ImagesUpdatedAt: now,
ImageFiles: []string{},
},
}
repo.parentResult = &model.Folder{
ID: "artistFolder",
Path: ".",
Name: "Artist",
ParentID: "libraryRoot",
ImagesUpdatedAt: expectedAt,
ImageFiles: []string{"folder.jpg"},
}
paths, _, _, err := loadAlbumFoldersPaths(ctx, ds, album)
Expect(err).ToNot(HaveOccurred())
Expect(paths).To(Equal([]string{filepath.Join("Artist", "Album"), "Artist"}))
})
It("does not include parent images when other albums' audio lives under the parent", func() {
// Simulates: Artist/folder.jpg with Artist/Album (no images) and
// another album's tracks elsewhere under the artist folder

58
core/artwork/reader_artist.go
View file

@ -19,6 +19,7 @@ import (
"github.com/navidrome/navidrome/core/external"
"github.com/navidrome/navidrome/log"
"github.com/navidrome/navidrome/model"
"github.com/navidrome/navidrome/utils/slice"
"github.com/navidrome/navidrome/utils/str"
)
@ -54,7 +55,7 @@ func newArtistArtworkReader(ctx context.Context, artwork *artwork, artID model.A
if err != nil {
return nil, err
}
albumPaths, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, artwork.ds, als...)
albumPaths, imgFiles, imagesUpdatedAt, err := loadArtistAlbumRoots(ctx, artwork.ds, als)
if err != nil {
return nil, err
}
@ -232,17 +233,64 @@ func escapeGlobLiteral(s string) string {
return b.String()
}
// loadArtistAlbumRoots returns one path per album — the deepest folder holding
// all of that album's tracks — so an album split into disc subfolders can't
// pull the artist folder's common prefix below the artist level.
func loadArtistAlbumRoots(ctx context.Context, ds model.DataStore, albums model.Albums) ([]string, []string, *time.Time, error) {
var folderIDs []string
for _, album := range albums {
folderIDs = append(folderIDs, album.FolderIDs...)
}
folders, err := loadFolders(ctx, ds, folderIDs)
if err != nil {
return nil, nil, nil, err
}
pathByID := slice.ToMap(folders, func(f model.Folder) (string, string) {
return f.ID, f.AbsolutePath()
})
var roots []string
for _, album := range albums {
var albumPaths []string
for _, fid := range album.FolderIDs {
if p, ok := pathByID[fid]; ok {
albumPaths = append(albumPaths, p)
}
}
if len(albumPaths) > 0 {
roots = append(roots, commonDir(albumPaths))
}
}
imgFiles, updatedAt := folderImages(folders)
return roots, imgFiles, &updatedAt, nil
}
// commonDir returns the deepest directory containing all paths. Trailing
// separators keep the comparison on segment boundaries, so a shared name
// fragment (".../Album" and ".../Album2") is never read as a shared directory.
func commonDir(paths []string) string {
sep := string(filepath.Separator)
common := str.LongestCommonPrefix(slice.Map(paths, func(p string) string { return p + sep }))
if !strings.HasSuffix(common, sep) {
common, _ = filepath.Split(common)
}
return filepath.Clean(common)
}
func loadArtistFolder(ctx context.Context, ds model.DataStore, albums model.Albums, paths []string) (string, time.Time, error) {
if len(albums) == 0 {
return "", time.Time{}, nil
}
libID := albums[0].LibraryID // Just need one of the albums, as they should all be in the same Library - for now! TODO: Support multiple libraries
folderPath := str.LongestCommonPrefix(paths)
if !strings.HasSuffix(folderPath, string(filepath.Separator)) {
folderPath, _ = filepath.Split(folderPath)
// paths holds one root per album: two or more distinct roots already meet at
// the artist folder, while a single root is an album folder needing a climb.
roots := slices.Compact(slices.Sorted(slices.Values(paths)))
folderPath := commonDir(roots)
if len(roots) < 2 {
folderPath = filepath.Dir(folderPath)
}
folderPath = filepath.Dir(folderPath)
// Manipulate the path to get the folder ID
// TODO: This is a bit hacky, but it's the easiest way to get the folder ID, ATM

13
core/artwork/reader_artist_test.go
View file

@ -85,6 +85,19 @@ var _ = Describe("artistArtworkReader", func() {
})
})
When("two albums share the same folder", func() {
It("climbs above the shared album folder", func() {
paths = []string{
filepath.FromSlash("/music/artist/split"),
filepath.FromSlash("/music/artist/split"),
}
folder, upd, err := loadArtistFolder(ctx, fds, albums, paths)
Expect(err).ToNot(HaveOccurred())
Expect(folder).To(Equal(filepath.FromSlash("/music/artist")))
Expect(upd).To(Equal(expectedUpdTime))
})
})
When("the album paths contain same prefix", func() {
It("returns the common prefix", func() {
paths = []string{