fix(iswf): Fixes issue with old Sentry App rules failing to serialize when alert schema is removed#113829
Conversation
… when alert schema is removed
ceorourke
left a comment
There was a problem hiding this comment.
Fix seems reasonable as long as we're ok with this behavior - I guess the alternative is a clearer error message saying they need to put the schema back.
Backend Test FailuresFailures on
|
|
|
||
| if not alert_rule_component: | ||
| raise ValidationError( | ||
| f"Alert Actions are not enabled for the {sentry_app.name} integration." | ||
| ) | ||
|
|
||
| schema_title = alert_rule_component.app_schema.get("title") | ||
| schema_title = None |
There was a problem hiding this comment.
Bug: The code breaks after finding the first alert-rule-action component, potentially missing a title defined in a subsequent component and causing inconsistent behavior with other parts of the system.
Severity: LOW
Suggested Fix
Modify the loop in sentry_app_issue_alert_handler.py to match the logic in notify_event.py. Instead of breaking on the first alert-rule-action component, iterate through all of them until a non-null title is found. This will ensure consistent title resolution across the application.
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/issue_alert_registry/handlers/sentry_app_issue_alert_handler.py#L109-L110
Potential issue: There is a behavioral inconsistency in how the alert rule action title
is resolved. The new code in `sentry_app_issue_alert_handler.py` iterates through a
Sentry App's components and breaks after finding the first one with type
`alert-rule-action`. It then checks for a `title`. If this first component has no title,
it defaults to `sentry_app.name`. However, other parts of the system, like
`notify_event.py`, continue iterating to find any component with a title. This can lead
to different results if a Sentry App has multiple `alert-rule-action` components and the
first one lacks a title while a subsequent one has it.
Did we get this right? 👍 / 👎 to inform future reviews.
Our label rendering logic for Sentry App rules seems to rely on a schema component being defined for alert rule creation, which is reasonable.
Unfortunately, this also breaks rule API queries if this schema component is removed. This PR fixes the issue by defaulting to the Sentry App name when a schema title is not availble.
To reproduce:
Resolves: ISWF-2340