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 f888a51298..e52af90b91 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, }, )