diff --git a/redisvl/query/filter.py b/redisvl/query/filter.py index 4f3b7a91..9d62c606 100644 --- a/redisvl/query/filter.py +++ b/redisvl/query/filter.py @@ -237,16 +237,122 @@ 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): + 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") + 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. 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 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}" + ) + return coerced + + class GeoSpec: - GEO_UNITS = ["m", "km", "mi", "ft"] + """The operand for a FilterExpression on a Geo field. + + Every argument is formatted into the query string, so each is coerced and + 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. + """ + + # 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) - # 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, 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( + 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. + """ + # 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 + raise ValueError(f"Unit must be one of {cls.GEO_UNITS}") class GeoRadius(GeoSpec): @@ -256,21 +362,55 @@ def __init__( self, longitude: float, latitude: float, - radius: int = 1, + radius: float = 1, unit: str = "km", ): """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 (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: - 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, 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) + 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. 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 def get_args(self) -> list[float | int | str]: @@ -279,15 +419,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)) @@ -465,25 +616,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/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 949cfb01..1c36f565 100644 --- a/tests/unit/test_filter.py +++ b/tests/unit/test_filter.py @@ -1,7 +1,10 @@ import calendar +import math import operator 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 @@ -11,6 +14,7 @@ FilterOperator, Geo, GeoRadius, + GeoSpec, Num, Tag, Text, @@ -395,17 +399,212 @@ 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 +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 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]"), + ({"longitude": 180, "latitude": -90}, "@geo_field:[180 -90 3 km]"), + ], + ids=[ + "uppercase_unit", + "numpy_scalars_are_real_but_not_int", + "str_overriding_subclasses", + "integral_float_radius", + "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), + # 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": 180.1}, ValueError), + ({"longitude": -180.1}, ValueError), + ({"latitude": 90.1}, ValueError), + ({"latitude": -90.1}, ValueError), + ({"longitude": 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), + # 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", + "latitude_injects", + "radius_injects", + "longitude_none", + "longitude_decimal", + "longitude_above_range", + "longitude_below_range", + "latitude_above_range", + "latitude_below_range", + "longitude_nan", + "radius_nan", + "longitude_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): + 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_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"): + _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_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"