Skip to content

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

Open
phacops wants to merge 2 commits into
masterfrom
feat/datetime-condition-validator-raises
Open

feat(query): Reject non-datetime conditions on datetime columns#8020
phacops wants to merge 2 commits into
masterfrom
feat/datetime-condition-validator-raises

Conversation

@phacops

@phacops phacops commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Make DatetimeConditionValidator raise InvalidQueryException when a datetime column is compared to a non-datetime literal, instead of emitting a warning log and letting the query through. Callers now get a proper 4xx error rather than a silent log line that's easy to miss.

This was the plan from the start — the original commit (a4569bf, Feb 2023) introduced the validator in log-only "shadow mode" with a TODO to flip it to an exception once product teams stopped sending bad queries. That follow-up never happened and the TODO has been sitting there for ~3 years. We've had plenty of time to clean up offending callers.

The datetime_condition_error metric is still emitted before the raise so we can keep tracking who's sending bad queries (and, if this regresses anything, know where to look).

Risk

Any caller still relying on the previous log-and-pass behavior will now get a 4xx. Worth eyeballing the datetime_condition_error metric before merging to gauge blast radius.

Agent transcript: https://claudescope.sentry.dev/share/xh6_yT5vSneLsYnzPV2YBjhBu_yRkmI2iGsg42x6aeA

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 <noreply@anthropic.com>

Agent transcript: https://claudescope.sentry.dev/share/sAb6K7VallsoyAv5s7-QhOmxc8o6NXZEdzIk6akjxsY
@phacops phacops requested a review from a team as a code owner June 11, 2026 17:15
@MeredithAnya

Copy link
Copy Markdown
Member

@phacops looks like this is happening in the subscriptions executor SNUBA-ABR - will changing this to raise an exception crash the consumer? if so, we should fix that too

@phacops

phacops commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@phacops looks like this is happening in the subscriptions executor SNUBA-ABR - will changing this to raise an exception crash the consumer? if so, we should fix that too

It would, but there's nothing else to do in snuba to handle this. The problem is we didn't format the date we're comparing against properly when we prepared the query.

I regenerated the subscriptions and I think the error went away, making this safe to merge.

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) <noreply@anthropic.com>
@phacops

phacops commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — yes, as originally written it would have. In the executor, build_request() runs the entity validators inside the future, and poll()/join() only catch QueryException; InvalidQueryException is a sibling class, so it would propagate and crash the consumer.

Rather than swallow it in the executor, I traced SNUBA-ABR to the root cause:

  • The failing value 1726374214000 = 2024-09-15 is a frozen epoch baked into a stale error.received alert subscription, generated by pre-2026-01-12 Sentry code. That generation bug was already fixed in fix(errors): Add error.received to timestamp fields  sentry#105713 (added error.received to TIMESTAMP_FIELDS), so new subscriptions render proper datetimes.
  • The 38k/day occurrences were stale subscriptions cached in the Redis store from before that fix. Those have now been regenerated, so the executor won't see the bad query anymore.

I also hardened this validator (latest commit): instead of rejecting anything that isn't a Python datetime instance, it now parses the value — a datetime or a string that parses via datetime.fromisoformat is accepted, and only genuinely-invalid values (bare epochs like 1726374214000) raise. That was necessary because the strict version rejected valid datetime strings that Sentry (event deletion: timestamp = '…+00:00') and the legacy JSON API send routinely — caught by the sentry (N) and test_api CI jobs. Net effect: the exception only fires on conditions that were never valid, so the blast radius is much smaller.

@MeredithAnya

Copy link
Copy Markdown
Member

I regenerated the subscriptions and I think the error went away, making this safe to merge.

It's still happening so I think you should catch this in the subscriptions executor
https://sentry.sentry.io/issues/7310246411/?environment=us&project=300688&query=is%3Aunresolved&referrer=issue-stream

phacops commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

We're definitely not catching this in the executor. Also, the script to regenerate subscriptions didn't fully run.

I'd rather delete the one subscription that trigger this.

@MeredithAnya MeredithAnya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only merge when we see the bad subscription is gone, otherwise you will suffer consequences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants