feat(query): Reject non-datetime conditions on datetime columns

snuba/query/validation/validators.py

@@ -364,6 +364,24 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None:
                             )
 
 
+def _is_valid_datetime_literal(value: object) -> bool:
+    """
+    A datetime condition is valid when the literal is either a `datetime`
+    object or a string that parses as one (e.g. ``2023-01-25T20:03:13+00:00``,
+    which is how the legacy JSON API and Sentry routinely express datetimes).
+    A bare epoch like ``1726374214000`` does not parse and is rejected.
+    """
+    if isinstance(value, datetime):
+        return True
+    if isinstance(value, str):
+        try:
+            datetime.fromisoformat(value)
+        except ValueError:
+            return False
+        return True
+    return False
+
+
 class DatetimeConditionValidator(QueryValidator):
     def __init__(self) -> None:
         self.matchers: list[Or[Expression]] = []
@@ -402,37 +420,33 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None:
                     if match:
                         rhs = match.expression("rhs")
                         if isinstance(rhs, Literal):
-                            if not isinstance(rhs.value, datetime):
+                            if not _is_valid_datetime_literal(rhs.value):
                                 lhs = match.expression("column")
-                                # TODO: change this to a proper exception after ensuring the product isn't
-                                # passing bad queries
                                 metrics.increment(
                                     "datetime_condition_error",
                                     tags={
                                         "column": str(lhs),
                                         "entity": query.get_from_clause().key.value,
                                     },
                                 )
-                                logger.warning(
+                                raise InvalidQueryException(
                                     f"{lhs} requires datetime conditions: '{rhs.value}' is not a valid datetime"
                                 )
                         elif isinstance(rhs, FunctionCall):
                             # The matcher only matches array/tuples of literals
                             for param in rhs.parameters:
-                                if isinstance(param, Literal) and not isinstance(
-                                    param.value, datetime
+                                if isinstance(param, Literal) and not _is_valid_datetime_literal(
+                                    param.value
                                 ):
                                     lhs = match.expression("column")
-                                    # TODO: change this to a proper exception after ensuring the product isn't
-                                    # passing bad queries
                                     metrics.increment(
                                         "datetime_condition_error",
                                         tags={
                                             "column": str(lhs),
                                             "entity": query.get_from_clause().key.value,
                                         },
                                     )
-                                    logger.warning(
+                                    raise InvalidQueryException(
                                         f"{lhs} requires datetime conditions: '{param.value}' is not a valid datetime"
                                     )
 

tests/datasets/validation/test_datetime_condition_validator.py

@@ -1,6 +1,6 @@
 import datetime
+import re
 from typing import Optional
-from unittest.mock import MagicMock
 
 import pytest
 
@@ -9,9 +9,10 @@
 from snuba.query import SelectedExpression
 from snuba.query.conditions import binary_condition
 from snuba.query.data_source.simple import Entity as QueryEntity
+from snuba.query.exceptions import InvalidQueryException
 from snuba.query.expressions import Column, Expression, FunctionCall, Literal
 from snuba.query.logical import Query as LogicalQuery
-from snuba.query.validation.validators import DatetimeConditionValidator, logger
+from snuba.query.validation.validators import DatetimeConditionValidator
 
 required_column_tests = [
     pytest.param(
@@ -59,11 +60,48 @@ def test_datetime_column_validation(condition: Optional[Expression]) -> None:
         binary_condition(
             "equals",
             Column("_snuba_received", None, "received"),
-            Literal(None, "2023-01-25T20:03:13+00:00"),
+            # A bare epoch (here in milliseconds) is not a valid datetime.
+            Literal(None, "1726374214000"),
         ),
-        "received AS `_snuba_received` requires datetime conditions: '2023-01-25T20:03:13+00:00' is not a valid datetime",
+        "received AS `_snuba_received` requires datetime conditions: '1726374214000' is not a valid datetime",
     ),
     (
+        binary_condition(
+            "in",
+            Column("_snuba_received", None, "received"),
+            FunctionCall(
+                None,
+                "array",
+                (
+                    Literal(None, datetime.datetime.utcnow()),
+                    Literal(None, "not a datetime"),
+                ),
+            ),
+        ),
+        "received AS `_snuba_received` requires datetime conditions: 'not a datetime' is not a valid datetime",
+    ),
+]
+
+
+valid_datetime_string_tests = [
+    pytest.param(
+        binary_condition(
+            "equals",
+            Column("_snuba_received", None, "received"),
+            Literal(None, "2023-01-25T20:03:13+00:00"),
+        ),
+        id="ISO datetime string with timezone",
+    ),
+    pytest.param(
+        binary_condition(
+            "equals",
+            Column("_snuba_received", None, "received"),
+            # str(datetime), as emitted by the legacy JSON query API.
+            Literal(None, "2023-01-25 20:03:13"),
+        ),
+        id="space-separated datetime string",
+    ),
+    pytest.param(
         binary_condition(
             "in",
             Column("_snuba_received", None, "received"),
@@ -76,15 +114,25 @@ def test_datetime_column_validation(condition: Optional[Expression]) -> None:
                 ),
             ),
         ),
-        "received AS `_snuba_received` requires datetime conditions: '2023-02-25T20:03:13+00:00' is not a valid datetime",
+        id="array of datetime and datetime string",
     ),
 ]
 
 
+@pytest.mark.parametrize("condition", valid_datetime_string_tests)
+def test_valid_datetime_string_conditions(condition: Expression) -> None:
+    query = LogicalQuery(
+        QueryEntity(EntityKey.EVENTS, get_entity(EntityKey.EVENTS).get_data_model()),
+        selected_columns=[
+            SelectedExpression("time", Column("_snuba_timestamp", None, "timestamp")),
+        ],
+        condition=condition,
+    )
+
+    DatetimeConditionValidator().validate(query)
+
+
 def test_invalid_datetime_column_validation() -> None:
-    old_logger = logger.warning
-    mock_logger = MagicMock()
-    logger.warning = mock_logger  # type: ignore
     for condition, message in invalid_tests:
         query = LogicalQuery(
             QueryEntity(EntityKey.EVENTS, get_entity(EntityKey.EVENTS).get_data_model()),
@@ -95,9 +143,5 @@ def test_invalid_datetime_column_validation() -> None:
         )
 
         validator = DatetimeConditionValidator()
-        validator.validate(query)
-
-        mock_logger.assert_called_with(message)
-        mock_logger.reset_mock()
-
-    logger.warning = old_logger  # type: ignore
+        with pytest.raises(InvalidQueryException, match=re.escape(message)):
+            validator.validate(query)