From 424be1efc49cff4b46e5e884f484cbc6c1e97e95 Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Tue, 26 May 2026 17:29:05 +0300 Subject: [PATCH] fix: address fourth code review findings - Fix FK constraint violation in SyncArtistDiscography: delete notifications_sent rows before external_releases to prevent constraint failure when re-syncing artists with prior notifications. - Implement per-artist type filtering: FilterReleaseGroups now accepts FilterOptions with IgnoreSingles/IgnoreCompilations flags, read from artist_settings table via getArtistFilterOptions. - Fix inconsistent error wrapping: GetExternalRelease now wraps errors with fmt.Errorf like all other functions in the package; updated test to use errors.Is for sql.ErrNoRows check. - Add tests: FilterReleaseGroups ignore singles/compilations, SyncArtistDiscography per-artist type filtering, and FK-safe resync. --- internal/database/external_releases.go | 3 +- internal/database/external_releases_test.go | 8 +- internal/musicbrainz/api.go | 17 ++- internal/musicbrainz/api_test.go | 42 +++++- internal/musicbrainz/sync.go | 34 ++++- internal/musicbrainz/sync_test.go | 149 ++++++++++++++++++++ 6 files changed, 242 insertions(+), 11 deletions(-) diff --git a/internal/database/external_releases.go b/internal/database/external_releases.go index 6b9b567..753e566 100644 --- a/internal/database/external_releases.go +++ b/internal/database/external_releases.go @@ -9,7 +9,6 @@ import ( ) // GetExternalRelease retrieves an external_release row by RGID. -// Returns sql.ErrNoRows if the release is not found. func GetExternalRelease(db *DB, rgid string) (*ExternalRelease, error) { var r ExternalRelease var cachedAt sql.NullTime @@ -18,7 +17,7 @@ func GetExternalRelease(db *DB, rgid string) (*ExternalRelease, error) { rgid, ).Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt) if err != nil { - return nil, err + return nil, fmt.Errorf("get external release: %w", err) } if cachedAt.Valid { r.CachedAt = cachedAt.Time diff --git a/internal/database/external_releases_test.go b/internal/database/external_releases_test.go index 57d95a3..1ea1786 100644 --- a/internal/database/external_releases_test.go +++ b/internal/database/external_releases_test.go @@ -2,6 +2,7 @@ package database import ( "database/sql" + "errors" "testing" ) @@ -58,7 +59,8 @@ func TestGetExternalRelease_Found(t *testing.T) { } } -// TestGetExternalRelease_NotFound verifies that a missing release returns sql.ErrNoRows. +// TestGetExternalRelease_NotFound verifies that a missing release returns an error +// that wraps sql.ErrNoRows. func TestGetExternalRelease_NotFound(t *testing.T) { db, err := New(":memory:") if err != nil { @@ -67,8 +69,8 @@ func TestGetExternalRelease_NotFound(t *testing.T) { defer db.Close() _, err = GetExternalRelease(db, "nonexistent") - if err != sql.ErrNoRows { - t.Errorf("expected sql.ErrNoRows, got %v", err) + if !errors.Is(err, sql.ErrNoRows) { + t.Errorf("expected error wrapping sql.ErrNoRows, got %v", err) } } diff --git a/internal/musicbrainz/api.go b/internal/musicbrainz/api.go index 36a8ade..69fd98b 100644 --- a/internal/musicbrainz/api.go +++ b/internal/musicbrainz/api.go @@ -82,10 +82,17 @@ func (c *MusicBrainzClient) GetArtistReleaseGroups(ctx context.Context, artistMB return allGroups, nil } +// FilterOptions holds per-artist type filtering preferences. +type FilterOptions struct { + IgnoreSingles bool + IgnoreCompilations bool +} + // FilterReleaseGroups applies status and type filtering to a list of release groups. // It excludes Bootleg, Promotion, and Pseudo-Release statuses. -// It includes only Album, Single, EP, and Compilation types. -func FilterReleaseGroups(groups []ReleaseGroup) []ReleaseGroup { +// It includes only Album, Single, EP, and Compilation types, unless the type +// is disabled via FilterOptions. +func FilterReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGroup { var filtered []ReleaseGroup for _, rg := range groups { if IsStatusExcluded(rg.Status) { @@ -94,6 +101,12 @@ func FilterReleaseGroups(groups []ReleaseGroup) []ReleaseGroup { if !IsTypeIncluded(rg.Type) { continue } + if opts.IgnoreSingles && rg.Type == "Single" { + continue + } + if opts.IgnoreCompilations && rg.Type == "Compilation" { + continue + } filtered = append(filtered, rg) } return filtered diff --git a/internal/musicbrainz/api_test.go b/internal/musicbrainz/api_test.go index c7d1980..354b6e0 100644 --- a/internal/musicbrainz/api_test.go +++ b/internal/musicbrainz/api_test.go @@ -20,7 +20,7 @@ func TestFilterReleaseGroups_ExcludesBootlegPromotionPseudo(t *testing.T) { {ID: "rg-4", Title: "Pseudo Release", Type: "Album", Status: "Pseudo-Release"}, } - result := FilterReleaseGroups(groups) + result := FilterReleaseGroups(groups, FilterOptions{}) if len(result) != 1 { t.Fatalf("FilterReleaseGroups() returned %d groups, want 1", len(result)) @@ -41,7 +41,7 @@ func TestFilterReleaseGroups_IncludesOnlyAllowedTypes(t *testing.T) { {ID: "rg-7", Title: "Remix", Type: "Remix", Status: "Official"}, } - result := FilterReleaseGroups(groups) + result := FilterReleaseGroups(groups, FilterOptions{}) if len(result) != 4 { t.Fatalf("FilterReleaseGroups() returned %d groups, want 4", len(result)) @@ -55,6 +55,44 @@ func TestFilterReleaseGroups_IncludesOnlyAllowedTypes(t *testing.T) { } } +func TestFilterReleaseGroups_IgnoreSingles(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"}, + } + + result := FilterReleaseGroups(groups, FilterOptions{IgnoreSingles: true}) + + if len(result) != 2 { + t.Fatalf("FilterReleaseGroups() returned %d groups, want 2", len(result)) + } + for _, rg := range result { + if rg.Type == "Single" { + t.Errorf("single %q should have been filtered out", rg.ID) + } + } +} + +func TestFilterReleaseGroups_IgnoreCompilations(t *testing.T) { + groups := []ReleaseGroup{ + {ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"}, + {ID: "rg-2", Title: "Compilation", Type: "Compilation", Status: "Official"}, + {ID: "rg-3", Title: "EP", Type: "EP", Status: "Official"}, + } + + result := FilterReleaseGroups(groups, FilterOptions{IgnoreCompilations: true}) + + if len(result) != 2 { + t.Fatalf("FilterReleaseGroups() returned %d groups, want 2", len(result)) + } + for _, rg := range result { + if rg.Type == "Compilation" { + t.Errorf("compilation %q should have been filtered out", rg.ID) + } + } +} + // ---------- NormalizeString tests ---------- func TestNormalizeString_Basic(t *testing.T) { diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go index 282537b..af0f60f 100644 --- a/internal/musicbrainz/sync.go +++ b/internal/musicbrainz/sync.go @@ -2,6 +2,7 @@ package musicbrainz import ( "context" + "database/sql" "fmt" "time" @@ -51,8 +52,12 @@ func SyncArtistDiscography( return nil, fmt.Errorf("sync artist discography: fetch release groups for artist %s: %w", artistMBID, err) } - // Step 4: Apply filtering. - filtered := FilterReleaseGroups(groups) + // Step 4: Apply filtering with per-artist type preferences. + opts, err := getArtistFilterOptions(db, artistMBID) + if err != nil { + return nil, fmt.Errorf("sync artist discography: read artist filter options: %w", err) + } + filtered := FilterReleaseGroups(groups, opts) // Step 5: Upsert within a transaction — delete old entries first, then insert new ones. now := time.Now().UTC() @@ -80,6 +85,14 @@ func SyncArtistDiscography( rows.Close() // Delete old entries for this artist to avoid stale records. + // Must delete notifications_sent first to avoid FK violation since + // notifications_sent.rgid references external_releases.rgid. + if _, err := tx.Exec( + "DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ?)", + artistMBID, + ); err != nil { + return nil, fmt.Errorf("sync artist discography: delete old notifications: %w", err) + } 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) } @@ -115,3 +128,20 @@ func SyncArtistDiscography( return releases, nil } + +// getArtistFilterOptions reads per-artist type filtering preferences. +// Defaults to no filtering if artist_settings row doesn't exist. +func getArtistFilterOptions(db *database.DB, artistMBID string) (FilterOptions, error) { + var opts FilterOptions + err := db.Conn().QueryRow( + "SELECT COALESCE(ignore_singles, 0), COALESCE(ignore_compilations, 0) FROM artist_settings WHERE id = ?", + artistMBID, + ).Scan(&opts.IgnoreSingles, &opts.IgnoreCompilations) + if err == sql.ErrNoRows { + return opts, nil + } + if err != nil { + return opts, fmt.Errorf("query artist filter options: %w", err) + } + return opts, nil +} diff --git a/internal/musicbrainz/sync_test.go b/internal/musicbrainz/sync_test.go index d40ca56..e9a5f71 100644 --- a/internal/musicbrainz/sync_test.go +++ b/internal/musicbrainz/sync_test.go @@ -740,3 +740,152 @@ func TestSyncArtistDiscography_CleansStaleReleases(t *testing.T) { } } } + +// ----------------------------------------------------------------------- +// Test: per-artist ignore_singles filters out Single type +// ----------------------------------------------------------------------- +func TestSyncArtistDiscography_IgnoreSingles(t *testing.T) { + artistMBID := "artist-singles-test" + artistName := "Singles Artist" + + server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01")+ + mbReleaseGroupXML("rg-2", "Single", "Single", "Official", artistMBID, artistName, "2024-02-01"), + 2, + ) + w.Write([]byte(resp)) + }) + defer server.Close() + + db := newTestDB(t) + defer db.Close() + // Seed artist with ignore_singles = true. + if _, err := db.Conn().Exec( + "INSERT INTO artist_settings (id, name, ignore_singles, monitored) VALUES (?, ?, 1, 1)", + artistMBID, artistName, + ); err != nil { + t.Fatalf("seed artist: %v", err) + } + + client := newTestClient(server.URL) + ctx := context.Background() + + releases, err := SyncArtistDiscography(ctx, client, db, artistMBID, 0) + if err != nil { + t.Fatalf("SyncArtistDiscography() error: %v", err) + } + if len(releases) != 1 { + t.Fatalf("expected 1 release (singles filtered), got %d", len(releases)) + } + if releases[0].Type != "Album" { + t.Errorf("expected type Album, got %s", releases[0].Type) + } +} + +// ----------------------------------------------------------------------- +// Test: per-artist ignore_compilations filters out Compilation type +// ----------------------------------------------------------------------- +func TestSyncArtistDiscography_IgnoreCompilations(t *testing.T) { + artistMBID := "artist-comp-test" + artistName := "Comp Artist" + + server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01")+ + mbReleaseGroupXML("rg-2", "Best Of", "Compilation", "Official", artistMBID, artistName, "2024-02-01"), + 2, + ) + w.Write([]byte(resp)) + }) + defer server.Close() + + db := newTestDB(t) + defer db.Close() + // Seed artist with ignore_compilations = true. + if _, err := db.Conn().Exec( + "INSERT INTO artist_settings (id, name, ignore_compilations, monitored) VALUES (?, ?, 1, 1)", + artistMBID, artistName, + ); err != nil { + t.Fatalf("seed artist: %v", err) + } + + client := newTestClient(server.URL) + ctx := context.Background() + + releases, err := SyncArtistDiscography(ctx, client, db, artistMBID, 0) + if err != nil { + t.Fatalf("SyncArtistDiscography() error: %v", err) + } + if len(releases) != 1 { + t.Fatalf("expected 1 release (compilations filtered), got %d", len(releases)) + } + if releases[0].Type != "Album" { + t.Errorf("expected type Album, got %s", releases[0].Type) + } +} + +// ----------------------------------------------------------------------- +// Test: resync with notifications_sent does not violate FK constraint +// ----------------------------------------------------------------------- +func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) { + artistMBID := "artist-fk-test" + artistName := "FK Artist" + + server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) { + resp := mbReleaseGroupListResponse( + mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01"), + 1, + ) + w.Write([]byte(resp)) + }) + defer server.Close() + + db := newTestDB(t) + defer db.Close() + seedArtist(t, db, artistMBID, artistName) + + client := newTestClient(server.URL) + ctx := context.Background() + + // First sync. + _, err := SyncArtistDiscography(ctx, client, db, artistMBID, 24*time.Hour) + if err != nil { + t.Fatalf("first SyncArtistDiscography() error: %v", err) + } + + // Insert a notifications_sent row referencing the release. + _, err = db.Conn().Exec( + "INSERT INTO notifications_sent (rgid) VALUES (?)", "rg-1", + ) + if err != nil { + t.Fatalf("insert notification: %v", err) + } + + // Force cache expiry. + _, 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 succeed without FK violation. + releases, err := SyncArtistDiscography(ctx, client, db, artistMBID, 24*time.Hour) + if err != nil { + t.Fatalf("second SyncArtistDiscography() error (FK violation?): %v", err) + } + if len(releases) != 1 { + t.Fatalf("expected 1 release after resync, got %d", len(releases)) + } + + // Notification should have been cleaned up. + var count int + err = db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rg-1").Scan(&count) + if err != nil { + t.Fatalf("count notifications: %v", err) + } + if count != 0 { + t.Errorf("expected 0 notifications after resync, got %d", count) + } +} +