fix: address second code review findings

- Add CacheTTL default (24h) in config applyDefaults — without this,
  omitting cache_ttl from config silently defeats the entire caching
  mechanism (TTL=0 means cached data is never served)
- Fix pagination to use total Count instead of checking if last page
  was short — avoids wasting a rate-limit token when total count is
  an exact multiple of 100
- Preserve user-set IsIgnored flags across re-syncs — previously,
  DELETE+INSERT in the sync transaction reset all ignore flags to
  false, losing user preferences on every cache-expiry re-sync
- Check context cancellation on cache-hit code path — previously,
  ctx.Err() was not checked between cache check and returning cached
  data, violating the cancellation contract
This commit is contained in:
2026-05-26 14:55:50 +03:00
parent a5911c257c
commit 2baf586607
4 changed files with 31 additions and 4 deletions

View File

@@ -56,8 +56,8 @@ func (c *MusicBrainzClient) GetArtistReleaseGroups(ctx context.Context, artistMB
allGroups = append(allGroups, parsed.ReleaseGroups...)
// If we got fewer results than the limit, we've reached the end
if len(parsed.ReleaseGroups) < limit {
// If we've fetched all results, we've reached the end.
if offset+len(parsed.ReleaseGroups) >= parsed.Count {
break
}
offset += limit

View File

@@ -242,7 +242,7 @@ func TestGetArtistReleaseGroups_Pagination(t *testing.T) {
// First page: return 100 results (full page, matching limit) to trigger pagination
xml := `<?xml version="1.0" encoding="UTF-8"?>
<metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">
<release-group-list count="150">`
<release-group-list count="101">`
for i := 0; i < 100; i++ {
xml += `
<release-group id="rg-page1-` + string(rune('0'+i%10)) + `" type="Album">
@@ -263,7 +263,7 @@ func TestGetArtistReleaseGroups_Pagination(t *testing.T) {
// Second page: return only 1 result (< limit, signaling last page)
w.Write([]byte(`<?xml version="1.0" encoding="UTF-8"?>
<metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">
<release-group-list count="150">
<release-group-list count="101">
<release-group id="rg-page2" type="Album">
<title>Page 2 Album</title>
<first-release-date>2021-01-01</first-release-date>

View File

@@ -39,6 +39,9 @@ func SyncArtistDiscography(
// Step 2: If we have cached data, return it.
if cachedCount > 0 {
if err := ctx.Err(); err != nil {
return nil, fmt.Errorf("sync artist discography: %w", err)
}
return database.GetExternalReleasesByArtistWithCache(db, artistMBID, ttl)
}
@@ -59,6 +62,23 @@ func SyncArtistDiscography(
}
defer tx.Rollback()
// Read existing ignore states before deleting to preserve user-set flags.
ignoredMap := map[string]bool{}
rows, err := tx.Query("SELECT rgid, is_ignored FROM external_releases WHERE artist_id = ?", artistMBID)
if err != nil {
return nil, fmt.Errorf("sync artist discography: query existing releases: %w", err)
}
for rows.Next() {
var rgid string
var ignored bool
if err := rows.Scan(&rgid, &ignored); err != nil {
rows.Close()
return nil, fmt.Errorf("sync artist discography: scan existing release: %w", err)
}
ignoredMap[rgid] = ignored
}
rows.Close()
// Delete old entries for this artist to avoid stale records.
if _, err := tx.Exec("DELETE FROM external_releases WHERE artist_id = ?", artistMBID); err != nil {
return nil, fmt.Errorf("sync artist discography: delete old releases: %w", err)
@@ -73,6 +93,10 @@ func SyncArtistDiscography(
ext := rg.ToExternalRelease()
ext.CachedAt = now
// Preserve user-set ignore flag from previous sync.
if ignored, ok := ignoredMap[ext.RGID]; ok {
ext.IsIgnored = ignored
}
cachedAtStr := ext.CachedAt.Format("2006-01-02 15:04:05")
if _, err := tx.Exec(