MusicBrainz: Resolve release-group MBIDs to a release on album click
Clicking a MusicBrainz album returned 404 because the browse-based
search path now stores release-GROUP MBIDs in Album.id, but `get_album`
still hit `/ws/2/release/<mbid>` directly. Release-group MBIDs don't
resolve as release MBIDs — MB 404s. User log:
GET /api/spotify/album/b88655ba...?source=musicbrainz → 404
Error fetching release b88655ba...: 404 Client Error
The fix requires a two-step resolution for the new browse path:
1. Look up the release-group with `inc=releases+artist-credits` to get
the list of releases inside (original + reissues + regional + promo
editions). MB release-groups routinely hold 5-20 releases.
2. Pick a representative release: prefer Official status over Promo,
prefer releases with a real tracklist over stubs, then earliest date.
3. Fetch that release's full tracklist via `get_release`.
Two extra seconds at the 1-rps rate limit, but it's on click, not on
search results rendering.
Structure:
- New `MusicBrainzClient.get_release_group(mbid, includes)` method.
- New `_pick_representative_release(releases)` helper encapsulates the
ranking logic.
- Tracklist projection extracted into `_render_release_as_album` so
both paths share the same shape construction.
- `get_album` tries release-group first; falls back to direct release
lookup when the MBID turns out to be a release from the text-search
fallback path.
- Canonical Album.id stays the release-group MBID so a re-fetch with
the same URL hits the same code path idempotently.
3 new tests (now 33 total):
- End-to-end release-group → release resolution with mocked client
- Fallback to direct release lookup when rg lookup misses
- Representative-release picker ranks correctly
Verified against live API with the exact MBID that 404'd for the user
(b88655ba... for DAMN. by Kendrick Lamar): now returns in 1.2s with
the full 14-track listing (BLOOD., DNA., YAH., ELEMENT., FEEL., ...).
This commit is contained in:
parent
a6359a2690
commit
7dfe1ae88d
3 changed files with 257 additions and 61 deletions
|
|
@ -369,6 +369,38 @@ class MusicBrainzClient:
|
|||
logger.error(f"Error fetching release {mbid}: {e}")
|
||||
return None
|
||||
|
||||
@rate_limited
|
||||
def get_release_group(self, mbid: str, includes: Optional[List[str]] = None) -> Optional[Dict[str, Any]]:
|
||||
"""Get full release-group details by MBID.
|
||||
|
||||
Release-groups are the 'canonical album' entity in MusicBrainz —
|
||||
they group every edition/reissue/region-specific release of the
|
||||
same logical album under one MBID. Use `inc=releases` to list the
|
||||
individual releases this group contains (each with its own
|
||||
tracklist); use `inc=artist-credits` for artist info.
|
||||
|
||||
Args:
|
||||
mbid: Release-group's MusicBrainz ID
|
||||
includes: Optional list, e.g. ['releases', 'artist-credits']
|
||||
|
||||
Returns:
|
||||
Release-group data or None if not found.
|
||||
"""
|
||||
try:
|
||||
params = {'fmt': 'json'}
|
||||
if includes:
|
||||
params['inc'] = '+'.join(includes)
|
||||
response = self.session.get(
|
||||
f"{self.BASE_URL}/release-group/{mbid}",
|
||||
params=params,
|
||||
timeout=10
|
||||
)
|
||||
response.raise_for_status()
|
||||
return response.json()
|
||||
except Exception as e:
|
||||
logger.error(f"Error fetching release-group {mbid}: {e}")
|
||||
return None
|
||||
|
||||
@rate_limited
|
||||
def get_recording(self, mbid: str, includes: Optional[List[str]] = None) -> Optional[Dict[str, Any]]:
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -574,70 +574,139 @@ class MusicBrainzSearchClient:
|
|||
logger.warning(f"MusicBrainz track search failed: {e}")
|
||||
return []
|
||||
|
||||
def get_album(self, release_mbid: str) -> Optional[Dict[str, Any]]:
|
||||
"""Get full album details with track listing for download modal."""
|
||||
try:
|
||||
release = self._client.get_release(release_mbid, includes=['recordings', 'artist-credits', 'release-groups'])
|
||||
if not release:
|
||||
return None
|
||||
def _pick_representative_release(self, releases: List[Dict[str, Any]]) -> Optional[Dict[str, Any]]:
|
||||
"""Pick the best release out of a release-group's editions.
|
||||
|
||||
title = release.get('title', '')
|
||||
artists_raw = _extract_artist_credit(release.get('artist-credit', []))
|
||||
release_date = release.get('date', '') or ''
|
||||
|
||||
rg = release.get('release-group', {})
|
||||
primary_type = rg.get('primary-type', '') or ''
|
||||
secondary_types = rg.get('secondary-types', []) or []
|
||||
album_type = _map_release_type(primary_type, secondary_types)
|
||||
|
||||
# Cover art
|
||||
rg_mbid = rg.get('id', '')
|
||||
image_url = self._cached_art(release_mbid, rg_mbid)
|
||||
|
||||
# Build tracks from media
|
||||
tracks = []
|
||||
total_tracks = 0
|
||||
media_list = release.get('media', [])
|
||||
for media_idx, media in enumerate(media_list):
|
||||
disc_number = media.get('position', media_idx + 1)
|
||||
for track in media.get('tracks', []):
|
||||
total_tracks += 1
|
||||
recording = track.get('recording', {})
|
||||
track_artists = _extract_artist_credit(recording.get('artist-credit', []))
|
||||
if not track_artists:
|
||||
track_artists = artists_raw
|
||||
|
||||
try:
|
||||
track_num = int(track.get('number', track.get('position', total_tracks)))
|
||||
except (ValueError, TypeError):
|
||||
track_num = total_tracks
|
||||
|
||||
tracks.append({
|
||||
'id': recording.get('id', track.get('id', '')),
|
||||
'name': recording.get('title', track.get('title', '')),
|
||||
'artists': [{'name': a} for a in track_artists],
|
||||
'duration_ms': recording.get('length', 0) or track.get('length', 0) or 0,
|
||||
'track_number': track_num,
|
||||
'disc_number': disc_number,
|
||||
})
|
||||
|
||||
images = [{'url': image_url, 'height': 250, 'width': 250}] if image_url else []
|
||||
|
||||
return {
|
||||
'id': release_mbid,
|
||||
'name': title,
|
||||
'artists': [{'name': a, 'id': ''} for a in (artists_raw or ['Unknown Artist'])],
|
||||
'release_date': release_date,
|
||||
'total_tracks': total_tracks,
|
||||
'album_type': album_type,
|
||||
'images': images,
|
||||
'tracks': tracks,
|
||||
'external_urls': {'musicbrainz': f'https://musicbrainz.org/release/{release_mbid}'},
|
||||
}
|
||||
except Exception as e:
|
||||
logger.error(f"MusicBrainz album detail failed for {release_mbid}: {e}")
|
||||
Release-groups often contain 5-20+ releases (original, reissues,
|
||||
remasters, regional editions, bonus-track editions). We want a
|
||||
single canonical version to show the user as 'the album.' Prefer:
|
||||
1. Official releases (not promo/bootleg)
|
||||
2. Earliest date (the original)
|
||||
3. Any release with media (skip entries that are just stubs)
|
||||
"""
|
||||
if not releases:
|
||||
return None
|
||||
|
||||
def _key(r):
|
||||
status = (r.get('status') or '').lower()
|
||||
status_rank = 0 if status == 'official' else 1 # Official first
|
||||
has_media = 0 if r.get('media') else 1 # Real tracklists first
|
||||
date = (r.get('date') or '9999-99-99')[:10]
|
||||
return (has_media, status_rank, date)
|
||||
|
||||
return sorted(releases, key=_key)[0]
|
||||
|
||||
def get_album(self, album_mbid: str) -> Optional[Dict[str, Any]]:
|
||||
"""Get full album details with track listing for download modal.
|
||||
|
||||
The MBID passed in could be either:
|
||||
- A release-group MBID (from `search_albums` browse path — the
|
||||
common case now that bare-name searches route artist-first →
|
||||
browse), or
|
||||
- A release MBID (from the text-search fallback path).
|
||||
|
||||
Try release-group first since that's the majority; if it 404s,
|
||||
fall back to direct release lookup. Release-group resolution adds
|
||||
one extra API call (~1s at the 1-rps rate limit) to pick a
|
||||
representative release and then fetch its tracklist.
|
||||
"""
|
||||
try:
|
||||
# Path A: release-group MBID (new browse-based search default)
|
||||
rg = self._client.get_release_group(
|
||||
album_mbid, includes=['releases', 'artist-credits']
|
||||
)
|
||||
if rg:
|
||||
releases = rg.get('releases') or []
|
||||
rep = self._pick_representative_release(releases)
|
||||
if rep and rep.get('id'):
|
||||
album = self._render_release_as_album(
|
||||
rep['id'],
|
||||
rg_fallback=rg,
|
||||
)
|
||||
if album:
|
||||
# Keep the release-group MBID as the canonical
|
||||
# Album.id so downstream code can re-fetch with
|
||||
# the same URL.
|
||||
album['id'] = album_mbid
|
||||
album['external_urls'] = {
|
||||
'musicbrainz': f'https://musicbrainz.org/release-group/{album_mbid}'
|
||||
}
|
||||
return album
|
||||
|
||||
# Path B: release MBID (text-search fallback path)
|
||||
return self._render_release_as_album(album_mbid)
|
||||
except Exception as e:
|
||||
logger.error(f"MusicBrainz album detail failed for {album_mbid}: {e}")
|
||||
return None
|
||||
|
||||
def _render_release_as_album(self, release_mbid: str,
|
||||
rg_fallback: Optional[Dict[str, Any]] = None) -> Optional[Dict[str, Any]]:
|
||||
"""Fetch a specific release and project it to the album-detail dict
|
||||
shape the download modal expects. `rg_fallback` supplies release-group
|
||||
metadata (type, artist credits) when resolving from a release-group
|
||||
whose releases may be lightly populated."""
|
||||
release = self._client.get_release(
|
||||
release_mbid, includes=['recordings', 'artist-credits', 'release-groups']
|
||||
)
|
||||
if not release:
|
||||
return None
|
||||
|
||||
title = release.get('title', '')
|
||||
artists_raw = _extract_artist_credit(release.get('artist-credit', []))
|
||||
if not artists_raw and rg_fallback:
|
||||
artists_raw = _extract_artist_credit(rg_fallback.get('artist-credit', []))
|
||||
release_date = release.get('date', '') or ''
|
||||
if not release_date and rg_fallback:
|
||||
release_date = rg_fallback.get('first-release-date', '') or ''
|
||||
|
||||
rg = release.get('release-group', rg_fallback or {}) or {}
|
||||
primary_type = rg.get('primary-type', '') or ''
|
||||
secondary_types = rg.get('secondary-types', []) or []
|
||||
album_type = _map_release_type(primary_type, secondary_types)
|
||||
|
||||
rg_mbid = rg.get('id', '')
|
||||
image_url = self._cached_art(release_mbid, rg_mbid)
|
||||
|
||||
tracks = []
|
||||
total_tracks = 0
|
||||
media_list = release.get('media', [])
|
||||
for media_idx, media in enumerate(media_list):
|
||||
disc_number = media.get('position', media_idx + 1)
|
||||
for track in media.get('tracks', []):
|
||||
total_tracks += 1
|
||||
recording = track.get('recording', {})
|
||||
track_artists = _extract_artist_credit(recording.get('artist-credit', []))
|
||||
if not track_artists:
|
||||
track_artists = artists_raw
|
||||
|
||||
try:
|
||||
track_num = int(track.get('number', track.get('position', total_tracks)))
|
||||
except (ValueError, TypeError):
|
||||
track_num = total_tracks
|
||||
|
||||
tracks.append({
|
||||
'id': recording.get('id', track.get('id', '')),
|
||||
'name': recording.get('title', track.get('title', '')),
|
||||
'artists': [{'name': a} for a in track_artists],
|
||||
'duration_ms': recording.get('length', 0) or track.get('length', 0) or 0,
|
||||
'track_number': track_num,
|
||||
'disc_number': disc_number,
|
||||
})
|
||||
|
||||
images = [{'url': image_url, 'height': 250, 'width': 250}] if image_url else []
|
||||
|
||||
return {
|
||||
'id': release_mbid,
|
||||
'name': title,
|
||||
'artists': [{'name': a, 'id': ''} for a in (artists_raw or ['Unknown Artist'])],
|
||||
'release_date': release_date,
|
||||
'total_tracks': total_tracks,
|
||||
'album_type': album_type,
|
||||
'images': images,
|
||||
'tracks': tracks,
|
||||
'external_urls': {'musicbrainz': f'https://musicbrainz.org/release/{release_mbid}'},
|
||||
}
|
||||
|
||||
def get_artist_albums(self, artist_mbid: str, album_type: str = 'album,single') -> List:
|
||||
"""Get artist's releases for discography view."""
|
||||
try:
|
||||
|
|
|
|||
|
|
@ -440,6 +440,101 @@ def test_search_tracks_structured_query_uses_text_path():
|
|||
assert len(tracks) == 1
|
||||
|
||||
|
||||
def test_get_album_resolves_release_group_mbid_to_release():
|
||||
"""When the album ID is a release-group MBID (from the browse path),
|
||||
get_album must look up the release-group, pick a canonical release,
|
||||
and fetch that release's tracklist. Fetching /release/<rg-mbid>
|
||||
directly 404s."""
|
||||
client = MusicBrainzSearchClient()
|
||||
client._client = MagicMock()
|
||||
# Release-group lookup returns two editions — an Official release and
|
||||
# a promo. The Official earlier release should win.
|
||||
client._client.get_release_group.return_value = {
|
||||
'id': 'rg-damn',
|
||||
'title': 'DAMN.',
|
||||
'primary-type': 'Album',
|
||||
'secondary-types': [],
|
||||
'first-release-date': '2017-04-14',
|
||||
'artist-credit': [{'name': 'Kendrick Lamar'}],
|
||||
'releases': [
|
||||
{'id': 'rel-promo', 'status': 'Promotion', 'date': '2017-04-01',
|
||||
'media': [{'track-count': 14, 'tracks': []}]},
|
||||
{'id': 'rel-official', 'status': 'Official', 'date': '2017-04-14',
|
||||
'media': [{'track-count': 14, 'tracks': []}]},
|
||||
],
|
||||
}
|
||||
# Release lookup returns a full release with tracklist.
|
||||
client._client.get_release.return_value = {
|
||||
'id': 'rel-official',
|
||||
'title': 'DAMN.',
|
||||
'date': '2017-04-14',
|
||||
'artist-credit': [{'name': 'Kendrick Lamar'}],
|
||||
'release-group': {'id': 'rg-damn', 'primary-type': 'Album', 'secondary-types': []},
|
||||
'media': [
|
||||
{'position': 1, 'tracks': [
|
||||
{'id': 't1', 'number': '1', 'position': 1, 'length': 50000,
|
||||
'recording': {'id': 'rec-1', 'title': 'BLOOD.',
|
||||
'artist-credit': [{'name': 'Kendrick Lamar'}], 'length': 50000}},
|
||||
]},
|
||||
],
|
||||
}
|
||||
|
||||
album = client.get_album('rg-damn')
|
||||
|
||||
# Must have called release-group first, then release for the picked edition.
|
||||
client._client.get_release_group.assert_called_once_with(
|
||||
'rg-damn', includes=['releases', 'artist-credits']
|
||||
)
|
||||
client._client.get_release.assert_called_once_with(
|
||||
'rel-official', includes=['recordings', 'artist-credits', 'release-groups']
|
||||
)
|
||||
assert album is not None
|
||||
assert album['id'] == 'rg-damn' # Canonical ID stays the release-group MBID.
|
||||
assert album['name'] == 'DAMN.'
|
||||
assert len(album['tracks']) == 1
|
||||
assert album['tracks'][0]['name'] == 'BLOOD.'
|
||||
assert 'release-group' in album['external_urls']['musicbrainz']
|
||||
|
||||
|
||||
def test_get_album_falls_back_to_release_lookup_on_rg_miss():
|
||||
"""When the MBID is a release (from the text-search fallback path) the
|
||||
release-group lookup 404s, but the direct release lookup works."""
|
||||
client = MusicBrainzSearchClient()
|
||||
client._client = MagicMock()
|
||||
# Release-group lookup returns None (simulating 404).
|
||||
client._client.get_release_group.return_value = None
|
||||
client._client.get_release.return_value = {
|
||||
'id': 'rel-abc',
|
||||
'title': 'Some Album',
|
||||
'date': '2020-01-01',
|
||||
'artist-credit': [{'name': 'Some Artist'}],
|
||||
'release-group': {'id': 'rg-abc', 'primary-type': 'Album', 'secondary-types': []},
|
||||
'media': [{'position': 1, 'tracks': []}],
|
||||
}
|
||||
|
||||
album = client.get_album('rel-abc')
|
||||
|
||||
client._client.get_release_group.assert_called_once()
|
||||
client._client.get_release.assert_called_once()
|
||||
assert album is not None
|
||||
assert album['id'] == 'rel-abc' # Falls back to release MBID since rg lookup missed.
|
||||
|
||||
|
||||
def test_pick_representative_release_prefers_official_with_media():
|
||||
"""The release picker should skip stub releases (no media) and pick
|
||||
Official over Promotion status."""
|
||||
client = MusicBrainzSearchClient()
|
||||
releases = [
|
||||
{'id': 'stub', 'status': 'Official', 'date': '2020-01-01'}, # No media
|
||||
{'id': 'promo', 'status': 'Promotion', 'date': '2019-12-01',
|
||||
'media': [{'track-count': 10}]},
|
||||
{'id': 'official', 'status': 'Official', 'date': '2020-01-05',
|
||||
'media': [{'track-count': 10}]},
|
||||
]
|
||||
picked = client._pick_representative_release(releases)
|
||||
assert picked['id'] == 'official'
|
||||
|
||||
|
||||
def test_search_tracks_text_path_filters_by_score():
|
||||
client = MusicBrainzSearchClient()
|
||||
client._client = MagicMock()
|
||||
|
|
|
|||
Loading…
Reference in a new issue