feat(discover): add starred manager and endpoint for discover saved queries - #123795
Conversation
github-actions
Bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?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.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 02a75c7. Configure here.
| }, | ||
| ) | ||
|
|
||
| assert response.status_code == 400 |
There was a problem hiding this comment.
Actually good point, we can keep it to test the serializer is catching it
|
|
||
| with pytest.raises(ValueError, match="multiple positions"): | ||
| utils.reorder_starred_queries(self.org, self.user.id, [ref, ref]) | ||
|
|
There was a problem hiding this comment.
It's not handled by the util function anymore
| requested = list(refs) | ||
| if len(requested) != len(set(requested)): | ||
| raise ValueError("Single query cannot take up multiple positions.") | ||
| new_query_positions = list(refs) |
There was a problem hiding this comment.
is this being removed because it's done somewhere else?
There was a problem hiding this comment.
The SavedQueryStarredOrderSerializer handles it, so it was redundant
|
|
||
|
|
||
| @cell_silo_endpoint | ||
| class DiscoverSavedQueryStarredEndpoint(OrganizationEndpoint): |
There was a problem hiding this comment.
are we able to put the actual endpoint in a different pr? in my head that makes sense but maybe i'm missing the reason we need it now.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| new_query_positions = list(refs) | ||
|
|
||
| # grab all starred queries in both tables, and map based on SavedQueryRef. | ||
| discover_starred_queries = DiscoverSavedQueryStarred.objects.filter( | ||
| organization=organization, user_id=user_id, position__isnull=False | ||
| ).filter(organization=organization, user_id=user_id, position__isnull=False, starred=True) | ||
| organization=organization, user_id=user_id, position__isnull=False, starred=True |
There was a problem hiding this comment.
Bug: The reorder_starred_queries function incorrectly handles duplicate query references, as a flawed validation and dictionary comprehension lead to corrupted, non-contiguous position data being saved.
Severity: MEDIUM
Suggested Fix
Strengthen the validation within reorder_starred_queries to detect duplicates in the new_query_positions list before creating the position_map. A check like if len(new_query_positions) != len(set(new_query_positions)) should be added to the function to either raise a ValueError or to deduplicate the list explicitly, depending on the desired behavior.
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/explore/utils.py#L95-L99
Potential issue: The function `reorder_starred_queries` incorrectly handles duplicate
query references in its input. The validation check `existing_query_refs !=
set(new_query_positions)` fails to detect duplicates because it converts the input list
to a set, eliminating duplicates before comparison. Subsequently, a dictionary
comprehension `position_map = {ref: position ...}` is used to assign positions, but it
silently overwrites the position for a duplicate reference with the last one it
encounters. This results in non-contiguous position values (e.g., positions 2 and 3 for
two queries) being saved to the database, corrupting the user's starred query order.
This can be triggered by any internal caller that bypasses the serializer validation
present in the HTTP endpoint.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
THE SERIALIZER DOES THIS
2 participants
Changes
Addresses part of EXP-1166 in an effort to move remaining discover saved queries to All Queries tables. This PR adds/changes: