- altered part of the use contract first set up in #2992; we

now skip textual label references when copying ORDER BY elements
to the joined-eager-load subquery, as we can't know that these
expressions are compatible with this placement;  either because
they are meant for text(), or because they refer to label names
already stated and aren't bound to a table. fixes #3392

3 files changed, 154 insertions, 19 deletions

diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst
index b313c7cac..5b0ddf64f 100644
--- a/doc/build/changelog/changelog_10.rst
+++ b/doc/build/changelog/changelog_10.rst
@@ -19,6 +19,39 @@
     :version: 1.0.3
 
     .. change::
+        :tags: bug, orm
+        :tickets: 3392
+
+        Fixed regression due to :ticket:`2992` where textual elements placed
+        into the :meth:`.Query.order_by` clause in conjunction with joined
+        eager loading would be added to the columns clause of the inner query
+        in such a way that they were assumed to be table-bound column names,
+        in the case where the joined eager load needs to wrap the query
+        in a subquery to accommodate for a limit/offset.
+
+        Originally, the behavior here was intentional, in that a query such
+        as ``query(User).order_by('name').limit(1)``
+        would order by ``user.name`` even if the query was modified by
+        joined eager loading to be within a subquery, as ``'name'`` would
+        be interpreted as a symbol to be located within the FROM clauses,
+        in this case ``User.name``, which would then be copied into the
+        columns clause to ensure it were present for ORDER BY.  However, the
+        feature fails to anticipate the case where ``order_by("name")`` refers
+        to a specific label name present in the local columns clause already
+        and not a name bound to a selectable in the FROM clause.
+
+        Beyond that, the feature also fails for deprecated cases such as
+        ``order_by("name desc")``, which, while it emits a
+        warning that :func:`.text` should be used here (note that the issue
+        does not impact cases where :func:`.text` is used explicitly),
+        still produces a different query than previously where the "name desc"
+        expression is copied into the columns clause inappropriately.  The
+        resolution is such that the "joined eager loading" aspect of the
+        feature will skip over these so-called "label reference" expressions
+        when augmenting the inner columns clause, as though they were
+        :func:`.text` constructs already.
+
+    .. change::
         :tags: bug, sql
         :tickets: 3391
 
diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index bec5b5824..8f502fc86 100644
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -16,7 +16,8 @@ from itertools import chain
 from collections import deque
 
 from .elements import BindParameter, ColumnClause, ColumnElement, \
-    Null, UnaryExpression, literal_column, Label, _label_reference
+    Null, UnaryExpression, literal_column, Label, _label_reference, \
+    _textual_label_reference
 from .selectable import ScalarSelect, Join, FromClause, FromGrouping
 from .schema import Column
 
@@ -163,6 +164,8 @@ def unwrap_order_by(clause):
         ):
             if isinstance(t, _label_reference):
                 t = t.element
+            if isinstance(t, (_textual_label_reference)):
+                continue
             cols.add(t)
         else:
             for c in t.get_children():
diff --git a/test/orm/test_query.py b/test/orm/test_query.py
index 4021cdfbb..850014ab4 100644
--- a/test/orm/test_query.py
+++ b/test/orm/test_query.py
@@ -2860,44 +2860,143 @@ class TextTest(QueryTest, AssertsCompiledSQL):
             [User(id=7), User(id=8), User(id=9), User(id=10)]
         )
 
