Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(hc): Silo fixes for OrganizationAlertRuleAvailableActionIndexEndpoint #57949

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
from __future__ import annotations

from collections import defaultdict
from typing import Any, DefaultDict, List, Mapping

from rest_framework import status
from rest_framework.request import Request
Expand All @@ -8,7 +11,6 @@
from sentry.api.api_publish_status import ApiPublishStatus
from sentry.api.base import region_silo_endpoint
from sentry.api.exceptions import ResourceDoesNotExist
from sentry.constants import SentryAppStatus
from sentry.incidents.endpoints.bases import OrganizationEndpoint
from sentry.incidents.logic import (
get_available_action_integrations_for_org,
Expand All @@ -17,12 +19,17 @@
)
from sentry.incidents.models import AlertRuleTriggerAction
from sentry.incidents.serializers import ACTION_TARGET_TYPE_TO_STRING
from sentry.models.integrations.sentry_app_installation import SentryAppInstallation
from sentry.models.organization import Organization
from sentry.services.hybrid_cloud.app import RpcSentryAppInstallation, app_service
from sentry.services.hybrid_cloud.integration import RpcIntegration


def build_action_response(
registered_type, integration=None, organization=None, sentry_app_installation=None
):
registered_type,
integration: RpcIntegration | None = None,
organization: Organization | None = None,
sentry_app_installation: RpcSentryAppInstallation | None = None,
) -> Mapping[str, Any]:
"""
Build the "available action" objects for the API. Each one can have different fields.

Expand Down Expand Up @@ -57,14 +64,14 @@ def build_action_response(

elif sentry_app_installation:
action_response["sentryAppName"] = sentry_app_installation.sentry_app.name
action_response["sentryAppId"] = sentry_app_installation.sentry_app_id
action_response["sentryAppId"] = sentry_app_installation.sentry_app.id
action_response["sentryAppInstallationUuid"] = sentry_app_installation.uuid
action_response["status"] = SentryAppStatus.as_str(
sentry_app_installation.sentry_app.status
)
action_response["status"] = sentry_app_installation.sentry_app.status

# Sentry Apps can be alertable but not have an Alert Rule UI Component
component = sentry_app_installation.prepare_sentry_app_components("alert-rule-action")
component = app_service.prepare_sentry_app_components(
installation_id=sentry_app_installation.id, component_type="alert-rule-action"
)
if component:
action_response["settings"] = component.schema.get("settings", {})

Expand All @@ -87,7 +94,7 @@ def get(self, request: Request, organization) -> Response:
actions = []

# Cache Integration objects in this data structure to save DB calls.
provider_integrations = defaultdict(list)
provider_integrations: DefaultDict[str, List[RpcIntegration]] = defaultdict(list)
for integration in get_available_action_integrations_for_org(organization):
provider_integrations[integration.provider].append(integration)

Expand All @@ -103,13 +110,13 @@ def get(self, request: Request, organization) -> Response:

# Add all alertable SentryApps to the list.
elif registered_type.type == AlertRuleTriggerAction.Type.SENTRY_APP:
installs = app_service.get_installed_for_organization(
organization_id=organization.id
)
actions += [
build_action_response(registered_type, sentry_app_installation=install)
for install in SentryAppInstallation.objects.get_installed_for_organization(
organization.id
).filter(
sentry_app__is_alertable=True,
)
for install in installs
if install.sentry_app.is_alertable
]

else:
Expand Down
12 changes: 7 additions & 5 deletions src/sentry/incidents/logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

from sentry import analytics, audit_log, features, quotas
from sentry.auth.access import SystemAccess
from sentry.constants import CRASH_RATE_ALERT_AGGREGATE_ALIAS
from sentry.constants import CRASH_RATE_ALERT_AGGREGATE_ALIAS, ObjectStatus
from sentry.incidents import tasks
from sentry.incidents.models import (
AlertRule,
Expand All @@ -38,7 +38,6 @@
TriggerStatus,
)
from sentry.models.actor import Actor
from sentry.models.integrations.integration import Integration
from sentry.models.integrations.organization_integration import OrganizationIntegration
from sentry.models.notificationaction import ActionService, ActionTarget
from sentry.models.project import Project
Expand Down Expand Up @@ -1458,7 +1457,7 @@ def get_actions_for_trigger(trigger):
return AlertRuleTriggerAction.objects.filter(alert_rule_trigger=trigger)


def get_available_action_integrations_for_org(organization):
def get_available_action_integrations_for_org(organization) -> List[RpcIntegration]:
"""
Returns a list of integrations that the organization has installed. Integrations are
filtered by the list of registered providers.
Expand All @@ -1472,8 +1471,11 @@ def get_available_action_integrations_for_org(organization):
if registration.type != AlertRuleTriggerAction.Type.DISCORD
or features.has("organizations:integrations-discord-metric-alerts", organization)
]
return Integration.objects.get_active_integrations(organization.id).filter(
provider__in=providers
return integration_service.get_integrations(
status=ObjectStatus.ACTIVE,
org_integration_status=ObjectStatus.ACTIVE,
organization_id=organization.id,
providers=providers,
)


Expand Down
28 changes: 11 additions & 17 deletions src/sentry/models/integrations/sentry_app_installation.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import uuid
from itertools import chain
from typing import TYPE_CHECKING, Any, List
from typing import TYPE_CHECKING, Any, List, Mapping

from django.db import models, router, transaction
from django.db.models import OuterRef, QuerySet, Subquery
Expand All @@ -19,6 +19,7 @@
)
from sentry.db.models.fields.hybrid_cloud_foreign_key import HybridCloudForeignKey
from sentry.services.hybrid_cloud.auth import AuthenticatedToken
from sentry.services.hybrid_cloud.project import RpcProject
from sentry.types.region import find_regions_for_orgs

