ensure we unwrap desc() /label() all the way w/ order by

The deprecated logic to move order_by expressions
up into the columns clause needed adjustment to accommodate
for a more deeply-wrapped structure when desc() + label()
 are combined in an order by column.  This structure
now comes from coercions in 1.4.  it's not clear to me
at the moment why it's different from 1.3 but
this shouldn't really matter.

Fixes: #5443
Change-Id: If909a86f715992318d7aa283603197f7711f1d3b

3 files changed, 65 insertions, 2 deletions

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index e8726000b..b803ef912 100644
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -27,6 +27,7 @@ from .elements import _textual_label_reference
 from .elements import BindParameter
 from .elements import ColumnClause
 from .elements import ColumnElement
+from .elements import Label
 from .elements import Null
 from .elements import UnaryExpression
 from .schema import Column
@@ -279,14 +280,31 @@ def unwrap_order_by(clause):
     cols = util.column_set()
     result = []
     stack = deque([clause])
+
+    # examples
+    # column -> ASC/DESC == column
+    # column -> ASC/DESC -> label == column
+    # column -> label -> ASC/DESC -> label == column
+    # scalar_select -> label -> ASC/DESC == scalar_select -> label
+
     while stack:
         t = stack.popleft()
         if isinstance(t, ColumnElement) and (
             not isinstance(t, UnaryExpression)
             or not operators.is_ordering_modifier(t.modifier)
         ):
-            if isinstance(t, _label_reference):
+            if isinstance(t, Label) and not isinstance(
+                t.element, ScalarSelect
+            ):
+                t = t.element
+
+                stack.append(t)
+                continue
+            elif isinstance(t, _label_reference):
                 t = t.element
+
+                stack.append(t)
+                continue
             if isinstance(t, (_textual_label_reference)):
                 continue
             if t not in cols:
diff --git a/test/orm/test_deprecations.py b/test/orm/test_deprecations.py
index 4e9f50661..4cfded25c 100644
--- a/test/orm/test_deprecations.py
+++ b/test/orm/test_deprecations.py
@@ -1597,6 +1597,21 @@ class DistinctOrderByImplicitTest(QueryTest, AssertsCompiledSQL):
         ):
             eq_([User(id=7), User(id=9), User(id=8)], q.all())
 
+    def test_columns_augmented_roundtrip_two(self):
+        User, Address = self.classes.User, self.classes.Address
+
+        sess = create_session()
+        q = (
+            sess.query(User)
+            .join("addresses")
+            .distinct()
+            .order_by(desc(Address.email_address).label("foo"))
+        )
+        with testing.expect_deprecated(
+            "ORDER BY columns added implicitly due to "
+        ):
+            eq_([User(id=7), User(id=9), User(id=8)], q.all())
+
     def test_columns_augmented_roundtrip_three(self):
         User, Address = self.classes.User, self.classes.Address
 
diff --git a/test/sql/test_utils.py b/test/sql/test_utils.py
index d68a74475..676ad4298 100644
--- a/test/sql/test_utils.py
+++ b/test/sql/test_utils.py
@@ -4,9 +4,14 @@ from sqlalchemy import MetaData
 from sqlalchemy import select
 from sqlalchemy import String
 from sqlalchemy import Table
+from sqlalchemy import testing
+from sqlalchemy import util
 from sqlalchemy.sql import base as sql_base
+from sqlalchemy.sql import coercions
+from sqlalchemy.sql import column
+from sqlalchemy.sql import ColumnElement
+from sqlalchemy.sql import roles
 from sqlalchemy.sql import util as sql_util
-from sqlalchemy.sql.elements import ColumnElement
 from sqlalchemy.testing import assert_raises
 from sqlalchemy.testing import assert_raises_message
 from sqlalchemy.testing import eq_
@@ -89,3 +94,28 @@ class MiscTest(fixtures.TestBase):
         eq_(o4.bat, "hi")
 
         assert_raises(TypeError, opt2.safe_merge, o4)
+
+    @testing.combinations(
+        (column("q"), [column("q")]),
+        (column("q").desc(), [column("q")]),
+        (column("q").desc().label(None), [column("q")]),
+        (column("q").label(None).desc(), [column("q")]),
+        (column("q").label(None).desc().label(None), [column("q")]),
+        ("foo", []),  # textual label reference
+        (
+            select([column("q")]).scalar_subquery().label(None),
+            [select([column("q")]).scalar_subquery().label(None)],
+        ),
+        (
+            select([column("q")]).scalar_subquery().label(None).desc(),
+            [select([column("q")]).scalar_subquery().label(None)],
+        ),
+    )
+    def test_unwrap_order_by(self, expr, expected):
+
+        expr = coercions.expect(roles.OrderByRole, expr)
+
+        unwrapped = sql_util.unwrap_order_by(expr)
+
+        for a, b in util.zip_longest(unwrapped, expected):
+            assert a is not None and a.compare(b)