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
| project_slug=project.slug, | ||
| ) | ||
| serialized_rule = serialize(rule, request.user, rule_serializer) | ||
|
|
There was a problem hiding this comment.
PUT and DELETE crash when rule is a Workflow
High Severity
When the feature flag is on, convert_args can return a Workflow for dual-written or single-written rules. PUT and DELETE assume rule is a Rule and use rule.data, rule.label, rule.environment_id, rule.owner_team_id, rule.owner_user_id, rule.update(), RuleActivity.objects.create(rule=rule, ...), and rule.get_audit_log_data(). Workflow has .config and .name instead of .data and .label, and RuleActivity expects a Rule. These calls will raise AttributeError or type errors when rule is a Workflow.
There was a problem hiding this comment.
Yeah that's fine that's why it's behind a flag and we will get to it later.
![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
1 participant
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.