Skip to content

Commit d2da632

Browse files
jensensclaude
andauthored
fix(query): sort FieldIndex via jsonb -> so numbers sort numerically (#158) (#159)
Text operator `idx->>'{key}'` casts to TEXT before ORDER BY, so a FieldIndex over a numeric attribute ranked "10" before "2". The jsonb operator `idx->'{key}'` preserves type — numbers sort numerically, strings lexically — so homogeneous FieldIndexes sort correctly regardless of value type. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8f11164 commit d2da632

4 files changed

Lines changed: 63 additions & 10 deletions

File tree

CHANGES.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,18 @@
11
# Changelog
22

3+
## 1.0.0b64
4+
5+
### Fixed
6+
7+
- ``_process_sort`` now emits ``idx->'{key}'`` (JSONB operator) instead
8+
of ``idx->>'{key}'`` (text operator) for the ``FieldIndex`` fallback.
9+
Text-cast sorting compared everything lexicographically, so a
10+
``FieldIndex`` over a numeric attribute ranked ``"10"`` before
11+
``"2"``. JSONB comparison is type-aware: numbers sort numerically,
12+
strings lexically, so a homogeneous ``FieldIndex`` now always sorts
13+
correctly regardless of value type. Affects any ``FieldIndex`` with
14+
numeric source data (counters, priorities, prices, weights). #158
15+
316
## 1.0.0b63
417

518
### Changed

src/plone/pgcatalog/query.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -774,7 +774,10 @@ def _process_sort(self, sort_on_list, sort_order_list):
774774
elif idx_type == IndexType.BOOLEAN:
775775
expr = f"(idx->>'{idx_key}')::boolean"
776776
else:
777-
expr = f"idx->>'{idx_key}'"
777+
# jsonb operator `->` instead of text `->>` so PG uses type-aware
778+
# comparison: numbers sort numerically, strings lexicographically
779+
# (#158 — otherwise numeric FieldIndexes sort "10" < "2").
780+
expr = f"idx->'{idx_key}'"
778781

779782
parts.append(f"{expr} {direction}")
780783

tests/test_query.py

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -428,7 +428,14 @@ def test_sql_injection_in_path_rejected(self):
428428
class TestSort:
429429
def test_sort_ascending(self):
430430
qr = build_query({"portal_type": "Document", "sort_on": "sortable_title"})
431-
assert qr["order_by"] == "idx->>'sortable_title' ASC"
431+
assert qr["order_by"] == "idx->'sortable_title' ASC"
432+
433+
def test_sort_field_uses_jsonb_operator(self):
434+
"""FIELD sort must use `->` (jsonb) not `->>` (text) so numeric
435+
FieldIndexes sort numerically instead of lexicographically (#158)."""
436+
qr = build_query({"sort_on": "sortable_title"})
437+
assert "idx->'sortable_title'" in qr["order_by"]
438+
assert "idx->>'sortable_title'" not in qr["order_by"]
432439

433440
def test_sort_descending(self):
434441
qr = build_query(
@@ -481,7 +488,7 @@ def test_sort_on_list_two_fields(self):
481488
"sort_on": ["portal_type", "sortable_title"],
482489
}
483490
)
484-
assert qr["order_by"] == "idx->>'portal_type' ASC, idx->>'sortable_title' ASC"
491+
assert qr["order_by"] == "idx->'portal_type' ASC, idx->'sortable_title' ASC"
485492

486493
def test_sort_on_list_with_mixed_orders(self):
487494
qr = build_query(
@@ -494,7 +501,7 @@ def test_sort_on_list_with_mixed_orders(self):
494501
assert qr["order_by"] is not None
495502
parts = qr["order_by"].split(", ")
496503
assert len(parts) == 2
497-
assert parts[0] == "idx->>'sortable_title' ASC"
504+
assert parts[0] == "idx->'sortable_title' ASC"
498505
assert "pgcatalog_to_timestamptz" in parts[1]
499506
assert "DESC" in parts[1]
500507

@@ -506,7 +513,7 @@ def test_sort_on_list_single_order_applies_to_all(self):
506513
"sort_order": "descending",
507514
}
508515
)
509-
assert qr["order_by"] == "idx->>'portal_type' DESC, idx->>'sortable_title' DESC"
516+
assert qr["order_by"] == "idx->'portal_type' DESC, idx->'sortable_title' DESC"
510517

