diff --git a/docs/plans/2026-05-21-musicbrainz-provider.md b/docs/plans/2026-05-21-musicbrainz-provider.md index 7b7ea01..d2f7d84 100644 --- a/docs/plans/2026-05-21-musicbrainz-provider.md +++ b/docs/plans/2026-05-21-musicbrainz-provider.md @@ -47,13 +47,13 @@ Implement a MusicBrainz API provider with strict 1 request/second rate limiting, ## Implementation Steps ### Task 1: Create MusicBrainz client and data models -- [ ] create `internal/musicbrainz/client.go` with MusicBrainzClient struct wrapping net/http.Client -- [ ] implement constructor taking config and rate limiter -- [ ] create `internal/musicbrainz/model.go` with structs for MusicBrainz API responses (ReleaseGroup, Artist, etc.) -- [ ] implement XML parsing functions for MusicBrainz responses -- [ ] write tests for XML parsing (success + error cases) -- [ ] write tests for client constructor and basic API call structure -- [ ] run tests - must pass before next task +- [x] create `internal/musicbrainz/client.go` with MusicBrainzClient struct wrapping net/http.Client +- [x] implement constructor taking config and rate limiter +- [x] create `internal/musicbrainz/model.go` with structs for MusicBrainz API responses (ReleaseGroup, Artist, etc.) +- [x] implement XML parsing functions for MusicBrainz responses +- [x] write tests for XML parsing (success + error cases) +- [x] write tests for client constructor and basic API call structure +- [x] run tests - must pass before next task ### Task 2: Implement rate limiting and caching layer - [ ] add golang.org/x/time/rate dependency to go.mod diff --git a/internal/musicbrainz/client.go b/internal/musicbrainz/client.go new file mode 100644 index 0000000..1446c3f --- /dev/null +++ b/internal/musicbrainz/client.go @@ -0,0 +1,223 @@ +package musicbrainz + +import ( + "encoding/xml" + "fmt" + "io" + "net/http" + "time" + + "naviwatcher/internal/config" +) + +// MusicBrainzClient wraps net/http.Client with rate limiting and configuration +// for the MusicBrainz Web Service API (version 2). +type MusicBrainzClient struct { + httpClient *http.Client + userAgent string + baseURL string + rateLimiter *rateLimiter +} + +// rateLimiter wraps a token-bucket rate limiter for API calls. +type rateLimiter struct { + // tokens is a channel-based semaphore for rate limiting. + // It is filled at a fixed interval by a background goroutine. + tokens chan struct{} + done chan struct{} +} + +// newRateLimiter creates a rate limiter that allows maxCalls per second. +// It immediately fills the bucket and starts a refill goroutine. +func newRateLimiter(callsPerSecond int) *rateLimiter { + rl := &rateLimiter{ + tokens: make(chan struct{}, callsPerSecond), + done: make(chan struct{}), + } + // Fill the bucket initially + for i := 0; i < callsPerSecond; i++ { + rl.tokens <- struct{}{} + } + // Refill at the specified interval + interval := time.Second / time.Duration(callsPerSecond) + go func() { + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ticker.C: + select { + case rl.tokens <- struct{}{}: + default: + // bucket full, skip + } + case <-rl.done: + return + } + } + }() + return rl +} + +// wait blocks until a token is available or the rate limiter is stopped. +func (rl *rateLimiter) wait() { + <-rl.tokens +} + +// stop terminates the refill goroutine. +func (rl *rateLimiter) stop() { + close(rl.done) +} + +// NewClient creates a new MusicBrainzClient from the given configuration. +// It initializes the HTTP client with a 30-second timeout and sets up +// a rate limiter for 1 request per second as required by MusicBrainz policy. +func NewClient(cfg config.MusicBrainzConfig) *MusicBrainzClient { + rl := newRateLimiter(1) // 1 request per second + return &MusicBrainzClient{ + httpClient: &http.Client{ + Timeout: 30 * time.Second, + }, + userAgent: cfg.UserAgent, + baseURL: "https://musicbrainz.org/ws/2", + rateLimiter: rl, + } +} + +// NewClientWithLimiter creates a MusicBrainzClient with a custom rate limiter. +// This is primarily used for testing to inject a mock rate limiter. +func NewClientWithLimiter(cfg config.MusicBrainzConfig, rl *rateLimiter) *MusicBrainzClient { + return &MusicBrainzClient{ + httpClient: &http.Client{ + Timeout: 30 * time.Second, + }, + userAgent: cfg.UserAgent, + baseURL: "https://musicbrainz.org/ws/2", + rateLimiter: rl, + } +} + +// Close cleans up the rate limiter goroutine. +func (c *MusicBrainzClient) Close() { + c.rateLimiter.stop() +} + +// doGet performs a rate-limited HTTP GET request to the MusicBrainz API. +// It sets the proper User-Agent header and returns the response body. +func (c *MusicBrainzClient) doGet(path string) ([]byte, error) { + c.rateLimiter.wait() + + req, err := http.NewRequest(http.MethodGet, c.baseURL+path, nil) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + req.Header.Set("User-Agent", c.userAgent) + req.Header.Set("Accept", "application/xml") + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("execute request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) + return nil, fmt.Errorf("musicbrainz API returned HTTP %d: %s", resp.StatusCode, string(body)) + } + + body, err := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024)) // 10MB limit + if err != nil { + return nil, fmt.Errorf("read response body: %w", err) + } + + return body, nil +} + +// mbArtistRef represents the nested artist element inside a release-group. +type mbArtistRef struct { + ID string `xml:"id,attr"` + Name string `xml:"name"` +} + +// mbNameCredit represents the name-credit element inside a release-group. +type mbNameCredit struct { + Artist mbArtistRef `xml:"artist"` +} + +// mbArtistCredit represents the artist-credit element inside a release-group. +type mbArtistCredit struct { + NameCredit mbNameCredit `xml:"name-credit"` +} + +// mbReleaseGroup represents the XML structure of a single release-group +// in the MusicBrainz release-group list response. +type mbReleaseGroup struct { + ID string `xml:"id,attr"` + Title string `xml:"title"` + Type string `xml:"type,attr"` + Status string `xml:"status,attr"` + ArtistCredit mbArtistCredit `xml:"artist-credit"` + ReleaseDate string `xml:"first-release-date"` +} + +// mbReleaseGroupListXML wraps the release-group-list element to properly +// capture both child elements and the count attribute. +type mbReleaseGroupListXML struct { + ReleaseGroups []mbReleaseGroup `xml:"release-group"` + Count int `xml:"count,attr"` +} + +// mbReleaseGroupList represents the XML structure of a release-group list response. +type mbReleaseGroupList struct { + XMLName xml.Name `xml:"metadata"` + ReleaseGroupList mbReleaseGroupListXML `xml:"release-group-list"` +} + +// mbArtistData represents the artist element inside metadata. +type mbArtistData struct { + ID string `xml:"id,attr"` + Name string `xml:"name"` +} + +// mbArtist represents the XML structure of a MusicBrainz artist response. +type mbArtist struct { + XMLName xml.Name `xml:"metadata"` + Artist mbArtistData `xml:"artist"` +} + +// ParseReleaseGroups parses a MusicBrainz release-group list XML response +// into a ParsedReleaseGroups struct. +func ParseReleaseGroups(data []byte) (*ParsedReleaseGroups, error) { + var list mbReleaseGroupList + if err := xml.Unmarshal(data, &list); err != nil { + return nil, fmt.Errorf("parse release-group XML: %w", err) + } + + result := &ParsedReleaseGroups{ + Count: list.ReleaseGroupList.Count, + } + for _, rg := range list.ReleaseGroupList.ReleaseGroups { + result.ReleaseGroups = append(result.ReleaseGroups, ReleaseGroup{ + ID: rg.ID, + Title: rg.Title, + Type: rg.Type, + Status: rg.Status, + ArtistID: rg.ArtistCredit.NameCredit.Artist.ID, + ArtistName: rg.ArtistCredit.NameCredit.Artist.Name, + ReleaseDate: rg.ReleaseDate, + }) + } + return result, nil +} + +// ParseArtist parses a MusicBrainz artist XML response into a ParsedArtist struct. +func ParseArtist(data []byte) (*ParsedArtist, error) { + var artist mbArtist + if err := xml.Unmarshal(data, &artist); err != nil { + return nil, fmt.Errorf("parse artist XML: %w", err) + } + return &ParsedArtist{ + ID: artist.Artist.ID, + Name: artist.Artist.Name, + }, nil +} diff --git a/internal/musicbrainz/client_test.go b/internal/musicbrainz/client_test.go new file mode 100644 index 0000000..6c1e8f3 --- /dev/null +++ b/internal/musicbrainz/client_test.go @@ -0,0 +1,167 @@ +package musicbrainz + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" + + "naviwatcher/internal/config" +) + +func newUnbufferedRateLimiter() *rateLimiter { + return newRateLimiter(1000) // high rate to avoid blocking in tests +} + +func TestNewClient_ValidConfig(t *testing.T) { + cfg := config.MusicBrainzConfig{ + UserAgent: "naviwatcher/0.1.0 (test@example.com)", + } + + client := NewClient(cfg) + defer client.Close() + + if client == nil { + t.Fatal("NewClient() returned nil client") + } + + if client.httpClient == nil { + t.Fatal("NewClient() returned client with nil http.Client") + } + + if client.userAgent != cfg.UserAgent { + t.Errorf("NewClient().userAgent = %q, want %q", client.userAgent, cfg.UserAgent) + } + + expectedBaseURL := "https://musicbrainz.org/ws/2" + if client.baseURL != expectedBaseURL { + t.Errorf("NewClient().baseURL = %q, want %q", client.baseURL, expectedBaseURL) + } + + if client.rateLimiter == nil { + t.Fatal("NewClient() returned client with nil rate limiter") + } +} + +func TestNewClientWithLimiter(t *testing.T) { + cfg := config.MusicBrainzConfig{ + UserAgent: "naviwatcher/0.1.0 (test@example.com)", + } + + rl := newRateLimiter(1) + client := NewClientWithLimiter(cfg, rl) + defer client.Close() + + if client == nil { + t.Fatal("NewClientWithLimiter() returned nil client") + } + + if client.rateLimiter != rl { + t.Error("NewClientWithLimiter() did not use provided rate limiter") + } +} + +func TestDoGet_Success(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("User-Agent") == "" { + t.Error("doGet() request missing User-Agent header") + } + if r.Header.Get("Accept") != "application/xml" { + t.Errorf("doGet() Accept header = %q, want %q", r.Header.Get("Accept"), "application/xml") + } + w.Header().Set("Content-Type", "application/xml") + w.Write([]byte(`ok`)) + })) + defer server.Close() + + cfg := config.MusicBrainzConfig{ + UserAgent: "naviwatcher/0.1.0 (test@example.com)", + } + + rl := newUnbufferedRateLimiter() + client := &MusicBrainzClient{ + httpClient: server.Client(), + userAgent: cfg.UserAgent, + baseURL: server.URL, + rateLimiter: rl, + } + defer client.Close() + + body, err := client.doGet("/test") + if err != nil { + t.Fatalf("doGet() error = %v", err) + } + + if string(body) != `ok` { + t.Errorf("doGet() body = %q", string(body)) + } +} + +func TestDoGet_Non200Status(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusServiceUnavailable) + w.Write([]byte("Rate limit exceeded")) + })) + defer server.Close() + + cfg := config.MusicBrainzConfig{ + UserAgent: "naviwatcher/0.1.0 (test@example.com)", + } + + rl := newUnbufferedRateLimiter() + client := &MusicBrainzClient{ + httpClient: server.Client(), + userAgent: cfg.UserAgent, + baseURL: server.URL, + rateLimiter: rl, + } + defer client.Close() + + _, err := client.doGet("/test") + if err == nil { + t.Fatal("doGet() expected error for non-200 status, got nil") + } +} + +func TestDoGet_ServerUnreachable(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + server.Close() + + cfg := config.MusicBrainzConfig{ + UserAgent: "naviwatcher/0.1.0 (test@example.com)", + } + + rl := newUnbufferedRateLimiter() + client := &MusicBrainzClient{ + httpClient: server.Client(), + userAgent: cfg.UserAgent, + baseURL: server.URL, + rateLimiter: rl, + } + defer client.Close() + + _, err := client.doGet("/test") + if err == nil { + t.Fatal("doGet() expected error for unreachable server, got nil") + } +} + +func TestRateLimiter_BasicBehavior(t *testing.T) { + // Test that rate limiter can produce tokens + rl := newRateLimiter(1) + defer rl.stop() + + // Should be able to get a token immediately (bucket was pre-filled) + done := make(chan struct{}) + go func() { + rl.wait() + close(done) + }() + + select { + case <-done: + // success + case <-time.After(2 * time.Second): + t.Fatal("rateLimiter.wait() blocked on pre-filled bucket") + } +} diff --git a/internal/musicbrainz/model.go b/internal/musicbrainz/model.go new file mode 100644 index 0000000..ebf2ae2 --- /dev/null +++ b/internal/musicbrainz/model.go @@ -0,0 +1,46 @@ +package musicbrainz + +import "time" + +// ReleaseGroup represents a MusicBrainz Release Group entity. +// This is the primary data model for the provider - we work with +// Release Groups to minimize duplicates from different releases. +type ReleaseGroup struct { + ID string + Title string + Type string + Status string + ArtistID string + ArtistName string + ReleaseDate string +} + +// Artist represents a MusicBrainz artist entity. +type Artist struct { + ID string + Name string +} + +// ExternalRelease is the normalized form stored in the database, +// matching the external_releases table schema. +type ExternalRelease struct { + RGID string + ArtistID string + Title string + Type string + ReleaseDate string + CachedAt time.Time +} + +// ParsedReleaseGroups holds the result of parsing a MusicBrainz +// release-group list XML response. +type ParsedReleaseGroups struct { + ReleaseGroups []ReleaseGroup + Count int +} + +// ParsedArtist holds the result of parsing a MusicBrainz artist lookup. +type ParsedArtist struct { + ID string + Name string +} diff --git a/internal/musicbrainz/model_test.go b/internal/musicbrainz/model_test.go new file mode 100644 index 0000000..3533f2f --- /dev/null +++ b/internal/musicbrainz/model_test.go @@ -0,0 +1,201 @@ +package musicbrainz + +import ( + "testing" +) + +func TestParseReleaseGroups_Success(t *testing.T) { + data := []byte(` + + + + Dark Side of the Moon + 1973-03-01 + + + + Pink Floyd + + + + + + Another Brick in the Wall + 1979-11-30 + + + + Pink Floyd + + + + + +`) + + result, err := ParseReleaseGroups(data) + if err != nil { + t.Fatalf("ParseReleaseGroups() error = %v", err) + } + + if result.Count != 2 { + t.Errorf("ParseReleaseGroups().Count = %d, want 2", result.Count) + } + + if len(result.ReleaseGroups) != 2 { + t.Fatalf("ParseReleaseGroups() returned %d groups, want 2", len(result.ReleaseGroups)) + } + + expected := []ReleaseGroup{ + { + ID: "rg-uuid-1", + Title: "Dark Side of the Moon", + Type: "Album", + ArtistID: "artist-uuid-1", + ArtistName: "Pink Floyd", + ReleaseDate: "1973-03-01", + }, + { + ID: "rg-uuid-2", + Title: "Another Brick in the Wall", + Type: "Single", + ArtistID: "artist-uuid-1", + ArtistName: "Pink Floyd", + ReleaseDate: "1979-11-30", + }, + } + + for i, rg := range result.ReleaseGroups { + if rg.ID != expected[i].ID { + t.Errorf("ReleaseGroups[%d].ID = %q, want %q", i, rg.ID, expected[i].ID) + } + if rg.Title != expected[i].Title { + t.Errorf("ReleaseGroups[%d].Title = %q, want %q", i, rg.Title, expected[i].Title) + } + if rg.Type != expected[i].Type { + t.Errorf("ReleaseGroups[%d].Type = %q, want %q", i, rg.Type, expected[i].Type) + } + if rg.ArtistID != expected[i].ArtistID { + t.Errorf("ReleaseGroups[%d].ArtistID = %q, want %q", i, rg.ArtistID, expected[i].ArtistID) + } + if rg.ArtistName != expected[i].ArtistName { + t.Errorf("ReleaseGroups[%d].ArtistName = %q, want %q", i, rg.ArtistName, expected[i].ArtistName) + } + if rg.ReleaseDate != expected[i].ReleaseDate { + t.Errorf("ReleaseGroups[%d].ReleaseDate = %q, want %q", i, rg.ReleaseDate, expected[i].ReleaseDate) + } + } +} + +func TestParseReleaseGroups_Empty(t *testing.T) { + data := []byte(` + + + +`) + + result, err := ParseReleaseGroups(data) + if err != nil { + t.Fatalf("ParseReleaseGroups() error = %v", err) + } + + if result.Count != 0 { + t.Errorf("ParseReleaseGroups().Count = %d, want 0", result.Count) + } + + if len(result.ReleaseGroups) != 0 { + t.Errorf("ParseReleaseGroups() returned %d groups, want 0", len(result.ReleaseGroups)) + } +} + +func TestParseReleaseGroups_MalformedXML(t *testing.T) { + tests := []struct { + name string + data []byte + }{ + { + name: "truncated XML", + data: []byte(``), + }, + { + name: "not XML at all", + data: []byte(`this is not xml`), + }, + { + name: "wrong root element", + data: []byte(`test`), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := ParseReleaseGroups(tt.data) + if err == nil { + t.Fatal("ParseReleaseGroups() expected error for malformed XML, got nil") + } + }) + } +} + +func TestParseReleaseGroups_WithStatus(t *testing.T) { + data := []byte(` + + + + Unofficial Live Recording + 2020-01-01 + + + + Test Artist + + + + + +`) + + result, err := ParseReleaseGroups(data) + if err != nil { + t.Fatalf("ParseReleaseGroups() error = %v", err) + } + + if len(result.ReleaseGroups) != 1 { + t.Fatalf("ParseReleaseGroups() returned %d groups, want 1", len(result.ReleaseGroups)) + } + + if result.ReleaseGroups[0].Status != "Bootleg" { + t.Errorf("ReleaseGroups[0].Status = %q, want %q", result.ReleaseGroups[0].Status, "Bootleg") + } +} + +func TestParseArtist_Success(t *testing.T) { + data := []byte(` + + + Pink Floyd + +`) + + result, err := ParseArtist(data) + if err != nil { + t.Fatalf("ParseArtist() error = %v", err) + } + + if result.ID != "artist-uuid-1" { + t.Errorf("ParseArtist().ID = %q, want %q", result.ID, "artist-uuid-1") + } + + if result.Name != "Pink Floyd" { + t.Errorf("ParseArtist().Name = %q, want %q", result.Name, "Pink Floyd") + } +} + +func TestParseArtist_MalformedXML(t *testing.T) { + data := []byte(`this is not xml`) + + _, err := ParseArtist(data) + if err == nil { + t.Fatal("ParseArtist() expected error for malformed XML, got nil") + } +}