Browse Source

fix(discover): Allow saving queries with My projects (#19995)

* fix(discover): Allow saving queries with My projects

- Couldn't save queries when my projects was selected and project was
  part of the query
- Added optional project_ids parameter to get_filter_params since the
  default get_requested_project_ids assumes that project_ids is always
  passed via get params
William Mak 4 years ago
parent
commit
ba2aa70294

+ 22 - 6
src/sentry/api/bases/organization.py

@@ -157,7 +157,12 @@ class OrganizationEndpoint(Endpoint):
             raise ParseError(detail="Invalid project parameter. Values must be numbers.")
 
     def get_projects(
-        self, request, organization, force_global_perms=False, include_all_accessible=False
+        self,
+        request,
+        organization,
+        force_global_perms=False,
+        include_all_accessible=False,
+        project_ids=None,
     ):
         """
         Determines which project ids to filter the endpoint by. If a list of
@@ -177,9 +182,12 @@ class OrganizationEndpoint(Endpoint):
         :param include_all_accessible: Whether to factor the organization
         allow_joinleave flag into permission checks. We should ideally
         standardize how this is used and remove this parameter.
+        :param project_ids: Projects if they were passed via request
+        data instead of get params
         :return: A list of Project objects, or raises PermissionDenied.
         """
-        project_ids = self.get_requested_project_ids(request)
+        if project_ids is None:
+            project_ids = self.get_requested_project_ids(request)
         return self._get_projects_by_id(
             project_ids, request, organization, force_global_perms, include_all_accessible
         )
@@ -237,13 +245,17 @@ class OrganizationEndpoint(Endpoint):
         with sentry_sdk.start_span(op="PERF: Org.get_environments"):
             return get_environments(request, organization)
 
-    def get_filter_params(self, request, organization, date_filter_optional=False):
+    def get_filter_params(
+        self, request, organization, date_filter_optional=False, project_ids=None
+    ):
         """
         Extracts common filter parameters from the request and returns them
         in a standard format.
         :param request:
         :param organization: Organization to get params for
         :param date_filter_optional: Defines what happens if no date filter
+        :param project_ids: Project ids if they were already grabbed but not
+        validated yet
         parameters are passed. If False, no date filtering occurs. If True, we
         provide default values.
         :return: A dict with keys:
@@ -265,7 +277,7 @@ class OrganizationEndpoint(Endpoint):
 
         with sentry_sdk.start_span(op="PERF: org.get_filter_params - projects"):
             try:
-                projects = self.get_projects(request, organization)
+                projects = self.get_projects(request, organization, project_ids)
             except ValueError:
                 raise ParseError(detail="Invalid project ids")
 
@@ -315,7 +327,7 @@ class OrganizationEndpoint(Endpoint):
 class OrganizationReleasesBaseEndpoint(OrganizationEndpoint):
     permission_classes = (OrganizationReleasePermission,)
 
-    def get_projects(self, request, organization):
+    def get_projects(self, request, organization, project_ids=None):
         """
         Get all projects the current user or API token has access to. More
         detail in the parent class's method of the same name.
@@ -335,7 +347,11 @@ class OrganizationReleasesBaseEndpoint(OrganizationEndpoint):
             return []
 
         return super(OrganizationReleasesBaseEndpoint, self).get_projects(
-            request, organization, force_global_perms=has_valid_api_key, include_all_accessible=True
+            request,
+            organization,
+            force_global_perms=has_valid_api_key,
+            include_all_accessible=True,
+            project_ids=project_ids,
         )
 
     def has_release_permission(self, request, organization, release):

+ 6 - 1
src/sentry/discover/endpoints/discover_saved_queries.py

@@ -89,7 +89,12 @@ class DiscoverSavedQueriesEndpoint(OrganizationEndpoint):
             return self.respond(status=404)
 
         serializer = DiscoverSavedQuerySerializer(
-            data=request.data, context={"organization": organization}
+            data=request.data,
+            context={
+                "params": self.get_filter_params(
+                    request, organization, project_ids=request.data.get("projects")
+                )
+            },
         )
 
         if not serializer.is_valid():

+ 7 - 1
src/sentry/discover/endpoints/discover_saved_query_detail.py

@@ -43,8 +43,14 @@ class DiscoverSavedQueryDetailEndpoint(OrganizationEndpoint):
             model = DiscoverSavedQuery.objects.get(id=query_id, organization=organization)
         except DiscoverSavedQuery.DoesNotExist:
             raise ResourceDoesNotExist
+
         serializer = DiscoverSavedQuerySerializer(
-            data=request.data, context={"organization": organization}
+            data=request.data,
+            context={
+                "params": self.get_filter_params(
+                    request, organization, project_ids=request.data.get("projects")
+                )
+            },
         )
         if not serializer.is_valid():
             return Response(serializer.errors, status=400)

+ 6 - 17
src/sentry/discover/endpoints/serializers.py

@@ -5,7 +5,6 @@ import re
 from rest_framework import serializers
 from rest_framework.exceptions import PermissionDenied
 
-from sentry.models import Project, ProjectStatus
 from sentry.discover.models import KeyTransaction, MAX_KEY_TRANSACTIONS
 from sentry.api.fields.empty_integer import EmptyIntegerField
 from sentry.api.event_search import get_filter, InvalidSearchQuery
@@ -171,18 +170,14 @@ class DiscoverSavedQuerySerializer(serializers.Serializer):
     }
 
     def validate_projects(self, projects):
