From 9ebbcdfd8e2af03466622e1670d804dec4c578b1 Mon Sep 17 00:00:00 2001 From: Leander Rodrigues Date: Thu, 12 Nov 2026 11:02:47 -0500 Subject: [PATCH] feat(lint): Flag .first() on querysets with no explicit ordering Add flake8 rule S034. .first() on a queryset with no .order_by(...) makes Django add ORDER BY pk, which can pick a slow query plan. The rule walks the visible chain back to a manager: the last order_by() decides, and an empty order_by() counts as unordered. Helper calls and bare variables are not flagged since they may already be ordered. Tests are exempt. Refs ISWF-3679 --- src/AGENTS.md | 1 + tests/tools/test_flake8_plugin.py | 41 ++++++++++++++++++ tools/flake8_plugin.py | 69 +++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+) diff --git a/src/AGENTS.md b/src/AGENTS.md index 169d8752e2cc..4acec19149c7 100644 --- a/src/AGENTS.md +++ b/src/AGENTS.md @@ -135,6 +135,7 @@ Logging (`LOG005`, `LOG011`), spans, metric tag cardinality, and `options.get()` 2. Data migrations using `apps.get_model()` operate on historical model classes: custom `save()` methods are absent, and signals scoped to the live model class do not run. Identify and explicitly perform required side effects such as cache invalidation; see the **`generate-migration`** skill. 3. Add indexes for queries on 1M+ row tables (`db_index=True` or `db_index_together`). 4. **Composite indexes**: any query filtering multiple columns (e.g. `foreign_key_id__in=... AND id__gt=...`, or FK + timestamp range, or cursor pagination combining filters) needs an explicit `Index(fields=[...])` in `Meta.indexes`, ordered most-selective-first. A single FK auto-index does NOT cover multi-column filters. See **`django-perf-review`** for validation. +5. **Single-row lookups**: use `.get_or_none(...)` for a unique row. `.first()` with no `.order_by(...)` adds `ORDER BY pk`, which can pick a slow query plan (S034); add an explicit `.order_by(...)` when several rows can match. Do not add new `# noqa: S034`. ### Redis TTLs diff --git a/tests/tools/test_flake8_plugin.py b/tests/tools/test_flake8_plugin.py index b52e23f3187f..68c58fead8dd 100644 --- a/tests/tools/test_flake8_plugin.py +++ b/tests/tools/test_flake8_plugin.py @@ -22,6 +22,7 @@ S027_msg, S028_msg, S030_allowlist, + S034_msg, SentryCheck, _s015_msg, get_api_violations, @@ -2478,3 +2479,43 @@ def get(self, request, drf_request): ] assert _run(src, filename="src/sentry/auth/principal.py") == [] assert _run(src, filename="tests/sentry/example.py") == [] + + +def test_S034() -> None: + src = """\ +Model.objects.first() # bad +Model.objects.filter(id=1).first() # bad +Model.objects.exclude(status=1).select_related("owner").first() # bad +Model.objects.filter(id=1).values_list("id", flat=True).first() # bad +project.teams.all().first() # bad +group.groupassignee_set.first() # bad +qs.filter(active=True).first() # bad +Model.objects.order_by().first() # bad: no fields clears ordering +Model.objects.order_by("name").filter(a=1).order_by().first() # bad: last order_by wins +x = ( + Model.objects.filter(id=1) + .first() # bad: reported on this line +) +Model.objects.order_by("id").first() # ok +Model.objects.order_by("-date").filter(a=1).select_related("owner").first() # ok +Model.objects.order_by().filter(a=1).order_by("id").first() # ok +Model.objects.order_by(*fields).first() # ok +Model.objects.filter_by_semver(v).first() # ok: custom method may order +get_queryset().filter(a=1).first() # ok: helper may order +qs.first() # ok: variable may already be ordered +detector.data_sources.first() # ok: not visibly a queryset +Model.objects.get_or_none(id=1) # ok +Model.objects.filter(id=1).first # ok: not called +""" + expected = [ + f"t.py:{line}:{col}: {S034_msg}" + for line, col in [(1, 13), (2, 26), (3, 55), (4, 55), (5, 19), (6, 23), (7, 22), (8, 24)] + ] + assert _run(src, filename="src/sentry/example.py") == [ + *expected, + f"t.py:9:53: {S034_msg}", + f"t.py:12:4: {S034_msg}", + ] + assert _run(src, filename="getsentry/example.py") == _run(src, filename="src/sentry/example.py") + assert _run(src, filename="tests/sentry/example.py") == [] + assert _run(src, filename="src/sentry/testutils/example.py") == [] diff --git a/tools/flake8_plugin.py b/tools/flake8_plugin.py index 4e7bc13369f2..69b5884668c1 100644 --- a/tools/flake8_plugin.py +++ b/tools/flake8_plugin.py @@ -200,6 +200,34 @@ "set_request_principal(request, user, auth)." ) +S034_msg = ( + "S034 .first() on a queryset with no .order_by(...) makes Django add ORDER BY pk, " + "which can pick a slow query plan; use .get_or_none(...) for a single unique row, " + "or add an explicit .order_by(...) to choose which row is returned" +) +# Calls that return a QuerySet; seeing one in the chain means `.first()` is a +# queryset's. Only chains visible in the expression are checked: `qs.first()` on a +# variable may already be ordered elsewhere. +_S034_queryset_methods = frozenset( + ( + "alias", + "all", + "annotate", + "defer", + "distinct", + "exclude", + "extra", + "filter", + "only", + "prefetch_related", + "select_for_update", + "select_related", + "using", + "values", + "values_list", + ) +) + # Existing callers are pinned to their exact function and occurrence count. This is a # shrink-only migration baseline: remove entries as callers move to named Seer one-shots. S030_allowlist: dict[str, dict[tuple[str, str], int]] = {} @@ -570,6 +598,38 @@ def _name_of(node: ast.expr) -> str: return ast.unparse(node) +def _is_unordered_queryset_first(node: ast.Call) -> bool: + """`.first()` where the visible chain never orders the queryset. + + Walks back from `.first()` through known QuerySet methods. The last `order_by` + in the chain decides: with fields it orders, with none it clears ordering. The + chain must start at a manager (`Model.objects`, `obj.foo_set`) or apply a known + QuerySet method to a name; any other call (a helper, a custom manager method) + may order the queryset itself, so it is not flagged. + """ + if not (isinstance(node.func, ast.Attribute) and node.func.attr == "first"): + return False + if node.args or node.keywords: + return False + + current = node.func.value + saw_queryset_method = False + while isinstance(current, ast.Call) and isinstance(current.func, ast.Attribute): + method = current.func.attr + if method == "order_by": + return not current.args + if method not in _S034_queryset_methods: + return False + saw_queryset_method = True + current = current.func.value + + if isinstance(current, ast.Attribute) and ( + current.attr == "objects" or current.attr.endswith("_set") + ): + return True + return saw_queryset_method and isinstance(current, (ast.Name, ast.Attribute)) + + def _is_acting_user_id(node: ast.expr) -> bool: """A call to `acting_user_id(...)`.""" return ( @@ -1630,6 +1690,15 @@ def visit_Call(self, node: ast.Call) -> None: if _is_acting_user_id(keyword.value): self.errors.append((keyword.lineno, keyword.col_offset, S032_msg)) + # S034: `.first()` on a queryset that the chain never orders. Tests may rely + # on the implicit pk ordering. + if not _is_tests_path(self.filename) and _is_unordered_queryset_first(node): + # Point at `.first` rather than the start of a multi-line chain. + assert node.func.end_lineno is not None and node.func.end_col_offset is not None + self.errors.append( + (node.func.end_lineno, node.func.end_col_offset - len(".first"), S034_msg) + ) + if isinstance(node.func, ast.Attribute) and node.func.attr in S019_logging_methods: for keyword in node.keywords: if keyword.arg == "extra" and isinstance(keyword.value, ast.Dict):