Merge pull request #151 from bluedynamics/fix/field-range-numeric-minmax

fix(query): _field_range numeric cast + min/max normalization (#150)

Author: jensens

CHANGES.md

@@ -4,6 +4,17 @@
 
 ### Fixed
 
+- ``_field_range`` on FieldIndex no longer silently returns zero rows
+  for numeric range queries.  Two stacked bugs: ``[max, min]`` order
+  from the caller was not normalized (produced always-false SQL), and
+  ``idx->>'key'`` comparisons ran lexicographically on text — so
+  ``'46.1' <= '5.0' <= '49.0'`` included values outside the range.
+  ``_field_range`` now sorts min/max and casts ``idx->>'key'`` to
+  ``::numeric`` when the range values are ``int`` / ``float``.  String
+  values (ISO dates etc.) keep text comparison.  Affected aaf-6 prod
+  map-widget bbox filter via ``collective.collectionfilter`` — closes
+  #150.
+
 - `_CatalogCompat.getIndex` and `_CatalogIndexesView.__getitem__` no
   longer silently fall back to the raw ZCatalog index when they
   cannot find the catalog tool — that fallback returned empty BTrees

src/plone/pgcatalog/query.py

@@ -77,6 +77,16 @@ def _bool_to_lower_str(value):
     return str(value)
 
 
+def _is_numeric_range(values):
+    """Return True iff every value is numeric (``int`` or ``float``).
+
+    ``bool`` is deliberately excluded even though Python treats it as
+    ``int`` — a boolean range is nonsensical and should use the plain
+    text path.
+    """
+    return all(isinstance(v, (int, float)) and not isinstance(v, bool) for v in values)
+
+
 def build_query(query_dict):
     """Translate a ZCatalog query dict into SQL components.
 
@@ -307,23 +317,44 @@ def _handle_field(self, name, idx_key, spec):
                 self.params[p] = _bool_to_lower_str(not_val)
 
     def _field_range(self, idx_key, value, range_spec):
+        """Emit a range clause for a FieldIndex.
+
+        Two correctness properties matter (see #150):
+
+        1. **Min/max normalization.**  ``plone.app.querystring`` and
+           ``collective.collectionfilter`` pass values in caller order,
+           not sorted.  ZCatalog's FieldIndex silently normalizes; we
+           must do the same or caller-supplied ``[max, min]`` produces
+           always-false SQL.
+        2. **Numeric cast.**  ``idx->>'key'`` returns ``text``; ``>=`` /
+           ``<=`` on text is lexicographic — ``'46.1' <= '5.0'`` is
+           true.  For numeric values we cast to ``::numeric`` so the
+           comparison is arithmetic.  String values keep plain text
+           comparison (correct for ISO-format dates and similar
+           lexicographically-orderable strings).
+        """
         if range_spec in ("min:max", "minmax") and isinstance(value, (list, tuple)):
+            v0, v1 = value[0], value[1]
+            v_min, v_max = (v0, v1) if v0 <= v1 else (v1, v0)
+            cast = "::numeric" if _is_numeric_range((v_min, v_max)) else ""
+            col = f"(idx->>'{idx_key}'){cast}" if cast else f"idx->>'{idx_key}'"
             p_min = self._pname(idx_key + "_min")
             p_max = self._pname(idx_key + "_max")
-            self.clauses.append(
-                f"(idx->>'{idx_key}' >= %({p_min})s"
-                f" AND idx->>'{idx_key}' <= %({p_max})s)"
-            )
-            self.params[p_min] = _bool_to_lower_str(value[0])
-            self.params[p_max] = _bool_to_lower_str(value[1])
+            self.clauses.append(f"({col} >= %({p_min})s AND {col} <= %({p_max})s)")
+            self.params[p_min] = v_min if cast else _bool_to_lower_str(v_min)
+            self.params[p_max] = v_max if cast else _bool_to_lower_str(v_max)
         elif range_spec == "min":
+            cast = "::numeric" if _is_numeric_range((value,)) else ""
+            col = f"(idx->>'{idx_key}'){cast}" if cast else f"idx->>'{idx_key}'"
             p = self._pname(idx_key)
-            self.clauses.append(f"idx->>'{idx_key}' >= %({p})s")
-            self.params[p] = _bool_to_lower_str(value)
+            self.clauses.append(f"{col} >= %({p})s")
+            self.params[p] = value if cast else _bool_to_lower_str(value)
         elif range_spec == "max":
+            cast = "::numeric" if _is_numeric_range((value,)) else ""
+            col = f"(idx->>'{idx_key}'){cast}" if cast else f"idx->>'{idx_key}'"
             p = self._pname(idx_key)
-            self.clauses.append(f"idx->>'{idx_key}' <= %({p})s")
-            self.params[p] = _bool_to_lower_str(value)
+            self.clauses.append(f"{col} <= %({p})s")
+            self.params[p] = value if cast else _bool_to_lower_str(value)
 
     # -- KeywordIndex -------------------------------------------------------
 

tests/test_query.py

@@ -882,6 +882,94 @@ def test_dynamic_field_multi_value(self, populated_registry):
         assert "= ANY(" in qr["where"]
 
 
+class TestFieldRangeNumeric:
+    """Regression for #150 — FieldIndex range on numeric fields.
+
+    Two stacked bugs the old ``_field_range`` had:
+
+    1. No min/max normalization: caller-supplied ``[max, min]`` produced
+       always-false SQL.  ``plone.app.querystring`` / ``collective.collectionfilter``
+       pass values in URL order and do not sort.
+    2. Lexicographic string comparison on ``idx->>'key'`` — wrong for
+       numeric fields (``'46.1' <= '5.0' <= '49.0'`` returns true).
+
+    Both shipped together — the map-widget bbox filter on aaf-6 silently
+    returned zero rows.
+    """
+
+    def _register(self, name):
+        from plone.pgcatalog.columns import get_registry
+        from plone.pgcatalog.columns import IndexType
+
+        get_registry().register(name, IndexType.FIELD, name)
+
+    def test_numeric_field_range_casts_to_numeric(self, populated_registry):
+        self._register("latitude")
+        qr = build_query({"latitude": {"query": [46.1, 49.0], "range": "minmax"}})
+        assert "(idx->>'latitude')::numeric" in qr["where"]
+        # min param gets the lower value, max param the upper — regardless
+        # of the caller-supplied order.
+        params = qr["params"]
+        min_key = next(k for k in params if k.endswith("_min_1"))
+        max_key = next(k for k in params if k.endswith("_max_2"))
+        assert params[min_key] == 46.1
+        assert params[max_key] == 49.0
+
+    def test_numeric_field_range_normalizes_reversed_order(self, populated_registry):
+        self._register("latitude")
+        # Caller sends [max, min] — collective.collectionfilter's map
+        # widget does this on every pan/zoom.
+        qr = build_query({"latitude": {"query": [49.0, 46.1], "range": "minmax"}})
+        params = qr["params"]
+        min_key = next(k for k in params if k.endswith("_min_1"))
+        max_key = next(k for k in params if k.endswith("_max_2"))
+        assert params[min_key] == 46.1
+        assert params[max_key] == 49.0
+
+    def test_numeric_field_range_int(self, populated_registry):
+        self._register("priority")
+        qr = build_query({"priority": {"query": [5, 1], "range": "minmax"}})
+        assert "(idx->>'priority')::numeric" in qr["where"]
+        params = qr["params"]
+        min_key = next(k for k in params if k.endswith("_min_1"))
+        max_key = next(k for k in params if k.endswith("_max_2"))
+        assert params[min_key] == 1
+        assert params[max_key] == 5
+
+    def test_numeric_field_range_min_only(self, populated_registry):
+        self._register("latitude")
+        qr = build_query({"latitude": {"query": 46.1, "range": "min"}})
+        assert "(idx->>'latitude')::numeric >=" in qr["where"]
+
+    def test_numeric_field_range_max_only(self, populated_registry):
+        self._register("latitude")
+        qr = build_query({"latitude": {"query": 49.0, "range": "max"}})
+        assert "(idx->>'latitude')::numeric <=" in qr["where"]
+
+    def test_string_field_range_keeps_text_comparison(self, populated_registry):
+        """Non-numeric values continue to use plain ``idx->>'key'``
+        comparison (correct for ISO-format dates and similar
+        lexicographically-orderable strings).
+        """
+        self._register("getId")
+        qr = build_query({"getId": {"query": ["a", "m"], "range": "minmax"}})
+        assert "::numeric" not in qr["where"]
+        assert "idx->>'getId' >=" in qr["where"]
+        assert "idx->>'getId' <=" in qr["where"]
+
+    def test_string_field_range_normalizes_order(self, populated_registry):
+        """Normalization applies regardless of type — ``_field_range`` sorts
+        caller-supplied values even for strings.
+        """
+        self._register("getId")
+        qr = build_query({"getId": {"query": ["m", "a"], "range": "minmax"}})
+        params = qr["params"]
+        min_key = next(k for k in params if k.endswith("_min_1"))
+        max_key = next(k for k in params if k.endswith("_max_2"))
+        assert params[min_key] == "a"
+        assert params[max_key] == "m"
+
+
 class TestDynamicKeywordIndex:
     """KeywordIndex dynamically registered via registry."""