From a7803615cd16b79d6d4e3054998275646336ce8d Mon Sep 17 00:00:00 2001 From: Vladimir Zagainov Date: Sun, 19 Jul 2026 19:45:08 +0300 Subject: [PATCH] fix: address code review findings --- internal/musicbrainz/api.go | 56 ++++++++++++++------------- internal/musicbrainz/api_test.go | 55 ++++++++++++++------------- internal/musicbrainz/client.go | 49 +++++++++++++++++------- internal/musicbrainz/model.go | 19 ++++++---- internal/musicbrainz/model_test.go | 61 ++++++++++++++++++++++++++---- internal/musicbrainz/sync_test.go | 28 ++++++++------ internal/scanner/scanner.go | 11 ++++-- 7 files changed, 185 insertions(+), 94 deletions(-) diff --git a/internal/musicbrainz/api.go b/internal/musicbrainz/api.go index 476c014..5911174 100644 --- a/internal/musicbrainz/api.go +++ b/internal/musicbrainz/api.go @@ -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, diff --git a/internal/musicbrainz/api_test.go b/internal/musicbrainz/api_test.go index a3fac59..03026f9 100644 --- a/internal/musicbrainz/api_test.go +++ b/internal/musicbrainz/api_test.go @@ -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", diff --git a/internal/musicbrainz/client.go b/internal/musicbrainz/client.go index df2cecf..5ee2bc6 100644 --- a/internal/musicbrainz/client.go +++ b/internal/musicbrainz/client.go @@ -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 +// / 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 element, which holds +// zero or more 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 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 diff --git a/internal/musicbrainz/model.go b/internal/musicbrainz/model.go index 942f2ee..ab21588 100644 --- a/internal/musicbrainz/model.go +++ b/internal/musicbrainz/model.go @@ -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 diff --git a/internal/musicbrainz/model_test.go b/internal/musicbrainz/model_test.go index 4f1f241..f5440e8 100644 --- a/internal/musicbrainz/model_test.go +++ b/internal/musicbrainz/model_test.go @@ -137,12 +137,13 @@ func TestParseReleaseGroups_MalformedXML(t *testing.T) { } } -func TestParseReleaseGroups_WithStatus(t *testing.T) { +func TestParseReleaseGroups_PrimaryAndSecondaryTypes(t *testing.T) { data := []byte(` - - - Unofficial Live Recording + + + Studio Album + Album 2020-01-01 @@ -152,6 +153,22 @@ func TestParseReleaseGroups_WithStatus(t *testing.T) { + + Greatest Hits + Album + + Compilation + Live + + 2021-05-05 + + + + Test Artist + + + + `) @@ -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 +} diff --git a/internal/musicbrainz/sync_test.go b/internal/musicbrainz/sync_test.go index a920f8c..1dc2f80 100644 --- a/internal/musicbrainz/sync_test.go +++ b/internal/musicbrainz/sync_test.go @@ -19,8 +19,12 @@ func mbReleaseGroupXML(id, title, rgType, status, artistID, artistName, releaseD if status != "" { statusAttr = ` status="` + status + `"` } + // Release groups carry type via ; 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 `` + `` + title + `` + + `` + rgType + `` + `` + `` + artistName + `` + `` + @@ -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") + } } // ----------------------------------------------------------------------- diff --git a/internal/scanner/scanner.go b/internal/scanner/scanner.go index bbb7bc9..866b8fe 100644 --- a/internal/scanner/scanner.go +++ b/internal/scanner/scanner.go @@ -6,6 +6,8 @@ package scanner import ( + "unicode/utf8" + "github.com/lithammer/fuzzysearch/fuzzy" "naviwatcher/internal/normalize" ) @@ -44,10 +46,13 @@ func Similarity(a, b string) float64 { return 0.0 } + // fuzzy.LevenshteinDistance operates on runes, so the comparison basis + // must be rune count, not byte length, to avoid biasing the score for + // non-ASCII titles (where bytes > runes). dist := fuzzy.LevenshteinDistance(na, nb) - maxLen := len(na) - if len(nb) > maxLen { - maxLen = len(nb) + maxLen := utf8.RuneCountInString(na) + if rb := utf8.RuneCountInString(nb); rb > maxLen { + maxLen = rb } // 1.0 - normalized distance → higher is more similar.