Browse Source

feat(monitors): Slugify name as a default slug (#46479)

Fixes: https://github.com/getsentry/sentry/issues/46152

Newly created monitors will have a slug generated from their name

---

This is a BREAKING CHANGE in the sense that slugs by default will no
longer match GUIDs by default. This means to create checkins users MUST
use a checkin method which supports slugs (most do)

The only problematic scenario would be users explicitly using the legacy
orgless checkin endpoints using token auth. Which we do not recommend
anywhere (so this should be okay)
Evan Purkhiser 1 year ago
parent
commit
9c1eccba62

+ 15 - 10
src/sentry/monitors/models.py

@@ -1,7 +1,6 @@
 from __future__ import annotations
 
 from datetime import datetime, timedelta
-from uuid import uuid4
 
 import pytz
 from croniter import croniter
@@ -22,7 +21,10 @@ from sentry.db.models import (
     region_silo_only_model,
     sane_repr,
 )
+from sentry.db.models.utils import slugify_instance
+from sentry.locks import locks
 from sentry.models import Environment, Project
+from sentry.utils.retries import TimedRetryPolicy
 
 SCHEDULE_INTERVAL_MAP = {
     "year": rrule.YEARLY,
@@ -184,15 +186,18 @@ class Monitor(Model):
     __repr__ = sane_repr("guid", "project_id", "name")
 
     def save(self, *args, **kwargs):
-        # TODO(epurkhsier): This logic is to be removed when the `guid` field
-        # is removed and a slug is required when creating monitors
-
-        # NOTE: We ONLY set a slug while saving when creating a new monitor and
-        # the slug has not been set. Otherwise existing monitors without slugs
-        # would have their guids changed
-        if self._state.adding is True and not self.slug:
-            self.guid = uuid4()
-            self.slug = str(self.guid)
+        if not self.slug:
+            lock = locks.get(
+                f"slug:monitor:{self.organization_id}", duration=5, name="monitor_slug"
+            )
+            with TimedRetryPolicy(10)(lock.acquire):
+                slugify_instance(
+                    self,
+                    self.name,
+                    organization_id=self.organization_id,
+                    max_length=50,
+                )
+
         return super().save(*args, **kwargs)
 
     def get_schedule_type_display(self):

+ 2 - 2
tests/sentry/monitors/endpoints/test_monitor_ingest_checkin_index.py

@@ -219,7 +219,7 @@ class CreateMonitorCheckInTest(MonitorIngestTestCase):
                 == "Invalid monitor slug. Must match the pattern [a-zA-Z0-9_-]+"
             )
 
-    def test_with_dsn_auth(self):
+    def test_with_dsn_auth_and_guid(self):
         for path_func in self._get_path_functions():
             monitor = self._create_monitor()
             path = path_func(monitor.guid)
@@ -288,7 +288,7 @@ class CreateMonitorCheckInTest(MonitorIngestTestCase):
         path = reverse(self.endpoint, args=[monitor.slug])
         resp = self.client.post(path, **self.token_auth_headers)
 
-        assert resp.status_code == 403
+        assert resp.status_code == 404
 
     def test_mismatched_org_slugs(self):
         monitor = self._create_monitor()

+ 2 - 2
tests/sentry/monitors/endpoints/test_organization_monitor_details.py

@@ -26,9 +26,9 @@ class OrganizationMonitorDetailsTest(MonitorTestCase):
         monitor = self._create_monitor()
         self._create_monitor_environment(monitor)
 
-        self.get_success_response(self.organization.slug, monitor.guid, environment="production")
+        self.get_success_response(self.organization.slug, monitor.slug, environment="production")
         self.get_error_response(
-            self.organization.slug, monitor.guid, environment="jungle", status_code=404
+            self.organization.slug, monitor.slug, environment="jungle", status_code=404
         )
 
 

+ 16 - 9
tests/sentry/monitors/test_models.py

@@ -193,32 +193,39 @@ class MonitorTestCase(TestCase):
             },
         ) == dict(event)
 
-    def test_save_defaults_slug_to_guid(self):
+    def test_save_defaults_slug_to_name(self):
         monitor = Monitor.objects.create(
             organization_id=self.organization.id,
             project_id=self.project.id,
             type=MonitorType.CRON_JOB,
+            name="My Awesome Monitor",
             config={"schedule": [1, "month"], "schedule_type": ScheduleType.INTERVAL},
         )
 
-        assert str(monitor.guid) == monitor.slug
+        assert monitor.slug == "my-awesome-monitor"
 
-    def test_save_defaults_slug_to_guid_only_on_create(self):
+    def test_save_defaults_slug_unique(self):
         monitor = Monitor.objects.create(
             organization_id=self.organization.id,
             project_id=self.project.id,
             type=MonitorType.CRON_JOB,
+            name="My Awesome Monitor",
+            slug="my-awesome-monitor",
             config={"schedule": [1, "month"], "schedule_type": ScheduleType.INTERVAL},
         )
 
-        original_monitor_guid = monitor.guid
+        assert monitor.slug == "my-awesome-monitor"
 
-        # Simulate existing monitors entries that don't have a slug set
-        monitor.slug = ""
-        monitor.name = "New name"
-        monitor.save()
+        # Create another monitor with the same name
+        monitor = Monitor.objects.create(
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            name="My Awesome Monitor",
+            config={"schedule": [1, "month"], "schedule_type": ScheduleType.INTERVAL},
+        )
 
-        assert monitor.guid == original_monitor_guid
+        assert monitor.slug.startswith("my-awesome-monitor-")
 
 
 @region_silo_test(stable=True)