From 278881e37619278789942513916acafaa88d26f3 Mon Sep 17 00:00:00 2001 From: Simon Charette Date: Fri, 17 Feb 2023 20:38:08 -0500 Subject: [PATCH] Fixed #34346 -- Ordered selected expressions by position. Used the same approach as for #34176 by using selected expressions position to prevent ambiguous aliases in collisions. Thanks henribru for the report. Regression in 04518e310d4552ff7595a34f5a7f93487d78a406. --- django/db/models/sql/compiler.py | 43 ++++++++++++++++++++++++------ tests/ordering/tests.py | 19 +++++++++++++ tests/postgres_tests/test_array.py | 3 +-- 3 files changed, 55 insertions(+), 10 deletions(-) diff --git a/django/db/models/sql/compiler.py b/django/db/models/sql/compiler.py index db123a2516..919e951fa2 100644 --- a/django/db/models/sql/compiler.py +++ b/django/db/models/sql/compiler.py @@ -27,6 +27,15 @@ from django.utils.hashable import make_hashable from django.utils.regex_helper import _lazy_re_compile +class PositionRef(Ref): + def __init__(self, ordinal, refs, source): + self.ordinal = ordinal + super().__init__(refs, source) + + def as_sql(self, compiler, connection): + return str(self.ordinal), () + + class SQLCompiler: # Multiline ordering SQL clause may appear from RawSQL. ordering_parts = _lazy_re_compile( @@ -321,6 +330,14 @@ class SQLCompiler: else: default_order, _ = ORDER_DIR["DESC"] + selected_exprs = {} + if select := self.select: + for ordinal, (expr, _, alias) in enumerate(select, start=1): + pos_expr = PositionRef(ordinal, alias, expr) + if alias: + selected_exprs[alias] = pos_expr + selected_exprs[expr] = pos_expr + for field in ordering: if hasattr(field, "resolve_expression"): if isinstance(field, Value): @@ -331,13 +348,23 @@ class SQLCompiler: if not self.query.standard_ordering: field = field.copy() field.reverse_ordering() - if isinstance(field.expression, F) and ( - annotation := self.query.annotation_select.get( - field.expression.name - ) + select_ref = selected_exprs.get(field.expression) + if select_ref or ( + isinstance(field.expression, F) + and (select_ref := selected_exprs.get(field.expression.name)) ): - field.expression = Ref(field.expression.name, annotation) - yield field, isinstance(field.expression, Ref) + # Emulation of NULLS (FIRST|LAST) cannot be combined with + # the usage of ordering by position. + if ( + field.nulls_first is None and field.nulls_last is None + ) or self.connection.features.supports_order_by_nulls_modifier: + field.expression = select_ref + # Alias collisions are not possible when dealing with + # combined queries so fallback to it if emulation of NULLS + # handling is required. + elif self.query.combinator: + field.expression = Ref(select_ref.refs, select_ref.source) + yield field, select_ref is not None continue if field == "?": # random yield OrderBy(Random()), False @@ -346,11 +373,11 @@ class SQLCompiler: col, order = get_order_dir(field, default_order) descending = order == "DESC" - if col in self.query.annotation_select: + if select_ref := selected_exprs.get(col): # Reference to expression in SELECT clause yield ( OrderBy( - Ref(col, self.query.annotation_select[col]), + select_ref, descending=descending, ), True, diff --git a/tests/ordering/tests.py b/tests/ordering/tests.py index 79e1714ab6..7ff38acc4a 100644 --- a/tests/ordering/tests.py +++ b/tests/ordering/tests.py @@ -8,6 +8,7 @@ from django.db.models import ( DateTimeField, F, Max, + OrderBy, OuterRef, Subquery, Value, @@ -619,3 +620,21 @@ class OrderingTests(TestCase): ), Author.objects.order_by(Length(Upper("name"))), ) + + def test_ordering_select_related_collision(self): + self.assertEqual( + Article.objects.select_related("author") + .annotate(name=Upper("author__name")) + .filter(pk=self.a1.pk) + .order_by(OrderBy(F("name"))) + .first(), + self.a1, + ) + self.assertEqual( + Article.objects.select_related("author") + .annotate(name=Upper("author__name")) + .filter(pk=self.a1.pk) + .order_by("name") + .first(), + self.a1, + ) diff --git a/tests/postgres_tests/test_array.py b/tests/postgres_tests/test_array.py index f58c9477fc..f7615c974e 100644 --- a/tests/postgres_tests/test_array.py +++ b/tests/postgres_tests/test_array.py @@ -465,10 +465,9 @@ class TestQuerying(PostgreSQLTestCase): {"field__0": 20, "arrayagg": [self.objs[3].pk]}, ], ) - alias = connection.ops.quote_name("field__0") sql = ctx[0]["sql"] self.assertIn("GROUP BY 1", sql) - self.assertIn(f"ORDER BY {alias}", sql) + self.assertIn("ORDER BY 1", sql) def test_index(self): self.assertSequenceEqual(