Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 5 additions & 1 deletion src/sentry/api/endpoints/project_rule_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -137,8 +137,12 @@ def execute_future_on_test_event_workflow_engine(

try:
notification_actions_data = translate_rule_data_actions_to_notification_actions(
[action_blob], skip_failures=False
[action_blob], skip_failures=False, project=rule.project
)
# An action can translate to nothing (e.g. a NotifyEventAction on a project without
# legacy webhooks enabled), leaving no action to test-fire, so skip it.
if not notification_actions_data:
continue
actions = [Action(**action_data) for action_data in notification_actions_data]
action = actions[0]
action.id = TEST_NOTIFICATION_ID
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/api/endpoints/project_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ def format_request_data(

translated_actions: list[ActionInput] = []
for action_data in translate_rule_data_actions_to_notification_actions(
data.get("actions", []), False
data.get("actions", []), False, project=project
):
action: ActionInput = {
"type": action_data["type"],
Expand Down
8 changes: 8 additions & 0 deletions src/sentry/sentry_apps/services/legacy_webhook/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from sentry.eventstore.models import GroupEvent
from sentry.models.options.project_option import ProjectOption
from sentry.models.organization import Organization
from sentry.models.project import Project
from sentry.models.rule import Rule
from sentry.sentry_apps.services.app import app_service
from sentry.sentry_apps.tasks.sentry_apps import send_alert_webhook_v2
Expand Down Expand Up @@ -35,6 +36,13 @@ def split_urls(value: str) -> list[str]:
return list(filter(bool, (url.strip() for url in value.splitlines())))


def is_legacy_webhook_enabled(project: Project) -> bool:
"""Return whether the project has the legacy webhook enabled."""
# TODO: After the script migrates/removes all plugin actions, delete this helper and the
# `project` threading through the dual-write path.
return bool(ProjectOption.objects.get_value(project, "webhooks:enabled", default=False))
Comment on lines +41 to +43

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the helper/crux for passing down the project throughout all the dual write layers, since we only want to write a webhook action if we know it'll work/the person has legacy webhooks enabled. Otherwise the state makes no sense. Once the migration is done (i.e the plugin action rows are either deleted or migrated to webhook ones) we can delete this helper and the project passing



def get_triggering_rule_name(invocation: ActionInvocation) -> str:
try:
workflow = Workflow.objects.get(id=invocation.workflow_id)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import logging
from typing import Any

from sentry.models.project import Project
from sentry.models.rule import Rule
from sentry.rules.conditions.event_frequency import EventUniqueUserFrequencyConditionWithConditions
from sentry.rules.processing.processor import split_conditions_and_filters
Expand Down Expand Up @@ -76,8 +77,12 @@ def create_if_dcg(
return if_dcg


def create_workflow_actions(if_dcg: DataConditionGroup, actions: list[dict[str, Any]]) -> None:
notification_actions = build_notification_actions_from_rule_data_actions(actions)
def create_workflow_actions(
if_dcg: DataConditionGroup, actions: list[dict[str, Any]], project: Project
) -> None:
notification_actions = build_notification_actions_from_rule_data_actions(
actions, project=project
)
dcg_actions = [
DataConditionGroupAction(action=action, condition_group=if_dcg)
for action in notification_actions
Expand Down Expand Up @@ -143,7 +148,9 @@ def update_migrated_issue_alert(rule: Rule) -> Workflow | None:
)

delete_workflow_actions(if_dcg=if_dcg)
create_workflow_actions(if_dcg=if_dcg, actions=data["actions"]) # action(s) must exist
create_workflow_actions(
if_dcg=if_dcg, actions=data["actions"], project=rule.project
) # action(s) must exist

workflow.environment_id = rule.environment_id
if frequency := data["frequency"]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ def _create_workflow_actions(
self, if_dcg: DataConditionGroup, actions: list[dict[str, Any]]
) -> None:
notification_actions = build_notification_actions_from_rule_data_actions(
actions, is_dry_run=self.is_dry_run or False
actions, is_dry_run=self.is_dry_run or False, project=self.project
)
dcg_actions = [
DataConditionGroupAction(action=action, condition_group=if_dcg)
Expand Down
24 changes: 20 additions & 4 deletions src/sentry/workflow_engine/migration_helpers/rule_action.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import logging
from typing import Any, TypedDict

from sentry.models.project import Project
from sentry.sentry_apps.services.legacy_webhook.service import is_legacy_webhook_enabled
from sentry.workflow_engine.models.action import Action
from sentry.workflow_engine.typings.notification_action import issue_alert_action_translator_mapping
from sentry.workflow_engine.typings.notification_action import (
NotifyEventActionTranslator,
issue_alert_action_translator_mapping,
)

logger = logging.getLogger(__name__)

Expand All @@ -15,14 +20,16 @@ class NotificationActionData(TypedDict):


def translate_rule_data_actions_to_notification_actions(
actions: list[dict[str, Any]], skip_failures: bool
actions: list[dict[str, Any]], skip_failures: bool, project: Project | None = None
) -> list[NotificationActionData]:
"""
Builds notification actions from action field in Rule's data blob.
Will only create actions that are valid, and log any errors.

:param actions: list of action data (Rule.data.actions)
:param skip_failures: if True, invalid actions will be skipped instead of raising exceptions
:param project: the project the rule belongs to; used to gate the legacy NotifyEventAction
dual-write on whether the project has the legacy webhook enabled
:return: list of notification actions (Action)
"""

Expand Down Expand Up @@ -58,6 +65,13 @@ def translate_rule_data_actions_to_notification_actions(
f"Action translator not found for action with registry ID: {registry_id}, uuid: {action.get('uuid')}"
) from e

# NotifyEventAction delivers legacy webhooks; skip it unless the project has them enabled so
# we don't persist a dead action.
if isinstance(translator, NotifyEventActionTranslator) and not (
Comment thread
sentry-warden[bot] marked this conversation as resolved.
project is not None and is_legacy_webhook_enabled(project)
):
continue

# Check if the action is well-formed
if not translator.is_valid():
logger.error(
Expand Down Expand Up @@ -95,7 +109,7 @@ def translate_rule_data_actions_to_notification_actions(


def build_notification_actions_from_rule_data_actions(
actions: list[dict[str, Any]], is_dry_run: bool = False
actions: list[dict[str, Any]], is_dry_run: bool = False, project: Project | None = None
) -> list[Action]:
"""
Builds notification actions from action field in Rule's data blob.
Expand All @@ -107,11 +121,13 @@ def build_notification_actions_from_rule_data_actions(

:param actions: list of action data (Rule.data.actions)
:param is_dry_run: run in dry-run mode
:param project: the project the rule belongs to; used to gate the legacy NotifyEventAction
dual-write on whether the project has the legacy webhook enabled
:return: list of notification actions (Action)
"""

notification_actions_data = translate_rule_data_actions_to_notification_actions(
actions, skip_failures=not is_dry_run
actions, skip_failures=not is_dry_run, project=project
)
notification_actions = [Action(**action_data) for action_data in notification_actions_data]

Expand Down
40 changes: 21 additions & 19 deletions src/sentry/workflow_engine/typings/notification_action.py
Original file line number Diff line number Diff line change
Expand Up @@ -617,24 +617,6 @@ def get_sanitized_data(self) -> dict[str, Any]:
return {}


class PluginActionTranslator(BaseActionTranslator):
@property
def action_type(self) -> ActionType:
return ActionType.PLUGIN

@property
def required_fields(self) -> list[str]:
# NotifyEventAction doesn't appear to have any required fields
# beyond the standard id and uuid
return []

@property
def target_type(self) -> None:
# This appears to be a generic plugin notification
# so we'll use SPECIFIC as the target type
return None


class WebhookActionTranslator(BaseActionTranslator):
@property
def action_type(self) -> ActionType:
Expand All @@ -653,6 +635,26 @@ def required_fields(self) -> list[str]:
]


class NotifyEventActionTranslator(WebhookActionTranslator):
"""
Translates the legacy ``NotifyEventAction`` ("Send a notification for all legacy integrations")
into a ``WEBHOOK`` action targeting the legacy webhooks service.
"""

@property
def required_fields(self) -> list[str]:
# Unlike NotifyEventServiceAction, a NotifyEventAction blob has no `service` field, so there
# are no required fields beyond the standard id/uuid.
return []

@property
def target_identifier(self) -> str:
# At delivery time, WebhookActionHandler.execute routes the action to the legacy webhook
# path (_handle_legacy_webhooks) when target_identifier == "webhooks". The blob has no
# target field, so return the literal to opt into that path.
return "webhooks"


class SentryAppActionTranslator(BaseActionTranslator):
@property
def action_type(self) -> ActionType:
Expand Down Expand Up @@ -790,7 +792,7 @@ class EmailDataBlob(DataBlob):
ACTION_FIELD_MAPPINGS[ActionType.JIRA]["id"]: JiraActionTranslatorBase,
ACTION_FIELD_MAPPINGS[ActionType.JIRA_SERVER]["id"]: JiraServerActionTranslatorBase,
ACTION_FIELD_MAPPINGS[ActionType.EMAIL]["id"]: EmailActionTranslator,
ACTION_FIELD_MAPPINGS[ActionType.PLUGIN]["id"]: PluginActionTranslator,
ACTION_FIELD_MAPPINGS[ActionType.PLUGIN]["id"]: NotifyEventActionTranslator,
ACTION_FIELD_MAPPINGS[ActionType.WEBHOOK]["id"]: WebhookActionTranslator,
ACTION_FIELD_MAPPINGS[ActionType.SENTRY_APP]["id"]: SentryAppActionTranslator,
}
19 changes: 18 additions & 1 deletion tests/sentry/api/endpoints/test_project_rule_actions.py
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,11 @@ def setup_pd_service(self) -> PagerDutyServiceDict:
)

@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.plugin_handler.send_legacy_webhooks_for_invocation"
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.send_legacy_webhooks_for_invocation"
)
def test_actions(self, mock_send) -> None:
# NotifyEventAction only dual-writes a WEBHOOK action when webhooks are enabled.
self.project.update_option("webhooks:enabled", True)
action_data = [
{
"id": "sentry.rules.actions.notify_event.NotifyEventAction",
Expand All @@ -70,6 +72,21 @@ def test_actions(self, mock_send) -> None:
self.get_success_response(self.organization.slug, self.project.slug, actions=action_data)
assert mock_send.called

@mock.patch(
"sentry.notifications.notification_action.action_handler_registry.webhook_handler.send_legacy_webhooks_for_invocation"
)
def test_actions_notify_event_webhooks_disabled(self, mock_send) -> None:
# With webhooks disabled, NotifyEventAction translates to no action, so the test-fire should
# skip it gracefully (success, nothing fired) rather than raising an IndexError -> 400.
action_data = [
{
"id": "sentry.rules.actions.notify_event.NotifyEventAction",
}
]

self.get_success_response(self.organization.slug, self.project.slug, actions=action_data)
assert not mock_send.called

def test_unknown_action_returns_400(self) -> None:
action_data = [
{
Expand Down
17 changes: 17 additions & 0 deletions tests/sentry/api/endpoints/test_project_rule_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from sentry.incidents.endpoints.serializers.utils import get_fake_id_from_object_id
from sentry.integrations.slack.utils.channel import strip_channel_name
from sentry.models.environment import Environment
from sentry.models.options.project_option import ProjectOption
from sentry.models.rule import Rule, RuleActivity, RuleActivityType
from sentry.sentry_apps.services.app.model import RpcAlertRuleActionResult
from sentry.sentry_apps.utils.errors import SentryAppErrorType
Expand Down Expand Up @@ -124,6 +125,16 @@ def assert_serializer_results_match(
del rule_action_data["legacy_rule_id"]
if rule_action_data.get("workflow_id"):
del rule_action_data["workflow_id"]
# NotifyEventAction dual-writes to WEBHOOK("webhooks"), which serializes back as
# NotifyEventServiceAction(service="webhooks"). This divergence is intentional, so treat the
# two forms as equal.
if (
rule_action_data.get("id") == "sentry.rules.actions.notify_event.NotifyEventAction"
and workflow_action_data.get("id")
== "sentry.rules.actions.notify_event_service.NotifyEventServiceAction"
and workflow_action_data.get("service") == "webhooks"
):
continue
assert rule_action_data == workflow_action_data

# XXX: actionMatch is always coerced to 'any-short' for a Workflow as it is the only acceptable value
Expand Down Expand Up @@ -299,6 +310,12 @@ def test_issue_owners_action_returns_fallthrough_type(self) -> None:
class UpdateProjectRuleTest(ProjectRuleDetailsBaseTestCase):
method = "PUT"

def setUp(self) -> None:
super().setUp()
# NotifyEventAction (used by several tests) only dual-writes a WEBHOOK action when webhooks
# are enabled; enable it so the parity checks have a comparable action to compare.
ProjectOption.objects.set_value(self.project, "webhooks:enabled", True)

def mock_conversations_list(self, channels):
return patch(
"slack_sdk.web.client.WebClient.conversations_list",
Expand Down
7 changes: 7 additions & 0 deletions tests/sentry/api/endpoints/test_project_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
from sentry.integrations.slack.tasks.find_channel_id_for_rule import find_channel_id_for_rule
from sentry.integrations.slack.utils.channel import SlackChannelIdData
from sentry.models.environment import Environment
from sentry.models.options.project_option import ProjectOption
from sentry.models.rule import Rule, RuleActivity, RuleActivityType
from sentry.rules.conditions.existing_high_priority_issue import ExistingHighPriorityIssueCondition
from sentry.silo.base import SiloMode
Expand Down Expand Up @@ -339,6 +340,12 @@ def test_simple(self) -> None:
class CreateProjectRuleTest(ProjectRuleBaseTestCase):
method = "post"

def setUp(self) -> None:
super().setUp()
# NotifyEventAction (used by several tests) only dual-writes a WEBHOOK action when webhooks
# are enabled; enable it so the parity checks have a comparable action to compare.
ProjectOption.objects.set_value(self.project, "webhooks:enabled", True)

def mock_conversations_info(self, channel):
return patch(
"slack_sdk.web.client.WebClient.conversations_info",
Expand Down
15 changes: 14 additions & 1 deletion tests/sentry/projects/project_rules/test_creator.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from sentry.grouping.grouptype import ErrorGroupType
from sentry.models.options.project_option import ProjectOption
from sentry.models.rule import RuleSource
from sentry.projects.project_rules.creator import ProjectRuleCreator
from sentry.testutils.cases import TestCase
Expand Down Expand Up @@ -54,6 +55,9 @@ def setUp(self) -> None:
)

def test_create_rule_and_workflow(self) -> None:
# NotifyEventAction dual-writes a WEBHOOK action only when webhooks are enabled.
ProjectOption.objects.set_value(self.project, "webhooks:enabled", True)

rule = self.creator.run()

rule_id = rule.id
Expand Down Expand Up @@ -89,4 +93,13 @@ def test_create_rule_and_workflow(self) -> None:
assert data_condition.comparison == {"key": "foo", "match": "is"}

action = DataConditionGroupAction.objects.get(condition_group=action_filter).action
assert action.type == Action.Type.PLUGIN
assert action.type == Action.Type.WEBHOOK
assert action.config.get("target_identifier") == "webhooks"

def test_create_rule_and_workflow__notify_event_webhooks_disabled(self) -> None:
# Webhooks disabled: no action written; the workflow is a valid "No actions" automation.
rule = self.creator.run()

workflow = AlertRuleWorkflow.objects.get(rule_id=rule.id).workflow
action_filter = WorkflowDataConditionGroup.objects.get(workflow=workflow).condition_group
assert not DataConditionGroupAction.objects.filter(condition_group=action_filter).exists()
Loading
Loading