Browse Source

migration(crons): Slugify invalid monitor slugs (#52471)

Slugifies invalid monitor slugs + deletes on collision.
Richard Ortenberg 1 year ago
parent
commit
befafb168c

+ 1 - 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: 0506_null_boolean_fields
+sentry: 0507_slugify_invalid_monitors
 social_auth: 0001_initial

+ 56 - 0
src/sentry/migrations/0507_slugify_invalid_monitors.py

@@ -0,0 +1,56 @@
+# Generated by Django 2.2.28 on 2023-07-07 15:51
+from django.db import IntegrityError, migrations
+from django.utils.text import slugify
+
+from sentry.new_migrations.migrations import CheckedMigration
+from sentry.utils.query import RangeQuerySetWrapperWithProgressBar
+
+
+def migrate_monitor_slugs(apps, schema_editor):
+    Monitor = apps.get_model("sentry", "Monitor")
+
+    MAX_SLUG_LENGTH = 50
+    monitors_to_clean_up = []
+
+    for monitor in RangeQuerySetWrapperWithProgressBar(Monitor.objects.all()):
+        monitor_slug = monitor.slug
+        slugified = slugify(monitor_slug)[:MAX_SLUG_LENGTH].strip("-")
+
+        # Nothing to migrate if the monitor already has a valid slug
+        if monitor_slug == slugified:
+            continue
+
+        try:
+            monitor.slug = slugified
+            monitor.save()
+        except IntegrityError:
+            # If there is a collision, delete the old monitor as the new one is receiving all check-ins
+            monitors_to_clean_up.append(monitor.id)
+
+    Monitor.objects.filter(id__in=monitors_to_clean_up).delete()
+
+
+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
+
+    dependencies = [
+        ("sentry", "0506_null_boolean_fields"),
+    ]
+
+    operations = [
+        migrations.RunPython(
+            migrate_monitor_slugs,
+            migrations.RunPython.noop,
+            hints={"tables": ["sentry_monitior"]},
+        ),
+    ]

+ 79 - 0
tests/sentry/migrations/test_0507_slugify_invalid_monitors.py

@@ -0,0 +1,79 @@
+import pytest
+from django.utils.text import slugify
+
+from sentry.monitors.models import Monitor, MonitorType, ScheduleType
+from sentry.testutils.cases import TestMigrations
+
+
+class MigrateSlugifyInvalidMonitorTest(TestMigrations):
+    migrate_from = "0506_null_boolean_fields"
+    migrate_to = "0507_slugify_invalid_monitors"
+
+    def setup_before_migration(self, apps):
+        self.invalid_monitor_1 = Monitor.objects.create(
+            name="invalid_1",
+            slug="/api/analytics/admin.get?token=123",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.invalid_monitor_2 = Monitor.objects.create(
+            name="invalid_2",
+            slug="/api/cron",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.invalid_monitor_3 = Monitor.objects.create(
+            name="invalid_2",
+            slug="/api/cron/3",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.valid_monitor_1 = Monitor.objects.create(
+            name="valid_1",
+            slug="valid_1",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+        self.valid_monitor_2 = Monitor.objects.create(
+            name="valid_2",
+            slug="apicron3",
+            organization_id=self.organization.id,
+            project_id=self.project.id,
+            type=MonitorType.CRON_JOB,
+            config={"schedule": "* * * * *", "schedule_type": ScheduleType.CRONTAB},
+        )
+
+    def test(self):
+        MAX_SLUG_LENGTH = 50
+
+        invalid_monitor_1 = Monitor.objects.get(id=self.invalid_monitor_1.id)
+        invalid_monitor_2 = Monitor.objects.get(id=self.invalid_monitor_2.id)
+
+        valid_monitor_1 = Monitor.objects.get(id=self.valid_monitor_1.id)
+        valid_monitor_2 = Monitor.objects.get(id=self.valid_monitor_2.id)
+
+        assert invalid_monitor_1.slug == slugify(self.invalid_monitor_1.slug)[
+            :MAX_SLUG_LENGTH
+        ].strip("-")
+        assert invalid_monitor_2.slug == slugify(self.invalid_monitor_2.slug)[
+            :MAX_SLUG_LENGTH
+        ].strip("-")
+
+        # assert colliding monitor does not exist
+        with pytest.raises(Monitor.DoesNotExist):
+            Monitor.objects.get(id=self.invalid_monitor_3.id)
+
+        assert valid_monitor_1.slug == self.valid_monitor_1.slug
+        assert valid_monitor_2.slug == self.valid_monitor_2.slug