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/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", ) diff --git a/docs/changelog.rst b/docs/changelog.rst index 452db38f9..c99f7b295 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -59,6 +59,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. 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])