Browse Source

chore(hybrid-cloud): Break UserOption.organization fk (#45229)

In preparation for `Organization` fk breakage, ensuring that outboxes
are created on delete, breaking an organization fk (from user option),
and testing it all e2e.
Zach Collins 2 years ago
parent
commit
3c3f07e923

+ 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: 0368_monitor_remove_slug_nullable
+sentry: 0369_break_useroption_org_fk
 social_auth: 0001_initial

+ 3 - 3
src/sentry/api/endpoints/user_notification_details.py

@@ -22,7 +22,7 @@ from sentry.types.integrations import ExternalProviders
 class UserNotificationsSerializer(Serializer):
     def get_attrs(self, item_list, user, *args, **kwargs):
         user_options = UserOption.objects.filter(
-            user__in=item_list, organization=None, project=None
+            user__in=item_list, organization_id=None, project_id=None
         ).select_related("user")
         keys_to_user_option_objects = {user_option.key: user_option for user_option in user_options}
 
@@ -106,8 +106,8 @@ class UserNotificationDetailsEndpoint(UserEndpoint):
                 user_option, _ = UserOption.objects.get_or_create(
                     key=USER_OPTION_SETTINGS[key]["key"],
                     user=user,
-                    project=None,
-                    organization=None,
+                    project_id=None,
+                    organization_id=None,
                 )
                 user_option.update(value=str(int(value)))
 

+ 2 - 7
src/sentry/api/endpoints/user_notification_fine_tuning.py

@@ -7,7 +7,7 @@ from sentry.api.base import control_silo_endpoint
 from sentry.api.bases.user import UserEndpoint
 from sentry.api.serializers import serialize
 from sentry.api.serializers.models import UserNotificationsSerializer
-from sentry.models import NotificationSetting, Project, UserEmail, UserOption
+from sentry.models import NotificationSetting, UserEmail, UserOption
 from sentry.notifications.types import FineTuningAPIKey
 from sentry.notifications.utils.legacy_mappings import (
     get_option_value_from_int,
@@ -155,17 +155,12 @@ class UserNotificationFineTuningEndpoint(UserEndpoint):
         if len(emails) != len(emails_to_check):
             return Response({"detail": INVALID_EMAIL_MSG}, status=status.HTTP_400_BAD_REQUEST)
 
-        project_ids = [int(id) for id in data.keys()]
-        projects_map = {
-            int(project.id): project for project in Project.objects.filter(id__in=project_ids)
-        }
-
         with transaction.atomic():
             for id, value in data.items():
                 user_option, CREATED = UserOption.objects.get_or_create(
                     user=user,
                     key="mail:email",
-                    project=projects_map[int(id)],
+                    project_id=int(id),
                 )
                 user_option.update(value=str(value))
 

+ 1 - 1
src/sentry/api/serializers/models/user.py

@@ -192,7 +192,7 @@ class UserSerializer(Serializer):  # type: ignore
             d = cast(UserSerializerResponseSelf, d)
             options = {
                 o.key: o.value
-                for o in UserOption.objects.filter(user_id=user.id, project__isnull=True)
+                for o in UserOption.objects.filter(user_id=user.id, project_id__isnull=True)
             }
             stacktrace_order = int(options.get("stacktrace_order", -1) or -1)
 

+ 6 - 8
src/sentry/api/serializers/models/user_notifications.py

@@ -15,16 +15,14 @@ def handle_legacy(notification_type: FineTuningAPIKey, users: Iterable) -> Itera
     """For EMAIL and REPORTS, check UserOptions."""
     filter_args = {}
     if notification_type == FineTuningAPIKey.EMAIL:
-        filter_args["project__isnull"] = False
+        filter_args["project_id__isnull"] = False
 
     key = {
         FineTuningAPIKey.EMAIL: "mail:email",
         FineTuningAPIKey.REPORTS: "reports:disabled-organizations",
     }.get(notification_type)
 
-    return UserOption.objects.filter(key=key, user__in=users, **filter_args).select_related(
-        "user", "project", "organization"
-    )
+    return UserOption.objects.filter(key=key, user__in=users, **filter_args).select_related("user")
 
 
 class UserNotificationsSerializer(Serializer):
@@ -59,8 +57,8 @@ class UserNotificationsSerializer(Serializer):
                 # This UserOption should have both project + organization = None
                 for org_id in uo.value:
                     data[org_id] = "0"
-            elif uo.project is not None:
-                data[uo.project.id] = str(uo.value)
-            elif uo.organization is not None:
-                data[uo.organization.id] = str(uo.value)
+            elif uo.project_id is not None:
+                data[uo.project_id] = str(uo.value)
+            elif uo.organization_id is not None:
+                data[uo.organization_id] = str(uo.value)
         return data

+ 76 - 0
src/sentry/migrations/0369_break_useroption_org_fk.py

@@ -0,0 +1,76 @@
+# Generated by Django 2.2.28 on 2023-02-28 23:54
+
+from django.db import migrations
+
+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", "0368_monitor_remove_slug_nullable"),
+    ]
+
+    database_operations = [
+        migrations.AlterField(
+            model_name="useroption",
+            name="organization",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                to="sentry.Organization", db_constraint=False, db_index=True, null=True
+            ),
+        ),
+        migrations.AlterField(
+            model_name="useroption",
+            name="project",
+            field=sentry.db.models.fields.foreignkey.FlexibleForeignKey(
+                to="sentry.Project", db_constraint=False, db_index=True, null=True
+            ),
+        ),
+    ]
+
+    state_operations = [
+        migrations.AlterField(
+            model_name="useroption",
+            name="organization",
+            field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                "sentry.Organization", null=True, on_delete="CASCADE"
+            ),
+        ),
+        migrations.RenameField(
+            model_name="useroption",
+            old_name="organization",
+            new_name="organization_id",
+        ),
+        migrations.AlterField(
+            model_name="useroption",
+            name="project",
+            field=sentry.db.models.fields.hybrid_cloud_foreign_key.HybridCloudForeignKey(
+                "sentry.Project", null=True, on_delete="CASCADE"
+            ),
+        ),
+        migrations.RenameField(
+            model_name="useroption",
+            old_name="project",
+            new_name="project_id",
+        ),
+        migrations.AlterUniqueTogether(
+            name="useroption",
+            unique_together=(("user", "project_id", "key"), ("user", "organization_id", "key")),
+        ),
+    ]
+
+    operations = database_operations + [
+        migrations.SeparateDatabaseAndState(state_operations=state_operations)
+    ]

