From 0869a1f18e36c3d3fd5c260253ffa9d8f5b68252 Mon Sep 17 00:00:00 2001 From: limjoobin Date: Fri, 4 Sep 2026 16:54:20 +0800 Subject: [PATCH 1/3] fix: validate Geo filter arguments before they reach the query string --- redisvl/query/filter.py | 136 +++++++++++++++++++++++++++------- tests/unit/test_filter.py | 150 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 258 insertions(+), 28 deletions(-) diff --git a/redisvl/query/filter.py b/redisvl/query/filter.py index 4f3b7a91..465e70d6 100644 --- a/redisvl/query/filter.py +++ b/redisvl/query/filter.py @@ -237,16 +237,100 @@ def __str__(self) -> str: ) +def _coerce_to_number(value: Any, owner: str, name: str) -> int | float: + """Return a numeric filter value as a plain int or float. + + Coercion rather than the type check is the guard: every numeric value is + formatted into the query string, so a subclass overriding __str__ would + satisfy isinstance and inject syntax. int() and float() return builtins + regardless, which strips the override. + + A non-`numbers.Real` is rejected rather than converted, which is why + `Decimal` does not pass despite `float(Decimal("1.5"))` working: its exact + decimal arithmetic is different semantics, not a different spelling. + + ``owner`` and ``name`` only label the error -- pass the class that took the + value and the parameter it arrived as. + """ + if isinstance(value, numbers.Integral): + return int(value) + if isinstance(value, numbers.Real): + coerced = float(value) + if math.isnan(coerced): + # Renders `@field:[nan ...]`, which RediSearch rejects outright. + raise ValueError(f"{owner} {name} cannot be NaN") + return coerced + raise TypeError( + f"{owner} {name} must be an int, a float, or another " + f"numbers.Real; got {type(value).__name__}" + ) + + +def _coerce_to_number_within( + value: Any, owner: str, name: str, bounds: tuple[float, float] +) -> int | float: + """Coerce, then require a finite value inside ``bounds``, both ends included. + + Separate from _coerce_to_number because `Num` renders `-inf` and `+inf` by + design -- they are literal text in its own operator templates -- while a + coordinate has a domain and does not. + + `isfinite` is checked separately rather than left to the range, so an + infinite bound could not admit an infinite value. + """ + minimum, maximum = bounds + coerced = _coerce_to_number(value, owner, name) + if not math.isfinite(coerced) or not minimum <= coerced <= maximum: + raise ValueError( + f"{owner} {name} must be a finite number in " + f"[{minimum}, {maximum}]; got {coerced!r}" + ) + return coerced + + class GeoSpec: + """The operand for a FilterExpression on a Geo field. + + Every argument is formatted into the query string, so each is coerced and + range-checked here, at the constructor: the caller's own value selects or + scopes, and never itself renders. A `str` longitude used to interpolate raw, + so a value carrying `]` closed the geo clause and had its remainder parsed + as query syntax -- and an injected `|` binds looser than the implicit + space-AND, so it lifts to the root of the parse tree and any surrounding + filter stops constraining the query. + """ + GEO_UNITS = ["m", "km", "mi", "ft"] + LONGITUDE_RANGE = (-180.0, 180.0) + LATITUDE_RANGE = (-90.0, 90.0) - # class for the operand for FilterExpressions with Geo def __init__(self, longitude: float, latitude: float, unit: str = "km"): - if unit.lower() not in self.GEO_UNITS: - raise ValueError(f"Unit must be one of {self.GEO_UNITS}") - self._longitude = longitude - self._latitude = latitude - self._unit = unit.lower() + # Unit first, as before, so a caller passing both a bad unit and a bad + # coordinate still sees the error it saw yesterday. + self._unit = self._canonical_unit(unit) + owner = type(self).__name__ + self._longitude = _coerce_to_number_within( + longitude, owner, "longitude", self.LONGITUDE_RANGE + ) + self._latitude = _coerce_to_number_within( + latitude, owner, "latitude", self.LATITUDE_RANGE + ) + + @classmethod + def _canonical_unit(cls, unit: str) -> str: + """Return the library's own spelling of ``unit``. + + `str.lower` already returns a builtin `str`, so a `str` subclass + overriding `__str__` cannot reach the query string. An object whose + `lower()` and `__eq__` merely *match* a known unit still could, and + returning the matched element of GEO_UNITS closes that -- without an + isinstance check, which would reject a legitimate `str` subclass. + """ + requested = unit.lower() + for known in cls.GEO_UNITS: + if known == requested: + return known + raise ValueError(f"Unit must be one of {cls.GEO_UNITS}") class GeoRadius(GeoSpec): @@ -262,16 +346,24 @@ def __init__( """Create a GeoRadius specification (GeoSpec) Args: - longitude (float): The longitude of the center of the radius. - latitude (float): The latitude of the center of the radius. - radius (int, optional): The radius of the circle. Defaults to 1. + longitude (float): The longitude of the center of the radius, in + degrees, from -180 to 180. + latitude (float): The latitude of the center of the radius, in + degrees, from -90 to 90. + radius (int, optional): The radius of the circle, in ``unit``. + Defaults to 1. unit (str, optional): The unit of the radius. Defaults to "km". Raises: - ValueError: If the unit is not one of "m", "km", "mi", or "ft". + TypeError: If a coordinate or the radius is not an ``int``, a + ``float``, or another ``numbers.Real``. numpy scalars qualify; + ``Decimal`` and ``str`` do not. + ValueError: If a coordinate is NaN, infinite, or outside its range, + if the radius is NaN, or if the unit is not one of "m", "km", + "mi", or "ft". """ super().__init__(longitude, latitude, unit) - self._radius = radius + self._radius = _coerce_to_number(radius, type(self).__name__, "radius") def get_args(self) -> list[float | int | str]: return [self._longitude, self._latitude, self._radius, self._unit] @@ -465,25 +557,13 @@ def _validate_inclusive_string(inclusive: str) -> Inclusive: @classmethod def _coerce_numeric(cls, value: Any, name: str = "value") -> int | float: - """Return a numeric filter value as a plain int or float. + """Bind this class's name to the shared numeric coercion. - Coercion rather than the type check is the guard: every numeric value is - formatted into the query string, so a subclass overriding __str__ would - satisfy isinstance and inject syntax. int() and float() return builtins - regardless, which strips the override. + The seam a subclass would widen, and what lets ``Timestamp`` report its + own name through ``cls``. See ``_coerce_to_number`` for why coercion, + rather than the type check, is the guard. """ - if isinstance(value, numbers.Integral): - return int(value) - if isinstance(value, numbers.Real): - coerced = float(value) - if math.isnan(coerced): - # Renders `@field:[nan ...]`, which RediSearch rejects outright. - raise ValueError(f"{cls.__name__} {name} cannot be NaN") - return coerced - raise TypeError( - f"{cls.__name__} {name} must be an int, a float, or another " - f"numbers.Real; got {type(value).__name__}" - ) + return _coerce_to_number(value, cls.__name__, name) def _set_value( self, diff --git a/tests/unit/test_filter.py b/tests/unit/test_filter.py index 949cfb01..068f236e 100644 --- a/tests/unit/test_filter.py +++ b/tests/unit/test_filter.py @@ -1,7 +1,9 @@ import calendar +import math import operator import time as time_module from datetime import date, datetime, time, timedelta, timezone +from decimal import Decimal import numpy as np import pytest @@ -406,6 +408,154 @@ def test_geo_filter(operation, expected): assert str(getattr(geo_f, operation)(geo_radius)) == expected +def _geo_radius(**overrides) -> GeoRadius: + """A valid GeoRadius with arguments replaced, so a row names only what it changes.""" + return GeoRadius(**{"longitude": 1.0, "latitude": 2.0, "radius": 3, **overrides}) + + +@pytest.mark.parametrize( + "overrides, expected", + [ + # The unit that renders is GEO_UNITS' own spelling, not the caller's. + ({"unit": "KM"}, "@geo_field:[1.0 2.0 3 km]"), + # numpy integers are not `int` subclasses, so a concrete (int, float) + # check would reject them: `numbers.Real` is what keeps them working. + ( + { + "longitude": np.float64(1.0), + "latitude": np.int64(2), + "radius": np.int64(3), + }, + "@geo_field:[1.0 2 3 km]", + ), + # And why `numbers.Real` alone is not enough: every argument is + # formatted into the query string, so the type check admits a subclass + # that injects when rendered. Coercion is the guard, on both branches. + ( + {"longitude": _StrOverridingFloat(1.0), "radius": _StrOverridingInt(3)}, + "@geo_field:[1.0 2.0 3 km]", + ), + # The antimeridian and the poles are real places: the ranges include + # their endpoints. + ({"longitude": -180, "latitude": 90}, "@geo_field:[-180 90 3 km]"), + ({"longitude": 180, "latitude": -90}, "@geo_field:[180 -90 3 km]"), + ], + ids=[ + "uppercase_unit", + "numpy_scalars_are_real_but_not_int", + "str_overriding_subclasses", + "range_minimums", + "range_maximums", + ], +) +def test_geo_radius_renders_a_coerced_spec(overrides, expected): + """Asserted through `Geo`, because the rendering template is Geo's.""" + assert str(Geo("geo_field") == _geo_radius(**overrides)) == expected + + +@pytest.mark.parametrize( + "overrides, expected_error", + [ + # The bug. A `str` coordinate interpolated raw, so a value carrying `]` + # closed the geo clause and had its remainder parsed as syntax -- and an + # injected `|` lifts to the root of the parse tree, so a tenant filter + # sharing the query stops constraining it. + ({"longitude": "-122.4194 37.7749 10 km] | @secret:{leaked}"}, TypeError), + ({"latitude": "37.7749] | @secret:{leaked}"}, TypeError), + # `%i` refused a `str` radius, so it failed at render time with an + # obscure message; the coercion now refuses it at the caller's line. + ({"radius": "1 km] | @secret:{leaked}"}, TypeError), + ({"longitude": None}, TypeError), + # Registered as a `numbers.Number` but not a `numbers.Real`, which is + # what makes it the boundary case for the type the coercion accepts. + ({"longitude": Decimal("1.5")}, TypeError), + ({"longitude": [1.0]}, TypeError), + ({"longitude": 180.1}, ValueError), + ({"longitude": -180.1}, ValueError), + ({"latitude": 90.1}, ValueError), + ({"latitude": -90.1}, ValueError), + ({"longitude": float("nan")}, ValueError), + ({"latitude": float("nan")}, ValueError), + ({"radius": float("nan")}, ValueError), + # `Num` renders `-inf` and `+inf` by design -- they are literals in its + # own templates -- so this is the one rejection geo does not inherit. + ({"longitude": float("inf")}, ValueError), + ({"latitude": float("-inf")}, ValueError), + ({"unit": "parsec"}, ValueError), + ], + ids=[ + "longitude_injects", + "latitude_injects", + "radius_injects", + "longitude_none", + "longitude_decimal", + "longitude_list", + "longitude_above_range", + "longitude_below_range", + "latitude_above_range", + "latitude_below_range", + "longitude_nan", + "latitude_nan", + "radius_nan", + "longitude_infinite", + "latitude_infinite", + "unknown_unit", + ], +) +def test_geo_radius_refuses_an_unrenderable_argument(overrides, expected_error): + with pytest.raises(expected_error): + _geo_radius(**overrides) + + +def test_geo_radius_subclass_with_an_infinite_range_still_refuses_infinity(): + """`isfinite` is checked separately from the range, not implied by it. + + Unreachable through GeoSpec's own finite ranges -- `inf` already fails + `-180 <= v <= 180` -- so a subclass that widens a bound is the only way to + exercise the check, and the reason it is not left to the comparison. + """ + + class UnboundedGeoRadius(GeoRadius): + LONGITUDE_RANGE = (-math.inf, math.inf) + + with pytest.raises(ValueError, match="must be a finite number"): + UnboundedGeoRadius(float("inf"), 2.0, 3, "km") + + +def test_geo_radius_reports_the_unit_error_before_a_bad_coordinate(): + """Unit is validated first, so a caller sees the error they saw before.""" + with pytest.raises(ValueError, match="Unit must be one of"): + _geo_radius(longitude=9999, unit="parsec") + + +def test_geo_radius_renders_its_own_spelling_of_a_unit(): + """`GEO_UNITS`' literal renders, not the value that matched it. + + `str.lower` returns a builtin `str`, so a `str` subclass overriding + `__str__` never reaches the query string. An object that only *compares* + equal to a known unit does, since the membership test is that comparison -- + and returning the matched element closes it without an isinstance check + that would reject a legitimate `str` subclass. + """ + + class Kilometres: + def lower(self): + return self + + def __eq__(self, other): + return other == "km" + + def __hash__(self): + return hash("km") + + def __str__(self): + return "km] | @secret:{leaked}" + + rendered = str(Geo("geo_field") == _geo_radius(unit=Kilometres())) + + assert rendered == "@geo_field:[1.0 2.0 3 km]" + + def test_filters_combination(): tf1 = Tag("tag_field") == ["tag1", "tag2"] tf2 = Tag("tag_field") == "tag3" From 4581570928a7bb46e71c58a69d7dcf0d32db699f Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 4 Sep 2026 13:52:17 +0200 Subject: [PATCH 2/3] fix: stop the geo radius being truncated, and bound it `Geo.OPERATOR_MAP` rendered the radius with `%i`, which truncates toward zero. A 1.9 km radius silently queried 1 km, and 0.5 km rendered `0`, which the server answers `Invalid GeoFilter radius` -- so a sub-unit radius was inexpressible. Measured on 8.4.5 against a document 1.5 km out: `1 km` returns 1 hit, `1.9 km` returns 2. The radius now renders with `%s`, which is safe because `GeoRadius` coerces it to a builtin first. That coercion was already there and was already the real type guard; `%i` only looked like one by raising `TypeError` on a `str`. An integral float renders as an int, because `repr` switches to exponent form at 1e16 and `1e+16` is a syntax error at DIALECT 1, the server default. The radius is also bounded now. Zero, negative and infinite are refused at the constructor, where zero and negative previously built a query the server rejected and infinite escaped as an `OverflowError` from inside `Geo.__str__`. The bound is a chained comparison rather than `math.isfinite`, which raises `OverflowError` on an int too large to convert -- a radius has no upper bound to reject such a value first, unlike a coordinate. Four smaller corrections to the same change: - `_coerce_to_number_within` compares the range before `math.isfinite`, so an unconvertible int leaves as the documented `ValueError`, and `float()` is guarded for a `Real` that is finite but unrepresentable. - `GEO_UNITS` is an annotated tuple, so the allowlist is not mutable shared state and a subclass can still widen it. - A non-string unit leaves as the documented `ValueError`, found by lookup rather than by catching, so an `AttributeError` from inside a caller's own `lower()` still propagates. - The pole limit moves to the `Geo` class docstring, where it covers every geo query rather than contradicting the "-90 to 90" bullet it sat in. Every guard is mutation-checked by reverting it alone, both `OPERATOR_MAP` templates separately. The integration geo filter now uses a fractional radius, so a truncation regression fails against a real server. Co-authored-by: limjoobin --- redisvl/query/filter.py | 101 +++++++++++++++++++++++++------- tests/integration/test_query.py | 9 ++- tests/unit/test_filter.py | 73 +++++++++++++++++++---- 3 files changed, 146 insertions(+), 37 deletions(-) diff --git a/redisvl/query/filter.py b/redisvl/query/filter.py index 465e70d6..e391fb10 100644 --- a/redisvl/query/filter.py +++ b/redisvl/query/filter.py @@ -255,7 +255,15 @@ def _coerce_to_number(value: Any, owner: str, name: str) -> int | float: if isinstance(value, numbers.Integral): return int(value) if isinstance(value, numbers.Real): - coerced = float(value) + try: + coerced = float(value) + except OverflowError: + # A `Real` too large to convert, such as `Fraction(10**400, 1)`. + # Finite, but unrepresentable, so it leaves by the documented door + # rather than as an `OverflowError` from the conversion. + raise ValueError( + f"{owner} {name} is too large to represent as a float" + ) from None if math.isnan(coerced): # Renders `@field:[nan ...]`, which RediSearch rejects outright. raise ValueError(f"{owner} {name} cannot be NaN") @@ -276,11 +284,14 @@ def _coerce_to_number_within( coordinate has a domain and does not. `isfinite` is checked separately rather than left to the range, so an - infinite bound could not admit an infinite value. + infinite bound could not admit an infinite value. It is checked *second* + because `isfinite` itself raises `OverflowError` on an int too large to + convert, while the comparison handles one fine -- so the range rejects + `10**400` before finiteness is ever asked. """ minimum, maximum = bounds coerced = _coerce_to_number(value, owner, name) - if not math.isfinite(coerced) or not minimum <= coerced <= maximum: + if not minimum <= coerced <= maximum or not math.isfinite(coerced): raise ValueError( f"{owner} {name} must be a finite number in " f"[{minimum}, {maximum}]; got {coerced!r}" @@ -292,21 +303,24 @@ class GeoSpec: """The operand for a FilterExpression on a Geo field. Every argument is formatted into the query string, so each is coerced and - range-checked here, at the constructor: the caller's own value selects or - scopes, and never itself renders. A `str` longitude used to interpolate raw, - so a value carrying `]` closed the geo clause and had its remainder parsed - as query syntax -- and an injected `|` binds looser than the implicit - space-AND, so it lifts to the root of the parse tree and any surrounding - filter stops constraining the query. + checked at the constructor -- the coordinates and unit here, the radius in + `GeoRadius` -- so the caller's own value selects or scopes, and never itself + renders. Unchecked, a value carrying `]` would close the geo clause and + have its remainder parsed as query syntax, and an injected `|` binds looser + than the implicit space-AND, so it would lift to the root of the parse tree + and any surrounding filter would stop constraining the query. """ - GEO_UNITS = ["m", "km", "mi", "ft"] + # Immutable because it is public, shared, and interpolated into the error + # message, so a mutable default is state any caller could corrupt for every + # other. Annotated loosely so a subclass can still widen it. + GEO_UNITS: tuple[str, ...] = ("m", "km", "mi", "ft") LONGITUDE_RANGE = (-180.0, 180.0) LATITUDE_RANGE = (-90.0, 90.0) def __init__(self, longitude: float, latitude: float, unit: str = "km"): - # Unit first, as before, so a caller passing both a bad unit and a bad - # coordinate still sees the error it saw yesterday. + # Unit first, so which error a caller sees when more than one argument + # is bad is fixed rather than incidental. self._unit = self._canonical_unit(unit) owner = type(self).__name__ self._longitude = _coerce_to_number_within( @@ -326,7 +340,15 @@ def _canonical_unit(cls, unit: str) -> str: returning the matched element of GEO_UNITS closes that -- without an isinstance check, which would reject a legitimate `str` subclass. """ - requested = unit.lower() + # Looked up rather than caught, so an `AttributeError` raised from + # inside a caller's own `lower()` is not misreported as a bad unit. + # Still no isinstance check: anything that spells itself lowercase is + # welcome, and one that cannot is a bad value rather than an internal + # error, so it leaves by the documented door. + lower = getattr(unit, "lower", None) + if lower is None: + raise ValueError(f"Unit must be one of {cls.GEO_UNITS}") + requested = lower() for known in cls.GEO_UNITS: if known == requested: return known @@ -340,7 +362,7 @@ def __init__( self, longitude: float, latitude: float, - radius: int = 1, + radius: float = 1, unit: str = "km", ): """Create a GeoRadius specification (GeoSpec) @@ -350,8 +372,9 @@ def __init__( degrees, from -180 to 180. latitude (float): The latitude of the center of the radius, in degrees, from -90 to 90. - radius (int, optional): The radius of the circle, in ``unit``. - Defaults to 1. + radius (float, optional): The radius of the circle, in ``unit``, + greater than 0. Fractional radii are sent as given, so 0.5 with + a unit of "km" is half a kilometre. Defaults to 1. unit (str, optional): The unit of the radius. Defaults to "km". Raises: @@ -359,11 +382,34 @@ def __init__( ``float``, or another ``numbers.Real``. numpy scalars qualify; ``Decimal`` and ``str`` do not. ValueError: If a coordinate is NaN, infinite, or outside its range, - if the radius is NaN, or if the unit is not one of "m", "km", - "mi", or "ft". + if the radius is NaN, infinite, or not greater than 0, or if + the unit is not a string spelling one of "m", "km", "mi", or + "ft". """ super().__init__(longitude, latitude, unit) - self._radius = _coerce_to_number(radius, type(self).__name__, "radius") + owner = type(self).__name__ + radius = _coerce_to_number(radius, owner, "radius") + # Not `_coerce_to_number_within`: a radius is a positive magnitude + # rather than a bounded coordinate, so its lower bound is exclusive. + # Measured on 8.4.5, `@loc:[-122.4 37.7 0 km]` and the same with `-5` + # both answer `Invalid GeoFilter radius`. + # A comparison rather than `math.isfinite`, which raises `OverflowError` + # on an int too large to convert to a float. Chaining rejects zero, a + # negative, an infinity and a NaN, and admits a huge int, which is + # finite and renders exactly. + if not 0 < radius < math.inf: + raise ValueError( + f"{owner} radius must be a finite number greater than 0; " + f"got {radius!r}" + ) + if isinstance(radius, float) and radius.is_integer(): + # `repr` switches to exponent form at 1e16, and `@loc:[... 1e+20 km]` + # is a syntax error at DIALECT 1 -- the Redis 8 server default, which + # is what a rendered filter meets if it is run outside a RedisVL + # query class. Every float that large is integral, so an int renders + # the same value without an exponent. + radius = int(radius) + self._radius = radius def get_args(self) -> list[float | int | str]: return [self._longitude, self._latitude, self._radius, self._unit] @@ -371,15 +417,26 @@ def get_args(self) -> list[float | int | str]: class Geo(FilterField): """A Geo is a FilterField representing a geographic (lat/lon) field in a - Redis index.""" + Redis index. + + Note: + Redis indexes latitudes only within +/-85.05112878 degrees (EPSG:900913). + A document or a query center nearer a pole than that is silently + excluded: the query returns no error and no results, at any radius. + """ OPERATORS: dict[FilterOperator, str] = { FilterOperator.EQ: "==", FilterOperator.NE: "!=", } OPERATOR_MAP: dict[FilterOperator, str] = { - FilterOperator.EQ: "@%s:[%s %s %i %s]", - FilterOperator.NE: "(-@%s:[%s %s %i %s])", + # The third `%s` is the radius, and a string conversion is deliberate: + # an integer one truncates a fractional radius toward zero, and a + # sub-unit radius then renders `0`, which the server rejects. Nothing + # here guards the type -- `GeoRadius` coerces every argument to a + # builtin, and that coercion is the guard. + FilterOperator.EQ: "@%s:[%s %s %s %s]", + FilterOperator.NE: "(-@%s:[%s %s %s %s])", } SUPPORTED_VAL_TYPES = (GeoSpec, type(None)) diff --git a/tests/integration/test_query.py b/tests/integration/test_query.py index 39086199..1a282f67 100644 --- a/tests/integration/test_query.py +++ b/tests/integration/test_query.py @@ -428,11 +428,14 @@ def test_filters(index, query, sample_datetimes): n4 = Num("age") != 18 search(query, index, n4, 6, age_range=(0, 0, 18)) - # Geographic filters - g = Geo("location") == GeoRadius(-122.4194, 37.7749, 1, unit="m") + # Geographic filters. A fractional radius on purpose: truncating it would + # render `0`, which the server rejects outright, so this is what catches a + # radius that stops being sent as given. The matching documents sit on the + # centre, so a whole-kilometre radius would give the same counts. + g = Geo("location") == GeoRadius(-122.4194, 37.7749, 0.5, unit="km") search(query, index, g, 3, location="-122.4194,37.7749") - g = Geo("location") != GeoRadius(-122.4194, 37.7749, 1, unit="m") + g = Geo("location") != GeoRadius(-122.4194, 37.7749, 0.5, unit="km") search(query, index, g, 4, location="-110.0839,37.3861") # Text filters diff --git a/tests/unit/test_filter.py b/tests/unit/test_filter.py index 068f236e..1c36f565 100644 --- a/tests/unit/test_filter.py +++ b/tests/unit/test_filter.py @@ -4,6 +4,7 @@ import time as time_module from datetime import date, datetime, time, timedelta, timezone from decimal import Decimal +from fractions import Fraction import numpy as np import pytest @@ -13,6 +14,7 @@ FilterOperator, Geo, GeoRadius, + GeoSpec, Num, Tag, Text, @@ -397,13 +399,19 @@ def within(self, other): @pytest.mark.parametrize( "operation, expected", [ - ("__eq__", "@geo_field:[1.0 2.0 3 km]"), - ("__ne__", "(-@geo_field:[1.0 2.0 3 km])"), + ("__eq__", "@geo_field:[1.0 2.0 0.5 km]"), + ("__ne__", "(-@geo_field:[1.0 2.0 0.5 km])"), ], ids=["eq", "ne"], ) def test_geo_filter(operation, expected): - geo_radius = GeoRadius(1.0, 2.0, 3, "km") + """A fractional radius, so both templates pin the radius specifier. + + An integer conversion would truncate it, and only `__eq__` is asserted + anywhere else, so a whole radius here would let the `__ne__` template + regress unnoticed. + """ + geo_radius = GeoRadius(1.0, 2.0, 0.5, "km") geo_f = Geo("geo_field") assert str(getattr(geo_f, operation)(geo_radius)) == expected @@ -430,11 +438,16 @@ def _geo_radius(**overrides) -> GeoRadius: ), # And why `numbers.Real` alone is not enough: every argument is # formatted into the query string, so the type check admits a subclass - # that injects when rendered. Coercion is the guard, on both branches. + # that injects when rendered. Coercion is the guard on all three, the + # radius included, since the template converts it with `%s`. ( {"longitude": _StrOverridingFloat(1.0), "radius": _StrOverridingInt(3)}, "@geo_field:[1.0 2.0 3 km]", ), + # An integral float renders as an int. `repr` switches to exponent form + # at 1e16, and `1e+16` is a syntax error at DIALECT 1, the server + # default. + ({"radius": 1e16}, "@geo_field:[1.0 2.0 10000000000000000 km]"), # The antimeridian and the poles are real places: the ranges include # their endpoints. ({"longitude": -180, "latitude": 90}, "@geo_field:[-180 90 3 km]"), @@ -444,6 +457,7 @@ def _geo_radius(**overrides) -> GeoRadius: "uppercase_unit", "numpy_scalars_are_real_but_not_int", "str_overriding_subclasses", + "integral_float_radius", "range_minimums", "range_maximums", ], @@ -462,26 +476,40 @@ def test_geo_radius_renders_a_coerced_spec(overrides, expected): # sharing the query stops constraining it. ({"longitude": "-122.4194 37.7749 10 km] | @secret:{leaked}"}, TypeError), ({"latitude": "37.7749] | @secret:{leaked}"}, TypeError), - # `%i` refused a `str` radius, so it failed at render time with an - # obscure message; the coercion now refuses it at the caller's line. + # The radius renders through `%s`, so the coercion is the only thing + # standing between a `str` and the query string. Nothing in the + # template refuses one. ({"radius": "1 km] | @secret:{leaked}"}, TypeError), ({"longitude": None}, TypeError), # Registered as a `numbers.Number` but not a `numbers.Real`, which is # what makes it the boundary case for the type the coercion accepts. ({"longitude": Decimal("1.5")}, TypeError), - ({"longitude": [1.0]}, TypeError), ({"longitude": 180.1}, ValueError), ({"longitude": -180.1}, ValueError), ({"latitude": 90.1}, ValueError), ({"latitude": -90.1}, ValueError), ({"longitude": float("nan")}, ValueError), - ({"latitude": float("nan")}, ValueError), ({"radius": float("nan")}, ValueError), # `Num` renders `-inf` and `+inf` by design -- they are literals in its # own templates -- so this is the one rejection geo does not inherit. ({"longitude": float("inf")}, ValueError), - ({"latitude": float("-inf")}, ValueError), + # An int too large to convert to a float. The range is compared first + # because `isfinite` raises `OverflowError` on this value. + ({"longitude": 10**400}, ValueError), + # A `Real` that is finite but unrepresentable, so `float()` itself + # raises. The int path above never reaches the conversion. + ({"longitude": Fraction(10**400, 1)}, ValueError), + # Measured on 8.4.5: the server answers `Invalid GeoFilter radius` to a + # zero or negative radius, so both fail at the caller's line instead. + ({"radius": 0}, ValueError), + ({"radius": -5}, ValueError), + # An infinite radius the server would accept, but it is rejected for the + # same reason a coordinate is: the argument has to render as a number. + ({"radius": float("inf")}, ValueError), ({"unit": "parsec"}, ValueError), + # Nothing to lowercase is a bad value, not an internal error, so it + # leaves as the documented ValueError rather than an AttributeError. + ({"unit": None}, ValueError), ], ids=[ "longitude_injects", @@ -489,17 +517,20 @@ def test_geo_radius_renders_a_coerced_spec(overrides, expected): "radius_injects", "longitude_none", "longitude_decimal", - "longitude_list", "longitude_above_range", "longitude_below_range", "latitude_above_range", "latitude_below_range", "longitude_nan", - "latitude_nan", "radius_nan", "longitude_infinite", - "latitude_infinite", + "longitude_unconvertible_int", + "longitude_unconvertible_real", + "radius_zero", + "radius_negative", + "radius_infinite", "unknown_unit", + "unit_not_a_string", ], ) def test_geo_radius_refuses_an_unrenderable_argument(overrides, expected_error): @@ -522,6 +553,18 @@ class UnboundedGeoRadius(GeoRadius): UnboundedGeoRadius(float("inf"), 2.0, 3, "km") +def test_geo_radius_accepts_an_int_too_large_to_convert_to_a_float(): + """The radius guard compares rather than calling `math.isfinite`. + + `math.isfinite(10**400)` raises `OverflowError`, and there is no upper + bound to reject the value first, as there is for a coordinate. A huge int + is finite, so the documented contract admits it. + """ + rendered = str(Geo("geo_field") == _geo_radius(radius=10**400)) + + assert rendered == f"@geo_field:[1.0 2.0 {10**400} km]" + + def test_geo_radius_reports_the_unit_error_before_a_bad_coordinate(): """Unit is validated first, so a caller sees the error they saw before.""" with pytest.raises(ValueError, match="Unit must be one of"): @@ -556,6 +599,12 @@ def __str__(self): assert rendered == "@geo_field:[1.0 2.0 3 km]" +def test_geo_units_cannot_be_mutated(): + """The allowlist is public and shared, so it must not be a mutable default.""" + with pytest.raises(AttributeError): + GeoSpec.GEO_UNITS.append("parsec") # type: ignore[attr-defined] + + def test_filters_combination(): tf1 = Tag("tag_field") == ["tag1", "tag2"] tf2 = Tag("tag_field") == "tag3" From 6b53a4bd6a49efb5f5541ee28ddaeb9c1ae51084 Mon Sep 17 00:00:00 2001 From: Vishal Bala Date: Fri, 4 Sep 2026 14:16:06 +0200 Subject: [PATCH 3/3] docs: note that only a positive exponent breaks the geo radius The integral-float normalisation says `repr` switches to exponent form and that DIALECT 1 rejects it, without saying that this is true of a positive exponent only. Measured on 8.4.5, `@loc:[-122.4194 37.7749 1e-05 km]` parses at both dialects while `1e+16` is a syntax error at DIALECT 1, so a small radius needs no treatment and reaching for one would be wasted work. --- redisvl/query/filter.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/redisvl/query/filter.py b/redisvl/query/filter.py index e391fb10..9d62c606 100644 --- a/redisvl/query/filter.py +++ b/redisvl/query/filter.py @@ -407,7 +407,9 @@ def __init__( # is a syntax error at DIALECT 1 -- the Redis 8 server default, which # is what a rendered filter meets if it is run outside a RedisVL # query class. Every float that large is integral, so an int renders - # the same value without an exponent. + # the same value without an exponent. Only a positive exponent is a + # problem; `1e-05` parses at both dialects, so a small radius needs + # no treatment. radius = int(radius) self._radius = radius