diff --git a/src/sentry/api/endpoints/organization_events_timeseries.py b/src/sentry/api/endpoints/organization_events_timeseries.py index 73803e3496ba39..fe886a579e8725 100644 --- a/src/sentry/api/endpoints/organization_events_timeseries.py +++ b/src/sentry/api/endpoints/organization_events_timeseries.py @@ -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]: + raise ParseError("Cannot order by timestamp") if dataset in RPC_DATASETS: return dataset.run_top_events_timeseries_query( params=snuba_params, diff --git a/src/sentry/snuba/rpc_dataset_common.py b/src/sentry/snuba/rpc_dataset_common.py index 7897a4c18002d5..1286af420dbee6 100644 --- a/src/sentry/snuba/rpc_dataset_common.py +++ b/src/sentry/snuba/rpc_dataset_common.py @@ -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( diff --git a/tests/snuba/api/endpoints/test_organization_events_timeseries_spans.py b/tests/snuba/api/endpoints/test_organization_events_timeseries_spans.py index b9f2feb563461c..f159d73aaa2236 100644 --- a/tests/snuba/api/endpoints/test_organization_events_timeseries_spans.py +++ b/tests/snuba/api/endpoints/test_organization_events_timeseries_spans.py @@ -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( [