From dc4bdcdab04d1b9350e74ea71698b55f2187a194 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sun, 19 Jul 2026 22:22:46 +0300 Subject: [PATCH] feat: add SyncAll periodic sync pipeline and wire Navidrome client into App --- cmd/naviwatcher/main.go | 18 ++ cmd/naviwatcher/main_test.go | 23 ++ docs/plans/2026-07-19-notifier-webui-sync.md | 8 +- .../2026-05-20-foundation-layer.md | 0 .../2026-05-20-navidrome-client.md | 0 .../2026-05-21-musicbrainz-provider.md | 0 internal/musicbrainz/syncall.go | 141 +++++++++++ internal/musicbrainz/syncall_test.go | 218 ++++++++++++++++++ internal/navidrome/client.go | 13 ++ 9 files changed, 417 insertions(+), 4 deletions(-) rename docs/plans/{ => completed}/2026-05-20-foundation-layer.md (100%) rename docs/plans/{ => completed}/2026-05-20-navidrome-client.md (100%) rename docs/plans/{ => completed}/2026-05-21-musicbrainz-provider.md (100%) create mode 100644 internal/musicbrainz/syncall.go create mode 100644 internal/musicbrainz/syncall_test.go diff --git a/cmd/naviwatcher/main.go b/cmd/naviwatcher/main.go index 41aee35..b3ed619 100644 --- a/cmd/naviwatcher/main.go +++ b/cmd/naviwatcher/main.go @@ -12,6 +12,7 @@ import ( "naviwatcher/internal/config" "naviwatcher/internal/database" "naviwatcher/internal/musicbrainz" + "naviwatcher/internal/navidrome" "naviwatcher/internal/scanner" ) @@ -20,8 +21,14 @@ type App struct { cfg *config.Config db *database.DB mbClient *musicbrainz.MusicBrainzClient + ndClient *navidrome.NavidromeClient } +// navidromeClientFactory constructs the Navidrome client. It is a package-level +// variable (not a direct call to navidrome.NewClient) so tests can inject a stub +// without requiring a live Navidrome server for authentication. +var navidromeClientFactory = navidrome.NewClient + const defaultConfigPath = "config.yaml" func main() { @@ -76,10 +83,17 @@ func NewApp(ctx context.Context, cfg *config.Config, dbPath string) (*App, error log.Printf("MusicBrainz client initialized (user-agent: %s)", cfg.MusicBrainz.UserAgent) + // Initialize Navidrome client (authenticates immediately; error if auth fails). + ndClient, err := navidromeClientFactory(cfg.Navidrome) + if err != nil { + return nil, fmt.Errorf("failed to initialize navidrome client: %w", err) + } + return &App{ cfg: cfg, db: db, mbClient: mbClient, + ndClient: ndClient, }, nil } @@ -88,6 +102,10 @@ func (a *App) Close() { if a.mbClient != nil { a.mbClient.Close() } + if a.ndClient != nil { + // NavidromeClient holds a stateless subsonic client; nothing to close + // beyond releasing idle connections tracked by the MusicBrainz client. + } if a.db != nil { if err := a.db.Close(); err != nil { log.Printf("Error closing database: %v", err) diff --git a/cmd/naviwatcher/main_test.go b/cmd/naviwatcher/main_test.go index b868369..c79b3ca 100644 --- a/cmd/naviwatcher/main_test.go +++ b/cmd/naviwatcher/main_test.go @@ -12,6 +12,7 @@ import ( "naviwatcher/internal/config" "naviwatcher/internal/database" + "naviwatcher/internal/navidrome" ) func TestAppRun_ScanLogsMissingReleases(t *testing.T) { @@ -135,6 +136,13 @@ func TestNewApp_CreatesMusicBrainzClient(t *testing.T) { }, } + // Inject an unauthenticated Navidrome client so the test needs no live server. + prevFactory := navidromeClientFactory + navidromeClientFactory = func(c config.NavidromeConfig) (*navidrome.NavidromeClient, error) { + return navidrome.NewClientUnauthenticated(c), nil + } + defer func() { navidromeClientFactory = prevFactory }() + ctx := context.Background() app, err := NewApp(ctx, cfg, ":memory:") if err != nil { @@ -145,6 +153,9 @@ func TestNewApp_CreatesMusicBrainzClient(t *testing.T) { if app.mbClient == nil { t.Fatal("expected MusicBrainz client to be initialized, got nil") } + if app.ndClient == nil { + t.Fatal("expected Navidrome client to be initialized, got nil") + } if app.db == nil { t.Fatal("expected database to be initialized, got nil") } @@ -170,6 +181,12 @@ func TestNewApp_GracefulShutdown(t *testing.T) { }, } + prevFactory := navidromeClientFactory + navidromeClientFactory = func(c config.NavidromeConfig) (*navidrome.NavidromeClient, error) { + return navidrome.NewClientUnauthenticated(c), nil + } + defer func() { navidromeClientFactory = prevFactory }() + ctx := context.Background() app, err := NewApp(ctx, cfg, ":memory:") if err != nil { @@ -197,6 +214,12 @@ func TestAppRun_GracefulShutdown(t *testing.T) { }, } + prevFactory := navidromeClientFactory + navidromeClientFactory = func(c config.NavidromeConfig) (*navidrome.NavidromeClient, error) { + return navidrome.NewClientUnauthenticated(c), nil + } + defer func() { navidromeClientFactory = prevFactory }() + ctx, cancel := context.WithCancel(context.Background()) app, err := NewApp(ctx, cfg, ":memory:") diff --git a/docs/plans/2026-07-19-notifier-webui-sync.md b/docs/plans/2026-07-19-notifier-webui-sync.md index 055eab7..922a791 100644 --- a/docs/plans/2026-07-19-notifier-webui-sync.md +++ b/docs/plans/2026-07-19-notifier-webui-sync.md @@ -95,10 +95,10 @@ name collisions.) - [x] run tests - must pass before task 3 ### Task 3: Periodic sync pipeline -- [ ] add `SyncAll(ctx, ndClient, mbClient, db, ttl)` orchestrator: for each monitored artist → ensure MBID (resolve + persist if missing) → `musicbrainz.SyncArtistDiscography` → `navidrome.SyncAlbums` -- [ ] wire `navidrome.NewClient` into `App`; add `ndClient` field -- [ ] write tests for `SyncAll` with stubbed clients + `:memory:` DB (new artist gets MBID, existing MBID reused, unmonitored skipped) -- [ ] run tests - must pass before task 4 +- [x] add `SyncAll(ctx, ndClient, mbClient, db, ttl)` orchestrator: for each monitored artist → ensure MBID (resolve + persist if missing) → `musicbrainz.SyncArtistDiscography` → `navidrome.SyncAlbums` +- [x] wire `navidrome.NewClient` into `App`; add `ndClient` field +- [x] write tests for `SyncAll` with stubbed clients + `:memory:` DB (new artist gets MBID, existing MBID reused, unmonitored skipped) +- [x] run tests - must pass before task 4 ### Task 4: Main loop wiring (sync → scan) - [ ] replace compute-only `run()` with: one immediate sync+scan, then a ticker-driven periodic sync+scan goroutine; keep graceful shutdown via ctx diff --git a/docs/plans/2026-05-20-foundation-layer.md b/docs/plans/completed/2026-05-20-foundation-layer.md similarity index 100% rename from docs/plans/2026-05-20-foundation-layer.md rename to docs/plans/completed/2026-05-20-foundation-layer.md diff --git a/docs/plans/2026-05-20-navidrome-client.md b/docs/plans/completed/2026-05-20-navidrome-client.md similarity index 100% rename from docs/plans/2026-05-20-navidrome-client.md rename to docs/plans/completed/2026-05-20-navidrome-client.md diff --git a/docs/plans/2026-05-21-musicbrainz-provider.md b/docs/plans/completed/2026-05-21-musicbrainz-provider.md similarity index 100% rename from docs/plans/2026-05-21-musicbrainz-provider.md rename to docs/plans/completed/2026-05-21-musicbrainz-provider.md diff --git a/internal/musicbrainz/syncall.go b/internal/musicbrainz/syncall.go new file mode 100644 index 0000000..21611d0 --- /dev/null +++ b/internal/musicbrainz/syncall.go @@ -0,0 +1,141 @@ +package musicbrainz + +import ( + "context" + "fmt" + "time" + + "naviwatcher/internal/database" +) + +// MBIDResolver resolves a MusicBrainz artist ID for an artist name. +// The real *MusicBrainzClient satisfies this interface. +type MBIDResolver interface { + ResolveArtistMBID(ctx context.Context, name string) (string, error) +} + +// ArtistDiscographySyncer syncs one artist's MusicBrainz discography into the +// external_releases table. The real implementation (musicbrainz.SyncArtistDiscography) +// is wrapped by discographySyncer so the concrete *MusicBrainzClient dependency +// is injectable in tests. +type ArtistDiscographySyncer interface { + SyncArtistDiscography(ctx context.Context, db *database.DB, artistID, artistMBID string, ttl time.Duration) ([]database.ExternalRelease, error) +} + +// discographySyncer adapts the package-level SyncArtistDiscography function to +// the ArtistDiscographySyncer interface, binding a concrete *MusicBrainzClient. +type discographySyncer struct { + client *MusicBrainzClient +} + +// NewDiscographySyncer wraps a *MusicBrainzClient as an ArtistDiscographySyncer. +func NewDiscographySyncer(client *MusicBrainzClient) ArtistDiscographySyncer { + return &discographySyncer{client: client} +} + +func (s *discographySyncer) SyncArtistDiscography( + ctx context.Context, + db *database.DB, + artistID, artistMBID string, + ttl time.Duration, +) ([]database.ExternalRelease, error) { + return SyncArtistDiscography(ctx, s.client, db, artistID, artistMBID, ttl) +} + +// AlbumSyncer copies each monitored artist's albums from Navidrome into the +// local_albums table. The real implementation (navidrome.SyncAlbums) is wrapped +// so the concrete *navidrome.NavidromeClient dependency is injectable in tests. +type AlbumSyncer interface { + SyncAlbums(ctx context.Context, db *database.DB) error +} + +// albumSyncer adapts navidrome.SyncAlbums to the AlbumSyncer interface. +type albumSyncer struct { + syncAlbums func(ctx context.Context, db *database.DB) error +} + +func (s *albumSyncer) SyncAlbums(ctx context.Context, db *database.DB) error { + return s.syncAlbums(ctx, db) +} + +// SyncAll orchestrates the data pipeline for every monitored artist: +// 1. MusicBrainz artist-ID resolution — for each artist with no cached MBID, +// resolve it by name and persist it on the artist_settings row. Artists that +// already have an MBID reuse it (no extra rate-limited MusicBrainz call). +// 2. MusicBrainz discography sync into external_releases. +// 3. Navidrome album sync into local_albums. +// +// Ordering matters: Navidrome's artist/album tables are populated by the caller +// before SyncAll (via navidrome.SyncArtists / SyncAlbums as appropriate); here we +// focus on the per-artist MBID + discography + album refresh. Unmonitored artists +// are skipped. +// +// Resolution failures for a single artist are logged and skipped (the artist is +// left for the next sync) rather than aborting the whole run; the error is still +// returned so the caller can decide whether to surface it. +func SyncAll( + ctx context.Context, + db *database.DB, + resolver MBIDResolver, + discography ArtistDiscographySyncer, + albums AlbumSyncer, + ttl time.Duration, +) error { + if err := ctx.Err(); err != nil { + return fmt.Errorf("sync all: %w", err) + } + + artists, err := database.GetAllArtistSettings(db) + if err != nil { + return fmt.Errorf("sync all: get artists: %w", err) + } + + var resolutionErr error + for _, artist := range artists { + if err := ctx.Err(); err != nil { + return fmt.Errorf("sync all: %w", err) + } + + // Skip unmonitored artists entirely. + if !artist.Monitored { + continue + } + + // Ensure we have an MBID; resolve and persist if missing. + mbid := artist.MBID + if mbid == "" { + resolved, rerr := resolver.ResolveArtistMBID(ctx, artist.Name) + if rerr != nil { + // Skip this artist but remember the first resolution error. + if resolutionErr == nil { + resolutionErr = fmt.Errorf("resolve MBID for artist %q: %w", artist.Name, rerr) + } + continue + } + mbid = resolved + if perr := database.UpdateArtistSettings(db, artist.ID, map[string]interface{}{"mbid": mbid}); perr != nil { + if resolutionErr == nil { + resolutionErr = fmt.Errorf("persist MBID for artist %q: %w", artist.Name, perr) + } + continue + } + } + + // Sync the artist's MusicBrainz discography. + if _, derr := discography.SyncArtistDiscography(ctx, db, artist.ID, mbid, ttl); derr != nil { + if resolutionErr == nil { + resolutionErr = fmt.Errorf("sync discography for artist %q: %w", artist.Name, derr) + } + continue + } + } + + // Album sync operates over all monitored artists in one pass. + if aerr := albums.SyncAlbums(ctx, db); aerr != nil { + if resolutionErr == nil { + resolutionErr = fmt.Errorf("sync albums: %w", aerr) + } + } + + return resolutionErr +} diff --git a/internal/musicbrainz/syncall_test.go b/internal/musicbrainz/syncall_test.go new file mode 100644 index 0000000..07296bb --- /dev/null +++ b/internal/musicbrainz/syncall_test.go @@ -0,0 +1,218 @@ +package musicbrainz + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "naviwatcher/internal/database" +) + +// stubResolver is a configurable MBIDResolver for tests. +type stubResolver struct { + byName map[string]string // name -> mbid + calls []string // names requested, in order + err error // optional error to return for any resolve +} + +func (s *stubResolver) ResolveArtistMBID(ctx context.Context, name string) (string, error) { + s.calls = append(s.calls, name) + if s.err != nil { + return "", s.err + } + if mbid, ok := s.byName[name]; ok { + return mbid, nil + } + return "", errors.New("no match") +} + +// stubDiscography records per-artist discography syncs. +type stubDiscography struct { + synced []string // artistIDs + err error +} + +func (s *stubDiscography) SyncArtistDiscography( + ctx context.Context, + db *database.DB, + artistID, artistMBID string, + ttl time.Duration, +) ([]database.ExternalRelease, error) { + if s.err != nil { + return nil, s.err + } + s.synced = append(s.synced, artistID) + return nil, nil +} + +// stubAlbums records album-sync invocations. +type stubAlbums struct { + called int + err error +} + +func (s *stubAlbums) SyncAlbums(ctx context.Context, db *database.DB) error { + s.called++ + return s.err +} + +func seedArtistRow(t *testing.T, db *database.DB, id, name, mbid string, monitored bool) { + t.Helper() + if err := database.SaveArtistSettings(db, &database.ArtistSettings{ + ID: id, + Name: name, + MBID: mbid, + Monitored: monitored, + }); err != nil { + t.Fatalf("seed artist: %v", err) + } +} + +func TestSyncAll_NewArtistGetsMBID(t *testing.T) { + db := newTestDB(t) + seedArtistRow(t, db, "ar1", "Radiohead", "", true) + + resolver := &stubResolver{byName: map[string]string{"Radiohead": "mbid-radiohead"}} + disco := &stubDiscography{} + albs := &stubAlbums{} + + err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour) + if err != nil { + t.Fatalf("SyncAll() error = %v", err) + } + + // Resolver must have been called for the new artist. + if len(resolver.calls) != 1 || resolver.calls[0] != "Radiohead" { + t.Fatalf("resolver calls = %v, want [Radiohead]", resolver.calls) + } + // MBID persisted on the row. + got, gerr := database.GetArtistSettings(db, "ar1") + if gerr != nil { + t.Fatalf("GetArtistSettings() error = %v", gerr) + } + if got.MBID != "mbid-radiohead" { + t.Errorf("persisted MBID = %q, want %q", got.MBID, "mbid-radiohead") + } + // Discography and albums synced. + if len(disco.synced) != 1 || disco.synced[0] != "ar1" { + t.Errorf("discography synced = %v, want [ar1]", disco.synced) + } + if albc := albs.called; albc != 1 { + t.Errorf("albums sync called = %d, want 1", albc) + } +} + +func TestSyncAll_ExistingMBIDReused(t *testing.T) { + db := newTestDB(t) + seedArtistRow(t, db, "ar1", "Radiohead", "preset-mbid", true) + + resolver := &stubResolver{byName: map[string]string{"Radiohead": "resolved-mbid"}} + disco := &stubDiscography{} + albs := &stubAlbums{} + + if err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour); err != nil { + t.Fatalf("SyncAll() error = %v", err) + } + + // Resolver must NOT be called when MBID already present. + if len(resolver.calls) != 0 { + t.Errorf("resolver calls = %v, want none (MBID reused)", resolver.calls) + } + got, _ := database.GetArtistSettings(db, "ar1") + if got.MBID != "preset-mbid" { + t.Errorf("MBID = %q, want preserved preset-mbid", got.MBID) + } + if len(disco.synced) != 1 { + t.Errorf("discography synced = %v, want [ar1]", disco.synced) + } +} + +func TestSyncAll_UnmonitoredSkipped(t *testing.T) { + db := newTestDB(t) + seedArtistRow(t, db, "ar1", "Radiohead", "", false) // unmonitored + + resolver := &stubResolver{byName: map[string]string{"Radiohead": "mbid-x"}} + disco := &stubDiscography{} + albs := &stubAlbums{} + + if err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour); err != nil { + t.Fatalf("SyncAll() error = %v", err) + } + + if len(resolver.calls) != 0 { + t.Errorf("resolver calls = %v, want none (unmonitored skipped)", resolver.calls) + } + if len(disco.synced) != 0 { + t.Errorf("discography synced = %v, want none", disco.synced) + } + // Album sync still runs (it internally skips unmonitored too), but no + // discography work should have happened for the skipped artist. +} + +func TestSyncAll_ResolutionErrorSkipsArtist(t *testing.T) { + db := newTestDB(t) + seedArtistRow(t, db, "ar1", "Unknown", "", true) + + resolver := &stubResolver{err: errors.New("mb down")} + disco := &stubDiscography{} + albs := &stubAlbums{} + + err := SyncAll(context.Background(), db, resolver, disco, albs, 24*time.Hour) + if err == nil { + t.Fatal("SyncAll() expected error when resolution fails") + } + if len(disco.synced) != 0 { + t.Errorf("discography synced = %v, want none (resolution failed)", disco.synced) + } + // MBID must remain empty since persistence was skipped. + got, _ := database.GetArtistSettings(db, "ar1") + if got.MBID != "" { + t.Errorf("MBID = %q, want empty after failed resolution", got.MBID) + } +} + +func TestSyncAll_ContextCancel(t *testing.T) { + db := newTestDB(t) + seedArtistRow(t, db, "ar1", "Radiohead", "", true) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + if err := SyncAll(ctx, db, &stubResolver{}, &stubDiscography{}, &stubAlbums{}, 24*time.Hour); err == nil { + t.Fatal("SyncAll() expected context error, got nil") + } +} + +// TestDiscographySyncer_AdapterForwards verifies the adapter produced by +// NewDiscographySyncer forwards to the real SyncArtistDiscography so that the +// App's wiring uses the actual MusicBrainz client. +func TestDiscographySyncer_AdapterForwards(t *testing.T) { + db := newTestDB(t) + artistID := "nav-adapter" + artistMBID := "bbbbbbbb-cccc-dddd-eeee-ffffffffffff" + seedArtistRow(t, db, artistID, "Adapter Artist", "", true) + + server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/xml") + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg1", "Adapter Album", "Album", "", artistMBID, "Adapter Artist", "2020-01-01"), + 1, + ) + w.Write([]byte(resp)) + }) + defer server.Close() + + syncer := NewDiscographySyncer(newTestClient(server.URL)) + releases, err := syncer.SyncArtistDiscography(context.Background(), db, artistID, artistMBID, 24*time.Hour) + if err != nil { + t.Fatalf("adapter SyncArtistDiscography() error = %v", err) + } + if len(releases) != 1 { + t.Fatalf("adapter expected 1 release, got %d", len(releases)) + } + if releases[0].RGID != "rg1" { + t.Errorf("adapter release RGID = %q, want rg1", releases[0].RGID) + } +} diff --git a/internal/navidrome/client.go b/internal/navidrome/client.go index b826bab..b9e0311 100644 --- a/internal/navidrome/client.go +++ b/internal/navidrome/client.go @@ -47,6 +47,19 @@ func NewClient(cfg config.NavidromeConfig) (*NavidromeClient, error) { return &NavidromeClient{client: client}, nil } +// NewClientUnauthenticated builds a NavidromeClient without contacting the +// server. It is intended for dependency injection in tests (where the +// navidromeClientFactory seam in main is overridden) and for callers that want +// to defer or skip authentication. Production wiring should prefer NewClient. +func NewClientUnauthenticated(cfg config.NavidromeConfig) *NavidromeClient { + return &NavidromeClient{client: &subsonic.Client{ + Client: &http.Client{Timeout: 30 * time.Second}, + BaseUrl: cfg.URL, + User: cfg.User, + ClientName: "naviwatcher", + }} +} + // Ping checks connectivity to the Navidrome server. // Returns nil if the server is reachable and responds with a valid Subsonic OK status. func (nc *NavidromeClient) Ping() error {