511518
def test_sort_on_list_with_unknown_index_skipped(self):
512519
qr = build_query(
@@ -515,7 +522,7 @@ def test_sort_on_list_with_unknown_index_skipped(self):
515522
"sort_on": ["nonexistent", "sortable_title"],
516523
}
517524
)
518-
assert qr["order_by"] == "idx->>'sortable_title' ASC"
525+
assert qr["order_by"] == "idx->'sortable_title' ASC"
519526

520527
def test_sort_on_list_all_unknown(self):
521528
qr = build_query(
@@ -529,7 +536,7 @@ def test_sort_on_list_all_unknown(self):
529536
def test_sort_on_single_string_still_works(self):
530537
"""Backward compat: single string sort_on unchanged."""
531538
qr = build_query({"portal_type": "Document", "sort_on": "sortable_title"})
532-
assert qr["order_by"] == "idx->>'sortable_title' ASC"
539+
assert qr["order_by"] == "idx->'sortable_title' ASC"
533540

534541

535542
# ---------------------------------------------------------------------------
@@ -742,13 +749,13 @@ def test_tgpath_multiple_paths_subtree(self):
742749

743750
def test_tgpath_sort(self):
744751
qr = build_query({"tgpath": "/uuid1", "sort_on": "tgpath"})
745-
assert qr["order_by"] == "idx->>'tgpath' ASC"
752+
assert qr["order_by"] == "idx->'tgpath' ASC"
746753

747754
def test_tgpath_sort_descending(self):
748755
qr = build_query(
749756
{"tgpath": "/uuid1", "sort_on": "tgpath", "sort_order": "descending"}
750757
)
751-
assert qr["order_by"] == "idx->>'tgpath' DESC"
758+
assert qr["order_by"] == "idx->'tgpath' DESC"
752759

753760
def test_builtin_path_uses_typed_columns(self):
754761
"""Built-in 'path' index dispatches to typed columns, not idx JSONB (#132)."""
@@ -1054,7 +1061,7 @@ def test_sort_dynamic_field(self, populated_registry):
10541061
get_registry().register("my_sort_field", IndexType.FIELD, "my_sort_field")
10551062

10561063
qr = build_query({"sort_on": "my_sort_field"})
1057-
assert qr["order_by"] == "idx->>'my_sort_field' ASC"
1064+
assert qr["order_by"] == "idx->'my_sort_field' ASC"
10581065

10591066
def test_sort_dynamic_date(self, populated_registry):
10601067
from plone.pgcatalog.columns import get_registry

tests/test_query_integration.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -455,3 +455,33 @@ def test_pagination(self, pg_conn_with_catalog):
455455
zoids1 = {r["zoid"] for r in rows1}
456456
zoids2 = {r["zoid"] for r in rows2}
457457
assert zoids1.isdisjoint(zoids2)
458+
459+
def test_sort_numeric_field_numerically(self, pg_conn_with_catalog):
460+
"""Numeric FieldIndex values must sort numerically, not lexically (#158).
461+
462+
Under text-cast sorting, 10 < 2 because "10" < "2" lexicographically.
463+
The jsonb `->` operator preserves type and sorts numbers as numbers.
464+
"""
465+
from plone.pgcatalog.columns import get_registry
466+
from plone.pgcatalog.columns import IndexType
467+
468+
conn = pg_conn_with_catalog
469+
get_registry().register("priority", IndexType.FIELD, "priority")
470+
471+
priorities = {200: 10, 201: 2, 202: 20, 203: 3, 204: 1}
472+
for zoid, priority in priorities.items():
473+
insert_object(conn, zoid=zoid)
474+
catalog_object(
475+
conn,
476+
zoid=zoid,
477+
path=f"/plone/item{zoid}",
478+
idx={"portal_type": "Item", "priority": priority},
479+
)
480+
conn.commit()
481+
482+
rows = execute_query(
483+
conn,
484+
{"portal_type": "Item", "sort_on": "priority"},
485+
columns="zoid",
486+
)
487+
assert [r["zoid"] for r in rows] == [204, 201, 203, 200, 202] # 1,2,3,10,20

0 commit comments

Comments
 (0)