Просмотр исходного кода

test(hybrid-cloud): Add test for outbox generation with HC foreign keys (#58247)

This PR adds a test to ensure that any models linked with
`HybridCloudForeignKey`s generate outboxes. Without this assertion
verified, we may have linked models assuming they'll be updated when the
foreign silo parent is deleted/updated but the updates aren't actually
making their way to the linked model.

I believe this is the case for:
```
NotificationSetting -> Actor
NotificationAction -> SentryApp
AlertRuleTriggerAction -> SentryApp
```
which all have `on_delete` schemes specified but may not occur since no
outboxes are generated for the associated models (Actor, SentryApp).

The check is very primitive right now, just ensuring
`outboxes_for_update` or `outbox_for_update` is specified, but I'm not
100% on a better way to verify this.
Leander Rodrigues 1 год назад
Родитель
Сommit
1b1a61a7f2

+ 14 - 0
src/sentry/models/actor.py

@@ -16,6 +16,7 @@ from sentry.backup.scopes import ImportScope, RelocationScope
 from sentry.db.models import Model, region_silo_only_model
 from sentry.db.models.fields.foreignkey import FlexibleForeignKey
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
+from sentry.models.outbox import OutboxCategory, OutboxScope, RegionOutbox, outbox_context
 from sentry.services.hybrid_cloud.user import RpcUser
 from sentry.services.hybrid_cloud.user.service import user_service
 
@@ -134,6 +135,19 @@ class Actor(Model):
         app_label = "sentry"
         db_table = "sentry_actor"
 
+    def outbox_for_update(self) -> RegionOutbox:
+        return RegionOutbox(
+            shard_scope=OutboxScope.ORGANIZATION_SCOPE,
+            shard_identifier=self.id,
+            object_identifier=self.id,
+            category=OutboxCategory.ACTOR_UPDATE,
+        )
+
+    def delete(self, **kwargs):
+        with outbox_context(transaction.atomic(router.db_for_write(Actor))):
+            self.outbox_for_update().save()
+        return super().delete(**kwargs)
+
     def resolve(self) -> Union[Team, RpcUser]:
         # Returns User/Team model object
         return fetch_actor_by_actor_id(actor_type_to_class(self.type), self.id)

+ 20 - 1
src/sentry/models/integrations/sentry_app.py

@@ -2,8 +2,9 @@ import hmac
 import itertools
 import uuid
 from hashlib import sha256
+from typing import List
 
-from django.db import models
+from django.db import models, router, transaction
 from django.db.models import QuerySet
 from django.utils import timezone
 from django.utils.text import slugify
@@ -26,6 +27,8 @@ from sentry.db.models import (
 from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
 from sentry.db.models.fields.jsonfield import JSONField
 from sentry.models.apiscopes import HasApiScopes
+from sentry.models.outbox import ControlOutbox, OutboxCategory, OutboxScope, outbox_context
+from sentry.types.region import find_all_region_names
 from sentry.utils import metrics
 
 # When a developer selects to receive "<Resource> Webhooks" it really means
@@ -211,9 +214,25 @@ class SentryApp(ParanoidModel, HasApiScopes):
         encoded_scopes = set({"%s" % scope for scope in list(access.scopes)})
         return set(self.scope_list).issubset(encoded_scopes)
 
+    def outboxes_for_update(self) -> List[ControlOutbox]:
+        return [
+            ControlOutbox(
+                shard_scope=OutboxScope.APP_SCOPE,
+                shard_identifier=self.id,
+                object_identifier=self.id,
+                category=OutboxCategory.SENTRY_APP_UPDATE,
+                region_name=region_name,
+            )
+            for region_name in find_all_region_names()
+        ]
+
     def delete(self):
         from sentry.models.avatars.sentry_app_avatar import SentryAppAvatar
 
+        with outbox_context(transaction.atomic(using=router.db_for_write(SentryApp))):
+            for outbox in self.outboxes_for_update():
+                outbox.save()
+
         SentryAppAvatar.objects.filter(sentry_app=self).delete()
         return super().delete()
 

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

@@ -106,6 +106,8 @@ class OutboxCategory(IntEnum):
     ORGANIZATION_SLUG_RESERVATION_UPDATE = 27
     API_KEY_UPDATE = 28
     PARTNER_ACCOUNT_UPDATE = 29
+    SENTRY_APP_UPDATE = 30
+    ACTOR_UPDATE = 31
 
     @classmethod
     def as_choices(cls):
@@ -293,6 +295,7 @@ class OutboxScope(IntEnum):
             OutboxCategory.API_KEY_UPDATE,
             OutboxCategory.ORGANIZATION_SLUG_RESERVATION_UPDATE,
             OutboxCategory.PARTNER_ACCOUNT_UPDATE,
+            OutboxCategory.ACTOR_UPDATE,
         },
     )
     USER_SCOPE = scope_categories(
@@ -324,6 +327,7 @@ class OutboxScope(IntEnum):
         {
             OutboxCategory.API_APPLICATION_UPDATE,
             OutboxCategory.SENTRY_APP_INSTALLATION_UPDATE,
+            OutboxCategory.SENTRY_APP_UPDATE,
         },
     )
     # Deprecate?

