mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
* fix(artwork): return imagesUpdatedAt in LastUpdated when cover art changes When cover art (cover.jpg) is updated in an album folder, the HTTP Last-Modified header was incorrectly returning album.UpdatedAt (which only tracks media file changes) instead of imagesUpdatedAt (which tracks cover art changes). This caused browsers to use their cached cover art because the Last-Modified header didn't change, even though the actual cover art image data was new (due to cache key changing based on imagesUpdatedAt). The fix ensures LastUpdated() returns a.lastUpdate (which is the max of album.UpdatedAt and imagesUpdatedAt) instead of always returning album.UpdatedAt. Fixes navidrome/navidrome#5377 * refactor tests Signed-off-by: Deluan <deluan@navidrome.org> * fix(artwork): return imagesUpdatedAt in disc LastUpdated The discArtworkReader had the same bug as albumArtworkReader (fixed in 9a741859f): LastUpdated() returned album.UpdatedAt while Key() used the max of album.UpdatedAt and ImagesUpdatedAt. This mismatch caused browsers to keep stale disc cover art in cache when only the image file changed. Also strengthen the album LastUpdated tests and add matching tests for the disc reader. The tests use DescribeTable and were verified to fail when the fix is reverted. --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: Deluan <deluan@navidrome.org>
259 lines
7.3 KiB
Go
259 lines
7.3 KiB
Go
package artwork
|
|
|
|
import (
|
|
"context"
|
|
"crypto/md5"
|
|
"fmt"
|
|
"io"
|
|
"os"
|
|
"path/filepath"
|
|
"strconv"
|
|
"strings"
|
|
"time"
|
|
|
|
"github.com/Masterminds/squirrel"
|
|
"github.com/navidrome/navidrome/conf"
|
|
"github.com/navidrome/navidrome/core"
|
|
"github.com/navidrome/navidrome/core/ffmpeg"
|
|
"github.com/navidrome/navidrome/log"
|
|
"github.com/navidrome/navidrome/model"
|
|
)
|
|
|
|
type discArtworkReader struct {
|
|
cacheKey
|
|
a *artwork
|
|
album model.Album
|
|
discNumber int
|
|
imgFiles []string
|
|
discFolders map[string]bool
|
|
isMultiFolder bool
|
|
firstTrackPath string
|
|
updatedAt *time.Time
|
|
}
|
|
|
|
func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID) (*discArtworkReader, error) {
|
|
albumID, discNumber, err := model.ParseDiscArtworkID(artID.ID)
|
|
if err != nil {
|
|
return nil, fmt.Errorf("invalid disc artwork id '%s': %w", artID.ID, err)
|
|
}
|
|
|
|
al, err := a.ds.Album(ctx).Get(albumID)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
_, imgFiles, imagesUpdatedAt, err := loadAlbumFoldersPaths(ctx, a.ds, *al)
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
// Query mediafiles for this album + disc to find folder associations and first track
|
|
mfs, err := a.ds.MediaFile(ctx).GetAll(model.QueryOptions{
|
|
Sort: "track_number",
|
|
Order: "ASC",
|
|
Filters: squirrel.Eq{"album_id": albumID, "disc_number": discNumber},
|
|
})
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
|
|
// Build disc folder set and find first track
|
|
discFolders := make(map[string]bool)
|
|
var firstTrackPath string
|
|
allFolderIDs := make(map[string]bool)
|
|
for _, mf := range mfs {
|
|
allFolderIDs[mf.FolderID] = true
|
|
if firstTrackPath == "" {
|
|
firstTrackPath = mf.Path
|
|
}
|
|
}
|
|
|
|
// Resolve folder IDs to absolute paths
|
|
if len(allFolderIDs) > 0 {
|
|
folderIDs := make([]string, 0, len(allFolderIDs))
|
|
for id := range allFolderIDs {
|
|
folderIDs = append(folderIDs, id)
|
|
}
|
|
folders, err := a.ds.Folder(ctx).GetAll(model.QueryOptions{
|
|
Filters: squirrel.Eq{"folder.id": folderIDs},
|
|
})
|
|
if err != nil {
|
|
return nil, err
|
|
}
|
|
for _, f := range folders {
|
|
discFolders[f.AbsolutePath()] = true
|
|
}
|
|
}
|
|
|
|
isMultiFolder := len(al.FolderIDs) > 1
|
|
|
|
r := &discArtworkReader{
|
|
a: a,
|
|
album: *al,
|
|
discNumber: discNumber,
|
|
imgFiles: imgFiles,
|
|
discFolders: discFolders,
|
|
isMultiFolder: isMultiFolder,
|
|
firstTrackPath: core.AbsolutePath(ctx, a.ds, al.LibraryID, firstTrackPath),
|
|
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
|
|
}
|
|
return r, nil
|
|
}
|
|
|
|
func (d *discArtworkReader) Key() string {
|
|
hash := md5.Sum([]byte(conf.Server.DiscArtPriority))
|
|
return fmt.Sprintf(
|
|
"%s.%x",
|
|
d.cacheKey.Key(),
|
|
hash,
|
|
)
|
|
}
|
|
|
|
func (d *discArtworkReader) LastUpdated() time.Time {
|
|
return d.lastUpdate
|
|
}
|
|
|
|
func (d *discArtworkReader) Reader(ctx context.Context) (io.ReadCloser, string, error) {
|
|
var ff = d.fromDiscArtPriority(ctx, d.a.ffmpeg, conf.Server.DiscArtPriority)
|
|
// Fallback to album cover art
|
|
albumArtID := model.NewArtworkID(model.KindAlbumArtwork, d.album.ID, &d.album.UpdatedAt)
|
|
ff = append(ff, fromAlbum(ctx, d.a, albumArtID))
|
|
return selectImageReader(ctx, d.cacheKey.artID, ff...)
|
|
}
|
|
|
|
func (d *discArtworkReader) fromDiscArtPriority(ctx context.Context, ffmpeg ffmpeg.FFmpeg, priority string) []sourceFunc {
|
|
var ff []sourceFunc
|
|
for pattern := range strings.SplitSeq(strings.ToLower(priority), ",") {
|
|
pattern = strings.TrimSpace(pattern)
|
|
switch {
|
|
case pattern == "embedded":
|
|
ff = append(ff, fromTag(ctx, d.firstTrackPath), fromFFmpegTag(ctx, ffmpeg, d.firstTrackPath))
|
|
case pattern == "external":
|
|
// Not supported for disc art, silently ignore
|
|
case pattern == "discsubtitle":
|
|
if subtitle := strings.TrimSpace(d.album.Discs[d.discNumber]); subtitle != "" {
|
|
ff = append(ff, d.fromDiscSubtitle(ctx, subtitle))
|
|
}
|
|
case len(d.imgFiles) > 0:
|
|
ff = append(ff, d.fromExternalFile(ctx, pattern))
|
|
}
|
|
}
|
|
return ff
|
|
}
|
|
|
|
// fromDiscSubtitle returns a sourceFunc that matches image files whose stem
|
|
// (filename without extension) equals the disc subtitle (case-insensitive).
|
|
func (d *discArtworkReader) fromDiscSubtitle(ctx context.Context, subtitle string) sourceFunc {
|
|
return func() (io.ReadCloser, string, error) {
|
|
for _, file := range d.imgFiles {
|
|
_, name := filepath.Split(file)
|
|
stem := strings.TrimSuffix(name, filepath.Ext(name))
|
|
if !strings.EqualFold(stem, subtitle) {
|
|
continue
|
|
}
|
|
f, err := os.Open(file)
|
|
if err != nil {
|
|
log.Warn(ctx, "Could not open disc art file", "file", file, err)
|
|
continue
|
|
}
|
|
return f, file, nil
|
|
}
|
|
return nil, "", fmt.Errorf("disc %d: no image file matching subtitle %q", d.discNumber, subtitle)
|
|
}
|
|
}
|
|
|
|
// 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
|
|
// filepath.Match-style glob pattern.
|
|
//
|
|
// Both pattern and filename must already be lowercased by the caller, which
|
|
// is also expected to have verified that filepath.Match(pattern, filename)
|
|
// is true before calling this function.
|
|
func extractDiscNumber(pattern, filename string) (int, bool) {
|
|
metaIdx := strings.IndexAny(pattern, globMetaChars)
|
|
if metaIdx < 0 {
|
|
return 0, false
|
|
}
|
|
prefix := pattern[:metaIdx]
|
|
if !strings.HasPrefix(filename, prefix) {
|
|
return 0, false
|
|
}
|
|
|
|
start := len(prefix)
|
|
end := start
|
|
for end < len(filename) && filename[end] >= '0' && filename[end] <= '9' {
|
|
end++
|
|
}
|
|
if end == start {
|
|
return 0, false
|
|
}
|
|
num, err := strconv.Atoi(filename[start:end])
|
|
if err != nil {
|
|
return 0, false
|
|
}
|
|
return num, true
|
|
}
|
|
|
|
// fromExternalFile returns a sourceFunc that matches image files against a glob
|
|
// pattern. A numbered filename whose number equals the target disc wins over
|
|
// any unnumbered candidate; callers must pass a lowercase pattern.
|
|
func (d *discArtworkReader) fromExternalFile(ctx context.Context, pattern string) sourceFunc {
|
|
isLiteral := !strings.ContainsAny(pattern, globMetaChars)
|
|
return func() (io.ReadCloser, string, error) {
|
|
var fallbacks []string
|
|
for _, file := range d.imgFiles {
|
|
_, name := filepath.Split(file)
|
|
name = strings.ToLower(name)
|
|
match, err := filepath.Match(pattern, name)
|
|
if err != nil {
|
|
log.Warn(ctx, "Error matching disc art file to pattern", "pattern", pattern, "file", file)
|
|
continue
|
|
}
|
|
if !match {
|
|
continue
|
|
}
|
|
|
|
if !isLiteral {
|
|
if num, hasNum := extractDiscNumber(pattern, name); hasNum {
|
|
if num != d.discNumber {
|
|
continue
|
|
}
|
|
f, err := os.Open(file)
|
|
if err != nil {
|
|
log.Warn(ctx, "Could not open disc art file", "file", file, err)
|
|
continue
|
|
}
|
|
return f, file, nil
|
|
}
|
|
}
|
|
|
|
if d.isMultiFolder && !d.discFolders[filepath.Dir(file)] {
|
|
continue
|
|
}
|
|
fallbacks = append(fallbacks, file)
|
|
}
|
|
|
|
for _, file := range fallbacks {
|
|
f, err := os.Open(file)
|
|
if err != 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)
|
|
}
|
|
}
|