diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index 9fc9262c..36b2fff0 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -4,6 +4,7 @@ import ( "cmp" "context" "crypto/md5" + "errors" "fmt" "io" "path/filepath" @@ -17,6 +18,7 @@ import ( "github.com/navidrome/navidrome/core" "github.com/navidrome/navidrome/core/external" "github.com/navidrome/navidrome/core/ffmpeg" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" ) @@ -103,6 +105,28 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo if err != nil { return nil, nil, nil, err } + + folderIDSet := make(map[string]bool, len(folderIDs)) + for _, id := range folderIDs { + folderIDSet[id] = true + } + + // For multi-disc albums (2+ folders), 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 "Artist/Album/CD1/" and "Artist/Album/CD2/"). + // We skip single-folder albums to avoid pulling images from the artist folder. + if commonParentID := commonParentFolder(folders, folderIDSet); commonParentID != "" { + 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 { + folders = append(folders, *parentFolder) + } + } + var paths []string var imgFiles []string var updatedAt time.Time @@ -125,6 +149,24 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo return paths, imgFiles, &updatedAt, nil } +// commonParentFolder returns the shared parent folder ID when all folders have the +// same parent and that parent is not already in folderIDSet. Returns "" otherwise. +func commonParentFolder(folders []model.Folder, folderIDSet map[string]bool) string { + if len(folders) < 2 { + return "" + } + parentID := folders[0].ParentID + if parentID == "" || folderIDSet[parentID] { + return "" + } + for _, f := range folders[1:] { + if f.ParentID != parentID { + return "" + } + } + return parentID +} + // compareImageFiles compares two image file paths for sorting. // It extracts the base filename (without extension) and compares case-insensitively. // This ensures that "cover.jpg" sorts before "cover.1.jpg" since "cover" < "cover.1". diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index fd5f8a2b..a8a0eae3 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -2,6 +2,7 @@ package artwork import ( "context" + "errors" "path/filepath" "time" @@ -116,5 +117,181 @@ var _ = Describe("Album Artwork Reader", func() { Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg"))) Expect(imgFiles[2]).To(Equal(filepath.FromSlash("Artist/Album/Folder.jpg"))) }) + + It("includes images from parent folder for multi-disc albums", func() { + // Simulates: Artist/Album/cover.jpg with tracks in Artist/Album/CD1/ and Artist/Album/CD2/ + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist/Album", + Name: "CD1", + ParentID: "parentFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + { + ID: "folder2", + Path: "Artist/Album", + Name: "CD2", + ParentID: "parentFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.parentResult = &model.Folder{ + ID: "parentFolder", + Path: "Artist", + Name: "Album", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg", "back.jpg"}, + } + + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(*imagesUpdatedAt).To(Equal(expectedAt)) + Expect(imgFiles).To(HaveLen(2)) + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/back.jpg"))) + Expect(imgFiles[1]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg"))) + }) + + It("does not query parent when parent ID is already in album folders", func() { + // When the parent folder is already one of the album's folders, skip it + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist", + Name: "Album", + ParentID: "folder2", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + ID: "folder2", + Path: "", + Name: "Artist", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(1)) + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg"))) + // Get should not have been called (parent already in folder set) + Expect(repo.getCallCount).To(Equal(0)) + }) + + It("does not query parent when folders have different parents", func() { + // When album folders span different parents, don't search any parent + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist1/Album", + Name: "part1", + ParentID: "parentA", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + ID: "folder2", + Path: "Artist2/Album", + Name: "part2", + ParentID: "parentB", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(1)) + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist1/Album/part1/cover.jpg"))) + // Get should not have been called (different parents) + Expect(repo.getCallCount).To(Equal(0)) + }) + + It("does not query parent for single-folder albums", func() { + // A single-folder album's parent is typically the artist folder, + // which should not be searched for cover art + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist", + Name: "Album", + ParentID: "artistFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + } + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(1)) + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/cover.jpg"))) + // Get should not have been called (single folder, no parent lookup) + Expect(repo.getCallCount).To(Equal(0)) + }) + + It("propagates non-ErrNotFound errors from parent folder lookup", func() { + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist/Album", + Name: "CD1", + ParentID: "parentFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + ID: "folder2", + Path: "Artist/Album", + Name: "CD2", + ParentID: "parentFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + repo.getErr = errors.New("db connection failed") + + _, _, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).To(MatchError("db connection failed")) + Expect(repo.getCallCount).To(Equal(1)) + }) + + It("continues gracefully when parent folder is not found", func() { + // Parent folder may have been deleted; should log a warning and continue + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Artist/Album", + Name: "CD1", + ParentID: "missingParent", + ImagesUpdatedAt: now, + ImageFiles: []string{"cover.jpg"}, + }, + { + ID: "folder2", + Path: "Artist/Album", + Name: "CD2", + ParentID: "missingParent", + ImagesUpdatedAt: now, + ImageFiles: []string{}, + }, + } + // parentResult is nil, so Get will return ErrNotFound + + _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(imgFiles).To(HaveLen(1)) + Expect(imgFiles[0]).To(Equal(filepath.FromSlash("Artist/Album/CD1/cover.jpg"))) + Expect(repo.getCallCount).To(Equal(1)) + }) }) }) diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index e6a0168f..4aa71c9c 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -417,14 +417,28 @@ var _ = Describe("artistArtworkReader", func() { type fakeFolderRepo struct { model.FolderRepository - result []model.Folder - err error + result []model.Folder + parentResult *model.Folder + getErr error + getCallCount int + err error } func (f *fakeFolderRepo) GetAll(...model.QueryOptions) ([]model.Folder, error) { return f.result, f.err } +func (f *fakeFolderRepo) Get(id string) (*model.Folder, error) { + f.getCallCount++ + if f.getErr != nil { + return nil, f.getErr + } + if f.parentResult != nil { + return f.parentResult, nil + } + return nil, model.ErrNotFound +} + type fakeDataStore struct { model.DataStore folderRepo *fakeFolderRepo