diff --git a/internal/database/database.go b/internal/database/database.go index da3858a..848512f 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -117,6 +117,10 @@ func (db *DB) migrate() error { name: "005_add_cached_at_to_external_releases", sql: `ALTER TABLE external_releases ADD COLUMN cached_at DATETIME;`, }, + { + name: "006_add_secondary_types_to_external_releases", + sql: `ALTER TABLE external_releases ADD COLUMN secondary_types TEXT;`, + }, } for _, m := range migrations { @@ -179,13 +183,14 @@ type LocalAlbum struct { // ExternalRelease represents a row in the external_releases table. type ExternalRelease struct { - RGID string `json:"rgid"` - ArtistID string `json:"artist_id"` - Title string `json:"title"` - Type string `json:"type"` - ReleaseDate string `json:"release_date"` - IsIgnored bool `json:"is_ignored"` - CachedAt time.Time `json:"cached_at"` + RGID string `json:"rgid"` + ArtistID string `json:"artist_id"` + Title string `json:"title"` + Type string `json:"type"` + ReleaseDate string `json:"release_date"` + IsIgnored bool `json:"is_ignored"` + CachedAt time.Time `json:"cached_at"` + SecondaryTypes []string `json:"secondary_types"` } // NotificationSent represents a row in the notifications_sent table. diff --git a/internal/database/database_test.go b/internal/database/database_test.go index 291a979..a894b0e 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -205,8 +205,9 @@ func TestMigrationTracking(t *testing.T) { t.Fatalf("query migrations count: %v", err) } - // We have 5 recorded migrations: artist_settings, external_releases, local_albums, notifications_sent, cached_at column. - if count != 5 { - t.Errorf("expected 5 applied migrations, got %d", count) + // We have 6 recorded migrations: artist_settings, external_releases, + // local_albums, notifications_sent, cached_at column, secondary_types column. + if count != 6 { + t.Errorf("expected 6 applied migrations, got %d", count) } } diff --git a/internal/database/external_releases.go b/internal/database/external_releases.go index 59b8c01..ca4bccb 100644 --- a/internal/database/external_releases.go +++ b/internal/database/external_releases.go @@ -3,25 +3,52 @@ package database import ( "database/sql" "fmt" + "strings" "time" _ "github.com/mattn/go-sqlite3" ) +// joinSecondaryTypes renders a slice of secondary types as a comma-separated +// string for storage in the secondary_types TEXT column (empty when none). +func joinSecondaryTypes(types []string) string { + return strings.Join(types, ",") +} + +// splitSecondaryTypes parses the comma-separated secondary_types column back +// into a slice. A NULL/empty column yields an empty slice. +func splitSecondaryTypes(s string) []string { + if s == "" { + return nil + } + parts := strings.Split(s, ",") + out := make([]string, 0, len(parts)) + for _, p := range parts { + if p != "" { + out = append(out, p) + } + } + return out +} + // GetExternalRelease retrieves an external_release row by RGID. func GetExternalRelease(db *DB, rgid string) (*ExternalRelease, error) { var r ExternalRelease var cachedAt sql.NullTime + var secondaryTypes sql.NullString err := db.Conn().QueryRow( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at FROM external_releases WHERE rgid = ?", + "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE rgid = ?", rgid, - ).Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt) + ).Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt, &secondaryTypes) if err != nil { return nil, fmt.Errorf("get external release: %w", err) } if cachedAt.Valid { r.CachedAt = cachedAt.Time } + if secondaryTypes.Valid { + r.SecondaryTypes = splitSecondaryTypes(secondaryTypes.String) + } return &r, nil } @@ -32,8 +59,8 @@ func SaveExternalRelease(db *DB, release *ExternalRelease) error { cachedAt = release.CachedAt } _, 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, cachedAt, + "INSERT OR REPLACE INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types) VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + release.RGID, release.ArtistID, release.Title, release.Type, release.ReleaseDate, release.IsIgnored, cachedAt, joinSecondaryTypes(release.SecondaryTypes), ) if err != nil { return fmt.Errorf("save external release: %w", err) @@ -44,7 +71,7 @@ func SaveExternalRelease(db *DB, release *ExternalRelease) error { // GetExternalReleasesByArtist returns all external_release rows for a given artist_id. func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, error) { rows, err := db.Conn().Query( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at FROM external_releases WHERE artist_id = ?", + "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE artist_id = ?", artistID, ) if err != nil { @@ -56,12 +83,16 @@ func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, er for rows.Next() { var r ExternalRelease var cachedAt sql.NullTime - if err := rows.Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt); err != nil { + var secondaryTypes sql.NullString + if err := rows.Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt, &secondaryTypes); err != nil { return nil, fmt.Errorf("scan external release: %w", err) } if cachedAt.Valid { r.CachedAt = cachedAt.Time } + if secondaryTypes.Valid { + r.SecondaryTypes = splitSecondaryTypes(secondaryTypes.String) + } results = append(results, r) } if err := rows.Err(); err != nil { @@ -73,7 +104,7 @@ func GetExternalReleasesByArtist(db *DB, artistID string) ([]ExternalRelease, er // GetIgnoredReleases returns all external_release rows where is_ignored = 1. func GetIgnoredReleases(db *DB) ([]ExternalRelease, error) { rows, err := db.Conn().Query( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at FROM external_releases WHERE is_ignored = 1", + "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE is_ignored = 1", ) if err != nil { return nil, fmt.Errorf("query ignored releases: %w", err) @@ -84,12 +115,16 @@ func GetIgnoredReleases(db *DB) ([]ExternalRelease, error) { for rows.Next() { var r ExternalRelease var cachedAt sql.NullTime - if err := rows.Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt); err != nil { + var secondaryTypes sql.NullString + if err := rows.Scan(&r.RGID, &r.ArtistID, &r.Title, &r.Type, &r.ReleaseDate, &r.IsIgnored, &cachedAt, &secondaryTypes); err != nil { return nil, fmt.Errorf("scan ignored release: %w", err) } if cachedAt.Valid { r.CachedAt = cachedAt.Time } + if secondaryTypes.Valid { + r.SecondaryTypes = splitSecondaryTypes(secondaryTypes.String) + } results = append(results, r) } if err := rows.Err(); err != nil { @@ -122,9 +157,14 @@ func SetReleaseIgnored(db *DB, rgid string, ignored bool) error { // GetExternalReleasesByArtistWithCache returns cached external_release rows for a given artist_id // that are within the specified TTL. func GetExternalReleasesByArtistWithCache(db *DB, artistID string, ttl time.Duration) ([]ExternalRelease, error) { - cutoff := time.Now().UTC().Add(-ttl) + // cached_at is a TEXT DATETIME column serialized by the driver in the + // "2006-01-02 15:04:05" UTC layout. Compare against an explicitly + // formatted cutoff string in the same layout so the lexicographic + // comparison does not depend on the driver's time serialization behavior. + const layout = "2006-01-02 15:04:05" + cutoff := time.Now().UTC().Add(-ttl).Format(layout) rows, err := db.Conn().Query( - "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at FROM external_releases WHERE artist_id = ? AND cached_at >= ?", + "SELECT rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types FROM external_releases WHERE artist_id = ? AND cached_at >= ?", artistID, cutoff, ) if err != nil { @@ -138,7 +178,8 @@ func GetExternalReleasesByArtistWithCache(db *DB, artistID string, ttl time.Dura 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 { + var secondaryTypes sql.NullString + if err := rows.Scan(&r.RGID, &r.ArtistID, &r.Title, &releaseType, &releaseDate, &r.IsIgnored, &cachedAt, &secondaryTypes); err != nil { return nil, fmt.Errorf("scan cached external release: %w", err) } if releaseType.Valid { @@ -150,6 +191,9 @@ func GetExternalReleasesByArtistWithCache(db *DB, artistID string, ttl time.Dura if cachedAt.Valid { r.CachedAt = cachedAt.Time } + if secondaryTypes.Valid { + r.SecondaryTypes = splitSecondaryTypes(secondaryTypes.String) + } results = append(results, r) } if err := rows.Err(); err != nil { diff --git a/internal/musicbrainz/api.go b/internal/musicbrainz/api.go index 5911174..d7f440f 100644 --- a/internal/musicbrainz/api.go +++ b/internal/musicbrainz/api.go @@ -120,13 +120,16 @@ func IsTypeIncluded(releaseType string) bool { // MBID) must NOT be stored here, because artist_settings is keyed by the // Navidrome ID and the foreign key / join would otherwise never match. func (rg *ReleaseGroup) ToExternalRelease(artistID string) *database.ExternalRelease { - // Only the primary type is persisted; secondary types are used transiently - // for filtering above and are not stored in the external_releases schema. + // The primary type and the secondary types are both persisted so that the + // cache-hit path in SyncArtistDiscography can re-apply the same + // IgnoreSingles / IgnoreCompilations rules (which consider secondary types) + // as the cache-miss path, keeping results stable across cache refreshes. return &database.ExternalRelease{ - RGID: rg.ID, - ArtistID: artistID, - Title: rg.Title, - Type: rg.Type, - ReleaseDate: rg.ReleaseDate, + RGID: rg.ID, + ArtistID: artistID, + Title: rg.Title, + Type: rg.Type, + ReleaseDate: rg.ReleaseDate, + SecondaryTypes: rg.SecondaryTypes, } } diff --git a/internal/musicbrainz/sync.go b/internal/musicbrainz/sync.go index 845524f..0f7ff04 100644 --- a/internal/musicbrainz/sync.go +++ b/internal/musicbrainz/sync.go @@ -59,10 +59,10 @@ func SyncArtistDiscography( } filtered := make([]database.ExternalRelease, 0, len(cachedReleases)) for _, r := range cachedReleases { - if opts.IgnoreSingles && r.Type == "Single" { + if opts.IgnoreSingles && (r.Type == "Single" || hasSliceType(r.SecondaryTypes, "Single")) { continue } - if opts.IgnoreCompilations && r.Type == "Compilation" { + if opts.IgnoreCompilations && (r.Type == "Compilation" || hasSliceType(r.SecondaryTypes, "Compilation")) { continue } filtered = append(filtered, r) @@ -152,6 +152,18 @@ func SyncArtistDiscography( return releases, nil } +// hasSliceType reports whether the slice contains the wanted value. It mirrors +// hasSecondaryType in api.go but operates on the persisted []string form read +// back from external_releases (cache-hit path). +func hasSliceType(types []string, wanted string) bool { + for _, t := range types { + if t == wanted { + return true + } + } + return false +} + // getArtistFilterOptions reads per-artist type filtering preferences. // Defaults to no filtering if artist_settings row doesn't exist. // artistID is the Navidrome artist ID (artist_settings.id), not the MusicBrainz ID. diff --git a/internal/musicbrainz/sync_test.go b/internal/musicbrainz/sync_test.go index 1dc2f80..a903e0f 100644 --- a/internal/musicbrainz/sync_test.go +++ b/internal/musicbrainz/sync_test.go @@ -827,8 +827,50 @@ func TestSyncArtistDiscography_IgnoreCompilations(t *testing.T) { } // ----------------------------------------------------------------------- -// Test: resync with notifications_sent does not violate FK constraint +// Test: cache-hit path applies secondary-type filtering consistently with the +// cache-miss path. A release whose primary type is "Album" but which is also +// a "Compilation" via its secondary type must be dropped by IgnoreCompilations +// on a cache hit, exactly as FilterReleaseGroups drops it on a cache miss. // ----------------------------------------------------------------------- +func TestSyncArtistDiscography_CacheHit_IgnoreSecondaryCompilation(t *testing.T) { + artistID := "nav-comp-secondary-test" + artistName := "Secondary Comp Artist" + + db := newTestDB(t) + defer db.Close() + if _, err := db.Conn().Exec( + "INSERT INTO artist_settings (id, name, ignore_compilations, monitored) VALUES (?, ?, 1, 1)", + artistID, artistName, + ); err != nil { + t.Fatalf("seed artist: %v", err) + } + + // Seed a cached release: primary "Album" + secondary "Compilation". + // cached_at is set far in the past so it is still within any TTL (TTL 0). + if err := database.SaveExternalRelease(db, &database.ExternalRelease{ + RGID: "rg-comp", + ArtistID: artistID, + Title: "Greatest Hits", + Type: "Album", + SecondaryTypes: []string{"Compilation"}, + IsIgnored: false, + CachedAt: time.Now().UTC(), + }); err != nil { + t.Fatalf("seed cached release: %v", err) + } + + // No MusicBrainz server is started; a cache hit must not hit the API. + client := newTestClient("http://unused.invalid") + ctx := context.Background() + + releases, err := SyncArtistDiscography(ctx, client, db, artistID, "mbid-unused", 0) + if err != nil { + t.Fatalf("SyncArtistDiscography() error: %v", err) + } + if len(releases) != 0 { + t.Fatalf("expected 0 releases (secondary compilation filtered on cache hit), got %d", len(releases)) + } +} func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) { artistMBID := "artist-fk-test" artistID := "nav-fk-test"