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:45:08 +03:00
parent beef81d598
commit a7803615cd
7 changed files with 185 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
internal/musicbrainz/api.go
View File

@@ -8,19 +8,12 @@ import (
"naviwatcher/internal/database"
)
// excludedStatuses contains release-group statuses that should be filtered out.
var excludedStatuses = map[string]bool{
"Bootleg": true,
"Promotion": true,
"Pseudo-Release": true,
}
// includedTypes contains release-group types that should be included.
// includedTypes contains release-group primary types that should be included
// when no more specific type classification applies.
var includedTypes = map[string]bool{
"Album": true,
"Single": true,
"EP": true,
"Compilation": true,
"Album": true,
"Single": true,
"EP": true,
}
// GetArtistReleaseGroups fetches all release groups for a given artist from MusicBrainz.
@@ -77,23 +70,23 @@ type FilterOptions struct {
IgnoreCompilations bool
}
// FilterReleaseGroups applies status and type filtering to a list of release groups.
// It excludes Bootleg, Promotion, and Pseudo-Release statuses.
// It includes only Album, Single, EP, and Compilation types, unless the type
// is disabled via FilterOptions.
// FilterReleaseGroups applies type filtering to a list of release groups.
// It includes only Album/Single/EP primary types, or release groups whose
// secondary type list contains Single/EP/Compilation (e.g. an "Album" that is
// also a "Compilation"). The IgnoreSingles / IgnoreCompilations toggles drop
// release groups classified as such via either primary or secondary type.
//
// Release groups carry no status in ws/2, so there is no status filtering.
func FilterReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGroup {
var filtered []ReleaseGroup
for _, rg := range groups {
if IsStatusExcluded(rg.Status) {
if !IsTypeIncluded(rg.Type) && !hasSecondaryType(rg, "Single", "EP", "Compilation") {
continue
}
if !IsTypeIncluded(rg.Type) {
if opts.IgnoreSingles && (rg.Type == "Single" || hasSecondaryType(rg, "Single")) {
continue
}
if opts.IgnoreSingles && rg.Type == "Single" {
continue
}
if opts.IgnoreCompilations && rg.Type == "Compilation" {
if opts.IgnoreCompilations && (rg.Type == "Compilation" || hasSecondaryType(rg, "Compilation")) {
continue
}
filtered = append(filtered, rg)
@@ -101,12 +94,21 @@ func FilterReleaseGroups(groups []ReleaseGroup, opts FilterOptions) []ReleaseGro
return filtered
}
// IsStatusExcluded returns true if the given status should be excluded.
func IsStatusExcluded(status string) bool {
return excludedStatuses[status]
// hasSecondaryType reports whether any of the release group's secondary types
// matches one of the provided values.
func hasSecondaryType(rg ReleaseGroup, wanted ...string) bool {
for _, s := range rg.SecondaryTypes {
for _, w := range wanted {
if s == w {
return true
}
}
}
return false
}
// IsTypeIncluded returns true if the given type is in the base included set.
// IsTypeIncluded returns true if the given primary type is in the base
// included set (Album/Single/EP).
func IsTypeIncluded(releaseType string) bool {
return includedTypes[releaseType]
}
@@ -118,6 +120,8 @@ 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.
return &database.ExternalRelease{
RGID: rg.ID,
ArtistID: artistID,

55
internal/musicbrainz/api_test.go
View File

@@ -12,33 +12,35 @@ import (
// ---------- FilterReleaseGroups tests ----------
func TestFilterReleaseGroups_ExcludesBootlegPromotionPseudo(t *testing.T) {
// Release groups carry no status in ws/2, so status values are irrelevant to
// filtering. These groups differ only by the (ignored) status attribute; all
// are Album/Single and should be retained.
func TestFilterReleaseGroups_StatusIsNotFiltered(t *testing.T) {
groups := []ReleaseGroup{
{ID: "rg-1", Title: "Official Album", Type: "Album", Status: "Official"},
{ID: "rg-2", Title: "Bootleg Live", Type: "Album", Status: "Bootleg"},
{ID: "rg-3", Title: "Promo CD", Type: "Single", Status: "Promotion"},
{ID: "rg-4", Title: "Pseudo Release", Type: "Album", Status: "Pseudo-Release"},
{ID: "rg-1", Title: "Official Album", Type: "Album"},
{ID: "rg-2", Title: "Bootleg Live", Type: "Album"},
{ID: "rg-3", Title: "Promo CD", Type: "Single"},
{ID: "rg-4", Title: "Pseudo Release", Type: "Album"},
}
result := FilterReleaseGroups(groups, FilterOptions{})
if len(result) != 1 {
t.Fatalf("FilterReleaseGroups() returned %d groups, want 1", len(result))
}
if result[0].ID != "rg-1" {
t.Errorf("FilterReleaseGroups()[0].ID = %q, want %q", result[0].ID, "rg-1")
if len(result) != 4 {
t.Fatalf("FilterReleaseGroups() returned %d groups, want 4", len(result))
}
}
func TestFilterReleaseGroups_IncludesOnlyAllowedTypes(t *testing.T) {
groups := []ReleaseGroup{
{ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"},
{ID: "rg-2", Title: "Single", Type: "Single", Status: "Official"},
{ID: "rg-3", Title: "EP", Type: "EP", Status: "Official"},
{ID: "rg-4", Title: "Compilation", Type: "Compilation", Status: "Official"},
{ID: "rg-5", Title: "Soundtrack", Type: "Soundtrack", Status: "Official"},
{ID: "rg-6", Title: "Live", Type: "Live", Status: "Official"},
{ID: "rg-7", Title: "Remix", Type: "Remix", Status: "Official"},
{ID: "rg-1", Title: "Album", Type: "Album"},
{ID: "rg-2", Title: "Single", Type: "Single"},
{ID: "rg-3", Title: "EP", Type: "EP"},
// A compilation whose primary type is Album (the common case) is
// classified via its secondary type and must be included.
{ID: "rg-4", Title: "Greatest Hits", Type: "Album", SecondaryTypes: []string{"Compilation"}},
{ID: "rg-5", Title: "Soundtrack", Type: "Soundtrack"},
{ID: "rg-6", Title: "Live", Type: "Live"},
{ID: "rg-7", Title: "Remix", Type: "Remix"},
}
result := FilterReleaseGroups(groups, FilterOptions{})
@@ -57,9 +59,11 @@ func TestFilterReleaseGroups_IncludesOnlyAllowedTypes(t *testing.T) {
func TestFilterReleaseGroups_IgnoreSingles(t *testing.T) {
groups := []ReleaseGroup{
{ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"},
{ID: "rg-2", Title: "Single", Type: "Single", Status: "Official"},
{ID: "rg-3", Title: "EP", Type: "EP", Status: "Official"},
{ID: "rg-1", Title: "Album", Type: "Album"},
{ID: "rg-2", Title: "Single", Type: "Single"},
{ID: "rg-3", Title: "EP", Type: "EP"},
// Single expressed via secondary type (primary is Album).
{ID: "rg-4", Title: "Single from Album", Type: "Album", SecondaryTypes: []string{"Single"}},
}
result := FilterReleaseGroups(groups, FilterOptions{IgnoreSingles: true})
@@ -68,7 +72,7 @@ func TestFilterReleaseGroups_IgnoreSingles(t *testing.T) {
t.Fatalf("FilterReleaseGroups() returned %d groups, want 2", len(result))
}
for _, rg := range result {
if rg.Type == "Single" {
if rg.Type == "Single" || contains(rg.SecondaryTypes, "Single") {
t.Errorf("single %q should have been filtered out", rg.ID)
}
}
@@ -76,9 +80,9 @@ func TestFilterReleaseGroups_IgnoreSingles(t *testing.T) {
func TestFilterReleaseGroups_IgnoreCompilations(t *testing.T) {
groups := []ReleaseGroup{
{ID: "rg-1", Title: "Album", Type: "Album", Status: "Official"},
{ID: "rg-2", Title: "Compilation", Type: "Compilation", Status: "Official"},
{ID: "rg-3", Title: "EP", Type: "EP", Status: "Official"},
{ID: "rg-1", Title: "Album", Type: "Album"},
{ID: "rg-2", Title: "Greatest Hits", Type: "Album", SecondaryTypes: []string{"Compilation"}},
{ID: "rg-3", Title: "EP", Type: "EP"},
}
result := FilterReleaseGroups(groups, FilterOptions{IgnoreCompilations: true})
@@ -87,7 +91,7 @@ func TestFilterReleaseGroups_IgnoreCompilations(t *testing.T) {
t.Fatalf("FilterReleaseGroups() returned %d groups, want 2", len(result))
}
for _, rg := range result {
if rg.Type == "Compilation" {
if rg.Type == "Compilation" || contains(rg.SecondaryTypes, "Compilation") {
t.Errorf("compilation %q should have been filtered out", rg.ID)
}
}
@@ -100,7 +104,6 @@ func TestReleaseGroup_ToExternalRelease(t *testing.T) {
ID: "rg-uuid-1",
Title: "Dark Side of the Moon",
Type: "Album",
Status: "Official",
ArtistID: "mbid-artist-uuid-1",
ArtistName: "Pink Floyd",
ReleaseDate: "1973-03-01",

49
internal/musicbrainz/client.go
View File

@@ -6,6 +6,7 @@ import (
"fmt"
"io"
"net/http"
"strings"
"time"
"golang.org/x/time/rate"
@@ -93,13 +94,27 @@ type mbArtistCredit struct {
// mbReleaseGroup represents the XML structure of a single release-group
// in the MusicBrainz release-group list response.
//
// Note: release groups do NOT carry a "status" attribute in ws/2 (status
// belongs to individual releases, not release groups), so it is intentionally
// absent here. Type classification is read from the authoritative
// <primary-type> / <secondary-type-list> elements rather than the legacy
// "type" attribute, which only reflects the primary type and cannot detect
// e.g. a compilation whose primary type is Album.
type mbReleaseGroup struct {
ID string `xml:"id,attr"`
Title string `xml:"title"`
Type string `xml:"type,attr"`
Status string `xml:"status,attr"`
ArtistCredit mbArtistCredit `xml:"artist-credit"`
ReleaseDate string `xml:"first-release-date"`
ID string `xml:"id,attr"`
Title string `xml:"title"`
TypeAttr string `xml:"type,attr"`
PrimaryType string `xml:"primary-type"`
Secondary mbSecondaryTypes `xml:"secondary-type-list"`
ArtistCredit mbArtistCredit `xml:"artist-credit"`
ReleaseDate string `xml:"first-release-date"`
}
// mbSecondaryTypes captures the <secondary-type-list> element, which holds
// zero or more <secondary-type> children (e.g. Live, Compilation, Remix).
type mbSecondaryTypes struct {
Types []string `xml:"secondary-type"`
}
// mbReleaseGroupListXML wraps the release-group-list element to properly
@@ -127,14 +142,22 @@ func ParseReleaseGroups(data []byte) (*ParsedReleaseGroups, error) {
Count: list.ReleaseGroupList.Count,
}
for _, rg := range list.ReleaseGroupList.ReleaseGroups {
// Prefer the authoritative <primary-type> element; fall back to the
// legacy "type" attribute (which reflects the primary type) when the
// element is absent. The attribute is space-separated primary+secondary,
// so take the first token as the primary type.
primary := rg.PrimaryType
if primary == "" && rg.TypeAttr != "" {
primary = strings.Fields(rg.TypeAttr)[0]
}
result.ReleaseGroups = append(result.ReleaseGroups, ReleaseGroup{
ID: rg.ID,
Title: rg.Title,
Type: rg.Type,
Status: rg.Status,
ArtistID: rg.ArtistCredit.NameCredit.Artist.ID,
ArtistName: rg.ArtistCredit.NameCredit.Artist.Name,
ReleaseDate: rg.ReleaseDate,
ID: rg.ID,
Title: rg.Title,
Type: primary,
SecondaryTypes: rg.Secondary.Types,
ArtistID: rg.ArtistCredit.NameCredit.Artist.ID,
ArtistName: rg.ArtistCredit.NameCredit.Artist.Name,
ReleaseDate: rg.ReleaseDate,
})
}
return result, nil

19
internal/musicbrainz/model.go
View File

@@ -3,14 +3,19 @@ package musicbrainz
// ReleaseGroup represents a MusicBrainz Release Group entity.
// This is the primary data model for the provider - we work with
// Release Groups to minimize duplicates from different releases.
//
// Type holds the primary type (Album, Single, EP, Other, Broadcast, ...).
// SecondaryTypes holds secondary type classifications (Live, Compilation,
// Remix, ...). Together they drive the scanner's type filtering; release
// groups have no status, so there is no Status field.
type ReleaseGroup struct {
ID string
Title string
Type string
Status string
ArtistID string
ArtistName string
ReleaseDate string
ID string
Title string
Type string
SecondaryTypes []string
ArtistID string
ArtistName string
ReleaseDate string
}
// ParsedReleaseGroups holds the result of parsing a MusicBrainz

61
internal/musicbrainz/model_test.go
View File

@@ -137,12 +137,13 @@ func TestParseReleaseGroups_MalformedXML(t *testing.T) {
}
}
func TestParseReleaseGroups_WithStatus(t *testing.T) {
func TestParseReleaseGroups_PrimaryAndSecondaryTypes(t *testing.T) {
data := []byte(`<?xml version="1.0" encoding="UTF-8"?>
<metadata xmlns="http://musicbrainz.org/ns/mmd-2.0#">
<release-group-list count="1">
<release-group id="rg-bootleg" type="Album" status="Bootleg">
<title>Unofficial Live Recording</title>
<release-group-list count="2">
<release-group id="rg-album" type="Album">
<title>Studio Album</title>
<primary-type>Album</primary-type>
<first-release-date>2020-01-01</first-release-date>
<artist-credit>
<name-credit>
@@ -152,6 +153,22 @@ func TestParseReleaseGroups_WithStatus(t *testing.T) {
</name-credit>
</artist-credit>
</release-group>
<release-group id="rg-comp" type="Album Compilation">
<title>Greatest Hits</title>
<primary-type>Album</primary-type>
<secondary-type-list>
<secondary-type>Compilation</secondary-type>
<secondary-type>Live</secondary-type>
</secondary-type-list>
<first-release-date>2021-05-05</first-release-date>
<artist-credit>
<name-credit>
<artist id="artist-uuid-2">
<name>Test Artist</name>
</artist>
</name-credit>
</artist-credit>
</release-group>
</release-group-list>
</metadata>`)
@@ -160,11 +177,39 @@ func TestParseReleaseGroups_WithStatus(t *testing.T) {
t.Fatalf("ParseReleaseGroups() error = %v", err)
}
if len(result.ReleaseGroups) != 1 {
t.Fatalf("ParseReleaseGroups() returned %d groups, want 1", len(result.ReleaseGroups))
if len(result.ReleaseGroups) != 2 {
t.Fatalf("ParseReleaseGroups() returned %d groups, want 2", len(result.ReleaseGroups))
}
if result.ReleaseGroups[0].Status != "Bootleg" {
t.Errorf("ReleaseGroups[0].Status = %q, want %q", result.ReleaseGroups[0].Status, "Bootleg")
byID := make(map[string]ReleaseGroup)
for _, rg := range result.ReleaseGroups {
byID[rg.ID] = rg
}
album := byID["rg-album"]
if album.Type != "Album" {
t.Errorf("rg-album.Type = %q, want %q", album.Type, "Album")
}
if len(album.SecondaryTypes) != 0 {
t.Errorf("rg-album.SecondaryTypes = %v, want empty", album.SecondaryTypes)
}
comp := byID["rg-comp"]
if comp.Type != "Album" {
t.Errorf("rg-comp.Type = %q, want %q", comp.Type, "Album")
}
// Release groups have no status attribute; the secondary type list is the
// authoritative source for classifications like Compilation.
if !contains(comp.SecondaryTypes, "Compilation") || !contains(comp.SecondaryTypes, "Live") {
t.Errorf("rg-comp.SecondaryTypes = %v, want Compilation and Live", comp.SecondaryTypes)
}
}
func contains(s []string, want string) bool {
for _, v := range s {
if v == want {
return true
}
}
return false
}

28
internal/musicbrainz/sync_test.go
View File

@@ -19,8 +19,12 @@ func mbReleaseGroupXML(id, title, rgType, status, artistID, artistName, releaseD
if status != "" {
statusAttr = ` status="` + status + `"`
}
// Release groups carry type via <primary-type>; the legacy "type" attribute
// is also emitted (ignored by the parser) for realism. Status has no meaning
// for release groups and is not parsed.
return `<release-group id="` + id + `" type="` + rgType + `"` + statusAttr + `>` +
`<title>` + title + `</title>` +
`<primary-type>` + rgType + `</primary-type>` +
`<artist-credit><name-credit><artist id="` + artistID + `">` +
`<name>` + artistName + `</name>` +
`</artist></name-credit></artist-credit>` +
@@ -200,13 +204,14 @@ func TestSyncArtistDiscography_CacheHit_ReturnsCached(t *testing.T) {
// Test: SyncArtistDiscography applies filtering (excluded statuses)
// -----------------------------------------------------------------------
func TestSyncArtistDiscography_FiltersExcludedStatuses(t *testing.T) {
func TestSyncArtistDiscography_StatusIsNotFiltered(t *testing.T) {
artistMBID := "cccccccc-dddd-eeee-ffff-000000000000"
artistID := "nav-cccccccc"
server := newTestMBServer(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/xml")
// Include a Bootleg and a Promotion that should be filtered out.
// These all carry status values, but release groups have no status in
// ws/2, so none should be filtered on that basis.
resp := mbReleaseGroupListResponse(
mbReleaseGroupXML("rg-legit", "Legit Album", "Album", "", artistMBID, "Artist", "2020-01-01")+
mbReleaseGroupXML("rg-bootleg", "Bootleg Album", "Album", "Bootleg", artistMBID, "Artist", "2020-02-01")+
@@ -231,12 +236,9 @@ func TestSyncArtistDiscography_FiltersExcludedStatuses(t *testing.T) {
t.Fatalf("SyncArtistDiscography() error: %v", err)
}
// Only the legit album should remain after filtering.
if len(releases) != 1 {
t.Fatalf("expected 1 release after filtering, got %d", len(releases))
}
if releases[0].RGID != "rg-legit" {
t.Errorf("expected RGID 'rg-legit', got %q", releases[0].RGID)
// All four are Albums; status is not a filter, so all four are kept.
if len(releases) != 4 {
t.Fatalf("expected 4 releases (status is not filtered), got %d", len(releases))
}
}
@@ -275,9 +277,10 @@ func TestSyncArtistDiscography_FiltersExcludedTypes(t *testing.T) {
t.Fatalf("SyncArtistDiscography() error: %v", err)
}
// Soundtrack should be excluded (not in includedTypes).
if len(releases) != 4 {
t.Fatalf("expected 4 releases after type filtering, got %d", len(releases))
// Soundtrack and the bare "Compilation" primary type should be excluded
// (Compilation is not a primary type; it is classified via secondary type).
if len(releases) != 3 {
t.Fatalf("expected 3 releases after type filtering, got %d", len(releases))
}
rgIDs := make(map[string]bool)
@@ -287,6 +290,9 @@ func TestSyncArtistDiscography_FiltersExcludedTypes(t *testing.T) {
if rgIDs["rg-soundtrack"] {
t.Error("Soundtrack type should have been filtered out")
}
if rgIDs["rg-comp"] {
t.Error("Compilation primary type should have been filtered out")
}
}
// -----------------------------------------------------------------------