Skip to content
Merged
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
4 changes: 4 additions & 0 deletions src/sentry/api/endpoints/organization_events_timeseries.py
Original file line number Diff line number Diff line change
Expand Up @@ -252,10 +252,14 @@ def get_rpc_config():

if top_events > 0:
raw_groupby = self.get_field_list(organization, request, param_name="groupBy")
raw_orderby = self.get_orderby(request)
if len(raw_groupby) == 0:
raise ParseError("groupBy is a required parameter when doing topEvents")
if "timestamp" in raw_groupby:
raise ParseError("Cannot group by timestamp")
if raw_orderby:
if "timestamp" in [col.strip("-") for col in raw_orderby]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Incorrect use of strip instead of lstrip

The code uses strip("-") to remove the direction prefix from orderby columns, but this removes both leading and trailing hyphens. This differs from the standard pattern used throughout the codebase (like in rpc_dataset_common.py line 263) which uses lstrip("-") to only remove leading hyphens. If an orderby field name ends with a hyphen, strip would incorrectly remove it, causing the timestamp check to potentially fail or match the wrong field name.

Fix in Cursor Fix in Web

raise ParseError("Cannot order by timestamp")
if dataset in RPC_DATASETS:
return dataset.run_top_events_timeseries_query(
params=snuba_params,
Expand Down
3 changes: 3 additions & 0 deletions src/sentry/snuba/rpc_dataset_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,9 @@ def get_table_rpc_request(cls, query: TableQuery) -> TableRequest:
stripped_orderby = orderby_column.lstrip("-")
if stripped_orderby in orderby_aliases:
resolved_column = orderby_aliases[stripped_orderby]
# If this orderby isn't in the aliases, check if its a selected column
elif stripped_orderby not in query.selected_columns:
raise InvalidSearchQuery("orderby must also be in the selected columns or groupby")
else:
resolved_column = resolver.resolve_column(stripped_orderby)[0]
resolved_orderby.append(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,69 @@ def test_top_events(self) -> None:
"order": 2,
}

def test_top_events_orderby_not_in_groupby(self) -> None:
self.store_spans(
[
self.create_span(
{"sentry_tags": {"transaction": "foo", "status": "success"}},
start_ts=self.start + timedelta(minutes=1),
duration=2001,
),
self.create_span(
{"sentry_tags": {"transaction": "bar", "status": "success"}},
start_ts=self.start + timedelta(minutes=1),
duration=2000,
),
self.create_span(
{"sentry_tags": {"transaction": "baz", "status": "success"}},
start_ts=self.start + timedelta(minutes=1),
duration=1999,
),
self.create_span(
{"sentry_tags": {"transaction": "qux", "status": "success"}},
start_ts=self.start + timedelta(minutes=1),
duration=1998,
),
],
is_eap=True,
)

self.end = self.start + timedelta(minutes=6)
response = self._do_request(
data={
"start": self.start,
"end": self.end,
"interval": "1m",
"yAxis": "count()",
"groupBy": ["transaction"],
"orderby": ["-sum(span.self_time)"],
"project": self.project.id,
"dataset": "spans",
"excludeOther": 0,
"topEvents": 2,
},
)
assert response.status_code == 400, response.content
assert "orderby must also be in the selected columns or groupby" == response.data["detail"]

def test_top_events_orderby_is_timestamp(self) -> None:
response = self._do_request(
data={
"start": self.start,
"end": self.end,
"interval": "1m",
"yAxis": "count()",
"groupBy": ["transaction"],
"orderby": ["timestamp"],
"project": self.project.id,
"dataset": "spans",
"excludeOther": 0,
"topEvents": 2,
},
)
assert response.status_code == 400, response.content
assert "Cannot order by timestamp" == response.data["detail"]

def test_top_events_with_none_as_groupby(self) -> None:
self.store_spans(
[
Expand Down
Loading