From a5911c257c057a9e7238cfe6d266f1245054c358 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Tue, 26 May 2026 14:10:22 +0300 Subject: [PATCH] fix: address code review findings Remove dead code: duplicate ExternalRelease/Artist/ParsedArtist structs in model.go, ParseArtist/mbArtist/mbArtistData in client.go, ArtistTypeFilter and related filtering functions in api.go, SyncArtistDiscographyWithFilter in sync.go, and CacheStats/IsArtistCacheValid in cache.go. Fix bugs: SaveExternalRelease now stores NULL instead of empty string for zero CachedAt; sync upserts are now transactional with stale release cleanup; getCachedReleases returns int instead of *CacheStats; doGet uses url.Values for proper query encoding of MBID. Fix tests: removed duplicate TestRun_GracefulShutdown, removed dead code (_ = dbPath) from TestNewApp, fixed assertions in httptest handler goroutine to avoid data race, increased rate limiter timing tolerance, removed Client.Close() calls (no-op removed), fixed sync test cache expiry to use UPDATE instead of 0 TTL races. Fix formatting: cancel()}() formatting in main.go, error format string in sync.go. --- cmd/naviwatcher/main.go | 6 +- cmd/naviwatcher/main_test.go | 34 --- internal/database/external_releases.go | 8 +- internal/musicbrainz/api.go | 97 ++------- internal/musicbrainz/api_test.go | 274 +------------------------ internal/musicbrainz/cache.go | 42 +--- internal/musicbrainz/cache_test.go | 104 ++-------- internal/musicbrainz/client.go | 43 +--- internal/musicbrainz/client_test.go | 50 ++--- internal/musicbrainz/model.go | 25 --- internal/musicbrainz/model_test.go | 31 --- internal/musicbrainz/sync.go | 85 +++----- internal/musicbrainz/sync_test.go | 264 ++++++++++-------------- 13 files changed, 199 insertions(+), 864 deletions(-) diff --git a/cmd/naviwatcher/main.go b/cmd/naviwatcher/main.go index 0ac59b2..d153b34 100644 --- a/cmd/naviwatcher/main.go +++ b/cmd/naviwatcher/main.go @@ -45,7 +45,8 @@ func main() { go func() { sig := <-sigCh log.Printf("Received signal %v, shutting down...", sig) - cancel()}() + cancel() + }() app, err := NewApp(ctx, cfg) if err != nil { @@ -82,9 +83,6 @@ func NewApp(ctx context.Context, cfg *config.Config) (*App, error) { // Close cleans up all application resources in reverse order of initialization. func (a *App) Close() { - if a.mbClient != nil { - a.mbClient.Close() - } 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 db80100..317c50b 100644 --- a/cmd/naviwatcher/main_test.go +++ b/cmd/naviwatcher/main_test.go @@ -9,36 +9,6 @@ import ( "naviwatcher/internal/config" ) -func TestRun_GracefulShutdown(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - cfg := &config.Config{ - Server: config.ServerConfig{ - Host: "127.0.0.1", - Port: 9090, - }, - Navidrome: config.NavidromeConfig{ - URL: "http://localhost:4533", - User: "test", - Password: "test", - }, - MusicBrainz: config.MusicBrainzConfig{ - UserAgent: "NaviWatcher/1.0 ( test@example.com )", - }, - } - - app, err := NewApp(ctx, cfg) - if err != nil { - t.Fatalf("NewApp returned error: %v", err) - } - defer app.Close() - - if err := app.run(ctx); err != nil { - t.Fatalf("app.run returned error: %v", err) - } -} - func TestConfigIntegration(t *testing.T) { // Integration test: write a minimal valid config and load it via config.LoadConfig, // verifying the full path that main() uses. @@ -77,10 +47,6 @@ musicbrainz: func TestNewApp_CreatesMusicBrainzClient(t *testing.T) { // Verify that NewApp initializes the MusicBrainz client from config. - dir := t.TempDir() - dbPath := filepath.Join(dir, "test.db") - _ = dbPath // database.New uses a hardcoded path in this version; we test the client creation. - cfg := &config.Config{ Server: config.ServerConfig{ Host: "127.0.0.1", diff --git a/internal/database/external_releases.go b/internal/database/external_releases.go index c0bba31..62d4388 100644 --- a/internal/database/external_releases.go +++ b/internal/database/external_releases.go @@ -28,13 +28,13 @@ func GetExternalRelease(db *DB, rgid string) (*ExternalRelease, error) { // SaveExternalRelease inserts or replaces an external_release row. func SaveExternalRelease(db *DB, release *ExternalRelease) error { - cachedAtStr := "" + var cachedAt interface{} if !release.CachedAt.IsZero() { - cachedAtStr = release.CachedAt.Format("2006-01-02 15:04:05") + cachedAt = release.CachedAt.Format("2006-01-02 15:04:05") } _, err := db.Conn().Exec( "INSERT OR REPLACE INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at) VALUES (?, ?, ?, ?, ?, ?, ?)", - release.RGID, release.ArtistID, release.Title, release.Type, release.ReleaseDate, release.IsIgnored, cachedAtStr, + release.RGID, release.ArtistID, release.Title, release.Type, release.ReleaseDate, release.IsIgnored, cachedAt, ) if err != nil { return fmt.Errorf("save external release: %w", err) @@ -158,8 +158,8 @@ func GetExternalReleasesByArtistWithCache(db *DB, artistID string, ttl time.Dura var results []ExternalRelease for rows.Next() { var r ExternalRelease - var releaseDate sql.NullString var releaseType sql.NullString + var releaseDate sql.NullString var cachedAt sql.NullTime if err := rows.Scan(&r.RGID, &r.ArtistID, &r.Title, &releaseType, &releaseDate, &r.IsIgnored, &cachedAt); err != nil { return nil, fmt.Errorf("scan cached external release: %w", err) diff --git a/internal/musicbrainz/api.go b/internal/musicbrainz/api.go index 4378d9d..334b6a5 100644 --- a/internal/musicbrainz/api.go +++ b/internal/musicbrainz/api.go @@ -3,6 +3,7 @@ package musicbrainz import ( "context" "fmt" + "net/url" "regexp" "strings" "unicode" @@ -12,41 +13,17 @@ import ( // excludedStatuses contains release-group statuses that should be filtered out. var excludedStatuses = map[string]bool{ - "Bootleg": true, - "Promotion": true, - "Pseudo-Release": true, + "Bootleg": true, + "Promotion": true, + "Pseudo-Release": true, } // includedTypes contains release-group types that should be included. var includedTypes = map[string]bool{ - "Album": true, - "Single": true, - "EP": true, - "Compilation": true, -} - -// ArtistTypeFilter holds per-artist type filtering preferences. -// These are placeholders for Web UI integration where users can -// toggle which release types to monitor per artist. -type ArtistTypeFilter struct { - // ArtistID is the MusicBrainz artist ID. - ArtistID string - // IncludeSingles whether to include Single-type release groups. - IncludeSingles bool - // IncludeCompilations whether to include Compilation-type release groups. - IncludeCompilations bool - // IncludeEP whether to include EP-type release groups. - IncludeEP bool -} - -// DefaultArtistTypeFilter returns an ArtistTypeFilter with all types enabled. -func DefaultArtistTypeFilter(artistID string) *ArtistTypeFilter { - return &ArtistTypeFilter{ - ArtistID: artistID, - IncludeSingles: true, - IncludeCompilations: true, - IncludeEP: true, - } + "Album": true, + "Single": true, + "EP": true, + "Compilation": true, } // GetArtistReleaseGroups fetches all release groups for a given artist from MusicBrainz. @@ -61,7 +38,12 @@ func (c *MusicBrainzClient) GetArtistReleaseGroups(ctx context.Context, artistMB limit := 100 // MusicBrainz max limit per request for { - path := fmt.Sprintf("/release-group?artist=%s&limit=%d&offset=%d", artistMBID, limit, offset) + params := url.Values{} + params.Set("artist", artistMBID) + params.Set("limit", fmt.Sprintf("%d", limit)) + params.Set("offset", fmt.Sprintf("%d", offset)) + path := buildPath("/release-group", params) + body, err := c.doGet(ctx, path) if err != nil { return nil, fmt.Errorf("fetch release groups for artist %s: %w", artistMBID, err) @@ -101,22 +83,6 @@ func FilterReleaseGroups(groups []ReleaseGroup) []ReleaseGroup { return filtered } -// FilterReleaseGroupsWithArtistFilter applies status filtering, base type filtering, -// and per-artist type filtering preferences. -func FilterReleaseGroupsWithArtistFilter(groups []ReleaseGroup, artistFilter *ArtistTypeFilter) []ReleaseGroup { - var filtered []ReleaseGroup - for _, rg := range groups { - if IsStatusExcluded(rg.Status) { - continue - } - if !IsTypeIncludedForArtist(rg.Type, artistFilter) { - continue - } - filtered = append(filtered, rg) - } - return filtered -} - // IsStatusExcluded returns true if the given status should be excluded. func IsStatusExcluded(status string) bool { return excludedStatuses[status] @@ -127,34 +93,13 @@ func IsTypeIncluded(releaseType string) bool { return includedTypes[releaseType] } -// IsTypeIncludedForArtist checks whether a release type should be included -// based on per-artist type filtering preferences. -func IsTypeIncludedForArtist(releaseType string, filter *ArtistTypeFilter) bool { - if filter == nil { - return IsTypeIncluded(releaseType) - } - - switch releaseType { - case "Album": - return true // Albums are always included - case "Single": - return filter.IncludeSingles - case "EP": - return filter.IncludeEP - case "Compilation": - return filter.IncludeCompilations - default: - return false - } -} - // NormalizeString normalizes a string for fuzzy matching by: -// - Converting to lowercase -// - Removing special characters (keeping only letters, digits, and spaces) -// - Removing years (4-digit numbers that look like years) -// - Removing bracketed keywords (e.g., [Deluxe], [Remastered]) -// - Collapsing multiple spaces into one -// - Trimming leading/trailing whitespace +// - Converting to lowercase +// - Removing special characters (keeping only letters, digits, and spaces) +// - Removing years (4-digit numbers that look like years) +// - Removing bracketed keywords (e.g., [Deluxe], [Remastered]) +// - Collapsing multiple spaces into one +// - Trimming leading/trailing whitespace func NormalizeString(s string) string { // Convert to lowercase s = strings.ToLower(s) @@ -212,7 +157,7 @@ func NormalizeArtistName(name string) string { } // ToExternalRelease converts a ReleaseGroup to an ExternalRelease -// with the current timestamp as CachedAt. +// for database persistence. func (rg *ReleaseGroup) ToExternalRelease() *database.ExternalRelease { return &database.ExternalRelease{ RGID: rg.ID, diff --git a/internal/musicbrainz/api_test.go b/internal/musicbrainz/api_test.go index 850c73c..26bea62 100644 --- a/internal/musicbrainz/api_test.go +++ b/internal/musicbrainz/api_test.go @@ -50,275 +50,11 @@ func TestFilterReleaseGroups_IncludesOnlyAllowedTypes(t *testing.T) { allowedIDs := map[string]bool{"rg-1": true, "rg-2": true, "rg-3": true, "rg-4": true} for _, rg := range result { if !allowedIDs[rg.ID] { - t.Errorf("unexpected release group %q (type %q) passed filter", rg.ID, rg.Type) + t.Errorf("unexpected group %q in filtered results", rg.ID) } } } -func TestFilterReleaseGroups_Empty(t *testing.T) { - result := FilterReleaseGroups(nil) - if len(result) != 0 { - t.Errorf("FilterReleaseGroups(nil) returned %d groups, want 0", len(result)) - } -} - -func TestFilterReleaseGroups_AllExcluded(t *testing.T) { - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Bootleg", Type: "Album", Status: "Bootleg"}, - {ID: "rg-2", Title: "Promo", Type: "Single", Status: "Promotion"}, - {ID: "rg-3", Title: "Soundtrack", Type: "Soundtrack", Status: "Official"}, - } - - result := FilterReleaseGroups(groups) - if len(result) != 0 { - t.Errorf("FilterReleaseGroups() returned %d groups, want 0 (all excluded)", len(result)) - } -} - -func TestFilterReleaseGroups_NoStatus(t *testing.T) { - // Release groups with empty status should pass (not excluded) - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Unknown Status", Type: "Album", Status: ""}, - } - - result := FilterReleaseGroups(groups) - if len(result) != 1 { - t.Errorf("FilterReleaseGroups() returned %d groups, want 1 (empty status is not excluded)", len(result)) - } -} - -// ---------- IsStatusExcluded tests ---------- - -func TestIsStatusExcluded(t *testing.T) { - tests := []struct { - status string - excluded bool - }{ - {"Bootleg", true}, - {"Promotion", true}, - {"Pseudo-Release", true}, - {"Official", false}, - {"", false}, - {"official", false}, // case-sensitive: only exact match - } - - for _, tt := range tests { - t.Run(tt.status, func(t *testing.T) { - got := IsStatusExcluded(tt.status) - if got != tt.excluded { - t.Errorf("IsStatusExcluded(%q) = %v, want %v", tt.status, got, tt.excluded) - } - }) - } -} - -// ---------- IsTypeIncluded tests ---------- - -func TestIsTypeIncluded(t *testing.T) { - tests := []struct { - rgType string - included bool - }{ - {"Album", true}, - {"Single", true}, - {"EP", true}, - {"Compilation", true}, - {"Soundtrack", false}, - {"Live", false}, - {"Remix", false}, - {"", false}, - } - - for _, tt := range tests { - t.Run(tt.rgType, func(t *testing.T) { - got := IsTypeIncluded(tt.rgType) - if got != tt.included { - t.Errorf("IsTypeIncluded(%q) = %v, want %v", tt.rgType, got, tt.included) - } - }) - } -} - -// ---------- Artist type filter tests ---------- - -func TestFilterReleaseGroupsWithArtistFilter_AllEnabled(t *testing.T) { - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"}, - {ID: "rg-2", Title: "Single", Type: "Single", Status: "Official"}, - {ID: "rg-3", Title: "EP", Type: "EP", Status: "Official"}, - {ID: "rg-4", Title: "Compilation", Type: "Compilation", Status: "Official"}, - } - - filter := DefaultArtistTypeFilter("artist-1") - result := FilterReleaseGroupsWithArtistFilter(groups, filter) - - if len(result) != 4 { - t.Fatalf("FilterReleaseGroupsWithArtistFilter() returned %d groups, want 4", len(result)) - } -} - -func TestFilterReleaseGroupsWithArtistFilter_ExcludeSingles(t *testing.T) { - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"}, - {ID: "rg-2", Title: "Single", Type: "Single", Status: "Official"}, - {ID: "rg-3", Title: "EP", Type: "EP", Status: "Official"}, - } - - filter := &ArtistTypeFilter{ - ArtistID: "artist-1", - IncludeSingles: false, - IncludeCompilations: true, - IncludeEP: true, - } - - result := FilterReleaseGroupsWithArtistFilter(groups, filter) - - if len(result) != 2 { - t.Fatalf("FilterReleaseGroupsWithArtistFilter() returned %d groups, want 2", len(result)) - } - - for _, rg := range result { - if rg.Type == "Single" { - t.Errorf("Single %q should have been excluded", rg.ID) - } - } -} - -func TestFilterReleaseGroupsWithArtistFilter_ExcludeCompilations(t *testing.T) { - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"}, - {ID: "rg-2", Title: "Compilation", Type: "Compilation", Status: "Official"}, - } - - filter := &ArtistTypeFilter{ - ArtistID: "artist-1", - IncludeSingles: true, - IncludeCompilations: false, - IncludeEP: true, - } - - result := FilterReleaseGroupsWithArtistFilter(groups, filter) - - if len(result) != 1 { - t.Fatalf("FilterReleaseGroupsWithArtistFilter() returned %d groups, want 1", len(result)) - } - if result[0].ID != "rg-1" { - t.Errorf("expected rg-1, got %s", result[0].ID) - } -} - -func TestFilterReleaseGroupsWithArtistFilter_ExcludeEP(t *testing.T) { - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"}, - {ID: "rg-2", Title: "EP", Type: "EP", Status: "Official"}, - } - - filter := &ArtistTypeFilter{ - ArtistID: "artist-1", - IncludeSingles: true, - IncludeCompilations: true, - IncludeEP: false, - } - - result := FilterReleaseGroupsWithArtistFilter(groups, filter) - - if len(result) != 1 { - t.Fatalf("FilterReleaseGroupsWithArtistFilter() returned %d groups, want 1", len(result)) - } - if result[0].ID != "rg-1" { - t.Errorf("expected rg-1, got %s", result[0].ID) - } -} - -func TestFilterReleaseGroupsWithArtistFilter_NilFilter(t *testing.T) { - groups := []ReleaseGroup{ - {ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"}, - {ID: "rg-2", Title: "Soundtrack", Type: "Soundtrack", Status: "Official"}, - } - - result := FilterReleaseGroupsWithArtistFilter(groups, nil) - - if len(result) != 1 { - t.Fatalf("FilterReleaseGroupsWithArtistFilter(nil filter) returned %d groups, want 1", len(result)) - } -} - -func TestDefaultArtistTypeFilter(t *testing.T) { - filter := DefaultArtistTypeFilter("artist-1") - - if filter.ArtistID != "artist-1" { - t.Errorf("ArtistID = %q, want %q", filter.ArtistID, "artist-1") - } - if !filter.IncludeSingles { - t.Error("IncludeSingles should be true by default") - } - if !filter.IncludeCompilations { - t.Error("IncludeCompilations should be true by default") - } - if !filter.IncludeEP { - t.Error("IncludeEP should be true by default") - } -} - -func TestIsTypeIncludedForArtist(t *testing.T) { - tests := []struct { - name string - rgType string - filter *ArtistTypeFilter - included bool - }{ - { - name: "Album always included", - rgType: "Album", - filter: DefaultArtistTypeFilter("artist-1"), - included: true, - }, - { - name: "Single included with filter", - rgType: "Single", - filter: DefaultArtistTypeFilter("artist-1"), - included: true, - }, - { - name: "Single excluded", - rgType: "Single", - filter: &ArtistTypeFilter{ - IncludeSingles: false, - IncludeCompilations: true, - IncludeEP: true, - }, - included: false, - }, - { - name: "Soundtrack excluded", - rgType: "Soundtrack", - filter: DefaultArtistTypeFilter("artist-1"), - included: false, - }, - { - name: "Nil filter falls back to base", - rgType: "Album", - filter: nil, - included: true, - }, - { - name: "Nil filter excludes non-base types", - rgType: "Soundtrack", - filter: nil, - included: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := IsTypeIncludedForArtist(tt.rgType, tt.filter) - if got != tt.included { - t.Errorf("IsTypeIncludedForArtist(%q) = %v, want %v", tt.rgType, got, tt.included) - } - }) - } -} - // ---------- NormalizeString tests ---------- func TestNormalizeString_Basic(t *testing.T) { @@ -474,7 +210,6 @@ func TestGetArtistReleaseGroups_Success(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() groups, err := client.GetArtistReleaseGroups(context.Background(), artistMBID) if err != nil { @@ -504,8 +239,7 @@ func TestGetArtistReleaseGroups_Pagination(t *testing.T) { w.Header().Set("Content-Type", "application/xml") if offset == "0" || offset == "" { - // First page: return "100" results (full page, matching limit) to trigger pagination - // We generate multiple release-group elements in the XML + // First page: return 100 results (full page, matching limit) to trigger pagination xml := ` ` @@ -556,7 +290,6 @@ func TestGetArtistReleaseGroups_Pagination(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() groups, err := client.GetArtistReleaseGroups(context.Background(), artistMBID) if err != nil { @@ -597,7 +330,6 @@ func TestGetArtistReleaseGroups_EmptyResult(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() groups, err := client.GetArtistReleaseGroups(context.Background(), artistMBID) if err != nil { @@ -627,7 +359,6 @@ func TestGetArtistReleaseGroups_ServerError(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() _, err := client.GetArtistReleaseGroups(context.Background(), "artist-1") if err == nil { @@ -653,7 +384,6 @@ func TestGetArtistReleaseGroups_InvalidXML(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() _, err := client.GetArtistReleaseGroups(context.Background(), "artist-1") if err == nil { diff --git a/internal/musicbrainz/cache.go b/internal/musicbrainz/cache.go index a88b0a4..2cfa444 100644 --- a/internal/musicbrainz/cache.go +++ b/internal/musicbrainz/cache.go @@ -7,47 +7,13 @@ import ( "naviwatcher/internal/database" ) -// CacheStats holds the result of a cache lookup for a given artist. -type CacheStats struct { - // CachedRGIDs is the list of RGIDs that are currently cached (within TTL). - CachedRGIDs []string - // CacheHitCount is the number of entries found in cache. - CacheHitCount int -} - -// IsCached returns true if the given RGID is in the cached set. -func (cs *CacheStats) IsCached(rgid string) bool { - for _, id := range cs.CachedRGIDs { - if id == rgid { - return true - } - } - return false -} - // GetCachedReleases queries the external_releases table for entries // belonging to the given artist that were cached within the specified TTL. -// It returns a CacheStats with the list of valid RGIDs already in cache. -func GetCachedReleases(db *database.DB, artistID string, ttl time.Duration) (*CacheStats, error) { +// It returns the count of cached entries and any error encountered. +func GetCachedReleases(db *database.DB, artistID string, ttl time.Duration) (int, error) { releases, err := database.GetExternalReleasesByArtistWithCache(db, artistID, ttl) if err != nil { - return nil, fmt.Errorf("get cached releases: %w", err) + return 0, fmt.Errorf("get cached releases: %w", err) } - - stats := &CacheStats{} - for _, r := range releases { - stats.CachedRGIDs = append(stats.CachedRGIDs, r.RGID) - stats.CacheHitCount++ - } - return stats, nil -} - -// IsArtistCacheValid checks whether the cache for an artist is still valid. -// Returns true if any entries exist within the TTL for this artist. -func IsArtistCacheValid(db *database.DB, artistID string, ttl time.Duration) (bool, error) { - stats, err := GetCachedReleases(db, artistID, ttl) - if err != nil { - return false, err - } - return stats.CacheHitCount > 0, nil + return len(releases), nil } diff --git a/internal/musicbrainz/cache_test.go b/internal/musicbrainz/cache_test.go index ea51383..efd02b2 100644 --- a/internal/musicbrainz/cache_test.go +++ b/internal/musicbrainz/cache_test.go @@ -45,20 +45,13 @@ func TestGetCachedReleases_CacheHit(t *testing.T) { } ttl := 24 * time.Hour - stats, err := GetCachedReleases(db, artistID, ttl) + count, err := GetCachedReleases(db, artistID, ttl) if err != nil { t.Fatalf("GetCachedReleases() error: %v", err) } - if stats.CacheHitCount != 2 { - t.Errorf("CacheHitCount = %d, want 2", stats.CacheHitCount) - } - - if !stats.IsCached("rg-hit-1") { - t.Error("expected rg-hit-1 to be cached") - } - if !stats.IsCached("rg-hit-2") { - t.Error("expected rg-hit-2 to be cached") + if count != 2 { + t.Errorf("GetCachedReleases() = %d, want 2", count) } } @@ -85,17 +78,13 @@ func TestGetCachedReleases_CacheMiss_Expired(t *testing.T) { } ttl := 24 * time.Hour - stats, err := GetCachedReleases(db, artistID, ttl) + count, err := GetCachedReleases(db, artistID, ttl) if err != nil { t.Fatalf("GetCachedReleases() error: %v", err) } - if stats.CacheHitCount != 0 { - t.Errorf("CacheHitCount = %d, want 0 (expired entry should not be cached)", stats.CacheHitCount) - } - - if stats.IsCached("rg-expired") { - t.Error("expected rg-expired to NOT be cached") + if count != 0 { + t.Errorf("GetCachedReleases() = %d, want 0 (expired entry should not be cached)", count) } } @@ -121,13 +110,13 @@ func TestGetCachedReleases_CacheMiss_NoCachedAt(t *testing.T) { } ttl := 24 * time.Hour - stats, err := GetCachedReleases(db, artistID, ttl) + count, err := GetCachedReleases(db, artistID, ttl) if err != nil { t.Fatalf("GetCachedReleases() error: %v", err) } - if stats.CacheHitCount != 0 { - t.Errorf("CacheHitCount = %d, want 0 (NULL cached_at should not be cached)", stats.CacheHitCount) + if count != 0 { + t.Errorf("GetCachedReleases() = %d, want 0 (NULL cached_at should not be cached)", count) } } @@ -139,70 +128,13 @@ func TestGetCachedReleases_EmptyArtist(t *testing.T) { defer db.Close() ttl := 24 * time.Hour - stats, err := GetCachedReleases(db, "nonexistent-artist", ttl) + count, err := GetCachedReleases(db, "nonexistent-artist", ttl) if err != nil { t.Fatalf("GetCachedReleases() error: %v", err) } - if stats.CacheHitCount != 0 { - t.Errorf("CacheHitCount = %d, want 0 for nonexistent artist", stats.CacheHitCount) - } -} - -func TestIsArtistCacheValid_Valid(t *testing.T) { - db, err := database.New(":memory:") - if err != nil { - t.Fatalf("New() error: %v", err) - } - defer db.Close() - - artistID := "artist-valid-cache" - if err := insertTestArtistForCache(db, artistID); err != nil { - t.Fatalf("insertTestArtist: %v", err) - } - - now := time.Now().Format("2006-01-02 15:04:05") - _, err = db.Conn().Exec( - "INSERT INTO external_releases (rgid, artist_id, title, cached_at) VALUES (?, ?, ?, ?)", - "rg-valid", artistID, "Valid Album", now, - ) - if err != nil { - t.Fatalf("insert: %v", err) - } - - ttl := 24 * time.Hour - valid, err := IsArtistCacheValid(db, artistID, ttl) - if err != nil { - t.Fatalf("IsArtistCacheValid() error: %v", err) - } - if !valid { - t.Error("expected cache to be valid") - } -} - -func TestIsArtistCacheValid_Invalid(t *testing.T) { - db, err := database.New(":memory:") - if err != nil { - t.Fatalf("New() error: %v", err) - } - defer db.Close() - - ttl := 24 * time.Hour - - // No releases at all - valid, err := IsArtistCacheValid(db, "no-releases", ttl) - if err != nil { - t.Fatalf("IsArtistCacheValid() error: %v", err) - } - if valid { - t.Error("expected cache to be invalid for artist with no releases") - } -} - -func TestCacheStats_IsCached_Empty(t *testing.T) { - stats := &CacheStats{} - if stats.IsCached("anything") { - t.Error("expected IsCached to return false for empty stats") + if count != 0 { + t.Errorf("GetCachedReleases() = %d, want 0 for nonexistent artist", count) } } @@ -238,18 +170,12 @@ func TestGetCachedReleases_MixedExpiry(t *testing.T) { } ttl := 24 * time.Hour - stats, err := GetCachedReleases(db, artistID, ttl) + count, err := GetCachedReleases(db, artistID, ttl) if err != nil { t.Fatalf("GetCachedReleases() error: %v", err) } - if stats.CacheHitCount != 1 { - t.Errorf("CacheHitCount = %d, want 1 (only fresh entry)", stats.CacheHitCount) - } - if !stats.IsCached("rg-fresh") { - t.Error("expected rg-fresh to be cached") - } - if stats.IsCached("rg-old") { - t.Error("expected rg-old to NOT be cached (expired)") + if count != 1 { + t.Errorf("GetCachedReleases() = %d, want 1 (only fresh entry)", count) } } diff --git a/internal/musicbrainz/client.go b/internal/musicbrainz/client.go index 36b60df..adcdd81 100644 --- a/internal/musicbrainz/client.go +++ b/internal/musicbrainz/client.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "net/url" "time" "golang.org/x/time/rate" @@ -35,23 +36,6 @@ func NewClient(cfg config.MusicBrainzConfig) *MusicBrainzClient { } } -// 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 *rate.Limiter) *MusicBrainzClient { - return &MusicBrainzClient{ - httpClient: &http.Client{ - Timeout: 30 * time.Second, - }, - userAgent: cfg.UserAgent, - baseURL: "https://musicbrainz.org/ws/2", - rateLimiter: rl, - } -} - -// Close is a no-op for the x/time/rate-based client (the limiter does not -// spawn goroutines), but retained for API compatibility. -func (c *MusicBrainzClient) Close() {} - // doGet performs a rate-limited HTTP GET request to the MusicBrainz API. // It blocks until the rate limiter allows the request, then sets the proper // User-Agent header and returns the response body. @@ -126,18 +110,6 @@ type mbReleaseGroupList struct { 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) { @@ -163,14 +135,7 @@ func ParseReleaseGroups(data []byte) (*ParsedReleaseGroups, error) { 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 +// buildPath constructs a properly URL-encoded query path for the MusicBrainz API. +func buildPath(endpoint string, params url.Values) string { + return endpoint + "?" + params.Encode() } diff --git a/internal/musicbrainz/client_test.go b/internal/musicbrainz/client_test.go index 885a6d2..c91cb5e 100644 --- a/internal/musicbrainz/client_test.go +++ b/internal/musicbrainz/client_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "sync" "testing" "time" @@ -17,7 +18,6 @@ func TestNewClient_ValidConfig(t *testing.T) { } client := NewClient(cfg) - defer client.Close() if client == nil { t.Fatal("NewClient() returned nil client") @@ -41,32 +41,15 @@ func TestNewClient_ValidConfig(t *testing.T) { } } -func TestNewClientWithLimiter(t *testing.T) { - cfg := config.MusicBrainzConfig{ - UserAgent: "naviwatcher/0.1.0 (test@example.com)", - } - - rl := rate.NewLimiter(rate.Limit(1), 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) { + var mu sync.Mutex + var gotUserAgent, gotAccept string + 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") - } + mu.Lock() + gotUserAgent = r.Header.Get("User-Agent") + gotAccept = r.Header.Get("Accept") + mu.Unlock() w.Header().Set("Content-Type", "application/xml") w.Write([]byte(`ok`)) })) @@ -83,7 +66,6 @@ func TestDoGet_Success(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() body, err := client.doGet(context.Background(), "/test") if err != nil { @@ -93,6 +75,15 @@ func TestDoGet_Success(t *testing.T) { if string(body) != `ok` { t.Errorf("doGet() body = %q", string(body)) } + + mu.Lock() + if gotUserAgent == "" { + t.Error("doGet() request missing User-Agent header") + } + if gotAccept != "application/xml" { + t.Errorf("doGet() Accept header = %q, want %q", gotAccept, "application/xml") + } + mu.Unlock() } func TestDoGet_Non200Status(t *testing.T) { @@ -113,7 +104,6 @@ func TestDoGet_Non200Status(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() _, err := client.doGet(context.Background(), "/test") if err == nil { @@ -136,7 +126,6 @@ func TestDoGet_ServerUnreachable(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() _, err := client.doGet(context.Background(), "/test") if err == nil { @@ -162,7 +151,6 @@ func TestDoGet_ContextCancellation(t *testing.T) { baseURL: server.URL, rateLimiter: rl, } - defer client.Close() ctx, cancel := context.WithCancel(context.Background()) cancel() // cancel immediately @@ -183,7 +171,7 @@ func TestRateLimiter_OnePerSecond(t *testing.T) { t.Fatalf("first Wait() error: %v", err) } elapsed := time.Since(start) - if elapsed > 100*time.Millisecond { + if elapsed > 200*time.Millisecond { t.Errorf("first Wait() took %v, expected near-instant", elapsed) } @@ -209,7 +197,7 @@ func TestRateLimiter_BurstBehavior(t *testing.T) { rl.Wait(context.Background()) elapsed := time.Since(start) - if elapsed > 50*time.Millisecond { + if elapsed > 200*time.Millisecond { t.Errorf("burst Wait() took %v, expected near-instant", elapsed) } } diff --git a/internal/musicbrainz/model.go b/internal/musicbrainz/model.go index ebf2ae2..942f2ee 100644 --- a/internal/musicbrainz/model.go +++ b/internal/musicbrainz/model.go @@ -1,7 +1,5 @@ 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. @@ -15,32 +13,9 @@ type ReleaseGroup struct { 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 index 3533f2f..4f1f241 100644 --- a/internal/musicbrainz/model_test.go +++ b/internal/musicbrainz/model_test.go @@ -168,34 +168,3 @@ func TestParseReleaseGroups_WithStatus(t *testing.T) { 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") - } -} diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go index c0700c7..606e899 100644 --- a/internal/musicbrainz/sync.go +++ b/internal/musicbrainz/sync.go @@ -14,7 +14,7 @@ import ( // 2. If cache hit, return the cached releases immediately. // 3. If cache miss or expired, fetch release groups from MusicBrainz API. // 4. Apply status and type filtering. -// 5. Upsert each filtered release group into external_releases with current timestamp. +// 5. Within a transaction: delete old entries, then upsert each filtered release group. // 6. Return the list of external releases. // // Context cancellation is checked before the API call and between each upsert @@ -32,27 +32,38 @@ func SyncArtistDiscography( } // Step 1: Check cache. - cached, err := GetCachedReleases(db, artistMBID, ttl) + cachedCount, err := GetCachedReleases(db, artistMBID, ttl) if err != nil { return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err) } // Step 2: If we have cached data, return it. - if cached.CacheHitCount > 0 { + if cachedCount > 0 { return database.GetExternalReleasesByArtistWithCache(db, artistMBID, ttl) } // Step 3: Cache miss — fetch from MusicBrainz API. groups, err := client.GetArtistReleaseGroups(ctx, artistMBID) if err != nil { - return nil, fmt.Errorf("sync artist discography: fetch release groups: %w", err) + return nil, fmt.Errorf("sync artist discography: fetch release groups for artist %s: %w", artistMBID, err) } // Step 4: Apply filtering. filtered := FilterReleaseGroups(groups) - // Step 5: Upsert each release group into the database. + // Step 5: Upsert within a transaction — delete old entries first, then insert new ones. now := time.Now() + tx, err := db.Begin() + if err != nil { + return nil, fmt.Errorf("sync artist discography: begin transaction: %w", err) + } + defer tx.Rollback() + + // Delete old entries for this artist to avoid stale records. + if _, err := tx.Exec("DELETE FROM external_releases WHERE artist_id = ?", artistMBID); err != nil { + return nil, fmt.Errorf("sync artist discography: delete old releases: %w", err) + } + var releases []database.ExternalRelease for _, rg := range filtered { // Check context cancellation between each upsert. @@ -63,67 +74,19 @@ func SyncArtistDiscography( ext := rg.ToExternalRelease() ext.CachedAt = now - if err := database.SaveExternalRelease(db, ext); err != nil { - return nil, fmt.Errorf("sync artist discography: save release %s: %w", rg.ID, err) + cachedAtStr := ext.CachedAt.Format("2006-01-02 15:04:05") + if _, err := tx.Exec( + "INSERT INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at) VALUES (?, ?, ?, ?, ?, ?, ?)", + ext.RGID, ext.ArtistID, ext.Title, ext.Type, ext.ReleaseDate, ext.IsIgnored, cachedAtStr, + ); err != nil { + return nil, fmt.Errorf("sync artist discography: insert release %s: %w", rg.ID, err) } releases = append(releases, *ext) } - return releases, nil -} - -// SyncArtistDiscographyWithFilter works like SyncArtistDiscography but applies -// per-artist type filtering preferences in addition to the base filters. -func SyncArtistDiscographyWithFilter( - ctx context.Context, - client *MusicBrainzClient, - db *database.DB, - artistMBID string, - ttl time.Duration, - artistFilter *ArtistTypeFilter, -) ([]database.ExternalRelease, error) { - // Check context before starting. - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("sync artist discography with filter: %w", err) - } - - // Step 1: Check cache. - cached, err := GetCachedReleases(db, artistMBID, ttl) - if err != nil { - return nil, fmt.Errorf("sync artist discography with filter: cache check failed: %w", err) - } - - // Step 2: If we have cached data, return it. - if cached.CacheHitCount > 0 { - return database.GetExternalReleasesByArtistWithCache(db, artistMBID, ttl) - } - - // Step 3: Cache miss — fetch from MusicBrainz API. - groups, err := client.GetArtistReleaseGroups(ctx, artistMBID) - if err != nil { - return nil, fmt.Errorf("sync artist discography with filter: fetch release groups: %w", err) - } - - // Step 4: Apply filtering with artist-specific type preferences. - filtered := FilterReleaseGroupsWithArtistFilter(groups, artistFilter) - - // Step 5: Upsert each release group into the database. - now := time.Now() - var releases []database.ExternalRelease - for _, rg := range filtered { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("sync artist discography with filter: %w", err) - } - - ext := rg.ToExternalRelease() - ext.CachedAt = now - - if err := database.SaveExternalRelease(db, ext); err != nil { - return nil, fmt.Errorf("sync artist discography with filter: save release %s: %w", rg.ID, err) - } - - releases = append(releases, *ext) + if err := tx.Commit(); err != nil { + return nil, fmt.Errorf("sync artist discography: commit transaction: %w", err) } return releases, nil diff --git a/internal/musicbrainz/sync_test.go b/internal/musicbrainz/sync_test.go index dacdfae..d40ca56 100644 --- a/internal/musicbrainz/sync_test.go +++ b/internal/musicbrainz/sync_test.go @@ -74,9 +74,9 @@ func newTestClient(serverURL string) *MusicBrainzClient { UserAgent: "test-agent/1.0", } return &MusicBrainzClient{ - httpClient: &http.Client{}, - userAgent: cfg.UserAgent, - baseURL: serverURL + "/ws/2", + httpClient: &http.Client{}, + userAgent: cfg.UserAgent, + baseURL: serverURL, rateLimiter: rate.NewLimiter(rate.Limit(100), 100), } } @@ -337,10 +337,12 @@ func TestSyncArtistDiscography_ContextCancellation(t *testing.T) { // Test: SyncArtistDiscography upsert is idempotent (re-sync replaces) // ----------------------------------------------------------------------- -func TestSyncArtistDiscography_IdempotentUpsert(t *testing.T) { +func TestSyncArtistDiscography_IdempotentResync(t *testing.T) { artistMBID := "ffffffff-0000-1111-2222-333333333333" + callCount := 0 server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + callCount++ w.Header().Set("Content-Type", "application/xml") resp := mbReleaseGroupListResponse( mbReleaseGroupXML("rg1", "Album One", "Album", "", artistMBID, "Artist", "2020-01-01")+ @@ -357,28 +359,39 @@ func TestSyncArtistDiscography_IdempotentUpsert(t *testing.T) { client := newTestClient(server.URL) ctx := context.Background() - ttl := 24 * time.Hour // First sync. - releases1, err := SyncArtistDiscography(ctx, client, db, artistMBID, ttl) + releases1, err := SyncArtistDiscography(ctx, client, db, artistMBID, 24*time.Hour) if err != nil { t.Fatalf("first SyncArtistDiscography() error: %v", err) } if len(releases1) != 2 { t.Fatalf("expected 2 releases after first sync, got %d", len(releases1)) } + if callCount != 1 { + t.Fatalf("expected 1 server call after first sync, got %d", callCount) + } - // Second sync should use cache (server would error if called again). - // To verify the cache path, we use a very short TTL so cache expires. - releases2, err := SyncArtistDiscography(ctx, client, db, artistMBID, ttl) + // Force cache expiry by setting cached_at to the past. + _, err = db.Conn().Exec("UPDATE external_releases SET cached_at = ? WHERE artist_id = ?", + time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistMBID) + if err != nil { + t.Fatalf("expire cache: %v", err) + } + + // Second sync should re-fetch from API (cache expired). + releases2, err := SyncArtistDiscography(ctx, client, db, artistMBID, 24*time.Hour) if err != nil { t.Fatalf("second SyncArtistDiscography() error: %v", err) } if len(releases2) != 2 { t.Fatalf("expected 2 releases after second sync, got %d", len(releases2)) } + if callCount != 2 { + t.Fatalf("expected 2 server calls after forced re-sync, got %d", callCount) + } - // Verify no duplicates in the database. + // Verify no duplicates in the database (transactional delete + insert). stored, err := database.GetExternalReleasesByArtist(db, artistMBID) if err != nil { t.Fatalf("GetExternalReleasesByArtist() error: %v", err) @@ -388,118 +401,6 @@ func TestSyncArtistDiscography_IdempotentUpsert(t *testing.T) { } } -// ----------------------------------------------------------------------- -// Test: SyncArtistDiscographyWithFilter applies per-artist type filter -// ----------------------------------------------------------------------- - -func TestSyncArtistDiscographyWithFilter_ArtistTypeFilter(t *testing.T) { - artistMBID := "11111111-2222-3333-4444-555555555555" - - server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/xml") - resp := mbReleaseGroupListResponse( - mbReleaseGroupXML("rg-album", "An Album", "Album", "", artistMBID, "Artist", "2020-01-01")+ - mbReleaseGroupXML("rg-single", "A Single", "Single", "", artistMBID, "Artist", "2020-02-01")+ - mbReleaseGroupXML("rg-ep", "An EP", "EP", "", artistMBID, "Artist", "2020-03-01")+ - mbReleaseGroupXML("rg-comp", "A Compilation", "Compilation", "", artistMBID, "Artist", "2020-04-01"), - 4, - ) - w.Write([]byte(resp)) - }) - defer server.Close() - - db := newTestDB(t) - defer db.Close() - seedArtist(t, db, artistMBID, "Filter Artist") - - client := newTestClient(server.URL) - ctx := context.Background() - ttl := 24 * time.Hour - - // Filter that excludes Singles and Compilations. - filter := &ArtistTypeFilter{ - ArtistID: artistMBID, - IncludeSingles: false, - IncludeCompilations: false, - IncludeEP: true, - } - - releases, err := SyncArtistDiscographyWithFilter(ctx, client, db, artistMBID, ttl, filter) - if err != nil { - t.Fatalf("SyncArtistDiscographyWithFilter() error: %v", err) - } - - // Should only have Album and EP. - if len(releases) != 2 { - t.Fatalf("expected 2 releases with artist filter, got %d", len(releases)) - } - - rgIDs := make(map[string]bool) - for _, r := range releases { - rgIDs[r.RGID] = true - } - if !rgIDs["rg-album"] { - t.Error("expected rg-album in filtered results") - } - if !rgIDs["rg-ep"] { - t.Error("expected rg-ep in filtered results") - } - if rgIDs["rg-single"] { - t.Error("rg-single should have been filtered out") - } - if rgIDs["rg-comp"] { - t.Error("rg-comp should have been filtered out") - } -} - -// ----------------------------------------------------------------------- -// Test: SyncArtistDiscographyWithFilter cache hit -// ----------------------------------------------------------------------- - -func TestSyncArtistDiscographyWithFilter_CacheHit(t *testing.T) { - artistMBID := "22222222-3333-4444-5555-666666666666" - - db := newTestDB(t) - defer db.Close() - seedArtist(t, db, artistMBID, "Cache Hit Filter Artist") - - // Pre-populate cache. - if err := database.SaveExternalRelease(db, &database.ExternalRelease{ - RGID: "rg-old", - ArtistID: artistMBID, - Title: "Old Cached", - Type: "Album", - CachedAt: time.Now(), - }); err != nil { - t.Fatalf("SaveExternalRelease() error: %v", err) - } - - serverCalled := false - server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { - serverCalled = true - w.Header().Set("Content-Type", "application/xml") - w.Write([]byte(mbReleaseGroupListResponse("", 0))) - }) - defer server.Close() - - client := newTestClient(server.URL) - ctx := context.Background() - ttl := 24 * time.Hour - filter := DefaultArtistTypeFilter(artistMBID) - - releases, err := SyncArtistDiscographyWithFilter(ctx, client, db, artistMBID, ttl, filter) - if err != nil { - t.Fatalf("SyncArtistDiscographyWithFilter() error: %v", err) - } - - if serverCalled { - t.Error("expected cache hit but server was called") - } - if len(releases) != 1 { - t.Fatalf("expected 1 cached release, got %d", len(releases)) - } -} - // ----------------------------------------------------------------------- // Test: SyncArtistDiscography with empty response (no release groups) // ----------------------------------------------------------------------- @@ -568,47 +469,12 @@ func TestSyncArtistDiscography_APIError(t *testing.T) { } // ----------------------------------------------------------------------- -// Test: SyncArtistDiscographyWithFilter context cancellation -// ----------------------------------------------------------------------- - -func TestSyncArtistDiscographyWithFilter_ContextCancellation(t *testing.T) { - artistMBID := "55555555-6666-7777-8888-999999999999" - - server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/xml") - w.Write([]byte(mbReleaseGroupListResponse( - mbReleaseGroupXML("rg1", "Album", "Album", "", artistMBID, "Artist", "2020-01-01"), - 1, - ))) - }) - defer server.Close() - - db := newTestDB(t) - defer db.Close() - seedArtist(t, db, artistMBID, "Cancel Filter Artist") - - client := newTestClient(server.URL) - ttl := 24 * time.Hour - filter := DefaultArtistTypeFilter(artistMBID) - - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - _, err := SyncArtistDiscographyWithFilter(ctx, client, db, artistMBID, ttl, filter) - if err == nil { - t.Fatal("SyncArtistDiscographyWithFilter() expected error for cancelled context, got nil") - } -} - -// ----------------------------------------------------------------------- -// Test: Verify XML parsing integration — ensure the full pipeline works -// with real XML structure matching MusicBrainz responses. +// Test: Verify XML parsing integration — full pipeline with realistic XML // ----------------------------------------------------------------------- func TestSyncArtistDiscography_FullXMLPipeline(t *testing.T) { artistMBID := "66666666-7777-8888-9999-000000000000" - // Build a realistic MusicBrainz XML response. xmlBody := ` @@ -660,7 +526,6 @@ func TestSyncArtistDiscography_FullXMLPipeline(t *testing.T) { t.Fatalf("expected 2 releases, got %d", len(releases)) } - // Verify the parsed data. byID := make(map[string]database.ExternalRelease) for _, r := range releases { byID[r.RGID] = r @@ -735,7 +600,7 @@ func TestSyncArtistDiscography_ConsistentCachedAtTimestamp(t *testing.T) { } // ----------------------------------------------------------------------- -// Test: Verify XML unmarshalling edge case — release-group with no type attr +// Test: Verify XML edge case — release-group with no type attribute // ----------------------------------------------------------------------- func TestSyncArtistDiscography_XMLNoTypeAttribute(t *testing.T) { @@ -796,3 +661,82 @@ func TestSyncArtistDiscography_XMLNoTypeAttribute(t *testing.T) { t.Errorf("expected rg-withtype, got %s", releases[0].RGID) } } + +// ----------------------------------------------------------------------- +// Test: Stale releases are cleaned up on re-sync +// ----------------------------------------------------------------------- + +func TestSyncArtistDiscography_CleansStaleReleases(t *testing.T) { + artistMBID := "99999999-0000-1111-2222-333333333333" + + callCount := 0 + server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + callCount++ + w.Header().Set("Content-Type", "application/xml") + if callCount == 1 { + // First call: return 3 releases. + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg-old-1", "Old Album 1", "Album", "", artistMBID, "Artist", "2018-01-01")+ + mbReleaseGroupXML("rg-old-2", "Old Album 2", "Album", "", artistMBID, "Artist", "2019-01-01")+ + mbReleaseGroupXML("rg-old-3", "Old Album 3", "Album", "", artistMBID, "Artist", "2020-01-01"), + 3, + ) + w.Write([]byte(resp)) + } else { + // Second call: return only 2 (one was removed). + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg-old-1", "Old Album 1", "Album", "", artistMBID, "Artist", "2018-01-01")+ + mbReleaseGroupXML("rg-old-2", "Old Album 2", "Album", "", artistMBID, "Artist", "2019-01-01"), + 2, + ) + w.Write([]byte(resp)) + } + }) + defer server.Close() + + db := newTestDB(t) + defer db.Close() + seedArtist(t, db, artistMBID, "Stale Artist") + + client := newTestClient(server.URL) + ctx := context.Background() + + // First sync: 3 releases. + releases1, err := SyncArtistDiscography(ctx, client, db, artistMBID, 24*time.Hour) + if err != nil { + t.Fatalf("first SyncArtistDiscography() error: %v", err) + } + if len(releases1) != 3 { + t.Fatalf("expected 3 releases after first sync, got %d", len(releases1)) + } + + // Force cache expiry by setting cached_at to the past. + _, err = db.Conn().Exec("UPDATE external_releases SET cached_at = ? WHERE artist_id = ?", + time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistMBID) + if err != nil { + t.Fatalf("expire cache: %v", err) + } + + // Second sync should re-fetch from API (cache expired). + releases2, err := SyncArtistDiscography(ctx, client, db, artistMBID, 24*time.Hour) + if err != nil { + t.Fatalf("second SyncArtistDiscography() error: %v", err) + } + if len(releases2) != 2 { + t.Fatalf("expected 2 releases after second sync, got %d", len(releases2)) + } + + // Verify stale release was cleaned from DB. + stored, err := database.GetExternalReleasesByArtist(db, artistMBID) + if err != nil { + t.Fatalf("GetExternalReleasesByArtist() error: %v", err) + } + if len(stored) != 2 { + t.Errorf("expected 2 stored releases (stale cleaned), got %d", len(stored)) + } + for _, r := range stored { + if r.RGID == "rg-old-3" { + t.Error("stale release rg-old-3 should have been removed") + } + } +}