From 45edca088438b98d386daf3251aa8d1a63848f22 Mon Sep 17 00:00:00 2001 From: Pierre Massat Date: Thu, 11 Jun 2026 10:14:55 -0700 Subject: [PATCH 1/2] feat(query): Reject non-datetime conditions on datetime columns DatetimeConditionValidator previously logged a warning and let invalid queries through when a datetime column was compared to a non-datetime literal. Raise InvalidQueryException so callers get a proper 4xx response instead of a silent log line. The datetime_condition_error metric is still emitted before the raise so we can keep tracking offending callers. The original TODO from 2023 asked to flip this to an exception once product teams stopped sending bad queries; the validator has been in log-only mode for ~3 years. Co-Authored-By: Claude Agent transcript: https://claudescope.sentry.dev/share/sAb6K7VallsoyAv5s7-QhOmxc8o6NXZEdzIk6akjxsY --- snuba/query/validation/validators.py | 8 ++------ .../test_datetime_condition_validator.py | 16 +++++----------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/snuba/query/validation/validators.py b/snuba/query/validation/validators.py index 0b6b046bd75..7c132c4c7f8 100644 --- a/snuba/query/validation/validators.py +++ b/snuba/query/validation/validators.py @@ -404,8 +404,6 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None: if isinstance(rhs, Literal): if not isinstance(rhs.value, datetime): 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,7 +411,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: '{rhs.value}' is not a valid datetime" ) elif isinstance(rhs, FunctionCall): @@ -423,8 +421,6 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None: param.value, datetime ): 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 +428,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..992a9f8a378 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( @@ -82,9 +83,6 @@ def test_datetime_column_validation(condition: Optional[Expression]) -> None: 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 +93,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) From 424608d2d407af73a471b616a638f1505e51bad2 Mon Sep 17 00:00:00 2001 From: Pierre Massat Date: Thu, 11 Jun 2026 20:19:14 -0700 Subject: [PATCH 2/2] fix(query): validate datetime conditions by parsing, not isinstance The previous check raised InvalidQueryException whenever a datetime column was compared to anything that wasn't a Python `datetime` instance. That rejected valid datetime *strings* -- e.g. `2026-06-11T17:36:06.001000+00:00` (Sentry event deletion) and `2026-06-11 14:00:00` (the legacy JSON query API) -- turning routine queries into 4xx/500s. Validate the value instead: a `datetime` instance or a string that parses via `datetime.fromisoformat` is accepted; everything else (notably bare epochs like `1726374214000`, the value behind SNUBA-ABR) is rejected. This keeps the exception behavior while shrinking the blast radius to genuinely-invalid conditions. Co-Authored-By: Claude Opus 4.8 (1M context) --- snuba/query/validation/validators.py | 24 +++++++- .../test_datetime_condition_validator.py | 56 ++++++++++++++++++- 2 files changed, 74 insertions(+), 6 deletions(-) diff --git a/snuba/query/validation/validators.py b/snuba/query/validation/validators.py index 7c132c4c7f8..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,7 +420,7 @@ 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") metrics.increment( "datetime_condition_error", @@ -417,8 +435,8 @@ def validate(self, query: Query, alias: Optional[str] = None) -> None: 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") metrics.increment( diff --git a/tests/datasets/validation/test_datetime_condition_validator.py b/tests/datasets/validation/test_datetime_condition_validator.py index 992a9f8a378..b8957495cc2 100644 --- a/tests/datasets/validation/test_datetime_condition_validator.py +++ b/tests/datasets/validation/test_datetime_condition_validator.py @@ -60,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"), @@ -77,11 +114,24 @@ 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: for condition, message in invalid_tests: query = LogicalQuery(