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

@@ -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

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 {
+	if !errors.Is(err, sql.ErrNoRows) {
-		t.Errorf("expected sql.ErrNoRows, got %v", err)
+		t.Errorf("expected error wrapping sql.ErrNoRows, got %v", err)
 	}
 }
 

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.
+// It includes only Album, Single, EP, and Compilation types, unless the type
-func FilterReleaseGroups(groups []ReleaseGroup) []ReleaseGroup {
+// 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

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) {

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.
+	// Step 4: Apply filtering with per-artist type preferences.
-	filtered := FilterReleaseGroups(groups)
+	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
+}

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)
+	}
+}
+