fix: validate Geo filter arguments before they reach the query string

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,

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