Cancel polymorphic loading in optimized get

Since optimized_get for inheriting mappers writes a simple
SELECT, we need to cancel out any with_polymorphic selectables
that interfere with simple column lookup.  While adaptation is
another option, just removing the with_polymorphic is much
simpler.   The issue is not noticeable unless the ResultProxy
is not allowing "key fallback" column lookups, which will
be the case when this behavior is deprecated.

Fixes: #4718
Change-Id: I8fa2f5c0434b6a681813a92ac71fe12712f5d634

diff --git a/doc/build/changelog/unreleased_14/4718.rst b/doc/build/changelog/unreleased_14/4718.rst
new file mode 100644 (file)
index 0000000..be36efb
--- /dev/null
+++ b/doc/build/changelog/unreleased_14/4718.rst
@@ -0,0 +1,8 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4718
+
+    Fixed issue in polymorphic loading internals which would fall back to a
+    more expensive, soon-to-be-deprecated form of result column lookup within
+    certain unexpiration scenarios in conjunction with the use of
+    "with_polymorphic".

diff --git a/lib/sqlalchemy/orm/loading.py b/lib/sqlalchemy/orm/loading.py
index 94a9b8d2205c04c72f567c325edfc7a0c0c62942..886240591bea161189a563cb45e91572d0ab10fb 100644 (file)
--- a/lib/sqlalchemy/orm/loading.py
+++ b/lib/sqlalchemy/orm/loading.py
@@ -23,6 +23,7 @@ from . import strategy_options
 from .base import _DEFER_FOR_STATE
 from .base import _SET_DEFERRED_EXPIRED
 from .util import _none_set
+from .util import aliased
 from .util import state_str
 from .. import exc as sa_exc
 from .. import util
@@ -633,7 +634,6 @@ def _instance_processor(
     if mapper.polymorphic_map and not _polymorphic_from and not refresh_state:
         # if we are doing polymorphic, dispatch to a different _instance()
         # method specific to the subclass mapper
-
         def ensure_no_pk(row):
             identitykey = (
                 identity_class,
@@ -957,9 +957,10 @@ def load_scalar_attributes(mapper, state, attribute_names):
         # by default
         statement = mapper._optimized_get_statement(state, attribute_names)
         if statement is not None:
+            wp = aliased(mapper, statement)
             result = load_on_ident(
-                session.query(mapper)
-                .options(strategy_options.Load(mapper).undefer("*"))
+                session.query(wp)
+                .options(strategy_options.Load(wp).undefer("*"))
                 .from_statement(statement),
                 None,
                 only_load_props=attribute_names,

diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py
index 3d08dce22c4a419f47e106acbaf1dc021317b37b..0480bfb01aea13418dbea040f6ae5a0367a856bc 100644 (file)
--- a/lib/sqlalchemy/orm/query.py
+++ b/lib/sqlalchemy/orm/query.py
@@ -774,6 +774,11 @@ class Query(Generative):
             )
         entity = self._entities[0]._clone()
         self._entities = [entity] + self._entities[1:]
+
+        # NOTE: we likely should set primary_entity here, however
+        # this hasn't been changed for many years and we'd like to
+        # deprecate this method.
+
         entity.set_with_polymorphic(
             self,
             cls_or_mappers,

diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py
index 1f2f657285e4bb4f21afe0ff9557583714d34ab1..1c7c0546dc603a74c412b4e5a9e803459773287e 100644 (file)
--- a/lib/sqlalchemy/orm/strategies.py
+++ b/lib/sqlalchemy/orm/strategies.py
@@ -183,7 +183,6 @@ class ColumnLoader(LoaderStrategy):
         memoized_populators,
         **kwargs
     ):
-
         for c in self.columns:
             if adapter:
                 c = adapter.columns[c]

diff --git a/lib/sqlalchemy/sql/util.py b/lib/sqlalchemy/sql/util.py
index 3c7f904deaf6e2874462a0e11de1201ff78388eb..780cdc7b287f1e8c1b4c8c716a066a5c9425f56e 100644 (file)
--- a/lib/sqlalchemy/sql/util.py
+++ b/lib/sqlalchemy/sql/util.py
@@ -798,6 +798,8 @@ class ClauseAdapter(visitors.ReplacingCloningVisitor):
                 if newcol is not None:
                     return newcol
         if self.adapt_on_names and newcol is None:
+            # TODO: this should be changed to .exported_columns if and
+            # when we need to be able to adapt a plain Select statement
             newcol = self.selectable.c.get(col.name)
         return newcol
 

diff --git a/test/orm/inheritance/test_basic.py b/test/orm/inheritance/test_basic.py
index 57320d808f862dc9aaf2690bdc957e3cc18c6789..084563972b5ed93a542339697fb96551220d7af3 100644 (file)
--- a/test/orm/inheritance/test_basic.py
+++ b/test/orm/inheritance/test_basic.py
@@ -37,6 +37,7 @@ from sqlalchemy.testing import engines
 from sqlalchemy.testing import eq_
 from sqlalchemy.testing import fixtures
 from sqlalchemy.testing import is_
+from sqlalchemy.testing import mock
 from sqlalchemy.testing.assertsql import AllOf
 from sqlalchemy.testing.assertsql import CompiledSQL
 from sqlalchemy.testing.assertsql import Or
@@ -2484,6 +2485,63 @@ class OptimizedLoadTest(fixtures.MappedTest):
             ),
         )
 
+    def test_optimized_load_subclass_labels(self):
+        # test for issue #4718, however it may be difficult to maintain
+        # the conditions here:
+        # 1. optimized get is used to load some attributes
+        # 2. the subtable-only statement is generated
+        # 3. the mapper (a subclass mapper) is against a with_polymorphic
+        #    that is using a labeled select
+        #
+        # the optimized loader has to cancel out the polymorphic for the
+        # query (or adapt around it) since optimized get creates a simple
+        # SELECT statement.   Otherwise it often relies on _key_fallback
+        # columns in order to do the lookup.
+        #
+        # note this test can't fail when the fix is missing unless
+        # ResultProxy._key_fallback no longer allows a non-matching column
+        # lookup without warning or raising.
+
+        base, sub = self.tables.base, self.tables.sub
+
+        class Base(fixtures.ComparableEntity):
+            pass
+
+        class Sub(Base):
+            pass
+
+        mapper(
+            Base, base, polymorphic_on=base.c.type, polymorphic_identity="base"
+        )
+
+        mapper(
+            Sub,
+            sub,
+            inherits=Base,
+            polymorphic_identity="sub",
+            with_polymorphic=(
+                "*",
+                base.outerjoin(sub).select(use_labels=True).alias("foo"),
+            ),
+        )
+        sess = Session()
+        s1 = Sub(
+            data="s1data", sub="s1sub", subcounter=1, counter=1, subcounter2=1
+        )
+        sess.add(s1)
+        sess.flush()
+        sess.expire(s1, ["sub"])
+
+        def _key_fallback(self, key, raiseerr):
+            raise KeyError(key)
+
+        with mock.patch(
+            "sqlalchemy.engine.result.ResultMetaData._key_fallback",
+            _key_fallback,
+        ):
+
+            eq_(s1.sub, "s1sub")
+
     def test_optimized_passes(self):
         """"test that the 'optimized load' routine doesn't crash when
         a column in the join condition is not available."""