diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py index da621a767a..866162c4ad 100644 --- a/beets/dbcore/query.py +++ b/beets/dbcore/query.py @@ -985,7 +985,7 @@ class FieldSort(Sort): def __init__( self, - field, + field: str, ascending: bool = True, case_insensitive: bool = True, ): @@ -999,7 +999,14 @@ def sort(self, objs: list[AnyModel]) -> list[AnyModel]: # attributes with different types without falling over. def key(obj: Model) -> Any: - field_val = obj.get(self.field, "") + field_val = obj.get(self.field, None) + if field_val is None: + if _type := obj._types.get(self.field): + # If the field is typed, use its null value. + field_val = obj._types[self.field].null + else: + # If not, fall back to using an empty string. + field_val = "" if self.case_insensitive and isinstance(field_val, str): field_val = field_val.lower() return field_val diff --git a/docs/changelog.rst b/docs/changelog.rst index 5c206624b0..d318b053a3 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -30,6 +30,9 @@ Bug fixes: * :ref:`import-cmd`: Fix ``MemoryError`` and improve performance tagging large albums by replacing ``munkres`` library with ``lap.lapjv``. :bug:`5207` +* :ref:`query-sort`: Fix a bug that would raise an exception when sorting on + a non-string field that is not populated in all items. + :bug:`5512` For packagers: diff --git a/docs/dev/plugins.rst b/docs/dev/plugins.rst index 92a7f870e8..fa7fa645e8 100644 --- a/docs/dev/plugins.rst +++ b/docs/dev/plugins.rst @@ -542,6 +542,9 @@ Specifying types has several advantages: * User input for flexible fields may be validated and converted. +* Items missing the given field can use an appropriate null value for + querying and sorting purposes. + .. _plugin-logging: diff --git a/poetry.lock b/poetry.lock index 2d03e8cf0f..61413bf4e5 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1,4 +1,4 @@ -# This file is automatically @generated by Poetry 1.8.4 and should not be changed by hand. +# This file is automatically @generated by Poetry 1.8.3 and should not be changed by hand. [[package]] name = "accessible-pygments" @@ -3111,6 +3111,17 @@ files = [ {file = "types_html5lib-1.1.11.20241018-py3-none-any.whl", hash = "sha256:3f1e064d9ed2c289001ae6392c84c93833abb0816165c6ff0abfc304a779f403"}, ] +[[package]] +name = "types-mock" +version = "5.1.0.20240425" +description = "Typing stubs for mock" +optional = false +python-versions = ">=3.8" +files = [ + {file = "types-mock-5.1.0.20240425.tar.gz", hash = "sha256:5281a645d72e827d70043e3cc144fe33b1c003db084f789dc203aa90e812a5a4"}, + {file = "types_mock-5.1.0.20240425-py3-none-any.whl", hash = "sha256:d586a01d39ad919d3ddcd73de6cde73ca7f3c69707219f722d1b8d7733641ad7"}, +] + [[package]] name = "types-pillow" version = "10.2.0.20240822" @@ -3274,4 +3285,4 @@ web = ["flask", "flask-cors"] [metadata] lock-version = "2.0" python-versions = ">=3.9,<4" -content-hash = "b6b44295999e2b8c3868b03321df60a2501abc9162a7e802de37ab2ae8aa14ff" +content-hash = "2edbbe1f3488fb9d3a05e2d60c23d3fd1afaa8154d2a71ffad83b34476ceac78" diff --git a/pyproject.toml b/pyproject.toml index cf3347b134..3d91a59b4a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -101,6 +101,7 @@ ruff = ">=0.6.4" [tool.poetry.group.typing.dependencies] mypy = "*" types-beautifulsoup4 = "*" +types-mock = "*" types-Flask-Cors = "*" types-Pillow = "*" types-PyYAML = "*" diff --git a/test/test_query.py b/test/test_query.py index d17bce0e67..6f7fe4da72 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -21,6 +21,7 @@ from functools import partial import pytest +from mock import patch import beets.library from beets import dbcore, util @@ -30,7 +31,6 @@ NoneQuery, ParsingError, ) -from beets.library import Item from beets.test import _common from beets.test.helper import BeetsTestCase, ItemInDBTestCase from beets.util import syspath @@ -715,10 +715,6 @@ def test_detect_relative_path(self): class IntQueryTest(BeetsTestCase): - def tearDown(self): - super().tearDown() - Item._types = {} - def test_exact_value_match(self): item = self.add_item(bpm=120) matched = self.lib.items("bpm:120").get() @@ -732,14 +728,14 @@ def test_range_match(self): assert 1 == len(matched) assert item.id == matched.get().id + @patch("beets.library.Item._types", {"myint": types.Integer()}) def test_flex_range_match(self): - Item._types = {"myint": types.Integer()} item = self.add_item(myint=2) matched = self.lib.items("myint:2").get() assert item.id == matched.id + @patch("beets.library.Item._types", {"myint": types.Integer()}) def test_flex_dont_match_missing(self): - Item._types = {"myint": types.Integer()} self.add_item() matched = self.lib.items("myint:2").get() assert matched is None @@ -750,15 +746,8 @@ def test_no_substring_match(self): assert matched is None +@patch("beets.library.Item._types", {"flexbool": types.Boolean()}) class BoolQueryTest(BeetsTestCase, AssertsMixin): - def setUp(self): - super().setUp() - Item._types = {"flexbool": types.Boolean()} - - def tearDown(self): - super().tearDown() - Item._types = {} - def test_parse_true(self): item_true = self.add_item(comp=True) item_false = self.add_item(comp=False) diff --git a/test/test_sort.py b/test/test_sort.py index 1fc8a0463d..d6aa5c518b 100644 --- a/test/test_sort.py +++ b/test/test_sort.py @@ -14,8 +14,11 @@ """Various tests for querying the library database.""" +from mock import patch + import beets.library from beets import config, dbcore +from beets.dbcore import types from beets.library import Album from beets.test import _common from beets.test.helper import BeetsTestCase @@ -498,21 +501,57 @@ def test_combined_non_existing_field_desc(self): assert r1.id == r2.id def test_field_present_in_some_items(self): - """Test ordering by a field not present on all items.""" - # append 'foo' to two to items (1,2) - items = self.lib.items("id+") - ids = [i.id for i in items] - items[1].foo = "bar1" - items[2].foo = "bar2" - items[1].store() - items[2].store() + """Test ordering by a (string) field not present on all items.""" + # append 'foo' to two items (1,2) + lower_foo_item, higher_foo_item, *items_without_foo = self.lib.items( + "id+" + ) + lower_foo_item.foo, higher_foo_item.foo = "bar1", "bar2" + lower_foo_item.store() + higher_foo_item.store() results_asc = list(self.lib.items("foo+ id+")) - # items without field first - assert [i.id for i in results_asc] == [ids[0], ids[3], ids[1], ids[2]] + assert [i.id for i in results_asc] == [ + # items without field first + *[i.id for i in items_without_foo], + lower_foo_item.id, + higher_foo_item.id, + ] + results_desc = list(self.lib.items("foo- id+")) - # items without field last - assert [i.id for i in results_desc] == [ids[2], ids[1], ids[0], ids[3]] + assert [i.id for i in results_desc] == [ + higher_foo_item.id, + lower_foo_item.id, + # items without field last + *[i.id for i in items_without_foo], + ] + + @patch("beets.library.Item._types", {"myint": types.Integer()}) + def test_int_field_present_in_some_items(self): + """Test ordering by an int-type field not present on all items.""" + # append int-valued 'myint' to two items (1,2) + lower_myint_item, higher_myint_item, *items_without_myint = ( + self.lib.items("id+") + ) + lower_myint_item.myint, higher_myint_item.myint = 1, 2 + lower_myint_item.store() + higher_myint_item.store() + + results_asc = list(self.lib.items("myint+ id+")) + assert [i.id for i in results_asc] == [ + # items without field first + *[i.id for i in items_without_myint], + lower_myint_item.id, + higher_myint_item.id, + ] + + results_desc = list(self.lib.items("myint- id+")) + assert [i.id for i in results_desc] == [ + higher_myint_item.id, + lower_myint_item.id, + # items without field last + *[i.id for i in items_without_myint], + ] def test_negation_interaction(self): """Test the handling of negation and sorting together.