fix(ai-conversations): ignore statsPeriod for conversation details endpoint#107411
fix(ai-conversations): ignore statsPeriod for conversation details endpoint#107411vgrozdanic merged 3 commits intomasterfrom
Conversation
github-actions
bot
added
the
Scope: Backend
| return Response(status=404) | ||
|
|
||
| # Ignore statsPeriod so old links don't fail when opened later | ||
| request.GET = request.GET.copy() |
There was a problem hiding this comment.
we need to do this because request.GET has immutable dict by default, and we want to pop statsPeriod parameter for this test
There was a problem hiding this comment.
I think it's fine like this for our test, but we should also probably later ignore absolute time params like start and end
There was a problem hiding this comment.
Yep, if this is too slow, my next try will be to use those params to cap the time range that needs to be scanned (in UI we will define the start to know at least when was the first span we saw).
But I hope that scanning via conversation_id will be fast enough, even if we scan all 90d of data 🤞
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
| # Ignore statsPeriod so old links don't fail when opened later | ||
| mutable_query = request.GET.copy() | ||
| mutable_query.pop("statsPeriod", None) | ||
| request.GET = mutable_query # type: ignore[assignment] |
There was a problem hiding this comment.
Time parameter alias timeframe not removed with statsPeriod
Low Severity
The fix removes statsPeriod from request parameters but doesn't remove timeframe, which is aliased to statsPeriod in get_date_range_from_params. Any old links using timeframe instead of statsPeriod would still have their time range applied, defeating the purpose of the fix for those cases. Similarly, statsPeriodStart/statsPeriodEnd and timeframeStart/timeframeEnd parameters are not handled.
| query = { | ||
| "project": [self.project.id], | ||
| "start": (timestamp - timedelta(hours=1)).isoformat(), | ||
| "end": (timestamp + timedelta(hours=1)).isoformat(), | ||
| "statsPeriod": "1h", # This should be ignored | ||
| } | ||
|
|
||
| response = self.do_request(conversation_id, query) | ||
| assert response.status_code == 200, response.data | ||
| assert len(response.data) == 1 | ||
| assert response.data[0]["gen_ai.conversation.id"] == conversation_id |
There was a problem hiding this comment.
Bug: The attempt to remove statsPeriod by reassigning request.GET will silently fail because request.GET is a read-only property, rendering the intended fix ineffective.
Severity: HIGH
Suggested Fix
Instead of attempting to modify request.GET directly, create a mutable copy of the query parameters, remove statsPeriod from that copy, and then pass the modified dictionary to the functions that require it, such as self.get_snuba_params. Do not reassign request.GET.
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:
tests/sentry/api/endpoints/test_organization_ai_conversation_details.py#L388-L417
Potential issue: The code attempts to remove the `statsPeriod` parameter from the
request by creating a mutable copy of `request.GET`, removing the key, and then
reassigning the copy back to `request.GET`. However, `request.GET` is a read-only
property in Django/DRF, so this assignment silently fails. As a result, `statsPeriod` is
never actually removed from the request parameters. When
`get_date_range_from_stats_period` is subsequently called, it will prioritize the
still-present `statsPeriod` over the explicit `start` and `end` parameters due to its
`elif` structure, which contradicts the intended logic of the change. The fix is
therefore ineffective.
…dpoint (#107411) This is a temporal fix to ignore `statsPeriod` paremeter in request to make sure it doesn't influence the period of scanned data. Since this endpoint is only available to internal organizations, we will roll this change out and see if it is fast enough to keep it like this.
…dpoint (#107411) This is a temporal fix to ignore `statsPeriod` paremeter in request to make sure it doesn't influence the period of scanned data. Since this endpoint is only available to internal organizations, we will roll this change out and see if it is fast enough to keep it like this.
2 participants
This is a temporal fix to ignore
statsPeriodparemeter in request to make sure it doesn't influence the period of scanned data.Since this endpoint is only available to internal organizations, we will roll this change out and see if it is fast enough to keep it like this.