Browse Source

chore(actor) Remove grouphistory.actor_id from state (#69976)

- Remove grouphistory.actor_id from state
- Remove foreign key between grouphistory and actor.

Refs HC-1175
Mark Story 10 months ago
parent
commit
6b1ae049e4

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

@@ -2363,11 +2363,6 @@
   "sentry.grouphistory": {
     "dangling": false,
     "foreign_keys": {
-      "actor": {
-        "kind": "FlexibleForeignKey",
-        "model": "sentry.actor",
-        "nullable": true
-      },
       "group": {
         "kind": "FlexibleForeignKey",
         "model": "sentry.group",

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

@@ -325,7 +325,6 @@
     "sentry.project"
   ],
   "sentry.grouphistory": [
-    "sentry.actor",
     "sentry.group",
     "sentry.organization",
     "sentry.project",

+ 1 - 1
migrations_lockfile.txt

@@ -9,5 +9,5 @@ feedback: 0004_index_together
 hybridcloud: 0016_add_control_cacheversion
 nodestore: 0002_nodestore_no_dictfield
 replays: 0004_index_together
-sentry: 0709_alertrule_remove_owner_state
+sentry: 0710_grouphistory_remove_actor_state
 social_auth: 0002_default_auto_field

+ 2 - 0
src/sentry/backup/imports.py

@@ -70,6 +70,8 @@ DELETED_FIELDS: dict[
     "sentry.rule": {"owner"},
     # TODO(mark): Safe to remove after july 2024 after self-hosted 24.6.0 is released
     "sentry.alertrule": {"owner"},
+    # TODO(mark): Safe to remove after july 2024 after self-hosted 24.6.0 is released
+    "sentry.grouphistory": {"actor"},
 }
 
 # The maximum number of models that may be sent at a time.

+ 46 - 0
src/sentry/migrations/0710_grouphistory_remove_actor_state.py

@@ -0,0 +1,46 @@
+# Generated by Django 5.0.4 on 2024-04-30 18:07
+
+from django.db import migrations
+
+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.
+    # 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 post deployment:
+    # - Large data migrations. Typically we want these to be run manually 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
+    #   run this outside deployments so that we don't block them. Note that while adding an index
+    #   is a schema change, it's completely safe to run the operation after the code has deployed.
+    # Once deployed, run these manually via: https://develop.sentry.dev/database-migrations/#migration-deployment
+
+    is_post_deployment = False
+
+    dependencies = [
+        ("sentry", "0709_alertrule_remove_owner_state"),
+    ]
+
+    operations = [
+        # Remove the foreign key constraint from schema and remove the field from django metadata
+        migrations.SeparateDatabaseAndState(
+            database_operations=[
+                migrations.RunSQL(
+                    sql="""
+                    ALTER TABLE "sentry_grouphistory" DROP CONSTRAINT IF EXISTS "sentry_grouphistory_actor_id_085453d6_fk_sentry_actor_id"
+                    """,
+                    reverse_sql="",
+                    hints={"tables": ["sentry_grouphistory"]},
+                )
+            ],
+            state_operations=[
+                migrations.RemoveField(
+                    model_name="grouphistory",
+                    name="actor",
+                ),
+            ],
+        )
+    ]

+ 1 - 21
src/sentry/models/grouphistory.py

@@ -2,7 +2,7 @@ from typing import TYPE_CHECKING, ClassVar, Optional, Union
 
 from django.conf import settings
 from django.db import models
-from django.db.models import SET_NULL, Q
+from django.db.models import Q
 from django.utils import timezone
 from django.utils.translation import gettext_lazy as _
 
@@ -16,7 +16,6 @@ from sentry.db.models import (
     sane_repr,
 )
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
-from sentry.models.actor import get_actor_for_user
 from sentry.types.activity import ActivityType
 from sentry.types.group import GROUP_SUBSTATUS_TO_GROUP_HISTORY_STATUS
 
@@ -183,9 +182,6 @@ class GroupHistory(Model):
     project = FlexibleForeignKey("sentry.Project", db_constraint=False)
     release = FlexibleForeignKey("sentry.Release", null=True, db_constraint=False)
 
-    # Deprecated. Use user_id and team instead.
-    actor = FlexibleForeignKey("sentry.Actor", null=True, on_delete=SET_NULL)
-
     user_id = HybridCloudForeignKey(settings.AUTH_USER_MODEL, null=True, on_delete="SET_NULL")
     team = FlexibleForeignKey("sentry.Team", null=True, on_delete=models.SET_NULL)
 
@@ -217,14 +213,6 @@ class GroupHistory(Model):
     )  # This field is used to simplify query calculations.
     date_added = models.DateTimeField(default=timezone.now)
 
-    def _validate_owner(self) -> None:
-        if self.actor_id is not None and self.team_id is None and self.user_id is None:
-            raise ValueError("GroupHistory with actor requires either team_id or user_id")
-
-    def save(self, *args, **kwargs):
-        self._validate_owner()
-        return super().save(*args, **kwargs)
-
     class Meta:
         db_table = "sentry_grouphistory"
         app_label = "sentry"
@@ -284,15 +272,12 @@ def record_group_history(
     from sentry.services.hybrid_cloud.user import RpcUser
 
     prev_history = get_prev_history(group, status)
-    actor_id = None
     user_id = None
     team_id = None
     if actor:
         if isinstance(actor, RpcUser) or isinstance(actor, User):
-            actor_id = get_actor_for_user(actor).id
             user_id = actor.id
         elif isinstance(actor, Team):
-            actor_id = actor.actor_id
             team_id = actor.id
         else:
             raise ValueError("record_group_history actor argument must be RPCUser or Team")
@@ -302,7 +287,6 @@ def record_group_history(
         group=group,
         project=group.project,
         release=release,
-        actor_id=actor_id,
         user_id=user_id,
         team_id=team_id,
         status=status,
@@ -325,15 +309,12 @@ def bulk_record_group_history(
         prev_history = get_prev_history(group, status)
         return prev_history.date_added if prev_history else None
 
-    actor_id: int | None = None
     user_id: int | None = None
     team_id: int | None = None
     if actor:
         if isinstance(actor, RpcUser) or isinstance(actor, User):
-            actor_id = get_actor_for_user(actor).id
             user_id = actor.id
         elif isinstance(actor, Team):
-            actor_id = actor.actor_id
             team_id = actor.id
         else:
             raise ValueError("record_group_history actor argument must be RPCUser or Team")
@@ -345,7 +326,6 @@ def bulk_record_group_history(
                 group=group,
                 project=group.project,
                 release=release,
-                actor_id=actor_id,
                 team_id=team_id,
                 user_id=user_id,  # type:ignore[misc]
                 status=status,

+ 4 - 5
src/sentry/testutils/factories.py

@@ -56,7 +56,6 @@ from sentry.incidents.utils.types import AlertRuleActivationConditionType
 from sentry.issues.grouptype import get_group_type_by_type_id
 from sentry.mediators.token_exchange.grant_exchanger import GrantExchanger
 from sentry.models.activity import Activity
-from sentry.models.actor import Actor
 from sentry.models.apikey import ApiKey
 from sentry.models.apitoken import ApiToken
 from sentry.models.artifactbundle import ArtifactBundle
@@ -1744,7 +1743,8 @@ class Factories:
         group: Group,
         status: int,
         release: Release | None = None,
-        actor: Actor | None = None,
+        user_id: int | None = None,
+        team_id: int | None = None,
         prev_history: GroupHistory | None = None,
         date_added: datetime | None = None,
     ) -> GroupHistory:
@@ -1760,9 +1760,8 @@ class Factories:
             group=group,
             project=group.project,
             release=release,
-            actor=actor,
-            user_id=actor.user_id if actor else None,
-            team_id=actor.team_id if actor else None,
+            user_id=user_id,
+            team_id=team_id,
             status=status,
             prev_history=prev_history,
             prev_history_date=prev_history_date,

+ 2 - 3
src/sentry/testutils/fixtures.py

@@ -12,7 +12,6 @@ from sentry.eventstore.models import Event
 from sentry.incidents.models.alert_rule import AlertRuleMonitorType
 from sentry.incidents.models.incident import IncidentActivityType
 from sentry.models.activity import Activity
-from sentry.models.actor import Actor, get_actor_for_user
 from sentry.models.grouprelease import GroupRelease
 from sentry.models.identity import Identity, IdentityProvider
 from sentry.models.integrations.integration import Integration
@@ -562,8 +561,8 @@ class Fixtures:
         return Factories.create_identity_provider(integration=integration, config=config, **kwargs)
 
     def create_group_history(self, *args, **kwargs):
-        if "actor" not in kwargs:
-            kwargs["actor"] = Actor.objects.get(id=get_actor_for_user(self.user).id)
+        if "user_id" not in kwargs and "team" not in kwargs and "team_id" not in kwargs:
+            kwargs["user_id"] = self.user.id
         return Factories.create_group_history(*args, **kwargs)
 
     def create_comment(self, *args, **kwargs):

+ 10 - 11
tests/sentry/api/endpoints/test_team_time_to_resolution.py

@@ -2,7 +2,6 @@ from datetime import timedelta
 
 from django.utils.timezone import now
 
-from sentry.models.actor import get_actor_for_user
 from sentry.models.groupassignee import GroupAssignee
 from sentry.models.groupenvironment import GroupEnvironment
 from sentry.models.grouphistory import GroupHistoryStatus
@@ -25,14 +24,14 @@ class TeamTimeToResolutionTest(APITestCase):
         gh1 = self.create_group_history(
             group1,
             GroupHistoryStatus.UNRESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             date_added=before_now(days=5),
         )
 
         self.create_group_history(
             group1,
             GroupHistoryStatus.RESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             prev_history=gh1,
             date_added=before_now(days=2),
         )
@@ -40,14 +39,14 @@ class TeamTimeToResolutionTest(APITestCase):
         gh2 = self.create_group_history(
             group2,
             GroupHistoryStatus.UNRESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             date_added=before_now(days=10),
         )
 
         self.create_group_history(
             group2,
             GroupHistoryStatus.RESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             prev_history=gh2,
         )
         today = str(now().date())
@@ -66,13 +65,13 @@ class TeamTimeToResolutionTest(APITestCase):
         gh2 = self.create_group_history(
             group2,
             GroupHistoryStatus.UNRESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             date_added=before_now(days=5),
         )
         self.create_group_history(
             group2,
             GroupHistoryStatus.RESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             prev_history=gh2,
         )
 
@@ -80,14 +79,14 @@ class TeamTimeToResolutionTest(APITestCase):
         self.create_group_history(
             group2,
             GroupHistoryStatus.DELETED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             prev_history=gh2,
         )
         # Make sure that if we have a `GroupHistory` row with no prev history then we don't crash.
         self.create_group_history(
             group2,
             GroupHistoryStatus.RESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
         )
 
         response = self.get_success_response(self.team.organization.slug, self.team.slug)
@@ -107,14 +106,14 @@ class TeamTimeToResolutionTest(APITestCase):
         gh1 = self.create_group_history(
             group1,
             GroupHistoryStatus.UNRESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             date_added=before_now(days=5),
         )
 
         self.create_group_history(
             group1,
             GroupHistoryStatus.RESOLVED,
-            actor=get_actor_for_user(self.user),
+            user_id=self.user.id,
             prev_history=gh1,
             date_added=before_now(days=2),
         )

+ 0 - 1
tests/sentry/backup/snapshots/test_comparators/test_default_comparators.pysnap

@@ -594,7 +594,6 @@ source: tests/sentry/backup/test_comparators.py
 - comparators:
   - class: ForeignKeyComparator
     fields:
-    - actor
     - group
     - organization
     - project