Further fixes for ticket 5470

The fix for #5470 didn't actually take into account that
the "distinct" logic in query was also doubling up the criteria.
Added many more tests.   the 1.3 version here will be different
than 1.4 as the regression is not quite the same.

Fixes: #5470
Change-Id: I16a23917cab175761de9c867d9d9ac55031d9b97
(cherry picked from commit 7ac2d5c93b6334528aff93939559eee133f3e9fc)

diff --git a/doc/build/changelog/unreleased_13/5470.rst b/doc/build/changelog/unreleased_13/5470.rst
index ed7202df542027125c8422660de647f80c40d2f3..69d948e5aed8d46359bd52304f43ff96c074cd29 100644 (file)
--- a/doc/build/changelog/unreleased_13/5470.rst
+++ b/doc/build/changelog/unreleased_13/5470.rst
@@ -5,5 +5,7 @@
     Repaired an issue where the "ORDER BY" clause rendering a label name rather
     than a complete expression, which is particularly important for SQL Server,
     would fail to occur if the expression were enclosed in a parenthesized
-    grouping in some cases.   This case has been added to test support.
-
+    grouping in some cases.   This case has been added to test support. The
+    change additionally adjusts the "automatically add ORDER BY columns when
+    DISTINCT is present" behavior of ORM query, deprecated in 1.4, to more
+    accurately detect column expressions that are already present.

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index 0c574e3153379e2dea5281b0b3d50682ebedf1bc..29ed6fa7185719af617a9f73f5adedc529dffd75 100644 (file)
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -27,6 +27,8 @@ from .elements import _textual_label_reference
 from .elements import BindParameter
 from .elements import ColumnClause
 from .elements import ColumnElement
+from .elements import Grouping
+from .elements import Label
 from .elements import Null
 from .elements import UnaryExpression
 from .schema import Column
@@ -282,6 +284,17 @@ def unwrap_order_by(clause):
             not isinstance(t, UnaryExpression)
             or not operators.is_ordering_modifier(t.modifier)
         ):
+            if isinstance(t, Label) and not isinstance(
+                t.element, ScalarSelect
+            ):
+                t = t.element
+
+                if isinstance(t, Grouping):
+                    t = t.element
+
+                stack.append(t)
+                continue
+
             if isinstance(t, _label_reference):
                 t = t.element
             if isinstance(t, (_textual_label_reference)):
@@ -317,11 +330,9 @@ def expand_column_list_from_order_by(collist, order_by):
         ]
     )
 
-    return [
-        col
-        for col in chain(*[unwrap_order_by(o) for o in order_by])
-        if col not in cols_already_present
-    ]
+    to_look_for = list(chain(*[unwrap_order_by(o) for o in order_by]))
+
+    return [col for col in to_look_for if col not in cols_already_present]
 
 
 def clause_is_present(clause, search):

diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index df1cf27a7a67046ce4a177d1de130fc08f5dc35a..eb27e039459d85e3e5d361893bb637b3a12871a6 100644 (file)
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -3,6 +3,7 @@ import functools
 
 import sqlalchemy as sa
 from sqlalchemy import and_
+from sqlalchemy import asc
 from sqlalchemy import between
 from sqlalchemy import bindparam
 from sqlalchemy import Boolean
@@ -3787,6 +3788,76 @@ class DistinctTest(QueryTest, AssertsCompiledSQL):
             .all(),
         )
 
+    def test_issue_5470_one(self):
+        User = self.classes.User
+
+        expr = (User.id.op("+")(2)).label("label")
+
+        sess = create_session()
+
+        q = sess.query(expr).select_from(User).order_by(desc(expr)).distinct()
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :id_1 AS label "
+            "FROM users ORDER BY label DESC",
+        )
+
+    def test_issue_5470_two(self):
+        User = self.classes.User
+
+        expr = User.id + literal(1)
+
+        sess = create_session()
+        q = sess.query(expr).select_from(User).order_by(asc(expr)).distinct()
+
+        # no double col in the select list,
+        # there's no label so this is the requested SQL
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :param_1 AS anon_1 "
+            "FROM users ORDER BY users.id + :param_1 ASC",
+        )
+
+    def test_issue_5470_three(self):
+        User = self.classes.User
+
+        expr = (User.id + literal(1)).label("label")
+
+        sess = create_session()
+        q = sess.query(expr).select_from(User).order_by(asc(expr)).distinct()
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :param_1 AS label "
+            "FROM users ORDER BY label ASC",
+        )
+
+    def test_issue_5470_four(self):
+        User = self.classes.User
+
+        expr = (User.id + literal(1)).label("label")
+
+        sess = create_session()
+        q = (
+            sess.query(expr)
+            .select_from(User)
+            .order_by(asc("label"))
+            .distinct()
+        )
+
+        # no double col in the select list,
+        # orders by the label
+        self.assert_compile(
+            q,
+            "SELECT DISTINCT users.id + :param_1 AS label "
+            "FROM users ORDER BY label ASC",
+        )
+
     def test_columns_augmented_roundtrip_one(self):
         User, Address = self.classes.User, self.classes.Address