+ 21 - 12
src/sentry/models/options/user_option.py

@@ -7,6 +7,7 @@ from django.db import models
 
 from sentry.db.models import FlexibleForeignKey, Model, control_silo_only_model, sane_repr
 from sentry.db.models.fields import PickledObjectField
+from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
 from sentry.db.models.manager import OptionManager, Value
 
 if TYPE_CHECKING:
@@ -20,14 +21,16 @@ class UserOptionManager(OptionManager["User"]):
     def _make_key(
         self,
         user: User | RpcUser | int,
-        project: Project | None = None,
-        organization: Organization | None = None,
+        project: Project | int | None = None,
+        organization: Organization | int | None = None,
     ) -> str:
         uid = user.id if user and not isinstance(user, int) else user
+        org_id: int | None = organization.id if isinstance(organization, Model) else organization
+        proj_id: int | None = project.id if isinstance(project, Model) else project
         if project:
-            metakey = f"{uid}:{project.id}:project"
+            metakey = f"{uid}:{proj_id}:project"
         elif organization:
-            metakey = f"{uid}:{organization.id}:organization"
+            metakey = f"{uid}:{org_id}:organization"
         else:
             metakey = f"{uid}:user"
 
@@ -96,8 +99,8 @@ class UserOptionManager(OptionManager["User"]):
     def get_all_values(
         self,
         user: User | RpcUser | int,
-        project: Project | None = None,
-        organization: Organization | None = None,
+        project: Project | int | None = None,
+        organization: Organization | int | None = None,
         force_reload: bool = False,
     ) -> Mapping[str, Value]:
         if organization and project:
@@ -105,11 +108,17 @@ class UserOptionManager(OptionManager["User"]):
 
         uid = user.id if user and not isinstance(user, int) else user
         metakey = self._make_key(user, project=project, organization=organization)
+        project_id: int | None = project.id if isinstance(project, Model) else project
+        organization_id: int | None = (
+            organization.id if isinstance(organization, Model) else organization
+        )
 
         if metakey not in self._option_cache or force_reload:
             result = {
                 i.key: i.value
-                for i in self.filter(user_id=uid, project=project, organization=organization)
+                for i in self.filter(
+                    user_id=uid, project_id=project_id, organization_id=organization_id
+                )
             }
             self._option_cache[metakey] = result
 
