Fix tests and clean up code a bit

core/artwork/artwork.go

@@ -3,18 +3,14 @@ package artwork
 import (
 	"context"
 	"errors"
-	"fmt"
 	_ "image/gif"
 	"io"
 	"time"
 
-	"github.com/navidrome/navidrome/conf"
-	"github.com/navidrome/navidrome/consts"
 	"github.com/navidrome/navidrome/core/ffmpeg"
 	"github.com/navidrome/navidrome/log"
 	"github.com/navidrome/navidrome/model"
 	"github.com/navidrome/navidrome/utils/cache"
-	"github.com/navidrome/navidrome/utils/singleton"
 	_ "golang.org/x/image/webp"
 )
 
@@ -79,36 +75,3 @@ func (a *artwork) getArtworkReader(ctx context.Context, artID model.ArtworkID, s
 	}
 	return artReader, err
 }
-
-type cacheItem struct {
-	artID      model.ArtworkID
-	size       int
-	lastUpdate time.Time
-}
-
-func (i *cacheItem) Key() string {
-	return fmt.Sprintf(
-		"%s.%d.%d.%d.%t",
-		i.artID.ID,
-		i.lastUpdate.UnixMilli(),
-		i.size,
-		conf.Server.CoverJpegQuality,
-		conf.Server.EnableMediaFileCoverArt,
-	)
-}
-
-type imageCache struct {
-	cache.FileCache
-}
-
-func GetImageCache() cache.FileCache {
-	return singleton.GetInstance(func() *imageCache {
-		return &imageCache{
-			FileCache: cache.NewFileCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems,
-				func(ctx context.Context, arg cache.Item) (io.Reader, error) {
-					r, _, err := arg.(artworkReader).Reader(ctx)
-					return r, err
-				}),
-		}
-	})
-}

core/artwork/artwork_internal_test.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"image"
 	"io"
-	"testing"
 
 	"github.com/navidrome/navidrome/conf"
 	"github.com/navidrome/navidrome/conf/configtest"
@@ -17,13 +16,6 @@ import (
 	. "github.com/onsi/gomega"
 )
 
