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 20:53:06 +03:00
parent da8b8aa944
commit 2468859435
9 changed files with 34 additions and 228 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
internal/database/external_releases.go
View File

@@ -169,6 +169,12 @@ 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) {
// A zero or negative TTL means "no cache" — always expire. Returning early
// here avoids the boundary pitfall where cutoff == now would treat rows
// cached in the current second as fresh.
if ttl <= 0 {
return nil, nil
}
// cached_at is stored in the "2006-01-02 15:04:05" UTC layout via
// FormatCachedAt. Compare against an explicitly formatted cutoff string in
// the same layout so the lexicographic comparison is a valid time ordering.

12
internal/musicbrainz/api.go
View File

@@ -80,13 +80,13 @@ type FilterOptions struct {
func FilterReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGroup {
var filtered []ReleaseGroup
for _, rg := range groups {
if !IsTypeIncluded(rg.Type) && !hasSecondaryType(rg, "Single", "EP", "Compilation") {
if !IsTypeIncluded(rg.Type) && !hasSliceType(rg.SecondaryTypes, "Single", "EP", "Compilation") {
continue
}
if opts.IgnoreSingles && (rg.Type == "Single" || hasSecondaryType(rg, "Single")) {
if opts.IgnoreSingles && (rg.Type == "Single" || hasSliceType(rg.SecondaryTypes, "Single")) {
continue
}
if opts.IgnoreCompilations && (rg.Type == "Compilation" || hasSecondaryType(rg, "Compilation")) {
if opts.IgnoreCompilations && (rg.Type == "Compilation" || hasSliceType(rg.SecondaryTypes, "Compilation")) {
continue
}
filtered = append(filtered, rg)
@@ -106,12 +106,6 @@ func hasSliceType(types []string, wanted ...string) bool {
return false
}
// hasSecondaryType reports whether any of the release group's secondary types
// matches one of the provided values.
func hasSecondaryType(rg ReleaseGroup, wanted ...string) bool {
return hasSliceType(rg.SecondaryTypes, wanted...)
}
// IsTypeIncluded returns true if the given primary type is in the base
// included set (Album/Single/EP).
func IsTypeIncluded(releaseType string) bool {

19
internal/musicbrainz/cache.go
View File

@@ -1,19 +0,0 @@
package musicbrainz
import (
"fmt"
"time"
"naviwatcher/internal/database"
)
// GetCachedReleases queries the external_releases table for entries
// belonging to the given artist that were cached within the specified TTL.
// It returns the cached releases and any error encountered.
func GetCachedReleases(db *database.DB, artistID string, ttl time.Duration) ([]database.ExternalRelease, error) {
releases, err := database.GetExternalReleasesByArtistWithCache(db, artistID, ttl)
if err != nil {
return nil, fmt.Errorf("get cached releases: %w", err)
}
return releases, nil
}

184
internal/musicbrainz/cache_test.go
View File

@@ -1,184 +0,0 @@
package musicbrainz
import (
"testing"
"time"
"naviwatcher/internal/database"
)
func insertTestArtistForCache(db *database.DB, id string) error {
_, err := db.Conn().Exec(
"INSERT OR IGNORE INTO artist_settings (id, name) VALUES (?, ?)",
id, "Test Artist "+id,
)
return err
}
func TestGetCachedReleases_CacheHit(t *testing.T) {
db, err := database.New(":memory:")
if err != nil {
t.Fatalf("New() error: %v", err)
}
defer db.Close()
artistID := "artist-cache-hit"
if err := insertTestArtistForCache(db, artistID); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
// Insert releases with recent cached_at timestamps
now := time.Now().Format("2006-01-02 15:04:05")
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, type, cached_at) VALUES (?, ?, ?, ?, ?)",
"rg-hit-1", artistID, "Cached Album 1", "album", now,
)
if err != nil {
t.Fatalf("insert rg-hit-1: %v", err)
}
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, type, cached_at) VALUES (?, ?, ?, ?, ?)",
"rg-hit-2", artistID, "Cached Album 2", "single", now,
)
if err != nil {
t.Fatalf("insert rg-hit-2: %v", err)
}
ttl := 24 * time.Hour
releases, err := GetCachedReleases(db, artistID, ttl)
if err != nil {
t.Fatalf("GetCachedReleases() error: %v", err)
}
if len(releases) != 2 {
t.Errorf("GetCachedReleases() returned %d releases, want 2", len(releases))
}
}
func TestGetCachedReleases_CacheMiss_Expired(t *testing.T) {
db, err := database.New(":memory:")
if err != nil {
t.Fatalf("New() error: %v", err)
}
defer db.Close()
artistID := "artist-cache-miss"
if err := insertTestArtistForCache(db, artistID); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
// Insert a release with an expired cached_at (48 hours ago)
expired := time.Now().Add(-48 * time.Hour).Format("2006-01-02 15:04:05")
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, type, cached_at) VALUES (?, ?, ?, ?, ?)",
"rg-expired", artistID, "Expired Album", "album", expired,
)
if err != nil {
t.Fatalf("insert expired release: %v", err)
}
ttl := 24 * time.Hour
releases, err := GetCachedReleases(db, artistID, ttl)
if err != nil {
t.Fatalf("GetCachedReleases() error: %v", err)
}
if len(releases) != 0 {
t.Errorf("GetCachedReleases() returned %d releases, want 0 (expired entry should not be cached)", len(releases))
}
}
func TestGetCachedReleases_CacheMiss_NoCachedAt(t *testing.T) {
db, err := database.New(":memory:")
if err != nil {
t.Fatalf("New() error: %v", err)
}
defer db.Close()
artistID := "artist-no-cached"
if err := insertTestArtistForCache(db, artistID); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
// Insert a release WITHOUT cached_at (NULL)
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, type) VALUES (?, ?, ?, ?)",
"rg-nocached", artistID, "Uncached Album", "album",
)
if err != nil {
t.Fatalf("insert uncached release: %v", err)
}
ttl := 24 * time.Hour
releases, err := GetCachedReleases(db, artistID, ttl)
if err != nil {
t.Fatalf("GetCachedReleases() error: %v", err)
}
if len(releases) != 0 {
t.Errorf("GetCachedReleases() returned %d releases, want 0 (NULL cached_at should not be cached)", len(releases))
}
}
func TestGetCachedReleases_EmptyArtist(t *testing.T) {
db, err := database.New(":memory:")
if err != nil {
t.Fatalf("New() error: %v", err)
}
defer db.Close()
ttl := 24 * time.Hour
releases, err := GetCachedReleases(db, "nonexistent-artist", ttl)
if err != nil {
t.Fatalf("GetCachedReleases() error: %v", err)
}
if len(releases) != 0 {
t.Errorf("GetCachedReleases() returned %d releases, want 0 for nonexistent artist", len(releases))
}
}
func TestGetCachedReleases_MixedExpiry(t *testing.T) {
db, err := database.New(":memory:")
if err != nil {
t.Fatalf("New() error: %v", err)
}
defer db.Close()
artistID := "artist-mixed"
if err := insertTestArtistForCache(db, artistID); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
now := time.Now().Format("2006-01-02 15:04:05")
expired := time.Now().Add(-48 * time.Hour).Format("2006-01-02 15:04:05")
// Mix of fresh and expired
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, cached_at) VALUES (?, ?, ?, ?)",
"rg-fresh", artistID, "Fresh Album", now,
)
if err != nil {
t.Fatalf("insert fresh: %v", err)
}
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, cached_at) VALUES (?, ?, ?, ?)",
"rg-old", artistID, "Old Album", expired,
)
if err != nil {
t.Fatalf("insert old: %v", err)
}
ttl := 24 * time.Hour
releases, err := GetCachedReleases(db, artistID, ttl)
if err != nil {
t.Fatalf("GetCachedReleases() error: %v", err)
}
if len(releases) != 1 {
t.Errorf("GetCachedReleases() returned %d releases, want 1 (only fresh entry)", len(releases))
}
if len(releases) > 0 && releases[0].RGID != "rg-fresh" {
t.Errorf("expected rg-fresh, got %s", releases[0].RGID)
}
}

