From 0b365a00900882e63b112ae3c474626bbbd99669 Mon Sep 17 00:00:00 2001 From: Deluan Date: Thu, 16 Jul 2026 00:35:17 -0400 Subject: [PATCH] refactor(artwork): simplify blurhash cleanup review findings - detect placeholder artwork via the reader's source path instead of comparing against precomputed placeholder hashes (fragile fingerprint, dead artist-placeholder branch) - collapse the three identical UpdateBlurHash repo methods into a shared sqlRepository.updateBlurHash helper --- core/artwork/blurhash_updater.go | 41 ++++--------------- .../artwork/blurhash_updater_internal_test.go | 10 ----- persistence/album_repository.go | 8 +--- persistence/artist_repository.go | 8 +--- persistence/playlist_repository.go | 8 +--- persistence/sql_base_repository.go | 10 +++++ 6 files changed, 20 insertions(+), 65 deletions(-) diff --git a/core/artwork/blurhash_updater.go b/core/artwork/blurhash_updater.go index e031d82a4..90947c2b1 100644 --- a/core/artwork/blurhash_updater.go +++ b/core/artwork/blurhash_updater.go @@ -11,7 +11,6 @@ import ( "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/model" "github.com/navidrome/navidrome/model/request" - "github.com/navidrome/navidrome/resources" ) // blurHashUpdater keeps stored blurhashes in sync with the artwork actually served. Enqueue is @@ -127,26 +126,23 @@ func (u *blurHashUpdater) computeFromArtwork(ctx context.Context, artID model.Ar if err != nil { return "", err } - // Bypasses artwork.Get so the worker's own fetch is never re-enqueued. - r, err := u.a.cache.Get(ctx, artReader) + // Reads straight from the source (not artwork.Get): no re-enqueue, and the returned path + // identifies placeholder artwork, which must never be persisted as an entity's blurhash. + r, path, err := artReader.Reader(ctx) if err != nil { return "", err } defer r.Close() + if path == consts.PlaceholderAlbumArt || path == consts.PlaceholderArtistArt { + return "", nil + } img, _, err := image.Decode(r) if err != nil { return "", err } b := img.Bounds() x, y := blurhash.Components(b.Dx(), b.Dy()) - hash, err := blurhash.Encode(img, x, y) - if err != nil { - return "", err - } - if _, isPlaceholder := placeholderHashes()[hash]; isPlaceholder { - return "", nil - } - return hash, nil + return blurhash.Encode(img, x, y) } func (u *blurHashUpdater) persist(ctx context.Context, artID model.ArtworkID, hash string, version time.Time) error { @@ -160,26 +156,3 @@ func (u *blurHashUpdater) persist(ctx context.Context, artID model.ArtworkID, ha } return nil } - -// placeholderHashes identifies placeholder bytes by their hash, since cached reads lose the -// source path; placeholder artwork must never be persisted as an entity's blurhash. -var placeholderHashes = sync.OnceValue(func() map[string]struct{} { - hashes := make(map[string]struct{}) - for _, name := range []string{consts.PlaceholderAlbumArt, consts.PlaceholderArtistArt} { - f, err := resources.FS().Open(name) - if err != nil { - continue - } - img, _, err := image.Decode(f) - _ = f.Close() - if err != nil { - continue - } - b := img.Bounds() - x, y := blurhash.Components(b.Dx(), b.Dy()) - if hash, err := blurhash.Encode(img, x, y); err == nil { - hashes[hash] = struct{}{} - } - } - return hashes -}) diff --git a/core/artwork/blurhash_updater_internal_test.go b/core/artwork/blurhash_updater_internal_test.go index 36c06686e..1dd22f308 100644 --- a/core/artwork/blurhash_updater_internal_test.go +++ b/core/artwork/blurhash_updater_internal_test.go @@ -59,14 +59,4 @@ var _ = Describe("blurHashUpdater", func() { Expect(func() { u.process(GinkgoT().Context(), model.Album{ID: "missing"}.CoverArtID()) }).ToNot(Panic()) }) }) - - Describe("placeholderHashes", func() { - It("computes hashes for the embedded placeholder images", func() { - hashes := placeholderHashes() - Expect(hashes).To(HaveLen(2)) - for h := range hashes { - Expect(len(h)).To(BeNumerically(">", 6)) - } - }) - }) }) diff --git a/persistence/album_repository.go b/persistence/album_repository.go index 35e5dba8f..cd5e3f1c0 100644 --- a/persistence/album_repository.go +++ b/persistence/album_repository.go @@ -213,14 +213,8 @@ func (r *albumRepository) Put(al *model.Album) error { return nil } -// UpdateBlurHash is a targeted update: a full-row put would race with the scanner. Deliberately -// a plain UPDATE with no insert fallback — updating a just-deleted row must be a silent no-op. func (r *albumRepository) UpdateBlurHash(id, blurHash string, artworkUpdatedAt time.Time) error { - upd := Update(r.tableName).Where(Eq{"id": id}). - Set("blur_hash", blurHash). - Set("blur_hash_updated_at", artworkUpdatedAt) - _, err := r.executeSQL(upd) - return err + return r.updateBlurHash(id, blurHash, artworkUpdatedAt) } // TODO Move external metadata to a separated table diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 519e5cb94..1308a1d5c 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -232,14 +232,8 @@ func (r *artistRepository) Put(a *model.Artist, colsToUpdate ...string) error { return err } -// UpdateBlurHash is a targeted update: a full-row put would race with the scanner. Deliberately -// a plain UPDATE with no insert fallback — updating a just-deleted row must be a silent no-op. func (r *artistRepository) UpdateBlurHash(id, blurHash string, artworkUpdatedAt time.Time) error { - upd := Update(r.tableName).Where(Eq{"id": id}). - Set("blur_hash", blurHash). - Set("blur_hash_updated_at", artworkUpdatedAt) - _, err := r.executeSQL(upd) - return err + return r.updateBlurHash(id, blurHash, artworkUpdatedAt) } func (r *artistRepository) UpdateExternalInfo(a *model.Artist) error { diff --git a/persistence/playlist_repository.go b/persistence/playlist_repository.go index 87af440c4..e5e97043c 100644 --- a/persistence/playlist_repository.go +++ b/persistence/playlist_repository.go @@ -155,14 +155,8 @@ func (r *playlistRepository) GetWithTracks(id string, refreshSmartPlaylist, incl return pls, nil } -// UpdateBlurHash is a targeted update: a full-row put would race with playlist sync. Deliberately -// a plain UPDATE with no insert fallback — updating a just-deleted row must be a silent no-op. func (r *playlistRepository) UpdateBlurHash(id, blurHash string, artworkUpdatedAt time.Time) error { - upd := Update(r.tableName).Where(Eq{"id": id}). - Set("blur_hash", blurHash). - Set("blur_hash_updated_at", artworkUpdatedAt) - _, err := r.executeSQL(upd) - return err + return r.updateBlurHash(id, blurHash, artworkUpdatedAt) } func (r *playlistRepository) FindByPath(path string) (*model.Playlist, error) { diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index d0cbb2946..be6fcfb74 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -293,6 +293,16 @@ func (r sqlRepository) resetSeededRandom(options []model.QueryOptions) { } } +// updateBlurHash is a targeted update: a full-row put would race with the scanner. Deliberately +// a plain UPDATE with no insert fallback — updating a just-deleted row must be a silent no-op. +func (r sqlRepository) updateBlurHash(id, blurHash string, artworkUpdatedAt time.Time) error { + upd := Update(r.tableName).Where(Eq{"id": id}). + Set("blur_hash", blurHash). + Set("blur_hash_updated_at", artworkUpdatedAt) + _, err := r.executeSQL(upd) + return err +} + func (r sqlRepository) executeSQL(sq Sqlizer) (int64, error) { query, args, err := r.toSQL(sq) if err != nil {