From 25b32f97066bd5ea418f647ff0ce6b5e6e68607a Mon Sep 17 00:00:00 2001 From: Deluan Date: Wed, 22 Jul 2026 21:37:04 -0400 Subject: [PATCH] feat(server): serve artwork from persisted state with content-hash caching --- cmd/wire_gen.go | 42 +++--- server/imghttp/headers.go | 61 +++++++++ server/imghttp/headers_test.go | 166 ++++++++++++++++++++++++ server/jellyfin/api.go | 4 +- server/jellyfin/e2e/e2e_suite_test.go | 11 +- server/jellyfin/images.go | 25 +++- server/jellyfin/images_test.go | 43 +++++- server/public/handle_images.go | 12 +- server/public/handle_images_test.go | 55 ++++++++ server/public/public.go | 4 +- server/subsonic/api.go | 4 +- server/subsonic/e2e/e2e_suite_test.go | 12 +- server/subsonic/media_retrieval.go | 14 +- server/subsonic/media_retrieval_test.go | 65 +++++++++- 14 files changed, 456 insertions(+), 62 deletions(-) create mode 100644 server/imghttp/headers.go create mode 100644 server/imghttp/headers_test.go diff --git a/cmd/wire_gen.go b/cmd/wire_gen.go index 8f11b2efc..e7b961cf0 100644 --- a/cmd/wire_gen.go +++ b/cmd/wire_gen.go @@ -91,7 +91,14 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { sqlDB := db.Db() dataStore := persistence.New(sqlDB) fileCache := artwork.GetImageCache() + imageStore := artwork.ProvideImageStore() fFmpeg := ffmpeg.New() + service := artwork.NewService(dataStore, fileCache, imageStore, fFmpeg) + transcodingCache := stream.GetTranscodingCache() + mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) + share := core.NewShare(dataStore) + archiver := core.NewArchiver(mediaStreamer, dataStore, share) + players := core.NewPlayers(dataStore) broker := events.GetBroker() metricsMetrics := metrics.GetPrometheusInstance(dataStore) manager := plugins.GetManager(dataStore, broker, metricsMetrics) @@ -99,11 +106,6 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { matcherMatcher := matcher.New(dataStore) provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) - transcodingCache := stream.GetTranscodingCache() - mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) - share := core.NewShare(dataStore) - archiver := core.NewArchiver(mediaStreamer, dataStore, share) - players := core.NewPlayers(dataStore) cacheWarmer := artwork.NewCacheWarmer(artworkArtwork, fileCache) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) @@ -113,7 +115,7 @@ func CreateSubsonicAPIRouter(ctx context.Context) *subsonic.Router { lyricsLyrics := lyrics.NewLyrics(dataStore, manager) transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) sonicSonic := sonic.New(dataStore, manager, matcherMatcher) - router := subsonic.New(dataStore, artworkArtwork, mediaStreamer, archiver, players, provider, modelScanner, broker, playlistsPlaylists, playTracker, share, playbackServer, metricsMetrics, lyricsLyrics, transcodeDecider, sonicSonic) + router := subsonic.New(dataStore, service, mediaStreamer, archiver, players, provider, modelScanner, broker, playlistsPlaylists, playTracker, share, playbackServer, metricsMetrics, lyricsLyrics, transcodeDecider, sonicSonic) return router } @@ -121,24 +123,25 @@ func CreateJellyfinAPIRouter(ctx context.Context) *jellyfin.Router { sqlDB := db.Db() dataStore := persistence.New(sqlDB) fileCache := artwork.GetImageCache() + imageStore := artwork.ProvideImageStore() fFmpeg := ffmpeg.New() - broker := events.GetBroker() - metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) - matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) + service := artwork.NewService(dataStore, fileCache, imageStore, fFmpeg) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) transcodeDecider := stream.NewTranscodeDecider(dataStore, fFmpeg) players := core.NewPlayers(dataStore) + broker := events.GetBroker() + metricsMetrics := metrics.GetPrometheusInstance(dataStore) + manager := plugins.GetManager(dataStore, broker, metricsMetrics) playTracker := scrobbler.GetPlayTracker(dataStore, broker, manager) imageUploadService := core.NewImageUploadService() playlistsPlaylists := playlists.NewPlaylists(dataStore, imageUploadService) + agentsAgents := agents.GetAgents(dataStore, manager) + matcherMatcher := matcher.New(dataStore) + provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) sonicSonic := sonic.New(dataStore, manager, matcherMatcher) lyricsLyrics := lyrics.NewLyrics(dataStore, manager) - router := jellyfin.New(dataStore, artworkArtwork, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics, broker) + router := jellyfin.New(dataStore, service, mediaStreamer, transcodeDecider, players, playTracker, playlistsPlaylists, provider, sonicSonic, lyricsLyrics, broker) return router } @@ -146,19 +149,14 @@ func CreatePublicRouter() *public.Router { sqlDB := db.Db() dataStore := persistence.New(sqlDB) fileCache := artwork.GetImageCache() + imageStore := artwork.ProvideImageStore() fFmpeg := ffmpeg.New() - broker := events.GetBroker() - metricsMetrics := metrics.GetPrometheusInstance(dataStore) - manager := plugins.GetManager(dataStore, broker, metricsMetrics) - agentsAgents := agents.GetAgents(dataStore, manager) - matcherMatcher := matcher.New(dataStore) - provider := external.NewProvider(dataStore, agentsAgents, matcherMatcher) - artworkArtwork := artwork.NewArtwork(dataStore, fileCache, fFmpeg, provider) + service := artwork.NewService(dataStore, fileCache, imageStore, fFmpeg) transcodingCache := stream.GetTranscodingCache() mediaStreamer := stream.NewMediaStreamer(dataStore, fFmpeg, transcodingCache) share := core.NewShare(dataStore) archiver := core.NewArchiver(mediaStreamer, dataStore, share) - router := public.New(dataStore, artworkArtwork, mediaStreamer, share, archiver) + router := public.New(dataStore, service, mediaStreamer, share, archiver) return router } diff --git a/server/imghttp/headers.go b/server/imghttp/headers.go new file mode 100644 index 000000000..a5f3cf43e --- /dev/null +++ b/server/imghttp/headers.go @@ -0,0 +1,61 @@ +// Package imghttp holds the shared HTTP caching contract for artwork responses, so the +// subsonic, public, and jellyfin image handlers apply identical headers without importing +// each other. +package imghttp + +import ( + "net/http" + "strings" + + "github.com/navidrome/navidrome/core/artwork" +) + +// WriteImageHeaders applies the artwork caching contract and reports whether a 304 was written +// (in which case the caller must not write a body). requestedHash is the hash the client asserted +// (id suffix / JWT payload / jellyfin tag param), or "" when the request carried no hash. +func WriteImageHeaders(w http.ResponseWriter, r *http.Request, img *artwork.Image, requestedHash string) (wrote304 bool) { + h := w.Header() + // Placeholders are transient stand-ins for not-yet-resolved art: never cached, no validators. + if img.Placeholder { + h.Set("Cache-Control", "no-store") + return false + } + + h.Set("ETag", `"`+img.Hash+`"`) + if !img.LastUpdated.IsZero() { + h.Set("Last-Modified", img.LastUpdated.UTC().Format(http.TimeFormat)) + } + // Immutable only when the client asked for the exact current hash; bare/legacy/mismatched + // requests get cheap ETag revalidation instead, which fixes stale art after re-resolution. + if requestedHash != "" && requestedHash == img.Hash { + h.Set("Cache-Control", "public, max-age=31536000, immutable") + } else { + h.Set("Cache-Control", "public, no-cache") + } + + if ifNoneMatch(r.Header.Get("If-None-Match"), img.Hash) { + w.WriteHeader(http.StatusNotModified) + return true + } + return false +} + +// ifNoneMatch reports whether the If-None-Match header asserts the given hash, using weak +// comparison (RFC 9110): "*" matches any current representation and W/ prefixes are ignored. +func ifNoneMatch(header, hash string) bool { + header = strings.TrimSpace(header) + if header == "" { + return false + } + if header == "*" { + return true + } + for _, tag := range strings.Split(header, ",") { + tag = strings.TrimSpace(tag) + tag = strings.TrimPrefix(tag, "W/") + if strings.Trim(tag, `"`) == hash { + return true + } + } + return false +} diff --git a/server/imghttp/headers_test.go b/server/imghttp/headers_test.go new file mode 100644 index 000000000..6bd824fbe --- /dev/null +++ b/server/imghttp/headers_test.go @@ -0,0 +1,166 @@ +package imghttp_test + +import ( + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/navidrome/navidrome/core/artwork" + "github.com/navidrome/navidrome/server/imghttp" +) + +const testHash = "0123456789abcdef" + +var lastMod = time.Date(2024, 1, 2, 3, 4, 5, 0, time.UTC) + +func found() *artwork.Image { + return &artwork.Image{ + ReadCloser: io.NopCloser(strings.NewReader("IMG")), + Hash: testHash, + LastUpdated: lastMod, + } +} + +func placeholder() *artwork.Image { + return &artwork.Image{ReadCloser: io.NopCloser(strings.NewReader("PH")), Placeholder: true} +} + +func TestWriteImageHeaders(t *testing.T) { + tests := []struct { + name string + img *artwork.Image + requestedHash string + ifNoneMatch string + want304 bool + wantCache string + wantETag string + wantLastMod bool + }{ + { + name: "placeholder is never cached and carries no validators", + img: placeholder(), + wantCache: "no-store", + }, + { + name: "found with matching requested hash is immutable", + img: found(), + requestedHash: testHash, + wantCache: "public, max-age=31536000, immutable", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "found with bare id revalidates via no-cache", + img: found(), + wantCache: "public, no-cache", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "found with mismatched requested hash revalidates", + img: found(), + requestedHash: "ffffffffffffffff", + wantCache: "public, no-cache", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "If-None-Match matching the hash yields 304", + img: found(), + requestedHash: testHash, + ifNoneMatch: `"` + testHash + `"`, + want304: true, + wantCache: "public, max-age=31536000, immutable", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "weak If-None-Match matches (weak comparison)", + img: found(), + ifNoneMatch: `W/"` + testHash + `"`, + want304: true, + wantCache: "public, no-cache", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "If-None-Match with multiple values matches one", + img: found(), + ifNoneMatch: `"deadbeefdeadbeef", W/"` + testHash + `", "cafecafecafecafe"`, + want304: true, + wantCache: "public, no-cache", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "If-None-Match star matches any current representation", + img: found(), + ifNoneMatch: "*", + want304: true, + wantCache: "public, no-cache", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "non-matching If-None-Match serves body", + img: found(), + ifNoneMatch: `"deadbeefdeadbeef"`, + want304: false, + wantCache: "public, no-cache", + wantETag: `"` + testHash + `"`, + wantLastMod: true, + }, + { + name: "placeholder ignores If-None-Match and never 304s", + img: placeholder(), + ifNoneMatch: "*", + want304: false, + wantCache: "no-store", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", "/img", nil) + if tc.ifNoneMatch != "" { + r.Header.Set("If-None-Match", tc.ifNoneMatch) + } + + got := imghttp.WriteImageHeaders(w, r, tc.img, tc.requestedHash) + if got != tc.want304 { + t.Fatalf("wrote304 = %v, want %v", got, tc.want304) + } + + h := w.Header() + if got := h.Get("Cache-Control"); got != tc.wantCache { + t.Errorf("Cache-Control = %q, want %q", got, tc.wantCache) + } + if got := h.Get("ETag"); got != tc.wantETag { + t.Errorf("ETag = %q, want %q", got, tc.wantETag) + } + if tc.img.Placeholder { + if h.Get("ETag") != "" { + t.Errorf("placeholder must not set ETag, got %q", h.Get("ETag")) + } + if h.Get("Last-Modified") != "" { + t.Errorf("placeholder must not set Last-Modified, got %q", h.Get("Last-Modified")) + } + } + if hasLM := h.Get("Last-Modified") != ""; hasLM != tc.wantLastMod { + t.Errorf("has Last-Modified = %v, want %v", hasLM, tc.wantLastMod) + } + if tc.want304 { + if w.Code != http.StatusNotModified { + t.Errorf("status = %d, want 304", w.Code) + } + if w.Body.Len() != 0 { + t.Errorf("304 must have empty body, got %q", w.Body.String()) + } + } + }) + } +} diff --git a/server/jellyfin/api.go b/server/jellyfin/api.go index 1f46c08b4..f7a38feb4 100644 --- a/server/jellyfin/api.go +++ b/server/jellyfin/api.go @@ -30,7 +30,7 @@ import ( type Router struct { http.Handler ds model.DataStore - artwork artwork.Artwork + artwork artwork.Service streamer stream.MediaStreamer transcodeDecider stream.TranscodeDecider players core.Players @@ -46,7 +46,7 @@ type Router struct { serverIDVal string } -func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, +func New(ds model.DataStore, artwork artwork.Service, streamer stream.MediaStreamer, transcodeDecider stream.TranscodeDecider, players core.Players, scrobbler scrobbler.PlayTracker, playlists playlists.Playlists, provider external.Provider, sonicSvc sonic.Engine, lyricsSvc lyrics.Lyrics, broker events.Broker) *Router { diff --git a/server/jellyfin/e2e/e2e_suite_test.go b/server/jellyfin/e2e/e2e_suite_test.go index 31d98d1d9..1986a95ec 100644 --- a/server/jellyfin/e2e/e2e_suite_test.go +++ b/server/jellyfin/e2e/e2e_suite_test.go @@ -33,7 +33,6 @@ import ( "strings" "testing" "testing/fstest" - "time" "github.com/navidrome/navidrome/conf" "github.com/navidrome/navidrome/conf/configtest" @@ -406,18 +405,18 @@ type spyArtwork struct { data []byte } -func (s *spyArtwork) Get(context.Context, model.ArtworkID, int, bool) (io.ReadCloser, time.Time, error) { - return nil, time.Time{}, model.ErrNotFound +func (s *spyArtwork) Get(context.Context, model.ArtworkID, int, bool) (*artwork.Image, error) { + return nil, model.ErrNotFound } -func (s *spyArtwork) GetOrPlaceholder(c context.Context, id string, _ int, _ bool) (io.ReadCloser, time.Time, error) { +func (s *spyArtwork) GetOrPlaceholder(c context.Context, id string, _ int, _ bool) (*artwork.Image, error) { s.lastID = id s.lastCtx = c d := s.data if d == nil { d = []byte("IMG") } - return io.NopCloser(bytes.NewReader(d)), time.Time{}, nil + return &artwork.Image{ReadCloser: io.NopCloser(bytes.NewReader(d))}, nil } -var _ artwork.Artwork = &spyArtwork{} +var _ artwork.Service = &spyArtwork{} diff --git a/server/jellyfin/images.go b/server/jellyfin/images.go index 0ec34f491..35042e9c4 100644 --- a/server/jellyfin/images.go +++ b/server/jellyfin/images.go @@ -20,6 +20,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" + "github.com/navidrome/navidrome/server/imghttp" "github.com/navidrome/navidrome/server/jellyfin/dto" _ "golang.org/x/image/webp" ) @@ -32,7 +33,7 @@ func (api *Router) getItemImage(w http.ResponseWriter, r *http.Request) { size, _ := strconv.Atoi(r.URL.Query().Get("maxwidth")) artID := api.resolveArtworkID(ctx, itemId) - reader, _, err := api.artwork.GetOrPlaceholder(ctx, artID, size, false) + img, err := api.artwork.GetOrPlaceholder(ctx, artID, size, false) switch { case errors.Is(err, context.Canceled): return @@ -41,9 +42,27 @@ func (api *Router) getItemImage(w http.ResponseWriter, r *http.Request) { http.Error(w, "Not Found", http.StatusNotFound) return } - defer reader.Close() + defer img.Close() + if imghttp.WriteImageHeaders(w, r, img, hashFromTag(r)) { + return + } // Leave Content-Type unset so net/http sniffs it (covers may be PNG/WebP/JPEG). - _, _ = io.Copy(w, reader) + _, _ = io.Copy(w, img) +} + +// hashFromTag returns the ?tag query param when it is exactly a 16-char lowercase-hex content +// hash (what Finamp/Jellyfin clients append), so a matching request can be served immutable. +func hashFromTag(r *http.Request) string { + tag := r.URL.Query().Get("tag") + if len(tag) != 16 { + return "" + } + for _, c := range tag { + if !(c >= '0' && c <= '9' || c >= 'a' && c <= 'f') { + return "" + } + } + return tag } // resolveArtworkID maps a Jellyfin item id to a Navidrome ArtworkID, probing diff --git a/server/jellyfin/images_test.go b/server/jellyfin/images_test.go index e435b99fd..65285e3c5 100644 --- a/server/jellyfin/images_test.go +++ b/server/jellyfin/images_test.go @@ -29,20 +29,25 @@ import ( ) type fakeArtwork struct { - artwork.Artwork + artwork.Service recvId string recvCtx context.Context data []byte + hash string } -func (f *fakeArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) { +func (f *fakeArtwork) GetOrPlaceholder(ctx context.Context, id string, size int, square bool) (*artwork.Image, error) { f.recvId = id f.recvCtx = ctx data := f.data if data == nil { data = []byte("IMG") } - return io.NopCloser(bytes.NewReader(data)), time.Now(), nil + return &artwork.Image{ + ReadCloser: io.NopCloser(bytes.NewReader(data)), + Hash: f.hash, + LastUpdated: time.Now(), + }, nil } func newImageRequest(itemId string) (*httptest.ResponseRecorder, *http.Request) { @@ -115,6 +120,38 @@ var _ = Describe("Images", func() { Expect(ok).To(BeTrue()) Expect(u.IsAdmin).To(BeTrue()) }) + + It("serves immutable when the tag param asserts the current hash", func() { + const hash = "0123456789abcdef" + ds := &tests.MockDataStore{} + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + fa := &fakeArtwork{hash: hash} + api := &Router{ds: ds, artwork: fa} + + w, r := newImageRequest(dto.EncodeID("a1")) + q := r.URL.Query() + q.Set("tag", hash) + r.URL.RawQuery = q.Encode() + api.getItemImage(w, r) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Cache-Control")).To(Equal("public, max-age=31536000, immutable")) + Expect(w.Header().Get("ETag")).To(Equal(`"` + hash + `"`)) + }) + + It("revalidates via no-cache when no tag is provided", func() { + const hash = "0123456789abcdef" + ds := &tests.MockDataStore{} + ds.Album(context.Background()).(*tests.MockAlbumRepo).SetData(model.Albums{{ID: "a1", Name: "One"}}) + fa := &fakeArtwork{hash: hash} + api := &Router{ds: ds, artwork: fa} + + w, r := newImageRequest(dto.EncodeID("a1")) + api.getItemImage(w, r) + + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Cache-Control")).To(Equal("public, no-cache")) + }) }) // Real image fixtures: postItemImage validates uploads by decoding them. diff --git a/server/public/handle_images.go b/server/public/handle_images.go index 50f9238e5..9bcaa5933 100644 --- a/server/public/handle_images.go +++ b/server/public/handle_images.go @@ -11,6 +11,7 @@ import ( "github.com/navidrome/navidrome/core/auth" "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" + "github.com/navidrome/navidrome/server/imghttp" "github.com/navidrome/navidrome/utils/req" ) @@ -40,7 +41,7 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) { size := p.IntOr("size", 0) square := p.BoolOr("square", false) - imgReader, lastUpdate, err := pub.artwork.Get(ctx, artId, size, square) + img, err := pub.artwork.Get(ctx, artId, size, square) switch { case errors.Is(err, context.Canceled): return @@ -58,10 +59,11 @@ func (pub *Router) handleImages(w http.ResponseWriter, r *http.Request) { return } - defer imgReader.Close() - w.Header().Set("Cache-Control", "public, max-age=315360000") - w.Header().Set("Last-Modified", lastUpdate.Format(http.TimeFormat)) - cnt, err := io.Copy(w, imgReader) + defer img.Close() + if imghttp.WriteImageHeaders(w, r, img, artId.Hash) { + return + } + cnt, err := io.Copy(w, img) if err != nil { log.Warn(ctx, "Error sending image", "count", cnt, err) } diff --git a/server/public/handle_images_test.go b/server/public/handle_images_test.go index 6895241f6..edcf9d8c0 100644 --- a/server/public/handle_images_test.go +++ b/server/public/handle_images_test.go @@ -1,8 +1,17 @@ package public import ( + "bytes" + "context" + "io" + "net/http" + "net/http/httptest" + "net/url" + "github.com/go-chi/jwtauth/v5" + "github.com/navidrome/navidrome/core/artwork" "github.com/navidrome/navidrome/core/auth" + "github.com/navidrome/navidrome/model" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -23,3 +32,49 @@ var _ = Describe("decodeArtworkID", func() { Expect(err).To(HaveOccurred()) }) }) + +type fakeArtwork struct { + artwork.Service + img *artwork.Image + err error +} + +func (f *fakeArtwork) Get(context.Context, model.ArtworkID, int, bool) (*artwork.Image, error) { + return f.img, f.err +} + +var _ = Describe("handleImages", func() { + var w *httptest.ResponseRecorder + + newImageRequest := func(claimID string) *http.Request { + auth.TokenAuth = jwtauth.New("HS256", []byte("super secret"), nil) + token, _ := auth.CreatePublicToken(auth.Claims{ID: claimID}) + return httptest.NewRequest("GET", "/img?:id="+url.QueryEscape(token), nil) + } + + BeforeEach(func() { + w = httptest.NewRecorder() + }) + + It("returns 404 when the artwork is unavailable", func() { + pub := &Router{artwork: &fakeArtwork{err: artwork.ErrUnavailable}} + pub.handleImages(w, newImageRequest("al-1")) + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) + + It("returns 404 when the artwork is not found", func() { + pub := &Router{artwork: &fakeArtwork{err: model.ErrNotFound}} + pub.handleImages(w, newImageRequest("al-1")) + Expect(w.Code).To(Equal(http.StatusNotFound)) + }) + + It("serves the image immutable when the token asserts the current hash", func() { + const hash = "0123456789abcdef" + img := &artwork.Image{ReadCloser: io.NopCloser(bytes.NewReader([]byte("IMG"))), Hash: hash} + pub := &Router{artwork: &fakeArtwork{img: img}} + pub.handleImages(w, newImageRequest("al-1_"+hash)) + Expect(w.Code).To(Equal(http.StatusOK)) + Expect(w.Header().Get("Cache-Control")).To(Equal("public, max-age=31536000, immutable")) + Expect(w.Header().Get("ETag")).To(Equal(`"` + hash + `"`)) + }) +}) diff --git a/server/public/public.go b/server/public/public.go index 18867e1c4..35155ec59 100644 --- a/server/public/public.go +++ b/server/public/public.go @@ -18,7 +18,7 @@ import ( type Router struct { http.Handler - artwork artwork.Artwork + artwork artwork.Service streamer stream.MediaStreamer archiver core.Archiver share core.Share @@ -26,7 +26,7 @@ type Router struct { ds model.DataStore } -func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, share core.Share, archiver core.Archiver) *Router { +func New(ds model.DataStore, artwork artwork.Service, streamer stream.MediaStreamer, share core.Share, archiver core.Archiver) *Router { p := &Router{ds: ds, artwork: artwork, streamer: streamer, share: share, archiver: archiver} shareRoot := path.Join(conf.Server.BasePath, consts.URLPathPublic) p.assetsHandler = http.StripPrefix(shareRoot, http.FileServer(http.FS(ui.BuildAssets()))) diff --git a/server/subsonic/api.go b/server/subsonic/api.go index 82e404228..d57da7452 100644 --- a/server/subsonic/api.go +++ b/server/subsonic/api.go @@ -40,7 +40,7 @@ type handlerRaw = func(http.ResponseWriter, *http.Request) (*responses.Subsonic, type Router struct { http.Handler ds model.DataStore - artwork artwork.Artwork + artwork artwork.Service streamer stream.MediaStreamer archiver core.Archiver players core.Players @@ -57,7 +57,7 @@ type Router struct { sonic *sonicsvc.Sonic } -func New(ds model.DataStore, artwork artwork.Artwork, streamer stream.MediaStreamer, archiver core.Archiver, +func New(ds model.DataStore, artwork artwork.Service, streamer stream.MediaStreamer, archiver core.Archiver, players core.Players, provider external.Provider, scanner model.Scanner, broker events.Broker, playlists playlistsvc.Playlists, scrobbler scrobbler.PlayTracker, share core.Share, playback playback.PlaybackServer, metrics metrics.Metrics, lyrics lyricssvc.Lyrics, transcodeDecision stream.TranscodeDecider, diff --git a/server/subsonic/e2e/e2e_suite_test.go b/server/subsonic/e2e/e2e_suite_test.go index 98ad6d1ee..bd4651897 100644 --- a/server/subsonic/e2e/e2e_suite_test.go +++ b/server/subsonic/e2e/e2e_suite_test.go @@ -308,15 +308,15 @@ func parseJSONResponse(w *httptest.ResponseRecorder) *responses.Subsonic { // --- Noop stub implementations for Router dependencies --- -// noopArtwork implements artwork.Artwork +// noopArtwork implements artwork.Service type noopArtwork struct{} -func (n noopArtwork) Get(context.Context, model.ArtworkID, int, bool) (io.ReadCloser, time.Time, error) { - return nil, time.Time{}, model.ErrNotFound +func (n noopArtwork) Get(context.Context, model.ArtworkID, int, bool) (*artwork.Image, error) { + return nil, model.ErrNotFound } -func (n noopArtwork) GetOrPlaceholder(_ context.Context, _ string, _ int, _ bool) (io.ReadCloser, time.Time, error) { - return io.NopCloser(io.LimitReader(nil, 0)), time.Time{}, nil +func (n noopArtwork) GetOrPlaceholder(_ context.Context, _ string, _ int, _ bool) (*artwork.Image, error) { + return &artwork.Image{ReadCloser: io.NopCloser(io.LimitReader(nil, 0))}, nil } // noopArchiver implements core.Archiver @@ -371,7 +371,7 @@ func (n noopProvider) AlbumImage(context.Context, string) (*url.URL, error) { // Compile-time interface checks var ( - _ artwork.Artwork = noopArtwork{} + _ artwork.Service = noopArtwork{} _ core.Archiver = noopArchiver{} _ external.Provider = noopProvider{} ) diff --git a/server/subsonic/media_retrieval.go b/server/subsonic/media_retrieval.go index 089a1fdda..30124c5f8 100644 --- a/server/subsonic/media_retrieval.go +++ b/server/subsonic/media_retrieval.go @@ -13,6 +13,7 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/resources" + "github.com/navidrome/navidrome/server/imghttp" "github.com/navidrome/navidrome/server/subsonic/responses" "github.com/navidrome/navidrome/utils/gravatar" "github.com/navidrome/navidrome/utils/req" @@ -66,7 +67,7 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons size := p.IntOr("size", 0) square := p.BoolOr("square", false) - imgReader, lastUpdate, err := api.artwork.GetOrPlaceholder(ctx, id, size, square) + img, err := api.artwork.GetOrPlaceholder(ctx, id, size, square) switch { case errors.Is(err, context.Canceled): return nil, nil @@ -77,12 +78,13 @@ func (api *Router) GetCoverArt(w http.ResponseWriter, r *http.Request) (*respons log.Error(r, "Error retrieving coverArt", "id", id, err) return nil, err } + defer img.Close() - defer imgReader.Close() - w.Header().Set("cache-control", "public, max-age=315360000") - w.Header().Set("last-modified", lastUpdate.Format(http.TimeFormat)) - - cnt, err := io.Copy(w, imgReader) + artID, _ := model.ParseArtworkID(id) + if imghttp.WriteImageHeaders(w, r, img, artID.Hash) { + return nil, nil + } + cnt, err := io.Copy(w, img) if err != nil { log.Warn(ctx, "Error sending image", "count", cnt, err) } diff --git a/server/subsonic/media_retrieval_test.go b/server/subsonic/media_retrieval_test.go index 9331dfbe4..db15d5bfd 100644 --- a/server/subsonic/media_retrieval_test.go +++ b/server/subsonic/media_retrieval_test.go @@ -108,6 +108,53 @@ var _ = Describe("MediaRetrievalController", func() { Expect(w.Body.String()).To(BeEmpty()) }) }) + + Describe("caching headers", func() { + const hash = "0123456789abcdef" + + It("sets an ETag and no-cache for a bare id", func() { + artwork.hash = hash + r := newGetRequest("id=al-34") + _, err := router.GetCoverArt(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Header().Get("ETag")).To(Equal(`"` + hash + `"`)) + Expect(w.Header().Get("Cache-Control")).To(Equal("public, no-cache")) + Expect(w.Body.String()).To(Equal(artwork.data)) + }) + + It("marks the response immutable when the id asserts the current hash", func() { + artwork.hash = hash + r := newGetRequest("id=al-34_" + hash) + _, err := router.GetCoverArt(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Header().Get("Cache-Control")).To(Equal("public, max-age=31536000, immutable")) + }) + + It("returns 304 with no body when If-None-Match matches", func() { + artwork.hash = hash + r := newGetRequest("id=al-34") + r.Header.Set("If-None-Match", `"`+hash+`"`) + _, err := router.GetCoverArt(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(304)) + Expect(w.Body.Len()).To(BeZero()) + }) + + It("never caches a placeholder", func() { + artwork.placeholder = true + r := newGetRequest("id=al-missing") + _, err := router.GetCoverArt(w, r) + + Expect(err).ToNot(HaveOccurred()) + Expect(w.Code).To(Equal(200)) + Expect(w.Header().Get("Cache-Control")).To(Equal("no-store")) + Expect(w.Header().Get("ETag")).To(BeEmpty()) + Expect(w.Body.String()).To(Equal(artwork.data)) + }) + }) }) Describe("GetLyrics", func() { @@ -186,8 +233,11 @@ var _ = Describe("MediaRetrievalController", func() { }) type fakeArtwork struct { - artwork.Artwork + artwork.Service data string + hash string + lastUpdated time.Time + placeholder bool err error ctxCancelFunc func() recvId string @@ -195,18 +245,23 @@ type fakeArtwork struct { recvSquare bool } -func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, square bool) (io.ReadCloser, time.Time, error) { +func (c *fakeArtwork) GetOrPlaceholder(_ context.Context, id string, size int, square bool) (*artwork.Image, error) { if c.err != nil { - return nil, time.Time{}, c.err + return nil, c.err } c.recvId = id c.recvSize = size c.recvSquare = square if c.ctxCancelFunc != nil { c.ctxCancelFunc() - return nil, time.Time{}, context.Canceled + return nil, context.Canceled } - return io.NopCloser(bytes.NewReader([]byte(c.data))), time.Time{}, nil + return &artwork.Image{ + ReadCloser: io.NopCloser(bytes.NewReader([]byte(c.data))), + Hash: c.hash, + LastUpdated: c.lastUpdated, + Placeholder: c.placeholder, + }, nil } type mockedMediaFile struct {