From c30511e71b5eef28ec6285bdf29a9aa4b1c55e25 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 14 Jul 2026 07:43:22 -0400 Subject: [PATCH] feat(tags): MusicBrainz artist-MBID tag fallback (#1519) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Widen keyless coverage: when a track's recording is untagged or has no recording MBID, fall back to the artist's tags (/ws/2/artist/{mbid} ?inc=tags), down-weighted 0.6 as a coarser signal. Recording tags still win outright when present. - ListTracksMissingTags returns a.mbid AS artist_mbid; TrackRef gains ArtistMBID; the enricher threads it through. - MB provider: recording-first, artist-fallback via a shared fetchEntityTags helper. A transient error at the recording step is returned (retry) rather than masked by the fallback. Enricher, registry, and settings are untouched — the pluggable design absorbs the wider lookup. Tests cover fallback-when-untagged, artist-only-when-no-recording-MBID, and recording-preferred. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/db/dbq/track_tags.sql.go | 7 +- internal/db/queries/track_tags.sql | 5 +- internal/tags/enricher.go | 3 + internal/tags/provider.go | 5 +- internal/tags/provider_musicbrainz.go | 63 ++++++++++++---- internal/tags/provider_musicbrainz_test.go | 88 +++++++++++++++++++++- 6 files changed, 151 insertions(+), 20 deletions(-) diff --git a/internal/db/dbq/track_tags.sql.go b/internal/db/dbq/track_tags.sql.go index c0a4fbc5..5c2d78b4 100644 --- a/internal/db/dbq/track_tags.sql.go +++ b/internal/db/dbq/track_tags.sql.go @@ -112,7 +112,7 @@ func (q *Queries) ListPlayedTrackTagsForUser(ctx context.Context, arg ListPlayed const listTracksMissingTags = `-- name: ListTracksMissingTags :many -SELECT t.id, t.mbid, t.title, a.name AS artist_name +SELECT t.id, t.mbid, t.title, a.name AS artist_name, a.mbid AS artist_mbid FROM tracks t JOIN artists a ON a.id = t.artist_id WHERE t.tag_source IS NULL @@ -131,6 +131,7 @@ type ListTracksMissingTagsRow struct { Mbid *string Title string ArtistName string + ArtistMbid *string } // Track-tag enrichment queries (milestone #160, #1490). The tag enricher @@ -140,7 +141,8 @@ type ListTracksMissingTagsRow struct { // Tracks eligible for tag enrichment: never processed (tag_source NULL) // or previously settled 'none' under an older provider version. Returns // the fields the provider chain needs — recording MBID (nullable) for -// keyed lookups, plus title + artist name for name-based fallback. +// keyed lookups, title + artist name for name-based fallback, and the +// artist MBID (nullable) for MusicBrainz's artist-tag fallback (#1519). // $1 = current tag_sources_version, $2 = limit. func (q *Queries) ListTracksMissingTags(ctx context.Context, arg ListTracksMissingTagsParams) ([]ListTracksMissingTagsRow, error) { rows, err := q.db.Query(ctx, listTracksMissingTags, arg.TagSourcesVersion, arg.Limit) @@ -156,6 +158,7 @@ func (q *Queries) ListTracksMissingTags(ctx context.Context, arg ListTracksMissi &i.Mbid, &i.Title, &i.ArtistName, + &i.ArtistMbid, ); err != nil { return nil, err } diff --git a/internal/db/queries/track_tags.sql b/internal/db/queries/track_tags.sql index c5557fc3..d5c6b14f 100644 --- a/internal/db/queries/track_tags.sql +++ b/internal/db/queries/track_tags.sql @@ -7,9 +7,10 @@ -- Tracks eligible for tag enrichment: never processed (tag_source NULL) -- or previously settled 'none' under an older provider version. Returns -- the fields the provider chain needs — recording MBID (nullable) for --- keyed lookups, plus title + artist name for name-based fallback. +-- keyed lookups, title + artist name for name-based fallback, and the +-- artist MBID (nullable) for MusicBrainz's artist-tag fallback (#1519). -- $1 = current tag_sources_version, $2 = limit. -SELECT t.id, t.mbid, t.title, a.name AS artist_name +SELECT t.id, t.mbid, t.title, a.name AS artist_name, a.mbid AS artist_mbid FROM tracks t JOIN artists a ON a.id = t.artist_id WHERE t.tag_source IS NULL diff --git a/internal/tags/enricher.go b/internal/tags/enricher.go index 74e557b4..60b9909f 100644 --- a/internal/tags/enricher.go +++ b/internal/tags/enricher.go @@ -171,6 +171,9 @@ func (e *Enricher) EnrichTrackBatch(ctx context.Context, limit int, if r.Mbid != nil { ref.MBID = *r.Mbid } + if r.ArtistMbid != nil { + ref.ArtistMBID = *r.ArtistMbid + } oc, eerr := e.EnrichTrack(ctx, r.ID, ref) if eerr != nil { e.logger.Warn("tags: batch entry failed", "track_id", uuidString(r.ID), "err", eerr) diff --git a/internal/tags/provider.go b/internal/tags/provider.go index f1450b29..d1d8c8ce 100644 --- a/internal/tags/provider.go +++ b/internal/tags/provider.go @@ -32,11 +32,14 @@ type Tag struct { // TrackRef is the lookup key passed to a provider. MBID is the track's // recording MBID (preferred by MBID-keyed providers); ArtistName + Title -// are the always-populated fallback for name-based providers (Last.fm). +// are the always-populated fallback for name-based providers (Last.fm); +// ArtistMBID (nullable) enables MusicBrainz's artist-tag fallback (#1519) +// when the recording itself is untagged or MBID-less. type TrackRef struct { MBID string ArtistName string Title string + ArtistMBID string } // Provider is the base shape every tag source implements. Providers diff --git a/internal/tags/provider_musicbrainz.go b/internal/tags/provider_musicbrainz.go index 100c48ee..5c89e7d9 100644 --- a/internal/tags/provider_musicbrainz.go +++ b/internal/tags/provider_musicbrainz.go @@ -17,13 +17,18 @@ const ( mbMinPeriod = 1100 * time.Millisecond // mbUserAgent identifies the app — MusicBrainz rejects requests without one. mbUserAgent = "Minstrel/1.0 ( https://git.fabledsword.com/bvandeusen/minstrel )" + // artistTagWeightFactor down-weights artist-level fallback tags relative + // to recording-specific tags — the artist's overall character is a coarser + // signal than a tag on the exact recording (#1519). + artistTagWeightFactor = 0.6 ) -// musicbrainzProvider fetches recording-level folksonomy tags from the -// MusicBrainz web service. Keyless (the public endpoint needs no auth) and -// on by default — the always-available baseline source. MBID-only: a track -// with no recording MBID yields ErrNotFound (name search is ambiguous for -// recordings and deferred to name-based providers like Last.fm). +// musicbrainzProvider fetches folksonomy tags from the MusicBrainz web +// service. Keyless (the public endpoint needs no auth) and on by default — +// the always-available baseline source. Prefers recording-level tags by the +// track's recording MBID; when the recording is untagged or MBID-less it +// falls back to the artist's tags by artist MBID (#1519), down-weighted as a +// coarser signal. Name-based lookup is deferred to providers like Last.fm. type musicbrainzProvider struct { enabled atomic.Bool client *httpClient @@ -61,24 +66,54 @@ type mbTag struct { Name string `json:"name"` } -// FetchTrackTags looks up the recording's tags by MBID. Returns ErrNotFound -// when disabled, when the track has no MBID, or when MusicBrainz has no tags. +// FetchTrackTags looks up recording tags by MBID, then falls back to artist +// tags by artist MBID when the recording is untagged or MBID-less. Returns +// ErrNotFound when disabled or when neither level yields tags. A transient +// error at the recording step is returned as-is (retry) rather than silently +// masked by the artist fallback. func (p *musicbrainzProvider) FetchTrackTags(ctx context.Context, ref TrackRef) ([]Tag, error) { - if !p.enabled.Load() || ref.MBID == "" { + if !p.enabled.Load() { return nil, ErrNotFound } - q := url.Values{"inc": {"tags"}, "fmt": {"json"}} - full := mbBaseURL + "/recording/" + url.PathEscape(ref.MBID) + "?" + q.Encode() + if ref.MBID != "" { + tags, err := p.fetchEntityTags(ctx, "recording", ref.MBID, 1.0) + if err != nil && !errors.Is(err, ErrNotFound) { + return nil, err + } + if len(tags) > 0 { + return tags, nil + } + } + if ref.ArtistMBID != "" { + tags, err := p.fetchEntityTags(ctx, "artist", ref.ArtistMBID, artistTagWeightFactor) + if err != nil && !errors.Is(err, ErrNotFound) { + return nil, err + } + if len(tags) > 0 { + return tags, nil + } + } + return nil, ErrNotFound +} +// fetchEntityTags loads folksonomy tags for a MusicBrainz entity ("recording" +// or "artist") by MBID and scales the normalized weights by `scale`. Returns +// an empty slice (not ErrNotFound) when the entity exists but is untagged, so +// the caller can decide whether to fall through to the next level. +func (p *musicbrainzProvider) fetchEntityTags(ctx context.Context, entity, mbid string, scale float64) ([]Tag, error) { + q := url.Values{"inc": {"tags"}, "fmt": {"json"}} + full := mbBaseURL + "/" + entity + "/" + url.PathEscape(mbid) + "?" + q.Encode() var resp mbTagsResponse if err := p.client.getJSON(ctx, full, &resp); err != nil { return nil, err } - out := normalizeMBTags(resp.Tags) - if len(out) == 0 { - return nil, ErrNotFound + tags := normalizeMBTags(resp.Tags) + if scale != 1.0 { + for i := range tags { + tags[i].Weight *= scale + } } - return out, nil + return tags, nil } // TestConnection issues a tiny search request; any non-transient response diff --git a/internal/tags/provider_musicbrainz_test.go b/internal/tags/provider_musicbrainz_test.go index 8f6d4a07..8af8774c 100644 --- a/internal/tags/provider_musicbrainz_test.go +++ b/internal/tags/provider_musicbrainz_test.go @@ -5,6 +5,7 @@ import ( "errors" "net/http" "net/http/httptest" + "strings" "testing" ) @@ -50,13 +51,98 @@ func TestMusicBrainzFetch_GatedOff(t *testing.T) { TrackRef{MBID: "abc"}); !errors.Is(err, ErrNotFound) { t.Errorf("disabled: err = %v, want ErrNotFound", err) } - // No MBID → ErrNotFound (MBID-only provider). + // No recording MBID and no artist MBID → nothing to look up → ErrNotFound. if _, err := newMBProvider(true).FetchTrackTags(context.Background(), TrackRef{ArtistName: "A", Title: "B"}); !errors.Is(err, ErrNotFound) { t.Errorf("no MBID: err = %v, want ErrNotFound", err) } } +// mbEntityServer returns tags keyed by entity path segment ("recording" or +// "artist"), so a test can make the recording untagged while the artist has +// tags (or vice-versa). An empty body string means "{tags:[]}". +func mbEntityServer(recordingJSON, artistJSON string) *httptest.Server { + empty := `{"tags":[]}` + return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + body := empty + switch { + case strings.Contains(r.URL.Path, "/recording/"): + if recordingJSON != "" { + body = recordingJSON + } + case strings.Contains(r.URL.Path, "/artist/"): + if artistJSON != "" { + body = artistJSON + } + } + _, _ = w.Write([]byte(body)) + })) +} + +func TestMusicBrainzFetch_ArtistFallbackWhenRecordingUntagged(t *testing.T) { + // Recording exists but has no tags; artist has tags → use the artist's, + // down-weighted by artistTagWeightFactor. + srv := mbEntityServer(``, `{"tags":[{"count":4,"name":"art rock"},{"count":2,"name":"experimental"}]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + tags, err := newMBProvider(true).FetchTrackTags(context.Background(), + TrackRef{MBID: "rec-1", ArtistMBID: "art-1"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + m := tagsByName(tags) + // "art rock" is the top artist tag (weight 1.0) × 0.6 fallback factor. + if got := m["art rock"]; got != 0.6 { + t.Errorf("art rock = %v, want 0.6 (down-weighted)", got) + } + if got := m["experimental"]; got != 0.3 { // 0.5 × 0.6 + t.Errorf("experimental = %v, want 0.3", got) + } +} + +func TestMusicBrainzFetch_ArtistOnlyWhenNoRecordingMBID(t *testing.T) { + srv := mbEntityServer(``, `{"tags":[{"count":1,"name":"folk"}]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + tags, err := newMBProvider(true).FetchTrackTags(context.Background(), + TrackRef{ArtistName: "A", Title: "B", ArtistMBID: "art-2"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + if got := tagsByName(tags)["folk"]; got != 0.6 { + t.Errorf("folk = %v, want 0.6", got) + } +} + +func TestMusicBrainzFetch_RecordingPreferredOverArtist(t *testing.T) { + // Recording has tags → they win at full weight; artist is never consulted. + srv := mbEntityServer(`{"tags":[{"count":3,"name":"shoegaze"}]}`, + `{"tags":[{"count":9,"name":"rock"}]}`) + defer srv.Close() + old := mbBaseURL + mbBaseURL = srv.URL + defer func() { mbBaseURL = old }() + + tags, err := newMBProvider(true).FetchTrackTags(context.Background(), + TrackRef{MBID: "rec-2", ArtistMBID: "art-3"}) + if err != nil { + t.Fatalf("fetch: %v", err) + } + m := tagsByName(tags) + if got := m["shoegaze"]; got != 1.0 { + t.Errorf("shoegaze = %v, want 1.0 (recording, full weight)", got) + } + if _, ok := m["rock"]; ok { + t.Error("artist tag 'rock' leaked in — recording tags should win outright") + } +} + func TestMusicBrainzFetch_ParsesTags(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = w.Write([]byte(`{"tags":[{"count":2,"name":"post-punk"},{"count":4,"name":"melancholic"}]}`))