fix(api): Handle IntegrityError on SavedSearch (#33605)

* catch UniqueConstraint error when saving searches violating (organization_id, name, type) where (owner_id is NULL) unique index
* Update migration state and model to use appropriate constraint.

Related to: https://sentry.io/organizations/sentry/issues/3191049067/?project=1

migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0287_backfill_snubaquery_environment
+sentry: 0288_fix_savedsearch_state
 social_auth: 0001_initial

src/sentry/api/endpoints/organization_searches.py

@@ -1,3 +1,4 @@
+from django.db import IntegrityError
 from django.db.models import Q
 from rest_framework import serializers
 from rest_framework.request import Request
@@ -82,18 +83,29 @@ class OrganizationSearchesEndpoint(OrganizationEndpoint):
                     {"detail": "Query {} already exists".format(result["query"])}, status=400
                 )
 
-            saved_search = SavedSearch.objects.create(
-                organization=organization,
-                type=result["type"],
-                name=result["name"],
-                query=result["query"],
-                sort=result["sort"],
-            )
-            analytics.record(
-                "organization_saved_search.created",
-                search_type=SearchType(saved_search.type).name,
-                org_id=organization.id,
-                query=saved_search.query,
-            )
-            return Response(serialize(saved_search, request.user))
+            try:
+                saved_search = SavedSearch.objects.create(
+                    organization=organization,
+                    type=result["type"],
+                    name=result["name"],
+                    query=result["query"],
+                    sort=result["sort"],
+                )
+                analytics.record(
+                    "organization_saved_search.created",
+                    search_type=SearchType(saved_search.type).name,
+                    org_id=organization.id,
+                    query=saved_search.query,
+                )
+                return Response(serialize(saved_search, request.user))
+            except IntegrityError:
+                return Response(
+                    {
+                        "detail": "The combination ({}, {}, {}) for query {} already exists.".format(
+                            organization.id, result["name"], result["type"], result["query"]
+                        )
+                    },
+                    status=400,
+                )
+
         return Response(serializer.errors, status=400)

src/sentry/migrations/0288_fix_savedsearch_state.py

@@ -0,0 +1,52 @@
+# Generated by Django 2.2.24 on 2022-04-14 00:27
+
+from django.db import migrations
+from django.db.models import Q, UniqueConstraint
+
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    # This flag is used to decide whether to run this migration in a transaction or not. Generally
+    # we don't want to run in a transaction here, since for long running operations like data
+    # back-fills this results in us locking an increasing number of rows until we finally commit.
+    atomic = True
+
+    dependencies = [
+        ("sentry", "0287_backfill_snubaquery_environment"),
+    ]
+
+    operations = [
+        migrations.SeparateDatabaseAndState(
+            state_operations=[
+                migrations.AddConstraint(
+                    model_name="savedsearch",
+                    constraint=UniqueConstraint(
+                        fields=["organization", "name", "type"],
+                        condition=Q(owner__isnull=True),
+                        name="sentry_savedsearch_is_global_6793a2f9e1b59b95",
+                    ),
+                ),
+                migrations.AddConstraint(
+                    model_name="savedsearch",
+                    constraint=UniqueConstraint(
+                        fields=["is_global", "name"],
+                        condition=Q(is_global=True),
+                        name="sentry_savedsearch_organization_id_313a24e907cdef99",
+                    ),
+                ),
+            ],
+        ),
+    ]

src/sentry/models/savedsearch.py

@@ -1,4 +1,5 @@
 from django.db import models
+from django.db.models import Q, UniqueConstraint
 from django.utils import timezone
 from django.utils.translation import ugettext_lazy as _
 
@@ -54,14 +55,23 @@ class SavedSearch(Model):
     class Meta:
         app_label = "sentry"
         db_table = "sentry_savedsearch"
-        # Note that we also have a partial unique constraint on:
-        #   (organization_id, name, type) WHERE owner_id IS NULL
-        #   (is_global, name) WHERE is_global
         unique_together = (
             ("project", "name"),
             # Each user can have one default search per org
             ("organization", "owner", "type"),
         )
+        constraints = [
+            UniqueConstraint(
+                fields=["organization", "name", "type"],
+                condition=Q(owner__isnull=True),
+                name="sentry_savedsearch_is_global_6793a2f9e1b59b95",
+            ),
+            UniqueConstraint(
+                fields=["is_global", "name"],
+                condition=Q(is_global=True),
+                name="sentry_savedsearch_organization_id_313a24e907cdef99",
+            ),
+        ]
 
     @property
     def is_pinned(self):

tests/sentry/api/endpoints/test_organization_searches.py

@@ -170,3 +170,22 @@ class CreateOrganizationSearchesTest(APITestCase):
         )
         assert resp.status_code == 400
         assert "This field may not be blank." == resp.data["query"][0]
+
+    def test_create_dupe_on_organization_name_type(self):
+        self.login_as(user=self.manager)
+        resp = self.get_response(
+            self.organization.slug,
+            type=SearchType.ISSUE.value,
+            name="hello",
+            query="is:unresolved",
+        )
+        assert resp.status_code == 200
+
+        resp_dupe = self.get_response(
+            self.organization.slug,
+            type=SearchType.ISSUE.value,
+            name="hello",
+            query="is:resolved",
+        )
+        assert resp_dupe.status_code == 400
+        assert "The combination" in resp_dupe.data["detail"]