From 4e8577ca66d194661b1fed01c46a529655473b97 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 7 Jun 2021 18:49:04 -0400 Subject: [PATCH] restore adapter logic in ORM loading Fixed regression involving how the ORM would resolve a given mapped column to a result row, where under cases such as joined eager loading, a slightly more expensive "fallback" could take place to set up this resolution due to some logic that was removed since 1.3. The issue could also cause deprecation warnings involving column resolution to be emitted when using a 1.4 style query with joined eager loading. In order to ensure we don't look up columns by string name in the ORM, we've turned on future_result=True in all cases, which I thought was already the assumption here, but apparently not. That in turn led to the issue that Session autocommit relies on close_with_result=True, which is legacy result only. This was also hard to figure out. So a new exception is raised if one tries to use future_result=True along with close_with_result, and the Session now has an explicit path for "autocommit" that sets these flags to their legacy values. This does leave the possibility of some of these fallback cases emitting warnings for users using session in autocommit along with joined inheritance and column properties, as this patch identifies that joined inheritance + column properties produce the fallback logic when looking up in the result via the adapted column, which in those tests is actually a Label object that doesn't adapt nicely. Fixes: #6596 Change-Id: I107a47e873ae05ab50853bb00a9ea0e1a88d5aee --- doc/build/changelog/unreleased_14/6596.rst | 10 +++++ lib/sqlalchemy/engine/default.py | 4 ++ lib/sqlalchemy/orm/context.py | 24 ++++++----- lib/sqlalchemy/orm/loading.py | 24 +++++++++++ lib/sqlalchemy/orm/session.py | 14 ++++++- test/orm/test_bind.py | 2 +- test/orm/test_eager_relations.py | 48 ++++++++++++++++++++++ test/orm/test_query.py | 8 +++- 8 files changed, 121 insertions(+), 13 deletions(-) create mode 100644 doc/build/changelog/unreleased_14/6596.rst diff --git a/doc/build/changelog/unreleased_14/6596.rst b/doc/build/changelog/unreleased_14/6596.rst new file mode 100644 index 0000000000..65d307eb8e --- /dev/null +++ b/doc/build/changelog/unreleased_14/6596.rst @@ -0,0 +1,10 @@ +.. change:: + :tags: bug, orm, regression, performance + :tickets: 6596 + + Fixed regression involving how the ORM would resolve a given mapped column + to a result row, where under cases such as joined eager loading, a slightly + more expensive "fallback" could take place to set up this resolution due to + some logic that was removed since 1.3. The issue could also cause + deprecation warnings involving column resolution to be emitted when using a + 1.4 style query with joined eager loading. diff --git a/lib/sqlalchemy/engine/default.py b/lib/sqlalchemy/engine/default.py index aa7e4e5e9d..7b3fa091fd 100644 --- a/lib/sqlalchemy/engine/default.py +++ b/lib/sqlalchemy/engine/default.py @@ -1418,6 +1418,10 @@ class DefaultExecutionContext(interfaces.ExecutionContext): strategy = _cursor._NO_CURSOR_DQL if self._is_future_result: + if self.root_connection.should_close_with_result: + raise exc.InvalidRequestError( + "can't use future_result=True with close_with_result" + ) result = _cursor.CursorResult( self, strategy, cursor_description ) diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py index d60758ffcd..e4448f9536 100644 --- a/lib/sqlalchemy/orm/context.py +++ b/lib/sqlalchemy/orm/context.py @@ -127,8 +127,8 @@ class QueryContext(object): ) -_result_disable_adapt_to_context = util.immutabledict( - {"_result_disable_adapt_to_context": True} +_orm_load_exec_options = util.immutabledict( + {"_result_disable_adapt_to_context": True, "future_result": True} ) @@ -239,16 +239,20 @@ class ORMCompileState(CompileState): statement._execution_options, ) - # add _result_disable_adapt_to_context=True to execution options. - # this will disable the ResultSetMetadata._adapt_to_context() - # step which we don't need, as we have result processors cached - # against the original SELECT statement before caching. + # default execution options for ORM results: + # 1. _result_disable_adapt_to_context=True + # this will disable the ResultSetMetadata._adapt_to_context() + # step which we don't need, as we have result processors cached + # against the original SELECT statement before caching. + # 2. future_result=True. The ORM should **never** resolve columns + # in a result set based on names, only on Column objects that + # are correctly adapted to the context. W the legacy result + # it will still attempt name-based resolution and also emit a + # warning. if not execution_options: - execution_options = _result_disable_adapt_to_context + execution_options = _orm_load_exec_options else: - execution_options = execution_options.union( - _result_disable_adapt_to_context - ) + execution_options = execution_options.union(_orm_load_exec_options) if "yield_per" in execution_options or load_options._yield_per: execution_options = execution_options.union( diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py index cd2ec8301f..b063635ea5 100644 --- a/lib/sqlalchemy/orm/loading.py +++ b/lib/sqlalchemy/orm/loading.py @@ -741,6 +741,30 @@ def _instance_processor( ) else: getter = None + if adapter: + # this logic had been removed for all 1.4 releases + # up until 1.4.18; the adapter here is particularly + # the compound eager adapter which isn't accommodated + # in the quick_populators right now. The "fallback" + # logic below instead took over in many more cases + # until issue #6596 was identified. + + # note there is still an issue where this codepath + # produces no "getter" for cases where a joined-inh + # mapping includes a labeled column property, meaning + # KeyError is caught internally and we fall back to + # _getter(col), which works anyway. The adapter + # here for joined inh without any aliasing might not + # be useful. Tests which see this include + # test.orm.inheritance.test_basic -> + # EagerTargetingTest.test_adapt_stringency + # OptimizedLoadTest.test_column_expression_joined + # PolymorphicOnNotLocalTest.test_polymorphic_on_column_prop # noqa E501 + # + + adapted_col = adapter.columns[col] + if adapted_col is not None: + getter = result._getter(adapted_col, False) if not getter: getter = result._getter(col, False) if getter: diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py index ec11e7adf3..81da15283b 100644 --- a/lib/sqlalchemy/orm/session.py +++ b/lib/sqlalchemy/orm/session.py @@ -1491,6 +1491,9 @@ class Session(_SessionClassMethods): configured with ``autocommit=True`` and does not already have a transaction in progress. + .. deprecated:: 1.4 this parameter is deprecated and will be removed + in SQLAlchemy 2.0 + :param execution_options: a dictionary of execution options that will be passed to :meth:`_engine.Connection.execution_options`, **when the connection is first procured only**. If the connection is already @@ -1673,7 +1676,16 @@ class Session(_SessionClassMethods): bind = self.get_bind(**bind_arguments) - conn = self._connection_for_bind(bind, close_with_result=True) + if self.autocommit: + # legacy stuff, we can't use future_result w/ autocommit because + # we rely upon close_with_result, also legacy. it's all + # interrelated + conn = self._connection_for_bind(bind, close_with_result=True) + execution_options = execution_options.union( + dict(future_result=False) + ) + else: + conn = self._connection_for_bind(bind) result = conn._execute_20(statement, params or {}, execution_options) if compile_state_cls: diff --git a/test/orm/test_bind.py b/test/orm/test_bind.py index 42ba6e9be4..f448e2c529 100644 --- a/test/orm/test_bind.py +++ b/test/orm/test_bind.py @@ -377,7 +377,7 @@ class BindIntegrationTest(_fixtures.FixtureTest): canary.mock_calls, [ mock.call.get_bind(**expected_get_bind_args), - mock.call._connection_for_bind(engine, close_with_result=True), + mock.call._connection_for_bind(engine), ], ) sess.close() diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 4e11f98637..9ee209e98f 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -1,6 +1,7 @@ """tests of joined-eager loaded attributes""" import datetime +import operator import sqlalchemy as sa from sqlalchemy import and_ @@ -37,6 +38,7 @@ from sqlalchemy.testing import fixtures from sqlalchemy.testing import in_ from sqlalchemy.testing import is_ from sqlalchemy.testing import is_not +from sqlalchemy.testing import mock from sqlalchemy.testing.assertsql import CompiledSQL from sqlalchemy.testing.fixtures import fixture_session from sqlalchemy.testing.schema import Column @@ -556,6 +558,52 @@ class EagerTest(_fixtures.FixtureTest, testing.AssertsCompiledSQL): eq_(q.first(), (User(id=7), 1)) + def test_we_adapt_for_compound_for_getter(self): + """test #6596. + + Ensure loading.py uses the compound eager adapter on the target + column before looking for a populator, rather than creating + a new populator. + + """ + + User, Address = self.classes("User", "Address") + users, addresses = self.tables("users", "addresses") + mapper( + User, + users, + properties={ + "addresses": relationship(Address, order_by=addresses.c.id) + }, + ) + mapper(Address, addresses) + + s = fixture_session() + + q = ( + select(User) + .options(joinedload(User.addresses)) + .order_by(User.id) + .limit(2) + ) + + def strict_getter(self, key, raiseerr=True): + try: + rec = self._keymap[key] + except KeyError: + assert False + + index = rec[0] + + return operator.itemgetter(index) + + with mock.patch( + "sqlalchemy.engine.result.ResultMetaData._getter", strict_getter + ): + result = s.execute(q).unique().scalars().all() + + eq_(result, self.static.user_address_result[0:2]) + def test_options_pathing(self): ( users, diff --git a/test/orm/test_query.py b/test/orm/test_query.py index 7f2a0f7154..77535a9bbb 100644 --- a/test/orm/test_query.py +++ b/test/orm/test_query.py @@ -4930,7 +4930,12 @@ class YieldTest(_fixtures.FixtureTest): for k, v in ctx.execution_options.items() if not k.startswith("_") }, - {"max_row_buffer": 15, "stream_results": True, "foo": "bar"}, + { + "max_row_buffer": 15, + "stream_results": True, + "foo": "bar", + "future_result": True, + }, ) q = sess.query(User).yield_per(15) @@ -4958,6 +4963,7 @@ class YieldTest(_fixtures.FixtureTest): "max_row_buffer": 15, "stream_results": True, "yield_per": 15, + "future_result": True, }, ) -- 2.47.2