Gracefully handle 404s when importing from MusicBrainz.

[djl]

A 404 error can be raised when fetching from MusicBrainz in the case of re-importing an album that has since been deleted from MusicBrainz.

• Documentation. (N/A)
• Changelog. (N/A)
• Tests.

[github-actions]

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]

Hey - I've found 2 issues, and left some high level feedback:

• Decorating setUp with @responses.activate only wraps the setup method, not the actual tests, so the mocked 404 response in test_import_handles_404_gracefully will never be used; instead, decorate the specific test (or use a fixture/context manager) to ensure responses is active during the HTTP call.
• test_import_handles_404_gracefully passes a random uuid.uuid4() as the release ID, which will be rejected by the MBID validator before any HTTP request is made, so the test currently doesn’t exercise the 404-handling path; use a syntactically valid MBID string that passes validation to reach the new HTTPNotFoundError branch.
• Consider whether other MusicBrainz 404-style failures (e.g., different exception types or ResponseError variants) should also result in None, and if so, broaden the exception handling or add a brief comment justifying why only HTTPNotFoundError is caught.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Decorating `setUp` with `@responses.activate` only wraps the setup method, not the actual tests, so the mocked 404 response in `test_import_handles_404_gracefully` will never be used; instead, decorate the specific test (or use a fixture/context manager) to ensure `responses` is active during the HTTP call.
- `test_import_handles_404_gracefully` passes a random `uuid.uuid4()` as the release ID, which will be rejected by the MBID validator before any HTTP request is made, so the test currently doesn’t exercise the 404-handling path; use a syntactically valid MBID string that passes validation to reach the new `HTTPNotFoundError` branch.
- Consider whether other MusicBrainz 404-style failures (e.g., different exception types or `ResponseError` variants) should also result in `None`, and if so, broaden the exception handling or add a brief comment justifying why only `HTTPNotFoundError` is caught.

## Individual Comments

### Comment 1
<location> `test/plugins/test_musicbrainz.py:32` </location>
<code_context>


 class MusicBrainzTestCase(BeetsTestCase):
+    @responses.activate
     def setUp(self):
         super().setUp()
</code_context>

<issue_to_address>
**issue (testing):** Using `@responses.activate` on `setUp` likely doesn’t activate the mock for the actual test methods and may unintentionally affect the whole class.

Because the decorator only wraps `setUp`, `responses` is active only while `setUp` runs, not during the test bodies. Tests like `test_import_handles_404_gracefully` will still perform real HTTP calls unless they’re individually decorated or use the context manager, and it becomes unclear which tests rely on `responses`. Please remove the decorator from `setUp` and instead apply `@responses.activate` (or a context manager) directly to the tests that need HTTP mocking.
</issue_to_address>

### Comment 2
<location> `test/plugins/test_musicbrainz.py:1113-1122` </location>
<code_context>
         assert candidates[0].tracks[0].track_id == self.RECORDING["id"]
         assert candidates[0].album == "hi"
+
+    def test_import_handles_404_gracefully(self, mb):
+        id_ = uuid.uuid4()
+        responses.add(
+            responses.GET,
+            f"https://musicbrainz.org/release/{id_}",
+            status=404,
+            body="",
+        )
+        res = mb.album_for_id(id_)
+        assert res is None
</code_context>

<issue_to_address>
**issue (testing):** The new 404 handling test does not activate the `responses` mock, so it won’t intercept the HTTP call.

This test calls `responses.add(...)` but the test method itself isn’t under `@responses.activate` (and decorating `setUp` won’t cover the test body). That means `mb.album_for_id(id_)` will likely hit the real network instead of the stubbed 404. Please either add `@responses.activate` to this test or use a `with responses.RequestsMock():` context so the 404 path is exercised without depending on external HTTP calls.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[Copilot]

Pull request overview

This pull request adds graceful handling of HTTP 404 errors when importing albums from MusicBrainz. The change addresses the scenario where an album has been deleted from MusicBrainz since it was last imported, preventing the application from crashing.

Changes:

• Added exception handling for HTTPNotFoundError in the album_for_id method to return None instead of raising an exception
• Added test coverage to verify 404 errors are handled gracefully

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
beetsplug/musicbrainz.py	Added try-except block to catch HTTPNotFoundError when fetching a release by ID, returning None when a 404 occurs
test/plugins/test_musicbrainz.py	Added test imports (uuid, responses) and new test case to verify 404 handling behavior

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Consider adding a debug log message when a 404 error is caught to help with debugging, similar to the existing debug messages in this method. For example: self._log.debug("Release {} not found on MusicBrainz (404).", albumid). This would be helpful for users trying to understand why a release import failed.

Suggested change

	except HTTPNotFoundError:
	except HTTPNotFoundError:
	self._log.debug(
	"Release {} not found on MusicBrainz (404).", albumid
	)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.73%. Comparing base (ef59cfa) to head (8ce6b18).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6292      +/-   ##
