-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(aci): handle fake alert rule ids in AlertRuleDetector lookup #104031
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
mifu67
left a comment
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.
src/sentry/workflow_engine/endpoints/organization_alertrule_detector_index.py
Outdated
Show resolved
Hide resolved
| "alertRuleId": str(alert_rule_id), | ||
| "ruleId": None, | ||
| } | ||
| ) |
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: Fallback ignores detector_id and rule_id filters
The fallback logic activates whenever alert_rule_id is provided, even when combined with detector_id or rule_id filters. It then returns a detector based solely on the calculated detector ID from alert_rule_id, completely ignoring the other filter parameters. This creates inconsistent results where a user requests specific detector_id or rule_id values but receives data for a different detector derived from the alert_rule_id. The fallback should either validate that filters are consistent or only activate when alert_rule_id is the sole filter.
| calculated_detector_id = get_object_id_from_fake_id(int(alert_rule_id)) | ||
| detector = Detector.objects.get(id=calculated_detector_id) | ||
|
|
||
| if detector: | ||
| return Response( | ||
| { | ||
| "detectorId": str(detector.id), | ||
| "alertRuleId": str(alert_rule_id), | ||
| "ruleId": None, | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #104031 +/- ##
===========================================
+ Coverage 78.04% 80.44% +2.39%
===========================================
Files 9313 9316 +3
Lines 397417 397506 +89
Branches 25373 25373
===========================================
+ Hits 310178 319759 +9581
+ Misses 86787 77295 -9492
Partials 452 452 |

When looking up a Detector via alert rule id, it's possible that there is no corresponding row in the AlertRuleDetector table. We then assume that the alert rule id that was passed is a fake id, so we can use get_object_id_from_fake_id() to get the detector.