-        organization = self.context["organization"]
-
         projects = set(projects)
-        if projects == ALL_ACCESS_PROJECTS:
+
+        # Don't need to check all projects or my projects
+        if projects == ALL_ACCESS_PROJECTS or len(projects) == 0:
             return projects
 
-        org_projects = set(
-            Project.objects.filter(
-                organization=organization, id__in=projects, status=ProjectStatus.VISIBLE
-            ).values_list("id", flat=True)
-        )
-        if projects != org_projects:
+        # Params will have project_ids that have been validated
+        if projects != set(self.context["params"]["project_id"]):
             raise PermissionDenied
 
         return projects
@@ -221,13 +216,7 @@ class DiscoverSavedQuerySerializer(serializers.Serializer):
 
         if "query" in query:
             try:
-                get_filter(
-                    query["query"],
-                    {
-                        "project_id": data["projects"],
-                        "organization_id": self.context["organization"],
-                    },
-                )
+                get_filter(query["query"], self.context["params"])
             except InvalidSearchQuery as err:
                 raise serializers.ValidationError("Cannot save invalid query: {}".format(err))
 

+ 54 - 0
tests/snuba/api/endpoints/test_discover_saved_queries.py

@@ -347,6 +347,60 @@ class DiscoverSavedQueriesVersion2Test(DiscoverSavedQueryBase):
         assert response.status_code == 201, response.content
         assert DiscoverSavedQuery.objects.filter(name="project query").exists()
 
+    def test_save_with_project_and_my_projects(self):
+        team = self.create_team(organization=self.org, members=[self.user])
+        project = self.create_project(organization=self.org, teams=[team])
+        with self.feature(self.feature_name):
+            url = reverse("sentry-api-0-discover-saved-queries", args=[self.org.slug])
+            response = self.client.post(
+                url,
+                {
+                    "name": "project query",
+                    "projects": [],
+                    "fields": ["title", "count()"],
+                    "range": "24h",
+                    "query": "project:{}".format(project.slug),
+                    "version": 2,
+                },
+            )
+        assert response.status_code == 201, response.content
+        assert DiscoverSavedQuery.objects.filter(name="project query").exists()
+
+    def test_save_with_wrong_projects(self):
+        other_org = self.create_organization(owner=self.user)
+        project = self.create_project(organization=other_org)
+        with self.feature(self.feature_name):
+            url = reverse("sentry-api-0-discover-saved-queries", args=[self.org.slug])
+            response = self.client.post(
+                url,
+                {
+                    "name": "project query",
+                    "projects": [project.id],
+                    "fields": ["title", "count()"],
+                    "range": "24h",
+                    "query": "project:{}".format(project.slug),
+                    "version": 2,
+                },
+            )
+        assert response.status_code == 403, response.content
+        assert not DiscoverSavedQuery.objects.filter(name="project query").exists()
+
+        with self.feature(self.feature_name):
+            url = reverse("sentry-api-0-discover-saved-queries", args=[self.org.slug])
+            response = self.client.post(
+                url,
+                {
+                    "name": "project query",
+                    "projects": [-1],
+                    "fields": ["title", "count()"],
+                    "range": "24h",
+                    "query": "project:{}".format(project.slug),
+                    "version": 2,
+                },
+            )
+        assert response.status_code == 400, response.content
+        assert not DiscoverSavedQuery.objects.filter(name="project query").exists()
+
     def test_save_invalid_query(self):
         with self.feature(self.feature_name):
             response = self.client.post(