From 1e0578b9cbc2adf46aed4f3d06cd0f560fe5fd3c Mon Sep 17 00:00:00 2001 From: Ross Williams Date: Sun, 12 Jul 2026 20:06:25 +0100 Subject: [PATCH 1/3] test: fix ffprobe tests incorrectly skipped The `@NEEDS_FFPROBE` decorator was incorrectly skipping tests when `ffprobe` is available, because it was calling `ffprobe --version`, while the valid option is `ffprobe -version` (with a single dash). Signed-off-by: Ross Williams --- beets/test/helper.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beets/test/helper.py b/beets/test/helper.py index 61c5e650f..4a91a7261 100644 --- a/beets/test/helper.py +++ b/beets/test/helper.py @@ -95,7 +95,7 @@ NEEDS_REFLINK = pytest.mark.skipif( not check_reflink_support(gettempdir()), reason="need reflink" ) NEEDS_FFPROBE = pytest.mark.skipif( - not has_program("ffprobe") and not RUNNING_IN_CI, + not has_program("ffprobe", ("-version",)) and not RUNNING_IN_CI, reason="ffprobe (ffmpeg) is not available", ) From c2fc7a8a57ce73a359a3329fb0d74471113b92b3 Mon Sep 17 00:00:00 2001 From: Ross Williams Date: Sun, 12 Jul 2026 19:17:22 +0100 Subject: [PATCH 2/3] test: fix requests_mock setup in test_art.py `requests_mock` was configured with a fallback matcher to return an exception on any request. The intent was to have tests fail if any requests are attempted in tests without matching mocks. This is the default behavior of `requests_mock`, so no fallback matcher is necessary. The side-effect of the fallback matcher, removed by this commit, was that 1) the `NoMockAddress` exception was raised to the function under test rather than to the test harness and 2) the `NoMockAddress` exception was not able to be constructed because the `exc` parameter to `requests_mock.register_uri` does not support exceptions that require arguments. This behavior is visible in the following log snippet, taken from an incorrectly-passing test: ``` DEBUG beets.fetchart:logging.py:163 downloading image: https://images.amazon.com/images/P/xxxx.01.LZZZZZZZ.jpg DEBUG requests_mock.adapter:adapter.py:256 GET https://images.amazon.com/images/P/xxxx.01.LZZZZZZZ.jpg 200 DEBUG beets.fetchart:logging.py:163 not a supported image: text/html DEBUG beets.fetchart:logging.py:163 downloading image: https://images.amazon.com/images/P/xxxx.02.LZZZZZZZ.jpg DEBUG beets.fetchart:logging.py:163 error fetching art: NoMockAddress.__init__() missing 1 required positional argument: 'request' ``` The test that emitted these logs passed because the `FetchArt` plugin is written to handle exceptions raised by art sources, skipping and logging an error. For the exception to cause a failing test, the default behavior of `requests_mock` of raising an exception to the test harness when no matching mock requests can be found must be allowed. Signed-off-by: Ross Williams --- test/plugins/test_art.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/plugins/test_art.py b/test/plugins/test_art.py index a787c9034..709d0f7c4 100644 --- a/test/plugins/test_art.py +++ b/test/plugins/test_art.py @@ -10,8 +10,6 @@ from unittest.mock import patch import confuse import pytest -from requests_mock import ANY as ANYREEQUEST -from requests_mock.exceptions import NoMockAddress from beets import config, importer, logging, util from beets.autotag import AlbumInfo, AlbumMatch, Distance @@ -79,8 +77,8 @@ class UseThePlugin(TestHelper): # Register some “safe” mocks you actually want to allow, if any: # requests_mock.get("https://example.com/health", json={"status": "ok"}) - # Optional: disable any URL not explicitly mocked - requests_mock.register_uri(ANYREEQUEST, ANYREEQUEST, exc=NoMockAddress) + # Disable any URL not explicitly mocked + return @pytest.fixture(autouse=True, scope="class") def cleanup(self): @@ -343,6 +341,7 @@ class TestCombined(UseThePlugin, FetchImageHelper, CAAData): ASIN = "xxxx" MBID = "releaseid" AMAZON_URL = f"https://images.amazon.com/images/P/{ASIN}.01.LZZZZZZZ.jpg" + AMAZON_URL_2 = f"https://images.amazon.com/images/P/{ASIN}.02.LZZZZZZZ.jpg" AAO_URL = f"https://www.albumart.org/index_detail.php?asin={ASIN}" @pytest.fixture @@ -394,6 +393,7 @@ class TestCombined(UseThePlugin, FetchImageHelper, CAAData): def test_main_interface_falls_back_to_aao(self, dpath, image_request_mock): image_request_mock.get(self.AMAZON_URL, content_type="text/html") + image_request_mock.get(self.AMAZON_URL_2, content_type="text/html") image_request_mock.get(self.AAO_URL, content_type="image/jpeg") album = Album(asin=self.ASIN) self.plugin.art_for_album(album, [dpath]) From 2293d2f60ae50b79b68ac0b3bae6a35145eacd40 Mon Sep 17 00:00:00 2001 From: TowyTowy Date: Mon, 13 Jul 2026 14:00:57 +0200 Subject: [PATCH 3/3] Fix crash when sorting by a nullable field missing on some items Sorting by a field whose declared type is nullable (its null value is `None`, e.g. `NullInteger`/`NullFloat`) raised `TypeError: '<' not supported between instances of ...` when the field was present on some objects but missing on others, because `FieldSort.sort` compared `None` against real values. Group missing values together in the sort key so they are never compared against present ones: they are ordered first when sorting ascending and last when descending, matching SQLite's default ordering of NULLs used by the fast `FixedFieldSort` path. Fixes #3461. Co-Authored-By: Claude --- beets/dbcore/sort.py | 13 +++++++++---- docs/changelog.rst | 5 +++++ test/dbcore/test_sort.py | 28 ++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/beets/dbcore/sort.py b/beets/dbcore/sort.py index 65079b22b..d45577ea6 100644 --- a/beets/dbcore/sort.py +++ b/beets/dbcore/sort.py @@ -111,9 +111,8 @@ class FieldSort(Sort): self.case_insensitive = case_insensitive def sort(self, objs: list[AnyModel]) -> list[AnyModel]: - # TODO: Conversion and null-detection here. In Python 3, - # comparisons with None fail. We should also support flexible - # attributes with different types without falling over. + # TODO: Support flexible attributes with different types (e.g. a mix + # of strings and numbers) without falling over. def key(obj: Model) -> Any: field_val = obj.get(self.field, None) @@ -126,7 +125,13 @@ class FieldSort(Sort): field_val = "" if self.case_insensitive and isinstance(field_val, str): field_val = field_val.lower() - return field_val + # Nullable types (e.g. ``NullInteger``/``NullFloat``) use ``None`` + # as their null value, so a field may be missing on some objects + # and present on others. Comparing ``None`` with a real value + # raises a ``TypeError``, so group all missing values together: + # this places them first when sorting ascending and last when + # descending, matching SQLite's default ordering of NULLs. + return (field_val is not None, field_val) return sorted(objs, key=key, reverse=not self.ascending) diff --git a/docs/changelog.rst b/docs/changelog.rst index 1ffa1d9e3..b87b369b3 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -55,6 +55,11 @@ Bug fixes instrumental track, and store that state in ``lyrics_instrumental`` flexible attribute. Existing ``[Instrumental]`` lyrics are migrated automatically. :bug:`6719` +- Sorting by a nullable field (for example a flexible attribute with a declared + type whose null value is ``None``) that is present on only some items no + longer crashes with a ``TypeError``. Missing values are now grouped together, + ordered before present ones when sorting ascending and after them when + descending. :bug:`3461` .. For plugin developers diff --git a/test/dbcore/test_sort.py b/test/dbcore/test_sort.py index 876ab9019..0edd16397 100644 --- a/test/dbcore/test_sort.py +++ b/test/dbcore/test_sort.py @@ -253,6 +253,34 @@ class TestNonExistingField: assert ids_asc == [*null_values_ids, lower_item.id, higher_item.id] assert ids_desc == [higher_item.id, lower_item.id, *null_values_ids] + @pytest.mark.parametrize( + "field_type,lower,higher", + [ + _p(types.NullInteger(), 2, 10, id="null-integer"), + _p(types.NullFloat(), 2.5, 10.5, id="null-float"), + ], + ) + def test_nullable_field_present_in_some_items( + self, monkeypatch, field_type, lower, higher + ): + """Ordering by a nullable-typed field (whose null value is ``None``) + that is present on only some items should not raise. See #3461. + """ + monkeypatch.setitem(Item._types, "mynull", field_type) + + lower_item, higher_item, *items_without_val = self.lib.items("id+") + for item, value in zip((lower_item, higher_item), (lower, higher)): + item.mynull = value + item.store() + + null_values_ids = [i.id for i in items_without_val] + + ids_asc = [i.id for i in self.lib.items("mynull+ id+")] + ids_desc = [i.id for i in self.lib.items("mynull- id+")] + + assert ids_asc == [*null_values_ids, lower_item.id, higher_item.id] + assert ids_desc == [higher_item.id, lower_item.id, *null_values_ids] + def test_negation_interaction(self): """Test the handling of negation and sorting together.