-    def test_order_by_w_eager(self):
+    def test_order_by_w_eager_one(self):
+        User = self.classes.User
+        s = create_session()
+
+        # from 1.0.0 thru 1.0.2, the "name" symbol here was considered
+        # to be part of the things we need to ORDER BY and it was being
+        # placed into the inner query's columns clause, as part of
+        # query._compound_eager_statement where we add unwrap_order_by()
+        # to the columns clause.  However, as #3392 illustrates, unlocatable
+        # string expressions like "name desc" will only fail in this scenario,
+        # so in general the changing of the query structure with string labels
+        # is dangerous.
+        #
+        # the queries here are again "invalid" from a SQL perspective, as the
+        # "name" field isn't matched up to anything.
+        #
+        with expect_warnings("Can't resolve label reference 'name';"):
+            self.assert_compile(
+                s.query(User).options(joinedload("addresses")).
+                order_by(desc("name")).limit(1),
+                "SELECT anon_1.users_id AS anon_1_users_id, "
+                "anon_1.users_name AS anon_1_users_name, "
+                "addresses_1.id AS addresses_1_id, "
+                "addresses_1.user_id AS addresses_1_user_id, "
+                "addresses_1.email_address AS addresses_1_email_address "
+                "FROM (SELECT users.id AS users_id, users.name AS users_name "
+                "FROM users ORDER BY users.name "
+                "DESC LIMIT :param_1) AS anon_1 "
+                "LEFT OUTER JOIN addresses AS addresses_1 "
+                "ON anon_1.users_id = addresses_1.user_id "
+                "ORDER BY name DESC, addresses_1.id"
+            )
+
+    def test_order_by_w_eager_two(self):
+        User = self.classes.User
+        s = create_session()
+
+        with expect_warnings("Can't resolve label reference 'name';"):
+            self.assert_compile(
+                s.query(User).options(joinedload("addresses")).
+                order_by("name").limit(1),
+                "SELECT anon_1.users_id AS anon_1_users_id, "
+                "anon_1.users_name AS anon_1_users_name, "
+                "addresses_1.id AS addresses_1_id, "
+                "addresses_1.user_id AS addresses_1_user_id, "
+                "addresses_1.email_address AS addresses_1_email_address "
+                "FROM (SELECT users.id AS users_id, users.name AS users_name "
+                "FROM users ORDER BY users.name "
+                "LIMIT :param_1) AS anon_1 "
+                "LEFT OUTER JOIN addresses AS addresses_1 "
+                "ON anon_1.users_id = addresses_1.user_id "
+                "ORDER BY name, addresses_1.id"
+            )
+
+    def test_order_by_w_eager_three(self):
+        User = self.classes.User
+        s = create_session()
+
+        self.assert_compile(
+            s.query(User).options(joinedload("addresses")).
+            order_by("users_name").limit(1),
+            "SELECT anon_1.users_id AS anon_1_users_id, "
+            "anon_1.users_name AS anon_1_users_name, "
+            "addresses_1.id AS addresses_1_id, "
+            "addresses_1.user_id AS addresses_1_user_id, "
+            "addresses_1.email_address AS addresses_1_email_address "
+            "FROM (SELECT users.id AS users_id, users.name AS users_name "
+            "FROM users ORDER BY users.name "
+            "LIMIT :param_1) AS anon_1 "
+            "LEFT OUTER JOIN addresses AS addresses_1 "
+            "ON anon_1.users_id = addresses_1.user_id "
+            "ORDER BY anon_1.users_name, addresses_1.id"
+        )
+
+        # however! this works (again?)
+        eq_(
+            s.query(User).options(joinedload("addresses")).
+            order_by("users_name").first(),
+            User(name='chuck', addresses=[])
+        )
+
+    def test_order_by_w_eager_four(self):
         User = self.classes.User
         Address = self.classes.Address
         s = create_session()
 
-        # here, we are seeing how Query has to take the order by expressions
-        # of the query and then add them to the columns list, so that the
-        # outer subquery can order by that same label.  With the anonymous
-        # label, our column gets sucked up and restated again in the
-        # inner columns list!
-        # we could try to play games with making this "smarter" but it
-        # would add permanent overhead to Select._columns_plus_names,
-        # since that's where references would need to be resolved.
-        # so as it is, this query takes the _label_reference and makes a
-        # full blown proxy and all the rest of it.
         self.assert_compile(
             s.query(User).options(joinedload("addresses")).
-            order_by(desc("name")).limit(1),
+            order_by(desc("users_name")).limit(1),
             "SELECT anon_1.users_id AS anon_1_users_id, "
             "anon_1.users_name AS anon_1_users_name, "
-            "anon_1.anon_2 AS anon_1_anon_2, "
             "addresses_1.id AS addresses_1_id, "
             "addresses_1.user_id AS addresses_1_user_id, "
             "addresses_1.email_address AS addresses_1_email_address "
-            "FROM (SELECT users.id AS users_id, users.name AS users_name, "
-            "users.name AS anon_2 FROM users ORDER BY users.name "
-            "DESC LIMIT :param_1) AS anon_1 "
+            "FROM (SELECT users.id AS users_id, users.name AS users_name "
+            "FROM users ORDER BY users.name DESC "
+            "LIMIT :param_1) AS anon_1 "
             "LEFT OUTER JOIN addresses AS addresses_1 "
             "ON anon_1.users_id = addresses_1.user_id "
-            "ORDER BY anon_1.anon_2 DESC, addresses_1.id"
+            "ORDER BY anon_1.users_name DESC, addresses_1.id"
         )
 
+        # however! this works (again?)
         eq_(
             s.query(User).options(joinedload("addresses")).
-                order_by(desc("name")).first(),
+            order_by(desc("users_name")).first(),
             User(name='jack', addresses=[Address()])
         )
 
+    def test_order_by_w_eager_five(self):
+        """essentially the same as test_eager_relations -> test_limit_3,
+        but test for textual label elements that are freeform.
+        this is again #3392."""
+
+        User = self.classes.User
+        Address = self.classes.Address
+        Order = self.classes.Order
+
+        sess = create_session()
+
+        q = sess.query(User, Address.email_address.label('email_address'))
+
+        l = q.join('addresses').options(joinedload(User.orders)).\
+            order_by(
+            "email_address desc").limit(1).offset(0)
+        with expect_warnings(
+                "Can't resolve label reference 'email_address desc'"):
+            eq_(
+                [
+                    (User(
+                        id=7,
+                        orders=[Order(id=1), Order(id=3), Order(id=5)],
+                        addresses=[Address(id=1)]
+                    ), 'jack@bean.com')
+                ],
+                l.all())
+
 
 class TextWarningTest(QueryTest, AssertsCompiledSQL):
     def _test(self, fn, arg, offending_clause, expected):