fix(album): self-contained CopyAttributes stamp guard; close coverage gaps

CopyAttributes' cover-stamp guard read the source's uploaded_image only when
the caller also requested that column — a hidden cross-column contract a
future caller could silently violate. Fetch it internally instead, and align
the mock with the compound guard (it copied stamps from coverless sources).

Also cover the untested new code: Key() cover-stamp components in the disc
and mediafile readers, and the playlist tracks list path exposing
cover_art_updated_at.
This commit is contained in:
Deluan 2026-07-18 02:07:07 -04:00
parent 5a90e17693
commit a5d3d615d8
6 changed files with 65 additions and 2 deletions

View File

@ -4,6 +4,7 @@ import (
"context"
"os"
"path/filepath"
"time"
"github.com/navidrome/navidrome/model"
. "github.com/onsi/ginkgo/v2"
@ -11,6 +12,17 @@ import (
)
var _ = Describe("Disc Artwork Reader", func() {
Describe("Key", func() {
It("changes when the album's cover stamp changes", func() {
r := &discArtworkReader{}
r.album = model.Album{ID: "al-1"}
before := r.Key()
stamp := time.Now()
r.album.CoverArtUpdatedAt = &stamp
Expect(r.Key()).ToNot(Equal(before))
})
})
Describe("extractDiscNumber", func() {
DescribeTable("extracts disc number from filename based on glob pattern",
func(pattern, filename string, expectedNum int, expectedOk bool) {

View File

@ -0,0 +1,22 @@
package artwork
import (
"time"
"github.com/navidrome/navidrome/model"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)
var _ = Describe("MediaFile Artwork Reader", func() {
Describe("Key", func() {
It("changes when the album's cover stamp changes", func() {
r := &mediafileArtworkReader{}
r.album = model.Album{ID: "al-1"}
before := r.Key()
stamp := time.Now()
r.album.CoverArtUpdatedAt = &stamp
Expect(r.Key()).ToNot(Equal(before))
})
})
})

View File

@ -285,8 +285,14 @@ func (r *albumRepository) GetCursor(options ...model.QueryOptions) (model.AlbumC
}
func (r *albumRepository) CopyAttributes(fromID, toID string, columns ...string) error {
// The cover-stamp guard below needs the source's uploaded_image even when the
// caller didn't request it
selectCols := columns
if slices.Contains(columns, "cover_art_updated_at") && !slices.Contains(columns, "uploaded_image") {
selectCols = append(slices.Clone(columns), "uploaded_image")
}
var from dbx.NullStringMap
err := r.queryOne(Select(columns...).From(r.tableName).Where(Eq{"id": fromID}), &from)
err := r.queryOne(Select(selectCols...).From(r.tableName).Where(Eq{"id": fromID}), &from)
if err != nil {
return fmt.Errorf("getting album to copy fields from: %w", err)
}

View File

@ -174,6 +174,23 @@ var _ = Describe("AlbumRepository", func() {
Expect(got.UploadedImage).To(Equal("copy-dst_cover.jpg"))
Expect(got.CoverArtUpdatedAt).ToNot(BeNil())
})
It("applies the cover-stamp guard even when uploaded_image is not requested", func() {
Expect(albumRepo.UpdateImage("copy-src", "copy-src_cover.jpg")).To(Succeed())
Expect(albumRepo.CopyAttributes("copy-src", "copy-dst", "cover_art_updated_at")).To(Succeed())
got, err := albumRepo.Get("copy-dst")
Expect(err).ToNot(HaveOccurred())
Expect(got.CoverArtUpdatedAt).ToNot(BeNil(), "stamp must copy when the source has a cover")
Expect(albumRepo.UpdateImage("copy-src", "")).To(Succeed())
Expect(albumRepo.UpdateImage("copy-zero", "z.jpg")).To(Succeed())
Expect(albumRepo.UpdateImage("copy-zero", "")).To(Succeed())
before, err := albumRepo.Get("copy-dst")
Expect(err).ToNot(HaveOccurred())
Expect(albumRepo.CopyAttributes("copy-zero", "copy-dst", "cover_art_updated_at")).To(Succeed())
got, err = albumRepo.Get("copy-dst")
Expect(err).ToNot(HaveOccurred())
Expect(got.CoverArtUpdatedAt.Equal(*before.CoverArtUpdatedAt)).To(BeTrue(), "coverless source must not contribute a stamp")
})
It("does not copy a stale cover stamp left behind by a cover removal", func() {
// A removal clears uploaded_image but keeps cover_art_updated_at set
Expect(albumRepo.UpdateImage("copy-src", "copy-src_cover.jpg")).To(Succeed())

View File

@ -59,6 +59,11 @@ var _ = Describe("PlaylistTrackRepository", func() {
trk, ok := got.(*model.PlaylistTrack)
Expect(ok).To(BeTrue())
Expect(trk.CoverArtUpdatedAt).ToNot(BeNil())
// The list path (tracksQuery/loadTracks) must expose it too
all, err := repo.GetAll(model.QueryOptions{Sort: "id"})
Expect(err).ToNot(HaveOccurred())
Expect(all[0].CoverArtUpdatedAt).ToNot(BeNil())
})
})

View File

@ -208,7 +208,8 @@ func (m *MockAlbumRepo) CopyAttributes(fromID, toID string, columns ...string) e
to.UploadedImage = from.UploadedImage
}
case "cover_art_updated_at":
if from.CoverArtUpdatedAt != nil {
// Mirrors the real repo: a coverless source never contributes a stale stamp
if from.CoverArtUpdatedAt != nil && from.UploadedImage != "" {
to.CoverArtUpdatedAt = from.CoverArtUpdatedAt
}
}