feat(tags): MusicBrainz artist-MBID tag fallback (#1519)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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"}]}`))
|
||||
|
||||
Reference in New Issue
Block a user