feat(ACI): Make ProjectRuleDetailsEndpoint PUT method backwards compatible#110381
feat(ACI): Make ProjectRuleDetailsEndpoint PUT method backwards compatible#110381
Conversation
github-actions
bot
added
the
Scope: Backend
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
Backend Test FailuresFailures on
|
| "key": "foo", | ||
| "match": "eq", | ||
| "value": "bar", | ||
| } |
There was a problem hiding this comment.
these are invalid fields for a first seen event condition, idk why this test was using them so I just changed it to a different condition
| except KeyError: | ||
| raise ValidationError("Ensure all required fields are filled in.") | ||
| except ValueError as e: | ||
| raise ValidationError(str(e)) |
There was a problem hiding this comment.
This is mainly unsupported conditions
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # IssueCategoryFilter stores numeric string choices and must stay as str | ||
| if ( | ||
| f.get("value") | ||
| and f.get("id") != "sentry.rules.filters.issue_category.IssueCategoryFilter" |
There was a problem hiding this comment.
This is sort of hacky but outside of changing the value type of one of these it's the only way for the tests to pass and the serializers to be in sync.
| self.action_group, self.action = self.create_workflow_action(self.workflow) | ||
| self.fake_workflow_id = get_fake_id_from_object_id(self.workflow.id) | ||
|
|
||
| # fetch dual written workflow |
There was a problem hiding this comment.
I use the dual written workflow rather than the single written workflow so that it's set up exactly the same as the rule for comparison purposes when editing. I could have created the single written workflow to be the same but this was already here so it was slightly easier to use.
Backend Test FailuresFailures on
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
kcons
left a comment
kcons
left a comment
There was a problem hiding this comment.
Mostly looks good to me. I like how clean the tests look.
|
|
||
| def format_request_data( | ||
| data: ProjectRulePostData, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
It's a little bit sad how there is a type for the workflow validation input encoded in our code, but it's not really exposed to mypy unless we manually maintain a typeddict, so we don't get as much static validation here as we could.
... actually, I have a pr brewing, we'll see if it is worth it.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Owner field not cleared when switching owner types
- Updated workflow request formatting to always set the selected owner field and explicitly clear the opposite owner field when an owner is provided.
- ✅ Fixed: Tests compare wrong workflow missing coverage for specific rules
- Both tests now look up the dual-written workflow ID for the specific rule created in each test instead of reusing the fixture workflow ID.
Or push these changes by commenting:
@cursor push 7bbf3e4346
Preview (7bbf3e4346)
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
@@ -753,8 +753,10 @@
if actor and actor.is_team:
workflow_payload["owner_team_id"] = actor.id
+ workflow_payload["owner_user_id"] = None
elif actor:
workflow_payload["owner_user_id"] = actor.id
+ workflow_payload["owner_team_id"] = None
triggers: dict[str, Any] = {"logicType": "any-short", "conditions": []}
translated_filter_list = []
diff --git a/tests/sentry/api/endpoints/test_project_rule_details.py b/tests/sentry/api/endpoints/test_project_rule_details.py
--- a/tests/sentry/api/endpoints/test_project_rule_details.py
+++ b/tests/sentry/api/endpoints/test_project_rule_details.py
@@ -1002,12 +1002,15 @@
self.organization.slug, self.project.slug, rule.id, status_code=200, **payload
)
assert_rule_from_payload(rule, payload)
+ fake_dual_written_workflow_id = get_fake_id_from_object_id(
+ AlertRuleWorkflow.objects.get(rule_id=rule.id).workflow.id
+ )
with self.feature("organizations:workflow-engine-rule-serializers"):
workflow_response = self.get_success_response(
self.organization.slug,
self.project.slug,
- self.fake_dual_written_workflow_id,
+ fake_dual_written_workflow_id,
status_code=status.HTTP_200_OK,
**payload,
)
@@ -1253,11 +1256,14 @@
organization_id=self.organization.id,
),
)
+ fake_dual_written_workflow_id = get_fake_id_from_object_id(
+ AlertRuleWorkflow.objects.get(rule_id=rule.id).workflow.id
+ )
with self.feature("organizations:workflow-engine-rule-serializers"):
workflow_response = self.get_success_response(
self.organization.slug,
self.project.slug,
- self.fake_dual_written_workflow_id,
+ fake_dual_written_workflow_id,
status_code=status.HTTP_200_OK,
**payload,
)This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| status_code=status.HTTP_200_OK, | ||
| **payload, | ||
| ) | ||
| assert_serializer_results_match(response.data, workflow_response.data) |
There was a problem hiding this comment.
Tests compare wrong workflow missing coverage for specific rules
Medium Severity
In test_remove_conditions and test_reenable_disabled_rule, new rules are created and modified, but the workflow comparison uses self.fake_dual_written_workflow_id, which is tied to the original self.rule, not the newly created rules. This means test_remove_conditions doesn't actually test removing conditions from the correct workflow, and test_reenable_disabled_rule doesn't test re-enabling a disabled workflow (since the dual-written workflow was never disabled). Both tests pass coincidentally because the same payload is applied to both objects, hiding potential real bugs.
Additional Locations (1)
|
|
||
| workflow.refresh_from_db() | ||
| return Response( | ||
| serialize(workflow, request.user, WorkflowEngineRuleSerializer()), |
There was a problem hiding this comment.
PUT response serializer missing project_slug parameter
Medium Severity
The WorkflowEngineRuleSerializer() in the PUT response is instantiated without project_slug or prepare_component_fields, unlike the GET handler which passes project_slug=project.slug and prepare_component_fields=True. This means the PUT response may serialize the workflow with missing or incorrect project information and unprepared SentryApp component fields, producing an inconsistent response compared to what GET returns for the same workflow.
Additional Locations (1)
There was a problem hiding this comment.
That's consistence with the RuleSerializer usage
|
|
||
| def format_request_data( | ||
| data: ProjectRulePostData, | ||
| project: Project, |
There was a problem hiding this comment.
Unused project parameter in format_request_data
Low Severity
The project parameter was added to format_request_data but is never referenced anywhere in the function body (lines 742–802). Both call sites (project_rules.py POST and project_rule_details.py PUT) pass project, but the function only uses data. This is dead code that may signal a missing feature — the PR discussion mentions owner/actor validation that would likely need project context — or simply an oversight.
|
|
||
| request_data = format_request_data(cast(ProjectRulePostData, data), project) | ||
| if not request_data.get("config", {}).get("frequency"): | ||
| request_data["config"] = workflow.config |
There was a problem hiding this comment.
Falsy frequency check silently ignores invalid values
Low Severity
The check if not request_data.get("config", {}).get("frequency") uses a falsy test, so a frequency of 0 (or any other falsy value) silently falls back to workflow.config instead of being passed to the WorkflowValidator for proper rejection. This means an explicitly invalid frequency input gets silently replaced with the saved value rather than producing a validation error.
| return Response( | ||
| serialize(workflow, request.user, WorkflowEngineRuleSerializer()), | ||
| status=200, | ||
| ) |
There was a problem hiding this comment.
Bug: Updating a workflow rule with an empty action_filters list deletes all filters, causing an IndexError in the serializer which assumes at least one filter exists.
Severity: HIGH
Suggested Fix
Add a check in the WorkflowEngineRuleSerializer's get_attrs method to ensure workflow.prefetched_wdcgs is not empty before attempting to access its first element. If it is empty, handle it gracefully, for example by returning a default value or an empty structure for the related attributes.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/sentry/api/endpoints/project_rule_details.py#L241-L244
Potential issue: When a workflow rule is updated via a PUT request and the
`action_filters` payload is empty, the `remove_items_by_api_input` function deletes all
associated `WorkflowDataConditionGroup` entries. Subsequently, when the updated rule is
serialized, the `WorkflowEngineRuleSerializer` attempts to access
`workflow.prefetched_wdcgs[0]`. Because all filters were removed, `prefetched_wdcgs` is
an empty list, leading to an `IndexError: list index out of range` and a 500 Internal
Server Error response to the user. The serializer incorrectly assumes at least one
action filter will always be present.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| workflow.refresh_from_db() | ||
| return Response( | ||
| serialize(workflow, request.user, WorkflowEngineRuleSerializer()), |
There was a problem hiding this comment.
PUT response serializer missing project_slug parameter
Medium Severity
The PUT handler's response creates WorkflowEngineRuleSerializer() with no arguments, while the GET handler creates it with expand, prepare_component_fields=True, and project_slug=project.slug. This means the PUT response may serialize differently than the GET response for the same workflow — notably missing sentry app component field preparation and the project_slug context.
Additional Locations (1)
2 participants
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_teamtest_team_owner_not_membertest_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 successfullytest_rule_form_missing_actionsee above, condition is malformed but when fixed we return successfully. maybe it's ok for workflow engine if there is no actiontest_edit_non_condition_metric- skipped because it uses the every event condition which is not valid in workflow engine