From: Mike Bayer Date: Thu, 22 Apr 2021 14:45:01 +0000 (-0400) Subject: omit text from selected_columns; clear memoizations X-Git-Tag: rel_1_4_12~36^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=17072f8b04c6a3a989673e85ace163620f9130cd;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git omit text from selected_columns; clear memoizations Fixed regression where usage of the :func:`_sql.text` construct inside the columns clause of a :class:`_sql.Select` construct, which is better handled by using a :func:`_sql.literal_column` construct, would nonetheless prevent constructs like :func:`_sql.union` from working correctly. Other use cases, such as constructing subuqeries, continue to work the same as in prior versions where the :func:`_sql.text` construct is silently omitted from the collection of exported columns. Also repairs similar use within the ORM. This adds a new internal method _all_selected_columns. The existing "selected_columns" collection can't store a TextClause and this never worked, so they are omitted. The TextClause is also not "exported", i.e. available for SELECT from a subquery, as was already the case in 1.3, so the "exported_columns" and "exported_columns_iterator" accessors are where we now omit TextClause. Fixed regression involving legacy methods such as :meth:`_sql.Select.append_column` where internal assertions would fail. Fixes: #6343 Fixes: #6261 Change-Id: I7c2e5b9ae5d94131c77599a020f4310dcf812bcf --- diff --git a/doc/build/changelog/unreleased_14/6343.rst b/doc/build/changelog/unreleased_14/6343.rst new file mode 100644 index 0000000000..8a3c3befaa --- /dev/null +++ b/doc/build/changelog/unreleased_14/6343.rst @@ -0,0 +1,20 @@ +.. change:: + :tags: bug, regression, sql + :tickets: 6343 + + Fixed regression where usage of the :func:`_sql.text` construct inside the + columns clause of a :class:`_sql.Select` construct, which is better handled + by using a :func:`_sql.literal_column` construct, would nonetheless prevent + constructs like :func:`_sql.union` from working correctly. Other use cases, + such as constructing subuqeries, continue to work the same as in prior + versions where the :func:`_sql.text` construct is silently omitted from the + collection of exported columns. Also repairs similar use within the + ORM. + + +.. change:: + :tags: bug, regression, sql + :tickets: 6261 + + Fixed regression involving legacy methods such as + :meth:`_sql.Select.append_column` where internal assertions would fail. diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index b0b8faee73..6cdad9f417 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -789,6 +789,14 @@ class ORMSelectCompileState(ORMCompileState, SelectState): @classmethod def exported_columns_iterator(cls, statement): + return ( + elem + for elem in cls.all_selected_columns(statement) + if not elem._is_text_clause + ) + + @classmethod + def all_selected_columns(cls, statement): for element in statement._raw_columns: if ( element.is_selectable @@ -2614,7 +2622,10 @@ class _RawColumnEntity(_ColumnEntity): self.expr = column self.raw_column_index = raw_column_index self.translate_raw_column = raw_column_index is not None - self._label_name = compile_state._label_convention(column) + if column._is_text_clause: + self._label_name = None + else: + self._label_name = compile_state._label_convention(column) if parent_bundle: parent_bundle._entities.append(self) diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index 4c591a87f2..6168248ff7 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -3156,7 +3156,7 @@ class SQLCompiler(Compiled): entry["select_0"] = select elif compound_index: select_0 = entry["select_0"] - numcols = len(select_0.selected_columns) + numcols = len(select_0._all_selected_columns) if len(compile_state.columns_plus_names) != numcols: raise exc.CompileError( @@ -3168,7 +3168,7 @@ class SQLCompiler(Compiled): 1, numcols, compound_index + 1, - len(select.selected_columns), + len(select._all_selected_columns), ) ) diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index 45fd26e89a..f3c5008521 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -2807,16 +2807,45 @@ class SelectBase( statement; a subquery must be applied first which provides for the necessary parenthesization required by SQL. + .. note:: + + The :attr:`_sql.SelectBase.selected_columns` collection does not + include expressions established in the columns clause using the + :func:`_sql.text` construct; these are silently omitted from the + collection. To use plain textual column expressions inside of a + :class:`_sql.Select` construct, use the :func:`_sql.literal_column` + construct. + + .. seealso:: + + :attr:`_sql.Select.selected_columns` + .. versionadded:: 1.4 """ raise NotImplementedError() + @property + def _all_selected_columns(self): + """A sequence of expressions that correspond to what is rendered + in the columns clause, including :class:`_sql.TextClause` + constructs. + + .. versionadded:: 1.4.12 + + .. seealso:: + + :attr:`_sql.SelectBase.exported_columns` + + """ + raise NotImplementedError() + @property def exported_columns(self): """A :class:`_expression.ColumnCollection` that represents the "exported" - columns of this :class:`_expression.Selectable`. + columns of this :class:`_expression.Selectable`, not including + :class:`_sql.TextClause` constructs. The "exported" columns for a :class:`_expression.SelectBase` object are synonymous @@ -2826,6 +2855,8 @@ class SelectBase( .. seealso:: + :attr:`_expression.Select.exported_columns` + :attr:`_expression.Selectable.exported_columns` :attr:`_expression.FromClause.exported_columns` @@ -3081,17 +3112,22 @@ class SelectStatementGrouping(GroupedElement, SelectBase): def _exported_columns_iterator(self): return self.element._exported_columns_iterator() + @property + def _all_selected_columns(self): + return self.element._all_selected_columns + @property def selected_columns(self): """A :class:`_expression.ColumnCollection` representing the columns that - the embedded SELECT statement returns in its result set. + the embedded SELECT statement returns in its result set, not including + :class:`_sql.TextClause` constructs. .. versionadded:: 1.4 .. seealso:: - :ref:`.SelectBase.selected_columns` + :attr:`_sql.Select.selected_columns` """ return self.element.selected_columns @@ -3881,6 +3917,7 @@ class CompoundSelect(HasCompileState, GenerativeSelect): # to how low in the list of select()s the column occurs, so # that the corresponding_column() operation can resolve # conflicts + for subq_col, select_cols in zip( subquery.c._all_columns, zip(*[s.selected_columns for s in self.selects]), @@ -3898,11 +3935,16 @@ class CompoundSelect(HasCompileState, GenerativeSelect): def _exported_columns_iterator(self): return self.selects[0]._exported_columns_iterator() + @property + def _all_selected_columns(self): + return self.selects[0]._all_selected_columns + @property def selected_columns(self): """A :class:`_expression.ColumnCollection` representing the columns that - this SELECT statement or similar construct returns in its result set. + this SELECT statement or similar construct returns in its result set, + not including :class:`_sql.TextClause` constructs. For a :class:`_expression.CompoundSelect`, the :attr:`_expression.CompoundSelect.selected_columns` @@ -3910,6 +3952,10 @@ class CompoundSelect(HasCompileState, GenerativeSelect): columns of the first SELECT statement contained within the series of statements within the set operation. + .. seealso:: + + :attr:`_sql.Select.selected_columns` + .. versionadded:: 1.4 """ @@ -4108,6 +4154,10 @@ class SelectState(util.MemoizedSlots, CompileState): @classmethod def _column_naming_convention(cls, label_style): + # note: these functions won't work for TextClause objects, + # which should be omitted when iterating through + # _raw_columns. + if label_style is LABEL_STYLE_NONE: def go(c, col_name=None): @@ -4281,7 +4331,15 @@ class SelectState(util.MemoizedSlots, CompileState): @classmethod def exported_columns_iterator(cls, statement): - return _select_iterables(statement._raw_columns) + return [ + c + for c in _select_iterables(statement._raw_columns) + if not c._is_text_clause + ] + + @classmethod + def all_selected_columns(cls, statement): + return [c for c in _select_iterables(statement._raw_columns)] def _setup_joins(self, args): for (right, onclause, left, flags) in args: @@ -5241,10 +5299,7 @@ class Select( clone=clone, omit_attrs=("_from_obj",), **kw ) - # memoizations should be cleared here as of - # I95c560ffcbfa30b26644999412fb6a385125f663 , asserting this - # is the case for now. - self._assert_no_memoizations() + self._reset_memoizations() def get_children(self, **kwargs): return itertools.chain( @@ -5269,10 +5324,7 @@ class Select( :class:`_expression.Select` object. """ - # memoizations should be cleared here as of - # I95c560ffcbfa30b26644999412fb6a385125f663 , asserting this - # is the case for now. - self._assert_no_memoizations() + self._reset_memoizations() self._raw_columns = self._raw_columns + [ coercions.expect( @@ -5602,7 +5654,8 @@ class Select( def selected_columns(self): """A :class:`_expression.ColumnCollection` representing the columns that - this SELECT statement or similar construct returns in its result set. + this SELECT statement or similar construct returns in its result set, + not including :class:`_sql.TextClause` constructs. This collection differs from the :attr:`_expression.FromClause.columns` collection of a :class:`_expression.FromClause` in that the columns @@ -5626,6 +5679,16 @@ class Select( :class:`_expression.ColumnElement` objects that are in the :attr:`_expression.FromClause.c` collection of the from element. + .. note:: + + The :attr:`_sql.Select.selected_columns` collection does not + include expressions established in the columns clause using the + :func:`_sql.text` construct; these are silently omitted from the + collection. To use plain textual column expressions inside of a + :class:`_sql.Select` construct, use the :func:`_sql.literal_column` + construct. + + .. versionadded:: 1.4 """ @@ -5640,6 +5703,11 @@ class Select( [(conv(c), c) for c in self._exported_columns_iterator()] ).as_immutable() + @HasMemoized.memoized_attribute + def _all_selected_columns(self): + meth = SelectState.get_plugin_class(self).all_selected_columns + return meth(self) + def _exported_columns_iterator(self): meth = SelectState.get_plugin_class(self).exported_columns_iterator return meth(self) @@ -5658,7 +5726,7 @@ class Select( different rules. """ - cols = self._exported_columns_iterator() + cols = self._all_selected_columns # when use_labels is on: # in all cases == if we see the same label name, use _label_anon_label @@ -6237,7 +6305,8 @@ class TextualSelect(SelectBase): def selected_columns(self): """A :class:`_expression.ColumnCollection` representing the columns that - this SELECT statement or similar construct returns in its result set. + this SELECT statement or similar construct returns in its result set, + not including :class:`_sql.TextClause` constructs. This collection differs from the :attr:`_expression.FromClause.columns` collection of a :class:`_expression.FromClause` in that the columns @@ -6250,6 +6319,7 @@ class TextualSelect(SelectBase): passed to the constructor, typically via the :meth:`_expression.TextClause.columns` method. + .. versionadded:: 1.4 """ diff --git a/lib/sqlalchemy/testing/__init__.py b/lib/sqlalchemy/testing/__init__.py index a3ce242262..a311efa742 100644 --- a/lib/sqlalchemy/testing/__init__.py +++ b/lib/sqlalchemy/testing/__init__.py @@ -60,6 +60,7 @@ from .exclusions import only_if from .exclusions import only_on from .exclusions import skip from .exclusions import skip_if +from .schema import eq_clause_element from .schema import eq_type_affinity from .util import adict from .util import fail diff --git a/lib/sqlalchemy/testing/schema.py b/lib/sqlalchemy/testing/schema.py index fee021cff1..ede3702e58 100644 --- a/lib/sqlalchemy/testing/schema.py +++ b/lib/sqlalchemy/testing/schema.py @@ -156,6 +156,19 @@ class eq_type_affinity(object): return self.target._type_affinity is not other._type_affinity +class eq_clause_element(object): + """Helper to compare SQL structures based on compare()""" + + def __init__(self, target): + self.target = target + + def __eq__(self, other): + return self.target.compare(other) + + def __ne__(self, other): + return not self.target.compare(other) + + def _truncate_name(dialect, name): if len(name) > dialect.max_identifier_length: return ( diff --git a/test/orm/test_core_compilation.py b/test/orm/test_core_compilation.py index cbc069a85c..c313843e90 100644 --- a/test/orm/test_core_compilation.py +++ b/test/orm/test_core_compilation.py @@ -7,6 +7,7 @@ from sqlalchemy import null from sqlalchemy import or_ from sqlalchemy import select from sqlalchemy import testing +from sqlalchemy import text from sqlalchemy import util from sqlalchemy.orm import aliased from sqlalchemy.orm import column_property @@ -108,6 +109,25 @@ class SelectableTest(QueryTest, AssertsCompiledSQL): }, ], ), + ( + lambda User, Address: (User.id, text("whatever")), + lambda User, Address: [ + { + "name": "id", + "type": testing.eq_type_affinity(sqltypes.Integer), + "aliased": False, + "expr": User.id, + "entity": User, + }, + { + "name": None, + "type": testing.eq_type_affinity(sqltypes.NullType), + "aliased": False, + "expr": testing.eq_clause_element(text("whatever")), + "entity": None, + }, + ], + ), ) def test_column_descriptions(self, cols, expected): User, Address = self.classes("User", "Address") diff --git a/test/sql/test_selectable.py b/test/sql/test_selectable.py index 8415043386..2403db7c95 100644 --- a/test/sql/test_selectable.py +++ b/test/sql/test_selectable.py @@ -538,6 +538,7 @@ class SelectableTest( "JOIN (SELECT 1 AS a, 2 AS b) AS joinfrom " "ON basefrom.a = joinfrom.a", ) + replaced.selected_columns replaced.add_columns.non_generative(replaced, joinfrom.c.b) self.assert_compile( replaced, @@ -546,6 +547,29 @@ class SelectableTest( "ON basefrom.a = joinfrom.a", ) + @testing.combinations( + ("_internal_subquery",), + ("selected_columns",), + ("_all_selected_columns"), + ) + def test_append_column_after_legacy_subq(self, attr): + """test :ticket:`6261`""" + + t1 = table("t1", column("a"), column("b")) + s1 = select(t1.c.a) + + if attr == "selected_columns": + s1.selected_columns + elif attr == "_internal_subuqery": + with testing.expect_deprecated("The SelectBase.c"): + s1.c + elif attr == "_all_selected_columns": + s1._all_selected_columns + + s1.add_columns.non_generative(s1, t1.c.b) + + self.assert_compile(s1, "SELECT t1.a, t1.b FROM t1") + def test_against_cloned_non_table(self): # test that corresponding column digs across # clone boundaries with anonymous labeled elements diff --git a/test/sql/test_text.py b/test/sql/test_text.py index 6dccf3be60..89cfa62c9f 100644 --- a/test/sql/test_text.py +++ b/test/sql/test_text.py @@ -25,6 +25,8 @@ from sqlalchemy.sql import quoted_name from sqlalchemy.sql import sqltypes from sqlalchemy.sql import table from sqlalchemy.sql import util as sql_util +from sqlalchemy.sql.selectable import LABEL_STYLE_DISAMBIGUATE_ONLY +from sqlalchemy.sql.selectable import LABEL_STYLE_NONE from sqlalchemy.testing import assert_raises_message from sqlalchemy.testing import AssertsCompiledSQL from sqlalchemy.testing import eq_ @@ -179,6 +181,122 @@ class SelectCompositionTest(fixtures.TestBase, AssertsCompiledSQL): "(select f from bar where lala=heyhey) foo WHERE foo.f = t.id", ) + @testing.combinations( + ( + None, + "SELECT mytable.myid, whatever FROM mytable " + "UNION SELECT mytable.myid, whatever FROM mytable", + ), + ( + LABEL_STYLE_NONE, + "SELECT mytable.myid, whatever FROM mytable " + "UNION SELECT mytable.myid, whatever FROM mytable", + ), + ( + LABEL_STYLE_DISAMBIGUATE_ONLY, + "SELECT mytable.myid, whatever FROM mytable " + "UNION SELECT mytable.myid, whatever FROM mytable", + ), + ( + LABEL_STYLE_TABLENAME_PLUS_COL, + "SELECT mytable.myid AS mytable_myid, whatever FROM mytable " + "UNION SELECT mytable.myid AS mytable_myid, whatever FROM mytable", + ), + ) + def test_select_composition_nine(self, label_style, expected): + + s1 = select(table1.c.myid, text("whatever")) + if label_style: + s1 = s1.set_label_style(label_style) + + s2 = select(table1.c.myid, text("whatever")) + + if label_style: + s2 = s2.set_label_style(label_style) + + stmt = s1.union(s2) + + self.assert_compile(stmt, expected) + + @testing.combinations( + ( + None, + "SELECT anon_1.myid FROM (SELECT mytable.myid AS myid, " + "whatever FROM mytable UNION SELECT mytable.myid AS myid, " + "whatever FROM mytable) AS anon_1", + ), + ( + LABEL_STYLE_NONE, + "SELECT anon_1.myid FROM (SELECT mytable.myid AS myid, " + "whatever FROM mytable UNION SELECT mytable.myid AS myid, " + "whatever FROM mytable) AS anon_1", + ), + ( + LABEL_STYLE_DISAMBIGUATE_ONLY, + "SELECT anon_1.myid FROM (SELECT mytable.myid AS myid, " + "whatever FROM mytable UNION SELECT mytable.myid AS myid, " + "whatever FROM mytable) AS anon_1", + ), + ( + LABEL_STYLE_TABLENAME_PLUS_COL, + "SELECT anon_1.mytable_myid FROM " + "(SELECT mytable.myid AS mytable_myid, whatever FROM mytable " + "UNION SELECT mytable.myid AS mytable_myid, whatever " + "FROM mytable) AS anon_1", + ), + ) + def test_select_composition_ten(self, label_style, expected): + + s1 = select(table1.c.myid, text("whatever")) + if label_style: + s1 = s1.set_label_style(label_style) + + s2 = select(table1.c.myid, text("whatever")) + + if label_style: + s2 = s2.set_label_style(label_style) + + stmt = s1.union(s2).subquery().select() + + self.assert_compile(stmt, expected) + + @testing.combinations( + (None, "SELECT mytable.myid, whatever FROM mytable"), + (LABEL_STYLE_NONE, "SELECT mytable.myid, whatever FROM mytable"), + ( + LABEL_STYLE_DISAMBIGUATE_ONLY, + "SELECT mytable.myid, whatever FROM mytable", + ), + ( + LABEL_STYLE_TABLENAME_PLUS_COL, + "SELECT mytable.myid AS mytable_myid, whatever FROM mytable", + ), + ) + def test_select_composition_eleven(self, label_style, expected): + + stmt = select(table1.c.myid, text("whatever")) + if label_style: + stmt = stmt.set_label_style(label_style) + + self.assert_compile(stmt, expected) + + @testing.combinations( + (None, ["myid", "description"]), + (LABEL_STYLE_NONE, ["myid", "description"]), + (LABEL_STYLE_DISAMBIGUATE_ONLY, ["myid", "description"]), + ( + LABEL_STYLE_TABLENAME_PLUS_COL, + ["mytable_myid", "mytable_description"], + ), + ) + def test_select_selected_columns_ignores_text(self, label_style, expected): + + stmt = select(table1.c.myid, text("whatever"), table1.c.description) + if label_style: + stmt = stmt.set_label_style(label_style) + + eq_(stmt.selected_columns.keys(), expected) + def test_select_bundle_columns(self): self.assert_compile( select(