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.