2
internal/musicbrainz/sync.go
View File

@@ -39,7 +39,7 @@ func SyncArtistDiscography(
}
// Step 1: Check cache.
cachedReleases, err := GetCachedReleases(db, artistID, ttl)
cachedReleases, err := database.GetExternalReleasesByArtistWithCache(db, artistID, ttl)
if err != nil {
return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err)
}

6
internal/musicbrainz/sync_test.go
View File

@@ -846,7 +846,7 @@ func TestSyncArtistDiscography_CacheHit_IgnoreSecondaryCompilation(t *testing.T)
}
// 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).
// cached_at is set to the recent past so it is well within the 24h TTL.
if err := database.SaveExternalRelease(db, &database.ExternalRelease{
RGID: "rg-comp",
ArtistID: artistID,
@@ -854,7 +854,7 @@ func TestSyncArtistDiscography_CacheHit_IgnoreSecondaryCompilation(t *testing.T)
Type: "Album",
SecondaryTypes: []string{"Compilation"},
IsIgnored: false,
CachedAt: time.Now().UTC(),
CachedAt: time.Now().UTC().Add(-time.Hour),
}); err != nil {
t.Fatalf("seed cached release: %v", err)
}
@@ -863,7 +863,7 @@ func TestSyncArtistDiscography_CacheHit_IgnoreSecondaryCompilation(t *testing.T)
client := newTestClient("http://unused.invalid")
ctx := context.Background()
releases, err := SyncArtistDiscography(ctx, client, db, artistID, "mbid-unused", 0)
releases, err := SyncArtistDiscography(ctx, client, db, artistID, "mbid-unused", 24*time.Hour)
if err != nil {
t.Fatalf("SyncArtistDiscography() error: %v", err)
}

