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

feat(server/m7-cover-sources): album enricher uses provider chain

Browse Source 
Rewires EnrichAlbum to iterate EnabledAlbumProviders from the
SettingsService instead of a single hardcoded MBCAA fetcher. Sidecar
layer is unchanged (always tried first). Per-row eligibility check
honors cover_art_sources_version: terminal 'found' values skip;
'none' flips to NULL via ClearAlbumCoverNone and proceeds (the DB-
level ListAlbumsMissingCover query guards the version check for batch
callers; RetryAlbum clears via ClearAlbumCover first); NULL is eligible.

The provider chain uses an inline allWere404 accumulator to settle
the row to 'none' only when every provider returned ErrNotFound.
Any provider returning ErrTransient leaves the row NULL for next-
pass retry — even if other providers returned ErrNotFound — since
the transient call's MBID might succeed on a future attempt.

Boot wiring (cmd/minstrel/main.go) replaces the
NewFetcher+NewEnricher(fetcher, mbcaaOn) shape with
NewMBCAAProviderFromConfig (swaps in production User-Agent on the
registered provider) + NewSettingsService + NewEnricher(settings).
The old cfg.Library.CoverArtFromMBCAA flag is no-op'd; DB-backed
settings replace it. Field stays in the config struct for backward
compat with deployed config files.

Consolidates fetcher.go's HTTP logic into provider_mbcaa.go (no more
wrapper indirection): mbcaaProvider now holds cfg/mu/lastCall/client
directly. Deletes fetcher.go and fetcher_test.go. ErrNotFound and
ErrTransient move into provider.go alongside the other sentinels.

