feat(server/m7-cover-sources): MBCAA provider wrapper

Adapts the existing Fetcher-based MBCAA client to the new Provider abstraction. mbcaaProvider embeds *Fetcher and implements Provider + AlbumCoverProvider + TestableProvider. init() registers a default-config instance; NewMBCAAProviderFromConfig swaps in production config (User-Agent, MinPeriod) at boot. Wrapper approach (rather than reimplementing the HTTP logic) keeps the package compiling during the T4–T8 window: enricher.go and main.go still reference the legacy Fetcher type / NewFetcher constructor / Fetch method, all of which remain unchanged in fetcher.go. A-T8 will consolidate the HTTP logic into this file and delete fetcher.go after the enricher rewrite removes the last legacy call sites. TestConnection hits a hardcoded popular release MBID (Beatles "Abbey Road" UK release) and treats both 200 and 404 as "connection works" — MBCAA needs no auth, so only 5xx / network failures surface. Tests cover ID/DisplayName/Capability metadata, success path, 404→ErrNotFound, disabled-returns-ErrNotFound, Configure toggle, TestConnection variants, and NewMBCAAProviderFromConfig swap.
2026-05-06 12:35:35 -04:00
parent b8c758c7c9
commit 1e3a76eba8
2 changed files with 262 additions and 0 deletions
@@ -0,0 +1,91 @@
+package coverart
+
+import (
+	"context"
+	"errors"
+	"sync/atomic"
+)
+
+// mbcaaProvider implements Provider, AlbumCoverProvider, and
+// TestableProvider. It wraps an *Fetcher (the existing
+// MBCAA-specific HTTP client) for the actual fetch logic, adapting
+// the Fetcher's Fetch method to the AlbumCoverProvider interface.
+//
+// The wrapping (rather than reimplementing) is temporary: A-T8 will
+// consolidate fetcher.go's logic into this file and delete fetcher.go.
+// During the T4–T8 window, this struct lets the new provider chain
+// coexist with the legacy Fetcher used by enricher.go and main.go.
+type mbcaaProvider struct {
+	fetcher *Fetcher
+	enabled atomic.Bool
+}
+
+// Sample MBID used by TestConnection. The Beatles' "Abbey Road" UK
+// release; verified present on MBCAA at design time. Both 200 (image
+// returned) and 404 (no art for the sample) are treated as "connection
+// works" since MBCAA needs no auth — only network failures surface.
+const mbcaaTestSampleMBID = "f4b3df80-8a6c-3e87-b51e-5c1ee70a78ee"
+
+func init() {
+	// Register with a default-config Fetcher. main.go (in A-T8) will
+	// call NewMBCAAProviderFromConfig to swap in the production
+	// User-Agent + MinPeriod. Default config values come from
+	// FetcherConfig's zero-value handling in NewFetcher.
+	Register(&mbcaaProvider{
+		fetcher: NewFetcher(FetcherConfig{}),
+	})
+}
+
+// NewMBCAAProviderFromConfig replaces the registered MBCAA provider's
+// internal Fetcher with one built from the supplied config. main.go
+// calls this at boot to set the production User-Agent. Idempotent;
+// safe to call repeatedly.
+func NewMBCAAProviderFromConfig(cfg FetcherConfig) {
+	p, err := ProviderByID("mbcaa")
+	if err != nil {
+		// init() should have registered it; if not, register fresh.
+		Register(&mbcaaProvider{fetcher: NewFetcher(cfg)})
+		return
+	}
+	mp, ok := p.(*mbcaaProvider)
+	if !ok {
+		// Some other provider claimed the "mbcaa" ID — registry bug.
+		panic("coverart: provider with ID 'mbcaa' is not *mbcaaProvider")
+	}
+	mp.fetcher = NewFetcher(cfg)
+}
+
+func (p *mbcaaProvider) ID() string           { return "mbcaa" }
+func (p *mbcaaProvider) DisplayName() string  { return "Cover Art Archive" }
+func (p *mbcaaProvider) RequiresAPIKey() bool { return false }
+func (p *mbcaaProvider) DefaultEnabled() bool { return true }
+
+func (p *mbcaaProvider) Configure(s ProviderSettings) error {
+	p.enabled.Store(s.Enabled)
+	return nil // MBCAA has no per-operator config beyond enabled
+}
+
+// FetchAlbumCover delegates to the wrapped Fetcher. The Fetcher's
+// existing Fetch method already returns ErrNotFound / ErrTransient
+// per the contract, so no error mapping is needed.
+func (p *mbcaaProvider) FetchAlbumCover(ctx context.Context, mbid string) ([]byte, error) {
+	if !p.enabled.Load() {
+		return nil, ErrNotFound // defensive; enricher already filters on enabled
+	}
+	return p.fetcher.Fetch(ctx, mbid)
+}
+
+// TestConnection hits the sample MBID's front-500 endpoint. A 200
+// (image bytes) or 404 (no art for sample) both indicate the
+// connection works; 5xx / network errors surface as TestConnection
+// failures.
+func (p *mbcaaProvider) TestConnection(ctx context.Context) error {
+	_, err := p.fetcher.Fetch(ctx, mbcaaTestSampleMBID)
+	if err == nil {
+		return nil
+	}
+	if errors.Is(err, ErrNotFound) {
+		return nil
+	}
+	return err
+}
@@ -0,0 +1,171 @@
+package coverart
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+// newMBCAAProviderForTest registers a fresh mbcaaProvider pointing at
+// a test server, mirroring the pattern from fetcher_test.go but
+// exercising the interface-side methods.
+func newMBCAAProviderForTest(t *testing.T, mockURL string) *mbcaaProvider {
+	t.Helper()
+	resetRegistryForTests()
+	t.Cleanup(resetRegistryForTests)
+
+	p := &mbcaaProvider{
+		fetcher: NewFetcher(FetcherConfig{
+			BaseURL:    mockURL,
+			UserAgent:  "Minstrel/test (https://example.com)",
+			MinPeriod:  0,
+			HTTPClient: &http.Client{Timeout: 5 * time.Second},
+		}),
+	}
+	Register(p)
+	p.enabled.Store(true)
+	return p
+}
+
+func TestMBCAAProvider_ID(t *testing.T) {
+	resetRegistryForTests()
+	defer resetRegistryForTests()
+	p := &mbcaaProvider{fetcher: NewFetcher(FetcherConfig{})}
+	if p.ID() != "mbcaa" {
+		t.Errorf("ID = %q, want mbcaa", p.ID())
+	}
+	if p.DisplayName() != "Cover Art Archive" {
+		t.Errorf("DisplayName = %q, want Cover Art Archive", p.DisplayName())
+	}
+	if p.RequiresAPIKey() {
+		t.Error("RequiresAPIKey = true, want false")
+	}
+	if !p.DefaultEnabled() {
+		t.Error("DefaultEnabled = false, want true")
+	}
+}
+
+func TestMBCAAProvider_FetchAlbumCover_Success(t *testing.T) {
+	imageBody := []byte("FAKE_JPEG")
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !strings.Contains(r.URL.Path, "/release/abc-123/front-500") {
+			http.Error(w, "wrong path: "+r.URL.Path, http.StatusBadRequest)
+			return
+		}
+		w.Header().Set("Content-Type", "image/jpeg")
+		_, _ = w.Write(imageBody)
+	}))
+	defer srv.Close()
+
+	p := newMBCAAProviderForTest(t, srv.URL)
+	body, err := p.FetchAlbumCover(context.Background(), "abc-123")
+	if err != nil {
+		t.Fatalf("FetchAlbumCover err = %v, want nil", err)
+	}
+	if string(body) != string(imageBody) {
+		t.Errorf("body = %q, want %q", body, imageBody)
+	}
+}
+
+func TestMBCAAProvider_FetchAlbumCover_NotFound(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "not found", http.StatusNotFound)
+	}))
+	defer srv.Close()
+
+	p := newMBCAAProviderForTest(t, srv.URL)
+	_, err := p.FetchAlbumCover(context.Background(), "missing")
+	if !errors.Is(err, ErrNotFound) {
+		t.Errorf("err = %v, want ErrNotFound", err)
+	}
+}
+
+func TestMBCAAProvider_FetchAlbumCover_DisabledReturnsNotFound(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Error("server should not be hit when provider is disabled")
+	}))
+	defer srv.Close()
+
+	p := newMBCAAProviderForTest(t, srv.URL)
+	p.enabled.Store(false)
+
+	_, err := p.FetchAlbumCover(context.Background(), "any")
+	if !errors.Is(err, ErrNotFound) {
+		t.Errorf("err = %v, want ErrNotFound (disabled)", err)
+	}
+}
+
+func TestMBCAAProvider_Configure_TogglesEnabled(t *testing.T) {
+	resetRegistryForTests()
+	defer resetRegistryForTests()
+	p := &mbcaaProvider{fetcher: NewFetcher(FetcherConfig{})}
+
+	if err := p.Configure(ProviderSettings{Enabled: true}); err != nil {
+		t.Fatalf("Configure: %v", err)
+	}
+	if !p.enabled.Load() {
+		t.Error("after Configure(Enabled: true), enabled = false")
+	}
+
+	if err := p.Configure(ProviderSettings{Enabled: false}); err != nil {
+		t.Fatalf("Configure: %v", err)
+	}
+	if p.enabled.Load() {
+		t.Error("after Configure(Enabled: false), enabled = true")
+	}
+}
+
+func TestMBCAAProvider_TestConnection_TreatsNotFoundAsSuccess(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "not found", http.StatusNotFound)
+	}))
+	defer srv.Close()
+
+	p := newMBCAAProviderForTest(t, srv.URL)
+	if err := p.TestConnection(context.Background()); err != nil {
+		t.Errorf("TestConnection err = %v, want nil (404 should be treated as connection ok)", err)
+	}
+}
+
+func TestMBCAAProvider_TestConnection_PropagatesTransient(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Error(w, "boom", http.StatusInternalServerError)
+	}))
+	defer srv.Close()
+
+	p := newMBCAAProviderForTest(t, srv.URL)
+	if err := p.TestConnection(context.Background()); err == nil {
+		t.Error("TestConnection err = nil, want non-nil for 500")
+	}
+}
+
+func TestNewMBCAAProviderFromConfig_ReplacesRegisteredFetcher(t *testing.T) {
+	resetRegistryForTests()
+	defer resetRegistryForTests()
+
+	// Pre-register with default config (mimics init() behaviour).
+	Register(&mbcaaProvider{fetcher: NewFetcher(FetcherConfig{})})
+
+	cfg := FetcherConfig{
+		UserAgent: "Minstrel/test-replaced (x)",
+		MinPeriod: 100 * time.Millisecond,
+	}
+	NewMBCAAProviderFromConfig(cfg)
+
+	p, err := ProviderByID("mbcaa")
+	if err != nil {
+		t.Fatalf("ProviderByID: %v", err)
+	}
+	mp := p.(*mbcaaProvider)
+	if mp.fetcher == nil {
+		t.Fatal("fetcher is nil after NewMBCAAProviderFromConfig")
+	}
+	// We can't peek the fetcher's UserAgent directly (it's an unexported
+	// field on FetcherConfig held inside Fetcher), but the side effect of
+	// ProviderByID returning the same registered instance with a swapped
+	// fetcher is what we're verifying here.
+}