Mrixs/NaviWatcher
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address code review findings

Browse Source
This commit is contained in:
Vladimir Zagainov
2026-07-19 19:52:27 +03:00
parent a7803615cd
commit 0bee9b9b27
6 changed files with 138 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

19
internal/database/database.go
View File

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

7
internal/database/database_test.go
View File

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

66
internal/database/external_releases.go
View File

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

17
internal/musicbrainz/api.go
View File

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

16
internal/musicbrainz/sync.go
View File

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

44
internal/musicbrainz/sync_test.go
View File

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