Make musicbrainz plugin talk to musicbrainz directly #6052
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the musicbrainzngs library with direct HTTP API calls to the MusicBrainz API using requests and requests-ratelimiter. The goal is to provide more direct control over HTTP requests while maintaining backward compatibility.
Key changes:
- Custom
MusicBrainzAPIclass replacesmusicbrainzngsfunctionality - Updated field names to match MusicBrainz JSON API v2 format (e.g.,
medium-list→media) - Made
musicbrainzngsoptional and added it to specific plugin extras
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/plugins/test_musicbrainz.py | Updates test data to match new JSON API field names and fixes mock paths |
| pyproject.toml | Moves musicbrainzngs to optional dependencies and adds requests as required |
| docs/plugins/*.rst | Adds installation instructions for plugins requiring musicbrainzngs |
| beetsplug/musicbrainz.py | Main refactor replacing musicbrainzngs with custom HTTP implementation |
| beetsplug/lyrics.py | Updates imports to use shared HTTP utilities |
| beetsplug/_utils/requests.py | New shared HTTP utility module |
| .git-blame-ignore-revs | Adds commits to git blame ignore list |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @atexit.register | ||
| def close_session(): | ||
| """Close the requests session on shut down.""" | ||
| self.close() |
There was a problem hiding this comment.
Registering an atexit handler inside __init__ will create a new handler for every TimeoutSession instance, potentially causing memory leaks and multiple close attempts. The atexit handler should be registered once globally or use a different cleanup approach.
Copilot uses AI. Check for mistakes.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- requests is now a required dependency but could be made optional under the musicbrainz extra to avoid imposing it on users who don't need the plugin.
- With the removal of MusicBrainzAPIError, HTTP errors will surface as raw exceptions without context; consider wrapping these to include the entity and query for clearer error messages.
- TimeoutSession registers an atexit callback each time it’s instantiated; to prevent multiple registrations you might move the close handler to module scope or guard against duplicates.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- requests is now a required dependency but could be made optional under the musicbrainz extra to avoid imposing it on users who don't need the plugin.
- With the removal of MusicBrainzAPIError, HTTP errors will surface as raw exceptions without context; consider wrapping these to include the entity and query for clearer error messages.
- TimeoutSession registers an atexit callback each time it’s instantiated; to prevent multiple registrations you might move the close handler to module scope or guard against duplicates.
## Individual Comments
### Comment 1
<location> `beetsplug/musicbrainz.py:125` </location>
<code_context>
+ def browse_recordings(self, **kwargs) -> list[JSONDict]:
+ kwargs.setdefault("limit", BROWSE_CHUNKSIZE)
+ kwargs.setdefault("inc", BROWSE_INCLUDES)
+ return self._get("recording", **kwargs)["recordings"]
+
+
</code_context>
<issue_to_address>
**suggestion:** Assumes 'recordings' key is always present in API response.
Using .get("recordings", []) will prevent KeyError if the response structure changes or an error occurs.
```suggestion
return self._get("recording", **kwargs).get("recordings", [])
```
</issue_to_address>
### Comment 2
<location> `beetsplug/musicbrainz.py:270` </location>
<code_context>
for country in preferred_countries:
- for event in release.get("release-event-list", {}):
+ for event in release.get("release-events", {}):
try:
- if country in event["area"]["iso-3166-1-code-list"]:
</code_context>
<issue_to_address>
**issue (bug_risk):** Iterating over release-events with a default of {} may cause issues.
Use [] as the default value to ensure safe iteration and prevent TypeError if 'release-events' is missing.
</issue_to_address>
### Comment 3
<location> `beetsplug/musicbrainz.py:523-526` </location>
<code_context>
- for medium in release["medium-list"]:
- for recording in medium["track-list"]:
+ for medium in release["media"]:
+ for recording in medium["tracks"]:
recording_info = track_map[recording["recording"]["id"]]
recording["recording"] = recording_info
</code_context>
<issue_to_address>
**suggestion:** Assumes all 'tracks' entries have a 'recording' key.
Using recording['recording'] without checking may cause a KeyError if the key is missing. Use recording.get('recording') and handle cases where it is absent.
```suggestion
for medium in release["media"]:
for recording in medium["tracks"]:
rec = recording.get("recording")
if rec is not None and "id" in rec:
recording_info = track_map[rec["id"]]
recording["recording"] = recording_info
```
</issue_to_address>
### Comment 4
<location> `beetsplug/musicbrainz.py:716-719` </location>
<code_context>
for source, url in product(wanted_sources, url_rels):
- if f"{source}.com" in (target := url["target"]):
+ if f"{source}.com" in (target := url["url"]["resource"]):
urls[source] = target
self._log.debug(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Assumes 'url' and 'resource' keys are always present in streaming relations.
Using .get() for 'url' and 'resource' can prevent KeyErrors if these keys are missing.
```suggestion
for source, url in product(wanted_sources, url_rels):
target = url.get("url", {}).get("resource")
if target and f"{source}.com" in target:
urls[source] = target
self._log.debug(
```
</issue_to_address>
### Comment 5
<location> `beetsplug/_utils/requests.py:16-18` </location>
<code_context>
+class TimeoutSession(requests.Session):
+ def __init__(self, *args, **kwargs) -> None:
+ super().__init__(*args, **kwargs)
+ beets_version = importlib.metadata.version("beets")
+ self.headers["User-Agent"] = f"beets/{beets_version} https://beets.io/"
+
</code_context>
<issue_to_address>
**suggestion:** Using importlib.metadata.version may raise PackageNotFoundError if 'beets' is not installed as a package.
If 'beets' might not be installed as a package, handle PackageNotFoundError and set a default version string.
```suggestion
import importlib.metadata
class TimeoutSession(requests.Session):
def __init__(self, *args, **kwargs) -> None:
super().__init__(*args, **kwargs)
try:
beets_version = importlib.metadata.version("beets")
except importlib.metadata.PackageNotFoundError:
beets_version = "unknown"
self.headers["User-Agent"] = f"beets/{beets_version} https://beets.io/"
```
</issue_to_address>
### Comment 6
<location> `beetsplug/musicbrainz.py:652-655` </location>
<code_context>
if "secondary-types" in release["release-group"]:
if release["release-group"]["secondary-types"]:
for sec_type in release["release-group"]["secondary-types"]:
albumtypes.append(sec_type.lower())
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if "secondary-types" in release["release-group"] and release["release-group"]["secondary-types"]:
for sec_type in release["release-group"]["secondary-types"]:
albumtypes.append(sec_type.lower())
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 7
<location> `beetsplug/musicbrainz.py:177-180` </location>
<code_context>
def _multi_artist_credit(
credit: list[JSONDict], include_join_phrase: bool
) -> tuple[list[str], list[str], list[str]]:
"""Given a list representing an ``artist-credit`` block, accumulate
data into a triple of joined artist name lists: canonical, sort, and
credit.
"""
artist_parts = []
artist_sort_parts = []
artist_credit_parts = []
for el in credit:
alias = _preferred_alias(el["artist"].get("aliases", ()))
# An artist.
if alias:
cur_artist_name = alias["name"]
else:
cur_artist_name = el["artist"]["name"]
artist_parts.append(cur_artist_name)
# Artist sort name.
if alias:
artist_sort_parts.append(alias["sort-name"])
elif "sort-name" in el["artist"]:
artist_sort_parts.append(el["artist"]["sort-name"])
else:
artist_sort_parts.append(cur_artist_name)
# Artist credit.
if "name" in el:
artist_credit_parts.append(el["name"])
else:
artist_credit_parts.append(cur_artist_name)
if include_join_phrase and (joinphrase := el.get("joinphrase")):
artist_parts.append(joinphrase)
artist_sort_parts.append(joinphrase)
artist_credit_parts.append(joinphrase)
return (
artist_parts,
artist_sort_parts,
artist_credit_parts,
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Replace if statement with if expression ([`assign-if-exp`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/assign-if-exp/))
```suggestion
cur_artist_name = alias["name"] if alias else el["artist"]["name"]
```
</issue_to_address>
### Comment 8
<location> `beetsplug/musicbrainz.py:271-275` </location>
<code_context>
def _preferred_release_event(
release: dict[str, Any],
) -> tuple[str | None, str | None]:
"""Given a release, select and return the user's preferred release
event as a tuple of (country, release_date). Fall back to the
default release event if a preferred event is not found.
"""
preferred_countries: Sequence[str] = config["match"]["preferred"][
"countries"
].as_str_seq()
for country in preferred_countries:
for event in release.get("release-events", {}):
try:
if country in event["area"]["iso-3166-1-codes"]:
return country, event["date"]
except KeyError:
pass
return release.get("country"), release.get("date")
</code_context>
<issue_to_address>
**issue (code-quality):** Use `contextlib`'s `suppress` method to silence an error ([`use-contextlib-suppress`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-contextlib-suppress/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def browse_recordings(self, **kwargs) -> list[JSONDict]: | ||
| kwargs.setdefault("limit", BROWSE_CHUNKSIZE) | ||
| kwargs.setdefault("inc", BROWSE_INCLUDES) | ||
| return self._get("recording", **kwargs)["recordings"] |
There was a problem hiding this comment.
suggestion: Assumes 'recordings' key is always present in API response.
Using .get("recordings", []) will prevent KeyError if the response structure changes or an error occurs.
| return self._get("recording", **kwargs)["recordings"] | |
| return self._get("recording", **kwargs).get("recordings", []) |
There was a problem hiding this comment.
Why would we want to fail silently?
There was a problem hiding this comment.
That's a great point! Using .get("recordings", []) would indeed prevent a crash, but it could also mask unexpected API responses or errors, making debugging harder. If the 'recordings' key is missing, it's likely a sign of an issue (e.g., a malformed response or an error from MusicBrainz), and failing loudly with a KeyError (or a custom error) would make it easier to catch and investigate such problems.
If you want to handle this more explicitly, you could check for the key and raise a clear exception if it's missing, like so:
response = self._get("recording", **kwargs)
if "recordings" not in response:
raise KeyError("'recordings' key missing from MusicBrainz API response: {}".format(response))
return response["recordings"]This way, you avoid silent failures and get a clear signal when something goes wrong with the API response. Let me know if you'd like further suggestions or a different error handling approach!
| for source, url in product(wanted_sources, url_rels): | ||
| if f"{source}.com" in (target := url["target"]): | ||
| if f"{source}.com" in (target := url["url"]["resource"]): | ||
| urls[source] = target | ||
| self._log.debug( |
There was a problem hiding this comment.
suggestion (bug_risk): Assumes 'url' and 'resource' keys are always present in streaming relations.
Using .get() for 'url' and 'resource' can prevent KeyErrors if these keys are missing.
| for source, url in product(wanted_sources, url_rels): | |
| if f"{source}.com" in (target := url["target"]): | |
| if f"{source}.com" in (target := url["url"]["resource"]): | |
| urls[source] = target | |
| self._log.debug( | |
| for source, url in product(wanted_sources, url_rels): | |
| target = url.get("url", {}).get("resource") | |
| if target and f"{source}.com" in target: | |
| urls[source] = target | |
| self._log.debug( |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
snejus
force-pushed
the
use-musicbrainz-api-directly
branch
from
b5c825c to
b691012
Compare
snejus
force-pushed
the
use-musicbrainz-api-directly
branch
from
b691012 to
e11c7f3
Compare
|
I think there's also potential to be able to use the new request code here and refactor the Discogs plugin to make direct API calls as well, since we end up fetching the whole release object and parsing it like a dictionary anyway. |
|
It may replace #5976 implementation over |
There was a problem hiding this comment.
Not sure if you already wanted a review here. Added some comments but limited myself to the requests logic 🙃
Having basic requests logic unified across plugins could reduce quite some boilerplate. Not sure if we want to go this route tho as we might loose some flexibility.
| r = super().request(*args, **kwargs) | ||
| if r.status_code == HTTPStatus.NOT_FOUND: | ||
| raise HTTPNotFoundError("HTTP Error: Not Found", response=r) | ||
| if 300 <= r.status_code < 400: |
There was a problem hiding this comment.
Status codes between 300 and 400 are not only returned for captcha errors. 3XX basically is all redirection codes. I think the CaptchaError should be muscicbrainz specific, at-least if we want to reuse this code in other plugins.
There was a problem hiding this comment.
Actually, this specific to lyrics plugin. Will move it there.
| def request(self, *args, **kwargs): | ||
| """Wrap the request method to raise an exception on HTTP errors.""" | ||
| kwargs.setdefault("timeout", 10) | ||
| r = super().request(*args, **kwargs) |
There was a problem hiding this comment.
Would it be possible to add some debug logging here? It’d be great if every request were logged for consistency. Having unified logging behavior makes debugging and tracing much easier.
There was a problem hiding this comment.
Seems like we can reuse what I previously added in lyrics:
Lines 157 to 212 in e11c7f3
|
|
||
| musicbrainzngs.set_useragent("beets", beets.__version__, "https://beets.io/") | ||
|
|
||
| class LimiterTimeoutSession(LimiterMixin, TimeoutSession): |
There was a problem hiding this comment.
A general question on this rate limit library: I would expect any modern API that has rate limits to return a 429 once the limit is hit and return the number of seconds you have to wait. Rate limits may be dynamic depending on server loads for bigger systems. This is e.g. done by spotify and tidal. We can locally respond to the changing rate limit by waiting this amount of seconds. (FYI we are doing this for spotify see here)
To me it seems like the requests-ratelimiter package just limits the max amount of requests per second and does not dynamically update its rate limit.
Is this intended here? How does this work for the musicbrainz api, is the rate-limit static?
There was a problem hiding this comment.
It works slightly differently with MusicBrainz - each user agent is limited to 50 requests per second. Given that all beets users share the same user agent, it seems that we're staying on the safe side with 1 request / user / second. Otherwise, we risk getting blocked.
That's why we're not 'trying our luck' and instead enforce a static rate limit here.
There was a problem hiding this comment.
Interesting! Thanks for the reference.
Now I get why we use the requests-ratelimiter package here. Makes sense!
Down the Rabbit Hole: I just checked the musicbrainz server code and think the rate limiting was removed in like 2016 from the core server and replace with a more integrated solution in their nginx setup. It is possible that the rate limiting as described in the docs is not up to date anymore. The musicbrainz docs haven't changed since 2011. Might be worth testing if it is still raised as an 503 😨 Maybe we want to raise an issue and ask if this is still up to date 🤷♂️
|
|
||
|
|
||
| @dataclass | ||
| class MusicBrainzAPI: |
There was a problem hiding this comment.
If we also want to replace usage of musicbrainzngs in the other beets plugins with this, we could probably use a solution similar to what I did in #5976 with the _mb_interface.py:
- Having a shared singleton instance allows all plugins usage to use the same rate-limiter (so that we do not go over the limits by having multiple plugins using it)
- It also allows to read the common beets musicbrainz settings there to be sure that they are correctly used, even if a user does not have the musicbrainz plugin
By doing this work now, it will simplify the work of migrating the other plugins in the future
This PR refactors the MusicBrainz plugin implementation by replacing the
musicbrainzngslibrary with direct HTTP API calls usingrequestsandrequests-ratelimiter.Key Changes:
beetsplug/_utils/requests.pywithTimeoutSessionclass and HTTP error handling (HTTPNotFoundError,CaptchaError)musicbrainzngsdependency with customMusicBrainzAPIclass using direct HTTP requestsrequests-ratelimiterfor API rate limiting instead ofmusicbrainzngs.set_rate_limit()medium-list→media,track-list→tracks)musicbrainzngsoptional and added it to plugin-specific extras (listenbrainz,mbcollection,missing,parentwork). Updated plugin docs accordingly.requestsa required dependency to ensure backwards compatibility (ideally, we would make it an optional dependency undermusicbrainzextra).MusicBrainzAPIErrorwrapper classBenefits:
The changes maintain backward compatibility while modernizing the underlying implementation.