mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(artwork): treat an unreadable artist-folder image as a failure
Third and last site in this class: findImageInFolder logged and skipped an image the glob had already matched, so a permissions or mount failure during the artist-folder traversal read as "no image here" and let processItem settle the artist absent, discarding any artwork already resolved. A matched-but-unreadable file now propagates through fromArtistFolder and lands as localError, the same as album folder art, embedded art and uploads. A folder with no match stays a definitive miss. Also normalizes the e2e path assertions with filepath.ToSlash: the stored SourcePath is OS-native, so the forward-slash suffixes failed all 23 folder specs on Windows. Reported by Codex on #5847.
This commit is contained in:
parent
6f93fa3141
commit
7ca41e5c22
@ -203,7 +203,7 @@ func expectAlbumFolderCover(al model.Album, suffix string) {
|
||||
Expect(serveBytes(al.CoverArtID())).To(Equal(libFileBytes(suffix)))
|
||||
ia := acquire(model.KindAlbumArtwork, al.ID)
|
||||
Expect(ia.Source).To(Equal("folder"))
|
||||
Expect(ia.SourcePath).To(HaveSuffix(suffix))
|
||||
Expect(filepath.ToSlash(ia.SourcePath)).To(HaveSuffix(suffix))
|
||||
}
|
||||
|
||||
// requireNoStateRow guards the byte-level folder assertions: a drain resolves every ready queue
|
||||
@ -232,7 +232,7 @@ func expectArtistFolder(ar model.Artist, suffix string) {
|
||||
Expect(serveBytes(model.NewArtworkID(model.KindArtistArtwork, ar.ID, nil))).To(Equal(libFileBytes(suffix)))
|
||||
ia := acquire(model.KindArtistArtwork, ar.ID)
|
||||
Expect(ia.Source).To(Equal("folder"))
|
||||
Expect(ia.SourcePath).To(HaveSuffix(suffix))
|
||||
Expect(filepath.ToSlash(ia.SourcePath)).To(HaveSuffix(suffix))
|
||||
}
|
||||
|
||||
// writeUploadedImage drops raw bytes into the per-entity upload folder under DataFolder, matching
|
||||
|
||||
@ -2,6 +2,7 @@ package artwork
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"io/fs"
|
||||
@ -38,17 +39,24 @@ func fromArtistFolder(ctx context.Context, libFS fs.FS, libPath, artistFolder, p
|
||||
// return backslash separators on Windows.
|
||||
rel = filepath.ToSlash(rel)
|
||||
current := artistFolder
|
||||
var unreadable error
|
||||
for range maxArtistFolderTraversalDepth {
|
||||
reader, hit, err := findImageInFolder(ctx, libFS, rel, current, pattern)
|
||||
if err == nil {
|
||||
return reader, hit, nil
|
||||
}
|
||||
if errors.Is(err, errSourceUnreadable) {
|
||||
unreadable = err
|
||||
}
|
||||
if rel == "." {
|
||||
break // reached library root; don't traverse above it
|
||||
}
|
||||
rel = path.Dir(rel)
|
||||
current = filepath.Dir(current)
|
||||
}
|
||||
if unreadable != nil {
|
||||
return nil, "", unreadable
|
||||
}
|
||||
return nil, "", fmt.Errorf(`no matches for '%s' in '%s' or its parent directories (within library)`, pattern, artistFolder)
|
||||
}
|
||||
}
|
||||
@ -82,15 +90,20 @@ func findImageInFolder(ctx context.Context, libFS fs.FS, relFolder, absFolder, p
|
||||
// suffixes (e.g., artist.jpg before artist.1.jpg)
|
||||
slices.SortFunc(imagePaths, compareImageFiles)
|
||||
|
||||
var openErr error
|
||||
for _, p := range imagePaths {
|
||||
f, err := libFS.Open(p)
|
||||
if err != nil {
|
||||
log.Warn(ctx, "Could not open cover art file", "file", p, err)
|
||||
openErr = fmt.Errorf("%w: %s: %w", errSourceUnreadable, p, err)
|
||||
continue
|
||||
}
|
||||
_, name := path.Split(p)
|
||||
return f, filepath.Join(absFolder, name), nil
|
||||
}
|
||||
if openErr != nil {
|
||||
return nil, "", openErr
|
||||
}
|
||||
|
||||
return nil, "", fmt.Errorf(`no matches for '%s' in '%s'`, pattern, absFolder)
|
||||
}
|
||||
|
||||
48
core/artwork/folders_artist_test.go
Normal file
48
core/artwork/folders_artist_test.go
Normal file
@ -0,0 +1,48 @@
|
||||
package artwork
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"os"
|
||||
"path/filepath"
|
||||
|
||||
. "github.com/onsi/ginkgo/v2"
|
||||
. "github.com/onsi/gomega"
|
||||
)
|
||||
|
||||
var _ = Describe("findImageInFolder", func() {
|
||||
var ctx context.Context
|
||||
var dir string
|
||||
|
||||
BeforeEach(func() {
|
||||
ctx = context.Background()
|
||||
dir = GinkgoT().TempDir()
|
||||
})
|
||||
|
||||
It("returns the first matching image", func() {
|
||||
Expect(os.WriteFile(filepath.Join(dir, "artist.jpg"), []byte("img"), 0o600)).To(Succeed())
|
||||
|
||||
r, hit, err := findImageInFolder(ctx, os.DirFS(dir), ".", dir, "artist.*")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
defer r.Close()
|
||||
Expect(hit).To(HaveSuffix("artist.jpg"))
|
||||
})
|
||||
|
||||
// The glob matched, so the image exists; failing to open it says nothing about whether the
|
||||
// artist has one, and must not let the resolver settle on absent.
|
||||
It("reports a matched but unreadable image as unreadable, not as a miss", func() {
|
||||
img := filepath.Join(dir, "artist.jpg")
|
||||
Expect(os.WriteFile(img, []byte("img"), 0o600)).To(Succeed())
|
||||
Expect(os.Chmod(img, 0o000)).To(Succeed())
|
||||
DeferCleanup(func() { _ = os.Chmod(img, 0o600) })
|
||||
|
||||
_, _, err := findImageInFolder(ctx, os.DirFS(dir), ".", dir, "artist.*")
|
||||
Expect(err).To(MatchError(errSourceUnreadable))
|
||||
})
|
||||
|
||||
It("reports a plain miss when nothing matches", func() {
|
||||
_, _, err := findImageInFolder(ctx, os.DirFS(dir), ".", dir, "artist.*")
|
||||
Expect(err).To(HaveOccurred())
|
||||
Expect(errors.Is(err, errSourceUnreadable)).To(BeFalse(), "no match is definitive, not transient")
|
||||
})
|
||||
})
|
||||
@ -192,10 +192,12 @@ func resolveArtist(ctx context.Context, ds model.DataStore, ag *agents.Agents, f
|
||||
if lib.FS == nil || artistFolder == "" {
|
||||
continue
|
||||
}
|
||||
if res, ok := resolveArtistFolderPattern(ctx, lib, artistFolder, pattern); ok {
|
||||
res, ok := resolveArtistFolderPattern(ctx, lib, artistFolder, pattern)
|
||||
if ok {
|
||||
res.extError = extErr
|
||||
return res, nil
|
||||
}
|
||||
localErr = localErr || res.localError
|
||||
}
|
||||
}
|
||||
return resolution{extError: extErr, localError: localErr}, nil
|
||||
@ -393,9 +395,9 @@ func resolveArtistImageFolder(ar *model.Artist) (resolution, bool) {
|
||||
}
|
||||
|
||||
func resolveArtistFolderPattern(ctx context.Context, lib libraryView, artistFolder, pattern string) (resolution, bool) {
|
||||
r, path, _ := fromArtistFolder(ctx, lib.FS, lib.absRoot, artistFolder, pattern)()
|
||||
r, path, err := fromArtistFolder(ctx, lib.FS, lib.absRoot, artistFolder, pattern)()
|
||||
if r == nil {
|
||||
return resolution{}, false
|
||||
return resolution{localError: errors.Is(err, errSourceUnreadable)}, false
|
||||
}
|
||||
return resolution{reader: r, source: "folder", sourcePath: path, refMtime: mtimeOf(path)}, true
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user