feat(dashboards) Improve validation for dashboard update (#22239)

Add validation for field list, conditions and intervals on queries.

src/sentry/api/serializers/rest_framework/dashboard.py

@@ -4,11 +4,17 @@ from django.db.models import Max
 from rest_framework import serializers
 
 from sentry.api.serializers.rest_framework import CamelSnakeSerializer
+from sentry.api.event_search import (
+    resolve_field_list,
+    get_filter,
+    InvalidSearchQuery,
+)
 from sentry.models import (
     DashboardWidget,
     DashboardWidgetQuery,
     DashboardWidgetDisplayTypes,
 )
+from sentry.utils.dates import parse_stats_period
 
 
 def get_next_dashboard_order(dashboard_id):
@@ -46,6 +52,26 @@ class DashboardWidgetQuerySerializer(CamelSnakeSerializer):
 
     validate_id = validate_id
 
+    def validate_fields(self, fields):
+        snuba_filter = get_filter("")
+        try:
+            resolve_field_list(fields, snuba_filter)
+            return fields
+        except InvalidSearchQuery as err:
+            raise serializers.ValidationError(u"Invalid fields: {}".format(err))
+
+    def validate_conditions(self, conditions):
+        try:
+            get_filter(conditions)
+        except InvalidSearchQuery as err:
+            raise serializers.ValidationError(u"Invalid conditions: {}".format(err))
+        return conditions
+
+    def validate_interval(self, interval):
+        if parse_stats_period(interval) is None:
+            raise serializers.ValidationError("Invalid interval")
+        return interval
+
     def validate(self, data):
         if not data.get("id"):
             keys = set(data.keys())

tests/sentry/api/endpoints/test_organization_dashboard_details.py

@@ -1,7 +1,6 @@
 from __future__ import absolute_import
 
 import six
-import pytest
 
 from django.core.urlresolvers import reverse
 from sentry.models import (
@@ -225,13 +224,97 @@ class OrganizationDashboardDetailsPutTest(OrganizationDashboardDetailsTestCase):
         assert len(queries) == 1
         self.assert_serialized_widget_query(data["widgets"][4]["queries"][0], queries[0])
 
-    @pytest.mark.skip(reason="will be done in a future set of changes")
     def test_add_widget_invalid_query(self):
-        pass
+        data = {
+            "title": "First dashboard",
+            "widgets": [
+                {"id": six.text_type(self.widget_1.id)},
+                {
+                    "title": "Invalid fields",
+                    "displayType": "line",
+                    "queries": [
+                        {
+                            "name": "Errors",
+                            "fields": ["p95(transaction.duration)"],
+                            "conditions": "foo: bar:",
+                            "interval": "5m",
+                        }
+                    ],
+                },
+            ],
+        }
+        response = self.client.put(self.url(self.dashboard.id), data=data)
+        assert response.status_code == 400, response.data
+        assert b"Invalid conditions" in response.content
+
+    def test_add_widget_unknown_aggregation(self):
+        data = {
+            "title": "First dashboard",
+            "widgets": [
+                {"id": six.text_type(self.widget_1.id)},
+                {
+                    "title": "Invalid fields",
+                    "displayType": "line",
+                    "queries": [
+                        {
+                            "name": "Errors",
+                            "fields": ["wrong()"],
+                            "conditions": "",
+                            "interval": "5m",
+                        }
+                    ],
+                },
+            ],
+        }
+        response = self.client.put(self.url(self.dashboard.id), data=data)
+        assert response.status_code == 400, response.data
+        assert b"Invalid fields" in response.content
+
+    def test_add_widget_invalid_aggregate_parameter(self):
+        data = {
+            "title": "First dashboard",
+            "widgets": [
+                {"id": six.text_type(self.widget_1.id)},
+                {
+                    "title": "Invalid fields",
+                    "displayType": "line",
+                    "queries": [
+                        {
+                            "name": "Errors",
+                            "fields": ["p95(user)"],
+                            "conditions": "",
+                            "interval": "5m",
+                        }
+                    ],
+                },
+            ],
+        }
+        response = self.client.put(self.url(self.dashboard.id), data=data)
+        assert response.status_code == 400, response.data
+        assert b"Invalid fields" in response.content
 
-    @pytest.mark.skip(reason="will be done in a future set of changes")
-    def test_add_widget_invalid_fields(self):
-        pass
+    def test_add_widget_invalid_interval(self):
+        data = {
+            "title": "First dashboard",
+            "widgets": [
+                {"id": six.text_type(self.widget_1.id)},
+                {
+                    "title": "Invalid interval",
+                    "displayType": "line",
+                    "queries": [
+                        {
+                            "name": "Durations",
+                            "fields": ["p95(transaction.duration)"],
+                            "conditions": "",
+                            "interval": "1q",
+                        }
+                    ],
+                },
+            ],
+        }
+        response = self.client.put(self.url(self.dashboard.id), data=data)
+        assert response.status_code == 400, response.data
+        assert b"Invalid interval" in response.content
 
     def test_update_widget_title(self):
         data = {
@@ -371,13 +454,51 @@ class OrganizationDashboardDetailsPutTest(OrganizationDashboardDetailsTestCase):
         self.assert_serialized_widget(data["widgets"][2], widgets[2])
         assert self.widget_4.id == widgets[3].id
 
-    @pytest.mark.skip(reason="not done yet")
-    def test_update_widget_invalid_query(self):
-        pass
+    def test_update_widget_invalid_aggregate_parameter(self):
+        data = {
+            "title": "First dashboard",
+            "widgets": [
+                {
+                    "id": six.text_type(self.widget_1.id),
+                    "title": "Invalid fields",
+                    "displayType": "line",
+                    "queries": [
+                        {
+                            "name": "Errors",
+                            "fields": ["p95(user)"],
+                            "conditions": "",
+                            "interval": "5m",
+                        }
+                    ],
+                },
+            ],
+        }
+        response = self.client.put(self.url(self.dashboard.id), data=data)
+        assert response.status_code == 400, response.data
+        assert b"Invalid fields" in response.content
 
-    @pytest.mark.skip(reason="not done yet")
     def test_update_widget_invalid_fields(self):
-        pass
+        data = {
+            "title": "First dashboard",
+            "widgets": [
+                {
+                    "id": six.text_type(self.widget_1.id),
+                    "title": "Invalid fields",
+                    "displayType": "line",
+                    "queries": [
+                        {
+                            "name": "Errors",
+                            "fields": ["p95()"],
+                            "conditions": "foo: bar:",
+                            "interval": "5m",
+                        }
+                    ],
+                },
+            ],
+        }
+        response = self.client.put(self.url(self.dashboard.id), data=data)
+        assert response.status_code == 400, response.data
+        assert b"Invalid conditions" in response.content
 
     def test_remove_widgets(self):
         data = {