From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Thu, 13 Aug 2020 14:43:53 +0000 (-0400)
Subject: Further fixes for ticket 5470
X-Git-Tag: rel_1_3_19~4
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=2303c46087a6a2e49058ae71bb924fff6a2f21f9;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

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 ed7202df54..69d948e5ae 100644
--- 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 0c574e3153..29ed6fa718 100644
--- 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 df1cf27a7a..eb27e03945 100644
--- 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