Fix sorting on missing non-string fields (#5570)

## Description

Fixes #5512. When sorting on a field, if the field is missing from some
items and it has a type, use the type's `null` value. Otherwise,
continue to fall back to an empty string, as I don't think there's much
to be done in that case.

The new test `test_int_field_present_in_some_items` fails without the
fix in `query.py`.

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 @@ class FieldSort(Sort):
         # 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

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:
 

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:
 

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"

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 = "*"

test/test_query.py

@@ -21,6 +21,7 @@ from contextlib import contextmanager
 from functools import partial
 
 import pytest
+from mock import patch
 
 import beets.library
 from beets import dbcore, util
@@ -30,7 +31,6 @@ from beets.dbcore.query import (
     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 @@ class PathQueryTest(ItemInDBTestCase, AssertsMixin):
 
 
 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 @@ class IntQueryTest(BeetsTestCase):
         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 @@ class IntQueryTest(BeetsTestCase):
         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)

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 @@ class NonExistingFieldTest(DummyDataTestCase):
             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.