mirror of
https://github.com/navidrome/navidrome.git
synced 2026-08-01 07:21:17 +00:00
fix(album): don't wipe covers on album merges; bust disc/song art caches
Adversarial review findings: - CopyAttributes copied uploaded_image/cover_art_updated_at unconditionally, so merging an album without a cover into one with a cover (retag onto an existing album ID, or phase-2 moved-track matching) blanked the destination's uploaded cover. Skip empty/NULL source values, mirroring the created_at guard, and make the mock faithful to the same semantics. - The disc and mediafile artwork readers did not fold cover_art_updated_at into their cache keys, so song/disc art reached through the fromAlbum fallback kept serving stale bytes after a cover upload or delete. - The UI cover cache-buster compared timestamps lexicographically; mixed offset/fractional formats could pick the older one. Compare as dates.
This commit is contained in:
parent
319662174e
commit
35d08a25a3
@ -110,6 +110,9 @@ func newDiscArtworkReader(ctx context.Context, a *artwork, artID model.ArtworkID
|
||||
if imagesUpdatedAt != nil {
|
||||
r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *imagesUpdatedAt)
|
||||
}
|
||||
if al.CoverArtUpdatedAt != nil {
|
||||
r.cacheKey.lastUpdate = utils.TimeNewest(r.cacheKey.lastUpdate, *al.CoverArtUpdatedAt)
|
||||
}
|
||||
return r, nil
|
||||
}
|
||||
|
||||
|
||||
@ -49,6 +49,9 @@ func newMediafileArtworkReader(ctx context.Context, artwork *artwork, artID mode
|
||||
if imagesUpdatedAt != nil && imagesUpdatedAt.After(a.cacheKey.lastUpdate) {
|
||||
a.cacheKey.lastUpdate = *imagesUpdatedAt
|
||||
}
|
||||
if al.CoverArtUpdatedAt != nil && al.CoverArtUpdatedAt.After(a.cacheKey.lastUpdate) {
|
||||
a.cacheKey.lastUpdate = *al.CoverArtUpdatedAt
|
||||
}
|
||||
return a, nil
|
||||
}
|
||||
|
||||
|
||||
@ -287,6 +287,10 @@ func (r *albumRepository) CopyAttributes(fromID, toID string, columns ...string)
|
||||
if col == "created_at" && (!v.Valid || v.String == "" || strings.HasPrefix(v.String, "0001-")) {
|
||||
continue
|
||||
}
|
||||
// A source without an uploaded cover must not wipe one the destination has.
|
||||
if (col == "uploaded_image" || col == "cover_art_updated_at") && (!v.Valid || v.String == "") {
|
||||
continue
|
||||
}
|
||||
to[col] = v
|
||||
}
|
||||
if len(to) == 0 {
|
||||
|
||||
@ -116,6 +116,14 @@ var _ = Describe("AlbumRepository", func() {
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(got.UploadedImage).To(Equal("copy-src_cover.jpg"))
|
||||
})
|
||||
It("does not wipe the destination's cover when the source has none", func() {
|
||||
Expect(albumRepo.UpdateImage("copy-dst", "copy-dst_cover.jpg")).To(Succeed())
|
||||
Expect(albumRepo.CopyAttributes("copy-src", "copy-dst", "uploaded_image", "cover_art_updated_at")).To(Succeed())
|
||||
got, err := albumRepo.Get("copy-dst")
|
||||
Expect(err).ToNot(HaveOccurred())
|
||||
Expect(got.UploadedImage).To(Equal("copy-dst_cover.jpg"))
|
||||
Expect(got.CoverArtUpdatedAt).ToNot(BeNil())
|
||||
})
|
||||
})
|
||||
|
||||
Describe("GetCursor", func() {
|
||||
|
||||
@ -190,9 +190,14 @@ func (m *MockAlbumRepo) CopyAttributes(fromID, toID string, columns ...string) e
|
||||
case "created_at":
|
||||
to.CreatedAt = from.CreatedAt
|
||||
case "uploaded_image":
|
||||
to.UploadedImage = from.UploadedImage
|
||||
// Mirrors the real repo: an empty source never wipes the destination's cover
|
||||
if from.UploadedImage != "" {
|
||||
to.UploadedImage = from.UploadedImage
|
||||
}
|
||||
case "cover_art_updated_at":
|
||||
to.CoverArtUpdatedAt = from.CoverArtUpdatedAt
|
||||
if from.CoverArtUpdatedAt != nil {
|
||||
to.CoverArtUpdatedAt = from.CoverArtUpdatedAt
|
||||
}
|
||||
}
|
||||
}
|
||||
if m.CopyAttributesCalls == nil {
|
||||
|
||||
@ -82,10 +82,10 @@ const getAvatarUrl = (username, size) =>
|
||||
|
||||
const getCoverArtUrl = (record, size, square) => {
|
||||
// Bust the cache on the newest of updatedAt / coverArtUpdatedAt (album cover
|
||||
// uploads bump the latter) — ISO timestamps sort chronologically.
|
||||
// uploads bump the latter).
|
||||
const cacheKey = [record.updatedAt, record.coverArtUpdatedAt]
|
||||
.filter(Boolean)
|
||||
.sort()
|
||||
.sort((a, b) => new Date(a) - new Date(b))
|
||||
.pop()
|
||||
const options = {
|
||||
...(cacheKey && { _: cacheKey }),
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user