diff options
| -rw-r--r-- | doc/build/changelog/unreleased_14/6661.rst | 22 | ||||
| -rw-r--r-- | doc/build/changelog/unreleased_14/6663.rst | 9 | ||||
| -rw-r--r-- | lib/sqlalchemy/engine/cursor.py | 4 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/context.py | 23 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/mapper.py | 1 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/properties.py | 6 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/strategies.py | 4 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/compiler.py | 15 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/dml.py | 13 | ||||
| -rw-r--r-- | lib/sqlalchemy/sql/selectable.py | 62 | ||||
| -rw-r--r-- | test/orm/test_core_compilation.py | 179 | ||||
| -rw-r--r-- | test/sql/test_cte.py | 51 |
12 files changed, 316 insertions, 73 deletions
diff --git a/doc/build/changelog/unreleased_14/6661.rst b/doc/build/changelog/unreleased_14/6661.rst new file mode 100644 index 000000000..6f08971be --- /dev/null +++ b/doc/build/changelog/unreleased_14/6661.rst @@ -0,0 +1,22 @@ +.. change:: + :tags: bug, orm + :tickets: 6661 + + Refined the behavior of ORM subquery rendering with regards to deferred + columns and column properties to be more compatible with that of 1.3 while + also providing for 1.4's newer features. As a subquery in 1.4 does not make + use of loader options, including :func:`_orm.deferred`, a subquery that is + against an ORM entity with deferred attributes will now render those + deferred attributes that refer directly to mapped table columns, as these + are needed in the outer SELECT if that outer SELECT makes use of these + columns; however a deferred attribute that refers to a composed SQL + expression as we normally do with :func:`_orm.column_property` will not be + part of the subquery, as these can be selected explicitly if needed in the + subquery. If the entity is being SELECTed from this subquery, the column + expression can still render on "the outside" in terms of the derived + subquery columns. This produces essentially the same behavior as when + working with 1.3. However in this case the fix has to also make sure that + the ``.selected_columns`` collection of an ORM-enabled :func:`_sql.select` + also follows these rules, which in particular allows recursive CTEs to + render correctly in this scenario, which were previously failing to render + correctly due to this issue. diff --git a/doc/build/changelog/unreleased_14/6663.rst b/doc/build/changelog/unreleased_14/6663.rst new file mode 100644 index 000000000..50367be5a --- /dev/null +++ b/doc/build/changelog/unreleased_14/6663.rst @@ -0,0 +1,9 @@ +.. change:: + :tags: bug, sql + :tickets: 6663 + + Fixed issue in CTE constructs mostly relevant to ORM use cases where a + recursive CTE against "anonymous" labels such as those seen in ORM + ``column_property()`` mappings would render in the + ``WITH RECURSIVE xyz(...)`` section as their raw internal label and not a + cleanly anonymized name. diff --git a/lib/sqlalchemy/engine/cursor.py b/lib/sqlalchemy/engine/cursor.py index cf77d0835..965959846 100644 --- a/lib/sqlalchemy/engine/cursor.py +++ b/lib/sqlalchemy/engine/cursor.py @@ -132,9 +132,7 @@ class CursorResultMetaData(ResultMetaData): keymap_by_position = self._keymap_by_result_column_idx - for idx, new in enumerate( - invoked_statement._exported_columns_iterator() - ): + for idx, new in enumerate(invoked_statement._all_selected_columns): try: rec = keymap_by_position[idx] except KeyError: diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index 321eeada0..78026efb1 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -153,6 +153,7 @@ class ORMCompileState(CompileState): ("_only_load_props", InternalTraversal.dp_plain_obj), ("_set_base_alias", InternalTraversal.dp_boolean), ("_for_refresh_state", InternalTraversal.dp_boolean), + ("_render_for_subquery", InternalTraversal.dp_boolean), ] # set to True by default from Query._statement_20(), to indicate @@ -176,6 +177,7 @@ class ORMCompileState(CompileState): _only_load_props = None _set_base_alias = False _for_refresh_state = False + _render_for_subquery = False current_path = _path_registry @@ -530,9 +532,7 @@ class ORMSelectCompileState(ORMCompileState, SelectState): self.select_statement = select_statement # indicates this select() came from Query.statement - self.for_statement = ( - for_statement - ) = select_statement._compile_options._for_statement + self.for_statement = select_statement._compile_options._for_statement # generally if we are from Query or directly from a select() self.use_legacy_query_style = ( @@ -554,13 +554,12 @@ class ORMSelectCompileState(ORMCompileState, SelectState): self.compile_options = select_statement._compile_options - if not for_statement and not toplevel: - # for subqueries, turn off eagerloads. - # if "for_statement" mode is set, Query.subquery() - # would have set this flag to False already if that's what's - # desired + if not toplevel: + # for subqueries, turn off eagerloads and set + # "render_for_subquery". self.compile_options += { "_enable_eagerloads": False, + "_render_for_subquery": True, } # determine label style. we can make different decisions here. @@ -855,14 +854,6 @@ class ORMSelectCompileState(ORMCompileState, SelectState): return target @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 ( diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 6054029aa..378c65278 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -2311,6 +2311,7 @@ class Mapper( adapter.columns[prop.columns[0]] if adapter else prop.columns[0] for prop in poly_properties if isinstance(prop, properties.ColumnProperty) + and prop._renders_in_subqueries ] def _columns_plus_keys(self, polymorphic_mappers=()): diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py index 7823aca20..18121bb04 100644 --- a/lib/sqlalchemy/orm/properties.py +++ b/lib/sqlalchemy/orm/properties.py @@ -65,6 +65,7 @@ class ColumnProperty(StrategizedProperty): "_mapped_by_synonym", "_deferred_column_loader", "_raise_column_loader", + "_renders_in_subqueries", "raiseload", ) @@ -202,6 +203,11 @@ class ColumnProperty(StrategizedProperty): if self.raiseload: self.strategy_key += (("raiseload", True),) + def _memoized_attr__renders_in_subqueries(self): + return ("deferred", True) not in self.strategy_key or ( + self not in self.parent._readonly_props + ) + @util.preload_module("sqlalchemy.orm.state", "sqlalchemy.orm.strategies") def _memoized_attr__deferred_column_loader(self): state = util.preloaded.orm_state diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 4a8ebaabb..2a254f8de 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -421,6 +421,10 @@ class DeferredColumnLoader(LoaderStrategy): if ( ( + compile_state.compile_options._render_for_subquery + and self.parent_property._renders_in_subqueries + ) + or ( loadopt and "undefer_pks" in loadopt.local_opts and set(self.columns).intersection( diff --git a/lib/sqlalchemy/sql/compiler.py b/lib/sqlalchemy/sql/compiler.py index 220a0fa99..f47ea8f33 100644 --- a/lib/sqlalchemy/sql/compiler.py +++ b/lib/sqlalchemy/sql/compiler.py @@ -2562,18 +2562,20 @@ class SQLCompiler(Compiled): elif isinstance(cte.element, selectable.CompoundSelect): col_source = cte.element.selects[0] else: - assert False + assert False, "cte should only be against SelectBase" recur_cols = [ c for c in util.unique_list( - col_source._exported_columns_iterator() + col_source._all_selected_columns ) if c is not None ] text += "(%s)" % ( ", ".join( - self.preparer.format_column(ident) + self.preparer.format_column( + ident, anon_map=self.anon_map + ) for ident in recur_cols ) ) @@ -5012,11 +5014,18 @@ class IdentifierPreparer(object): name=None, table_name=None, use_schema=False, + anon_map=None, ): """Prepare a quoted column name.""" if name is None: name = column.name + + if anon_map is not None and isinstance( + name, elements._truncated_label + ): + name = name.apply_map(anon_map) + if not getattr(column, "is_literal", False): if use_table: return ( diff --git a/lib/sqlalchemy/sql/dml.py b/lib/sqlalchemy/sql/dml.py index ea10bfc27..a6ef62619 100644 --- a/lib/sqlalchemy/sql/dml.py +++ b/lib/sqlalchemy/sql/dml.py @@ -27,6 +27,7 @@ from .elements import ClauseElement from .elements import Null from .selectable import HasCTE from .selectable import HasPrefixes +from .selectable import ReturnsRows from .visitors import InternalTraversal from .. import exc from .. import util @@ -199,6 +200,7 @@ class UpdateBase( HasCompileState, DialectKWArgs, HasPrefixes, + ReturnsRows, Executable, ClauseElement, ): @@ -415,13 +417,8 @@ class UpdateBase( coercions.expect(roles.ColumnsClauseRole, c) for c in cols ) - def _exported_columns_iterator(self): - """Return the RETURNING columns as a sequence for this statement. - - .. versionadded:: 1.4 - - """ - + @property + def _all_selected_columns(self): return self._returning @property @@ -434,7 +431,7 @@ class UpdateBase( """ # TODO: no coverage here return ColumnCollection( - (c.key, c) for c in self._exported_columns_iterator() + (c.key, c) for c in self._all_selected_columns ).as_immutable() @_generative diff --git a/lib/sqlalchemy/sql/selectable.py b/lib/sqlalchemy/sql/selectable.py index e1dee091b..557c443bf 100644 --- a/lib/sqlalchemy/sql/selectable.py +++ b/lib/sqlalchemy/sql/selectable.py @@ -109,23 +109,19 @@ class ReturnsRows(roles.ReturnsRowsRole, ClauseElement): @property def selectable(self): - raise NotImplementedError() - - def _exported_columns_iterator(self): - """An iterator of column objects that represents the "exported" - columns of this :class:`_expression.ReturnsRows`. + return self - This is the same set of columns as are returned by - :meth:`_expression.ReturnsRows.exported_columns` - except they are returned - as a simple iterator or sequence, rather than as a - :class:`_expression.ColumnCollection` namespace. + @property + def _all_selected_columns(self): + """A sequence of column expression objects that represents the + "selected" columns of this :class:`_expression.ReturnsRows`. - Subclasses should re-implement this method to bypass the interim - creation of the :class:`_expression.ColumnCollection` if appropriate. + This is typically equivalent to .exported_columns except it is + delivered in the form of a straight sequence and not keyed + :class:`_expression.ColumnCollection`. """ - return iter(self.exported_columns) + raise NotImplementedError() @property def exported_columns(self): @@ -161,10 +157,6 @@ class Selectable(ReturnsRows): is_selectable = True - @property - def selectable(self): - return self - def _refresh_for_new_column(self, column): raise NotImplementedError() @@ -3113,9 +3105,6 @@ class SelectStatementGrouping(GroupedElement, SelectBase): def _generate_proxy_for_new_column(self, column, subquery): return self.element._generate_proxy_for_new_column(subquery) - def _exported_columns_iterator(self): - return self.element._exported_columns_iterator() - @property def _all_selected_columns(self): return self.element._all_selected_columns @@ -3935,9 +3924,6 @@ class CompoundSelect(HasCompileState, GenerativeSelect): for select in self.selects: select._refresh_for_new_column(column) - 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 @@ -4335,7 +4321,7 @@ class SelectState(util.MemoizedSlots, CompileState): def _memoized_attr__label_resolve_dict(self): with_cols = dict( (c._resolve_label or c._label or c.key, c) - for c in self.statement._exported_columns_iterator() + for c in self.statement._all_selected_columns if c._allow_label_resolve ) only_froms = dict( @@ -4357,14 +4343,6 @@ class SelectState(util.MemoizedSlots, CompileState): return None @classmethod - def exported_columns_iterator(cls, statement): - 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)] @@ -5318,7 +5296,7 @@ class Select( """ - return self._exported_columns_iterator() + return iter(self._all_selected_columns) def is_derived_from(self, fromclause): if self in fromclause._cloned_set: @@ -5470,7 +5448,7 @@ class Select( """ return self.with_only_columns( *util.preloaded.sql_util.reduce_columns( - self._exported_columns_iterator(), + self._all_selected_columns, only_synonyms=only_synonyms, *(self._where_criteria + self._from_obj) ) @@ -5779,7 +5757,11 @@ class Select( conv = SelectState._column_naming_convention(self._label_style) return ColumnCollection( - [(conv(c), c) for c in self._exported_columns_iterator()] + [ + (conv(c), c) + for c in self._all_selected_columns + if not c._is_text_clause + ] ).as_immutable() @HasMemoized.memoized_attribute @@ -5787,10 +5769,6 @@ class Select( meth = SelectState.get_plugin_class(self).all_selected_columns return list(meth(self)) - def _exported_columns_iterator(self): - meth = SelectState.get_plugin_class(self).exported_columns_iterator - return meth(self) - def _ensure_disambiguated_names(self): if self._label_style is LABEL_STYLE_NONE: self = self.set_label_style(LABEL_STYLE_DISAMBIGUATE_ONLY) @@ -5912,7 +5890,7 @@ class Select( disambiguate_only = self._label_style is LABEL_STYLE_DISAMBIGUATE_ONLY for name, c, repeated in self._generate_columns_plus_names(False): - if not hasattr(c, "_make_proxy"): + if c._is_text_clause: continue elif tablename_plus_col: key = c._key_label @@ -6405,6 +6383,10 @@ class TextualSelect(SelectBase): (c.key, c) for c in self.column_args ).as_immutable() + @property + def _all_selected_columns(self): + return self.column_args + def _set_label_style(self, style): return self diff --git a/test/orm/test_core_compilation.py b/test/orm/test_core_compilation.py index 7a7b8aa67..12cfef339 100644 --- a/test/orm/test_core_compilation.py +++ b/test/orm/test_core_compilation.py @@ -12,11 +12,13 @@ from sqlalchemy import util from sqlalchemy.orm import aliased from sqlalchemy.orm import column_property from sqlalchemy.orm import contains_eager +from sqlalchemy.orm import deferred from sqlalchemy.orm import join as orm_join from sqlalchemy.orm import joinedload from sqlalchemy.orm import mapper from sqlalchemy.orm import query_expression from sqlalchemy.orm import relationship +from sqlalchemy.orm import undefer from sqlalchemy.orm import with_expression from sqlalchemy.orm import with_polymorphic from sqlalchemy.sql import and_ @@ -383,6 +385,39 @@ class LoadersInSubqueriesTest(QueryTest, AssertsCompiledSQL): return User, Address + @testing.fixture + def deferred_fixture(self): + User = self.classes.User + users = self.tables.users + + mapper( + User, + users, + properties={ + "name": deferred(users.c.name), + "name_upper": column_property( + func.upper(users.c.name), deferred=True + ), + }, + ) + + return User + + @testing.fixture + def non_deferred_fixture(self): + User = self.classes.User + users = self.tables.users + + mapper( + User, + users, + properties={ + "name_upper": column_property(func.upper(users.c.name)) + }, + ) + + return User + def test_no_joinedload_in_subquery_select_rows(self, joinedload_fixture): User, Address = joinedload_fixture @@ -434,9 +469,147 @@ class LoadersInSubqueriesTest(QueryTest, AssertsCompiledSQL): self.assert_compile(stmt2, expected) - # TODO: need to test joinedload options, deferred mappings, deferred - # options. these are all loader options that should *only* have an - # effect on the outermost statement, never a subquery. + def test_deferred_subq_one(self, deferred_fixture): + """test for #6661""" + User = deferred_fixture + + subq = select(User).subquery() + + u1 = aliased(User, subq) + q = select(u1) + + self.assert_compile( + q, + "SELECT anon_1.id " + "FROM (SELECT users.name AS name, " + "users.id AS id FROM users) AS anon_1", + ) + + # testing deferred opts separately for deterministic SQL generation + + q = select(u1).options(undefer(u1.name)) + + self.assert_compile( + q, + "SELECT anon_1.name, anon_1.id " + "FROM (SELECT users.name AS name, " + "users.id AS id FROM users) AS anon_1", + ) + + q = select(u1).options(undefer(u1.name_upper)) + + self.assert_compile( + q, + "SELECT upper(anon_1.name) AS upper_1, anon_1.id " + "FROM (SELECT users.name AS name, " + "users.id AS id FROM users) AS anon_1", + ) + + def test_non_deferred_subq_one(self, non_deferred_fixture): + """test for #6661 + + cols that aren't deferred go into subqueries. 1.3 did this also. + + """ + User = non_deferred_fixture + + subq = select(User).subquery() + + u1 = aliased(User, subq) + q = select(u1) + + self.assert_compile( + q, + "SELECT upper(anon_1.name) AS upper_1, anon_1.id, anon_1.name " + "FROM (SELECT upper(users.name) AS upper_2, users.id AS id, " + "users.name AS name FROM users) AS anon_1", + ) + + def test_deferred_subq_two(self, deferred_fixture): + """test for #6661 + + in this test, we are only confirming the current contract of ORM + subqueries which is that deferred + derived column_property's don't + export themselves into the .c. collection of a subquery. + We might want to revisit this in some way. + + """ + User = deferred_fixture + + subq = select(User).subquery() + + assert not hasattr(subq.c, "name_upper") + + # "undefer" it by including it + subq = select(User, User.name_upper).subquery() + + assert hasattr(subq.c, "name_upper") + + def test_non_deferred_col_prop_targetable_in_subq( + self, non_deferred_fixture + ): + """test for #6661""" + User = non_deferred_fixture + + subq = select(User).subquery() + + assert hasattr(subq.c, "name_upper") + + def test_recursive_cte_render_on_deferred(self, deferred_fixture): + """test for #6661. + + this test is most directly the bug reported in #6661, + as the CTE uses stmt._exported_columns_iterator() ahead of compiling + the SELECT in order to get the list of columns that will be selected, + this has to match what the subquery is going to render. + + This is also pretty fundamental to why deferred() as an option + can't be honored in a subquery; the subquery needs to export the + correct columns and it needs to do so without having to process + all the loader options. 1.3 OTOH when you got a subquery from + Query, it did a full compile_context. 1.4/2.0 we don't do that + anymore. + + """ + + User = deferred_fixture + + cte = select(User).cte(recursive=True) + + # nonsensical, but we are just testing form + cte = cte.union_all(select(User).join(cte, cte.c.id == User.id)) + + stmt = select(User).join(cte, User.id == cte.c.id) + + self.assert_compile( + stmt, + "WITH RECURSIVE anon_1(name, id) AS " + "(SELECT users.name AS name, users.id AS id FROM users " + "UNION ALL SELECT users.name AS name, users.id AS id " + "FROM users JOIN anon_1 ON anon_1.id = users.id) " + "SELECT users.id FROM users JOIN anon_1 ON users.id = anon_1.id", + ) + + # testing deferred opts separately for deterministic SQL generation + self.assert_compile( + stmt.options(undefer(User.name_upper)), + "WITH RECURSIVE anon_1(name, id) AS " + "(SELECT users.name AS name, users.id AS id FROM users " + "UNION ALL SELECT users.name AS name, users.id AS id " + "FROM users JOIN anon_1 ON anon_1.id = users.id) " + "SELECT upper(users.name) AS upper_1, users.id " + "FROM users JOIN anon_1 ON users.id = anon_1.id", + ) + + self.assert_compile( + stmt.options(undefer(User.name)), + "WITH RECURSIVE anon_1(name, id) AS " + "(SELECT users.name AS name, users.id AS id FROM users " + "UNION ALL SELECT users.name AS name, users.id AS id " + "FROM users JOIN anon_1 ON anon_1.id = users.id) " + "SELECT users.name, users.id " + "FROM users JOIN anon_1 ON users.id = anon_1.id", + ) class ExtraColsTest(QueryTest, AssertsCompiledSQL): diff --git a/test/sql/test_cte.py b/test/sql/test_cte.py index 59358aadf..01186c340 100644 --- a/test/sql/test_cte.py +++ b/test/sql/test_cte.py @@ -155,6 +155,57 @@ class CTETest(fixtures.TestBase, AssertsCompiledSQL): dialect=mssql.dialect(), ) + def test_recursive_w_anon_labels(self): + parts = table( + "parts", column("part"), column("sub_part"), column("quantity") + ) + + included_parts = ( + select( + parts.c.sub_part.label(None), + parts.c.part.label(None), + parts.c.quantity, + ) + .where(parts.c.part == "our part") + .cte(recursive=True) + ) + + incl_alias = included_parts.alias() + parts_alias = parts.alias() + included_parts = included_parts.union( + select( + parts_alias.c.sub_part, + parts_alias.c.part, + parts_alias.c.quantity, + ).where(parts_alias.c.part == incl_alias.c[0]) + ) + + s = ( + select( + included_parts.c[0], + func.sum(included_parts.c.quantity).label("total_quantity"), + ) + .select_from( + included_parts.join(parts, included_parts.c[1] == parts.c.part) + ) + .group_by(included_parts.c[0]) + ) + self.assert_compile( + s, + "WITH RECURSIVE anon_1(sub_part_1, part_1, quantity) " + "AS (SELECT parts.sub_part AS sub_part_1, parts.part " + "AS part_1, parts.quantity AS quantity FROM parts " + "WHERE parts.part = :part_2 UNION " + "SELECT parts_1.sub_part AS sub_part, " + "parts_1.part AS part, parts_1.quantity " + "AS quantity FROM parts AS parts_1, anon_1 AS anon_2 " + "WHERE parts_1.part = anon_2.sub_part_1) " + "SELECT anon_1.sub_part_1, " + "sum(anon_1.quantity) AS total_quantity FROM anon_1 " + "JOIN parts ON anon_1.part_1 = parts.part " + "GROUP BY anon_1.sub_part_1", + ) + def test_recursive_inner_cte_unioned_to_alias(self): parts = table( "parts", column("part"), column("sub_part"), column("quantity") |