Merge "restore adapter logic in ORM loading"

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.

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
                 )

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(

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:

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:

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()

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,

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,
                 },
             )