fix: address code review findings
- Fix duplicate Telegram notifications: SyncArtistDiscography no longer wipes notifications_sent for the whole artist on every cache-miss re-sync; only markers for releases that disappear are pruned (FK-safe via INSERT OR REPLACE + rgid NOT IN (...)). - Cache empty MusicBrainz discographies via a new artist_settings.last_synced column (migration 009) so zero-release artists honor the TTL instead of being re-fetched every cycle. - Wire the Web UI server and Telegram notifier scheduler into main.run/NewApp. - Guard startPeriodicSync against overlapping syncs with a done-channel slot. - Add server.public_url config; NewServerWithConfig derives reachable links and no longer advertises the 0.0.0.0 bind address. - Web handlers: use scanner.ScanArtist per artist, drop always-false releaseIgnored lookup and dead endsWith, thread configured threshold. - Limit :memory: DB pool to one connection so migrations and queries share the same in-memory store.
This commit is contained in:
@@ -82,6 +82,11 @@ server:
|
||||
port: 8080
|
||||
username: "admin"
|
||||
password: "CHANGE_ME"
|
||||
# Externally-reachable base URL for links in Telegram digests (e.g. behind a
|
||||
# reverse proxy). If omitted, links are derived from host:port — but when host
|
||||
# is 0.0.0.0 (the unspecified bind address) no link is emitted, since it is
|
||||
# not reachable from outside the host.
|
||||
public_url: "https://naviwatcher.example.com"
|
||||
|
||||
navidrome:
|
||||
url: "http://localhost:4533"
|
||||
@@ -203,6 +208,10 @@ server:
|
||||
port: 8080
|
||||
username: "admin"
|
||||
password: "CHANGE_ME"
|
||||
# Внешний базовый URL для ссылок в дайджестах Telegram (напр. за обратным прокси).
|
||||
# Если не задан, ссылки строятся из host:port — но при host 0.0.0.0 (несpecificированный
|
||||
# адрес привязки) ссылка не генерируется, так как недоступна снаружи хоста.
|
||||
public_url: "https://naviwatcher.example.com"
|
||||
|
||||
navidrome:
|
||||
url: "http://localhost:4533"
|
||||
|
||||
@@ -106,7 +106,7 @@ func TestDataFlowSmoke(t *testing.T) {
|
||||
Username: "admin",
|
||||
Password: "secret",
|
||||
}
|
||||
srv := web.NewServer(srvCfg, db, "http://localhost:8080")
|
||||
srv := web.NewServer(srvCfg, db, "http://localhost:8080", 0)
|
||||
|
||||
// Unauthenticated -> 401.
|
||||
rec := httptest.NewRecorder()
|
||||
|
||||
@@ -14,7 +14,9 @@ import (
|
||||
"naviwatcher/internal/database"
|
||||
"naviwatcher/internal/musicbrainz"
|
||||
"naviwatcher/internal/navidrome"
|
||||
"naviwatcher/internal/notifier"
|
||||
"naviwatcher/internal/scanner"
|
||||
"naviwatcher/internal/web"
|
||||
)
|
||||
|
||||
// App holds all application dependencies for clean shutdown and testability.
|
||||
@@ -23,6 +25,8 @@ type App struct {
|
||||
db *database.DB
|
||||
mbClient *musicbrainz.MusicBrainzClient
|
||||
ndClient *navidrome.NavidromeClient
|
||||
web *web.Server
|
||||
sender notifier.Sender
|
||||
|
||||
// syncFn, when non-nil, replaces the real syncAndScan call in
|
||||
// startPeriodicSync so tests can observe the loop without live clients.
|
||||
@@ -94,11 +98,23 @@ func NewApp(ctx context.Context, cfg *config.Config, dbPath string) (*App, error
|
||||
return nil, fmt.Errorf("failed to initialize navidrome client: %w", err)
|
||||
}
|
||||
|
||||
// Build the Web UI dashboard server (not started until run).
|
||||
webServer := web.NewServerWithConfig(cfg, db)
|
||||
|
||||
// Build the notifier sender. A nil sender is fine when Telegram is disabled;
|
||||
// the scheduler is no-op-safe and the web UI needs no sender.
|
||||
var sender notifier.Sender
|
||||
if cfg.Telegram.Enabled {
|
||||
sender = notifier.NewTelegramSender(cfg.Telegram)
|
||||
}
|
||||
|
||||
return &App{
|
||||
cfg: cfg,
|
||||
db: db,
|
||||
mbClient: mbClient,
|
||||
ndClient: ndClient,
|
||||
web: webServer,
|
||||
sender: sender,
|
||||
}, nil
|
||||
}
|
||||
|
||||
@@ -107,10 +123,6 @@ func (a *App) Close() {
|
||||
if a.mbClient != nil {
|
||||
a.mbClient.Close()
|
||||
}
|
||||
if a.ndClient != nil {
|
||||
// NavidromeClient holds a stateless subsonic client; nothing to close
|
||||
// beyond releasing idle connections tracked by the MusicBrainz client.
|
||||
}
|
||||
if a.db != nil {
|
||||
if err := a.db.Close(); err != nil {
|
||||
log.Printf("Error closing database: %v", err)
|
||||
@@ -120,9 +132,7 @@ func (a *App) Close() {
|
||||
|
||||
func (a *App) run(ctx context.Context) error {
|
||||
// Run an immediate sync+scan so the service produces results without
|
||||
// waiting a full interval, then kick off the periodic loop goroutine.
|
||||
// Business logic added in later tasks (notifier, web server) will be
|
||||
// wired as additional goroutines below.
|
||||
// waiting a full interval.
|
||||
if err := a.doSync(ctx); err != nil {
|
||||
if ctx.Err() != nil {
|
||||
return nil
|
||||
@@ -130,6 +140,24 @@ func (a *App) run(ctx context.Context) error {
|
||||
log.Printf("Initial sync+scan failed: %v", err)
|
||||
}
|
||||
|
||||
// Start the Web UI dashboard in its own goroutine; it serves until ctx is
|
||||
// cancelled, then shuts down gracefully.
|
||||
if a.web != nil {
|
||||
go func() {
|
||||
if err := a.web.Start(ctx); err != nil {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
}
|
||||
log.Printf("Web UI server stopped with error: %v", err)
|
||||
}
|
||||
}()
|
||||
}
|
||||
|
||||
// Start the Telegram notifier scheduler. It is no-op-safe when Telegram is
|
||||
// disabled (sender nil / enabled false), so always calling it is safe.
|
||||
a.startNotifier(ctx)
|
||||
|
||||
// Kick off the periodic sync+scan loop goroutine.
|
||||
a.startPeriodicSync(ctx)
|
||||
|
||||
<-ctx.Done()
|
||||
@@ -173,21 +201,51 @@ func (a *App) syncAndScan(ctx context.Context) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// startNotifier wires the Telegram digest scheduler. The scheduler is
|
||||
// no-op-safe (returns without starting when disabled or sender is nil), so it
|
||||
// is always safe to call. The base URL for dashboard links comes from the
|
||||
// server's configured public_url.
|
||||
func (a *App) startNotifier(ctx context.Context) {
|
||||
if !a.cfg.Telegram.Enabled {
|
||||
return
|
||||
}
|
||||
schedule, err := notifier.NewCronSchedule(a.cfg.Telegram.CronSchedule)
|
||||
if err != nil {
|
||||
log.Printf("Notifier schedule invalid (%q): %v; notifier disabled", a.cfg.Telegram.CronSchedule, err)
|
||||
return
|
||||
}
|
||||
uiBaseURL := a.cfg.Server.PublicURL
|
||||
notifier.StartScheduler(ctx, true, schedule, func(ctx context.Context) error {
|
||||
_, err := notifier.NotifyOnce(ctx, a.db, a.sender, a.cfg.Telegram, uiBaseURL)
|
||||
return err
|
||||
}, nil)
|
||||
}
|
||||
|
||||
// startPeriodicSync runs syncAndScan on a ticker at cfg.Sync.Interval. It
|
||||
// blocks until ctx is cancelled, then returns cleanly. Each tick runs in its
|
||||
// own goroutine so a slow sync does not block the ticker; a fresh interval is
|
||||
// still scheduled regardless.
|
||||
// blocks until ctx is cancelled, then returns cleanly. Each tick spawns a
|
||||
// goroutine so a slow sync does not block the ticker, but a new sync is
|
||||
// skipped while the previous one is still running (guarded by a done channel)
|
||||
// so syncs never overlap and contend for the shared DB and rate-limited
|
||||
// MusicBrainz client.
|
||||
func (a *App) startPeriodicSync(ctx context.Context) {
|
||||
ticker := time.NewTicker(a.cfg.Sync.Interval)
|
||||
defer ticker.Stop()
|
||||
|
||||
// free is a sentinel channel: nil means a sync is currently in flight.
|
||||
var free = make(chan struct{}, 1)
|
||||
free <- struct{}{}
|
||||
|
||||
for {
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
log.Println("Periodic sync stopped.")
|
||||
return
|
||||
case <-ticker.C:
|
||||
select {
|
||||
case <-free:
|
||||
// Slot was free; start a sync and release the slot when done.
|
||||
go func() {
|
||||
defer func() { free <- struct{}{} }()
|
||||
if err := a.doSync(ctx); err != nil {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
@@ -195,6 +253,10 @@ func (a *App) startPeriodicSync(ctx context.Context) {
|
||||
log.Printf("Periodic sync+scan failed: %v", err)
|
||||
}
|
||||
}()
|
||||
default:
|
||||
// Previous sync still running; skip this tick.
|
||||
log.Println("Skipping periodic sync: previous sync still in progress.")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4,6 +4,11 @@ server:
|
||||
# Basic Auth for Web UI access
|
||||
username: "admin"
|
||||
password: "CHANGE_ME"
|
||||
# Externally-reachable base URL for links in Telegram digests (e.g. behind a
|
||||
# reverse proxy). If omitted, links are derived from host:port — but when host
|
||||
# is 0.0.0.0 (the unspecified bind address) no link is emitted, since it is
|
||||
# not reachable from outside the host.
|
||||
public_url: "https://naviwatcher.example.com"
|
||||
|
||||
navidrome:
|
||||
url: "http://localhost:4533"
|
||||
|
||||
@@ -161,3 +161,30 @@ name collisions.)
|
||||
ignore/restore actions persist.
|
||||
- **External**: ensure `config.yaml.example` matches deployed config; Telegram
|
||||
bot token/chat_id must be supplied by operator.
|
||||
|
||||
## Follow-up fixes (post code review)
|
||||
After the plan's Tasks 1-10 merged, a code review surfaced and fixed:
|
||||
- **Duplicate notifications**: `SyncArtistDiscography` previously deleted
|
||||
`notifications_sent` for the whole artist on every cache-miss re-sync, which
|
||||
wiped "already notified" tracking and re-sent digests. Now only notifications
|
||||
for releases that disappear are pruned, and surviving releases keep their sent
|
||||
markers. Re-sync is FK-safe (uses `INSERT OR REPLACE` + `rgid NOT IN (…)`).
|
||||
- **Empty-discography caching**: artists with zero MusicBrainz release groups
|
||||
were never cached (a `0`-row result was treated as a cache miss), re-fetching
|
||||
every cycle. Added `artist_settings.last_synced` (migration `009`) as the cache
|
||||
freshness signal so empty discographies honor the TTL.
|
||||
- **`main.run` wiring**: the Web UI server and Telegram notifier scheduler are
|
||||
now constructed in `NewApp` and started as goroutines in `run()` (previously
|
||||
only the sync loop ran).
|
||||
- **Overlap guard**: `startPeriodicSync` now skips a tick while a previous sync
|
||||
is still in flight (buffered `done` channel) so syncs never overlap.
|
||||
- **`server.public_url` config**: added so Telegram digest links use an
|
||||
externally-reachable origin instead of the bind `host:port` (which defaults to
|
||||
`0.0.0.0`). `NewServerWithConfig` falls back to host:port only for a real host.
|
||||
- **Web handlers**: artist detail page now uses `scanner.ScanArtist` (per-artist)
|
||||
instead of a full `ScanAll`; removed the always-false `releaseIgnored` lookup
|
||||
and dead `endsWith` helper; the configured fuzzy threshold is now threaded
|
||||
through `Server`.
|
||||
- **DB connection pooling**: `:memory:` databases now use `SetMaxOpenConns(1)`
|
||||
so migrations and queries share one in-memory store (prevents "missing column"
|
||||
errors under the connection pool).
|
||||
|
||||
@@ -24,6 +24,7 @@ type ServerConfig struct {
|
||||
Port int `yaml:"port"`
|
||||
Username string `yaml:"username"`
|
||||
Password string `yaml:"password"`
|
||||
PublicURL string `yaml:"public_url"`
|
||||
}
|
||||
|
||||
// NavidromeConfig holds Subsonic API connection details.
|
||||
|
||||
@@ -4,19 +4,27 @@ import (
|
||||
"database/sql"
|
||||
"errors"
|
||||
"fmt"
|
||||
"time"
|
||||
)
|
||||
|
||||
// DBer is the minimal query interface satisfied by both *sql.DB and *sql.Tx,
|
||||
// so callers can run statements inside or outside a transaction.
|
||||
type DBer interface {
|
||||
Exec(query string, args ...interface{}) (sql.Result, error)
|
||||
}
|
||||
|
||||
// GetArtistSettings retrieves an artist_settings row by ID.
|
||||
// Returns sql.ErrNoRows if the artist is not found.
|
||||
func GetArtistSettings(db *DB, id string) (*ArtistSettings, error) {
|
||||
var (
|
||||
s ArtistSettings
|
||||
mbid sql.NullString
|
||||
lastSynced sql.NullTime
|
||||
)
|
||||
err := db.Conn().QueryRow(
|
||||
"SELECT id, name, mbid, ignore_singles, ignore_compilations, monitored FROM artist_settings WHERE id = ?",
|
||||
"SELECT id, name, mbid, ignore_singles, ignore_compilations, monitored, last_synced FROM artist_settings WHERE id = ?",
|
||||
id,
|
||||
).Scan(&s.ID, &s.Name, &mbid, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored)
|
||||
).Scan(&s.ID, &s.Name, &mbid, &s.IgnoreSingles, &s.IgnoreCompilations, &s.Monitored, &lastSynced)
|
||||
if err != nil {
|
||||
if errors.Is(err, sql.ErrNoRows) {
|
||||
return nil, ErrArtistNotFound
|
||||
@@ -24,9 +32,26 @@ func GetArtistSettings(db *DB, id string) (*ArtistSettings, error) {
|
||||
return nil, err
|
||||
}
|
||||
s.MBID = mbid.String
|
||||
if lastSynced.Valid {
|
||||
s.LastSynced = lastSynced.Time
|
||||
}
|
||||
return &s, nil
|
||||
}
|
||||
|
||||
// TouchArtistSynced records that the artist was synced at the given time. It
|
||||
// is used by the MusicBrainz pipeline to mark a successful sync (even one that
|
||||
// found zero release groups) so the cache TTL is honoured.
|
||||
func TouchArtistSynced(db DBer, artistID string, syncedAt time.Time) error {
|
||||
_, err := db.Exec(
|
||||
"UPDATE artist_settings SET last_synced = ? WHERE id = ?",
|
||||
FormatCachedAt(syncedAt), artistID,
|
||||
)
|
||||
if err != nil {
|
||||
return fmt.Errorf("touch artist synced: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// SaveArtistSettings inserts or replaces an artist_settings row.
|
||||
func SaveArtistSettings(db *DB, settings *ArtistSettings) error {
|
||||
_, err := db.Conn().Exec(
|
||||
|
||||
@@ -25,10 +25,21 @@ func New(dbPath string) (*DB, error) {
|
||||
// EVERY connection in the pool. A one-off "PRAGMA foreign_keys=ON" executed
|
||||
// on the pooled *sql.DB only applies to the first connection and is lost on
|
||||
// connections opened later by the pool, silently disabling the safety net.
|
||||
// A plain ":memory:" database is private to the connection that opened it,
|
||||
// so a pool of N connections would give N separate empty databases and
|
||||
// migrations would appear missing on some. Limiting the pool to a single
|
||||
// connection keeps one in-memory database per New() call, which is correct
|
||||
// for both tests (isolated) and the single-process production service.
|
||||
conn, err := sql.Open("sqlite3", dbPath+"?_foreign_keys=on")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("open database: %w", err)
|
||||
}
|
||||
if dbPath == ":memory:" {
|
||||
conn.SetMaxOpenConns(1)
|
||||
}
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("open database: %w", err)
|
||||
}
|
||||
|
||||
// Enable WAL mode for better concurrent read performance.
|
||||
if _, err := conn.Exec("PRAGMA journal_mode=WAL"); err != nil {
|
||||
@@ -135,6 +146,10 @@ func (db *DB) migrate() error {
|
||||
name: "008_add_mbid_to_artist_settings",
|
||||
sql: `ALTER TABLE artist_settings ADD COLUMN mbid TEXT;`,
|
||||
},
|
||||
{
|
||||
name: "009_add_last_synced_to_artist_settings",
|
||||
sql: `ALTER TABLE artist_settings ADD COLUMN last_synced DATETIME;`,
|
||||
},
|
||||
}
|
||||
|
||||
for _, m := range migrations {
|
||||
@@ -187,6 +202,7 @@ type ArtistSettings struct {
|
||||
IgnoreSingles bool `json:"ignore_singles"`
|
||||
IgnoreCompilations bool `json:"ignore_compilations"`
|
||||
Monitored bool `json:"monitored"`
|
||||
LastSynced time.Time `json:"last_synced"`
|
||||
}
|
||||
|
||||
// LocalAlbum represents a row in the local_albums table.
|
||||
|
||||
@@ -205,11 +205,11 @@ func TestMigrationTracking(t *testing.T) {
|
||||
t.Fatalf("query migrations count: %v", err)
|
||||
}
|
||||
|
||||
// We have 8 recorded migrations: artist_settings, external_releases,
|
||||
// We have 9 recorded migrations: artist_settings, external_releases,
|
||||
// local_albums, notifications_sent, cached_at column, secondary_types
|
||||
// column, the external_releases.artist_id index, and the artist_settings
|
||||
// mbid column.
|
||||
if count != 8 {
|
||||
t.Errorf("expected 8 applied migrations, got %d", count)
|
||||
// column, the external_releases.artist_id index, the artist_settings mbid
|
||||
// column, and the artist_settings last_synced column.
|
||||
if count != 9 {
|
||||
t.Errorf("expected 9 applied migrations, got %d", count)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -166,6 +166,42 @@ func SetReleaseIgnored(db *DB, rgid string, ignored bool) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// ArtistCacheFresh reports whether the artist was synced within the TTL. A hit
|
||||
// requires a fresh external_releases row (covers pre-seeded/non-empty caches)
|
||||
// OR a fresh artist_settings.last_synced (covers empty discographies, which
|
||||
// store no external_releases rows but are still marked as synced). Either
|
||||
// signal means we should not re-fetch from MusicBrainz.
|
||||
func ArtistCacheFresh(db *DB, artistID string, ttl time.Duration) (bool, error) {
|
||||
if ttl <= 0 {
|
||||
return false, nil
|
||||
}
|
||||
cutoff := time.Now().UTC().Add(-ttl).Format(utcLayout)
|
||||
var dummy int
|
||||
err := db.Conn().QueryRow(
|
||||
"SELECT 1 FROM external_releases WHERE artist_id = ? AND cached_at >= ? LIMIT 1",
|
||||
artistID, cutoff,
|
||||
).Scan(&dummy)
|
||||
if err == nil {
|
||||
return true, nil
|
||||
}
|
||||
if err != sql.ErrNoRows {
|
||||
return false, fmt.Errorf("check artist cache freshness (releases): %w", err)
|
||||
}
|
||||
|
||||
// Fall back to the per-artist last_synced marker (set even on empty syncs).
|
||||
err = db.Conn().QueryRow(
|
||||
"SELECT 1 FROM artist_settings WHERE id = ? AND last_synced >= ? LIMIT 1",
|
||||
artistID, cutoff,
|
||||
).Scan(&dummy)
|
||||
if err == nil {
|
||||
return true, nil
|
||||
}
|
||||
if err == sql.ErrNoRows {
|
||||
return false, nil
|
||||
}
|
||||
return false, fmt.Errorf("check artist cache freshness (settings): %w", err)
|
||||
}
|
||||
|
||||
// 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) {
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"fmt"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"naviwatcher/internal/database"
|
||||
@@ -38,18 +39,25 @@ func SyncArtistDiscography(
|
||||
return nil, fmt.Errorf("sync artist discography: %w", err)
|
||||
}
|
||||
|
||||
// Step 1: Check cache.
|
||||
// Step 1: Check cache freshness. A genuine hit means the artist was synced
|
||||
// within the TTL — even when it has zero release groups. We must not gate on
|
||||
// row count, or artists with an empty MusicBrainz discography would be
|
||||
// re-fetched on every sync (defeating the TTL and wasting the 1 req/s budget).
|
||||
cachedReleases, err := database.GetExternalReleasesByArtistWithCache(db, artistID, ttl)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: cache check failed: %w", err)
|
||||
}
|
||||
fresh, err := database.ArtistCacheFresh(db, artistID, ttl)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: cache freshness check failed: %w", err)
|
||||
}
|
||||
|
||||
// Step 2: If we have cached data, return it. Re-apply the per-artist type
|
||||
// toggles even on a cache hit so user changes to ignore_singles /
|
||||
// ignore_compilations take effect without waiting for cache expiry.
|
||||
// (Status/type inclusion was already applied when the rows were first
|
||||
// Step 2: If we have a fresh cache, return the cached data. Re-apply the
|
||||
// per-artist type toggles even on a cache hit so user changes to
|
||||
// ignore_singles / ignore_compilations take effect without waiting for cache
|
||||
// expiry. (Status/type inclusion was already applied when the rows were first
|
||||
// synced and stored, so only the toggles can change.)
|
||||
if len(cachedReleases) > 0 {
|
||||
if fresh {
|
||||
if err := ctx.Err(); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: %w", err)
|
||||
}
|
||||
@@ -108,18 +116,45 @@ 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 = ?)",
|
||||
artistID,
|
||||
); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: delete old notifications: %w", err)
|
||||
// Build the set of RGIDs present in this sync so we can drop only the rows
|
||||
// that disappeared, leaving the rest (and their notification markers) intact.
|
||||
synced := make([]any, 0, len(filtered))
|
||||
for _, rg := range filtered {
|
||||
synced = append(synced, rg.ID)
|
||||
}
|
||||
|
||||
// Drop notification markers for releases that are gone. This runs before the
|
||||
// external_releases delete so the FK on notifications_sent.rgid stays valid
|
||||
// (we only ever delete from notifications_sent here).
|
||||
if len(synced) > 0 {
|
||||
placeholders := strings.Repeat("?,", len(synced))
|
||||
placeholders = placeholders[:len(placeholders)-1]
|
||||
query := fmt.Sprintf(
|
||||
"DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ? AND rgid NOT IN (%s))",
|
||||
placeholders,
|
||||
)
|
||||
args := append([]any{artistID}, synced...)
|
||||
if _, err := tx.Exec(query, args...); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: prune stale notifications: %w", err)
|
||||
}
|
||||
// Remove external_release rows that are no longer part of the discography.
|
||||
delQuery := fmt.Sprintf(
|
||||
"DELETE FROM external_releases WHERE artist_id = ? AND rgid NOT IN (%s)",
|
||||
placeholders,
|
||||
)
|
||||
if _, err := tx.Exec(delQuery, args...); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: delete stale releases: %w", err)
|
||||
}
|
||||
} else {
|
||||
// No releases this sync: the artist may have an empty discography. Drop
|
||||
// everything we previously cached for them.
|
||||
if _, err := tx.Exec("DELETE FROM notifications_sent WHERE rgid IN (SELECT rgid FROM external_releases WHERE artist_id = ?)", artistID); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: delete notifications: %w", err)
|
||||
}
|
||||
if _, err := tx.Exec("DELETE FROM external_releases WHERE artist_id = ?", artistID); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: delete old releases: %w", err)
|
||||
}
|
||||
}
|
||||
|
||||
var releases []database.ExternalRelease
|
||||
for _, rg := range filtered {
|
||||
@@ -136,7 +171,7 @@ func SyncArtistDiscography(
|
||||
}
|
||||
|
||||
if _, err := tx.Exec(
|
||||
"INSERT INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types) VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
|
||||
"INSERT OR REPLACE INTO external_releases (rgid, artist_id, title, type, release_date, is_ignored, cached_at, secondary_types) VALUES (?, ?, ?, ?, ?, ?, ?, ?)",
|
||||
ext.RGID, ext.ArtistID, ext.Title, ext.Type, ext.ReleaseDate, ext.IsIgnored, database.FormatCachedAt(ext.CachedAt), database.JoinSecondaryTypes(ext.SecondaryTypes),
|
||||
); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: insert release %s: %w", rg.ID, err)
|
||||
@@ -145,6 +180,13 @@ func SyncArtistDiscography(
|
||||
releases = append(releases, *ext)
|
||||
}
|
||||
|
||||
// Mark the artist as synced (even when it has zero release groups) so the
|
||||
// cache TTL honours empty discographies and they are not re-fetched every
|
||||
// cycle.
|
||||
if err := database.TouchArtistSynced(tx, artistID, now); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: touch last_synced: %w", err)
|
||||
}
|
||||
|
||||
if err := tx.Commit(); err != nil {
|
||||
return nil, fmt.Errorf("sync artist discography: commit transaction: %w", err)
|
||||
}
|
||||
|
||||
@@ -370,11 +370,16 @@ func TestSyncArtistDiscography_IdempotentResync(t *testing.T) {
|
||||
t.Fatalf("expected 1 server call after first sync, got %d", callCount)
|
||||
}
|
||||
|
||||
// Force cache expiry by setting cached_at to the past.
|
||||
// Force cache expiry by setting cached_at (on external_releases) and
|
||||
// last_synced (on artist_settings) to the past.
|
||||
_, 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"), artistID)
|
||||
if err != nil {
|
||||
t.Fatalf("expire cache: %v", err)
|
||||
t.Fatalf("expire cache (releases): %v", err)
|
||||
}
|
||||
if _, err = db.Conn().Exec("UPDATE artist_settings SET last_synced = ? WHERE id = ?",
|
||||
time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil {
|
||||
t.Fatalf("expire cache (settings): %v", err)
|
||||
}
|
||||
|
||||
// Second sync should re-fetch from API (cache expired).
|
||||
@@ -438,6 +443,28 @@ func TestSyncArtistDiscography_EmptyResponse(t *testing.T) {
|
||||
if len(stored) != 0 {
|
||||
t.Errorf("expected 0 stored releases, got %d", len(stored))
|
||||
}
|
||||
|
||||
// A second sync within the TTL must be a cache hit: an empty discography is
|
||||
// now cached via artist_settings.last_synced, so the MusicBrainz API must
|
||||
// not be re-queried (and still returns 0 releases).
|
||||
serverHits := 0
|
||||
server2 := newTestMBServer(func(w http.ResponseWriter, r *http.Request) {
|
||||
serverHits++
|
||||
w.Header().Set("Content-Type", "application/xml")
|
||||
w.Write([]byte(mbReleaseGroupListResponse("", 0)))
|
||||
})
|
||||
defer server2.Close()
|
||||
|
||||
releases2, err := SyncArtistDiscography(ctx, newTestClient(server2.URL), db, artistID, artistMBID, ttl)
|
||||
if err != nil {
|
||||
t.Fatalf("second SyncArtistDiscography() error: %v", err)
|
||||
}
|
||||
if len(releases2) != 0 {
|
||||
t.Errorf("expected 0 releases on cached empty sync, got %d", len(releases2))
|
||||
}
|
||||
if serverHits != 0 {
|
||||
t.Errorf("expected empty discography to be cached (0 API calls), got %d", serverHits)
|
||||
}
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
@@ -714,11 +741,16 @@ func TestSyncArtistDiscography_CleansStaleReleases(t *testing.T) {
|
||||
t.Fatalf("expected 3 releases after first sync, got %d", len(releases1))
|
||||
}
|
||||
|
||||
// Force cache expiry by setting cached_at to the past.
|
||||
// Force cache expiry by setting cached_at (on external_releases) and
|
||||
// last_synced (on artist_settings) to the past.
|
||||
_, 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"), artistID)
|
||||
if err != nil {
|
||||
t.Fatalf("expire cache: %v", err)
|
||||
t.Fatalf("expire cache (releases): %v", err)
|
||||
}
|
||||
if _, err = db.Conn().Exec("UPDATE artist_settings SET last_synced = ? WHERE id = ?",
|
||||
time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil {
|
||||
t.Fatalf("expire cache (settings): %v", err)
|
||||
}
|
||||
|
||||
// Second sync should re-fetch from API (cache expired).
|
||||
@@ -876,10 +908,14 @@ func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) {
|
||||
artistID := "nav-fk-test"
|
||||
artistName := "FK Artist"
|
||||
|
||||
// First response includes two release groups; the second sync drops one
|
||||
// ("rg-2") so we can verify its notification is pruned while the surviving
|
||||
// release's notification ("rg-1") is preserved.
|
||||
server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) {
|
||||
resp := mbReleaseGroupListResponse(
|
||||
mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01"),
|
||||
1,
|
||||
mbReleaseGroupXML("rg-1", "Album", "Album", "Official", artistMBID, artistName, "2024-01-01")+
|
||||
mbReleaseGroupXML("rg-2", "Album", "Album", "Official", artistMBID, artistName, "2023-01-01"),
|
||||
2,
|
||||
)
|
||||
w.Write([]byte(resp))
|
||||
})
|
||||
@@ -893,27 +929,37 @@ func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) {
|
||||
ctx := context.Background()
|
||||
|
||||
// First sync.
|
||||
_, err := SyncArtistDiscography(ctx, client, db, artistID, artistMBID, 24*time.Hour)
|
||||
if err != nil {
|
||||
if _, err := SyncArtistDiscography(ctx, client, db, artistID, artistMBID, 24*time.Hour); 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 {
|
||||
// Mark both releases as already notified.
|
||||
for _, rgid := range []string{"rg-1", "rg-2"} {
|
||||
if _, err := db.Conn().Exec("INSERT INTO notifications_sent (rgid) VALUES (?)", rgid); 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"), artistID)
|
||||
if err != nil {
|
||||
t.Fatalf("expire cache: %v", err)
|
||||
}
|
||||
|
||||
// Second sync should succeed without FK violation.
|
||||
// Force cache expiry on the first sync so the second sync re-fetches.
|
||||
if _, 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"), artistID); err != nil {
|
||||
t.Fatalf("expire cache (releases): %v", err)
|
||||
}
|
||||
if _, err := db.Conn().Exec("UPDATE artist_settings SET last_synced = ? WHERE id = ?",
|
||||
time.Now().Add(-48*time.Hour).Format("2006-01-02 15:04:05"), artistID); err != nil {
|
||||
t.Fatalf("expire cache (settings): %v", err)
|
||||
}
|
||||
|
||||
// Second sync returns only rg-1 (drop rg-2 from the server response) and
|
||||
// must succeed without an FK violation.
|
||||
server.Config.Handler = http.HandlerFunc(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))
|
||||
})
|
||||
|
||||
releases, err := SyncArtistDiscography(ctx, client, db, artistID, artistMBID, 24*time.Hour)
|
||||
if err != nil {
|
||||
t.Fatalf("second SyncArtistDiscography() error (FK violation?): %v", err)
|
||||
@@ -922,13 +968,20 @@ func TestSyncArtistDiscography_ResyncWithNotifications(t *testing.T) {
|
||||
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)
|
||||
// The surviving release's notification must be preserved (no duplicate
|
||||
// digest on the next notify run). The dropped release's notification must
|
||||
// be pruned.
|
||||
var count1, count2 int
|
||||
if err := db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rg-1").Scan(&count1); err != nil {
|
||||
t.Fatalf("count rg-1 notifications: %v", err)
|
||||
}
|
||||
if count != 0 {
|
||||
t.Errorf("expected 0 notifications after resync, got %d", count)
|
||||
if err := db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rg-2").Scan(&count2); err != nil {
|
||||
t.Fatalf("count rg-2 notifications: %v", err)
|
||||
}
|
||||
if count1 != 1 {
|
||||
t.Errorf("expected surviving release rg-1 notification preserved (1), got %d", count1)
|
||||
}
|
||||
if count2 != 0 {
|
||||
t.Errorf("expected dropped release rg-2 notification pruned (0), got %d", count2)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -32,7 +32,7 @@ func (s *Server) handleDashboard(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
|
||||
threshold := s.defaultThreshold()
|
||||
threshold := s.threshold
|
||||
data, err := s.buildDashboardData(r.Context(), threshold)
|
||||
if err != nil {
|
||||
http.Error(w, fmt.Sprintf("failed to build dashboard: %v", err), http.StatusInternalServerError)
|
||||
@@ -120,11 +120,14 @@ func (s *Server) buildArtistData(ctx context.Context, id string) (*ArtistData, e
|
||||
return nil, fmt.Errorf("load local albums: %w", err)
|
||||
}
|
||||
|
||||
// Compute the missing releases for this artist.
|
||||
threshold := s.defaultThreshold()
|
||||
missing, err := scanner.ScanAll(ctx, s.db, threshold)
|
||||
// Compute the missing releases for this single artist (ScanArtist scopes the
|
||||
// query to the artist instead of scanning every monitored artist). ScanAll
|
||||
// excludes ignored releases, so every missing release surfaced here is, by
|
||||
// definition, not ignored.
|
||||
threshold := s.threshold
|
||||
missing, err := scanner.ScanArtist(ctx, s.db, id, threshold)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("scan: %w", err)
|
||||
return nil, fmt.Errorf("scan artist: %w", err)
|
||||
}
|
||||
|
||||
data := &ArtistData{
|
||||
@@ -138,38 +141,18 @@ func (s *Server) buildArtistData(ctx context.Context, id string) (*ArtistData, e
|
||||
data.LocalAlbums = append(data.LocalAlbums, LocalAlbumView{Title: a.Title})
|
||||
}
|
||||
for _, m := range missing {
|
||||
if m.ArtistID != id {
|
||||
continue
|
||||
}
|
||||
ignored, igErr := s.releaseIgnored(id, m.RGID)
|
||||
if igErr != nil {
|
||||
return nil, igErr
|
||||
}
|
||||
data.Missing = append(data.Missing, MissingReleaseView{
|
||||
ArtistID: id,
|
||||
ArtistID: m.ArtistID,
|
||||
RGID: m.RGID,
|
||||
Title: m.Title,
|
||||
Type: m.Type,
|
||||
ReleaseDate: m.ReleaseDate,
|
||||
Ignored: ignored,
|
||||
Ignored: false,
|
||||
})
|
||||
}
|
||||
return data, nil
|
||||
}
|
||||
|
||||
// releaseIgnored reports whether the external release with the given RGID is
|
||||
// flagged ignored.
|
||||
func (s *Server) releaseIgnored(artistID, rgid string) (bool, error) {
|
||||
// ScanAll already excludes ignored releases, so a missing release shown here
|
||||
// is, by definition, not ignored. We still surface the persisted flag so the
|
||||
// UI can reflect a release that was ignored and later re-evaluated.
|
||||
rel, err := database.GetExternalRelease(s.db, rgid)
|
||||
if err != nil {
|
||||
return false, fmt.Errorf("get external release %s: %w", rgid, err)
|
||||
}
|
||||
return rel.IsIgnored, nil
|
||||
}
|
||||
|
||||
// ArchiveData is the view model for the ignored-releases archive page.
|
||||
type ArchiveData struct {
|
||||
Releases []MissingReleaseView
|
||||
@@ -208,20 +191,20 @@ func (s *Server) handleArchive(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
}
|
||||
|
||||
// defaultThreshold returns the fuzzy threshold to use when scanning for the
|
||||
// dashboard. It is currently fixed at the engine default; later wiring can
|
||||
// thread the configured threshold through the Server if desired.
|
||||
func (s *Server) defaultThreshold() float64 {
|
||||
return 0 // 0 → scanner.DefaultThreshold
|
||||
}
|
||||
|
||||
// NewServerWithConfig is a convenience constructor that accepts the full
|
||||
// *config.Config (mirroring how the app constructs other components). It
|
||||
// forwards the server sub-config and derives uiBaseURL from host/port.
|
||||
// forwards the server sub-config and derives uiBaseURL from the configured
|
||||
// public_url, falling back to a best-effort host:port.
|
||||
func NewServerWithConfig(cfg *config.Config, db *database.DB) *Server {
|
||||
// Build a best-effort external base URL from the server config.
|
||||
base := fmt.Sprintf("http://%s:%d", cfg.Server.Host, cfg.Server.Port)
|
||||
return NewServer(&cfg.Server, db, base)
|
||||
// Prefer an explicit, externally-reachable public_url (e.g. behind a
|
||||
// reverse proxy). Fall back to host:port — but if the bind host is the
|
||||
// unspecified "0.0.0.0", it is not reachable from outside the host, so
|
||||
// omit the link rather than advertise an unusable address.
|
||||
base := cfg.Server.PublicURL
|
||||
if base == "" && cfg.Server.Host != "0.0.0.0" && cfg.Server.Host != "" {
|
||||
base = fmt.Sprintf("http://%s:%d", cfg.Server.Host, cfg.Server.Port)
|
||||
}
|
||||
return NewServer(&cfg.Server, db, base, cfg.Scanner.FuzzyThreshold)
|
||||
}
|
||||
|
||||
// ignoreOrRestore handles the POST /artist/{id}/ignore and .../restore routes.
|
||||
@@ -290,8 +273,3 @@ func (s *Server) toggleIgnoreSingles(w http.ResponseWriter, r *http.Request) {
|
||||
http.Redirect(w, r, "/artist/"+id, http.StatusSeeOther)
|
||||
}
|
||||
|
||||
// endsWith reports whether s ends with suffix.
|
||||
func endsWith(s, suffix string) bool {
|
||||
return len(s) >= len(suffix) && s[len(s)-len(suffix):] == suffix
|
||||
}
|
||||
|
||||
|
||||
@@ -27,16 +27,22 @@ type Server struct {
|
||||
// uiBaseURL is the externally reachable base URL of the dashboard (scheme +
|
||||
// host), used to build links in notifications and elsewhere. Optional.
|
||||
uiBaseURL string
|
||||
|
||||
// threshold is the fuzzy-similarity cutoff used when scanning for missing
|
||||
// releases; 0 means use the scanner default.
|
||||
threshold float64
|
||||
}
|
||||
|
||||
// NewServer constructs a dashboard Server bound to the given DB and server
|
||||
// config. uiBaseURL is the externally reachable origin (e.g.
|
||||
// "http://localhost:8080") used when rendering absolute links; pass "" to omit.
|
||||
func NewServer(cfg *config.ServerConfig, db *database.DB, uiBaseURL string) *Server {
|
||||
// threshold is the fuzzy-similarity cutoff (0 → scanner default).
|
||||
func NewServer(cfg *config.ServerConfig, db *database.DB, uiBaseURL string, threshold float64) *Server {
|
||||
s := &Server{
|
||||
cfg: cfg,
|
||||
db: db,
|
||||
uiBaseURL: strings.TrimRight(uiBaseURL, "/"),
|
||||
threshold: threshold,
|
||||
}
|
||||
mux := http.NewServeMux()
|
||||
mux.HandleFunc("/", s.handleDashboard)
|
||||
|
||||
@@ -51,7 +51,7 @@ func newServer(t *testing.T, user, pass string) (*Server, *database.DB) {
|
||||
t.Fatalf("open db: %v", err)
|
||||
}
|
||||
cfg := &config.ServerConfig{Host: "0.0.0.0", Port: 8080, Username: user, Password: pass}
|
||||
s := NewServer(cfg, db, "http://ui.example")
|
||||
s := NewServer(cfg, db, "http://ui.example", 0)
|
||||
return s, db
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user