if TYPE_CHECKING:
Expand Down Expand Up @@ -195,19 +196,12 @@ def outboxes_for_update(self) -> List[ControlOutbox]:
for region_name in find_regions_for_orgs([self.organization_id])
]

def prepare_sentry_app_components(self, component_type, project=None, values=None):
from sentry.models.integrations.sentry_app_component import SentryAppComponent

try:
component = SentryAppComponent.objects.get(
sentry_app_id=self.sentry_app_id, type=component_type
)
except SentryAppComponent.DoesNotExist:
return None

return self.prepare_ui_component(component, project, values)

def prepare_ui_component(self, component, project=None, values=None):
def prepare_ui_component(
self,
component: SentryAppComponent,
project: Project | RpcProject | None = None,
values: Any = None,
) -> SentryAppComponent | None:
return prepare_ui_component(
self, component, project_slug=project.slug if project else None, values=values
)
Expand All @@ -217,8 +211,8 @@ def prepare_sentry_app_components(
installation: SentryAppInstallation,
component_type: str,
project_slug: str | None = None,
values: Any = None,
):
values: List[Mapping[str, Any]] | None = None,
) -> SentryAppComponent | None:
from sentry.models.integrations.sentry_app_component import SentryAppComponent

try:
Expand All @@ -235,7 +229,7 @@ def prepare_ui_component(
installation: SentryAppInstallation,
component: SentryAppComponent,
project_slug: str | None = None,
values: Any = None,
values: List[Mapping[str, Any]] | None = None,
) -> SentryAppComponent | None:
from sentry.coreapi import APIError
from sentry.sentry_apps.components import SentryAppComponentPreparer
Expand Down
26 changes: 20 additions & 6 deletions src/sentry/services/hybrid_cloud/app/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
)
from sentry.services.hybrid_cloud.app.serial import (
serialize_sentry_app,
serialize_sentry_app_component,
serialize_sentry_app_installation,
)
from sentry.services.hybrid_cloud.auth import AuthenticationContext
Expand All @@ -36,6 +37,7 @@
OpaqueSerializedResponse,
)
from sentry.services.hybrid_cloud.user import RpcUser
from sentry.utils import json


