-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(aci): fix rule serializer lastTriggered to account for WorkflowFireHistory #95939
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
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.
smort
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95939 +/- ##
===========================================
+ Coverage 38.50% 87.78% +49.27%
===========================================
Files 9955 10585 +630
Lines 557639 610023 +52384
Branches 23982 23982
===========================================
+ Hits 214738 535520 +320782
+ Misses 342617 74219 -268398
Partials 284 284 |
@iamrajjoshi cursor concern is legit so gonna fix it up before merge |
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: Missing Keys in Lookup Cause Errors
A KeyError
occurs when attempting to update last_triggered_lookup
with WorkflowFireHistory
data. The last_triggered_lookup
dictionary is initially populated only with RuleFireHistory
entries. If a rule has WorkflowFireHistory
but no RuleFireHistory
, its rule_id
will be missing from last_triggered_lookup
, causing a KeyError
during direct access. Additionally, if existing_date
is None
, the comparison new_date > existing_date
would result in a TypeError
.
src/sentry/api/serializers/models/rule.py#L235-L239
sentry/src/sentry/api/serializers/models/rule.py
Lines 235 to 239 in e992fc2
# Take the maximum date between RuleFireHistory and WorkflowFireHistory | |
existing_date = last_triggered_lookup[rule_id] | |
new_date = wfh["date_added"] | |
if new_date > existing_date: | |
last_triggered_lookup[rule_id] = new_date |
Was this report helpful? Give feedback by reacting with 👍 or 👎
After we enable single processing, the last triggered field for the Rule in the serializer should also account for WorkflowFireHistory.