-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(tags): Handle status tag #106056
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(tags): Handle status tag #106056
Conversation
- The status field has unique handling in the Errors querybuilder where it becomes the group.status instead. But this means if a user sends an event with status as a tag the two get confused. This updates the tag endpoint so if we see `status` we assume the user doesn't want the group status but rather whatever status they sent us instead
| elif dataset == Dataset.Events and tag.key == "status": | ||
| tag.key = "tags[status]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Modifying tag.key to "tags[status]" causes the TagKeySerializer to produce an incorrect key and name in the API response, breaking the expected contract and causing test failures.
Severity: CRITICAL
🔍 Detailed Analysis
The code modifies tag.key to "tags[status]" when the dataset is Dataset.Events. This modified tag object is then passed to TagKeySerializer. The serializer processes this new key, resulting in an API response with {"key": "tags[status]", "name": "Tags[Status]"}. This contradicts the test expectations, which assert the response should be {"key": "status", "name": "Status"}. This discrepancy will cause the test_overlapping_tag test to fail and will likely break API clients or frontend components that depend on the "status" tag key.
💡 Suggested Fix
The modification of tag.key should be reconsidered. Instead of altering the object's property before serialization, the logic for handling the Dataset.Events case should be moved into the serializer or another appropriate layer to ensure the final output matches the API contract. Alternatively, if the new output is desired, the test expectations must be updated to match.
🤖 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_tags.py#L84-L85
Potential issue: The code modifies `tag.key` to `"tags[status]"` when the dataset is
`Dataset.Events`. This modified `tag` object is then passed to `TagKeySerializer`. The
serializer processes this new key, resulting in an API response with `{"key":
"tags[status]", "name": "Tags[Status]"}`. This contradicts the test expectations, which
assert the response should be `{"key": "status", "name": "Status"}`. This discrepancy
will cause the `test_overlapping_tag` test to fail and will likely break API clients or
frontend components that depend on the "status" tag key.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8427954
| assert data == [ | ||
| {"name": "Level", "key": "level", "totalValues": 1}, | ||
| {"name": "Status", "key": "status", "totalValues": 1}, | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test assertion expects wrong key after status transformation
High Severity
The test assertion expects "key": "status" and "name": "Status", but the code transforms the tag key from "status" to "tags[status]" when dataset == Dataset.Events. After serialization via TagKeySerializer, the output would have "key": "tags[status]" and "name": "Tags[Status]" (since the name is derived by calling .title() on the key). This test will fail because the expected values don't match the actual transformed values.
Additional Locations (1)
gggritso
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻
statuswe assume the user doesn't want the group status but rather whatever status they sent us instead