bvandeusen/minstrel
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 38 Wiki Activity

refactor(server/coverart): provider interface takes ArtistRef/AlbumRef

Browse Source 
Changes the AlbumCoverProvider and ArtistArtProvider interfaces from
MBID-only string parameters to ref-struct signatures so name-based
providers (Deezer, Last.fm in upcoming tasks) can act on rows
without MBIDs.

Today, 53 of 213 artists in the dev DB have NULL MBIDs — featured
artists, remixers, and compilation contributors created from
track-tag splits where the primary-artist MBID is in the file but
collaborator IDs aren't. These rows are unreachable by the current
MBID-only chain. Refactoring the interface unblocks future
name-based providers from acting on them.

TheAudioDB and MBCAA keep MBID-only behavior internally: they return
ErrNotFound when ref.MBID is empty rather than attempting a
name-based fallback. The MBID guard inside EnrichAlbum/EnrichArtist
is removed; providers receive the ref unconditionally and decide
whether they can act on it.

GetAlbumWithFirstTrackPath now JOINs artists to return artist_name
alongside the existing fields, supporting AlbumRef construction.

No behavioral change today (only TheAudioDB + MBCAA registered, both
keep MBID-only behavior). The change unblocks T3 (Deezer) and T4
(Last.fm) which can now register and act on every artist/album
regardless of MBID presence.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Bryan Van Deusen
2026-05-07 07:44:34 -04:00
parent 9a705d0ba4
commit 425f12db38
13 changed files with 99 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/coverart/artist_enricher.go
+8 -5
View File
@@ -56,18 +56,21 @@ func (e *Enricher) EnrichArtist(ctx context.Context, artistID pgtype.UUID, dataD
}
}
// MBID guard.
if row.Mbid == nil || *row.Mbid == "" {
return nil
// Build the ref. Every provider receives it and decides whether it
// can act on the fields available (MBID-only providers return
// ErrNotFound when MBID is empty; name-based providers fall back to
// Name).
ref := ArtistRef{Name: row.Name}
if row.Mbid != nil {
ref.MBID = *row.Mbid
}
mbid := *row.Mbid
// Provider chain. allWere404 tracks whether every provider returned
// ErrNotFound — only then do we settle to 'none'. Any transient
// failure leaves the row NULL for next-pass retry.
allWere404 := true
for _, provider := range e.settings.EnabledArtistProviders() {
thumb, fanart, perr := provider.FetchArtistArt(ctx, mbid)
thumb, fanart, perr := provider.FetchArtistArt(ctx, ref)
if perr == nil {
// Atomic write — at least one of (thumb, fanart) is non-nil
// per the ArtistArtProvider contract; persist whichever
internal/coverart/artist_enricher_test.go
+1 -1
View File
@@ -20,7 +20,7 @@ type stubArtistProvider struct {
err error
}
func (f *stubArtistProvider) FetchArtistArt(_ context.Context, _ string) ([]byte, []byte, error) {
func (f *stubArtistProvider) FetchArtistArt(_ context.Context, _ ArtistRef) ([]byte, []byte, error) {
return f.thumb, f.fanart, f.err
}
internal/coverart/enricher.go
+11 -5
View File
@@ -89,18 +89,24 @@ func (e *Enricher) EnrichAlbum(ctx context.Context, albumID pgtype.UUID) error {
}
}
// MBID guard — providers can't fetch without one.
if row.Mbid == nil || *row.Mbid == "" {
return nil // leave NULL; mbid backfill or future scan can heal
// Build the ref. Every provider receives it and decides whether it
// can act on the fields available (MBID-only providers return
// ErrNotFound when MBID is empty; name-based providers fall back to
// ArtistName + AlbumTitle).
ref := AlbumRef{
ArtistName: row.ArtistName,
AlbumTitle: row.Title,
}
if row.Mbid != nil {
ref.MBID = *row.Mbid
}
mbid := *row.Mbid
// Provider chain. allWere404 tracks whether every provider
// returned ErrNotFound — only then do we settle to 'none'. Any
// transient failure leaves the row NULL for next-pass retry.
allWere404 := true
for _, provider := range e.settings.EnabledAlbumProviders() {
body, perr := provider.FetchAlbumCover(ctx, mbid)
body, perr := provider.FetchAlbumCover(ctx, ref)
if perr == nil {
if row.TrackFilePath == nil || *row.TrackFilePath == "" {
// Defensive: shouldn't happen if MBID was set on an
internal/coverart/enricher_test.go
+2 -2
View File
@@ -532,8 +532,8 @@ func (p *testAlbumProvider) DisplayName() string { return p.id }
func (p *testAlbumProvider) RequiresAPIKey() bool { return false }
func (p *testAlbumProvider) DefaultEnabled() bool { return true }
func (p *testAlbumProvider) Configure(ProviderSettings) error { return nil }
func (p *testAlbumProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.baseURL+"/"+mbid, nil)
func (p *testAlbumProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]byte, error) {
req, err := http.NewRequestWithContext(ctx, http.MethodGet, p.baseURL+"/"+ref.MBID, nil)
if err != nil {
return nil, err
}
internal/coverart/provider.go
+30 -11
View File
@@ -50,26 +50,45 @@ type ProviderSettings struct {
APIKey string
}
// ArtistRef is the lookup key passed to ArtistArtProvider.FetchArtistArt.
// MBID is preferred when present; Name is the always-populated fallback
// for providers that support name-based search (Deezer, Last.fm).
// MBID-only providers (TheAudioDB) return ErrNotFound when MBID is empty.
type ArtistRef struct {
MBID string
Name string
}
// AlbumRef is the lookup key passed to AlbumCoverProvider.FetchAlbumCover.
// Same MBID-preferred semantics as ArtistRef; ArtistName + AlbumTitle
// support name-based providers.
type AlbumRef struct {
MBID string
ArtistName string
AlbumTitle string
}
// AlbumCoverProvider is an opt-in capability: providers implementing
// it can fetch album cover art by MusicBrainz release MBID.
// it can fetch album cover art. MBID-only providers use ref.MBID and
// return ErrNotFound when it is empty; name-based providers (Deezer,
// Last.fm) fall back to ref.ArtistName + ref.AlbumTitle.
type AlbumCoverProvider interface {
Provider
// FetchAlbumCover returns the image bytes for the given release
// MBID, or ErrNotFound (terminal — no art available) /
// FetchAlbumCover returns the image bytes for the given album ref,
// or ErrNotFound (terminal — no art available) /
// ErrTransient (retry-eligible failure).
FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error)
FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]byte, error)
}
// ArtistArtProvider is an opt-in capability: providers implementing
// it can fetch artist images by MusicBrainz artist MBID. Returns
// thumb + fanart atomically at the JSON metadata step (one round-
// trip yields both URLs); the two image GETs are independent —
// partial success returns whatever bytes landed and the source is
// still stamped, so the enricher persists asymmetric results
// (thumb-only or fanart-only) gracefully.
// it can fetch artist images. Returns thumb + fanart atomically at
// the JSON metadata step (one round-trip yields both URLs); the two
// image GETs are independent — partial success returns whatever bytes
// landed and the source is still stamped, so the enricher persists
// asymmetric results (thumb-only or fanart-only) gracefully.
type ArtistArtProvider interface {
Provider
FetchArtistArt(ctx context.Context, mbid string) (thumb, fanart []byte, err error)
FetchArtistArt(ctx context.Context, ref ArtistRef) (thumb, fanart []byte, err error)
}
// TestableProvider is an opt-in capability: providers that can answer
internal/coverart/provider_mbcaa.go
+7 -6
View File
@@ -89,13 +89,14 @@ func (p *mbcaaProvider) Configure(s ProviderSettings) error {
}
// FetchAlbumCover retrieves the 500px front cover for the given
// release MBID. Returns ErrNotFound on 404 or ErrTransient otherwise.
func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error) {
// release MBID. MBCAA is MBID-only; returns ErrNotFound when
// ref.MBID is empty. Returns ErrNotFound on 404 or ErrTransient otherwise.
func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]byte, error) {
if !p.enabled.Load() {
return nil, ErrNotFound // defensive; enricher already filters on enabled
}
if mbid == "" {
return nil, fmt.Errorf("coverart: empty mbid")
if ref.MBID == "" {
return nil, ErrNotFound // MBCAA is MBID-only
}
// Rate limit guard.
@@ -118,7 +119,7 @@ func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byt
p.lastCall = time.Now()
p.mu.Unlock()
url := fmt.Sprintf("%s/release/%s/front-500", cfg.BaseURL, mbid)
url := fmt.Sprintf("%s/release/%s/front-500", cfg.BaseURL, ref.MBID)
req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
if err != nil {
return nil, fmt.Errorf("coverart: build request: %w", err)
@@ -147,7 +148,7 @@ func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byt
}
func (p *mbcaaProvider) TestConnection(ctx context.Context) error {
_, err := p.FetchAlbumCover(ctx, mbcaaTestSampleMBID)
_, err := p.FetchAlbumCover(ctx, AlbumRef{MBID: mbcaaTestSampleMBID})
if err == nil {
return nil
}
internal/coverart/provider_mbcaa_test.go
+3 -3
View File
@@ -59,7 +59,7 @@ func TestMBCAAProvider_FetchAlbumCover_Success(t *testing.T) {
defer srv.Close()
p := newMBCAAProviderForTest(t, srv.URL)
body, err := p.FetchAlbumCover(context.Background(), "abc-123")
body, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "abc-123"})
if err != nil {
t.Fatalf("FetchAlbumCover err = %v, want nil", err)
}
@@ -75,7 +75,7 @@ func TestMBCAAProvider_FetchAlbumCover_NotFound(t *testing.T) {
defer srv.Close()
p := newMBCAAProviderForTest(t, srv.URL)
_, err := p.FetchAlbumCover(context.Background(), "missing")
_, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "missing"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("err = %v, want ErrNotFound", err)
}
@@ -90,7 +90,7 @@ func TestMBCAAProvider_FetchAlbumCover_DisabledReturnsNotFound(t *testing.T) {
p := newMBCAAProviderForTest(t, srv.URL)
p.enabled.Store(false)
_, err := p.FetchAlbumCover(context.Background(), "any")
_, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "any"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("err = %v, want ErrNotFound (disabled)", err)
}
internal/coverart/provider_test.go
+1 -1
View File
@@ -27,7 +27,7 @@ type fakeAlbumProvider struct {
fakeProvider
}
func (f *fakeAlbumProvider) FetchAlbumCover(_ context.Context, _ string) ([]byte, error) {
func (f *fakeAlbumProvider) FetchAlbumCover(_ context.Context, _ AlbumRef) ([]byte, error) {
return []byte("img"), nil
}
internal/coverart/provider_theaudiodb.go
+11 -9
View File
@@ -86,13 +86,14 @@ func (p *theAudioDBProvider) currentKey() string {
// FetchAlbumCover hits /album-mb.php?i=<mbid>, parses strAlbumThumb,
// then GETs the image URL. Two HTTP round-trips per call; both
// counted against the rate-limit guard.
func (p *theAudioDBProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error) {
// counted against the rate-limit guard. TheAudioDB is MBID-only;
// returns ErrNotFound when ref.MBID is empty.
func (p *theAudioDBProvider) FetchAlbumCover(ctx context.Context, ref AlbumRef) ([]byte, error) {
if !p.enabled.Load() {
return nil, ErrNotFound // defensive; enricher already filters on enabled
}
if mbid == "" {
return nil, fmt.Errorf("coverart: empty mbid")
if ref.MBID == "" {
return nil, ErrNotFound // TheAudioDB is MBID-only
}
var resp struct {
@@ -100,7 +101,7 @@ func (p *theAudioDBProvider) FetchAlbumCover(ctx context.Context, mbid string) (
StrAlbumThumb *string `json:"strAlbumThumb"`
} `json:"album"`
}
if err := p.getJSON(ctx, "album-mb.php?i="+mbid, &resp); err != nil {
if err := p.getJSON(ctx, "album-mb.php?i="+ref.MBID, &resp); err != nil {
return nil, err
}
if len(resp.Album) == 0 || resp.Album[0].StrAlbumThumb == nil || *resp.Album[0].StrAlbumThumb == "" {
@@ -111,6 +112,7 @@ func (p *theAudioDBProvider) FetchAlbumCover(ctx context.Context, mbid string) (
// FetchArtistArt hits /artist-mb.php?i=<mbid>, parses strArtistThumb +
// strArtistFanart, GETs both. Up to three HTTP round-trips per call.
// TheAudioDB is MBID-only; returns ErrNotFound when ref.MBID is empty.
//
// Atomic at the JSON step: if the JSON parse yields no images at all
// (artist not found OR both image fields null/empty), returns
@@ -119,12 +121,12 @@ func (p *theAudioDBProvider) FetchAlbumCover(ctx context.Context, mbid string) (
// returns the successful one with nil for the failed one — the
// enricher persists whichever bytes landed. If BOTH image GETs fail
// transiently, returns ErrTransient so the row stays NULL for retry.
func (p *theAudioDBProvider) FetchArtistArt(ctx context.Context, mbid string) (thumb, fanart []byte, err error) {
func (p *theAudioDBProvider) FetchArtistArt(ctx context.Context, ref ArtistRef) (thumb, fanart []byte, err error) {
if !p.enabled.Load() {
return nil, nil, ErrNotFound
}
if mbid == "" {
return nil, nil, fmt.Errorf("coverart: empty mbid")
if ref.MBID == "" {
return nil, nil, ErrNotFound // TheAudioDB is MBID-only
}
var resp struct {
@@ -133,7 +135,7 @@ func (p *theAudioDBProvider) FetchArtistArt(ctx context.Context, mbid string) (t
StrArtistFanart *string `json:"strArtistFanart"`
} `json:"artists"`
}
if err := p.getJSON(ctx, "artist-mb.php?i="+mbid, &resp); err != nil {
if err := p.getJSON(ctx, "artist-mb.php?i="+ref.MBID, &resp); err != nil {
return nil, nil, err
}
if len(resp.Artists) == 0 {
internal/coverart/provider_theaudiodb_test.go
+10 -10
View File
@@ -76,7 +76,7 @@ func TestTheAudioDB_FetchAlbumCover_Success(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
body, err := p.FetchAlbumCover(context.Background(), "test-mbid")
body, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "test-mbid"})
if err != nil {
t.Fatalf("FetchAlbumCover err = %v, want nil", err)
}
@@ -93,7 +93,7 @@ func TestTheAudioDB_FetchAlbumCover_NotFound(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
_, err := p.FetchAlbumCover(context.Background(), "missing-mbid")
_, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "missing-mbid"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("err = %v, want ErrNotFound", err)
}
@@ -107,7 +107,7 @@ func TestTheAudioDB_FetchAlbumCover_NullThumbURL(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
_, err := p.FetchAlbumCover(context.Background(), "mbid")
_, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "mbid"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("err = %v, want ErrNotFound when thumb URL is null", err)
}
@@ -127,7 +127,7 @@ func TestTheAudioDB_FetchAlbumCover_429TriggersRetry(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
_, err := p.FetchAlbumCover(context.Background(), "mbid")
_, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "mbid"})
if attempts.Load() < 2 {
t.Errorf("attempts = %d, want >= 2 (retry should have run)", attempts.Load())
}
@@ -164,7 +164,7 @@ func TestTheAudioDB_FetchArtistArt_BothPresent(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
thumb, fanart, err := p.FetchArtistArt(context.Background(), "mbid")
thumb, fanart, err := p.FetchArtistArt(context.Background(), ArtistRef{MBID: "mbid"})
if err != nil {
t.Fatalf("err = %v", err)
}
@@ -193,7 +193,7 @@ func TestTheAudioDB_FetchArtistArt_ThumbOnly(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
thumb, fanart, err := p.FetchArtistArt(context.Background(), "mbid")
thumb, fanart, err := p.FetchArtistArt(context.Background(), ArtistRef{MBID: "mbid"})
if err != nil {
t.Fatalf("err = %v", err)
}
@@ -213,7 +213,7 @@ func TestTheAudioDB_FetchArtistArt_BothEmpty(t *testing.T) {
defer srv.Close()
p := newTheAudioDBProviderForTest(t, srv.URL)
_, _, err := p.FetchArtistArt(context.Background(), "mbid")
_, _, err := p.FetchArtistArt(context.Background(), ArtistRef{MBID: "mbid"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("err = %v, want ErrNotFound", err)
}
@@ -241,7 +241,7 @@ func TestTheAudioDB_DefaultsToTestKeyOnEmpty(t *testing.T) {
t.Fatalf("Configure: %v", err)
}
_, _ = p.FetchAlbumCover(context.Background(), "mbid")
_, _ = p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "mbid"})
}
func TestTheAudioDB_DisabledReturnsNotFound(t *testing.T) {
@@ -253,12 +253,12 @@ func TestTheAudioDB_DisabledReturnsNotFound(t *testing.T) {
p := newTheAudioDBProviderForTest(t, srv.URL)
p.enabled.Store(false)
_, err := p.FetchAlbumCover(context.Background(), "any")
_, err := p.FetchAlbumCover(context.Background(), AlbumRef{MBID: "any"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("err = %v, want ErrNotFound (disabled)", err)
}
_, _, err = p.FetchArtistArt(context.Background(), "any")
_, _, err = p.FetchArtistArt(context.Background(), ArtistRef{MBID: "any"})
if !errors.Is(err, ErrNotFound) {
t.Errorf("artist err = %v, want ErrNotFound (disabled)", err)
}
internal/coverart/settings_test.go
+1 -1
View File
@@ -11,7 +11,7 @@ type fakeArtistProvider struct {
fakeProvider
}
func (f *fakeArtistProvider) FetchArtistArt(_ context.Context, _ string) (thumb, fanart []byte, err error) {
func (f *fakeArtistProvider) FetchArtistArt(_ context.Context, _ ArtistRef) (thumb, fanart []byte, err error) {
return []byte("t"), []byte("f"), nil
}
internal/db/dbq/covers.sql.go
+8 -2
View File
@@ -74,8 +74,10 @@ func (q *Queries) CountAlbumCoverSources(ctx context.Context) (CountAlbumCoverSo
const getAlbumWithFirstTrackPath = `-- name: GetAlbumWithFirstTrackPath :one
SELECT a.id, a.mbid, a.title, a.cover_art_path, a.cover_art_source,
ar.name AS artist_name,
t.file_path AS track_file_path
FROM albums a
JOIN artists ar ON ar.id = a.artist_id
LEFT JOIN tracks t ON t.album_id = a.id
WHERE a.id = $1
ORDER BY t.disc_number NULLS LAST, t.track_number NULLS LAST, t.id
@@ -88,11 +90,14 @@ type GetAlbumWithFirstTrackPathRow struct {
Title string
CoverArtPath *string
CoverArtSource *string
ArtistName string
TrackFilePath *string
}
// Returns the album row and one of its track file paths (used to derive
// the album directory). Pick lowest position track for determinism.
// Returns the album row, the artist's name (used by name-based art
// providers like Deezer/Last.fm), and one of its track file paths
// (used to derive the album directory for sidecar writes). Pick
// lowest position track for determinism.
func (q *Queries) GetAlbumWithFirstTrackPath(ctx context.Context, id pgtype.UUID) (GetAlbumWithFirstTrackPathRow, error) {
row := q.db.QueryRow(ctx, getAlbumWithFirstTrackPath, id)
var i GetAlbumWithFirstTrackPathRow
@@ -102,6 +107,7 @@ func (q *Queries) GetAlbumWithFirstTrackPath(ctx context.Context, id pgtype.UUID
&i.Title,
&i.CoverArtPath,
&i.CoverArtSource,
&i.ArtistName,
&i.TrackFilePath,
)
return i, err
internal/db/queries/covers.sql
+6 -2
View File
@@ -23,11 +23,15 @@ SELECT a.id, a.mbid, a.title, a.artist_id
LIMIT $1;
-- name: GetAlbumWithFirstTrackPath :one
-- Returns the album row and one of its track file paths (used to derive
-- the album directory). Pick lowest position track for determinism.
-- Returns the album row, the artist's name (used by name-based art
-- providers like Deezer/Last.fm), and one of its track file paths
-- (used to derive the album directory for sidecar writes). Pick
-- lowest position track for determinism.
SELECT a.id, a.mbid, a.title, a.cover_art_path, a.cover_art_source,
ar.name AS artist_name,
t.file_path AS track_file_path
FROM albums a
JOIN artists ar ON ar.id = a.artist_id
LEFT JOIN tracks t ON t.album_id = a.id
WHERE a.id = $1
ORDER BY t.disc_number NULLS LAST, t.track_number NULLS LAST, t.id