-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
chore(aci): populate legacy_rule_id and/or workflow_id in action dispatch #106051
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
85f9312 to
7e94a89
Compare
13bf816 to
57234f9
Compare
| workflow_id=workflow_id, | ||
| ) | ||
|
|
||
| rule = Rule.objects.get(id=alert_rule_workflow.rule_id) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will actually raise Rule.DoesNotExist
|
|
||
| # attempt to find legacy_rule_id from the alert rule workflow | ||
| try: | ||
| alert_rule_workflow = AlertRuleWorkflow.objects.get( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bah, shame we're not using foreign keys or we could just do something like Rule.objects.filter(alertruleworkflow__workflow_id=workflow_id).first().
I still think single expr and first might be nice here, eg:
rule = Rule.objects.filter(
id__in=AlertRuleWorkflow.objects.filter(workflow_id=workflow_id).values('rule_id')
).first()
if rule:
label = ..
rule_id = ...
3de4aa0 to
146639e
Compare
| alert_rule_workflow = AlertRuleWorkflow.objects.filter( | ||
| workflow_id=workflow_id, | ||
| ).first() | ||
| if alert_rule_workflow: | ||
| try: | ||
| rule = Rule.objects.get(id=alert_rule_workflow.rule_id) | ||
| label = rule.label | ||
| label = Rule.objects.get(id=alert_rule_workflow.rule_id).label | ||
| rule_id = alert_rule_workflow.rule_id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The query for AlertRuleWorkflow can fetch a metric alert where rule_id is None, causing an unhandled ValueError when Rule.objects.get(id=None) is called.
Severity: CRITICAL
Suggested Fix
Modify the AlertRuleWorkflow query to explicitly filter for entries related to issue alerts by adding rule_id__isnull=False. This ensures that the query only returns workflows that have a valid rule_id.
alert_rule_workflow = AlertRuleWorkflow.objects.filter(
workflow_id=workflow_id,
rule_id__isnull=False,
).first()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/notifications/notification_action/types.py#L206-L212
Potential issue: The query for an `AlertRuleWorkflow` using
`.filter(workflow_id=workflow_id).first()` is non-deterministic. If a workflow is
associated with both an issue alert (`rule_id` is set) and a metric alert
(`alert_rule_id` is set), this query might return the metric alert entry where `rule_id`
is `None`. The subsequent call, `Rule.objects.get(id=alert_rule_workflow.rule_id)`, will
then execute as `Rule.objects.get(id=None)`, raising a `ValueError`. This exception is
not caught by the `except Rule.DoesNotExist` block, leading to an unhandled error.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This raises Rule.DoesNotExist which is caught
Fully removes uses of workflow-engine-ui-links flag for issue alert firing.
We try to attach
legacy_rule_idto the fake Rule's action when possible and attach theworkflow_id. In downstream code we look forlegacy_rule_idfirst to generate links, and thenworkflow_id. If we generate a link to the Rule, the FE will redirect it to the workflow link if the new UI feature flag is on. Otherwise the workflow is single-written and we can only generate link to the workflow.Builds on #106048 (last one to remove the flag for issue alerts)