From a7ce38e454dd58bc1f1f03b3c24adbfda704b73c Mon Sep 17 00:00:00 2001 From: Vineeth Voruganti <13438633+VVoruganti@users.noreply.github.com> Date: Tue, 28 Jul 2026 14:59:50 -0400 Subject: [PATCH] fix(filter): handle null operands and non-numeric columns in comparisons MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects in _build_comparison_conditions, both reachable from any route that accepts filters: 1. A null operand hit float(None), raising TypeError where only ValueError was caught — an unhandled 500. A null operand is a null check, not a value comparison, so {"ne": null} now compiles to IS NOT NULL and the other operators reject null with a 422. Equality against null already produced IS NULL via _build_field_condition. 2. Numeric operators float()-cast on every column type, so a string inequality on a text column ({"session_id": {"ne": "abc"}}) was rejected as an invalid number. Coercion is now gated on the column actually being numeric; text columns compare as text. Numeric columns still validate, and TypeError is caught alongside ValueError. Existing ne coverage only exercised the JSONB metadata path, which uses _safe_numeric_cast and handles strings — the scalar column path was untested. Adds cases for both. Co-Authored-By: Claude Opus 5 (1M context) --- src/utils/filter.py | 25 ++++++++++++++++++++++--- tests/utils/test_filter.py | 31 ++++++++++++++++++++++++++++++- 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/utils/filter.py b/src/utils/filter.py index e7d1764f..850fb70d 100644 --- a/src/utils/filter.py +++ b/src/utils/filter.py @@ -1,5 +1,6 @@ import datetime from collections.abc import Callable, Sequence +from decimal import Decimal from logging import getLogger from typing import Any, TypeVar from typing import cast as typing_cast @@ -566,6 +567,12 @@ def _build_comparison_conditions( column.type.python_type, datetime.datetime ) + # Numeric coercion applies only to actually-numeric columns. On a text + # column, `ne` is a string comparison, not a failed float parse. + is_numeric_column = hasattr(column.type, "python_type") and issubclass( + column.type.python_type, int | float | Decimal + ) + for operator, op_value in comparisons.items(): # Validate that the operator is supported if operator not in COMPARISON_OPERATORS: @@ -575,6 +582,17 @@ def _build_comparison_conditions( if op_value == "*": continue + # A null operand is a null check, not a value comparison, on every + # column type. Only `ne` is meaningful: {"col": None} already covers + # IS NULL via the equality path in _build_field_condition. + if op_value is None: + if operator != "ne": + raise FilterError( + f"Operator '{operator}' does not accept null. Use {{\"ne\": null}} for a not-null check, or null on its own for a null check." + ) + conditions.append(column.is_not(None)) + continue + condition = None # For datetime columns, cast string values to timestamp @@ -588,11 +606,12 @@ def _build_comparison_conditions( # Use the validated datetime object directly instead of string interpolation casted_value = validated_datetime else: - # if the operator is a numeric operator, the value must cast to a number - if operator in NUMERIC_OPERATORS: + # On a numeric column, a numeric operator's value must cast to a + # number. On a text column, `ne` is a string comparison. + if operator in NUMERIC_OPERATORS and is_numeric_column: try: casted_value = float(op_value) - except ValueError: + except (TypeError, ValueError): raise FilterError( f"Invalid numeric value: {op_value}. Expected a number, got {type(op_value).__name__}" ) from None diff --git a/tests/utils/test_filter.py b/tests/utils/test_filter.py index 49f84e23..140023c1 100644 --- a/tests/utils/test_filter.py +++ b/tests/utils/test_filter.py @@ -4,7 +4,7 @@ import pytest from sqlalchemy import select from src.exceptions import FilterError -from src.models import Document +from src.models import Document, Message from src.utils.filter import apply_filter @@ -29,3 +29,32 @@ def test_known_operator_dict_on_scalar_column_still_works(): def test_dict_on_jsonb_column_still_works(): stmt = apply_filter(select(Document), Document, {"metadata": {"kind": "note"}}) assert "internal_metadata" in str(stmt) + + +def test_ne_none_on_scalar_column_is_not_null(): + """Regression: float(None) raised TypeError, which the ValueError handler + missed -> unhandled 500.""" + stmt = apply_filter(select(Document), Document, {"session_id": {"ne": None}}) + assert "session_name IS NOT NULL" in str(stmt) + + +def test_ne_string_on_text_column_compares_as_string(): + """Regression: numeric operators float()-cast on every column type, so a + string inequality on a text column was rejected as a bad number.""" + stmt = apply_filter(select(Document), Document, {"session_id": {"ne": "abc"}}) + assert "session_name !=" in str(stmt) + + +def test_numeric_operator_still_validates_on_numeric_column(): + with pytest.raises(FilterError): + apply_filter(select(Message), Message, {"token_count": {"gt": "nope"}}) + + +def test_ne_none_on_numeric_column_is_not_null(): + stmt = apply_filter(select(Message), Message, {"token_count": {"ne": None}}) + assert "token_count IS NOT NULL" in str(stmt) + + +def test_null_operand_on_non_ne_operator_raises(): + with pytest.raises(FilterError): + apply_filter(select(Message), Message, {"token_count": {"gt": None}})