6
internal/normalize/normalize.go
View File

@@ -16,13 +16,13 @@ import (
var (
bracketRe = regexp.MustCompile(`\[[^\]]*\]`)
parenRe = regexp.MustCompile(`\([^)]*\)`)
yearRe = regexp.MustCompile(`\b(1[0-9]{3}|2[0-9]{3})\b`)
yearRe = regexp.MustCompile(`\b[0-9]{4}\b`)
spaceRe = regexp.MustCompile(`\s+`)
// bareYearRe matches a title that is *only* a single year (with optional
// surrounding whitespace), e.g. "1989" or "2112". Used to decide whether a
// title that collapses entirely to a year should keep it (so it matches
// itself) or be treated as a distinct reissue that must collapse to empty.
bareYearRe = regexp.MustCompile(`^\s*(1[0-9]{3}|2[0-9]{3})\s*$`)
bareYearRe = regexp.MustCompile(`^\s*[0-9]{4}\s*$`)
)
// NormalizeString normalizes a string for fuzzy matching by:
@@ -46,7 +46,7 @@ func NormalizeString(s string) string {
// Remove parenthesized content (e.g., (Deluxe), (Remastered))
s = parenRe.ReplaceAllString(s, "")
// Remove years (4-digit numbers between 1000-2999). If stripping the year
// Remove years (any 4-digit number). If stripping the year
// empties the entire string, decide what to keep:
// - A bare year title (e.g. "1989", "2112") has no other words, so keep
// the year so it can still match itself (the user owns that album).

11
internal/normalize/normalize_test.go
View File

@@ -48,8 +48,17 @@ func TestNormalizeString_Basic(t *testing.T) {
// the bare year (it falsely matched "1989" before). It collapses to empty.
{"1989 [2020]", ""},
{"1989 2020", ""},
{"3000 2000", "3000"},
// Both tokens are years → both stripped → empty (no album words remain).
{"3000 2000", ""},
{"1989 RMX", "rmx"},
// Regression: year regex must cover ALL 4-digit years, not just 1000-2999.
// A reissue of a year-titled album outside that range must still collapse
// to empty so it is correctly reported as missing and does NOT falsely
// match a bare year-titled local album.
{"3000", "3000"},
{"3000 (Remastered)", ""},
{"3010 [Deluxe Edition]", ""},
{"4000 (Remastered)", ""},
}
for _, tt := range tests {