Browse Source

feat(semver): Filter semver related filters by any project ids passed to the search. (#27734)

Currently we only filter releases used in semver down to the org level. This can cause issues in
projects with many releases in cases when we return a lot of release candidates.

This pr fixes the semver and release related filters to include project_id in the queries used to fetch
release candidates.
Dan Fuller 3 years ago
parent
commit
ff13f4a079

+ 3 - 1
src/sentry/api/serializers/models/group.py

@@ -767,6 +767,7 @@ class GroupSerializerSnuba(GroupSerializerBase):
         collapse=None,
         expand=None,
         organization_id=None,
+        project_ids=None,
     ):
         super().__init__(collapse=collapse, expand=expand)
         from sentry.search.snuba.executors import get_search_filter
@@ -789,7 +790,8 @@ class GroupSerializerSnuba(GroupSerializerBase):
         self.conditions = (
             [
                 convert_search_filter_to_snuba_query(
-                    search_filter, params={"organization_id": organization_id}
+                    search_filter,
+                    params={"organization_id": organization_id, "project_id": project_ids},
                 )
                 for search_filter in search_filters
                 if search_filter.key.name not in self.skip_snuba_fields

+ 15 - 4
src/sentry/models/release.py

@@ -139,7 +139,11 @@ class ReleaseQuerySet(models.QuerySet):
         return self.filter(major__isnull=False)
 
     def filter_by_semver_build(
-        self, organization_id: int, operator: str, build: str, project_ids: Sequence[int] = None
+        self,
+        organization_id: int,
+        operator: str,
+        build: str,
+        project_ids: Optional[Sequence[int]] = None,
     ) -> models.QuerySet:
         """
         Filters released by build. If the passed `build` is a numeric string, we'll filter on
@@ -170,7 +174,7 @@ class ReleaseQuerySet(models.QuerySet):
         self,
         organization_id: int,
         semver_filter: SemverFilter,
-        project_ids: Sequence[int] = None,
+        project_ids: Optional[Sequence[int]] = None,
     ) -> models.QuerySet:
         """
         Filters releases based on a based `SemverFilter` instance.
@@ -261,14 +265,21 @@ class ReleaseModelManager(models.Manager):
         return self.get_queryset().filter_to_semver()
 
     def filter_by_semver_build(
-        self, organization_id: int, operator: str, build: str, project_ids: Sequence[int] = None
+        self,
+        organization_id: int,
+        operator: str,
+        build: str,
+        project_ids: Optional[Sequence[int]] = None,
     ) -> models.QuerySet:
         return self.get_queryset().filter_by_semver_build(
             organization_id, operator, build, project_ids
         )
 
     def filter_by_semver(
-        self, organization_id: int, semver_filter: SemverFilter, project_ids: Sequence[int] = None
+        self,
+        organization_id: int,
+        semver_filter: SemverFilter,
+        project_ids: Optional[Sequence[int]] = None,
     ) -> models.QuerySet:
         return self.get_queryset().filter_by_semver(organization_id, semver_filter, project_ids)
 

+ 20 - 4
src/sentry/search/events/filter.py

@@ -360,9 +360,13 @@ def _release_stage_filter_converter(
         raise ValueError("organization_id is a required param")
 
     organization_id: int = params["organization_id"]
+    project_ids: Optional[list[int]] = params.get("project_id")
     qs = (
         Release.objects.filter_by_stage(
-            organization_id, search_filter.operator, search_filter.value.value
+            organization_id,
+            search_filter.operator,
+            search_filter.value.value,
+            project_ids=project_ids,
         )
         .values_list("version", flat=True)
         .order_by("date_added")[:MAX_SEARCH_RELEASES]
@@ -401,6 +405,7 @@ def _semver_filter_converter(
         raise ValueError("organization_id is a required param")
 
     organization_id: int = params["organization_id"]
+    project_ids: Optional[list[int]] = params.get("project_id")
     # We explicitly use `raw_value` here to avoid converting wildcards to shell values
     version: str = search_filter.value.raw_value
     operator: str = search_filter.operator
@@ -412,7 +417,11 @@ def _semver_filter_converter(
     if operator.startswith("<"):
         order_by = list(map(_flip_field_sort, order_by))
     qs = (
-        Release.objects.filter_by_semver(organization_id, parse_semver(version, operator))
+        Release.objects.filter_by_semver(
+            organization_id,
+            parse_semver(version, operator),
+            project_ids=project_ids,
+        )
         .values_list("version", flat=True)
         .order_by(*order_by)[:MAX_SEARCH_RELEASES]
     )
@@ -459,11 +468,14 @@ def _semver_package_filter_converter(
         raise ValueError("organization_id is a required param")
 
     organization_id: int = params["organization_id"]
+    project_ids: Optional[list[int]] = params.get("project_id")
     package: str = search_filter.value.raw_value
 
     versions = list(
         Release.objects.filter_by_semver(
-            organization_id, SemverFilter("exact", [], package)
+            organization_id,
+            SemverFilter("exact", [], package),
+            project_ids=project_ids,
         ).values_list("version", flat=True)[:MAX_SEARCH_RELEASES]
     )
 
@@ -487,11 +499,15 @@ def _semver_build_filter_converter(
         raise ValueError("organization_id is a required param")
 
     organization_id: int = params["organization_id"]
+    project_ids: Optional[list[int]] = params.get("project_id")
     build: str = search_filter.value.raw_value
 
     versions = list(
         Release.objects.filter_by_semver_build(
-            organization_id, OPERATOR_TO_DJANGO[search_filter.operator], build
+            organization_id,
+            OPERATOR_TO_DJANGO[search_filter.operator],
+            build,
+            project_ids=project_ids,
         ).values_list("version", flat=True)[:MAX_SEARCH_RELEASES]
     )
 

+ 2 - 1
src/sentry/search/snuba/executors.py

@@ -134,7 +134,8 @@ class AbstractQueryExecutor(metaclass=ABCMeta):
             ):
                 continue
             converted_filter = convert_search_filter_to_snuba_query(
-                search_filter, params={"organization_id": organization_id}
+                search_filter,
+                params={"organization_id": organization_id, "project_id": project_ids},
             )
             converted_filter = self._transform_converted_filter(
                 search_filter, converted_filter, project_ids, environment_ids

+ 86 - 39
tests/sentry/search/events/test_filter.py

@@ -1500,7 +1500,40 @@ class DiscoverFunctionTest(unittest.TestCase):
         assert fn.is_accessible(["fn"]) is True
 
 
-class SemverFilterConverterTest(TestCase):
+class BaseSemverConverterTest:
+    key = None
+
+    def converter(self, *args, **kwargs):
+        raise NotImplementedError
+
+    def run_test(
+        self,
+        operator,
+        version,
+        expected_operator,
+        expected_releases,
+        organization_id=None,
+        project_id=None,
+    ):
+        organization_id = organization_id if organization_id else self.organization.id
+        filter = SearchFilter(SearchKey(self.key), operator, SearchValue(version))
+        params = {}
+        if organization_id:
+            params["organization_id"] = organization_id
+        if project_id:
+            params["project_id"] = project_id
+        converted = self.converter(filter, self.key, params)
+        assert converted[0] == "release"
+        assert converted[1] == expected_operator
+        assert set(converted[2]) == set(expected_releases)
+
+
+class SemverFilterConverterTest(BaseSemverConverterTest, TestCase):
+    key = SEMVER_ALIAS
+
+    def converter(self, *args, **kwargs):
+        return _semver_filter_converter(*args, **kwargs)
+
     def test_invalid_params(self):
         key = SEMVER_ALIAS
         filter = SearchFilter(SearchKey(key), ">", SearchValue("1.2.3"))
@@ -1518,18 +1551,6 @@ class SemverFilterConverterTest(TestCase):
         ):
             _semver_filter_converter(filter, key, {"organization_id": self.organization.id})
 
-    def run_test(
-        self, operator, version, expected_operator, expected_releases, organization_id=None
-    ):
-        organization_id = organization_id if organization_id else self.organization.id
-        key = SEMVER_ALIAS
-        filter = SearchFilter(SearchKey(key), operator, SearchValue(version))
-        assert _semver_filter_converter(filter, key, {"organization_id": organization_id}) == [
-            "release",
-            expected_operator,
-            expected_releases,
-        ]
-
     def test_empty(self):
         self.run_test(">", "1.2.3", "IN", [SEMVER_EMPTY_RELEASE])
 
@@ -1633,20 +1654,39 @@ class SemverFilterConverterTest(TestCase):
         self.run_test(">=", "test@1.0", "IN", [release_1.version, release_2.version])
         self.run_test(">", "test_2@1.0", "IN", [release_3.version])
 
-
-class SemverPackageFilterConverterTest(TestCase):
-    def run_test(
-        self, operator, version, expected_operator, expected_releases, organization_id=None
-    ):
-        organization_id = organization_id if organization_id else self.organization.id
-        key = SEMVER_PACKAGE_ALIAS
-        filter = SearchFilter(SearchKey(key), operator, SearchValue(version))
-        converted = _semver_package_filter_converter(
-            filter, key, {"organization_id": organization_id}
+    def test_projects(self):
+        project_2 = self.create_project()
+        release_1 = self.create_release(version="test@1.0.0.0")
+        release_2 = self.create_release(version="test@1.2.0.0", project=project_2)
+        release_3 = self.create_release(version="test@1.2.3.0")
+        self.run_test(
+            ">=",
+            "test@1.0",
+            "IN",
+            [release_1.version, release_2.version, release_3.version],
+            project_id=[self.project.id, project_2.id],
         )
-        assert converted[0] == "release"
-        assert converted[1] == expected_operator
-        assert set(converted[2]) == set(expected_releases)
+        self.run_test(
+            ">=",
+            "test@1.0",
+            "IN",
+            [release_1.version, release_3.version],
+            project_id=[self.project.id],
+        )
+        self.run_test(
+            ">=",
+            "test@1.0",
+            "IN",
+            [release_2.version],
+            project_id=[project_2.id],
+        )
+
+
+class SemverPackageFilterConverterTest(BaseSemverConverterTest, TestCase):
+    key = SEMVER_PACKAGE_ALIAS
+
+    def converter(self, *args, **kwargs):
+        return _semver_package_filter_converter(*args, **kwargs)
 
     def test_invalid_params(self):
         key = SEMVER_PACKAGE_ALIAS
@@ -1667,20 +1707,27 @@ class SemverPackageFilterConverterTest(TestCase):
         self.run_test("=", "test2", "IN", [release_3.version])
         self.run_test("=", "test3", "IN", [SEMVER_EMPTY_RELEASE])
 
-
-class SemverBuildFilterConverterTest(TestCase):
-    def run_test(
-        self, operator, version, expected_operator, expected_releases, organization_id=None
-    ):
-        organization_id = organization_id if organization_id else self.organization.id
-        key = SEMVER_BUILD_ALIAS
-        filter = SearchFilter(SearchKey(key), operator, SearchValue(version))
-        converted = _semver_build_filter_converter(
-            filter, key, {"organization_id": organization_id}
+    def test_projects(self):
+        project_2 = self.create_project()
+        release_1 = self.create_release(version="test@1.0.0.0")
+        release_2 = self.create_release(version="test@1.2.0.0", project=project_2)
+        self.create_release(version="test2@1.2.3.0")
+        self.run_test("=", "test", "IN", [release_1.version], project_id=[self.project.id])
+        self.run_test("=", "test", "IN", [release_2.version], project_id=[project_2.id])
+        self.run_test(
+            "=",
+            "test",
+            "IN",
+            [release_1.version, release_2.version],
+            project_id=[self.project.id, project_2.id],
         )
-        assert converted[0] == "release"
-        assert converted[1] == expected_operator
-        assert set(converted[2]) == set(expected_releases)
+
+
+class SemverBuildFilterConverterTest(BaseSemverConverterTest, TestCase):
+    key = SEMVER_BUILD_ALIAS
+
+    def converter(self, *args, **kwargs):
+        return _semver_build_filter_converter(*args, **kwargs)
 
     def test_invalid_params(self):
         key = SEMVER_BUILD_ALIAS