==========================================
+ Coverage   68.71%   68.73%   +0.01%     
==========================================
  Files         138      138              
  Lines       18558    18558              
  Branches     3062     3062              
==========================================
+ Hits        12753    12755       +2     
+ Misses       5154     5152       -2     
  Partials      651      651              

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

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]

Hey - I've found 1 issue, and left some high level feedback:

• In album_for_id, consider logging when an HTTPNotFoundError is caught so that silent 404s don’t make debugging missing releases harder.
• In test_import_handles_404_gracefully, using responses query parameter matching instead of hardcoding the full encoded URL (including inc) would make the test less brittle to future changes in how the URL is constructed.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `album_for_id`, consider logging when an `HTTPNotFoundError` is caught so that silent 404s don’t make debugging missing releases harder.
- In `test_import_handles_404_gracefully`, using `responses` query parameter matching instead of hardcoding the full encoded URL (including `inc`) would make the test less brittle to future changes in how the URL is constructed.

## Individual Comments

### Comment 1
<location> `test/plugins/test_musicbrainz.py:1113-1122` </location>
<code_context>
         assert candidates[0].album == "hi"
+
+    @responses.activate
+    def test_import_handles_404_gracefully(self, mb):
+        id_ = uuid.uuid4()
+        inc = "%2B".join(musicbrainz.RELEASE_INCLUDES)
+        responses.add(
+            responses.GET,
+            f"https://musicbrainz.org/ws/2/release/{id_}?inc={inc}&fmt=json",
+            status=404,
+            body="",
+        )
+        res = mb.album_for_id(str(id_))
+        assert res is None
</code_context>

<issue_to_address>
**suggestion (testing):** Missing negative test to show that non-404 errors are *not* swallowed by `album_for_id`.

Please also add a test where `mb.mb_api.get_release` raises a non-404 HTTP error (e.g. 500) and assert that it is propagated instead of returning `None`. This will confirm the change only special-cases 404s and preserves existing behavior for other errors.

Suggested implementation:

```python
import pytest
import responses
import requests

from beets import config
from beets.library import Item

```

```python
        assert len(candidates) == 1
        assert candidates[0].tracks[0].track_id == self.RECORDING["id"]
        assert candidates[0].album == "hi"

    @responses.activate
    def test_import_handles_404_gracefully(self, mb):
        id_ = uuid.uuid4()
        inc = "%2B".join(musicbrainz.RELEASE_INCLUDES)
        responses.add(
            responses.GET,
            f"https://musicbrainz.org/ws/2/release/{id_}?inc={inc}&fmt=json",
            status=404,
            body="",
        )
        res = mb.album_for_id(str(id_))
        assert res is None

    def test_import_propagates_non_404_errors(self, mb):
        class DummyResponse:
            status_code = 500

        error = requests.exceptions.HTTPError(response=DummyResponse())

        def raise_error(*args, **kwargs):
            raise error

        # Simulate mb.mb_api.get_release raising a non-404 HTTP error
        mb.mb_api.get_release = raise_error

        with pytest.raises(requests.exceptions.HTTPError) as excinfo:
            mb.album_for_id("some-id")

        # Ensure the exact error is propagated, not swallowed
        assert excinfo.value is error

```
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Missing negative test to show that non-404 errors are not swallowed by album_for_id.

Please also add a test where mb.mb_api.get_release raises a non-404 HTTP error (e.g. 500) and assert that it is propagated instead of returning None. This will confirm the change only special-cases 404s and preserves existing behavior for other errors.

Suggested implementation:

import pytest
import responses
import requests

from beets import config
from beets.library import Item

        assert len(candidates) == 1
        assert candidates[0].tracks[0].track_id == self.RECORDING["id"]
        assert candidates[0].album == "hi"

    @responses.activate
    def test_import_handles_404_gracefully(self, mb):
        id_ = uuid.uuid4()
        inc = "%2B".join(musicbrainz.RELEASE_INCLUDES)
        responses.add(
            responses.GET,
            f"https://musicbrainz.org/ws/2/release/{id_}?inc={inc}&fmt=json",
            status=404,
            body="",
        )
        res = mb.album_for_id(str(id_))
        assert res is None

    def test_import_propagates_non_404_errors(self, mb):
        class DummyResponse:
            status_code = 500

        error = requests.exceptions.HTTPError(response=DummyResponse())

        def raise_error(*args, **kwargs):
            raise error

        # Simulate mb.mb_api.get_release raising a non-404 HTTP error
        mb.mb_api.get_release = raise_error

        with pytest.raises(requests.exceptions.HTTPError) as excinfo:
            mb.album_for_id("some-id")

        # Ensure the exact error is propagated, not swallowed
        assert excinfo.value is error

[snejus]

Please use requests_mock fixture: you will find some examples in test_lyrics.py tests.