-func TestArtwork(t *testing.T) {
-	tests.Init(t, false)
-	log.SetLevel(log.LevelFatal)
-	RegisterFailHandler(Fail)
-	RunSpecs(t, "Artwork Suite")
-}
-
 var _ = Describe("Artwork", func() {
 	var aw *artwork
 	var ds model.DataStore
@@ -54,20 +46,11 @@ var _ = Describe("Artwork", func() {
 		aw = NewArtwork(ds, cache, ffmpeg).(*artwork)
 	})
 
-	Context("Empty ID", func() {
+	Describe("albumArtworkReader", func() {
-		It("returns placeholder if album is not in the DB", func() {
-			_, path, err := aw.get(context.Background(), model.ArtworkID{}, 0)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(path).To(Equal(consts.PlaceholderAlbumArt))
-		})
-	})
-
-	Context("Albums", func() {
 		Context("ID not found", func() {
-			It("returns placeholder if album is not in the DB", func() {
+			It("returns ErrNotFound if album is not in the DB", func() {
-				_, path, err := aw.get(context.Background(), model.MustParseArtworkID("al-NOT_FOUND-0"), 0)
+				_, err := newAlbumArtworkReader(ctx, aw, model.MustParseArtworkID("al-NOT_FOUND"))
-				Expect(err).ToNot(HaveOccurred())
+				Expect(err).To(MatchError(model.ErrNotFound))
-				Expect(path).To(Equal(consts.PlaceholderAlbumArt))
 			})
 		})
 		Context("Embed images", func() {
@@ -78,13 +61,17 @@ var _ = Describe("Artwork", func() {
 				})
 			})
 			It("returns embed cover", func() {
-				_, path, err := aw.get(context.Background(), alOnlyEmbed.CoverArtID(), 0)
+				aw, err := newAlbumArtworkReader(ctx, aw, alOnlyEmbed.CoverArtID())
+				Expect(err).ToNot(HaveOccurred())
+				_, path, err := aw.Reader(ctx)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(path).To(Equal("tests/fixtures/test.mp3"))
 			})
 			It("returns placeholder if embed path is not available", func() {
 				ffmpeg.Error = errors.New("not available")
-				_, path, err := aw.get(context.Background(), alEmbedNotFound.CoverArtID(), 0)
+				aw, err := newAlbumArtworkReader(ctx, aw, alEmbedNotFound.CoverArtID())
+				Expect(err).ToNot(HaveOccurred())
+				_, path, err := aw.Reader(ctx)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(path).To(Equal(consts.PlaceholderAlbumArt))
 			})
@@ -93,15 +80,20 @@ var _ = Describe("Artwork", func() {
 			BeforeEach(func() {
 				ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{
 					alOnlyExternal,
+					alExternalNotFound,
 				})
 			})
 			It("returns external cover", func() {
-				_, path, err := aw.get(context.Background(), alOnlyExternal.CoverArtID(), 0)
+				aw, err := newAlbumArtworkReader(ctx, aw, alOnlyExternal.CoverArtID())
+				Expect(err).ToNot(HaveOccurred())
+				_, path, err := aw.Reader(ctx)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(path).To(Equal("tests/fixtures/front.png"))
 			})
 			It("returns placeholder if external file is not available", func() {
-				_, path, err := aw.get(context.Background(), alExternalNotFound.CoverArtID(), 0)
+				aw, err := newAlbumArtworkReader(ctx, aw, alExternalNotFound.CoverArtID())
+				Expect(err).ToNot(HaveOccurred())
+				_, path, err := aw.Reader(ctx)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(path).To(Equal(consts.PlaceholderAlbumArt))
 			})
@@ -115,7 +107,9 @@ var _ = Describe("Artwork", func() {
 			DescribeTable("CoverArtPriority",
 				func(priority string, expected string) {
 					conf.Server.CoverArtPriority = priority
-					_, path, err := aw.get(context.Background(), alMultipleCovers.CoverArtID(), 0)
+					aw, err := newAlbumArtworkReader(ctx, aw, alMultipleCovers.CoverArtID())
+					Expect(err).ToNot(HaveOccurred())
+					_, path, err := aw.Reader(ctx)
 					Expect(err).ToNot(HaveOccurred())
 					Expect(path).To(Equal(expected))
 				},
@@ -125,12 +119,11 @@ var _ = Describe("Artwork", func() {
 			)
 		})
 	})
-	Context("MediaFiles", func() {
+	Describe("mediafileArtworkReader", func() {
 		Context("ID not found", func() {
-			It("returns placeholder if album is not in the DB", func() {
+			It("returns ErrNotFound if mediafile is not in the DB", func() {
-				_, path, err := aw.get(context.Background(), model.MustParseArtworkID("mf-NOT_FOUND-0"), 0)
+				_, err := newAlbumArtworkReader(ctx, aw, alMultipleCovers.CoverArtID())
-				Expect(err).ToNot(HaveOccurred())
+				Expect(err).To(MatchError(model.ErrNotFound))
-				Expect(path).To(Equal(consts.PlaceholderAlbumArt))
 			})
 		})
 		Context("Embed images", func() {
@@ -146,30 +139,38 @@ var _ = Describe("Artwork", func() {
 				})
 			})
 			It("returns embed cover", func() {
-				_, path, err := aw.get(context.Background(), mfWithEmbed.CoverArtID(), 0)
+				aw, err := newMediafileArtworkReader(ctx, aw, mfWithEmbed.CoverArtID())
+				Expect(err).ToNot(HaveOccurred())
+				_, path, err := aw.Reader(ctx)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(path).To(Equal("tests/fixtures/test.mp3"))
 			})
 			It("returns embed cover if successfully extracted by ffmpeg", func() {
-				r, path, err := aw.get(context.Background(), mfCorruptedCover.CoverArtID(), 0)
+				aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID())
+				Expect(err).ToNot(HaveOccurred())
+				r, path, err := aw.Reader(ctx)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(io.ReadAll(r)).To(Equal([]byte("content from ffmpeg")))
 				Expect(path).To(Equal("tests/fixtures/test.ogg"))
 			})
 			It("returns album cover if cannot read embed artwork", func() {
 				ffmpeg.Error = errors.New("not available")
-				_, path, err := aw.get(context.Background(), mfCorruptedCover.CoverArtID(), 0)
+				aw, err := newMediafileArtworkReader(ctx, aw, mfCorruptedCover.CoverArtID())
 				Expect(err).ToNot(HaveOccurred())
-				Expect(path).To(Equal("tests/fixtures/front.png"))
+				_, path, err := aw.Reader(ctx)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(path).To(Equal("al-444"))
 			})
 			It("returns album cover if media file has no cover art", func() {
-				_, path, err := aw.get(context.Background(), mfWithoutEmbed.CoverArtID(), 0)
+				aw, err := newMediafileArtworkReader(ctx, aw, model.MustParseArtworkID("mf-"+mfWithoutEmbed.ID))
 				Expect(err).ToNot(HaveOccurred())
-				Expect(path).To(Equal("tests/fixtures/front.png"))
+				_, path, err := aw.Reader(ctx)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(path).To(Equal("al-444"))
 			})
 		})
 	})
-	Context("Resize", func() {
+	Describe("resizedArtworkReader", func() {
 		BeforeEach(func() {
 			ds.Album(ctx).(*tests.MockAlbumRepo).SetData(model.Albums{
 				alMultipleCovers,
@@ -177,7 +178,7 @@ var _ = Describe("Artwork", func() {
 		})
 		It("returns a PNG if original image is a PNG", func() {
 			conf.Server.CoverArtPriority = "front.png"
-			r, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID().String(), 300)
+			r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID().String(), 300)
 			Expect(err).ToNot(HaveOccurred())
 
 			br, format, err := asImageReader(r)
@@ -191,7 +192,7 @@ var _ = Describe("Artwork", func() {
 		})
 		It("returns a JPEG if original image is not a PNG", func() {
 			conf.Server.CoverArtPriority = "cover.jpg"
-			r, _, err := aw.get(context.Background(), alMultipleCovers.CoverArtID(), 200)
+			r, _, err := aw.Get(context.Background(), alMultipleCovers.CoverArtID().String(), 200)
 			Expect(err).ToNot(HaveOccurred())
 
 			br, format, err := asImageReader(r)

core/artwork/artwork_suite_test.go

@@ -0,0 +1,17 @@
+package artwork
+
+import (
+	"testing"
+
+	"github.com/navidrome/navidrome/log"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestArtwork(t *testing.T) {
+	tests.Init(t, false)
+	log.SetLevel(log.LevelFatal)
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Artwork Suite")
+}

core/artwork/artwork_test.go

@@ -0,0 +1,47 @@
+package artwork_test
+
+import (
+	"context"
+	"io"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/conf/configtest"
+	"github.com/navidrome/navidrome/consts"
+	"github.com/navidrome/navidrome/core/artwork"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/resources"
+	"github.com/navidrome/navidrome/tests"
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+var _ = Describe("Artwork", func() {
+	var aw artwork.Artwork
+	var ds model.DataStore
+	var ffmpeg *tests.MockFFmpeg
+
+	BeforeEach(func() {
+		DeferCleanup(configtest.SetupConfig())
+		conf.Server.ImageCacheSize = "0" // Disable cache
+		cache := artwork.GetImageCache()
+		ffmpeg = tests.NewMockFFmpeg("content from ffmpeg")
+		aw = artwork.NewArtwork(ds, cache, ffmpeg)
+	})
+
+	Context("Empty ID", func() {
+		It("returns placeholder if album is not in the DB", func() {
+			r, _, err := aw.Get(context.Background(), "", 0)
+			Expect(err).ToNot(HaveOccurred())
+
+			ph, err := resources.FS().Open(consts.PlaceholderAlbumArt)
+			Expect(err).ToNot(HaveOccurred())
+			phBytes, err := io.ReadAll(ph)
+			Expect(err).ToNot(HaveOccurred())
+
+			result, err := io.ReadAll(r)
+			Expect(err).ToNot(HaveOccurred())
+
+			Expect(result).To(Equal(phBytes))
+		})
+	})
+})

core/artwork/image_cache.go

@@ -0,0 +1,47 @@
+package artwork
+
+import (
+	"context"
+	"fmt"
+	"io"
+	"time"
+
+	"github.com/navidrome/navidrome/conf"
+	"github.com/navidrome/navidrome/consts"
+	"github.com/navidrome/navidrome/model"
+	"github.com/navidrome/navidrome/utils/cache"
+	"github.com/navidrome/navidrome/utils/singleton"
+)
+
+type cacheKey struct {
+	artID      model.ArtworkID
+	size       int
+	lastUpdate time.Time
+}
+
+func (k *cacheKey) Key() string {
+	return fmt.Sprintf(
+		"%s.%d.%d.%d.%t",
+		k.artID.ID,
+		k.lastUpdate.UnixMilli(),
+		k.size,
+		conf.Server.CoverJpegQuality,
+		conf.Server.EnableMediaFileCoverArt,
+	)
+}
+
+type imageCache struct {
+	cache.FileCache
+}
+
+func GetImageCache() cache.FileCache {
+	return singleton.GetInstance(func() *imageCache {
+		return &imageCache{
+			FileCache: cache.NewFileCache("Image", conf.Server.ImageCacheSize, consts.ImageCacheDir, consts.DefaultImageCacheMaxItems,
+				func(ctx context.Context, arg cache.Item) (io.Reader, error) {
+					r, _, err := arg.(artworkReader).Reader(ctx)
+					return r, err
+				}),
+		}
+	})
+}

core/artwork/reader_album.go

@@ -12,7 +12,7 @@ import (
 )
 
 type albumArtworkReader struct {
-	cacheItem
+	cacheKey
 	a     *artwork
 	album model.Album
 }
@@ -26,8 +26,8 @@ func newAlbumArtworkReader(ctx context.Context, artwork *artwork, artID model.Ar
 		a:     artwork,
 		album: *al,
 	}
-	a.cacheItem.artID = artID
+	a.cacheKey.artID = artID
-	a.cacheItem.lastUpdate = al.UpdatedAt
+	a.cacheKey.lastUpdate = al.UpdatedAt
 	return a, nil
 }
 

core/artwork/reader_mediafile.go

@@ -9,7 +9,7 @@ import (
 )
 
 type mediafileArtworkReader struct {
-	cacheItem
+	cacheKey
 	a         *artwork
 	mediafile model.MediaFile
 	album     model.Album
@@ -29,8 +29,8 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode
 		mediafile: *mf,
 		album:     *al,
 	}
-	a.cacheItem.artID = artID
+	a.cacheKey.artID = artID
-	a.cacheItem.lastUpdate = a.LastUpdated()
+	a.cacheKey.lastUpdate = a.LastUpdated()
 	return a, nil
 }
 

core/artwork/reader_resized.go

@@ -20,21 +20,21 @@ import (
 )
 
 type resizedArtworkReader struct {
-	cacheItem
+	cacheKey
 	a *artwork
 }
 
 func resizedFromOriginal(ctx context.Context, a *artwork, artID model.ArtworkID, size int) (*resizedArtworkReader, error) {
 	r := &resizedArtworkReader{a: a}
-	r.cacheItem.artID = artID
+	r.cacheKey.artID = artID
-	r.cacheItem.size = size
+	r.cacheKey.size = size
 
 	// Get lastUpdated from original artwork
 	original, err := a.getArtworkReader(ctx, artID, 0)
 	if err != nil {
 		return nil, err
 	}
-	r.cacheItem.lastUpdate = original.LastUpdated()
+	r.cacheKey.lastUpdate = original.LastUpdated()
 	return r, nil
 }
 

core/wire_providers.go

@@ -9,17 +9,17 @@ import (
 )
 
 var Set = wire.NewSet(
-	artwork.NewArtwork,
 	NewMediaStreamer,
 	GetTranscodingCache,
-	artwork.GetImageCache,
 	NewArchiver,
 	NewExternalMetadata,
 	NewPlayers,
+	NewShare,
+	NewPlaylists,
 	agents.New,
 	ffmpeg.New,
 	scrobbler.GetPlayTracker,
-	NewShare,
+	artwork.NewArtwork,
-	NewPlaylists,
+	artwork.GetImageCache,
 	artwork.NewCacheWarmer,
 )

model/artwork_id.go

@@ -19,6 +19,9 @@ type ArtworkID struct {
 }
 
 func (id ArtworkID) String() string {
+	if id.ID == "" {
+		return ""
+	}
 	return fmt.Sprintf("%s-%s", id.Kind.prefix, id.ID)
 }