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.
This commit is contained in:
@@ -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 := `<?xml version="1.0" encoding="UTF-8"?>
|
||||
<metadata>
|
||||
<release-group-list count="2">
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user