Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions src/sentry/api/endpoints/organization_tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,23 @@ def get(self, request: Request, organization: Organization) -> Response:

# Filter out device.class from tags since it's already specified as a field in the frontend.
# This prevents the tag from being displayed twice.
results = [tag for tag in results if tag.key != "device.class"]
final_results = []
for tag in results:
if tag.key == "device.class":
continue
# the events dataset has special handling of the column "status" that breaks when users also send
# the tag "status". So we need to be explicit its a tag in these cases
elif dataset == Dataset.Events and tag.key == "status":
tag.key = "tags[status]"
Comment on lines +84 to +85
Copy link

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

final_results.append(tag)

# Setting the tag for now since the measurement is still experimental
sentry_sdk.set_tag("custom_tags.count", len(results))
sentry_sdk.set_tag("custom_tags.count", len(final_results))
sentry_sdk.set_tag(
"custom_tags.count.grouped",
format_grouped_length(len(results), [1, 10, 50, 100]),
format_grouped_length(len(final_results), [1, 10, 50, 100]),
)
sentry_sdk.set_tag("dataset_queried", dataset.value)
set_span_attribute("custom_tags.count", len(results))
set_span_attribute("custom_tags.count", len(final_results))

return Response(serialize(results, request.user))
return Response(serialize(final_results, request.user))
32 changes: 32 additions & 0 deletions tests/snuba/api/endpoints/test_organization_tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,38 @@ def test_different_times_retrieves_cache(self) -> None:

assert original_data == cached_data

def test_overlapping_tag(self) -> None:
user = self.create_user()
org = self.create_organization()
team = self.create_team(organization=org)
self.create_member(organization=org, user=user, teams=[team])

self.login_as(user=user)

project = self.create_project(organization=org, teams=[team])
self.store_event(
data={
"event_id": "a" * 32,
"tags": {"status": "404", "project": "test"},
"timestamp": self.min_ago,
},
project_id=project.id,
)

url = reverse(
"sentry-api-0-organization-tags", kwargs={"organization_id_or_slug": org.slug}
)

response = self.client.get(url, {"statsPeriod": "14d", "dataset": "events"}, format="json")

assert response.status_code == 200, response.content
data = response.data
data.sort(key=lambda val: val["name"])
assert data == [
{"name": "Level", "key": "level", "totalValues": 1},
{"name": "Status", "key": "status", "totalValues": 1},
]
Copy link
Contributor

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)

Fix in Cursor Fix in Web



class ReplayOrganizationTagsTest(APITestCase, ReplaysSnubaTestCase):
def test_dataset_replays(self) -> None:
Expand Down
Loading