feat(ACI): Make ProjectRulesEndpoint POST method backwards compatible#109926
feat(ACI): Make ProjectRulesEndpoint POST method backwards compatible#109926
Conversation
github-actions
bot
added
the
Scope: Backend
|
|
||
| with transaction.atomic(router.db_for_write(Workflow)): | ||
| workflow = validator.create(validator.validated_data) | ||
| # XXX: fetch issue stream detector (or leave unconnected?) |
There was a problem hiding this comment.
I think it makes the most sense to connect this to the issue stream detector, but correct me if I'm wrong.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Legacy
filterMatch"any" not converted to "any-short"format_request_datanow maps legacyfilterMatch: "any"toDataConditionGroup.Type.ANY_SHORT_CIRCUIT.valuebefore settingactionFilters.logicTypeand includes a regression test.
- ✅ Fixed:
ActionDataTypedDict mismatches actual dict shape returnedtranslate_rule_data_actions_to_notification_actionsnow uses a dedicatedNotificationActionDataTypedDict that matches the returned action-constructor payload and removes the unsafe cast.
Or push these changes by commenting:
@cursor push 49f1e8f6bd
Preview (49f1e8f6bd)
diff --git a/src/sentry/api/endpoints/project_rules.py b/src/sentry/api/endpoints/project_rules.py
--- a/src/sentry/api/endpoints/project_rules.py
+++ b/src/sentry/api/endpoints/project_rules.py
@@ -775,8 +775,12 @@
if target_type is not None:
action["config"]["target_type"] = ActionTarget.get_name(target_type)
+ filter_match = data.get("filterMatch", "any-short")
+ if filter_match == "any":
+ filter_match = DataConditionGroup.Type.ANY_SHORT_CIRCUIT.value
+
action_filters = {
- "logicType": data.get("filterMatch", "any-short"),
+ "logicType": filter_match,
"conditions": translated_filter_list,
"actions": translated_actions,
}
diff --git a/src/sentry/workflow_engine/migration_helpers/rule_action.py b/src/sentry/workflow_engine/migration_helpers/rule_action.py
--- a/src/sentry/workflow_engine/migration_helpers/rule_action.py
+++ b/src/sentry/workflow_engine/migration_helpers/rule_action.py
@@ -1,16 +1,22 @@
import logging
-from typing import Any, cast
+from typing import Any, TypedDict
-from sentry.workflow_engine.endpoints.validators.base.action import ActionData
from sentry.workflow_engine.models.action import Action
from sentry.workflow_engine.typings.notification_action import issue_alert_action_translator_mapping
logger = logging.getLogger(__name__)
+class NotificationActionData(TypedDict):
+ type: str
+ data: dict[str, Any]
+ integration_id: int | None
+ config: dict[str, str | int | None]
+
+
def translate_rule_data_actions_to_notification_actions(
actions: list[dict[str, Any]], skip_failures: bool
-) -> list[ActionData]:
+) -> list[NotificationActionData]:
"""
Builds notification actions from action field in Rule's data blob.
Will only create actions that are valid, and log any errors.
@@ -20,7 +26,7 @@
:return: list of notification actions (Action)
"""
- notification_actions: list[ActionData] = []
+ notification_actions: list[NotificationActionData] = []
for action in actions:
# Fetch the registry ID
@@ -76,7 +82,7 @@
"config": translator.action_config,
}
- notification_actions.append(cast(ActionData, notification_action_data))
+ notification_actions.append(notification_action_data)
except Exception as e:
if not skip_failures:
raise
diff --git a/tests/sentry/api/endpoints/test_project_rules.py b/tests/sentry/api/endpoints/test_project_rules.py
--- a/tests/sentry/api/endpoints/test_project_rules.py
+++ b/tests/sentry/api/endpoints/test_project_rules.py
@@ -12,7 +12,7 @@
from rest_framework import status
from slack_sdk.web import SlackResponse
-from sentry.api.endpoints.project_rules import get_max_alerts
+from sentry.api.endpoints.project_rules import format_request_data, get_max_alerts
from sentry.constants import ObjectStatus
from sentry.incidents.endpoints.serializers.utils import (
get_fake_id_from_object_id,
@@ -395,6 +395,24 @@
assert RuleActivity.objects.filter(rule=rule, type=RuleActivityType.CREATED.value).exists()
return response
+ def test_format_request_data_maps_any_filter_match_to_any_short(self) -> None:
+ payload = {
+ "name": "my rule",
+ "status": "active",
+ "frequency": 30,
+ "conditions": [],
+ "filters": [],
+ "actions": [],
+ "filterMatch": "any",
+ }
+
+ workflow_payload = format_request_data(payload)
+
+ assert (
+ workflow_payload["actionFilters"][0]["logicType"]
+ == DataConditionGroup.Type.ANY_SHORT_CIRCUIT.value
+ )
+
def test_simple(self) -> None:
self.run_test(actions=self.notify_issue_owners_action, conditions=self.first_seen_condition)
| if action_config is not None: | ||
| target_type = action_config.get("target_type") | ||
| if target_type is not None: | ||
| assert isinstance(target_type, int) |
There was a problem hiding this comment.
I verified every Action.config.get("target_type") is an integer in the db
| fallthroughType: NotRequired[Literal["ActiveMembers", "AllMembers", "NoOne"]] | ||
| oldest_or_newest: NotRequired[str] |
There was a problem hiding this comment.
note the mix of camel and snake case - I double checked this is what we send from the front end 🤷♀️
| ) | ||
|
|
||
| try: | ||
| notification_action = Action( |
There was a problem hiding this comment.
This is all just so I can reuse the function
| dcgs = DataConditionGroup.objects.filter(id=wdcg.condition_group_id) | ||
| dc_filters = DataCondition.objects.filter(condition_group__in=dcgs) | ||
| assert len(dc_filters) == len(payload["filters"]) | ||
| # spot check |
There was a problem hiding this comment.
Calling out that this does only check one filter and one condition, but the PR would be massive if I checked everything. I mostly made every condition and every filter to make sure we didn't bump up against conversion issues - I can make a follow up that asserts everything in each filter and condition if we think that's valuable.
There was a problem hiding this comment.
a good call out, but I'd also note that our source of truth here is the legacy implementation, so we can get a lot of validation mileage by diffing our returned dict with what the legacy code would give. It's easier in GET cases, but should still be possible here.
I'm also tempted here to set up a fixtures to enable and disable our flag for all test methods, maybe just by setting self.legacy and using that to guide our features and our validation where needed. that'd allow us to reuse existing test cases and pretty trivially excise legacy testing once we aren't using legacy paths anymore.
There was a problem hiding this comment.
+1 to using artifacts from the legacy code as fixtures. I think that'll be a great way to ensure we have fewer / no regressions. I think we should probably split that out as a separate pr though (i think you've already done that?)
| translated_filter_list.append(translated_filters) | ||
|
|
||
| translated_actions = translate_rule_data_actions_to_notification_actions( | ||
| data.get("actions", []), False |
There was a problem hiding this comment.
Unhandled ValueError causes 500 on invalid input
Medium Severity
format_request_data() is called before any validation in the POST handler. Both translate_to_data_condition_data() and translate_rule_data_actions_to_notification_actions(..., False) raise ValueError for unsupported condition/action IDs. These exceptions are not caught, resulting in a 500 error instead of a proper 400 validation response. The legacy code path validates input through DrfRuleSerializer first and returns clean error messages, making this a regression in error handling for the public API.
Additional Locations (1)
There was a problem hiding this comment.
The purpose of format_request_data is to format it into the expected payload format before validating it. I'm not terribly concerned about being passed invalid invalid condition and action IDs.
| - Actions: specify what should happen when the trigger conditions are met and the filters match. | ||
| """ | ||
| if features.has("organizations:workflow-engine-rule-serializers", project.organization): | ||
| request_data = format_request_data(cast("ProjectRulePostData", request.data)) |
There was a problem hiding this comment.
do we need to quote the type name here?
| workflow = validator.create(validator.validated_data) | ||
| # Ensure default detectors exist for the project before linking | ||
| default_detectors = ensure_default_detectors(project) | ||
| issue_stream_detector = default_detectors.get(IssueStreamGroupType.slug) |
There was a problem hiding this comment.
ah, Detector.get_issue_stream_detector_for_project is probably the more conventional way to do this.
The 'ensure' shouldn't be needed.
| dcgs = DataConditionGroup.objects.filter(id=wdcg.condition_group_id) | ||
| dc_filters = DataCondition.objects.filter(condition_group__in=dcgs) | ||
| assert len(dc_filters) == len(payload["filters"]) | ||
| # spot check |
There was a problem hiding this comment.
a good call out, but I'd also note that our source of truth here is the legacy implementation, so we can get a lot of validation mileage by diffing our returned dict with what the legacy code would give. It's easier in GET cases, but should still be possible here.
I'm also tempted here to set up a fixtures to enable and disable our flag for all test methods, maybe just by setting self.legacy and using that to guide our features and our validation where needed. that'd allow us to reuse existing test cases and pretty trivially excise legacy testing once we aren't using legacy paths anymore.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| if action_config is not None: | ||
| target_type = action_config.get("target_type") | ||
| if target_type is not None: | ||
| assert isinstance(target_type, int) |
There was a problem hiding this comment.
Production assert used for input validation in request handler
Medium Severity
assert isinstance(target_type, int) is used for runtime validation in format_request_data, which is called during request handling. Python's -O flag strips assert statements entirely, so this check would be silently skipped in optimized mode. Even with assertions enabled, an AssertionError would surface as an unhandled 500 error rather than a meaningful validation response. A proper ValueError or ValidationError would be more appropriate here.
| - Actions: specify what should happen when the trigger conditions are met and the filters match. | ||
| """ | ||
| if features.has("organizations:workflow-engine-rule-serializers", project.organization): | ||
| request_data = format_request_data(cast(ProjectRulePostData, request.data)) |
There was a problem hiding this comment.
Unhandled exceptions in format_request_data cause 500 errors
Medium Severity
format_request_data calls translate_rule_data_actions_to_notification_actions with skip_failures=False and translate_to_data_condition_data, both of which raise ValueError on invalid input. The call at line 884 has no try/except, so malformed actions or unsupported conditions result in unhandled 500 errors instead of 400 validation responses. The legacy path validates input via DrfRuleSerializer before processing, which the new path skips entirely.
Additional Locations (1)
…tible (#110381) Follow up to #109926 to transform an issue alert PUT payload into a workflow PUT payload and serialize it as if it were an issue alert rule. I intentionally skipped a few tests as we do not appear to fail on the same things in workflow engine. We may want to go back and block these things from occurring but that is outside of the scope of this PR. Those tests are: * `test_cannot_reassign_owner_from_other_team` * `test_team_owner_not_member` * `test_rule_form_owner_perms` - note that for workflow engine, it first fails on the malformed condition earlier than it fails on the owner being in a different org. to verify I passed match, key, and value and it ran successfully * `test_rule_form_missing_action` see above, condition is malformed but when fixed we return successfully. maybe it's ok for workflow engine if there is no action * `test_edit_non_condition_metric` - skipped because it uses the every event condition which is not valid in workflow engine
3 participants
Transform an issue alert rule payload into a workflow payload to make the endpoint backwards compatible.