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

fix: address code review findings

Browse Source 
- Fix notifications_sent PK: changed from (rgid, sent_at) to rgid-only PK
  to prevent duplicate RGID rows across seconds. Use INSERT OR REPLACE
  instead of INSERT OR IGNORE for true idempotency.
- Add foreign key constraints to DDL (artist_id references artist_settings,
  rgid references external_releases) per specification.
- Enable PRAGMA foreign_keys=ON and PRAGMA busy_timeout=5000 for concurrent
  access safety.
- Fix GetNotificationSentAt query: add ORDER BY sent_at DESC LIMIT 1 for
  deterministic results.
- Fix config test: change YAML key from 'chat' to 'chat_id' to match struct
  tag, add ChatID assertion.
- Fix migration tracking test: correct error message from "expected 4" to
  "expected 3".
- Remove dead code in TestLoadConfig_InvalidPort: eliminate unused YAML
  template and remove port 0 case (valid, not invalid).
- Remove unused path parameter from buildConfigWithPort helper.
- Remove pointless 100ms sleep in run() and unused time import.
- Remove tautological TestDefaultConfigPath test.
- Update README.md: Go version 1.21+ to 1.25+, placeholder passwords to
  CHANGE_ME.
- Update config.yaml.example: placeholder passwords to CHANGE_ME.
- Update all database tests to insert parent rows first for FK satisfaction.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Vladimir Zagainov
2026-05-20 11:07:28 +03:00
parent fb74b5fbba
commit 47d4ec4e32
11 changed files with 273 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
internal/config/config_test.go
View File

