bvandeusen/minstrel
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 38 Wiki Activity

fix(server/m7-382): MBID extraction never matched dhowden/tag's Raw() keys

Browse Source 
The hand-rolled extractor in internal/library/mbids.go looked up keys that
dhowden/tag never produces, for every supported audio format:

  Vorbis (FLAC/OGG): dhowden lowercases keys at parse time (vorbis.go:59),
    we looked up MUSICBRAINZ_ALBUMID. Always missed.
  ID3v2 (MP3): dhowden stores TXXX frames under bare TXXX/TXXX_N keys
    with *tag.Comm values; the Picard tag name is on the value's
    Description field, not part of the key. We looked up
    TXXX:MusicBrainz Album Id. Always missed.
  MP4 (M4A): dhowden stores freeform iTunes atoms under their bare
    sub-name. We looked up ----:com.apple.iTunes:MusicBrainz Album Id.
    Always missed.

Net effect: every album in the m7-380 boot backfill reported
processed=N, healed=0, skipped=N. The m7-379 enricher gate then
short-circuited NULL on every album, so MBCAA was never queried.
Symptom: "all albums skipped" during cover enrichment.

The pre-existing unit tests passed because they hand-built Raw() maps
with the intended-but-incorrect keys, never round-tripping through
dhowden's parser.

