Merge pull request #848 from RollingBase/fix/discogs-master-release-id-collision
Discogs: fix master/release ID collision fetching the wrong album
This commit is contained in:
BoulderBadgeDad
GitHub
commit
f557d3b203
3 changed files with 261 additions and 15 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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', [])
|
||||
|
|
|
|||
189
tests/test_discogs_id_typing.py
Normal file
189
tests/test_discogs_id_typing.py
Normal file
|
|
@ -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'
|
||||
Loading…
Reference in a new issue