From: Mike Bayer Date: Sun, 26 Dec 2021 16:25:00 +0000 (-0500) Subject: restore graceful degrade of subqueryload w from_statement X-Git-Tag: rel_2_0_0b1~581 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=818d62be00530549aa52dd5d981819010e4ae484;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 --- 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 4e2586203c..e834b22e85 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 51e81e104d..d5f5f8527d 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -27,6 +27,7 @@ from .base import _SET_DEFERRED_EXPIRED from .base import PASSIVE_OFF 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 @@ -1791,6 +1792,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 4bd0f14171..6d6d10deaf 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 d3ca9521f8..74def1c2b8 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -5975,12 +5975,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), ) @@ -6008,7 +6007,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 e07da638f2..c48fe45881 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,