class DatabaseBackedAppService(AppService):
Expand All @@ -55,12 +57,7 @@ def get_many(

def find_app_components(self, *, app_id: int) -> List[RpcSentryAppComponent]:
return [
RpcSentryAppComponent(
uuid=str(c.uuid),
sentry_app_id=c.sentry_app_id,
type=c.type,
app_schema=c.schema,
)
serialize_sentry_app_component(c)
for c in SentryAppComponent.objects.filter(sentry_app_id=app_id)
]

Expand Down Expand Up @@ -253,3 +250,20 @@ def create_internal_integration_for_channel_request(
installation = SentryAppInstallation.objects.get(sentry_app=sentry_app)

return serialize_sentry_app_installation(installation=installation, app=sentry_app)

def prepare_sentry_app_components(
self,
*,
installation_id: int,
component_type: str,
project_slug: Optional[str] = None,
values_json: Optional[str] = None,
) -> Optional[RpcSentryAppComponent]:
from sentry.models.integrations.sentry_app_installation import prepare_sentry_app_components

installation = SentryAppInstallation.objects.get(id=installation_id)
values = json.loads(values_json) if values_json else None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we handle json parsing failures here? We are getting a JSON blob off the wire.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, and one that we should probably generalize to other RPC methods. (Grep for data_json to see those cases.)

However, considering that it's not used by the one call site and may remain unused indefinitely, I'll just remove it. We can always add it later (with good validation) when we need to.

component = prepare_sentry_app_components(
installation, component_type, project_slug, values
)
return serialize_sentry_app_component(component) if component else None
1 change: 1 addition & 0 deletions src/sentry/services/hybrid_cloud/app/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ class RpcSentryApp(RpcModel):
uuid: str = ""
events: List[str] = Field(default_factory=list)
webhook_url: Optional[str] = None
is_alertable: bool = False
is_published: bool = False
is_unpublished: bool = False
is_internal: bool = True
Expand Down
14 changes: 13 additions & 1 deletion src/sentry/services/hybrid_cloud/app/serial.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,13 @@

from sentry.constants import SentryAppStatus
from sentry.models.apiapplication import ApiApplication
from sentry.models.integrations import SentryAppComponent
from sentry.models.integrations.sentry_app import SentryApp
from sentry.models.integrations.sentry_app_installation import SentryAppInstallation
from sentry.services.hybrid_cloud.app import (
RpcApiApplication,
RpcSentryApp,
RpcSentryAppComponent,
RpcSentryAppInstallation,
)

Expand Down Expand Up @@ -34,11 +36,12 @@ def serialize_sentry_app(app: SentryApp) -> RpcSentryApp:
uuid=app.uuid,
events=app.events,
webhook_url=app.webhook_url,
is_alertable=app.is_alertable,
is_published=app.status == SentryAppStatus.PUBLISHED,
is_unpublished=app.status == SentryAppStatus.UNPUBLISHED,
is_internal=app.status == SentryAppStatus.INTERNAL,
is_publish_request_inprogress=app.status == SentryAppStatus.PUBLISH_REQUEST_INPROGRESS,
status=app.status,
status=SentryAppStatus.as_str(app.status),
)


Expand All @@ -58,3 +61,12 @@ def serialize_sentry_app_installation(
uuid=installation.uuid,
api_token=installation.api_token.token if installation.api_token else None,
)


def serialize_sentry_app_component(component: SentryAppComponent) -> RpcSentryAppComponent:
return RpcSentryAppComponent(
uuid=str(component.uuid),
sentry_app_id=component.sentry_app_id,
type=component.type,
app_schema=component.schema,
)
10 changes: 10 additions & 0 deletions src/sentry/services/hybrid_cloud/app/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -140,5 +140,15 @@ def create_internal_integration_for_channel_request(
) -> RpcSentryAppInstallation:
pass

def prepare_sentry_app_components(
self,
*,
installation_id: int,
component_type: str,
project_slug: Optional[str] = None,
values_json: Optional[str] = None,
) -> Optional[RpcSentryAppComponent]:
pass


app_service = cast(AppService, AppService.create_delegation())
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from sentry.incidents.models import AlertRuleTriggerAction
from sentry.models.integrations.integration import Integration
from sentry.models.integrations.organization_integration import OrganizationIntegration
from sentry.services.hybrid_cloud.app.serial import serialize_sentry_app_installation
from sentry.silo import SiloMode
from sentry.testutils.cases import APITestCase
from sentry.testutils.silo import assume_test_silo_mode, region_silo_test
Expand All @@ -25,7 +26,7 @@
}


