From 5cbef438e4a6490c13fc7bd8f43d1e2cbf3ba6c5 Mon Sep 17 00:00:00 2001 From: rollingbase Date: Wed, 10 Jun 2026 15:01:31 +0200 Subject: [PATCH] Discogs: fix master/release ID collision fetching the wrong album MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discogs masters (/masters/{id}) and releases (/releases/{id}) share one numeric ID namespace, so release N and master N are different albums. search_albums() (type=release) yields RELEASE ids, but get_album, get_album_tracks and _fetch_and_cache_album all tried /masters first and only fell back to /releases. Whenever a release id collided with a real master id, the master lookup succeeded and returned a valid-but-wrong album — the fallback never fired. Tag the object type into the id string ('r12345' / 'm12345') at the single parse point (Album.from_discogs_release) and route each fetch to the matching endpoint. Legacy untagged ids are tried release-first (then master), which self-heals pre-existing bad matches without a migration. The enrichment worker now persists the tagged id too. Co-Authored-By: Claude Opus 4.8 (1M context) --- core/discogs_client.py | 81 +++++++++++--- core/discogs_worker.py | 6 +- tests/test_discogs_id_typing.py | 189 ++++++++++++++++++++++++++++++++ 3 files changed, 261 insertions(+), 15 deletions(-) create mode 100644 tests/test_discogs_id_typing.py diff --git a/core/discogs_client.py b/core/discogs_client.py index 445b7c68..54b95775 100644 --- a/core/discogs_client.py +++ b/core/discogs_client.py @@ -79,6 +79,51 @@ def _clean_discogs_artist_name(name: Optional[str]) -> str: return _DISCOGS_DISAMBIG_RE.sub('', name).strip() +# --- Discogs album ID typing ------------------------------------------------- +# Discogs has two album object types — masters (/masters/{id}) and releases +# (/releases/{id}) — whose numeric IDs share one space, so release N and master +# N are DIFFERENT albums. A bare numeric ID is therefore ambiguous. We tag the +# type into the ID string ('m12345' / 'r12345') at the point we parse it, so the +# correct endpoint can be chosen later without guessing. (Artist IDs are a single +# namespace and stay untagged.) + +def _discogs_album_kind(data: Dict[str, Any]) -> str: + """Classify a Discogs album payload as 'master' or 'release'. + + Search results and artist-discography items carry an explicit ``type``; + full detail responses don't, but only master detail has ``main_release``.""" + t = (data.get('type') or '').lower() + if t in ('master', 'release'): + return t + return 'master' if 'main_release' in data else 'release' + + +def _tag_discogs_album_id(raw_id: Any, kind: str) -> str: + """``'12345'`` + ``'master'`` -> ``'m12345'``; empty input -> ``''``.""" + s = str(raw_id or '').strip() + if not s: + return '' + return f"{'m' if kind == 'master' else 'r'}{s}" + + +def _discogs_album_endpoints(album_id: Any) -> List[str]: + """Map a (possibly tagged) album ID to the API path(s) to try, in order. + + ``'m12345'`` -> ``['/masters/12345']`` + ``'r12345'`` -> ``['/releases/12345']`` + ``'12345'`` (legacy untagged) -> ``['/releases/12345', '/masters/12345']`` + + Legacy bare IDs are tried release-first because stored IDs originate + overwhelmingly from search / manual-match / collection sync (all releases); + this also self-heals pre-fix bad matches. Returns ``[]`` for unusable input.""" + s = str(album_id or '').strip() + if len(s) > 1 and s[0] in ('m', 'r') and s[1:].isdigit(): + return [f"/{'masters' if s[0] == 'm' else 'releases'}/{s[1:]}"] + if s.isdigit(): + return [f'/releases/{s}', f'/masters/{s}'] + return [] + + # --- Shared dataclasses (same shape as iTunes/Deezer/Spotify) --- @dataclass @@ -304,7 +349,7 @@ class Album: external_urls['discogs_api'] = release_data['resource_url'] return cls( - id=str(release_data.get('id', '')), + id=_tag_discogs_album_id(release_data.get('id', ''), _discogs_album_kind(release_data)), name=title, artists=artists, release_date=release_date, @@ -643,10 +688,13 @@ class DiscogsClient: if cached and cached.get('title'): data = cached else: - # Try as master first (artist discography returns master IDs) - data = self._api_get(f'/masters/{release_id}') - if not data or not data.get('title'): - data = self._api_get(f'/releases/{release_id}') + # Hit the endpoint that matches the ID's type (tag-driven, no guessing). + data = None + for path in _discogs_album_endpoints(release_id): + data = self._api_get(path) + if data and data.get('title'): + break + data = None if not data: return None cache.store_entity('discogs', 'album', release_id, data) @@ -767,10 +815,13 @@ class DiscogsClient: if cached: return cached - # Try as master first (master IDs are used in artist discography) - data = self._api_get(f'/masters/{release_id}') - if not data or not data.get('tracklist'): - data = self._api_get(f'/releases/{release_id}') + # Hit the endpoint that matches the ID's type (tag-driven, no guessing). + data = None + for path in _discogs_album_endpoints(release_id): + data = self._api_get(path) + if data and data.get('tracklist'): + break + data = None if not data or not data.get('tracklist'): return None @@ -782,7 +833,7 @@ class DiscogsClient: image_url = (primary or images[0]).get('uri') album_info = { - 'id': str(data.get('id', release_id)), + 'id': str(release_id), 'name': data.get('title', ''), 'images': [{'url': image_url, 'height': 600, 'width': 600}] if image_url else [], 'release_date': str(data.get('year', '')) if data.get('year') else '', @@ -871,9 +922,13 @@ class DiscogsClient: cached = cache.get_entity('discogs', 'album', str(release_id)) if cached and cached.get('title'): return cached - data = self._api_get(f'/masters/{release_id}') - if not data or not data.get('title'): - data = self._api_get(f'/releases/{release_id}') + # Hit the endpoint that matches the ID's type (tag-driven, no guessing). + data = None + for path in _discogs_album_endpoints(release_id): + data = self._api_get(path) + if data and data.get('title'): + break + data = None if data: cache.store_entity('discogs', 'album', str(release_id), data) return data diff --git a/core/discogs_worker.py b/core/discogs_worker.py index 994ee259..12454c2f 100644 --- a/core/discogs_worker.py +++ b/core/discogs_worker.py @@ -17,7 +17,7 @@ from typing import Optional, Dict, Any from datetime import datetime, timedelta from utils.logging_config import get_logger from database.music_database import MusicDatabase -from core.discogs_client import DiscogsClient +from core.discogs_client import DiscogsClient, _discogs_album_kind, _tag_discogs_album_id from core.worker_utils import accept_artist_match, interruptible_sleep, set_album_api_track_count logger = get_logger("discogs_worker") @@ -436,7 +436,9 @@ class DiscogsWorker: conn = self.db._get_connection() cursor = conn.cursor() - discogs_id = str(data.get('id', '')) + # Tag the ID with its Discogs type so later re-fetches hit the right + # endpoint (master vs release share one numeric space). + discogs_id = _tag_discogs_album_id(data.get('id', ''), _discogs_album_kind(data)) genres = json.dumps(data.get('genres', [])) styles = json.dumps(data.get('styles', [])) labels = data.get('labels', []) diff --git a/tests/test_discogs_id_typing.py b/tests/test_discogs_id_typing.py new file mode 100644 index 00000000..f1ca71d4 --- /dev/null +++ b/tests/test_discogs_id_typing.py @@ -0,0 +1,189 @@ +"""Tests for Discogs master-vs-release ID disambiguation. + +Discogs has two album object types — masters (``/masters/{id}``) and releases +(``/releases/{id}``) — whose numeric IDs share ONE namespace, so release N and +master N are different albums. The old fetch code tried ``/masters/{id}`` first +and fell back to ``/releases/{id}``; because ``search_albums`` only ever yields +RELEASE ids, any release id that happened to collide with a real master id +returned a valid-but-WRONG album (the fallback never fired). See +``core.discogs_client`` ID-typing helpers. + +The fix tags the type into the id string ('r12345' / 'm12345') at parse time and +routes each fetch to the matching endpoint, with legacy bare ids tried +release-first (and master only as a fallback). +""" + +import pytest + +from core.discogs_client import ( + Album, + DiscogsClient, + _discogs_album_endpoints, + _discogs_album_kind, + _tag_discogs_album_id, +) + + +# --------------------------------------------------------------------------- +# _discogs_album_endpoints — the routing table +# --------------------------------------------------------------------------- + +def test_release_tagged_id_hits_only_releases(): + assert _discogs_album_endpoints('r12345') == ['/releases/12345'] + + +def test_master_tagged_id_hits_only_masters(): + assert _discogs_album_endpoints('m12345') == ['/masters/12345'] + + +def test_legacy_bare_id_tries_release_first_then_master(): + # The crux of the fix: bare ids are release-first, NOT master-first. + assert _discogs_album_endpoints('12345') == ['/releases/12345', '/masters/12345'] + + +def test_unusable_ids_return_empty(): + assert _discogs_album_endpoints('') == [] + assert _discogs_album_endpoints(None) == [] + assert _discogs_album_endpoints('not-an-id') == [] + # A lone letter prefix with no digits is not a valid tagged id. + assert _discogs_album_endpoints('r') == [] + + +# --------------------------------------------------------------------------- +# _discogs_album_kind / _tag_discogs_album_id — classification + tagging +# --------------------------------------------------------------------------- + +def test_kind_from_explicit_type(): + assert _discogs_album_kind({'type': 'master'}) == 'master' + assert _discogs_album_kind({'type': 'release'}) == 'release' + + +def test_kind_full_master_detail_has_main_release(): + # Full /masters/{id} responses carry no `type`, but do carry `main_release`. + assert _discogs_album_kind({'id': 1, 'main_release': 42, 'title': 'X'}) == 'master' + + +def test_kind_full_release_detail_defaults_release(): + # Full /releases/{id} responses carry no `type` and no `main_release`. + assert _discogs_album_kind({'id': 1, 'title': 'X', 'master_id': 42}) == 'release' + + +def test_tagging(): + assert _tag_discogs_album_id('123', 'master') == 'm123' + assert _tag_discogs_album_id('123', 'release') == 'r123' + assert _tag_discogs_album_id('', 'release') == '' + assert _tag_discogs_album_id(None, 'master') == '' + + +# --------------------------------------------------------------------------- +# Album.from_discogs_release — the single tagging point +# --------------------------------------------------------------------------- + +def test_search_result_tagged_as_release(): + # search_albums uses type=release; results carry type='release'. + album = Album.from_discogs_release({'id': 999, 'title': 'Radiohead - OK Computer', + 'type': 'release'}) + assert album.id == 'r999' + + +def test_discography_master_tagged_as_master(): + album = Album.from_discogs_release({'id': 777, 'title': 'OK Computer', 'type': 'master'}) + assert album.id == 'm777' + + +def test_full_master_detail_tagged_as_master(): + album = Album.from_discogs_release({'id': 777, 'title': 'OK Computer', 'main_release': 12}) + assert album.id == 'm777' + + +# --------------------------------------------------------------------------- +# Fetch routing — the regression lock on the original bug +# --------------------------------------------------------------------------- + +class _FakeCache: + """Always-miss metadata cache.""" + def get_entity(self, *a, **k): + return None + + def store_entity(self, *a, **k): + pass + + def get_search_results(self, *a, **k): + return None + + def store_search_results(self, *a, **k): + pass + + def store_entities_bulk(self, *a, **k): + pass + + +@pytest.fixture +def client(monkeypatch): + monkeypatch.setattr('core.discogs_client.get_metadata_cache', lambda: _FakeCache()) + return DiscogsClient(token='test-token') + + +def _record(client, monkeypatch, responses): + """Replace _api_get with a recorder that returns `responses[path]`.""" + calls = [] + + def fake_api_get(endpoint, params=None): + calls.append(endpoint) + return responses.get(endpoint) + + monkeypatch.setattr(client, '_api_get', fake_api_get) + return calls + + +def test_get_album_release_id_never_hits_masters(client, monkeypatch): + """A release-tagged id must NOT touch /masters — that was the bug.""" + calls = _record(client, monkeypatch, { + '/releases/249504': {'id': 249504, 'title': 'The Real Album', 'artists': [{'name': 'A'}]}, + '/masters/249504': {'id': 249504, 'title': 'A DIFFERENT Album', 'artists': [{'name': 'B'}]}, + }) + result = client.get_album('r249504', include_tracks=False) + assert calls == ['/releases/249504'] # master endpoint never consulted + assert result['name'] == 'The Real Album' + + +def test_get_album_master_id_hits_masters_only(client, monkeypatch): + calls = _record(client, monkeypatch, { + '/masters/777': {'id': 777, 'title': 'Master Album', 'main_release': 1}, + }) + result = client.get_album('m777', include_tracks=False) + assert calls == ['/masters/777'] + assert result['name'] == 'Master Album' + + +def test_legacy_bare_id_release_first(client, monkeypatch): + """Legacy untagged id resolves as a release without ever hitting /masters + when the release lookup succeeds.""" + calls = _record(client, monkeypatch, { + '/releases/249504': {'id': 249504, 'title': 'The Real Album'}, + '/masters/249504': {'id': 249504, 'title': 'A DIFFERENT Album'}, + }) + result = client.get_album('249504', include_tracks=False) + assert calls == ['/releases/249504'] + assert result['name'] == 'The Real Album' + + +def test_legacy_bare_id_falls_back_to_master(client, monkeypatch): + """If the release lookup yields nothing, the bare id still tries master.""" + calls = _record(client, monkeypatch, { + '/releases/777': None, + '/masters/777': {'id': 777, 'title': 'Master Only', 'main_release': 1}, + }) + result = client.get_album('777', include_tracks=False) + assert calls == ['/releases/777', '/masters/777'] + assert result['name'] == 'Master Only' + + +def test_fetch_and_cache_release_id_never_hits_masters(client, monkeypatch): + calls = _record(client, monkeypatch, { + '/releases/249504': {'id': 249504, 'title': 'The Real Album'}, + '/masters/249504': {'id': 249504, 'title': 'A DIFFERENT Album'}, + }) + data = client._fetch_and_cache_album('r249504') + assert calls == ['/releases/249504'] + assert data['title'] == 'The Real Album'