Browse Source

feat(widget-builder): Pass validation if widget orders by unselected function (#34016)

Nar Saynorath 2 years ago
parent
commit
3acbcdd3ff

+ 13 - 6
src/sentry/api/serializers/rest_framework/dashboard.py

@@ -17,6 +17,7 @@ from sentry.models import (
     DashboardWidgetTypes,
 )
 from sentry.search.events.builder import UnresolvedQuery
+from sentry.search.events.fields import is_function
 from sentry.snuba.dataset import Dataset
 from sentry.utils.dates import parse_stats_period
 
@@ -145,17 +146,23 @@ class DashboardWidgetQuerySerializer(CamelSnakeSerializer):
         aggregates = self._get_attr(data, "aggregates", []).copy()
         fields = columns + aggregates
 
-        injected_orderby_equation = None
-        if is_equation(orderby.lstrip("-")):
-            injected_orderby_equation = orderby.lstrip("-")
+        # Handle the orderby since it can be a value that's not included in fields
+        # e.g. a custom equation, or a function that isn't plotted as a y-axis
+        injected_orderby_equation, orderby_prefix = None, None
+        stripped_orderby = orderby.lstrip("-")
+        if is_equation(stripped_orderby):
+            # The orderby is a custom equation and needs to be added to fields
+            injected_orderby_equation = stripped_orderby
             fields.append(injected_orderby_equation)
-            equations, fields = categorize_columns(fields)
             orderby_prefix = "-" if orderby.startswith("-") else ""
+        elif is_function(stripped_orderby) and stripped_orderby not in fields:
+            fields.append(stripped_orderby)
 
+        equations, fields = categorize_columns(fields)
+
+        if injected_orderby_equation is not None and orderby_prefix is not None:
             # Subtract one because the equation is injected to fields
             orderby = f"{orderby_prefix}equation[{len(equations) - 1}]"
-        else:
-            equations, fields = categorize_columns(fields)
 
         try:
             parse_search_query(conditions)

+ 50 - 1
tests/sentry/api/endpoints/test_organization_dashboard_widget_details.py

@@ -480,7 +480,7 @@ class OrganizationDashboardWidgetDetailsTestCase(OrganizationDashboardWidgetTest
         assert response.status_code == 400, response.data
         assert "queries" in response.data, response.data
 
-    def test_save_with_no_fields_and_orderby(self):
+    def test_save_with_orderby_from_columns(self):
         data = {
             "title": "Test Query",
             "displayType": "line",
@@ -503,3 +503,52 @@ class OrganizationDashboardWidgetDetailsTestCase(OrganizationDashboardWidgetTest
             data=data,
         )
         assert response.status_code == 200, response.data
+
+    def test_save_with_orderby_not_from_columns_or_aggregates(self):
+        data = {
+            "title": "Test Query",
+            "displayType": "line",
+            "widgetType": "discover",
+            "limit": 5,
+            "queries": [
+                {
+                    "name": "",
+                    "conditions": "",
+                    "fields": [],
+                    "columns": ["project"],
+                    "aggregates": ["count()"],
+                    "orderby": "-epm()",
+                }
+            ],
+        }
+        response = self.do_request(
+            "post",
+            self.url(),
+            data=data,
+        )
+        assert response.status_code == 200, response.data
+
+    def test_save_with_invalid_orderby_not_from_columns_or_aggregates(self):
+        data = {
+            "title": "Test Query",
+            "displayType": "line",
+            "widgetType": "discover",
+            "limit": 5,
+            "queries": [
+                {
+                    "name": "",
+                    "conditions": "",
+                    "fields": [],
+                    "columns": ["project"],
+                    "aggregates": ["count()"],
+                    "orderby": "-eeeeeeeepm()",
+                }
+            ],
+        }
+        response = self.do_request(
+            "post",
+            self.url(),
+            data=data,
+        )
+        assert response.status_code == 400, response.data
+        assert "queries" in response.data, response.data