From 5d12593ae142ea8f377916f785b59c33d65046a4 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Sun, 26 Dec 2021 11:25:00 -0500 Subject: [PATCH] restore graceful degrade of subqueryload w from_statement Fixed regression from 1.3 where the "subqueryload" loader strategy would fail with a stack trace if used against a query that made use of :meth:`_orm.Query.from_statement` or :meth:`_sql.Select.from_statement`. As subqueryload requires modifying the original statement, it's not compatible with the "from_statement" use case, especially for statements made against the :func:`_sql.text` construct. The behavior now is equivalent to that of 1.3 and previously, which is that the loader strategy silently degrades to not be used for such statements, typically falling back to using the lazyload strategy. Fixes: #7505 Change-Id: I950800dc86a77f8320a5e696edce1ff2c84b1eb9 (cherry picked from commit 818d62be00530549aa52dd5d981819010e4ae484) --- doc/build/changelog/unreleased_14/7505.rst | 14 ++++ lib/sqlalchemy/orm/context.py | 6 ++ lib/sqlalchemy/orm/strategies.py | 6 ++ test/orm/test_eager_relations.py | 93 ++++++++++++++++++++++ test/orm/test_query.py | 16 ++-- test/orm/test_selectin_relations.py | 42 ++++++++++ test/orm/test_subquery_relations.py | 66 +++++++++++++++ 7 files changed, 237 insertions(+), 6 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/7505.rst diff --git a/doc/build/changelog/unreleased_14/7505.rst b/doc/build/changelog/unreleased_14/7505.rst new file mode 100644 index 0000000000..b017c0ae13 --- /dev/null +++ b/doc/build/changelog/unreleased_14/7505.rst @@ -0,0 +1,14 @@ +.. change:: + :tags: bug, orm, regression + :tickets: 7505 + + Fixed regression from 1.3 where the "subqueryload" loader strategy would + fail with a stack trace if used against a query that made use of + :meth:`_orm.Query.from_statement` or :meth:`_sql.Select.from_statement`. As + subqueryload requires modifying the original statement, it's not compatible + with the "from_statement" use case, especially for statements made against + the :func:`_sql.text` construct. The behavior now is equivalent to that of + 1.3 and previously, which is that the loader strategy silently degrades to + not be used for such statements, typically falling back to using the + lazyload strategy. + diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index b828bcb460..130fe67af6 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -382,6 +382,12 @@ class ORMCompileState(CompileState): for m in m2.iterate_to_root(): # TODO: redundant ? self._polymorphic_adapters[m.local_table] = adapter + @classmethod + def _create_entities_collection(cls, query, legacy): + raise NotImplementedError( + "this method only works for ORMSelectCompileState" + ) + @sql.base.CompileState.plugin_for("orm", "orm_from_statement") class ORMFromStatementCompileState(ORMCompileState): diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 71c4a69761..679b35a21e 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -27,6 +27,7 @@ from .base import _RAISE_FOR_STATE from .base import _SET_DEFERRED_EXPIRED from .context import _column_descriptions from .context import ORMCompileState +from .context import ORMSelectCompileState from .context import QueryContext from .interfaces import LoaderStrategy from .interfaces import StrategizedProperty @@ -1801,6 +1802,11 @@ class SubqueryLoader(PostLoader): # the other post loaders, however we have this here for consistency elif self._check_recursive_postload(context, path, self.join_depth): return + elif not isinstance(context.compile_state, ORMSelectCompileState): + # issue 7505 - subqueryload() in 1.3 and previous would silently + # degrade for from_statement() without warning. this behavior + # is restored here + return if not self.parent.class_manager[self.key].impl.supports_population: raise sa_exc.InvalidRequestError( diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 2ab2bba5c5..d9da36073c 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -86,6 +86,99 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): ) eq_(self.static.user_address_result, q.order_by(User.id).all()) + @testing.combinations(True, False) + def test_from_statement(self, legacy): + users, Address, addresses, User = ( + self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User, + ) + + self.mapper_registry.map_imperatively( + User, + users, + properties={ + "addresses": relationship( + self.mapper_registry.map_imperatively(Address, addresses), + order_by=Address.id, + ) + }, + ) + + sess = fixture_session() + + stmt = select(User).where(User.id == 7) + + def go(): + if legacy: + ret = ( + sess.query(User) + .from_statement(stmt) + .options(joinedload(User.addresses)) + .all() + ) + else: + ret = sess.scalars( + select(User) + .from_statement(stmt) + .options(joinedload(User.addresses)) + ).all() + + eq_(self.static.user_address_result[0:1], ret) + + # joinedload can't be applied here so this necessarily + # has to lazy load the addresses + self.assert_sql_count(testing.db, go, 2) + + @testing.combinations(True, False) + def test_from_statement_contains_eager(self, legacy): + users, Address, addresses, User = ( + self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User, + ) + + self.mapper_registry.map_imperatively( + User, + users, + properties={ + "addresses": relationship( + self.mapper_registry.map_imperatively(Address, addresses), + order_by=Address.id, + ) + }, + ) + + sess = fixture_session() + + # for contains_eager, Address.id is enough for it to be picked up + stmt = ( + select(User, Address.id).where(User.id == 7).join(User.addresses) + ) + + def go(): + if legacy: + ret = ( + sess.query(User) + .from_statement(stmt) + .options(contains_eager(User.addresses)) + .all() + ) + else: + ret = sess.scalars( + select(User) + .from_statement(stmt) + .options(contains_eager(User.addresses)) + ).all() + + eq_(self.static.user_address_result[0:1], ret) + + # joinedload can't be applied here so this necessarily + # has to lazy load the addresses + self.assert_sql_count(testing.db, go, 1) + def test_no_render_in_subquery(self): """test #6378""" diff --git a/test/orm/test_query.py b/test/orm/test_query.py index 8bf3dcdb5c..a1dbb2f617 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -5976,12 +5976,11 @@ class TextTest(QueryTest, AssertsCompiledSQL): ( False, subqueryload, - # sqlite seems happy to interpret the broken SQL and give you the - # correct result somehow, this is a bug in SQLite so don't rely - # upon it doing that - testing.fails("not working yet") + testing.skip_if("sqlite"), ), - (True, subqueryload, testing.fails("not sure about implementation")), + ( + True, + subqueryload, + ), (False, selectinload), (True, selectinload), ) @@ -6009,7 +6008,12 @@ class TextTest(QueryTest, AssertsCompiledSQL): def go(): eq_(set(q.all()), set(self.static.user_address_result)) - self.assert_sql_count(testing.db, go, 2) + if loader_option is subqueryload: + # subqueryload necessarily degrades to lazy loads for a text + # statement. + self.assert_sql_count(testing.db, go, 5) + else: + self.assert_sql_count(testing.db, go, 2) def test_whereclause(self): User = self.classes.User diff --git a/test/orm/test_selectin_relations.py b/test/orm/test_selectin_relations.py index 2add1015ff..7a5bb0e7ed 100644 --- a/test/orm/test_selectin_relations.py +++ b/test/orm/test_selectin_relations.py @@ -89,6 +89,48 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): self.assert_sql_count(testing.db, go, 2) + @testing.combinations(True, False) + def test_from_statement(self, legacy): + users, Address, addresses, User = ( + self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User, + ) + + self.mapper_registry.map_imperatively( + User, + users, + properties={ + "addresses": relationship( + self.mapper_registry.map_imperatively(Address, addresses), + order_by=Address.id, + ) + }, + ) + sess = fixture_session() + + stmt = select(User).where(User.id == 7) + + def go(): + if legacy: + ret = ( + sess.query(User) + .from_statement(stmt) + .options(selectinload(User.addresses)) + .all() + ) + else: + ret = sess.scalars( + select(User) + .from_statement(stmt) + .options(selectinload(User.addresses)) + ).all() + + eq_(self.static.user_address_result[0:1], ret) + + self.assert_sql_count(testing.db, go, 2) + def user_dingaling_fixture(self): users, Dingaling, User, dingalings, Address, addresses = ( self.tables.users, diff --git a/test/orm/test_subquery_relations.py b/test/orm/test_subquery_relations.py index 5be0042b0d..bf14d7212a 100644 --- a/test/orm/test_subquery_relations.py +++ b/test/orm/test_subquery_relations.py @@ -28,6 +28,7 @@ from sqlalchemy.testing import is_ from sqlalchemy.testing import is_not from sqlalchemy.testing import is_true from sqlalchemy.testing.assertsql import CompiledSQL +from sqlalchemy.testing.assertsql import Or from sqlalchemy.testing.entities import ComparableEntity from sqlalchemy.testing.fixtures import fixture_session from sqlalchemy.testing.schema import Column @@ -89,6 +90,71 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): self.assert_sql_count(testing.db, go, 2) + @testing.combinations(True, False) + def test_from_statement(self, legacy): + users, Address, addresses, User = ( + self.tables.users, + self.classes.Address, + self.tables.addresses, + self.classes.User, + ) + + self.mapper_registry.map_imperatively( + User, + users, + properties={ + "addresses": relationship( + self.mapper_registry.map_imperatively(Address, addresses), + order_by=Address.id, + ) + }, + ) + sess = fixture_session() + + stmt = select(User).where(User.id == 7) + + with self.sql_execution_asserter(testing.db) as asserter: + if legacy: + ret = ( + sess.query(User) + # .where(User.id == 7) + .from_statement(stmt) + .options(subqueryload(User.addresses)) + .all() + ) + else: + ret = sess.scalars( + select(User) + .from_statement(stmt) + .options(subqueryload(User.addresses)) + ).all() + + eq_(self.static.user_address_result[0:1], ret) + + asserter.assert_( + Or( + CompiledSQL( + "SELECT users.id AS users_id, users.name AS users_name " + "FROM users WHERE users.id = :id_1", + [{"id_1": 7}], + ), + CompiledSQL( + "SELECT users.id, users.name " + "FROM users WHERE users.id = :id_1", + [{"id_1": 7}], + ), + ), + # issue 7505 + # subqueryload degrades for a from_statement. this is a lazyload + CompiledSQL( + "SELECT addresses.id AS addresses_id, addresses.user_id AS " + "addresses_user_id, addresses.email_address AS " + "addresses_email_address FROM addresses " + "WHERE :param_1 = addresses.user_id ORDER BY addresses.id", + [{"param_1": 7}], + ), + ) + def test_params_arent_cached(self): users, Address, addresses, User = ( self.tables.users, -- 2.47.2