Repository: beetbox/beets
Pull request: Fix sorting on missing non-string fields
## 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`.

- [x] Documentation.
- [x] Changelog.
- [x] Tests.


Problem statement:
mpdstats: Error when querying `play_count` with a sort order
<!--
Describe your problem, feature request, or discussion topic here.

Please fill out this and the "Setup" section below and remember to include
enough detail so that other people can reproduce the problem.
-->

### Error when sorting certain mpdstats queries

Running this command in verbose (`-vv`) mode:

```sh
$ beet -vv list play_count+
```

Led to this problem:

```
user configuration: ~/.config/beets/config.yaml
data directory: ~/.config/beets
plugin paths: 
Sending event: pluginload
library database: ~/Music/musiclibrary.db
library directory: ~/Music
Sending event: library_opened
Parsed query: AndQuery([TrueQuery()])
Parsed sort: SlowFieldSort('play_count', ascending=True)
Traceback (most recent call last):
  File "/usr/bin/beet", line 8, in <module>
    sys.exit(main())
             ^^^^^^
  File "/usr/lib/python3.12/site-packages/beets/ui/__init__.py", line 1865, in main
    _raw_main(args)
  File "/usr/lib/python3.12/site-packages/beets/ui/__init__.py", line 1852, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/usr/lib/python3.12/site-packages/beets/ui/commands.py", line 1599, in list_func
    list_items(lib, decargs(args), opts.album)
  File "/usr/lib/python3.12/site-packages/beets/ui/commands.py", line 1594, in list_items
    for item in lib.items(query):
                ^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/beets/dbcore/db.py", line 823, in __iter__
    objects = self.sort.sort(list(self._get_objects()))
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/site-packages/beets/dbcore/query.py", line 984, in sort
    return sorted(objs, key=key, reverse=not self.ascending)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '<' not supported between instances of 'int' and 'str'
```
### Setup

* OS: Artix Linux
* Python version: 3.12.7
* beets version: 1.6.1
* Turning off plugins made problem go away (yes/no): yes

<!--
You can turn off plugins temporarily by passing --plugins= on the command line:

$ beet --plugins= version
-->

My configuration (output of `beet config`) is:

```yaml
directory: ~/Music
# --------------- Main ---------------

library: ~/Music/musiclibrary.db

# --------------- Plugins ---------------

plugins: mpdstats

import:
    copy: no
mpd:
    music_directory: ~/Music
    strip_path: ''
    rating: yes
    rating_mix: 0.75
    host: localhost
    port: 6600
    password: REDACTED
```


It also happens with `list rating+` and `list skip_count+`. It happens with both ascending (`play_count+`) and descending (`play_count-`) sorting.

Hints from issue discussion:
This is happening because values for these fields are expected to be numeric, however beets fails handling **empty** values which appear as empty strings. This needs fixing for sure.

Meanwhile, try filtering for non-empty values in your query:

```diff
-$ beet -vv list play_count+
+$ beet -vv list play_count+ play_count::.
```

`play_count::.` picks up everything that has at least one character

Commit message: format

Patch to review:
diff --git a/beets/dbcore/query.py b/beets/dbcore/query.py
index da621a767a..5af1cc417b 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 the field is typed, use its null value.
+            if field_val is None:
+                if self.field in obj._types:
+                    field_val = obj._types[self.field].null
+            # If not, or the null value is None, fall back to using an empty string.
+            if field_val is None:
+                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/test/test_sort.py b/test/test_sort.py
index 1fc8a0463d..cff4c96480 100644
--- a/test/test_sort.py
+++ b/test/test_sort.py
@@ -16,7 +16,8 @@
 
 import beets.library
 from beets import config, dbcore
-from beets.library import Album
+from beets.dbcore import types
+from beets.library import Album, Item
 from beets.test import _common
 from beets.test.helper import BeetsTestCase
 
@@ -471,6 +472,10 @@ def test_case_sensitive_only_affects_text(self):
 class NonExistingFieldTest(DummyDataTestCase):
     """Test sorting by non-existing fields"""
 
+    def tearDown(self):
+        super().tearDown()
+        Item._types = {}
+
     def test_non_existing_fields_not_fail(self):
         qs = ["foo+", "foo-", "--", "-+", "+-", "++", "-foo-", "-foo+", "---"]
 
@@ -499,7 +504,7 @@ def test_combined_non_existing_field_desc(self):
 
     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)
+        # append 'foo' to two items (1,2)
         items = self.lib.items("id+")
         ids = [i.id for i in items]
         items[1].foo = "bar1"
@@ -514,6 +519,24 @@ def test_field_present_in_some_items(self):
         # items without field last
         assert [i.id for i in results_desc] == [ids[2], ids[1], ids[0], ids[3]]
 
+    def test_int_field_present_in_some_items(self):
+        """Test ordering by a field not present on all items."""
+        # append int-valued 'foo' to two items (1,2)
+        Item._types = {"foo": types.Integer()}
+        items = self.lib.items("id+")
+        ids = [i.id for i in items]
+        items[1].foo = 1
+        items[2].foo = 2
+        items[1].store()
+        items[2].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]]
+        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]]
+
     def test_negation_interaction(self):
         """Test the handling of negation and sorting together.
 


Code review comments:
diff_hunk: @@ -514,6 +519,24 @@ def test_field_present_in_some_items(self):
         # items without field last
         assert [i.id for i in results_desc] == [ids[2], ids[1], ids[0], ids[3]]
 
+    def test_int_field_present_in_some_items(self):
+        """Test ordering by a field not present on all items."""
+        # append int-valued 'foo' to two items (1,2)
+        Item._types = {"foo": types.Integer()}
+        items = self.lib.items("id+")
+        ids = [i.id for i in items]
+        items[1].foo = 1
+        items[2].foo = 2
+        items[1].store()
+        items[2].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]]
+        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]]
+
path: test/test_sort.py
text: @user2:
Consider naming variables in a slightly more clear way, something like this

```suggestion
    @user1("beets.library.Item._types", {"foo": types.Integer()})
    def test_int_field_present_in_some_items(self):
        """Test ordering by a field not present on all items."""
        # append int-valued 'foo' to two items (1,2)
        items = self.lib.items("id+")
        lower_foo_item, higher_foo_item, *items_without_foo = items
        lower_foo_item.foo = 1
        higher_foo_item.foo = 2
        lower_foo_item.store()
        higher_foo_item.store()

        items_without_foo_ids = [i.id for i in items_without_foo]
        results_asc = list(self.lib.items("foo+ id+"))
        # items without field first
        assert [i.id for i in results_asc] == [
            *items_without_foo_ids,
            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] == [
            higher_foo_item.id,
            lower_foo_item.id,
            *items_without_foo_ids,
        ]

```


@author:
Done. Like the other change this one was c/p from elsewhere so I modified the source too.

diff_hunk: @@ -514,6 +519,24 @@ def test_field_present_in_some_items(self):
         # items without field last
         assert [i.id for i in results_desc] == [ids[2], ids[1], ids[0], ids[3]]
 
+    def test_int_field_present_in_some_items(self):
+        """Test ordering by a field not present on all items."""
+        # append int-valued 'foo' to two items (1,2)
+        Item._types = {"foo": types.Integer()}
path: test/test_sort.py
text: @user2:
Instead of overwriting global `Item` try patching it:

```
    @user1("beets.library.Item._types", {"foo": types.Integer()})
    def test_int_field_present_in_some_items(self):
```

Then you can remove the `tearDown` that got added in line 475.

@author:
Nice, thank you. I copied this pattern from some tests in `test_query.py` so I updated those to patch as well, for consistency's sake.

@user2:
Amazing stuff!

diff_hunk: @@ -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 the field is typed, use its null value.
+            if field_val is None:
+                if self.field in obj._types:
+                    field_val = obj._types[self.field].null
+            # If not, or the null value is None, fall back to using an empty string.
+            if field_val is None:
+                field_val = ""
path: beets/dbcore/query.py
text: @user1:
I think this condition can be slightly simplified

```suggestion
            if field_val is None:
                if _type := obj._types.get(self.field):
                    # If the field is typed, use its null value.
                    field_val = _type.null
                else:
                    # If not, fall back to using an empty string.
                    field_val = ""
```