From: Mike Bayer Date: Tue, 1 Oct 2019 01:26:08 +0000 (-0400) Subject: Cancel polymorphic loading in optimized get X-Git-Tag: rel_1_4_0b1~711 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=9a2a0f324c13b5a1b334a3982a766cb9f21f428e;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 index 0000000000..be36efbcd2 --- /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 94a9b8d220..886240591b 100644 --- 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 3d08dce22c..0480bfb01a 100644 --- 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 1f2f657285..1c7c0546dc 100644 --- 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 3c7f904dea..780cdc7b28 100644 --- 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 57320d808f..084563972b 100644 --- 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."""