Replace the hand-rolled extractor with dhowden's own mbz sub-package,
which knows the per-format Raw() conventions and uses lowercase
canonical keys (mbz.Album, mbz.Artist). Update both call sites to pass
the full tag.Metadata instead of just Raw(). Rewrite the test with a
stubMeta implementing tag.Metadata, asserting against the actual
per-format Raw() shape, plus a regression guard that pins "uppercase
Vorbis keys must not match" so the v0 bug can't sneak back.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Bryan Van Deusen
2026-05-04 21:15:51 -04:00
parent 0db22971c7
commit 947f944fe9
4 changed files with 108 additions and 102 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/library/mbidbackfill.go
+1 -1
View File
@@ -119,5 +119,5 @@ func readMBIDsForFile(path string, logger *slog.Logger) (albumMBID, artistMBID s
logger.Warn("mbid backfill: tag read failed", "path", path, "err", err)
return "", ""
}
return extractMBIDs(meta.Raw())
return extractMBIDs(meta)
}
internal/library/mbids.go
+19 -55
View File
@@ -1,59 +1,23 @@
package library
// MusicBrainz tag keys vary by audio format. dhowden/tag's Raw() map exposes
// the format-specific entries as-is — so we look up multiple candidates and
// take the first non-empty match.
import (
"github.com/dhowden/tag"
"github.com/dhowden/tag/mbz"
)
// extractMBIDs reads MusicBrainz album and artist IDs from a tag.Metadata.
//
// References:
// - ID3v2 (MP3): TXXX frames keyed by description.
// - Vorbis (FLAC/OGG): all-caps comment names.
// - MP4 (M4A): iTunes freeform atoms ("----:com.apple.iTunes:...").
var albumMBIDKeys = []string{
"TXXX:MusicBrainz Album Id",
"MUSICBRAINZ_ALBUMID",
"----:com.apple.iTunes:MusicBrainz Album Id",
}
var artistMBIDKeys = []string{
"TXXX:MusicBrainz Artist Id",
"MUSICBRAINZ_ARTISTID",
"----:com.apple.iTunes:MusicBrainz Artist Id",
}
// extractMBIDs reads MusicBrainz album and artist IDs from a tag.Metadata
// Raw() map. Returns ("", "") when the keys are absent. Multi-value keys
// (some formats list multiple artist IDs for collaborations) take only the
// first value — Minstrel uses the primary release artist for MBCAA lookup.
func extractMBIDs(raw map[string]interface{}) (albumMBID, artistMBID string) {
albumMBID = firstStringValue(raw, albumMBIDKeys)
artistMBID = firstStringValue(raw, artistMBIDKeys)
return
}
func firstStringValue(raw map[string]interface{}, keys []string) string {
for _, k := range keys {
v, ok := raw[k]
if !ok || v == nil {
continue
}
switch x := v.(type) {
case string:
if x != "" {
return x
}
case []string:
if len(x) > 0 && x[0] != "" {
return x[0]
}
case []interface{}:
if len(x) == 0 {
continue
}
if s, ok := x[0].(string); ok && s != "" {
return s
}
}
}
return ""
// Delegates to dhowden/tag/mbz, which knows the per-format Raw() conventions:
// - Vorbis (FLAC/OGG) keys are lowercased by the parser.
// - ID3v2 (MP3) TXXX frames are stored under "TXXX"/"TXXX_N" with
// a *tag.Comm value whose Description holds the Picard tag name.
// - MP4 (M4A) freeform iTunes atoms are stored under their bare sub-name.
//
// We previously hand-rolled this lookup against guessed keys
// ("MUSICBRAINZ_ALBUMID", "TXXX:MusicBrainz Album Id", ...), none of which
// dhowden actually produces — so every extraction returned ("",""), which
// fed an empty MBID into both the scanner and the boot-time backfill.
func extractMBIDs(m tag.Metadata) (albumMBID, artistMBID string) {
info := mbz.Extract(m)
return info.Get(mbz.Album), info.Get(mbz.Artist)
}
internal/library/mbids_test.go
+87 -45
View File
@@ -1,69 +1,111 @@
package library
import "testing"
import (
"testing"
"github.com/dhowden/tag"
)
// stubMeta is a minimal tag.Metadata used to feed the dhowden mbz extractor
// the same Raw()/Format() shape its real parsers produce. The other interface
// methods return zero values — the extractor only branches on Format and
// iterates Raw().
type stubMeta struct {
format tag.Format
raw map[string]interface{}
}
func (s stubMeta) Format() tag.Format { return s.format }
func (s stubMeta) FileType() tag.FileType { return tag.UnknownFileType }
func (s stubMeta) Title() string { return "" }
func (s stubMeta) Album() string { return "" }
func (s stubMeta) Artist() string { return "" }
func (s stubMeta) AlbumArtist() string { return "" }
func (s stubMeta) Composer() string { return "" }
func (s stubMeta) Year() int { return 0 }
func (s stubMeta) Genre() string { return "" }
func (s stubMeta) Track() (int, int) { return 0, 0 }
func (s stubMeta) Disc() (int, int) { return 0, 0 }
func (s stubMeta) Picture() *tag.Picture { return nil }
func (s stubMeta) Lyrics() string { return "" }
func (s stubMeta) Comment() string { return "" }
func (s stubMeta) Raw() map[string]interface{} { return s.raw }
func TestExtractMBIDs_Vorbis(t *testing.T) {
// dhowden lowercases all Vorbis comment keys at parse time.
m := stubMeta{
format: tag.VORBIS,
raw: map[string]interface{}{
"musicbrainz_albumid": "abc",
"musicbrainz_artistid": "def",
},
}
a, ar := extractMBIDs(m)
if a != "abc" || ar != "def" {
t.Errorf("got (%q, %q), want (\"abc\", \"def\")", a, ar)
}
}
func TestExtractMBIDs_ID3v2(t *testing.T) {
raw := map[string]interface{}{
"TXXX:MusicBrainz Album Id": "abc-123",
"TXXX:MusicBrainz Artist Id": "def-456",
// dhowden stores TXXX frames under bare "TXXX"/"TXXX_N" keys with
// *tag.Comm values whose Description holds the Picard tag name.
m := stubMeta{
format: tag.ID3v2_4,
raw: map[string]interface{}{
"TXXX": &tag.Comm{
Description: "MusicBrainz Album Id",
Text: "abc-123",
},
"TXXX_0": &tag.Comm{
Description: "MusicBrainz Artist Id",
Text: "def-456",
},
},
}
a, ar := extractMBIDs(raw)
a, ar := extractMBIDs(m)
if a != "abc-123" || ar != "def-456" {
t.Errorf("got (%q, %q), want (\"abc-123\", \"def-456\")", a, ar)
}
}
func TestExtractMBIDs_Vorbis(t *testing.T) {
raw := map[string]interface{}{
"MUSICBRAINZ_ALBUMID": "abc",
"MUSICBRAINZ_ARTISTID": "def",
}
a, ar := extractMBIDs(raw)
if a != "abc" || ar != "def" {
t.Errorf("got (%q, %q)", a, ar)
}
}
func TestExtractMBIDs_MP4(t *testing.T) {
raw := map[string]interface{}{
"----:com.apple.iTunes:MusicBrainz Album Id": "abc",
"----:com.apple.iTunes:MusicBrainz Artist Id": "def",
// dhowden stores freeform iTunes atoms under their bare sub-name.
// mbz.Extract maps the human-readable form back to its lowercase key.
m := stubMeta{
format: tag.MP4,
raw: map[string]interface{}{
"MusicBrainz Album Id": "abc",
"MusicBrainz Artist Id": "def",
},
}
a, ar := extractMBIDs(raw)
a, ar := extractMBIDs(m)
if a != "abc" || ar != "def" {
t.Errorf("got (%q, %q)", a, ar)
t.Errorf("got (%q, %q), want (\"abc\", \"def\")", a, ar)
}
}
func TestExtractMBIDs_MissingKeysReturnEmpty(t *testing.T) {
a, ar := extractMBIDs(map[string]interface{}{})
func TestExtractMBIDs_MissingTagsReturnEmpty(t *testing.T) {
m := stubMeta{format: tag.VORBIS, raw: map[string]interface{}{}}
a, ar := extractMBIDs(m)
if a != "" || ar != "" {
t.Errorf("got (%q, %q), want empty", a, ar)
}
}
func TestExtractMBIDs_MultiValueArtistTakesFirst(t *testing.T) {
// Some FLAC tags list collaborators as multi-value MUSICBRAINZ_ARTISTID.
// Minstrel uses the primary release artist; first wins.
raw := map[string]interface{}{
"MUSICBRAINZ_ARTISTID": []string{"primary-mbid", "collab-mbid"},
func TestExtractMBIDs_VorbisUppercaseKeysMiss(t *testing.T) {
// Regression guard: real dhowden never produces uppercase Vorbis
// keys. If a future change starts looking up "MUSICBRAINZ_ALBUMID"
// directly, this test must keep returning empty so we don't
// silently re-introduce the v0 bug where every album skipped.
m := stubMeta{
format: tag.VORBIS,
raw: map[string]interface{}{
"MUSICBRAINZ_ALBUMID": "wrong-case",
"MUSICBRAINZ_ARTISTID": "wrong-case",
},
}
_, ar := extractMBIDs(raw)
if ar != "primary-mbid" {
t.Errorf("got %q, want %q", ar, "primary-mbid")
}
}
func TestExtractMBIDs_NilEntryIgnored(t *testing.T) {
raw := map[string]interface{}{
"TXXX:MusicBrainz Album Id": nil,
"TXXX:MusicBrainz Artist Id": "real-id",
}
a, ar := extractMBIDs(raw)
if a != "" {
t.Errorf("nil album entry should be ignored, got %q", a)
}
if ar != "real-id" {
t.Errorf("artist = %q", ar)
a, ar := extractMBIDs(m)
if a != "" || ar != "" {
t.Errorf("uppercase Vorbis keys must not match dhowden output; got (%q, %q)", a, ar)
}
}
internal/library/scanner.go
+1 -1
View File
@@ -131,7 +131,7 @@ func (s *Scanner) scanFile(ctx context.Context, q *dbq.Queries, path string, sta
if err != nil {
return fmt.Errorf("tag read: %w", err)
}
albumMBID, artistMBID := extractMBIDs(meta.Raw())
albumMBID, artistMBID := extractMBIDs(meta)
artistName := meta.Artist()
if artistName == "" {