From: Mike Bayer Date: Thu, 13 Aug 2020 14:43:53 +0000 (-0400) Subject: Further fixes for ticket 5470 X-Git-Tag: rel_1_4_0b1~179^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=9c0b7baa956a7309f0a95fc36322a759b293eba1;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 --- diff --git a/doc/build/changelog/unreleased_13/5470.rst b/doc/build/changelog/unreleased_13/5470.rst index ed7202df54..8bf868f3a7 100644 --- a/doc/build/changelog/unreleased_13/5470.rst +++ b/doc/build/changelog/unreleased_13/5470.rst @@ -5,5 +5,8 @@ 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/orm/context.py b/lib/sqlalchemy/orm/context.py index a35b2f9fdf..0868fb29b4 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -923,7 +923,11 @@ class ORMSelectCompileState(ORMCompileState, SelectState): def _simple_statement(self): - if (self.distinct and not self.distinct_on) and self.order_by: + if ( + self.compile_options._use_legacy_query_style + and (self.distinct and not self.distinct_on) + and self.order_by + ): to_add = sql_util.expand_column_list_from_order_by( self.primary_columns, self.order_by ) diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py index 814253266f..b3ead718ac 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 Grouping from .elements import Label from .elements import Null from .elements import UnaryExpression @@ -298,6 +299,9 @@ def unwrap_order_by(clause): ): t = t.element + if isinstance(t, Grouping): + t = t.element + stack.append(t) continue elif isinstance(t, _label_reference): @@ -310,6 +314,7 @@ def unwrap_order_by(clause): if t not in cols: cols.add(t) result.append(t) + else: for c in t.get_children(): stack.append(c) @@ -338,11 +343,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 4663789c4d..99389439ea 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 @@ -3827,6 +3828,100 @@ class DistinctTest(QueryTest, AssertsCompiledSQL): .all(), ) + def test_no_automatic_distinct_thing_w_future(self): + User = self.classes.User + + stmt = select(User.id).order_by(User.name).distinct() + + self.assert_compile( + stmt, "SELECT DISTINCT users.id FROM users ORDER BY users.name" + ) + + 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_issue_5470_five(self): + User = self.classes.User + + expr = (User.id.op("+")(2)).label("label") + + stmt = select(expr).select_from(User).order_by(desc(expr)).distinct() + + # no double col in the select list, + # orders by the label + self.assert_compile( + stmt, + "SELECT DISTINCT users.id + :id_1 AS label " + "FROM users ORDER BY label DESC", + ) + def test_columns_augmented_roundtrip_one_from_self(self): """Test workaround for legacy style DISTINCT on extra column.