feat(ai-conversation): Do not allow for querying more than 30d of data#107843
feat(ai-conversation): Do not allow for querying more than 30d of data#107843vgrozdanic merged 2 commits intomasterfrom
Conversation
| def test_stats_period_is_ignored(self) -> None: | ||
| """Test that statsPeriod parameter is ignored so old links still work""" | ||
| timestamp = before_now(days=90).replace(microsecond=0) | ||
| def test_stats_period_overrides_to_30d(self) -> None: |
There was a problem hiding this comment.
Just for my understanding: so no matter which request, we always query 30d?
There was a problem hiding this comment.
We ignore statsPeriod, but if start and end are present, we will respect them as long as they are not older than 30d.
We trust absolute time, but statsPeriod is often wrong
src/sentry/api/endpoints/organization_ai_conversation_details.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| if stats_period or not has_explicit_range: | ||
| # Always use full 30d range when statsPeriod is passed or no date params | ||
| snuba_params.start = max_retention_cutoff | ||
| snuba_params.end = now |
There was a problem hiding this comment.
Explicit date range ignored when statsPeriod also present
Medium Severity
When both statsPeriod and explicit start/end are provided, the condition if stats_period or not has_explicit_range always takes the override-to-30d branch because stats_period is truthy. This contradicts the stated intent from the PR discussion to respect explicit start/end dates. Additionally, because statsPeriod is no longer stripped from the request before calling get_snuba_params (the old code did this), get_snuba_params will compute dates from statsPeriod rather than start/end, so even fixing the condition alone wouldn't be sufficient — the snuba_params would still contain wrong dates.
Additional Locations (1)
There was a problem hiding this comment.
statsPeriod also has priority in snuba params, so we need to do this!
|
|
||
| # Enforce 30-day retention limit | ||
| max_retention = timedelta(days=MAX_RETENTION_DAYS) |
There was a problem hiding this comment.
Bug: When both statsPeriod and an explicit date range are provided, the explicit range is silently ignored, and the query defaults to a 30-day window due to incorrect conditional logic.
Severity: MEDIUM
Suggested Fix
To fix this, the conditional logic should be adjusted to handle this case correctly. Change the condition on line 78 from if stats_period or not has_explicit_range: to if stats_period and not has_explicit_range:. This ensures the 30-day override only applies when statsPeriod is present without an explicit range. Alternatively, return a 400 error if both sets of parameters are provided.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/api/endpoints/organization_ai_conversation_details.py#L72-L74
Potential issue: When both `statsPeriod` and explicit `start`/`end` date parameters are
provided to the endpoint, the logic incorrectly prioritizes `statsPeriod`. The condition
`if stats_period or not has_explicit_range:` evaluates to true, causing the code to
overwrite the user-provided explicit date range with a default 30-day window. This leads
to the explicit `start` and `end` parameters being silently ignored, and the API returns
data for an unintended time range. The expected behavior would be to either prioritize
the explicit date range or return an error for the conflicting parameters.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
statsPeriod also has priority in snuba params, so we need to do this!
#107843) We handle this on frontend, but just to be extra safe we should also handle this on the backend as well. Closes [TET-1898: EAP going to downsampled tier for `statsPeriod` older than 30d](https://linear.app/getsentry/issue/TET-1898/eap-going-to-downsampled-tier-for-statsperiod-older-than-30d)


We handle this on frontend, but just to be extra safe we should also handle this on the backend as well.
Closes TET-1898: EAP going to downsampled tier for
statsPeriodolder than 30d