mirror of
https://github.com/navidrome/navidrome.git
synced 2026-04-17 13:10:27 -04:00
fix(artwork): retry remaining fallbacks when the first one fails to open
Review feedback: the previous shape remembered only the first unnumbered candidate and fell through to a generic error if os.Open failed on it, even though other matching unnumbered files in imgFiles could have succeeded. The pre-PR code was more resilient because it looped and continued on open failure. fromExternalFile now collects every viable unnumbered candidate into a slice during the scan, then tries them in order after the loop, mirroring the pre-PR retry-on-open-failure behavior. Numbered matches still return immediately on first success and skip the candidate list entirely — an open failure on a numbered match means no other file has that number anyway. Also: - globMetaChars doc comment now notes that '\' escape is intentionally excluded (filepath.Match supports it but treating it as a metachar here would misalign extractDiscNumber's literal-prefix extraction with no benefit for realistic config patterns). - The 'cover.jpg doesn't match disc*.*' Entry in the extractDiscNumber table is renamed to 'cover.jpg with disc*.* (no prefix match)' to reflect that the test now exercises the HasPrefix defensive guard, not the removed internal filepath.Match check. Regression test added: a single-folder album with a deleted first candidate file resolves to the second candidate.
This commit is contained in:
@@ -168,7 +168,11 @@ func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle strin
|
||||
}
|
||||
}
|
||||
|
||||
// globMetaChars lists every metacharacter understood by filepath.Match.
|
||||
// globMetaChars holds the substitution metacharacters understood by
|
||||
// filepath.Match. The '\' escape character is intentionally excluded:
|
||||
// disc art patterns come from user config and never include escaped
|
||||
// metachars in practice, and treating '\' as a metachar would misalign
|
||||
// the literal-prefix extraction in extractDiscNumber.
|
||||
const globMetaChars = "*?["
|
||||
|
||||
// extractDiscNumber parses the disc number from a filename matched by a
|
||||
@@ -208,7 +212,7 @@ func extractDiscNumber(pattern, filename string) (int, bool) {
|
||||
func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc {
|
||||
isLiteral := !strings.ContainsAny(pattern, globMetaChars)
|
||||
return func() (io.ReadCloser, string, error) {
|
||||
var fallback string
|
||||
var fallbacks []string
|
||||
for _, file := range d.imgFiles {
|
||||
_, name := filepath.Split(file)
|
||||
name = strings.ToLower(name)
|
||||
@@ -235,13 +239,10 @@ func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string
|
||||
}
|
||||
}
|
||||
|
||||
if fallback != "" {
|
||||
continue
|
||||
}
|
||||
if d.isMultiFolder && !d.discFolders[filepath.Dir(file)] {
|
||||
continue
|
||||
}
|
||||
fallback = file
|
||||
fallbacks = append(fallbacks, file)
|
||||
// A literal filename can only match one file, so stop as soon
|
||||
// as we have a viable fallback.
|
||||
if isLiteral {
|
||||
@@ -249,13 +250,13 @@ func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string
|
||||
}
|
||||
}
|
||||
|
||||
if fallback != "" {
|
||||
f, err := os.Open(fallback)
|
||||
for _, file := range fallbacks {
|
||||
f, err := os.Open(file)
|
||||
if err != nil {
|
||||
log.Warn(ctx, "Could not open disc art file", "file", fallback, err)
|
||||
} else {
|
||||
return f, fallback, nil
|
||||
log.Warn(ctx, "Could not open disc art file", "file", file, err)
|
||||
continue
|
||||
}
|
||||
return f, file, nil
|
||||
}
|
||||
return nil, "", fmt.Errorf("disc %d: pattern '%s' not matched by files", d.discNumber, pattern)
|
||||
}
|
||||
|
||||
@@ -42,8 +42,8 @@ var _ = Describe("Disc Artwork Reader", func() {
|
||||
// Case insensitive (filename already lowered by caller)
|
||||
Entry("Disc1.jpg lowered", "disc*.*", "disc1.jpg", 1, true),
|
||||
|
||||
// Pattern doesn't match
|
||||
Entry("cover.jpg doesn't match disc*.*", "disc*.*", "cover.jpg", 0, false),
|
||||
// HasPrefix guard: filename doesn't share the pattern's literal prefix
|
||||
Entry("cover.jpg with disc*.* (no prefix match)", "disc*.*", "cover.jpg", 0, false),
|
||||
|
||||
// Pattern with no wildcard before dot
|
||||
Entry("front1.jpg with front*.*", "front*.*", "front1.jpg", 1, true),
|
||||
@@ -207,6 +207,25 @@ var _ = Describe("Disc Artwork Reader", func() {
|
||||
Entry("disc 3 falls back to disc.jpg when no numbered match exists", 3, 0),
|
||||
)
|
||||
|
||||
It("tries the next fallback candidate when the first one cannot be opened", func() {
|
||||
f1 := createFile("album/cover.jpg")
|
||||
f2 := createFile("album/cover.png")
|
||||
// Remove f1 so os.Open will fail on it; f2 should still win.
|
||||
Expect(os.Remove(f1)).To(Succeed())
|
||||
reader := &discArtworkReader{
|
||||
discNumber: 1,
|
||||
imgFiles: []string{f1, f2},
|
||||
discFolders: map[string]bool{filepath.Join(tmpDir, "album"): true},
|
||||
}
|
||||
|
||||
sf := reader.fromExternalFile(ctx, "cover.*")
|
||||
r, path, err := sf()
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(r).ToNot(BeNil())
|
||||
r.Close()
|
||||
Expect(path).To(Equal(f2))
|
||||
})
|
||||
|
||||
DescribeTable("filters by disc number for non-'*' wildcard patterns",
|
||||
func(pattern string, discNumber, expectedIdx int) {
|
||||
files := []string{
|
||||
|
||||
Reference in New Issue
Block a user