@@ -119,12 +128,12 @@ class UserOptionManager(OptionManager["User"]):
 
     def post_save(self, instance: UserOption, **kwargs: Any) -> None:
         self.get_all_values(
-            instance.user, instance.project, instance.organization, force_reload=True
+            instance.user, instance.project_id, instance.organization_id, force_reload=True
         )
 
     def post_delete(self, instance: UserOption, **kwargs: Any) -> None:
         self.get_all_values(
-            instance.user, instance.project, instance.organization, force_reload=True
+            instance.user, instance.project_id, instance.organization_id, force_reload=True
         )
 
 
@@ -183,8 +192,8 @@ class UserOption(Model):  # type: ignore
     __include_in_export__ = True
 
     user = FlexibleForeignKey(settings.AUTH_USER_MODEL)
-    project = FlexibleForeignKey("sentry.Project", null=True)
-    organization = FlexibleForeignKey("sentry.Organization", null=True)
+    project_id = HybridCloudForeignKey("sentry.Project", null=True, on_delete="CASCADE")
+    organization_id = HybridCloudForeignKey("sentry.Organization", null=True, on_delete="CASCADE")
     key = models.CharField(max_length=64)
     value = PickledObjectField()
 
@@ -193,6 +202,6 @@ class UserOption(Model):  # type: ignore
     class Meta:
         app_label = "sentry"
         db_table = "sentry_useroption"
-        unique_together = (("user", "project", "key"), ("user", "organization", "key"))
+        unique_together = (("user", "project_id", "key"), ("user", "organization_id", "key"))
 
     __repr__ = sane_repr("user_id", "project_id", "organization_id", "key", "value")

+ 4 - 1
src/sentry/models/organization.py

@@ -30,6 +30,7 @@ from sentry.db.models import (
     sane_repr,
 )
 from sentry.db.models.utils import slugify_instance
+from sentry.db.postgres.roles import in_test_psql_role_override
 from sentry.locks import locks
 from sentry.models.organizationmember import OrganizationMember
 from sentry.models.organizationmemberteam import OrganizationMemberTeam
@@ -271,7 +272,9 @@ class Organization(Model, SnowflakeIdMixin):
         # There is no foreign key relationship so we have to manually cascade.
         NotificationSetting.objects.remove_for_organization(self)
 
-        return super().delete(**kwargs)
+        with transaction.atomic(), in_test_psql_role_override("postgres"):
+            Organization.outbox_for_update(self.id).save()
+            return super().delete(**kwargs)
 
     @staticmethod
     def outbox_for_update(org_id: int) -> RegionOutbox:

+ 1 - 0
src/sentry/models/outbox.py

@@ -54,6 +54,7 @@ class OutboxCategory(IntEnum):
     AUDIT_LOG_EVENT = 5
     USER_IP_EVENT = 6
     INTEGRATION_UPDATE = 7
+    PROJECT_UPDATE = 8
 
     @classmethod
     def as_choices(cls):

+ 14 - 2
src/sentry/models/project.py

@@ -29,7 +29,9 @@ from sentry.db.models import (
     sane_repr,
 )
 from sentry.db.models.utils import slugify_instance
+from sentry.db.postgres.roles import in_test_psql_role_override
 from sentry.locks import locks
+from sentry.models.outbox import OutboxCategory, OutboxScope, RegionOutbox
 from sentry.snuba.models import SnubaQuery
 from sentry.utils import metrics
 from sentry.utils.colors import get_hashed_color
@@ -475,13 +477,23 @@ class Project(Model, PendingDeletionMixin, SnowflakeIdMixin):
             return True
         return integration_doc_exists(value)
 
+    @staticmethod
+    def outbox_for_update(project_identifier: int, organization_identifier: int) -> RegionOutbox:
+        return RegionOutbox(
+            shard_scope=OutboxScope.ORGANIZATION_SCOPE,
+            shard_identifier=organization_identifier,
+            category=OutboxCategory.PROJECT_UPDATE,
+            object_identifier=project_identifier,
+        )
+
     def delete(self, **kwargs):
         from sentry.models import NotificationSetting
 
         # There is no foreign key relationship so we have to manually cascade.
         NotificationSetting.objects.remove_for_project(self)
-
-        return super().delete(**kwargs)
+        with transaction.atomic(), in_test_psql_role_override("postgres"):
+            Project.outbox_for_update(self.id, self.organization_id).save()
+            return super().delete(**kwargs)
 
 
 pre_delete.connect(delete_pending_deletion_option, sender=Project, weak=False)

Some files were not shown because too many files changed in this diff