vrr/navidrome
2
0
Fork 0
mirror of https://github.com/navidrome/navidrome.git synced 2026-07-09 17:18:45 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 4

fix(artwork): never serve artist folder images as album art (#5596)

Browse source 
* test(artwork): add failing e2e tests for artist image leaking as album art

Reproduces a v0.62.0 regression (#5451/#5457): the album cover-art
parent-folder fallback can include the artist folder, serving the artist
thumbnail (e.g. Artist/folder.jpg) as album art for any album without
image files in its own folder(s). Covers three scenarios: a plain
Artist/Album layout with no album images, a single-disc album spread
across sibling folders under the artist folder, and a spread album whose
own front.jpg is shadowed by the artist's cover.jpg via CoverArtPriority
order. Also adds an albumByName test helper for multi-album layouts.

The tests are expected to fail until the parent-folder inclusion is
gated by a structural check (skip the common parent when audio from
other albums lives under it).

* fix(artwork): never serve artist folder images as album art

The album cover-art parent-folder fallback (introduced in #5451/#5457)
could include the artist folder as a source of album images, serving the
artist thumbnail (e.g. Artist/folder.jpg) as cover art for any album
without image files in its own folder(s). This affected both plain
Artist/Album layouts and single-disc albums spread across sibling
folders under the artist folder.

Gate the common-parent inclusion with a structural check: the parent
only qualifies as an album root when no audio belonging to other albums
lives in it or anywhere beneath it. An artist folder contains other
albums' tracks, while an album root above disc subfolders contains only
this album's, so the check works for any disc folder naming scheme and
never affects the multi-disc fixes from #5376/#5456. A single-album
artist with no images anywhere remains structurally indistinguishable
from an album root and is a known residual case.

* refactor(artwork): move album-root audio check into folder repository

Replace the raw subtree SQL (LIKE/ESCAPE expression and wildcard
escaping) that lived in core/artwork with an explicit
FolderRepository.HasAudioOutsideFolders method, implemented in the
persistence layer next to the existing folder-subtree query pattern.
This also removes the test mock's brittle dispatch that sniffed the
generated SQL to recognize the query; the fake now overrides the new
method directly.

Extract the whole parent-folder resolution from loadAlbumFoldersPaths
into an albumRootParent helper, flattening four levels of nesting back
into a linear flow. Behavior is unchanged; the unit test for a parent
containing audio moved to the persistence suite, with added coverage
for subtree boundaries, missing folders, and LIKE-wildcard escaping in
folder paths.

* refactor(persistence): use exists helper in HasAudioOutsideFolders

Replace the hand-rolled count(*) query with the repository's canonical
exists helper, as suggested in PR review.
This commit is contained in:
Deluan Quintão 2026-06-13 13:29:29 -04:00 • committed by GitHub
parent da56df3160
commit af78bdeb3a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
8 changed files with 311 additions and 21 deletions
Download patch file Download diff file Expand all files Collapse all files

92
core/artwork/e2e/album_test.go
View file

@ -357,6 +357,98 @@ var _ = Describe("Album artwork resolution", func() {
})
})
// Regression introduced in v0.62.0 (#5451 + #5457): the parent-folder
// fallback can pick up images from the ARTIST folder, serving the artist
// thumbnail as album art for any album without its own image files.
When("an album has no images and the artist folder has folder.jpg", func() {
// Artist/
// ├── folder.jpg ← artist thumbnail, must NOT become album art
// ├── Album A/
// │ └── 01 - Track.mp3 (no images)
// └── Album B/
// ├── 01 - Track.mp3
// └── cover.jpg
It("does not use the artist image as album art", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
"Artist/folder.jpg": imageFile("artist-thumbnail"),
"Artist/Album A/01 - Track.mp3": trackFile(1, "Track A", map[string]any{"album": "Album A", "albumartist": "Artist"}),
"Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}),
"Artist/Album B/cover.jpg": imageFile("album-b"),
})
scan()
alA := albumByName("Album A")
_, err := readArtworkOrErr(alA.CoverArtID())
Expect(err).To(HaveOccurred(),
"Album A has no images of its own, so it must fall through to the placeholder "+
"instead of inheriting the artist folder's folder.jpg")
alB := albumByName("Album B")
Expect(readArtwork(alB.CoverArtID())).To(Equal(imageBytes("album-b")))
})
})
When("a single-disc album is spread across sibling folders under the artist folder", func() {
// Artist/
// ├── folder.jpg ← artist thumbnail, must NOT become album art
// ├── Album A/
// │ └── 01 - Track.mp3 (album: "Album A")
// ├── Album A bonus/
// │ └── 02 - Track.mp3 (album: "Album A" — same album, second folder)
// └── Album B/
// ├── 01 - Track.mp3
// └── cover.jpg
It("does not use the artist image as album art for the spread album", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
"Artist/folder.jpg": imageFile("artist-thumbnail"),
"Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}),
"Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}),
"Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}),
"Artist/Album B/cover.jpg": imageFile("album-b"),
})
scan()
alA := albumByName("Album A")
Expect(alA.FolderIDs).To(HaveLen(2),
"sanity check: scanner should treat the two sibling folders as one spread album")
_, err := readArtworkOrErr(alA.CoverArtID())
Expect(err).To(HaveOccurred(),
"the spread album has no images of its own, so it must fall through to the "+
"placeholder instead of inheriting the artist folder's folder.jpg")
})
})
When("a spread album has its own front.jpg but the artist folder has cover.jpg", func() {
// Artist/
// ├── cover.jpg ← artist image; matches cover.* (first pattern),
// │ must NOT shadow the album's own front.jpg
// ├── Album A/
// │ ├── 01 - Track.mp3 (album: "Album A")
// │ └── front.jpg ← should win
// ├── Album A bonus/
// │ └── 02 - Track.mp3 (album: "Album A")
// └── Album B/
// └── 01 - Track.mp3
It("prefers the album's own art over the artist image", func() {
conf.Server.CoverArtPriority = defaultCoverPriority
setLayout(fstest.MapFS{
"Artist/cover.jpg": imageFile("artist-image"),
"Artist/Album A/01 - Track.mp3": trackFile(1, "Track A1", map[string]any{"album": "Album A", "albumartist": "Artist"}),
"Artist/Album A/front.jpg": imageFile("album-a-front"),
"Artist/Album A bonus/02 - Track.mp3": trackFile(2, "Track A2", map[string]any{"album": "Album A", "albumartist": "Artist"}),
"Artist/Album B/01 - Track.mp3": trackFile(1, "Track B", map[string]any{"album": "Album B", "albumartist": "Artist"}),
})
scan()
alA := albumByName("Album A")
Expect(alA.FolderIDs).To(HaveLen(2),
"sanity check: scanner should treat the two sibling folders as one spread album")
Expect(readArtwork(alA.CoverArtID())).To(Equal(imageBytes("album-a-front")))
})
})
When("embedded is first in CoverArtPriority but the track has no embedded art", func() {
// Artist/
// └── Album/

14
core/artwork/e2e/suite_test.go
View file

@ -2,6 +2,7 @@ package artworke2e_test
import (
"context"
"fmt"
"path/filepath"
"testing"
@ -104,3 +105,16 @@ func firstAlbum() model.Album {
Expect(albums).To(HaveLen(1), "expected exactly one album, got %d", len(albums))
return albums[0]
}
func albumByName(name string) model.Album {
GinkgoHelper()
albums, err := ds.Album(ctx).GetAll(model.QueryOptions{})
Expect(err).ToNot(HaveOccurred())
for _, al := range albums {
if al.Name == name {
return al
}
}
Fail(fmt.Sprintf("album %q not found among %d albums", name, len(albums)))
return model.Album{}
}

70
core/artwork/reader_album.go
View file

@ -113,28 +113,12 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
return nil, nil, nil, err
}
folderIDSet := make(map[string]bool, len(folderIDs))
for _, id := range folderIDs {
folderIDSet[id] = true
parent, err := albumRootParent(ctx, ds, folders, folderIDs)
if err != nil {
return nil, nil, nil, err
}
// Check if all folders share a common parent that is not already included.
// This finds cover art in the album root folder (e.g., "Artist/Album/cover.jpg"
// when tracks are in disc subfolders like "Artist/Album/CD1/" and "Artist/Album/CD2/").
// For single-folder albums, the parent is only included when the folder has no
// images of its own (indicating a disc subfolder needing parent artwork).
if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" {
if len(folders) >= 2 || !anyFolderHasImages(folders) {
parentFolder, err := ds.Folder(ctx).Get(commonParentID)
if errors.Is(err, model.ErrNotFound) {
log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID)
} else if err != nil {
return nil, nil, nil, err
}
if parentFolder != nil && parentFolder.ParentID != "" {
folders = append(folders, *parentFolder)
}
}
if parent != nil {
folders = append(folders, *parent)
}
var paths []string
@ -159,6 +143,50 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo
return paths, imgFiles, &updatedAt, nil
}
// albumRootParent returns the common parent of the album's folders when it
// qualifies as the album's root folder (e.g. "Artist/Album" above disc
// subfolders), or nil when there is no such parent. This finds cover art in
// the album root folder when tracks live in disc subfolders, like
// "Artist/Album/cover.jpg" with tracks in "Artist/Album/CD1/" and
// "Artist/Album/CD2/". The parent must look like an album root, not an
// artist-level folder — it qualifies only when it holds no audio belonging to
// other albums — so artist images are never served as album art.
func albumRootParent(ctx context.Context, ds model.DataStore, folders []model.Folder, folderIDs []string) (*model.Folder, error) {
folderIDSet := make(map[string]bool, len(folderIDs))
for _, id := range folderIDs {
folderIDSet[id] = true
}
commonParentID := commonParentFolder(folders, folderIDSet)
if commonParentID == "" {
return nil, nil
}
// Single-folder albums only use the parent when the folder has no images
// of its own (indicating a disc subfolder needing parent artwork).
if len(folders) < 2 && anyFolderHasImages(folders) {
return nil, nil
}
parent, err := ds.Folder(ctx).Get(commonParentID)
if errors.Is(err, model.ErrNotFound) {
log.Warn(ctx, "Parent folder not found for album cover art lookup", "parentID", commonParentID)
return nil, nil
}
if err != nil {
return nil, err
}
if parent.ParentID == "" {
// The library root can never be an album root
return nil, nil
}
hasOtherAudio, err := ds.Folder(ctx).HasAudioOutsideFolders(*parent, folderIDs)
if err != nil {
return nil, err
}
if hasOtherAudio {
return nil, nil
}
return parent, nil
}
func anyFolderHasImages(folders []model.Folder) bool {
for _, f := range folders {
if len(f.ImageFiles) > 0 {

55
core/artwork/reader_album_test.go
View file

@ -339,6 +339,61 @@ var _ = Describe("Album Artwork Reader", func() {
Expect(repo.getCallCount).To(Equal(1))
})
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
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"},
}
repo.hasOtherAudio = true
_, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album)
Expect(err).ToNot(HaveOccurred())
Expect(imgFiles).To(BeEmpty())
})
It("propagates errors from the album-root check", func() {
repo.result = []model.Folder{
{
ID: "folder1",
Path: "Artist/Album",
Name: "disc1",
ParentID: "albumFolder",
ImagesUpdatedAt: now,
ImageFiles: []string{},
},
}
repo.parentResult = &model.Folder{
ID: "albumFolder",
Path: "Artist",
Name: "Album",
ParentID: "artistFolder",
ImagesUpdatedAt: expectedAt,
ImageFiles: []string{"cover.jpg"},
}
repo.otherAudioErr = errors.New("db connection failed")
_, _, _, err := loadAlbumFoldersPaths(ctx, ds, album)
Expect(err).To(MatchError("db connection failed"))
})
It("propagates non-ErrNotFound errors from parent folder lookup", func() {
repo.result = []model.Folder{
{

8
core/artwork/reader_artist_test.go
View file

@ -702,12 +702,20 @@ type fakeFolderRepo struct {
getErr error
getCallCount int
err error
// hasOtherAudio is returned by HasAudioOutsideFolders (the album-root
// check). False means the parent qualifies as an album root.
hasOtherAudio bool
otherAudioErr error
}
func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) {
return f.result, f.err
}
func (f *fakeFolderRepo) HasAudioOutsideFolders(model.Folder, []string) (bool, error) {
return f.hasOtherAudio, f.otherAudioErr
}
func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) {
f.getCallCount++
if f.getErr != nil {

4
model/folder.go
View file

@ -86,6 +86,10 @@ type FolderRepository interface {
GetAll(...QueryOptions) ([]Folder, error)
CountAll(...QueryOptions) (int64, error)
GetFolderUpdateInfo(lib Library, targetPaths ...string) (map[string]FolderUpdateInfo, error)
// HasAudioOutsideFolders reports whether any folder in parent's subtree
// (including parent itself) contains audio files and is not one of the
// given folder IDs.
HasAudioOutsideFolders(parent Folder, excludeFolderIDs []string) (bool, error)
Put(*Folder) error
MarkMissing(missing bool, ids ...string) error
GetTouchedWithPlaylists() (FolderCursor, error)

28
persistence/folder_repository.go
View file

@ -7,6 +7,7 @@ import (
"iter"
"maps"
"os"
"path"
"path/filepath"
"slices"
"strings"
@ -188,6 +189,33 @@ func (r folderRepository) queryFolderUpdateInfo(where And) (map[string]model.Fol
return m, nil
}
// HasAudioOutsideFolders reports whether any folder in parent's subtree
// (including parent itself) contains audio files and is not one of the given
// folder IDs. LIKE wildcards in the parent path are escaped, so it is always
// matched as a literal prefix.
func (r folderRepository) HasAudioOutsideFolders(parent model.Folder, excludeFolderIDs []string) (bool, error) {
if parent.NumAudioFiles > 0 {
return true, nil
}
parentPath := strings.TrimPrefix(path.Join(parent.Path, parent.Name), "/")
return r.exists(And{
Eq{"library_id": parent.LibraryID, "missing": false},
Gt{"num_audio_files": 0},
NotEq{"id": excludeFolderIDs},
Or{
// Direct children have path = parentPath; deeper descendants match the prefix
Eq{"path": parentPath},
Expr(`path LIKE ? ESCAPE '\'`, escapeLikePrefix(parentPath)+"/%"),
},
})
}
// escapeLikePrefix escapes SQL LIKE wildcards so a string can be used as a
// literal prefix in a LIKE pattern (with ESCAPE '\').
func escapeLikePrefix(s string) string {
return strings.NewReplacer(`\`, `\\`, `%`, `\%`, `_`, `\_`).Replace(s)
}
func (r folderRepository) Put(f *model.Folder) error {
dbf := dbFolder{Folder: f}
_, err := r.put(dbf.ID, &dbf)

61
persistence/folder_repository_test.go
View file

@ -217,6 +217,67 @@ var _ = Describe("FolderRepository", func() {
})
})
Describe("HasAudioOutsideFolders", func() {
var albumRoot, disc1, disc2 *model.Folder
// TestHasAudio/Album/
// ├── CD1/ (audio, belongs to the album)
// └── CD2/ (audio, belongs to the album)
BeforeEach(func() {
albumRoot = model.NewFolder(testLib, "TestHasAudio/Album")
disc1 = model.NewFolder(testLib, "TestHasAudio/Album/CD1")
disc1.NumAudioFiles = 5
disc2 = model.NewFolder(testLib, "TestHasAudio/Album/CD2")
disc2.NumAudioFiles = 5
for _, f := range []*model.Folder{albumRoot, disc1, disc2} {
Expect(repo.Put(f)).To(Succeed())
}
})
It("returns false when all audio under the parent belongs to the given folders", func() {
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse())
})
It("returns true when another folder under the parent has audio", func() {
bonus := model.NewFolder(testLib, "TestHasAudio/Album/Bonus")
bonus.NumAudioFiles = 1
Expect(repo.Put(bonus)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue())
})
It("returns true when the parent itself contains audio files", func() {
albumRoot.NumAudioFiles = 2
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeTrue())
})
It("ignores audio outside the parent's subtree", func() {
other := model.NewFolder(testLib, "TestHasAudio/Other Album")
other.NumAudioFiles = 10
Expect(repo.Put(other)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse())
})
It("ignores missing folders", func() {
gone := model.NewFolder(testLib, "TestHasAudio/Album/Gone")
gone.NumAudioFiles = 3
gone.Missing = true
Expect(repo.Put(gone)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*albumRoot, []string{disc1.ID, disc2.ID})).To(BeFalse())
})
It("does not treat LIKE wildcards in the parent path as patterns", func() {
// "TestHas_udio" would LIKE-match "TestHasAudio" if "_" were not escaped
wildcardRoot := model.NewFolder(testLib, "TestHas_udio/Album")
Expect(repo.Put(wildcardRoot)).To(Succeed())
Expect(repo.HasAudioOutsideFolders(*wildcardRoot, []string{"none"})).To(BeFalse())
})
})
Describe("wrapFolderCursor", func() {
It("does not panic when the cursor yields a dbFolder with nil Folder", func() {
// Simulate what queryWithStableResults does on the rows.Err() path: