Browse Source

ref(hc): Begin writing team and user ids into external actors (#55298)

First step in the ExternalActor removal of actor column is to begin
writing team ids and user ids on the current write paths.

This PR (https://github.com/getsentry/sentry/pull/54771/files) confirms
(via removing the actor column altogether) that these are in fact the
only write paths (in non tests).

After deploying this, there will be a backfill that writes in team /
user ids from actors alongside this code writing it for new entries.

Then, I can merge the read paths in ^^ PR to switch all read paths to
the new columns.

Lastly I can remove the null constraint, and drop the actor column.
Zach Collins 1 year ago
parent
commit
2bebc17cb5

+ 10 - 8
src/sentry/api/bases/external_actor.py

@@ -1,4 +1,4 @@
-from typing import Any, MutableMapping, Optional
+from typing import Any, Mapping, MutableMapping, Optional
 
 from django.db import IntegrityError
 from django.http import Http404
@@ -15,8 +15,7 @@ from sentry.api.validators.external_actor import (
     validate_integration_id,
 )
 from sentry.api.validators.integrations import validate_provider
-from sentry.models import ExternalActor, Organization, Team
-from sentry.models.actor import Actor, get_actor_for_user
+from sentry.models import Actor, ExternalActor, Organization, Team, get_actor_for_user
 from sentry.services.hybrid_cloud.organization import organization_service
 from sentry.services.hybrid_cloud.user import RpcUser
 from sentry.services.hybrid_cloud.user.service import user_service
@@ -63,20 +62,23 @@ class ExternalActorSerializerBase(CamelSnakeModelSerializer):
         provider = validate_provider(provider_name_option, available_providers=AVAILABLE_PROVIDERS)
         return int(provider.value)
 
-    def get_actor_id(self, validated_data: MutableMapping[str, Any]) -> int:
+    def get_actor_params(self, validated_data: MutableMapping[str, Any]) -> Mapping[str, int]:
         actor_model = validated_data.pop(self._actor_key)
         if isinstance(actor_model, Team):
-            actor = Actor.objects.get(**{self._actor_key: actor_model.id})
+            actor = Actor.objects.get(**{"team_id": actor_model.id})
+            return dict(team_id=actor_model.id, actor_id=actor.id)
         else:
             actor = get_actor_for_user(actor_model)
-        return int(actor.id)
+            return dict(user_id=actor_model.id, actor_id=actor.id)
 
     def create(self, validated_data: MutableMapping[str, Any]) -> ExternalActor:
-        actor_id = self.get_actor_id(validated_data)
+        actor_params = self.get_actor_params(validated_data)
+        actor_id = actor_params.pop("actor_id")
         return ExternalActor.objects.get_or_create(
             **validated_data,
             actor_id=actor_id,
             organization=self.organization,
+            defaults=actor_params,
         )
 
     def update(
@@ -87,7 +89,7 @@ class ExternalActorSerializerBase(CamelSnakeModelSerializer):
             validated_data.pop("id")
 
         if self._actor_key in validated_data:
-            validated_data["actor_id"] = self.get_actor_id({**validated_data})
+            validated_data.update(self.get_actor_params({**validated_data}))
 
         for key, value in validated_data.items():
             setattr(self.instance, key, value)

+ 2 - 2
src/sentry/api/endpoints/codeowners/external_actor/user_index.py

@@ -4,7 +4,7 @@ from rest_framework import status
 from rest_framework.request import Request
 from rest_framework.response import Response
 
-from sentry.api.base import control_silo_endpoint
+from sentry.api.base import region_silo_endpoint
 from sentry.api.bases import OrganizationEndpoint
 from sentry.api.bases.external_actor import ExternalActorEndpointMixin, ExternalUserSerializer
 from sentry.api.serializers import serialize
@@ -13,7 +13,7 @@ from sentry.models import Organization
 logger = logging.getLogger(__name__)
 
 
-@control_silo_endpoint
+@region_silo_endpoint
 class ExternalUserEndpoint(OrganizationEndpoint, ExternalActorEndpointMixin):
     def post(self, request: Request, organization: Organization) -> Response:
         """

+ 8 - 6
tests/sentry/api/endpoints/test_external_user.py

@@ -1,9 +1,10 @@
 from sentry.models import Integration
+from sentry.silo import SiloMode
 from sentry.testutils.cases import APITestCase
-from sentry.testutils.silo import control_silo_test
+from sentry.testutils.silo import assume_test_silo_mode, region_silo_test
 
 
-@control_silo_test  # TODO(hybrid-cloud): blocked on org membership mapping
+@region_silo_test(stable=True)
 class ExternalUserTest(APITestCase):
     endpoint = "sentry-api-0-organization-external-user"
     method = "post"
@@ -13,11 +14,12 @@ class ExternalUserTest(APITestCase):
         self.login_as(self.user)
 
         self.org_slug = self.organization.slug  # force creation
-        self.integration = Integration.objects.create(
-            provider="github", name="GitHub", external_id="github:1"
-        )
+        with assume_test_silo_mode(SiloMode.CONTROL):
+            self.integration = Integration.objects.create(
+                provider="github", name="GitHub", external_id="github:1"
+            )
 
-        self.integration.add_organization(self.organization, self.user)
+            self.integration.add_organization(self.organization, self.user)
         self.data = {
             "externalName": "@NisanthanNanthakumar",
             "provider": "github",