feat(ACI): Make ProjectRuleDetailsEndpoint GET method backwards compatible#109387
feat(ACI): Make ProjectRuleDetailsEndpoint GET method backwards compatible#109387
Conversation
github-actions
bot
added
the
Scope: Backend
| return args, kwargs | ||
|
|
||
|
|
||
| class WorkflowEngineRuleEndpoint(RuleEndpoint): |
There was a problem hiding this comment.
I had to make a new class here because RuleEndpoint encompasses ProjectRuleGroupHistoryIndexEndpoint and ProjectRuleStatsIndexEndpoint as well which we don't want to affect.
| except AlertRuleWorkflow.DoesNotExist: | ||
| # XXX: this means the workflow was single written and has no ARW or related Rule object | ||
| try: | ||
| workflow_id = get_object_id_from_fake_id(int(rule_id)) |
There was a problem hiding this comment.
We assume we are being passed a "fake id" here because for singly written rules we will not have a rule id, so the rule index endpoint will generate a fake id for a workflow so it can be looked up here.
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| except Workflow.DoesNotExist: | ||
| raise ResourceDoesNotExist | ||
|
|
||
| return args, kwargs |
There was a problem hiding this comment.
Legacy rules return 404 when feature flag enabled
High Severity
When organizations:workflow-engine-rule-serializers is enabled, WorkflowEngineRuleEndpoint.convert_args returns 404 for legacy rules. If AlertRuleWorkflow lookup fails, it only tries Workflow via get_object_id_from_fake_id. When that also fails (e.g. for a legacy Rule id), it raises ResourceDoesNotExist instead of falling back to the legacy Rule lookup, breaking backwards compatibility.
| prepare_component_fields=True, | ||
| project_slug=project.slug, | ||
| ) | ||
| serialized_rule = serialize(rule, request.user, rule_serializer) |
There was a problem hiding this comment.
GET uses wrong serializer when legacy Rule returned
High Severity
The GET handler selects the serializer using only the feature flag. If convert_args is fixed to fall back to legacy Rule when both AlertRuleWorkflow and Workflow lookups fail, the handler would pass a Rule to WorkflowEngineRuleSerializer, which expects Workflow objects. That would cause get_attrs and serialize to fail (e.g. KeyError on attrs["rule_id"]). Serializer selection should branch on isinstance(rule, Workflow) so legacy rules use RuleSerializer.
There was a problem hiding this comment.
If convert_args is fixed to fall back to legacy Rule when both AlertRuleWorkflow and Workflow lookups fail
idk why you'd think that would happen
![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.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| result[workflow]["rule_id"] = workflow_rule_ids[workflow.id] | ||
| result[workflow]["rule_id"] = workflow_rule_ids.get( | ||
| workflow.id, get_fake_id_from_object_id(workflow.id) | ||
| ) |
There was a problem hiding this comment.
IndexError when workflow has no WorkflowDataConditionGroup
Medium Severity
WorkflowEngineRuleSerializer.get_attrs assumes every workflow has at least one WorkflowDataConditionGroup and does workflow.prefetched_wdcgs[0] without checking. A workflow with no WorkflowDataConditionGroup raises IndexError during serialization.
There was a problem hiding this comment.
I think it'll always have one
There was a problem hiding this comment.
might be polite to type annotate the others too with an assertion of the isinstance calling out "this shouldn't be enabled; not fully supported" or whatever.
| ) | ||
|
|
||
| result[workflow]["action_match"] = ( | ||
| workflow.when_condition_group.logic_type if workflow.when_condition_group else None |
There was a problem hiding this comment.
Bug: The code accesses prefetched_wdcgs[0] without checking if the list is empty, which can cause an IndexError when serializing a workflow with no action filters.
Severity: HIGH
Suggested Fix
Add a conditional check to ensure workflow.prefetched_wdcgs is not empty before accessing its first element, for example: if workflow.prefetched_wdcgs:. Alternatively, consider making action_filters a required field in the validator to prevent this data state from occurring.
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/serializers/models/rule.py#L486
Potential issue: In `WorkflowEngineRuleSerializer.get_attrs()`, the code directly
accesses the first element of `workflow.prefetched_wdcgs`. However, the
`WorkflowValidator` allows for the creation of workflows without `action_filters`, which
results in `prefetched_wdcgs` being an empty list. When the serializer attempts to
process such a workflow, accessing `workflow.prefetched_wdcgs[0]` will raise an
`IndexError`, causing the API endpoint for retrieving the rule to return a 500 error.
2 participants
Make
ProjectRuleDetailsEndpointGET method backwards compatible mostly via a newconvert_argsmethod that first attempts to looks up theAlertRuleWorkflowfor dual written rules, and if that fails we assume it is a singly written workflow and look it up directly on theWorkflowmodel.