Browse Source

ref(hc): Adding team and user ids to x actor. (#54849)

First step of
https://github.com/getsentry/sentry/pull/54771/files#diff-5a16f9d3d0b7d38200e118ff45dfac859ceebe3793d0f40202cf2fb9ad0d86f4

[ ] Add new columns <-- we are here
[ ] Change code to write both to actor and team/user ids
[ ] Write migration to backfill existing team / user ids
[ ] Write code to read from team / user ids
[ ] Make actor column nullable, stop writing to it
[ ] Drop actor column

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
Zach Collins 1 year ago
parent
commit
2a28d88216

+ 4 - 0
fixtures/backup/model_dependencies/detailed.json

@@ -707,6 +707,10 @@
       "organization": {
         "kind": "FlexibleForeignKey",
         "model": "sentry.Organization"
+      },
+      "team": {
+        "kind": "FlexibleForeignKey",
+        "model": "sentry.Team"
       }
     },
     "model": "sentry.ExternalActor",

+ 2 - 1
fixtures/backup/model_dependencies/flat.json

@@ -158,7 +158,8 @@
   ],
   "sentry.ExternalActor": [
     "sentry.Actor",
-    "sentry.Organization"
+    "sentry.Organization",
+    "sentry.Team"
   ],
   "sentry.ExternalIssue": [
     "sentry.Organization"

+ 1 - 1
fixtures/backup/model_dependencies/sorted.json

@@ -31,7 +31,6 @@
   "sentry.Release",
   "sentry.IdentityProvider",
   "sentry.DocIntegration",
-  "sentry.ExternalActor",
   "sentry.ExternalIssue",
   "sentry.Integration",
   "sentry.IntegrationExternalProject",
@@ -181,6 +180,7 @@
   "sentry.GroupShare",
   "sentry.GroupSnooze",
   "sentry.GroupSubscription",
+  "sentry.ExternalActor",
   "sentry.SentryApp",
   "sentry.SentryAppComponent",
   "sentry.SentryAppInstallation",

+ 1 - 1
migrations_lockfile.txt

@@ -7,5 +7,5 @@ will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
 replays: 0003_add_size_to_recording_segment
-sentry: 0531_add_notification_uuid_to_incident_activity
+sentry: 0532_denormalize_team_and_user_x_actor
 social_auth: 0002_default_auto_field

+ 0 - 1
pyproject.toml

@@ -563,7 +563,6 @@ module = [
     "sentry.migrations.0418_add_actor_constraints",
     "sentry.models.artifactbundle",
     "sentry.models.auditlogentry",
-    "sentry.models.integrations.external_actor",
     "sentry.models.integrations.external_issue",
     "sentry.models.integrations.organization_integrity_backfill_mixin",
     "sentry.models.integrations.sentry_app",

+ 51 - 0
src/sentry/migrations/0532_denormalize_team_and_user_x_actor.py

@@ -0,0 +1,51 @@
+# Generated by Django 3.2.20 on 2023-08-16 16:16
+
+import django.db.models.deletion
+from django.db import migrations
+
+import sentry.db.models.fields.foreignkey
+import sentry.db.models.fields.hybrid_cloud_foreign_key
+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
+
+    dependencies = [
+        ("sentry", "0531_add_notification_uuid_to_incident_activity"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="externalactor",
+            name="team",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                null=True, on_delete=django.db.models.deletion.CASCADE, to="sentry.team"
+            ),
+        ),
+        migrations.AddField(
+            model_name="externalactor",
+            name="user_id",
+            field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                "sentry.User", db_index=True, null=True, on_delete="CASCADE"
+            ),
+        ),
+        migrations.AlterUniqueTogether(
+            name="externalactor",
+            unique_together={
+                ("organization", "provider", "external_name", "actor"),
+                ("organization", "provider", "external_name", "team_id"),
+                ("organization", "provider", "external_name", "user_id"),
+            },
+        ),
+    ]

+ 14 - 8
src/sentry/models/integrations/external_actor.py

@@ -16,12 +16,13 @@ from sentry.types.integrations import ExternalProviders
 logger = logging.getLogger(__name__)
 
 
-# TODO(hybrid-cloud): This should probably be a control silo model. We'd need to replace the actor reference with a team_id and user_id
 @region_silo_only_model
 class ExternalActor(DefaultFieldsModel):
     __include_in_export__ = False
 
     actor = FlexibleForeignKey("sentry.Actor", db_index=True, on_delete=models.CASCADE)
+    team = FlexibleForeignKey("sentry.Team", null=True, db_index=True, on_delete=models.CASCADE)
+    user_id = HybridCloudForeignKey("sentry.User", null=True, db_index=True, on_delete="CASCADE")
     organization = FlexibleForeignKey("sentry.Organization")
     integration_id = HybridCloudForeignKey("sentry.Integration", on_delete="CASCADE")
     provider = BoundedPositiveIntegerField(
@@ -44,19 +45,24 @@ class ExternalActor(DefaultFieldsModel):
     class Meta:
         app_label = "sentry"
         db_table = "sentry_externalactor"
-        unique_together = (("organization", "provider", "external_name", "actor"),)
+        unique_together = (
+            ("organization", "provider", "external_name", "actor"),
+            ("organization", "provider", "external_name", "team_id"),
+            ("organization", "provider", "external_name", "user_id"),
+        )
 
     def delete(self, **kwargs):
         from sentry.services.hybrid_cloud.integration import integration_service
 
         integration = integration_service.get_integration(integration_id=self.integration_id)
-        install = integration.get_installation(organization_id=self.organization.id)
+        if integration:
+            install = integration.get_installation(organization_id=self.organization.id)
 
-        team = self.actor.resolve()
-        install.notify_remove_external_team(external_team=self, team=team)
-        notifications_service.remove_notification_settings_for_team(
-            team_id=team.id, provider=ExternalProviders(self.provider)
-        )
+            team = self.actor.resolve()
+            install.notify_remove_external_team(external_team=self, team=team)
+            notifications_service.remove_notification_settings_for_team(
+                team_id=team.id, provider=ExternalProviders(self.provider)
+            )
 
         return super().delete(**kwargs)