From 392b8aa12a4312f2f195ea94cb7224cc2028b5dc Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 6 May 2026 12:43:54 -0400 Subject: [PATCH] feat(server/m7-cover-sources): SettingsService MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reconciles the compile-time provider registry with the cover_art_provider_settings table at boot, and on every admin update. Calls Configure() on each provider with the row data so runtime config (enabled, api_key) propagates immediately. Owns the cover_art_sources_meta.current_version stamp. UpdateProvider returns versionBumped=true only when the change altered the *enabled set* of providers — key rotations don't bump version (the recheck trigger is for "we have a new source to ask," not config rotations). Capability filtering: EnabledAlbumProviders / EnabledArtistProviders return only registered providers that (a) are enabled per DB and (b) implement the requested capability interface. Snapshots — callers don't mutate. ListProviderInfo builds the wire-shaped slice the admin GET handler returns. APIKeySet bool replaces echoing the key (credential never leaves the server). TestProvider dispatches to TestableProvider.TestConnection on the named provider, ErrNotTestable if the provider doesn't implement the capability, ErrProviderNotFound if not registered. UpdateProviderSettingsParams uses the sqlc-generated field names: Enabled bool (not *bool — the COALESCE is handled by passing the cached current value when patch.Enabled is nil), Column3 bool (apiKeyChanged flag), ApiKey *string (the api_key value — sqlc named this from the column, not Column4 as the task spec assumed). Tests gated on MINSTREL_TEST_DATABASE_URL: boot-inserts-defaults, flip-enabled-bumps-version, key-only-doesn't-bump, clear-key, TestProvider routing for testable / non-testable / missing, ListProviderInfo capability badges. Reuses newPool / discardLogger and fakeProvider / fakeAlbumProvider from enricher_test.go and provider_test.go (same package). --- internal/coverart/settings.go | 307 +++++++++++++++++++++++++++++ internal/coverart/settings_test.go | 233 ++++++++++++++++++++++ 2 files changed, 540 insertions(+) create mode 100644 internal/coverart/settings.go create mode 100644 internal/coverart/settings_test.go diff --git a/internal/coverart/settings.go b/internal/coverart/settings.go new file mode 100644 index 00000000..fbd57d3b --- /dev/null +++ b/internal/coverart/settings.go @@ -0,0 +1,307 @@ +package coverart + +import ( + "context" + "fmt" + "log/slog" + "sort" + "strings" + "sync" + + "github.com/jackc/pgx/v5/pgxpool" + + "git.fabledsword.com/bvandeusen/minstrel/internal/db/dbq" +) + +// SettingsService reconciles the registry of compiled-in providers +// with the cover_art_provider_settings table at boot, and on every +// admin-initiated update. It owns the cover_art_sources_meta +// singleton's current_version stamp, bumping it whenever the +// *enabled set* of providers changes. +type SettingsService struct { + pool *pgxpool.Pool + logger *slog.Logger + + mu sync.RWMutex + enabledIDs map[string]bool // provider_id → enabled + apiKeys map[string]string // provider_id → api_key (empty if NULL) + currentVersion int32 +} + +// ProviderUpdatePatch is the admin-PATCH body shape. nil pointers +// mean "leave unchanged"; non-nil means "set to this value (which +// may be empty string for api_key clearing)". +type ProviderUpdatePatch struct { + Enabled *bool + APIKey *string +} + +// ProviderInfo is the SettingsService's view of a provider for the +// admin GET handler. Combines registry metadata with DB settings. +type ProviderInfo struct { + ID string + DisplayName string + RequiresAPIKey bool + Supports []string // "album_cover", "artist_thumb", "artist_fanart" + Enabled bool + APIKeySet bool + DisplayOrder int32 + Testable bool +} + +// NewSettingsService boots the service: reads the DB rows, INSERTs +// defaults for any registered provider missing a row, calls +// Configure() on every registered provider with the row data, and +// loads the current_version. Returns the populated service. +func NewSettingsService(ctx context.Context, pool *pgxpool.Pool, logger *slog.Logger) (*SettingsService, error) { + s := &SettingsService{ + pool: pool, + logger: logger, + enabledIDs: map[string]bool{}, + apiKeys: map[string]string{}, + } + if err := s.reconcile(ctx); err != nil { + return nil, fmt.Errorf("settings service boot: %w", err) + } + return s, nil +} + +// reconcile reads the DB, INSERTs defaults for missing providers, +// calls Configure() on each registered provider, and loads the +// current_version into the cache. +func (s *SettingsService) reconcile(ctx context.Context) error { + q := dbq.New(s.pool) + + // Insert defaults for any registered provider missing a row. + registered := AllProviders() + for i, p := range registered { + if err := q.UpsertProviderSettings(ctx, dbq.UpsertProviderSettingsParams{ + ProviderID: p.ID(), + Enabled: p.DefaultEnabled(), + DisplayOrder: int32(i), + }); err != nil { + return fmt.Errorf("upsert provider %q: %w", p.ID(), err) + } + } + + // Read all rows back. + rows, err := q.ListProviderSettings(ctx) + if err != nil { + return fmt.Errorf("list provider settings: %w", err) + } + + // Build the lookup maps and call Configure on each registered + // provider with the corresponding row data. + settingsByID := map[string]dbq.CoverArtProviderSetting{} + for _, r := range rows { + settingsByID[r.ProviderID] = r + } + + s.mu.Lock() + defer s.mu.Unlock() + + s.enabledIDs = map[string]bool{} + s.apiKeys = map[string]string{} + for _, p := range registered { + row, ok := settingsByID[p.ID()] + if !ok { + s.logger.Warn("settings: missing row after upsert", "provider", p.ID()) + continue + } + key := "" + if row.ApiKey != nil { + key = *row.ApiKey + } + s.enabledIDs[p.ID()] = row.Enabled + s.apiKeys[p.ID()] = key + if err := p.Configure(ProviderSettings{Enabled: row.Enabled, APIKey: key}); err != nil { + s.logger.Warn("settings: provider Configure failed", + "provider", p.ID(), "err", err) + } + } + + v, err := q.GetCurrentSourcesVersion(ctx) + if err != nil { + return fmt.Errorf("get current sources version: %w", err) + } + s.currentVersion = v + return nil +} + +// EnabledAlbumProviders returns the enabled providers that implement +// AlbumCoverProvider, in registration order. Snapshot — caller must +// not mutate. +func (s *SettingsService) EnabledAlbumProviders() []AlbumCoverProvider { + s.mu.RLock() + defer s.mu.RUnlock() + var out []AlbumCoverProvider + for _, p := range AllProviders() { + if !s.enabledIDs[p.ID()] { + continue + } + if ap, ok := p.(AlbumCoverProvider); ok { + out = append(out, ap) + } + } + return out +} + +// EnabledArtistProviders is the parallel for ArtistArtProvider. +func (s *SettingsService) EnabledArtistProviders() []ArtistArtProvider { + s.mu.RLock() + defer s.mu.RUnlock() + var out []ArtistArtProvider + for _, p := range AllProviders() { + if !s.enabledIDs[p.ID()] { + continue + } + if ap, ok := p.(ArtistArtProvider); ok { + out = append(out, ap) + } + } + return out +} + +// CurrentVersion returns the current sources-set version (the stamp +// the enricher writes onto rows it processes). +func (s *SettingsService) CurrentVersion() int32 { + s.mu.RLock() + defer s.mu.RUnlock() + return s.currentVersion +} + +// UpdateProvider applies an admin-UI patch. Returns versionBumped=true +// if the change altered the *enabled set* (not on key changes). +// Re-runs the provider's Configure() with the new settings. +func (s *SettingsService) UpdateProvider(ctx context.Context, providerID string, patch ProviderUpdatePatch) (bool, error) { + p, err := ProviderByID(providerID) + if err != nil { + return false, err + } + + q := dbq.New(s.pool) + + // Compute current enabled-set signature before applying the patch. + s.mu.RLock() + beforeSig := enabledSetSignature(s.enabledIDs) + // Use the cached enabled value when the patch doesn't specify it, + // since the sqlc-generated Enabled field is bool (not *bool) and + // COALESCE($2, enabled) only works with a nullable param. Passing + // the current value when nil achieves the same "leave unchanged" + // semantics. + currentEnabled := s.enabledIDs[providerID] + s.mu.RUnlock() + + enabledVal := currentEnabled + if patch.Enabled != nil { + enabledVal = *patch.Enabled + } + + // Apply DB update. The sqlc-generated UpdateProviderSettingsParams + // uses Column3/ApiKey because the underlying SQL has $3::boolean + // (apiKeyChanged flag) and $4 (api_key value). The actual generated + // field for $4 is ApiKey *string. + apiKeyChanged := patch.APIKey != nil + if err := q.UpdateProviderSettings(ctx, dbq.UpdateProviderSettingsParams{ + ProviderID: providerID, + Enabled: enabledVal, + Column3: apiKeyChanged, + ApiKey: patch.APIKey, + }); err != nil { + return false, fmt.Errorf("update provider settings: %w", err) + } + + // Re-read rows + re-cache + Configure the changed provider. + rows, err := q.ListProviderSettings(ctx) + if err != nil { + return false, fmt.Errorf("re-list provider settings: %w", err) + } + + s.mu.Lock() + defer s.mu.Unlock() + + for _, r := range rows { + key := "" + if r.ApiKey != nil { + key = *r.ApiKey + } + s.enabledIDs[r.ProviderID] = r.Enabled + s.apiKeys[r.ProviderID] = key + if r.ProviderID == providerID { + if err := p.Configure(ProviderSettings{Enabled: r.Enabled, APIKey: key}); err != nil { + s.logger.Warn("provider Configure failed after update", + "provider", providerID, "err", err) + } + } + } + + afterSig := enabledSetSignature(s.enabledIDs) + if beforeSig == afterSig { + return false, nil + } + + newVersion, err := q.BumpSourcesVersion(ctx) + if err != nil { + return false, fmt.Errorf("bump sources version: %w", err) + } + s.currentVersion = newVersion + return true, nil +} + +// enabledSetSignature returns a deterministic string of the +// currently-enabled provider IDs. +func enabledSetSignature(enabledIDs map[string]bool) string { + ids := make([]string, 0, len(enabledIDs)) + for id, on := range enabledIDs { + if on { + ids = append(ids, id) + } + } + sort.Strings(ids) + return strings.Join(ids, ",") +} + +// TestProvider invokes TestConnection on the named provider if it +// implements TestableProvider. Returns ErrNotTestable if the +// provider doesn't, ErrProviderNotFound if not registered. +func (s *SettingsService) TestProvider(ctx context.Context, providerID string) error { + p, err := ProviderByID(providerID) + if err != nil { + return err + } + tp, ok := p.(TestableProvider) + if !ok { + return ErrNotTestable + } + return tp.TestConnection(ctx) +} + +// ListProviderInfo builds the wire-shaped slice for GET +// /api/admin/cover-sources. Returns providers in registration order. +func (s *SettingsService) ListProviderInfo() []ProviderInfo { + s.mu.RLock() + defer s.mu.RUnlock() + + var out []ProviderInfo + for i, p := range AllProviders() { + info := ProviderInfo{ + ID: p.ID(), + DisplayName: p.DisplayName(), + RequiresAPIKey: p.RequiresAPIKey(), + Enabled: s.enabledIDs[p.ID()], + APIKeySet: s.apiKeys[p.ID()] != "", + DisplayOrder: int32(i), + } + if _, ok := p.(AlbumCoverProvider); ok { + info.Supports = append(info.Supports, "album_cover") + } + if _, ok := p.(ArtistArtProvider); ok { + info.Supports = append(info.Supports, "artist_thumb", "artist_fanart") + } + if _, ok := p.(TestableProvider); ok { + info.Testable = true + } + out = append(out, info) + } + return out +} diff --git a/internal/coverart/settings_test.go b/internal/coverart/settings_test.go new file mode 100644 index 00000000..3b250e82 --- /dev/null +++ b/internal/coverart/settings_test.go @@ -0,0 +1,233 @@ +package coverart + +import ( + "context" + "errors" + "testing" +) + +// fakeArtistProvider opts into ArtistArtProvider for capability tests. +type fakeArtistProvider struct { + fakeProvider +} + +func (f *fakeArtistProvider) FetchArtistArt(_ context.Context, _ string) (thumb, fanart []byte, err error) { + return []byte("t"), []byte("f"), nil +} + +// fakeTestableAlbumProvider implements TestableProvider for the +// TestProvider test. +type fakeTestableAlbumProvider struct { + fakeAlbumProvider + testErr error +} + +func (f *fakeTestableAlbumProvider) TestConnection(_ context.Context) error { + return f.testErr +} + +func TestSettingsService_BootInsertsDefaultRows(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + Register(&fakeAlbumProvider{fakeProvider: fakeProvider{id: "fake-album", display: "Fake Album", defaultOn: true}}) + Register(&fakeArtistProvider{fakeProvider: fakeProvider{id: "fake-artist", display: "Fake Artist", defaultOn: false}}) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatalf("NewSettingsService: %v", err) + } + + // Both providers should now have rows; only fake-album is enabled. + enabled := s.EnabledAlbumProviders() + if len(enabled) != 1 { + t.Fatalf("EnabledAlbumProviders = %d, want 1", len(enabled)) + } + if enabled[0].ID() != "fake-album" { + t.Errorf("enabled[0].ID = %q, want fake-album", enabled[0].ID()) + } + + // fake-artist defaultOn=false, so it should NOT be in EnabledArtistProviders. + artists := s.EnabledArtistProviders() + if len(artists) != 0 { + t.Errorf("EnabledArtistProviders = %d, want 0", len(artists)) + } + + if s.CurrentVersion() != 1 { + t.Errorf("CurrentVersion = %d, want 1", s.CurrentVersion()) + } +} + +func TestSettingsService_UpdateProvider_FlippingEnabledBumpsVersion(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + Register(&fakeAlbumProvider{fakeProvider: fakeProvider{id: "fake-album", defaultOn: true}}) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatal(err) + } + + startVersion := s.CurrentVersion() + + disabled := false + bumped, err := s.UpdateProvider(context.Background(), "fake-album", ProviderUpdatePatch{Enabled: &disabled}) + if err != nil { + t.Fatalf("UpdateProvider: %v", err) + } + if !bumped { + t.Error("versionBumped = false, want true after disabling") + } + if s.CurrentVersion() <= startVersion { + t.Errorf("CurrentVersion = %d, want > %d", s.CurrentVersion(), startVersion) + } + + enabled := s.EnabledAlbumProviders() + if len(enabled) != 0 { + t.Errorf("after disable: EnabledAlbumProviders = %d, want 0", len(enabled)) + } +} + +func TestSettingsService_UpdateProvider_KeyOnlyDoesNotBumpVersion(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + Register(&fakeAlbumProvider{fakeProvider: fakeProvider{id: "fake-album", defaultOn: true}}) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatal(err) + } + + startVersion := s.CurrentVersion() + + newKey := "abc123" + bumped, err := s.UpdateProvider(context.Background(), "fake-album", ProviderUpdatePatch{APIKey: &newKey}) + if err != nil { + t.Fatalf("UpdateProvider: %v", err) + } + if bumped { + t.Error("versionBumped = true, want false for key-only change") + } + if s.CurrentVersion() != startVersion { + t.Errorf("CurrentVersion changed: %d → %d, want unchanged", startVersion, s.CurrentVersion()) + } + + infos := s.ListProviderInfo() + var info ProviderInfo + for _, i := range infos { + if i.ID == "fake-album" { + info = i + break + } + } + if !info.APIKeySet { + t.Error("APIKeySet = false, want true after setting key") + } +} + +func TestSettingsService_UpdateProvider_ClearKey(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + Register(&fakeAlbumProvider{fakeProvider: fakeProvider{id: "fake-album", defaultOn: true}}) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatal(err) + } + + // Set a key, then clear it. + setKey := "abc" + if _, err := s.UpdateProvider(context.Background(), "fake-album", ProviderUpdatePatch{APIKey: &setKey}); err != nil { + t.Fatal(err) + } + emptyKey := "" + if _, err := s.UpdateProvider(context.Background(), "fake-album", ProviderUpdatePatch{APIKey: &emptyKey}); err != nil { + t.Fatal(err) + } + + infos := s.ListProviderInfo() + for _, i := range infos { + if i.ID == "fake-album" && i.APIKeySet { + t.Error("APIKeySet = true after clear, want false") + } + } +} + +func TestSettingsService_TestProvider_Routing(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + plain := &fakeAlbumProvider{fakeProvider: fakeProvider{id: "plain", defaultOn: true}} + testable := &fakeTestableAlbumProvider{ + fakeAlbumProvider: fakeAlbumProvider{fakeProvider: fakeProvider{id: "testable", defaultOn: true}}, + testErr: nil, + } + Register(plain) + Register(testable) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatal(err) + } + + // Testable returns nil → success. + if err := s.TestProvider(context.Background(), "testable"); err != nil { + t.Errorf("TestProvider(testable) = %v, want nil", err) + } + + // Plain doesn't implement TestableProvider → ErrNotTestable. + if err := s.TestProvider(context.Background(), "plain"); !errors.Is(err, ErrNotTestable) { + t.Errorf("TestProvider(plain) = %v, want ErrNotTestable", err) + } + + // Missing → ErrProviderNotFound. + if err := s.TestProvider(context.Background(), "missing"); !errors.Is(err, ErrProviderNotFound) { + t.Errorf("TestProvider(missing) = %v, want ErrProviderNotFound", err) + } +} + +func TestSettingsService_ListProviderInfo_Capabilities(t *testing.T) { + pool := newPool(t) + resetRegistryForTests() + defer resetRegistryForTests() + + plain := &fakeProvider{id: "plain", defaultOn: true} + album := &fakeAlbumProvider{fakeProvider: fakeProvider{id: "album-only", defaultOn: true}} + artist := &fakeArtistProvider{fakeProvider: fakeProvider{id: "artist-only", defaultOn: true}} + Register(plain) + Register(album) + Register(artist) + + s, err := NewSettingsService(context.Background(), pool, discardLogger()) + if err != nil { + t.Fatal(err) + } + + infos := s.ListProviderInfo() + if len(infos) != 3 { + t.Fatalf("ListProviderInfo len = %d, want 3", len(infos)) + } + + byID := map[string]ProviderInfo{} + for _, i := range infos { + byID[i.ID] = i + } + + if got := byID["plain"].Supports; len(got) != 0 { + t.Errorf("plain.Supports = %v, want empty", got) + } + if got := byID["album-only"].Supports; len(got) != 1 || got[0] != "album_cover" { + t.Errorf("album-only.Supports = %v, want [album_cover]", got) + } + if got := byID["artist-only"].Supports; len(got) != 2 || got[0] != "artist_thumb" || got[1] != "artist_fanart" { + t.Errorf("artist-only.Supports = %v, want [artist_thumb, artist_fanart]", got) + } +}