@@ -30,7 +30,7 @@ musicbrainz:
telegram:
enabled: true
token: "abc123"
chat: "chat456"
chat_id: "chat456"
cron_schedule: "0 10 * * *"
scanner:
@@ -59,6 +59,9 @@ scanner:
if cfg.Navidrome.URL != "http://navidrome:4533" {
t.Errorf("expected navidrome url http://navidrome:4533, got %s", cfg.Navidrome.URL)
}
if cfg.Telegram.ChatID != "chat456" {
t.Errorf("expected telegram chat_id chat456, got %s", cfg.Telegram.ChatID)
}
if cfg.Navidrome.User != "service" {
t.Errorf("expected navidrome user service, got %s", cfg.Navidrome.User)
}
@@ -137,7 +140,6 @@ func TestLoadConfig_InvalidPort(t *testing.T) {
name string
port int
}{
{"port zero with explicit 0 and no default override", 0},
{"port too high", 70000},
{"port negative", -1},
}
@@ -147,32 +149,12 @@ func TestLoadConfig_InvalidPort(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "config.yaml")
yaml := `
navidrome:
url: "http://localhost:4533"
user: "u"
password: "p"
musicbrainz:
user_agent: "NaviWatcher/1.0 ( test@example.com )"
server:
port: ` + string(rune('0'+tt.port%10)) + `
`
// Use Sprintf for proper port formatting
yaml = buildConfigWithPort(path, tt.port)
yaml := buildConfigWithPort(tt.port)
if err := os.WriteFile(path, []byte(yaml), 0644); err != nil {
t.Fatalf("failed to write config: %v", err)
}
_, err := LoadConfig(path)
if tt.port == 0 {
// port 0 gets defaulted to 8080, which is valid
if err != nil {
t.Fatalf("port 0 should get default, got error: %v", err)
}
return
}
if err == nil {
t.Fatalf("expected error for port %d, got nil", tt.port)
}
@@ -180,7 +162,7 @@ server:
}
}
func buildConfigWithPort(path string, port int) string {
func buildConfigWithPort(port int) string {
return "navidrome:\n url: \"http://localhost:4533\"\n user: \"u\"\n password: \"p\"\n\nmusicbrainz:\n user_agent: \"NaviWatcher/1.0 ( test@example.com )\"\n\nserver:\n port: " + fmt.Sprintf("%d", port) + "\n"
}
@@ -349,7 +331,7 @@ func TestValidate_BoundaryPort(t *testing.T) {
dir := t.TempDir()
path := filepath.Join(dir, "config.yaml")
yaml := buildConfigWithPort(path, tt.port)
yaml := buildConfigWithPort(tt.port)
if err := os.WriteFile(path, []byte(yaml), 0644); err != nil {
t.Fatalf("failed to write config: %v", err)
}

19
internal/database/database.go
View File

@@ -26,6 +26,18 @@ func New(dbPath string) (*DB, error) {
return nil, fmt.Errorf("set WAL mode: %w", err)
}
// Enable foreign key enforcement.
if _, err := conn.Exec("PRAGMA foreign_keys=ON"); err != nil {
conn.Close()
return nil, fmt.Errorf("enable foreign keys: %w", err)
}
// Set busy timeout to handle concurrent write contention.
if _, err := conn.Exec("PRAGMA busy_timeout=5000"); err != nil {
conn.Close()
return nil, fmt.Errorf("set busy timeout: %w", err)
}
db := &DB{conn: conn}
if err := db.migrate(); err != nil {
conn.Close()
@@ -74,7 +86,7 @@ func (db *DB) migrate() error {
name: "002_create_external_releases",
sql: `CREATE TABLE IF NOT EXISTS external_releases (
rgid TEXT PRIMARY KEY,
artist_id TEXT NOT NULL,
artist_id TEXT NOT NULL REFERENCES artist_settings(id),
title TEXT NOT NULL,
type TEXT,
release_date TEXT,
@@ -84,9 +96,8 @@ func (db *DB) migrate() error {
{
name: "003_create_notifications_sent",
sql: `CREATE TABLE IF NOT EXISTS notifications_sent (
rgid TEXT NOT NULL,
sent_at DATETIME DEFAULT CURRENT_TIMESTAMP,
PRIMARY KEY (rgid, sent_at)
rgid TEXT PRIMARY KEY REFERENCES external_releases(rgid),
sent_at DATETIME DEFAULT CURRENT_TIMESTAMP
);`,
},
}

16
internal/database/database_test.go
View File

@@ -119,6 +119,11 @@ func TestExternalReleasesSchema(t *testing.T) {
}
defer db.Close()
// Insert parent artist first (FK requirement).
_, err = db.Conn().Exec("INSERT INTO artist_settings (id, name) VALUES (?, ?)", "artist-1", "Test Artist")
if err != nil {
t.Fatalf("insert artist: %v", err)
}
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored) VALUES (?, ?, ?, ?, ?, ?)",
"rgid-1", "artist-1", "Test Album", "album", "2024-01-01", false,
@@ -150,6 +155,15 @@ func TestNotificationsSentSchema(t *testing.T) {
}
defer db.Close()
// Insert parent artist and release first (FK requirements).
_, err = db.Conn().Exec("INSERT INTO artist_settings (id, name) VALUES (?, ?)", "artist-1", "Test Artist")
if err != nil {
t.Fatalf("insert artist: %v", err)
}
_, err = db.Conn().Exec("INSERT INTO external_releases (rgid, artist_id, title) VALUES (?, ?, ?)", "rgid-1", "artist-1", "Test Album")
if err != nil {
t.Fatalf("insert release: %v", err)
}
_, err = db.Conn().Exec(
"INSERT INTO notifications_sent (rgid) VALUES (?)",
"rgid-1",
@@ -192,6 +206,6 @@ func TestMigrationTracking(t *testing.T) {
// We have 3 recorded migrations: artist_settings, external_releases, notifications_sent.
if count != 3 {
t.Errorf("expected 4 applied migrations, got %d", count)
t.Errorf("expected 3 applied migrations, got %d", count)
}
}

38
internal/database/external_releases_test.go
View File

@@ -5,6 +5,15 @@ import (
"testing"
)
// insertTestArtist inserts a minimal artist_settings row for use in tests that need FK satisfaction.
func insertTestArtist(db *DB, id string) error {
_, err := db.Conn().Exec(
"INSERT OR IGNORE INTO artist_settings (id, name) VALUES (?, ?)",
id, "Test Artist "+id,
)
return err
}
// TestGetExternalRelease_Found verifies retrieving an existing release.
func TestGetExternalRelease_Found(t *testing.T) {
db, err := New(":memory:")
@@ -13,7 +22,9 @@ func TestGetExternalRelease_Found(t *testing.T) {
}
defer db.Close()
// Insert a row directly.
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
_, err = db.Conn().Exec(
"INSERT INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored) VALUES (?, ?, ?, ?, ?, ?)",
"rgid-1", "artist-1", "Test Album", "album", "2024-01-01", false,
@@ -78,6 +89,9 @@ func TestSaveExternalRelease_Insert(t *testing.T) {
IsIgnored: false,
}
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
if err := SaveExternalRelease(db, r); err != nil {
t.Fatalf("SaveExternalRelease() error: %v", err)
}
@@ -102,6 +116,10 @@ func TestSaveExternalRelease_Update(t *testing.T) {
}
defer db.Close()
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
// Insert initial row.
r1 := &ExternalRelease{
RGID: "rgid-1",
@@ -177,6 +195,12 @@ func TestGetExternalReleasesByArtist_Populated(t *testing.T) {
{RGID: "rg3", ArtistID: "artist-b", Title: "Album B1", Type: "single", ReleaseDate: "2024-03-01"},
}
if err := insertTestArtist(db, "artist-a"); err != nil {
t.Fatalf("insertTestArtist(artist-a): %v", err)
}
if err := insertTestArtist(db, "artist-b"); err != nil {
t.Fatalf("insertTestArtist(artist-b): %v", err)
}
for _, r := range releases {
if err := SaveExternalRelease(db, &r); err != nil {
t.Fatalf("SaveExternalRelease(%s) error: %v", r.RGID, err)
@@ -243,6 +267,12 @@ func TestGetIgnoredReleases_Populated(t *testing.T) {
{RGID: "rg3", ArtistID: "a2", Title: "Another Ignored", IsIgnored: true},
}
if err := insertTestArtist(db, "a1"); err != nil {
t.Fatalf("insertTestArtist(a1): %v", err)
}
if err := insertTestArtist(db, "a2"); err != nil {
t.Fatalf("insertTestArtist(a2): %v", err)
}
for _, r := range releases {
if err := SaveExternalRelease(db, &r); err != nil {
t.Fatalf("SaveExternalRelease(%s) error: %v", r.RGID, err)
@@ -279,6 +309,9 @@ func TestSetReleaseIgnored_SetTrue(t *testing.T) {
Title: "Test Album",
IsIgnored: false,
}
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
if err := SaveExternalRelease(db, r); err != nil {
t.Fatalf("SaveExternalRelease() error: %v", err)
}
@@ -310,6 +343,9 @@ func TestSetReleaseIgnored_SetFalse(t *testing.T) {
Title: "Test Album",
IsIgnored: true,
}
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
if err := SaveExternalRelease(db, r); err != nil {
t.Fatalf("SaveExternalRelease() error: %v", err)
}

4
internal/database/notifications.go
View File

@@ -7,7 +7,7 @@ import (
// MarkNotificationSent records that a notification has been sent for the given RGID.
func MarkNotificationSent(db *DB, rgid string) error {
_, err := db.Conn().Exec(
"INSERT OR IGNORE INTO notifications_sent (rgid) VALUES (?)",
"INSERT OR REPLACE INTO notifications_sent (rgid) VALUES (?)",
rgid,
)
if err != nil {
@@ -60,7 +60,7 @@ func GetUnnotifiedReleases(db *DB) ([]ExternalRelease, error) {
func GetNotificationSentAt(db *DB, rgid string) (string, error) {
var sentAt string
err := db.Conn().QueryRow(
"SELECT sent_at FROM notifications_sent WHERE rgid = ?", rgid,
"SELECT sent_at FROM notifications_sent WHERE rgid = ? ORDER BY sent_at DESC LIMIT 1", rgid,
).Scan(&sentAt)
if err != nil {
return "", err

51
internal/database/notifications_test.go
View File

@@ -5,6 +5,15 @@ import (
"testing"
)
// insertTestRelease inserts a minimal external_releases row for use in tests that need FK satisfaction.
func insertTestRelease(db *DB, rgid, artistID string) error {
_, err := db.Conn().Exec(
"INSERT OR IGNORE INTO external_releases (rgid, artist_id, title) VALUES (?, ?, ?)",
rgid, artistID, "Test Release "+rgid,
)
return err
}
// TestMarkNotificationSent_New verifies inserting a new notification record.
func TestMarkNotificationSent_New(t *testing.T) {
db, err := New(":memory:")
@@ -13,6 +22,12 @@ func TestMarkNotificationSent_New(t *testing.T) {
}
defer db.Close()
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
if err := insertTestRelease(db, "rgid-1", "artist-1"); err != nil {
t.Fatalf("insertTestRelease: %v", err)
}
if err := MarkNotificationSent(db, "rgid-1"); err != nil {
t.Fatalf("MarkNotificationSent() error: %v", err)
}
@@ -34,6 +49,12 @@ func TestMarkNotificationSent_Idempotent(t *testing.T) {
}
defer db.Close()
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
if err := insertTestRelease(db, "rgid-1", "artist-1"); err != nil {
t.Fatalf("insertTestRelease: %v", err)
}
if err := MarkNotificationSent(db, "rgid-1"); err != nil {
t.Fatalf("first MarkNotificationSent() error: %v", err)
}
@@ -60,6 +81,12 @@ func TestIsNotificationSent_True(t *testing.T) {
}
defer db.Close()
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
if err := insertTestRelease(db, "rgid-1", "artist-1"); err != nil {
t.Fatalf("insertTestRelease: %v", err)
}
if err := MarkNotificationSent(db, "rgid-1"); err != nil {
t.Fatalf("MarkNotificationSent() error: %v", err)
}
@@ -118,6 +145,12 @@ func TestGetUnnotifiedReleases_AllUnnotified(t *testing.T) {
{RGID: "rg3", ArtistID: "artist-2", Title: "Single 1", Type: "single", ReleaseDate: "2024-03-01"},
}
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist(artist-1): %v", err)
}
if err := insertTestArtist(db, "artist-2"); err != nil {
t.Fatalf("insertTestArtist(artist-2): %v", err)
}
for _, r := range releases {
if err := SaveExternalRelease(db, &r); err != nil {
t.Fatalf("SaveExternalRelease(%s) error: %v", r.RGID, err)
@@ -147,6 +180,12 @@ func TestGetUnnotifiedReleases_SomeNotified(t *testing.T) {
{RGID: "rg3", ArtistID: "artist-2", Title: "Single 1", Type: "single", ReleaseDate: "2024-03-01"},
}
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist(artist-1): %v", err)
}
if err := insertTestArtist(db, "artist-2"); err != nil {
t.Fatalf("insertTestArtist(artist-2): %v", err)
}
for _, r := range releases {
if err := SaveExternalRelease(db, &r); err != nil {
t.Fatalf("SaveExternalRelease(%s) error: %v", r.RGID, err)
@@ -189,6 +228,9 @@ func TestGetUnnotifiedReleases_AllNotified(t *testing.T) {
{RGID: "rg2", ArtistID: "artist-1", Title: "Album 2"},
}
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist(artist-1): %v", err)
}
for _, r := range releases {
if err := SaveExternalRelease(db, &r); err != nil {
t.Fatalf("SaveExternalRelease(%s) error: %v", r.RGID, err)
@@ -236,6 +278,15 @@ func TestMarkNotificationSent_MultipleReleases(t *testing.T) {
}
defer db.Close()
if err := insertTestArtist(db, "artist-1"); err != nil {
t.Fatalf("insertTestArtist: %v", err)
}
for _, rgid := range []string{"rg1", "rg2", "rg3"} {
if err := insertTestRelease(db, rgid, "artist-1"); err != nil {
t.Fatalf("insertTestRelease(%s): %v", rgid, err)
}
}
rgids := []string{"rg1", "rg2", "rg3"}
for _, rgid := range rgids {
if err := MarkNotificationSent(db, rgid); err != nil {