Replace usage of the musicbrainzngs library with mbzero
#5976
+1,259
−449
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR refactors all MusicBrainz interactions to use a new mbzero-based interface (MbInterface/SharedMbInterface), centralizes configuration and rate limiting, migrates existing plugins off musicbrainzngs (updating error handling and JSON key conventions), removes the direct musicbrainzngs dependency in favor of mbzero, and adds a thread-safe RateLimiter utility with comprehensive tests. Sequence diagram for MusicBrainz API request with shared rate limitingsequenceDiagram
participant Plugin
participant SharedMbInterface
participant MbInterface
participant RateLimiter
participant mbzero
Plugin->>SharedMbInterface: get()
SharedMbInterface->>MbInterface: return instance
Plugin->>MbInterface: API request (e.g. get_release_by_id)
MbInterface->>RateLimiter: __enter__ (acquire rate limit)
RateLimiter-->>MbInterface: allow request
MbInterface->>mbzero: send request
mbzero-->>MbInterface: response
MbInterface->>Plugin: return result
ER diagram for MusicBrainz configuration centralizationerDiagram
MUSICBRAINZ_CONFIG {
host string
https bool
ratelimit int
ratelimit_interval int
user string
pass string
}
SHARED_MB_INTERFACE {
uses MUSICBRAINZ_CONFIG
}
PLUGIN {
uses SHARED_MB_INTERFACE
}
SHARED_MB_INTERFACE ||--o| MUSICBRAINZ_CONFIG: centralizes
PLUGIN ||--o| SHARED_MB_INTERFACE: accesses
Class diagram for new MbInterface and SharedMbInterfaceclassDiagram
class MbInterface {
-hostname: str
-https: bool
-rate_limiter: RateLimiter
-useragent: str
-auth: MbzCredentials | None
+browse_recordings(...)
+browse_release(...)
+browse_release_groups(...)
+get_release_by_id(...)
+get_recording_by_id(...)
+get_work_by_id(...)
+get_user_collections(...)
+search_releases(...)
+search_recordings(...)
+add_releases_to_collection(...)
+remove_releases_from_collection(...)
}
class SharedMbInterface {
-mb_interface: MbInterface
+require_auth_for_plugin(...)
+get(): MbInterface
}
SharedMbInterface --> MbInterface
class RateLimiter {
-reqs_per_interval: int
-interval_sec: float
-lock: threading.Lock
-last_call: float
-remaining_requests: float | None
+__enter__()
+__exit__()
}
MbInterface --> RateLimiter
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. |
![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:
- Consider abstracting the repetitive JSON field renaming logic (e.g. mapping 'medium-list'→'media', 'track-list'→'tracks', etc.) into a shared transformation utility to reduce boilerplate and improve consistency.
- Most of the new MbInterface public methods are missing explicit return type annotations; adding these will improve readability and help catch type errors earlier.
- The custom RateLimiter uses sleep loops under a lock; consider switching to a condition-based wait or token-bucket approach to avoid potential thread contention and improve responsiveness under high concurrency.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider abstracting the repetitive JSON field renaming logic (e.g. mapping 'medium-list'→'media', 'track-list'→'tracks', etc.) into a shared transformation utility to reduce boilerplate and improve consistency.
- Most of the new MbInterface public methods are missing explicit return type annotations; adding these will improve readability and help catch type errors earlier.
- The custom RateLimiter uses sleep loops under a lock; consider switching to a condition-based wait or token-bucket approach to avoid potential thread contention and improve responsiveness under high concurrency.
## Individual Comments
### Comment 1
<location> `beetsplug/musicbrainz.py:862` </location>
<code_context>
)
# release is potentially a pseudo release
- release = self.album_info(res["release"])
+ release = self.album_info(res)
# should be None unless we're dealing with a pseudo release
</code_context>
<issue_to_address>
Passing the entire response instead of the 'release' key may affect downstream logic.
Verify that album_info is designed to handle the full response object; otherwise, this change may cause runtime errors or incomplete data extraction.
</issue_to_address>
### Comment 2
<location> `beetsplug/_mb_interface.py:138` </location>
<code_context>
+ scheme = "https" if self.https else "http"
+ mbr.set_url(f"{scheme}://{self.hostname}/ws/2")
+ opts = {}
+ if limit:
+ opts["limit"] = limit
+ if offset:
+ opts["offset"] = offset
+ return mbr.send(opts=opts, credentials=self.auth)
</code_context>
<issue_to_address>
Using 'if limit:' and 'if offset:' may skip zero values, which could be valid for paging.
Use 'if limit is not None' and 'if offset is not None' to ensure zero values are included in opts.
</issue_to_address>
### Comment 3
<location> `test/plugins/test_musicbrainz.py:1042` </location>
<code_context>
def test_candidates(self, monkeypatch, mb):
monkeypatch.setattr(
- "musicbrainzngs.search_releases",
</code_context>
<issue_to_address>
Missing test for error handling in candidate search.
Add a test where MbInterface.search_releases raises an exception to confirm MusicBrainzAPIError is properly raised and handled.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| assert len(candidates) == 1 | ||
| assert candidates[0].track_id == self.RECORDING["id"] | ||
|
|
||
| def test_candidates(self, monkeypatch, mb): |
There was a problem hiding this comment.
suggestion (testing): Missing test for error handling in candidate search.
Add a test where MbInterface.search_releases raises an exception to confirm MusicBrainzAPIError is properly raised and handled.
| except mbzerror.MbzWebServiceError as exc: | ||
| raise MusicBrainzAPIError( | ||
| exc, f"{query_type} search", filters, traceback.format_exc() | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| ) | |
| ) from exc |
| except mbzerror.MbzWebServiceError as exc: | ||
| raise MusicBrainzAPIError( | ||
| exc, "get release by ID", albumid, traceback.format_exc() | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| ) | |
| ) from exc |
| except mbzerror.MbzWebServiceError as exc: | ||
| raise MusicBrainzAPIError( | ||
| exc, "get recording by ID", trackid, traceback.format_exc() | ||
| ) |
There was a problem hiding this comment.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
Suggested change
| ) | |
| ) from exc |
nicomem
force-pushed
the
feature/mbzero-squashed
branch
from
793a36c to
32468f1
Compare
|
Applied some of the suggestions and fixed the problems indentified in the CI jobs. For the ones I did not resolve, it was some existing code so I prefer leaving as-is to avoid any risk unless told otherwise |
nicomem
force-pushed
the
feature/mbzero-squashed
branch
from
32468f1 to
624cddd
Compare
|
Fixed tests instabilities by removing the rate-limiter checks of requests not executed too late (because threads may sleep for more time than requested). Failure on 3.9 due to |
nicomem
force-pushed
the
feature/mbzero-squashed
branch
from
624cddd to
5a75bab
Compare
`mbzero` is a lighter interface to access the MusicBrainz API, so a `MbInterface` has been created to re-add similar functions that `musicbrainzngs` provided. Create a shared `MbInterface` outside the plugin so that it can later be reused by other plugins, using the same rate limiter. - Instead of the `musicbrainz` plugin reading the shared config values, it is now `MbInterface` that does it. This makes sure these values are correctly handled in case the `musicbrainz` plugin is disabled, but not the others. Rate limiting is not done by `mbzero`, so it has been implemented here. The implementation is not specific to `MbInterface` and may be reused by others if needed in the future. - A rate-limiting bug has also been fixed compared to the `musicbrainzngs` implementation - See issue 294 on `python-musicbrainzngs` Tests have been modified as the structure of data received is a bit different than before. Signed-off-by: Louis Rannou <[email protected]> Signed-off-by: Nicolas Mémeint <[email protected]>
This migration required adding a new `browse_release_groups` to the `MbInterface`, but this is trivial.
Also remove the `musicbrainzngs` dependency now that it is not used anymore
nicomem
force-pushed
the
feature/mbzero-squashed
branch
from
5a75bab to
d0b9f8f
Compare
nicomem
changed the title
musicbrainzbgs library with mbzeromusicbrainzngs library with mbzero
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
General information
This PR continues the work started on #4936 by @Louson by rebasing the changes on master, fixing errors encountered, and also converting all usages of
musicbrainzngsin the other plugins tombzero.How this PR is composed
This PR is composed of the following commits:
MbInterfaceusable by plugins and migrates themusicbrainzplugin tombzerombzeromusicbrainzngslibrary since it is not used anymoreEach commit comes with a description detailling useful information about them.
Other benefits
On top of migrating off of
musicbrainzngswhich does not receive any update anymore, this PR already solves some problems I found while developping it:musicbrainzngsrate-limiter and it does not work correctly for non-default configuration (see Invalid rate limiting sleeping due to error in computation alastair/python-musicbrainzngs#294 for more information). The new implementation does not suffer of this problem, and has quite extensive testing added to make sure of its correctness.musicbrainzconfiguration options were previously only read and applied by themusicbrainzplugin; meaning that if it were disabled, the other plugins making MusicBrainz requests did not use the configuration. The shared configuration options is now applied bySharedMbInterfacewhich is thus applied when the first plugin making use of it is initialized.Tests done
Tested with
poe test, which returns no failure, and also by enabling integration tests for theparentworkplugin tests.I also manually tested a lot the
musicbrainzplugin by importing my quite varied music library (e.g. JP artists and titles, very long titles, missing tracks in albums, etc.), and tested many different user inputs (e.g. giving invalid MBID, aborting and continuing the import, importing as tracks or albums, etc.).I also tried to test manually the other plugins as best as I could, but since I do not use them much, there may be cases which I may have not tested (but I did make sure to test all of them for at least a few requests).
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)Summary by Sourcery
Replace all direct uses of the legacy musicbrainzngs library with a new mbzero-based MbInterface abstraction and consolidate MusicBrainz configuration and rate limiting across plugins
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests: