diff --git a/core/artwork/e2e/album_test.go b/core/artwork/e2e/album_test.go index 370844e34..e765e1b1b 100644 --- a/core/artwork/e2e/album_test.go +++ b/core/artwork/e2e/album_test.go @@ -37,15 +37,15 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 2 variant: cover.* basenames tie across album-root and per-disc folders; - // compareImageFiles' lexicographic full-path tiebreaker ranks disc-subfolder - // files first. + // https://github.com/navidrome/navidrome/issues/5376 + // cover.* basenames tie across album-root and per-disc folders; + // compareImageFiles must prefer shallower paths. When("a multi-disc album has a cover.jpg at the album root and per-disc covers", func() { // Artist/ // └── Album/ // ├── CD1/ // │ ├── 01 - Track.mp3 - // │ └── cover.jpg ← currently wins (bug) + // │ └── cover.jpg ← should not win // ├── CD2/ // │ ├── 01 - Track.mp3 // │ └── cover.jpg @@ -68,15 +68,15 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 2: folder.jpg basenames tie across album-root and per-disc folders; - // the lexicographic full-path tiebreaker in compareImageFiles ranks - // "Artist/Album/CD1/folder.jpg" ahead of "Artist/Album/folder.jpg". + // https://github.com/navidrome/navidrome/issues/5376 + // folder.jpg basenames tie across album-root and per-disc folders; + // compareImageFiles must prefer shallower paths. When("a multi-disc album has folder.jpg at the album root AND in each disc subfolder", func() { // Artist/ // └── Album/ // ├── CD1/ // │ ├── 01 - Track.mp3 - // │ └── folder.jpg ← currently wins (bug) + // │ └── folder.jpg ← should not win // ├── CD2/ // │ ├── 01 - Track.mp3 // │ └── folder.jpg @@ -97,15 +97,14 @@ var _ = Describe("Album artwork resolution", func() { }) }) - // Bug 1: commonParentFolder's `len(folders) < 2` guard skips the parent-folder - // lookup whenever an album lives entirely under a single subfolder, so an - // album-root cover is never considered. + // https://github.com/navidrome/navidrome/issues/5376 + // Single-subfolder albums must still consider the parent folder's images. When("an album lives entirely under a single disc subfolder with cover.jpg at the parent", func() { // Artist/ // └── Album/ // ├── disc1/ // │ └── 01 - Track.mp3 - // └── cover.jpg ← should win (parent-folder fallback, currently ignored — bug) + // └── cover.jpg ← should win (parent-folder fallback) It("uses the parent-folder cover for single-disc-subfolder albums", func() { conf.Server.CoverArtPriority = defaultCoverPriority setLayout(fstest.MapFS{ @@ -119,6 +118,32 @@ var _ = Describe("Album artwork resolution", func() { }) }) + // https://github.com/navidrome/navidrome/issues/5456 + When("a top-level multi-disc album has cover.jpg at the album root and per-disc folder.jpg", func() { + // Album/ (top-level folder, Path=".") + // ├── CD1/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // ├── CD2/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── cover.jpg ← should win (album-root) + It("prefers the album-root cover.jpg", func() { + conf.Server.CoverArtPriority = defaultCoverPriority + setLayout(fstest.MapFS{ + "Album/CD1/01 - Track.mp3": trackFile(1, "Track CD1"), + "Album/CD2/01 - Track.mp3": trackFile(1, "Track CD2"), + "Album/cover.jpg": imageFile("album-root"), + "Album/CD1/folder.jpg": imageFile("disc1"), + "Album/CD2/folder.jpg": imageFile("disc2"), + }) + scan() + + al := firstAlbum() + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root"))) + }) + }) + When("CoverArtPriority puts embedded first and the album has both embedded and external art", func() { // Artist/ // └── Album/ diff --git a/core/artwork/e2e/disc_test.go b/core/artwork/e2e/disc_test.go index 7569cbc32..667079458 100644 --- a/core/artwork/e2e/disc_test.go +++ b/core/artwork/e2e/disc_test.go @@ -1,6 +1,7 @@ package artworke2e_test import ( + "fmt" "testing/fstest" "github.com/navidrome/navidrome/conf" @@ -255,6 +256,100 @@ var _ = Describe("Disc artwork resolution", func() { }) }) + // Reproduces https://github.com/navidrome/navidrome/issues/5456 + // Deeply nested layout matching the reporter's actual structure. + When("a deeply nested multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Genre/Artist/Album/ ← album root with cover.jpg + // ├── cover.jpg ← album-level cover + // ├── Disc 01 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02 (Subtitle)/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── ... (12 discs) + It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { + conf.Server.DiscArtPriority = defaultDiscPriority + conf.Server.CoverArtPriority = defaultCoverPriority + discNames := []string{ + "Disc 01 (Birth of the Dead - The Studio Sides)", + "Disc 02 (Birth of the Dead - The Live Sides)", + "Disc 03 (The Grateful Dead)", + "Disc 04 (Anthem of the Sun)", + "Disc 05 (Aoxomoxoa)", + "Disc 06 (Live; Dead)", + "Disc 07 (Workingman's Dead)", + "Disc 08 (American Beauty)", + "Disc 09 (Grateful Dead)", + "Disc 10 (Europe '72)", + "Disc 11 (Europe '72)", + "Disc 12 (History of the Grateful Dead, Volume One (Bear's Choice))", + } + layout := fstest.MapFS{ + "Pop; Rock/Grateful Dead/(2001) The Golden Road/cover.jpg": imageFile("album-root-cover"), + } + for i, name := range discNames { + discNum := i + 1 + prefix := fmt.Sprintf("Pop; Rock/Grateful Dead/(2001) The Golden Road/%s/", name) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", discNum), map[string]any{"disc": fmt.Sprintf("%d", discNum)}) + layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", discNum)) + } + setLayout(layout) + scan() + + al := firstAlbum() + + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) + + for i := range discNames { + discNum := i + 1 + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, discNum), &al.UpdatedAt) + Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", discNum))), + "disc %d should use its own folder.jpg", discNum) + } + }) + }) + + // https://github.com/navidrome/navidrome/issues/5456 + // Top-level album variant — album folder at library root (Path="."). + When("a top-level multi-disc album has cover.jpg and per-disc folder.jpg", func() { + // Album/ (top-level, Path=".") + // ├── cover.jpg ← album-level cover + // ├── Disc 01/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg ← disc 1 art + // ├── Disc 02/ + // │ ├── 01 - Track.mp3 + // │ └── folder.jpg + // └── Disc 03/ + // ├── 01 - Track.mp3 + // └── folder.jpg + It("uses album-root cover.jpg for album art and per-disc folder.jpg for each disc", func() { + conf.Server.DiscArtPriority = defaultDiscPriority + conf.Server.CoverArtPriority = defaultCoverPriority + layout := fstest.MapFS{ + "Album/cover.jpg": imageFile("album-root-cover"), + } + for i := 1; i <= 3; i++ { + prefix := fmt.Sprintf("Album/Disc %02d/", i) + layout[prefix+"01 - Track.mp3"] = trackFile(1, fmt.Sprintf("T%d", i), map[string]any{"disc": fmt.Sprintf("%d", i)}) + layout[prefix+"folder.jpg"] = imageFile(fmt.Sprintf("disc-%02d-folder", i)) + } + setLayout(layout) + scan() + + al := firstAlbum() + + Expect(readArtwork(al.CoverArtID())).To(Equal(imageBytes("album-root-cover"))) + + for i := 1; i <= 3; i++ { + discID := model.NewArtworkID(model.KindDiscArtwork, model.DiscArtworkID(al.ID, i), &al.UpdatedAt) + Expect(readArtwork(discID)).To(Equal(imageBytes(fmt.Sprintf("disc-%02d-folder", i))), + "disc %d should use its own folder.jpg", i) + } + }) + }) + When("discsubtitle is set but no image filename matches the subtitle", func() { // Artist/ // └── Album/ diff --git a/core/artwork/reader_album.go b/core/artwork/reader_album.go index cf5497641..73ba9b5ee 100644 --- a/core/artwork/reader_album.go +++ b/core/artwork/reader_album.go @@ -18,6 +18,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" "github.com/navidrome/navidrome/utils/natural" ) @@ -53,10 +54,9 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar lib: lib, } a.cacheKey.artID = artID - if a.updatedAt != nil && a.updatedAt.After(al.UpdatedAt) { - a.cacheKey.lastUpdate = *a.updatedAt - } else { - a.cacheKey.lastUpdate = al.UpdatedAt + a.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) + if imagesUpdateAt != nil { + a.cacheKey.lastUpdate = utils.TimeNewest(a.cacheKey.lastUpdate, *imagesUpdateAt) } return a, nil } @@ -131,7 +131,7 @@ func loadAlbumFoldersPaths(ctx context.Context, ds model.DataStore, albums ...mo } else if err != nil { return nil, nil, nil, err } - if parentFolder != nil && parentFolder.Path != "." { + if parentFolder != nil && parentFolder.ParentID != "" { folders = append(folders, *parentFolder) } } diff --git a/core/artwork/reader_album_test.go b/core/artwork/reader_album_test.go index b8f4f2dfa..1cf039bee 100644 --- a/core/artwork/reader_album_test.go +++ b/core/artwork/reader_album_test.go @@ -141,6 +141,7 @@ var _ = Describe("Album Artwork Reader", func() { ID: "parentFolder", Path: "Artist", Name: "Album", + ParentID: "artistFolder", ImagesUpdatedAt: expectedAt, ImageFiles: []string{"cover.jpg", "back.jpg"}, } @@ -213,14 +214,14 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(0)) }) - It("does not include top-level parent for multi-folder albums", func() { - // Two album parts under the same artist folder — parent is artist-level + It("does not include library root parent for multi-folder albums", func() { + // Two album parts directly under the library root — parent is the root itself repo.result = []model.Folder{ { ID: "folder1", Path: ".", Name: "AlbumPart1", - ParentID: "artistFolder", + ParentID: "rootFolder", ImagesUpdatedAt: now, ImageFiles: []string{"cover.jpg"}, }, @@ -228,16 +229,17 @@ var _ = Describe("Album Artwork Reader", func() { ID: "folder2", Path: ".", Name: "AlbumPart2", - ParentID: "artistFolder", + ParentID: "rootFolder", ImagesUpdatedAt: now, ImageFiles: []string{}, }, } repo.parentResult = &model.Folder{ - ID: "artistFolder", - Path: ".", - Name: "Artist", - ImageFiles: []string{"artist.jpg"}, + ID: "rootFolder", + Path: "", + Name: ".", + ParentID: "", + ImageFiles: []string{"unrelated.jpg"}, } _, imgFiles, _, err := loadAlbumFoldersPaths(ctx, ds, album) @@ -248,6 +250,46 @@ var _ = Describe("Album Artwork Reader", func() { Expect(repo.getCallCount).To(Equal(1)) }) + It("includes top-level album folder for multi-disc albums", func() { + // Album folder directly under library root, with disc subfolders + repo.result = []model.Folder{ + { + ID: "folder1", + Path: "Album", + Name: "Disc1", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"folder.jpg"}, + }, + { + ID: "folder2", + Path: "Album", + Name: "Disc2", + ParentID: "albumFolder", + ImagesUpdatedAt: now, + ImageFiles: []string{"folder.jpg"}, + }, + } + repo.parentResult = &model.Folder{ + ID: "albumFolder", + Path: ".", + Name: "Album", + ParentID: "rootFolder", + ImagesUpdatedAt: expectedAt, + ImageFiles: []string{"cover.jpg"}, + } + + _, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, ds, album) + + Expect(err).ToNot(HaveOccurred()) + Expect(*imagesUpdatedAt).To(Equal(expectedAt)) + Expect(imgFiles).To(HaveLen(3)) + Expect(imgFiles[0]).To(Equal("Album/cover.jpg")) + Expect(imgFiles[1]).To(Equal("Album/Disc1/folder.jpg")) + Expect(imgFiles[2]).To(Equal("Album/Disc2/folder.jpg")) + Expect(repo.getCallCount).To(Equal(1)) + }) + It("does not query parent for single-folder albums that already have images", func() { repo.result = []model.Folder{ { @@ -283,6 +325,7 @@ var _ = Describe("Album Artwork Reader", func() { ID: "albumFolder", Path: "Artist", Name: "Album", + ParentID: "artistFolder", ImagesUpdatedAt: expectedAt, ImageFiles: []string{"cover.jpg"}, } diff --git a/core/artwork/reader_disc.go b/core/artwork/reader_disc.go index de0a765f0..0f648c987 100644 --- a/core/artwork/reader_disc.go +++ b/core/artwork/reader_disc.go @@ -16,6 +16,7 @@ import ( "github.com/navidrome/navidrome/core/ffmpeg" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/utils" ) type discArtworkReader struct { @@ -105,10 +106,9 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID updatedAt: imagesUpdatedAt, } r.cacheKey.artID = artID - if r.updatedAt != nil && r.updatedAt.After(al.UpdatedAt) { - r.cacheKey.lastUpdate = *r.updatedAt - } else { - r.cacheKey.lastUpdate = al.UpdatedAt + r.cacheKey.lastUpdate = utils.TimeNewest(al.UpdatedAt, al.ImportedAt) + if imagesUpdatedAt != nil { + r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt) } return r, nil } diff --git a/core/auth/auth.go b/core/auth/auth.go index a75111b35..7b3511bdf 100644 --- a/core/auth/auth.go +++ b/core/auth/auth.go @@ -100,7 +100,7 @@ func WithAdminUser(ctx context.Context, ds model.DataStore) context.Context { } else { log.Error(ctx, "No admin user found!", err) } - u = &model.User{} + u = &model.User{IsAdmin: true, UserName: "admin"} } ctx = request.WithUsername(ctx, u.UserName)