Replace usage of the musicbrainzngs library with mbzero#5976
Replace usage of the musicbrainzngs library with mbzero#5976nicomem wants to merge 6 commits intobeetbox:masterfrom
musicbrainzngs library with mbzero#5976Conversation
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. |
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.
| @@ -1043,34 +1041,32 @@ def test_item_candidates(self, monkeypatch, mb): | |||
|
|
|||
| 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)
| ) | |
| ) 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)
| ) | |
| ) 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)
| ) | |
| ) from exc |
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 |
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 |
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 <louson@gresille.org> Signed-off-by: Nicolas Mémeint <git@nicomem.com>
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
5a75bab to
d0b9f8f
Compare
musicbrainzbgs library with mbzeromusicbrainzngs library with mbzero
|
Hi @nicomem Thanks so much for your contribution! We had an internal discussion about the best way to move forward, and we’ve decided to continue with #6052 instead, which removes external dependencies entirely. Because of this, we won’t be merging this PR. Relying heavily on external libraries has been tricky for us in the past, especially when we had to migrate away from unmaintained ones or deal with constantly changing public APIs That said, we really value your experience with the MusicBrainz API and would love any feedback, ideas, or suggestions you might have on #6052, it would be incredibly helpful as we move forward. Apologies for the long silence from our side. We really appreciate your work and effort! |
Hi @semohr, Thanks for your comment. I completely understand the choice of not relying on external libraries. I already had a quick look at the other PR before and it had a similar approach to what had been done in this one (which is a good sign ^^). |
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: