Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sorting on missing non-string fields #5570

Merged
merged 10 commits into from
Jan 1, 2025
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions beets/dbcore/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -985,7 +985,7 @@ class FieldSort(Sort):

def __init__(
self,
field,
field: str,
ascending: bool = True,
case_insensitive: bool = True,
):
Expand All @@ -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 = ""
valrus marked this conversation as resolved.
Show resolved Hide resolved
if self.case_insensitive and isinstance(field_val, str):
field_val = field_val.lower()
return field_val
Expand Down
3 changes: 3 additions & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
3 changes: 3 additions & 0 deletions docs/dev/plugins.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
27 changes: 25 additions & 2 deletions test/test_sort.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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+", "---"]

Expand Down Expand Up @@ -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"
Expand All @@ -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()}
valrus marked this conversation as resolved.
Show resolved Hide resolved
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]]

valrus marked this conversation as resolved.
Show resolved Hide resolved
def test_negation_interaction(self):
"""Test the handling of negation and sorting together.

Expand Down
Loading