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

3 files changed, 51 insertions, 11 deletions

diff --git a/lib/sqlalchemy/orm/context.py b/lib/sqlalchemy/orm/context.py
index d60758ffc..e4448f953 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 cd2ec8301..b063635ea 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 ec11e7adf..81da15283 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: