Skip to content

Commit 753dc7b

Browse files
authored
fix(workflows): Handle duplicated AlertRuleWorkflow entries for a Workflow more gracefully (#113138)
We've got a few fixes for this situation in the works, but if we can avoid crashing in the meantime that seems ideal. Part of ISWF-2470.
1 parent edeeb63 commit 753dc7b

File tree

6 files changed

+72
-8
lines changed

6 files changed

+72
-8
lines changed

src/sentry/rules/history/backends/postgres.py

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,17 @@ def convert_hourly_stats(
5353
return results
5454

5555

56-
def get_rule_workflow_ids(target: Workflow | Rule) -> IdPair:
56+
def get_rule_workflow_ids(target: Workflow | Rule, project_id: int | None = None) -> IdPair:
5757
if isinstance(target, Workflow):
5858
workflow_id = target.id
5959
try:
60-
alert_rule_workflow = AlertRuleWorkflow.objects.get(workflow=target)
60+
qs = AlertRuleWorkflow.objects.filter(workflow=target)
61+
if project_id is not None:
62+
# rule_id is not a Django FK, so we use an __in subquery to
63+
# scope to the project. Django evaluates this as a single SQL
64+
# statement (subquery), not a separate round-trip.
65+
qs = qs.filter(rule_id__in=Rule.objects.filter(project_id=project_id).values("id"))
66+
alert_rule_workflow = qs.get()
6167
rule_id = alert_rule_workflow.rule_id
6268
except AlertRuleWorkflow.DoesNotExist:
6369
rule_id = None
@@ -186,8 +192,9 @@ def fetch_rule_groups_paginated(
186192
end: datetime,
187193
cursor: Cursor | None = None,
188194
per_page: int = 25,
195+
project_id: int | None = None,
189196
) -> CursorResult[RuleGroupHistory]:
190-
workflow_id, rule_id = get_rule_workflow_ids(target)
197+
workflow_id, rule_id = get_rule_workflow_ids(target, project_id=project_id)
191198

192199
if not workflow_id and isinstance(target, Rule):
193200
logger.warning("No workflow associated with rule", extra={"rule_id": rule_id})
@@ -257,12 +264,13 @@ def fetch_rule_hourly_stats(
257264
target: Rule | Workflow,
258265
start: datetime,
259266
end: datetime,
267+
project_id: int | None = None,
260268
) -> Sequence[TimeSeriesValue]:
261269
start = start.replace(tzinfo=timezone.utc)
262270
end = end.replace(tzinfo=timezone.utc)
263271
existing_data: dict[datetime, TimeSeriesValue] = {}
264272

265-
workflow_id, rule_id = get_rule_workflow_ids(target)
273+
workflow_id, rule_id = get_rule_workflow_ids(target, project_id=project_id)
266274
if not workflow_id and isinstance(target, Rule):
267275
logger.warning("No workflow associated with rule", extra={"rule_id": target.id})
268276
existing_data = self._fetch_rule_fire_history_hourly_stats(target, start, end)

src/sentry/rules/history/base.py

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,13 @@ def record(
5050
raise NotImplementedError
5151

5252
def fetch_rule_groups_paginated(
53-
self, target: Rule | Workflow, start: datetime, end: datetime, cursor: Cursor, per_page: int
53+
self,
54+
target: Rule | Workflow,
55+
start: datetime,
56+
end: datetime,
57+
cursor: Cursor,
58+
per_page: int,
59+
project_id: int | None = None,
5460
) -> CursorResult[RuleGroupHistory]:
5561
"""
5662
Fetches groups that triggered a rule or workflow within a given timeframe, ordered by number of
@@ -59,7 +65,7 @@ def fetch_rule_groups_paginated(
5965
raise NotImplementedError
6066

6167
def fetch_rule_hourly_stats(
62-
self, target: Rule | Workflow, start: datetime, end: datetime
68+
self, target: Rule | Workflow, start: datetime, end: datetime, project_id: int | None = None
6369
) -> Sequence[TimeSeriesValue]:
6470
"""
6571
Fetches counts of how often a rule has fired withing a given datetime range, bucketed by

src/sentry/rules/history/endpoints/project_rule_group_history.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,9 @@ def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Resp
8686
except InvalidParams:
8787
raise ParseError(detail="Invalid start and end dates")
8888

89-
results = fetch_rule_groups_paginated(rule, start, end, cursor, per_page)
89+
results = fetch_rule_groups_paginated(
90+
rule, start, end, cursor, per_page, project_id=project.id
91+
)
9092

9193
response = Response(serialize(results.results, request.user, RuleGroupHistorySerializer()))
9294
self.add_cursor_headers(request, response, results)

src/sentry/rules/history/endpoints/project_rule_stats.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,5 +66,5 @@ def get(self, request: Request, project: Project, rule: Rule | Workflow) -> Resp
6666
Note that results are returned in hourly buckets.
6767
"""
6868
start, end = get_date_range_from_params(request.GET)
69-
results = fetch_rule_hourly_stats(rule, start, end)
69+
results = fetch_rule_hourly_stats(rule, start, end, project_id=project.id)
7070
return Response(serialize(results, request.user, TimeSeriesValueSerializer()))

tests/sentry/rules/history/endpoints/test_project_rule_group_history.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
from sentry.testutils.cases import APITestCase, TestCase
88
from sentry.testutils.helpers.datetime import before_now, freeze_time
99
from sentry.testutils.skips import requires_snuba
10+
from sentry.workflow_engine.models import AlertRuleWorkflow
1011

1112
pytestmark = [requires_snuba]
1213

@@ -95,6 +96,29 @@ def test(self) -> None:
9596
RuleGroupHistorySerializer(),
9697
)
9798

99+
def test_shared_workflow_across_projects(self) -> None:
100+
project_a = self.project
101+
project_b = self.create_project(organization=self.organization)
102+
rule_a = self.create_project_rule(project=project_a)
103+
rule_b = self.create_project_rule(project=project_b)
104+
105+
# Simulate the bug: point both rules at the same Workflow
106+
arw_a = AlertRuleWorkflow.objects.get(rule_id=rule_a.id)
107+
shared_workflow = arw_a.workflow
108+
arw_b = AlertRuleWorkflow.objects.get(rule_id=rule_b.id)
109+
arw_b.workflow = shared_workflow
110+
arw_b.save()
111+
112+
self.login_as(self.user)
113+
# This would crash with MultipleObjectsReturned without the project_id fix
114+
self.get_success_response(
115+
self.organization.slug,
116+
project_a.slug,
117+
rule_a.id,
118+
start=before_now(days=1),
119+
end=before_now(days=0),
120+
)
121+
98122
def test_invalid_dates(self) -> None:
99123
rule = self.create_project_rule()
100124

tests/sentry/rules/history/endpoints/test_project_rule_stats.py

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from sentry.testutils.helpers.datetime import before_now, freeze_time
1111
from sentry.testutils.silo import control_silo_test
1212
from sentry.testutils.skips import requires_snuba
13+
from sentry.workflow_engine.models import AlertRuleWorkflow
1314

1415
pytestmark = [requires_snuba]
1516

@@ -75,6 +76,29 @@ def test(self) -> None:
7576
{"date": now, "count": 0},
7677
]
7778

79+
def test_shared_workflow_across_projects(self) -> None:
80+
project_a = self.project
81+
project_b = self.create_project(organization=self.organization)
82+
rule_a = self.create_project_rule(project=project_a)
83+
rule_b = self.create_project_rule(project=project_b)
84+
85+
# Simulate the bug: point both rules at the same Workflow
86+
arw_a = AlertRuleWorkflow.objects.get(rule_id=rule_a.id)
87+
shared_workflow = arw_a.workflow
88+
arw_b = AlertRuleWorkflow.objects.get(rule_id=rule_b.id)
89+
arw_b.workflow = shared_workflow
90+
arw_b.save()
91+
92+
self.login_as(self.user)
93+
# This would crash with MultipleObjectsReturned without the project_id fix
94+
self.get_success_response(
95+
self.organization.slug,
96+
project_a.slug,
97+
rule_a.id,
98+
start=before_now(days=1),
99+
end=before_now(days=0),
100+
)
101+
78102
def test_invalid_date_range(self) -> None:
79103
rule = self.create_project_rule(project=self.event.project)
80104
self.login_as(self.user)

0 commit comments

Comments
 (0)