@region_silo_test
@region_silo_test(stable=True)
class OrganizationAlertRuleAvailableActionIndexEndpointTest(APITestCase):
endpoint = "sentry-api-0-organization-alert-rule-available-actions"
email = AlertRuleTriggerAction.get_registered_type(AlertRuleTriggerAction.Type.EMAIL)
Expand Down Expand Up @@ -109,7 +110,9 @@ def test_build_action_response_pagerduty(self):
def test_build_action_response_sentry_app(self):
installation = self.install_new_sentry_app("foo")

data = build_action_response(self.sentry_app, sentry_app_installation=installation)
data = build_action_response(
self.sentry_app, sentry_app_installation=serialize_sentry_app_installation(installation)
)

assert data["type"] == "sentry_app"
assert data["allowedTargetTypes"] == ["sentry_app"]
Expand Down Expand Up @@ -180,7 +183,10 @@ def test_sentry_apps(self):
assert len(response.data) == 2
assert build_action_response(self.email) in response.data
assert (
build_action_response(self.sentry_app, sentry_app_installation=installation)
build_action_response(
self.sentry_app,
sentry_app_installation=serialize_sentry_app_installation(installation),
)
in response.data
)

Expand All @@ -193,7 +199,10 @@ def test_published_sentry_apps(self):

assert len(response.data) == 2
assert (
build_action_response(self.sentry_app, sentry_app_installation=installation)
build_action_response(
self.sentry_app,
sentry_app_installation=serialize_sentry_app_installation(installation),
)
in response.data
)

Expand Down
9 changes: 7 additions & 2 deletions tests/sentry/incidents/test_logic.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
from sentry.models.actor import ActorTuple, get_actor_id_for_user
from sentry.models.integrations.integration import Integration
from sentry.models.integrations.organization_integration import OrganizationIntegration
from sentry.services.hybrid_cloud.integration.serial import serialize_integration
from sentry.shared_integrations.exceptions import ApiError, ApiRateLimitedError, ApiTimeoutError
from sentry.snuba.dataset import Dataset
from sentry.snuba.models import QuerySubscription, SnubaQuery, SnubaQueryEventType
Expand Down Expand Up @@ -1997,14 +1998,18 @@ def test_unregistered(self):
def test_registered(self):
integration = Integration.objects.create(external_id="1", provider="slack")
integration.add_organization(self.organization)
assert list(get_available_action_integrations_for_org(self.organization)) == [integration]
assert list(get_available_action_integrations_for_org(self.organization)) == [
serialize_integration(integration)
]

def test_mixed(self):
integration = Integration.objects.create(external_id="1", provider="slack")
integration.add_organization(self.organization)
other_integration = Integration.objects.create(external_id="12345", provider="random")
other_integration.add_organization(self.organization)
assert list(get_available_action_integrations_for_org(self.organization)) == [integration]
assert list(get_available_action_integrations_for_org(self.organization)) == [
serialize_integration(integration)
]

def test_disabled_integration(self):
integration = Integration.objects.create(
Expand Down
Loading