fix(filter): handle null operands and non-numeric columns in comparisons
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) <noreply@anthropic.com>
This commit is contained in:
parent
5922f617ef
commit
a7ce38e454
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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}})
|
||||
|
|
|
|||
Loading…
Reference in New Issue