Updates admin_covers_test.go and provider_mbcaa_test.go to the new
constructor shapes. Adds multi-provider-chain, transient-leaves-null,
and stale-none-flips-and-retries test cases in enricher_test.go.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Bryan Van Deusen
2026-05-06 12:52:57 -04:00
parent 392b8aa12a
commit 385eb3b163
9 changed files with 521 additions and 381 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/coverart/enricher_test.go
+247 -16
View File
@@ -43,6 +43,21 @@ func discardLogger() *slog.Logger {
return slog.New(slog.NewTextHandler(io.Discard, nil))
}
// newTestEnricher constructs an Enricher backed by a real DB pool for
// integration tests. Callers should register any fakes they need in
// the registry before calling this; SettingsService.reconcile() will
// pick them up.
func newTestEnricher(t *testing.T, pool *pgxpool.Pool) *Enricher {
t.Helper()
resetRegistryForTests()
t.Cleanup(resetRegistryForTests)
s, err := NewSettingsService(context.Background(), pool, discardLogger())
if err != nil {
t.Fatalf("NewSettingsService: %v", err)
}
return NewEnricher(pool, discardLogger(), s)
}
// seedAlbumWithTrack creates an artist + album + track triple in a temp
// dir. Returns album ID and the album-dir path.
func seedAlbumWithTrack(t *testing.T, pool *pgxpool.Pool, title, artistName, mbid string) (pgtype.UUID, string) {
@@ -85,7 +100,7 @@ func TestEnrichAlbum_SidecarFound(t *testing.T) {
t.Fatal(err)
}
e := NewEnricher(pool, discardLogger(), nil, false)
e := newTestEnricher(t, pool)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
@@ -104,7 +119,7 @@ func TestEnrichAlbum_SidecarFound(t *testing.T) {
func TestEnrichAlbum_NoSidecarNoMBID_LeavesNull(t *testing.T) {
pool := newPool(t)
id, _ := seedAlbumWithTrack(t, pool, "NoMbid", "Artist", "")
e := NewEnricher(pool, discardLogger(), nil, true)
e := newTestEnricher(t, pool)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
@@ -117,9 +132,9 @@ func TestEnrichAlbum_NoSidecarNoMBID_LeavesNull(t *testing.T) {
}
}
func TestEnrichAlbum_MBCAAFetchSuccess(t *testing.T) {
func TestEnrichAlbum_ProviderFetchSuccess(t *testing.T) {
pool := newPool(t)
id, dir := seedAlbumWithTrack(t, pool, "Mbcaa", "Artist", "test-mbid-123")
id, dir := seedAlbumWithTrack(t, pool, "Prov", "Artist", "test-mbid-123")
upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "image/jpeg")
@@ -127,8 +142,23 @@ func TestEnrichAlbum_MBCAAFetchSuccess(t *testing.T) {
}))
defer upstream.Close()
f := NewFetcher(FetcherConfig{BaseURL: upstream.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: upstream.Client()})
e := NewEnricher(pool, discardLogger(), f, true)
// Register the MBCAA provider pointing at the test server, then
// build the Enricher so SettingsService picks it up.
resetRegistryForTests()
t.Cleanup(resetRegistryForTests)
p := newMBCAAProvider(FetcherConfig{
BaseURL: upstream.URL,
UserAgent: "test",
MinPeriod: 0,
HTTPClient: upstream.Client(),
})
Register(p)
s, err := NewSettingsService(context.Background(), pool, discardLogger())
if err != nil {
t.Fatalf("NewSettingsService: %v", err)
}
e := NewEnricher(pool, discardLogger(), s)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
@@ -153,7 +183,7 @@ func TestEnrichAlbum_MBCAAFetchSuccess(t *testing.T) {
}
}
func TestEnrichAlbum_MBCAA404_RecordsNone(t *testing.T) {
func TestEnrichAlbum_All404_RecordsNone(t *testing.T) {
pool := newPool(t)
id, _ := seedAlbumWithTrack(t, pool, "Missing", "Artist", "missing-mbid")
@@ -162,8 +192,21 @@ func TestEnrichAlbum_MBCAA404_RecordsNone(t *testing.T) {
}))
defer upstream.Close()
f := NewFetcher(FetcherConfig{BaseURL: upstream.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: upstream.Client()})
e := NewEnricher(pool, discardLogger(), f, true)
resetRegistryForTests()
t.Cleanup(resetRegistryForTests)
p := newMBCAAProvider(FetcherConfig{
BaseURL: upstream.URL,
UserAgent: "test",
MinPeriod: 0,
HTTPClient: upstream.Client(),
})
Register(p)
s, err := NewSettingsService(context.Background(), pool, discardLogger())
if err != nil {
t.Fatalf("NewSettingsService: %v", err)
}
e := NewEnricher(pool, discardLogger(), s)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
@@ -184,7 +227,7 @@ func TestEnrichAlbum_AlreadySidecar_NoOp(t *testing.T) {
t.Fatal(err)
}
e := NewEnricher(pool, discardLogger(), nil, false)
e := newTestEnricher(t, pool)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("first enrich: %v", err)
}
@@ -210,7 +253,8 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) {
}
id2, _ := seedAlbumWithTrack(t, pool, "Drain2", "B", "")
// Pre-mark id2 as 'none' — should NOT be drained by EnrichBatch.
// Pre-mark id2 as 'none' with current version — should NOT be drained by EnrichBatch
// (ListAlbumsMissingCover only returns stale-version 'none').
src := "none"
if err := dbq.New(pool).SetAlbumCover(context.Background(), dbq.SetAlbumCoverParams{
ID: id2, Column2: "", CoverArtSource: &src,
@@ -218,7 +262,7 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) {
t.Fatal(err)
}
e := NewEnricher(pool, discardLogger(), nil, false)
e := newTestEnricher(t, pool)
processed, _, _, err := e.EnrichBatch(context.Background(), 100)
if err != nil {
t.Fatalf("batch: %v", err)
@@ -237,7 +281,7 @@ func TestEnrichBatch_DrainsNullSourceOnly(t *testing.T) {
}
}
func TestRetryAlbum_DeletesMbcaaFileAndRefetches(t *testing.T) {
func TestRetryAlbum_DeletesWrittenFileAndRefetches(t *testing.T) {
pool := newPool(t)
id, dir := seedAlbumWithTrack(t, pool, "Retry", "Artist", "retry-mbid")
@@ -247,8 +291,21 @@ func TestRetryAlbum_DeletesMbcaaFileAndRefetches(t *testing.T) {
_, _ = w.Write([]byte("data"))
}))
defer upstream.Close()
f := NewFetcher(FetcherConfig{BaseURL: upstream.URL, UserAgent: "test", MinPeriod: 0, HTTPClient: upstream.Client()})
e := NewEnricher(pool, discardLogger(), f, true)
resetRegistryForTests()
t.Cleanup(resetRegistryForTests)
p := newMBCAAProvider(FetcherConfig{
BaseURL: upstream.URL,
UserAgent: "test",
MinPeriod: 0,
HTTPClient: upstream.Client(),
})
Register(p)
s, err := NewSettingsService(context.Background(), pool, discardLogger())
if err != nil {
t.Fatalf("NewSettingsService: %v", err)
}
e := NewEnricher(pool, discardLogger(), s)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("first enrich: %v", err)
@@ -304,7 +361,7 @@ func TestEnrichRetryMissing_DrainsNullAndNone(t *testing.T) {
t.Fatal(err)
}
e := NewEnricher(pool, discardLogger(), nil, false)
e := newTestEnricher(t, pool)
processed, err := e.EnrichRetryMissing(context.Background(), 100)
if err != nil {
t.Fatalf("retry missing: %v", err)
@@ -328,3 +385,177 @@ func TestEnrichRetryMissing_DrainsNullAndNone(t *testing.T) {
}
}
}
// TestEnrichAlbum_MultiProviderChain verifies that when provider A
// returns ErrNotFound, the enricher continues to provider B.
func TestEnrichAlbum_MultiProviderChain(t *testing.T) {
pool := newPool(t)
id, dir := seedAlbumWithTrack(t, pool, "Multi", "Artist", "multi-mbid")
// Provider A: always 404.
srvA := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
http.Error(w, "not found", http.StatusNotFound)
}))
defer srvA.Close()
// Provider B: always succeeds.
srvB := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
w.Header().Set("Content-Type", "image/jpeg")
_, _ = w.Write([]byte("PROVIDER_B_BYTES"))
}))
defer srvB.Close()
resetRegistryForTests()
t.Cleanup(resetRegistryForTests)
pA := newMBCAAProvider(FetcherConfig{
BaseURL: srvA.URL,
UserAgent: "test",
MinPeriod: 0,
HTTPClient: srvA.Client(),
})
Register(pA)
// fakeAlbumProvider from provider_test.go doesn't exist here yet;
// use a second mbcaa-shaped provider by registering a distinct one.
// We need a second AlbumCoverProvider with a different ID.
// Use a simple inline struct that wraps the second test server.
Register(&testAlbumProvider{
id: "test-provider-b",
baseURL: srvB.URL,
client: srvB.Client(),
})
s, err := NewSettingsService(context.Background(), pool, discardLogger())
if err != nil {
t.Fatalf("NewSettingsService: %v", err)
}
e := NewEnricher(pool, discardLogger(), s)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
row, err := dbq.New(pool).GetAlbumWithFirstTrackPath(context.Background(), id)
if err != nil {
t.Fatalf("get: %v", err)
}
if row.CoverArtSource == nil || *row.CoverArtSource != "test-provider-b" {
t.Errorf("source = %v, want 'test-provider-b' (provider B should win)", row.CoverArtSource)
}
if body, err := os.ReadFile(filepath.Join(dir, "cover.jpg")); err != nil || string(body) != "PROVIDER_B_BYTES" {
t.Errorf("cover.jpg body = %q, err = %v", body, err)
}
}
// TestEnrichAlbum_TransientLeavesNull verifies that a transient error
// from the provider leaves the row NULL for next-pass retry.
func TestEnrichAlbum_TransientLeavesNull(t *testing.T) {
pool := newPool(t)
id, _ := seedAlbumWithTrack(t, pool, "Transient", "Artist", "transient-mbid")
upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
http.Error(w, "boom", http.StatusInternalServerError)
}))
defer upstream.Close()
resetRegistryForTests()
t.Cleanup(resetRegistryForTests)
p := newMBCAAProvider(FetcherConfig{
BaseURL: upstream.URL,
UserAgent: "test",
MinPeriod: 0,
HTTPClient: upstream.Client(),
})
Register(p)
s, err := NewSettingsService(context.Background(), pool, discardLogger())
if err != nil {
t.Fatalf("NewSettingsService: %v", err)
}
e := NewEnricher(pool, discardLogger(), s)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
row, err := dbq.New(pool).GetAlbumWithFirstTrackPath(context.Background(), id)
if err != nil {
t.Fatalf("get: %v", err)
}
// Transient failure: source must remain NULL (eligible for retry).
if row.CoverArtSource != nil {
t.Errorf("source = %v, want nil (NULL — transient should not settle)", row.CoverArtSource)
}
}
// TestEnrichAlbum_StaleNoneFlipsAndRetries verifies that a row with
// cover_art_source='none' and a stale version gets cleared and
// re-enriched (sidecar wins because the album dir now has a cover file).
func TestEnrichAlbum_StaleNoneFlipsAndRetries(t *testing.T) {
pool := newPool(t)
id, dir := seedAlbumWithTrack(t, pool, "StaleNone", "Artist", "")
// Mark the album as 'none' (simulating a previous pass).
src := "none"
if err := dbq.New(pool).SetAlbumCover(context.Background(), dbq.SetAlbumCoverParams{
ID: id, Column2: "", CoverArtSource: &src,
}); err != nil {
t.Fatal(err)
}
// Now add a sidecar — EnrichAlbum should flip 'none' → NULL and pick it up.
if err := os.WriteFile(filepath.Join(dir, "cover.jpg"), []byte("new_art"), 0o644); err != nil {
t.Fatal(err)
}
e := newTestEnricher(t, pool)
if err := e.EnrichAlbum(context.Background(), id); err != nil {
t.Fatalf("enrich: %v", err)
}
row, err := dbq.New(pool).GetAlbumWithFirstTrackPath(context.Background(), id)
if err != nil {
t.Fatalf("get: %v", err)
}
if row.CoverArtSource == nil || *row.CoverArtSource != "sidecar" {
t.Errorf("source = %v, want 'sidecar' (stale none flipped and sidecar found)", row.CoverArtSource)
}
}
// testAlbumProvider is a minimal AlbumCoverProvider for multi-chain tests.
type testAlbumProvider struct {
id string
baseURL string
client *http.Client
}
func (p *testAlbumProvider) ID() string { return p.id }
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)
if err != nil {
return nil, err
}
resp, err := p.client.Do(req)
if err != nil {
return nil, err
}
defer func() { _ = resp.Body.Close() }()
if resp.StatusCode == http.StatusNotFound {
return nil, ErrNotFound
}
if resp.StatusCode != http.StatusOK {
return nil, ErrTransient
}
body := make([]byte, 0, 512)
buf := make([]byte, 512)
for {
n, rerr := resp.Body.Read(buf)
body = append(body, buf[:n]...)
if rerr != nil {
break
}
}
return body, nil
}