+ 12 - 0
src/sentry/receivers/outbox/control.py

@@ -14,6 +14,7 @@ from django.dispatch import receiver
 
 from sentry.models.apiapplication import ApiApplication
 from sentry.models.integrations.integration import Integration
+from sentry.models.integrations.sentry_app import SentryApp
 from sentry.models.integrations.sentry_app_installation import SentryAppInstallation
 from sentry.models.outbox import OutboxCategory, process_control_outbox
 from sentry.receivers.outbox import maybe_process_tombstone
@@ -34,6 +35,17 @@ def process_integration_updates(object_identifier: int, region_name: str, **kwds
     integration  # Currently we do not sync any other integration changes, but if we did, you can use this variable.
 
 
+@receiver(process_control_outbox, sender=OutboxCategory.SENTRY_APP_UPDATE)
+def process_sentry_app_updates(object_identifier: int, region_name: str, **kwds: Any):
+    if (
+        sentry_app := maybe_process_tombstone(
+            model=SentryApp, object_identifier=object_identifier, region_name=region_name
+        )
+    ) is None:
+        return
+    sentry_app  # Currently we do not sync any other sentry_app changes, but if we did, you can use this variable.
+
+
 @receiver(process_control_outbox, sender=OutboxCategory.API_APPLICATION_UPDATE)
 def process_api_application_updates(object_identifier: int, region_name: str, **kwds: Any):
     if (

+ 8 - 0
src/sentry/receivers/outbox/region.py

@@ -11,6 +11,7 @@ from typing import Any
 
 from django.dispatch import receiver
 
+from sentry.models.actor import Actor
 from sentry.models.authproviderreplica import AuthProviderReplica
 from sentry.models.organization import Organization
 from sentry.models.outbox import OutboxCategory, process_region_outbox
@@ -51,6 +52,13 @@ def process_project_updates(object_identifier: int, **kwds: Any):
     proj
 
 
+@receiver(process_region_outbox, sender=OutboxCategory.ACTOR_UPDATE)
+def process_actor_updates(object_identifier: int, **kwds: Any):
+    if (actor := maybe_process_tombstone(Actor, object_identifier)) is None:
+        return
+    actor
+
+
 @receiver(process_region_outbox, sender=OutboxCategory.ORGANIZATION_MAPPING_CUSTOMER_ID_UPDATE)
 def process_organization_mapping_customer_id_update(
     object_identifier: int, payload: Any, **kwds: Any

+ 26 - 0
tests/sentry/db/models/fields/test_hybrid_cloud_foreign_key.py

@@ -0,0 +1,26 @@
+from django.apps import apps
+
+from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
+from sentry.testutils.pytest.fixtures import django_db_all
+
+
+@django_db_all
+def test_all_hybrid_cloud_foreign_keys_generate_outboxes():
+    hcfk_models = set()
+    for app_models in apps.all_models.values():
+        for model in app_models.values():
+            if not hasattr(model._meta, "silo_limit"):
+                continue
+            for field in model._meta.fields:
+                if not isinstance(field, HybridCloudForeignKey):
+                    continue
+                if field.on_delete.upper() in {"CASCADE", "SET_NULL"}:
+                    hcfk_models.add((model, field.foreign_model))
+    for model, foreign_model in hcfk_models:
+        if not (
+            hasattr(foreign_model, "outboxes_for_update")
+            or hasattr(foreign_model, "outbox_for_update")
+        ):
+            raise NotImplementedError(
+                f"{model.__name__} model uses a HybridCloudForeignKey to the {foreign_model.__name__} model, but it does not produce outboxes!"
+            )