diff --git a/core/artwork/reader_artist.go b/core/artwork/reader_artist.go index 487346b4d..cb029a16e 100644 --- a/core/artwork/reader_artist.go +++ b/core/artwork/reader_artist.go @@ -20,6 +20,12 @@ import ( "github.com/navidrome/navidrome/utils/str" ) +const ( + // maxArtistFolderTraversalDepth defines how many directory levels to search + // when looking for artist images (artist folder + parent directories) + maxArtistFolderTraversalDepth = 3 +) + type artistReader struct { cacheKey a *artwork @@ -108,36 +114,52 @@ func (a *artistReader) fromArtistArtPriority(ctx context.Context, priority strin func fromArtistFolder(ctx context.Context, artistFolder string, pattern string) sourceFunc { return func() (io.ReadCloser, string, error) { - fsys := os.DirFS(artistFolder) - matches, err := fs.Glob(fsys, pattern) - if err != nil { - log.Warn(ctx, "Error matching artist image pattern", "pattern", pattern, "folder", artistFolder) - return nil, "", err - } - if len(matches) == 0 { - return nil, "", fmt.Errorf(`no matches for '%s' in '%s'`, pattern, artistFolder) - } - for _, m := range matches { - filePath := filepath.Join(artistFolder, m) - if !model.IsImageFile(m) { - continue + current := artistFolder + for i := 0; i < maxArtistFolderTraversalDepth; i++ { + if reader, path, err := findImageInFolder(ctx, current, pattern); err == nil { + return reader, path, nil } - f, err := os.Open(filePath) - if err != nil { - log.Warn(ctx, "Could not open cover art file", "file", filePath, err) - return nil, "", err + + parent := filepath.Dir(current) + if parent == current { + break } - return f, filePath, nil + current = parent } - return nil, "", nil + return nil, "", fmt.Errorf(`no matches for '%s' in '%s' or its parent directories`, pattern, artistFolder) } } +func findImageInFolder(ctx context.Context, folder, pattern string) (io.ReadCloser, string, error) { + log.Trace(ctx, "looking for artist image", "pattern", pattern, "folder", folder) + fsys := os.DirFS(folder) + matches, err := fs.Glob(fsys, pattern) + if err != nil { + log.Warn(ctx, "Error matching artist image pattern", "pattern", pattern, "folder", folder, err) + return nil, "", err + } + + for _, m := range matches { + if !model.IsImageFile(m) { + continue + } + filePath := filepath.Join(folder, m) + f, err := os.Open(filePath) + if err != nil { + log.Warn(ctx, "Could not open cover art file", "file", filePath, err) + continue + } + return f, filePath, nil + } + + return nil, "", fmt.Errorf(`no matches for '%s' in '%s'`, pattern, folder) +} + 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 + 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)) { diff --git a/core/artwork/reader_artist_test.go b/core/artwork/reader_artist_test.go index 294a5db0b..527b0849f 100644 --- a/core/artwork/reader_artist_test.go +++ b/core/artwork/reader_artist_test.go @@ -3,6 +3,8 @@ package artwork import ( "context" "errors" + "io" + "os" "path/filepath" "time" @@ -108,6 +110,254 @@ var _ = Describe("artistArtworkReader", func() { }) }) }) + + var _ = Describe("fromArtistFolder", func() { + var ( + ctx context.Context + tempDir string + testFunc sourceFunc + ) + + BeforeEach(func() { + ctx = context.Background() + tempDir = GinkgoT().TempDir() + }) + + When("artist folder contains matching image", func() { + BeforeEach(func() { + // Create test structure: /temp/artist/artist.jpg + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + artistImagePath := filepath.Join(artistDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("fake image data"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds and returns the image", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("artist.jpg")) + + // Verify we can read the content + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("fake image data")) + reader.Close() + }) + }) + + When("artist folder is empty but parent contains image", func() { + BeforeEach(func() { + // Create test structure: /temp/parent/artist.jpg and /temp/parent/artist/album/ + parentDir := filepath.Join(tempDir, "parent") + artistDir := filepath.Join(parentDir, "artist") + albumDir := filepath.Join(artistDir, "album") + Expect(os.MkdirAll(albumDir, 0755)).To(Succeed()) + + // Put artist image in parent directory + artistImagePath := filepath.Join(parentDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("parent image"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds image in parent directory", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("parent" + string(filepath.Separator) + "artist.jpg")) + + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("parent image")) + reader.Close() + }) + }) + + When("image is two levels up", func() { + BeforeEach(func() { + // Create test structure: /temp/grandparent/artist.jpg and /temp/grandparent/parent/artist/ + grandparentDir := filepath.Join(tempDir, "grandparent") + parentDir := filepath.Join(grandparentDir, "parent") + artistDir := filepath.Join(parentDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Put artist image in grandparent directory + artistImagePath := filepath.Join(grandparentDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("grandparent image"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds image in grandparent directory", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("grandparent" + string(filepath.Separator) + "artist.jpg")) + + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("grandparent image")) + reader.Close() + }) + }) + + When("images exist at multiple levels", func() { + BeforeEach(func() { + // Create test structure with images at multiple levels + grandparentDir := filepath.Join(tempDir, "grandparent") + parentDir := filepath.Join(grandparentDir, "parent") + artistDir := filepath.Join(parentDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Put artist images at all levels + Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("artist level"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(parentDir, "artist.jpg"), []byte("parent level"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(grandparentDir, "artist.jpg"), []byte("grandparent level"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("prioritizes the closest (artist folder) image", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("artist" + string(filepath.Separator) + "artist.jpg")) + + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("artist level")) + reader.Close() + }) + }) + + When("pattern matches multiple files", func() { + BeforeEach(func() { + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Create multiple matching files + Expect(os.WriteFile(filepath.Join(artistDir, "artist.jpg"), []byte("jpg image"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(artistDir, "artist.png"), []byte("png image"), 0600)).To(Succeed()) + Expect(os.WriteFile(filepath.Join(artistDir, "artist.txt"), []byte("text file"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("returns the first valid image file", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + + // Should return an image file, not the text file + Expect(path).To(SatisfyAny( + ContainSubstring("artist.jpg"), + ContainSubstring("artist.png"), + )) + Expect(path).ToNot(ContainSubstring("artist.txt")) + reader.Close() + }) + }) + + When("no matching files exist anywhere", func() { + BeforeEach(func() { + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Create non-matching files + Expect(os.WriteFile(filepath.Join(artistDir, "cover.jpg"), []byte("cover image"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("returns an error", func() { + reader, path, err := testFunc() + Expect(err).To(HaveOccurred()) + Expect(reader).To(BeNil()) + Expect(path).To(BeEmpty()) + Expect(err.Error()).To(ContainSubstring("no matches for 'artist.*'")) + Expect(err.Error()).To(ContainSubstring("parent directories")) + }) + }) + + When("directory traversal reaches filesystem root", func() { + BeforeEach(func() { + // Start from a shallow directory to test root boundary + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("handles root boundary gracefully", func() { + reader, path, err := testFunc() + Expect(err).To(HaveOccurred()) + Expect(reader).To(BeNil()) + Expect(path).To(BeEmpty()) + // Should not panic or cause infinite loop + }) + }) + + When("file exists but cannot be opened", func() { + BeforeEach(func() { + artistDir := filepath.Join(tempDir, "artist") + Expect(os.MkdirAll(artistDir, 0755)).To(Succeed()) + + // Create a file that cannot be opened (permission denied) + restrictedFile := filepath.Join(artistDir, "artist.jpg") + Expect(os.WriteFile(restrictedFile, []byte("restricted"), 0600)).To(Succeed()) + + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("logs warning and continues searching", func() { + // This test depends on the ability to restrict file permissions + // For now, we'll just ensure it doesn't panic and returns appropriate error + reader, _, err := testFunc() + // The file should be readable in test environment, so this will succeed + // In a real scenario with permission issues, it would continue searching + if err == nil { + Expect(reader).ToNot(BeNil()) + reader.Close() + } + }) + }) + + When("single album artist scenario (original issue)", func() { + BeforeEach(func() { + // Simulate the exact folder structure from the issue: + // /music/artist/album1/ (single album) + // /music/artist/artist.jpg (artist image that should be found) + artistDir := filepath.Join(tempDir, "music", "artist") + albumDir := filepath.Join(artistDir, "album1") + Expect(os.MkdirAll(albumDir, 0755)).To(Succeed()) + + // Create artist.jpg in the artist folder (this was not being found before) + artistImagePath := filepath.Join(artistDir, "artist.jpg") + Expect(os.WriteFile(artistImagePath, []byte("single album artist image"), 0600)).To(Succeed()) + + // The fromArtistFolder is called with the artist folder path + testFunc = fromArtistFolder(ctx, artistDir, "artist.*") + }) + + It("finds artist.jpg in artist folder for single album artist", func() { + reader, path, err := testFunc() + Expect(err).ToNot(HaveOccurred()) + Expect(reader).ToNot(BeNil()) + Expect(path).To(ContainSubstring("artist.jpg")) + Expect(path).To(ContainSubstring("artist")) + + // Verify the content + data, err := io.ReadAll(reader) + Expect(err).ToNot(HaveOccurred()) + Expect(string(data)).To(Equal("single album artist image")) + reader.Close() + }) + }) + }) }) type fakeFolderRepo struct {