Skip to content

Commit 5455611

Browse files
authored
fix(query): Guard against ordering by non unique columns in RangeQuerySetWrapper (#69367)
Follow-up from inc-709. This incident was caused by `RangeQuerySetWrapper` looping infinitely and slamming queries into the database. It did this because we ordered on a non-unique column, and currently the logic here will loop forever when the last two rows of the result set have the same value for the sort col.
1 parent 73d8f89 commit 5455611

File tree

3 files changed

+29
-0
lines changed

3 files changed

+29
-0
lines changed

src/sentry/tasks/auto_ongoing_issues.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ def get_total_count(results):
140140
limit=ITERATOR_CHUNK * CHILD_TASK_COUNT,
141141
callbacks=[get_total_count],
142142
order_by="first_seen",
143+
override_unique_safety_check=True,
143144
),
144145
ITERATOR_CHUNK,
145146
):

src/sentry/utils/query.py

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@ def __init__(
9090
order_by="pk",
9191
callbacks=(),
9292
result_value_getter=None,
93+
override_unique_safety_check=True,
9394
):
9495
# Support for slicing
9596
if queryset.query.low_mark == 0 and not (
@@ -114,6 +115,16 @@ def __init__(
114115
self.callbacks = callbacks
115116
self.result_value_getter = result_value_getter
116117

118+
order_by_col = queryset.model._meta.get_field(order_by if order_by != "pk" else "id")
119+
if not override_unique_safety_check and not order_by_col.unique:
120+
# TODO: Ideally we could fix this bug and support ordering by a non unique col
121+
raise InvalidQuerySetError(
122+
"Order by column must be unique, otherwise this wrapper can get "
123+
"stuck in an infinite loop. If you're sure your data is unique, "
124+
"you can disable this by passing "
125+
"`override_unique_safety_check=True`"
126+
)
127+
117128
def __iter__(self):
118129
if self.min_value is not None:
119130
cur_value = self.min_value

tests/sentry/utils/test_query.py

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
1+
import pytest
2+
13
from sentry.db.models.query import in_iexact
24
from sentry.models.organization import Organization
35
from sentry.models.user import User
46
from sentry.models.userreport import UserReport
57
from sentry.testutils.cases import TestCase
68
from sentry.testutils.silo import no_silo_test
79
from sentry.utils.query import (
10+
InvalidQuerySetError,
811
RangeQuerySetWrapper,
912
RangeQuerySetWrapperWithProgressBar,
1013
RangeQuerySetWrapperWithProgressBarApprox,
@@ -54,6 +57,20 @@ def test_empty(self):
5457
qs = User.objects.all()
5558
assert len(list(self.range_wrapper(qs, step=2))) == 0
5659

60+
def test_order_by_non_unique_fails(self):
61+
qs = User.objects.all()
62+
with pytest.raises(InvalidQuerySetError):
63+
self.range_wrapper(qs, order_by="name", override_unique_safety_check=False)
64+
65+
# Shouldn't error if the safety check is disabled
66+
self.range_wrapper(qs, order_by="name", override_unique_safety_check=True)
67+
68+
def test_order_by_unique(self):
69+
self.create_user()
70+
qs = User.objects.all()
71+
self.range_wrapper(qs, order_by="username", override_unique_safety_check=False)
72+
assert len(list(self.range_wrapper(qs, order_by="username", step=2))) == 1
73+
5774

5875
@no_silo_test
5976
class RangeQuerySetWrapperWithProgressBarTest(RangeQuerySetWrapperTest):

0 commit comments

Comments
 (0)