diff --git a/snuba/query/validation/validators.py b/snuba/query/validation/validators.py index 0b6b046bd75..091e0f9cd73 100644 --- a/snuba/query/validation/validators.py +++ b/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,10 +420,8 @@ 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={ @@ -413,18 +429,16 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None: "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={ @@ -432,7 +446,7 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None: "entity": query.get_from_clause().key.value, }, ) - logger.warning( + raise InvalidQueryException( f"{lhs} requires datetime conditions: '{param.value}' is not a valid datetime" ) diff --git a/tests/datasets/validation/test_datetime_condition_validator.py b/tests/datasets/validation/test_datetime_condition_validator.py index 4725d6df52b..b8957495cc2 100644 --- a/tests/datasets/validation/test_datetime_condition_validator.py +++ b/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)