feat(ACI): Make rule stats and group history endpoints backwards compatible#110282
Conversation
github-actions
Bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
saponifi3d
left a comment
saponifi3d
left a comment
There was a problem hiding this comment.
lgtm! my biggest question is around the tests and the workflow history being saved to the db, that + green seems good to me!
| alert_rule_workflow = AlertRuleWorkflow.objects.get(rule_id=rule.id) | ||
| workflow = alert_rule_workflow.workflow | ||
|
|
||
| if isinstance(target, Workflow): |
There was a problem hiding this comment.
nit: it might be nice to use this logic branch to create a new method that just handles workflows, and then another method that jut handles the Rule variant.
That could help with testing / readability of the code, and allow us to narrow the type of target to Rule / Workflow for any other methods being used.
|
|
||
| # make a couple in a different workflow to ensure we're not mixing them up | ||
| for i in range(2): | ||
| wfh = WorkflowFireHistory( |
There was a problem hiding this comment.
| wfh = WorkflowFireHistory( | |
| wfh = WorkflowFireHistory.objects.create( |
Should this be creating / saving these objects to the DB so that it can persist for the tests?
There was a problem hiding this comment.
Yeah oops, I copied the previous test that creates RuleFireHistory objects in bulk and forgot to update this 👍
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.
| """ | ||
|
|
||
| with connection.cursor() as cursor: | ||
| cursor.execute(query, [workflow.id, start, end, limit, offset]) |
There was a problem hiding this comment.
Pagination cursor shadowed by database cursor variable
Low Severity
The with connection.cursor() as cursor: on line 91 shadows the cursor pagination parameter from fetch_workflow_groups (line 68). While this happens to work because the database cursor binding is local to data_fn and the pagination cursor is only used at line 103 outside data_fn, the name collision is confusing. The neighboring fetch_workflow_stats method uses db_cursor for the database cursor, making this inconsistency especially noticeable.
Additional Locations (1)
…atible (#111159) Redo of #110282 to make the rule stats and group history endpoints backwards compatible by being flexible enough to handle either a `Rule` or a `Workflow`. The previous PR was reverted because I incorrectly assumed `Rule` objects were using `RuleFireHistory`, but it is correct to continue to combine those results with `WorkflowFireHistory` for now. For both methods the basic logic is as follows: - If passed a workflow, try to fetch the rule ID via `AlertRuleWorkflow` and fetch combined results. If the workflow was single written, fetch `WorkflowFireHistory` results only. - If passed a rule, try to fetch the workflow via `AlertRuleWorkflow` and fetch combined results. If the rule for some reason doesn't have a workflow, fetch `RuleFireHistory` results only (this maintains the existing behavior).
2 participants
Update the
ProjectRuleStatsIndexEndpointandProjectRuleGroupHistoryIndexEndpointto accept a workflow ID. Thankfully we already had the functions they call updated to use workflows so this wasn't too large of a change.