From 947f944fe9c45dc8a5965a8904ea50d97b39a150 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Mon, 4 May 2026 21:15:51 -0400 Subject: [PATCH] fix(server/m7-382): MBID extraction never matched dhowden/tag's Raw() keys 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) --- internal/library/mbidbackfill.go | 2 +- internal/library/mbids.go | 74 +++++------------ internal/library/mbids_test.go | 132 ++++++++++++++++++++----------- internal/library/scanner.go | 2 +- 4 files changed, 108 insertions(+), 102 deletions(-) diff --git a/internal/library/mbidbackfill.go b/internal/library/mbidbackfill.go index 6715be65..84040595 100644 --- a/internal/library/mbidbackfill.go +++ b/internal/library/mbidbackfill.go @@ -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) } diff --git a/internal/library/mbids.go b/internal/library/mbids.go index a1064684..fcaf78ad 100644 --- a/internal/library/mbids.go +++ b/internal/library/mbids.go @@ -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) } diff --git a/internal/library/mbids_test.go b/internal/library/mbids_test.go index 00c92e21..376257be 100644 --- a/internal/library/mbids_test.go +++ b/internal/library/mbids_test.go @@ -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) } } diff --git a/internal/library/scanner.go b/internal/library/scanner.go index c44f5277..d3753269 100644 --- a/internal/library/scanner.go +++ b/internal/library/scanner.go @@ -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 == "" {