From: Mike Bayer Date: Mon, 13 Jan 2020 18:50:38 +0000 (-0500) Subject: Adjust use_mapper_path rule for poly subclasses X-Git-Tag: rel_1_3_13~3^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=172de113f88a8129393f2efdc81452bf65af65cc;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Adjust use_mapper_path rule for poly subclasses We must change the approach from 2734439 as the information loss is breaking subquery eager loading. Move the adjustment into a deeper set of logic inside of path_regsitry. We can distinguish between a path that will "naturally" build from an aliased entity at the base, vs. one that will "naturally" build on all raw mappers, based on if when we observe that we are being given a with_polymorphic(), if the existing parent path is already in progress or not. In general, we prefer paths to have as much of the original information as possible, and the "natural path" is supposed to be where the loader lookup stuff happens. Fixes: #5082 Change-Id: I3c0ee72993bae8a6f067bdef3dc9a57d83f64950 (cherry picked from commit 7cc2de880b0de2b127a54910761f357f3753c689) --- diff --git a/lib/sqlalchemy/orm/path_registry.py b/lib/sqlalchemy/orm/path_registry.py index 1215511a1d..cd6815717a 100644 --- a/lib/sqlalchemy/orm/path_registry.py +++ b/lib/sqlalchemy/orm/path_registry.py @@ -235,27 +235,63 @@ class TokenRegistry(PathRegistry): class PropRegistry(PathRegistry): + is_unnatural = False + def __init__(self, parent, prop): # restate this path in terms of the # given MapperProperty's parent. insp = inspection.inspect(parent[-1]) + natural_parent = parent if not insp.is_aliased_class or insp._use_mapper_path: - parent = parent.parent[prop.parent] + parent = natural_parent = parent.parent[prop.parent] elif insp.is_aliased_class and insp.with_polymorphic_mappers: if ( prop.parent is not insp.mapper and prop.parent in insp.with_polymorphic_mappers ): subclass_entity = parent[-1]._entity_for_mapper(prop.parent) - if subclass_entity._use_mapper_path: - parent = parent.parent[subclass_entity.mapper] + parent = parent.parent[subclass_entity] + + # when building a path where with_polymorphic() is in use, + # special logic to determine the "natural path" when subclass + # entities are used. + # + # here we are trying to distinguish between a path that starts + # on a the with_polymorhpic entity vs. one that starts on a + # normal entity that introduces a with_polymorphic() in the + # middle using of_type(): + # + # # as in test_polymorphic_rel-> + # # test_subqueryload_on_subclass_uses_path_correctly + # wp = with_polymorphic(RegularEntity, "*") + # sess.query(wp).options(someload(wp.SomeSubEntity.foos)) + # + # vs + # + # # as in test_relationship->JoinedloadWPolyOfTypeContinued + # wp = with_polymorphic(SomeFoo, "*") + # sess.query(RegularEntity).options( + # someload(RegularEntity.foos.of_type(wp)) + # .someload(wp.SubFoo.bar) + # ) + # + # in the former case, the Query as it generates a path that we + # want to match will be in terms of the with_polymorphic at the + # beginning. in the latter case, Query will generate simple + # paths that don't know about this with_polymorphic, so we must + # use a separate natural path. + # + # + if parent.parent: + natural_parent = parent.parent[subclass_entity.mapper] + self.is_unnatural = True else: - parent = parent.parent[subclass_entity] + natural_parent = parent self.prop = prop self.parent = parent self.path = parent.path + (prop,) - self.natural_path = parent.natural_path + (prop,) + self.natural_path = natural_parent.natural_path + (prop,) self._wildcard_path_loader_key = ( "loader", @@ -311,7 +347,11 @@ class AbstractEntityRegistry(PathRegistry): # "enhanced" path in self.path and the "natural" path that doesn't # include those objects so these two traversals can be matched up. - if parent.path and self.is_aliased_class: + # the test here for "(self.is_aliased_class or parent.is_unnatural)" + # are to avoid the more expensive conditional logic that follows if we + # know we don't have to do it. This conditional can just as well be + # "if parent.path:", it just is more function calls. + if parent.path and (self.is_aliased_class or parent.is_unnatural): # this is an infrequent code path used only for loader strategies # that also make use of of_type(). if entity.mapper.isa(parent.natural_path[-1].entity): diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index ed5d113c8f..08af18d18b 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -603,7 +603,7 @@ class AliasedInsp(InspectionAttr): selectable, base_alias=self, adapt_on_names=adapt_on_names, - use_mapper_path=True, + use_mapper_path=_use_mapper_path, ) setattr(self.entity, poly.class_.__name__, ent) diff --git a/test/orm/inheritance/_poly_fixtures.py b/test/orm/inheritance/_poly_fixtures.py index cc253a3806..08898d7d9c 100644 --- a/test/orm/inheritance/_poly_fixtures.py +++ b/test/orm/inheritance/_poly_fixtures.py @@ -1,6 +1,7 @@ from sqlalchemy import ForeignKey from sqlalchemy import Integer from sqlalchemy import String +from sqlalchemy import util from sqlalchemy.orm import create_session from sqlalchemy.orm import mapper from sqlalchemy.orm import polymorphic_union @@ -315,9 +316,10 @@ class _PolymorphicFixtureBase(fixtures.MappedTest, AssertsCompiledSQL): mapper(Machine, machines) - person_with_polymorphic, manager_with_polymorphic = ( - cls._get_polymorphics() - ) + ( + person_with_polymorphic, + manager_with_polymorphic, + ) = cls._get_polymorphics() mapper( Person, @@ -385,10 +387,12 @@ class _PolymorphicUnions(_PolymorphicFixtureBase): cls.tables.boss, ) person_join = polymorphic_union( - { - "engineer": people.join(engineers), - "manager": people.join(managers), - }, + util.OrderedDict( + [ + ("engineer", people.join(engineers)), + ("manager", people.join(managers)), + ] + ), None, "pjoin", ) diff --git a/test/orm/inheritance/test_polymorphic_rel.py b/test/orm/inheritance/test_polymorphic_rel.py index c16573b237..5003c61dce 100644 --- a/test/orm/inheritance/test_polymorphic_rel.py +++ b/test/orm/inheritance/test_polymorphic_rel.py @@ -11,6 +11,7 @@ from sqlalchemy.orm import subqueryload from sqlalchemy.orm import with_polymorphic from sqlalchemy.testing import assert_raises from sqlalchemy.testing import eq_ +from sqlalchemy.testing.assertsql import CompiledSQL from ._poly_fixtures import _Polymorphic from ._poly_fixtures import _PolymorphicAliasedJoins from ._poly_fixtures import _PolymorphicJoins @@ -896,7 +897,6 @@ class _PolymorphicTestBase(object): ] def go(): - # test load People with subqueryload to engineers + machines eq_( sess.query(Person) .with_polymorphic("*") @@ -1861,7 +1861,83 @@ class PolymorphicPolymorphicTest( class PolymorphicUnionsTest(_PolymorphicTestBase, _PolymorphicUnions): - pass + def test_subqueryload_on_subclass_uses_path_correctly(self): + sess = create_session() + expected = [ + Engineer( + name="dilbert", + engineer_name="dilbert", + primary_language="java", + status="regular engineer", + machines=[ + Machine(name="IBM ThinkPad"), + Machine(name="IPhone"), + ], + ) + ] + + with self.sql_execution_asserter(testing.db) as asserter: + wp = with_polymorphic(Person, "*") + eq_( + sess.query(wp) + .options(subqueryload(wp.Engineer.machines)) + .filter(wp.name == "dilbert") + .all(), + expected, + ) + + asserter.assert_( + CompiledSQL( + "SELECT pjoin.person_id AS pjoin_person_id, " + "pjoin.company_id AS pjoin_company_id, " + "pjoin.name AS pjoin_name, pjoin.type AS pjoin_type, " + "pjoin.status AS pjoin_status, " + "pjoin.engineer_name AS pjoin_engineer_name, " + "pjoin.primary_language AS pjoin_primary_language, " + "pjoin.manager_name AS pjoin_manager_name " + "FROM (SELECT engineers.person_id AS person_id, " + "people.company_id AS company_id, people.name AS name, " + "people.type AS type, engineers.status AS status, " + "engineers.engineer_name AS engineer_name, " + "engineers.primary_language AS primary_language, " + "CAST(NULL AS VARCHAR(50)) AS manager_name " + "FROM people JOIN engineers ON people.person_id = " + "engineers.person_id UNION ALL SELECT managers.person_id " + "AS person_id, people.company_id AS company_id, people.name " + "AS name, people.type AS type, managers.status AS status, " + "CAST(NULL AS VARCHAR(50)) AS engineer_name, " + "CAST(NULL AS VARCHAR(50)) AS primary_language, " + "managers.manager_name AS manager_name FROM people " + "JOIN managers ON people.person_id = managers.person_id) " + "AS pjoin WHERE pjoin.name = :name_1", + params=[{"name_1": "dilbert"}], + ), + CompiledSQL( + "SELECT machines.machine_id AS machines_machine_id, " + "machines.name AS machines_name, machines.engineer_id " + "AS machines_engineer_id, anon_1.pjoin_person_id AS " + "anon_1_pjoin_person_id FROM " + "(SELECT pjoin.person_id AS pjoin_person_id FROM " + "(SELECT engineers.person_id AS person_id, people.company_id " + "AS company_id, people.name AS name, " + "people.type AS type, engineers.status AS status, " + "engineers.engineer_name AS engineer_name, " + "engineers.primary_language AS primary_language, " + "CAST(NULL AS VARCHAR(50)) AS manager_name FROM people " + "JOIN engineers ON people.person_id = engineers.person_id " + "UNION ALL SELECT managers.person_id AS person_id, " + "people.company_id AS company_id, people.name AS name, " + "people.type AS type, managers.status AS status, " + "CAST(NULL AS VARCHAR(50)) AS engineer_name, " + "CAST(NULL AS VARCHAR(50)) AS primary_language, " + "managers.manager_name AS manager_name FROM people " + "JOIN managers ON people.person_id = managers.person_id) " + "AS pjoin WHERE pjoin.name = :name_1) AS anon_1 JOIN " + "machines ON anon_1.pjoin_person_id = machines.engineer_id " + "ORDER BY anon_1.pjoin_person_id, machines.machine_id", + params=[{"name_1": "dilbert"}], + ), + ) class PolymorphicAliasedJoinsTest( diff --git a/test/orm/test_utils.py b/test/orm/test_utils.py index 2b4eadaf0e..c274c05410 100644 --- a/test/orm/test_utils.py +++ b/test/orm/test_utils.py @@ -913,24 +913,60 @@ class PathRegistryInhTest(_poly_fixtures._Polymorphic): ) def test_with_poly_sub(self): + Company = _poly_fixtures.Company Person = _poly_fixtures.Person Engineer = _poly_fixtures.Engineer emapper = inspect(Engineer) + cmapper = inspect(Company) p_poly = with_polymorphic(Person, [Engineer]) - e_poly = inspect(p_poly.Engineer) # noqa - used by comment below + e_poly_insp = inspect(p_poly.Engineer) # noqa - used by comment below p_poly_insp = inspect(p_poly) p1 = PathRegistry.coerce((p_poly_insp, emapper.attrs.machines)) - # polymorphic AliasedClass - as of #5082, for the sub entities that are - # generated for each subclass by with_polymorphic(), use_mapper_path - # is not True so that creating paths from the sub entities, which don't - # by themselves encapsulate the with_polymorphic selectable, states the - # path in terms of that plain entity. previously, this path would be - # (e_poly, emapper.attrs.machines), but a loader strategy would never - # match on "e_poly", it would see "emapper". - eq_(p1.path, (emapper, emapper.attrs.machines)) + # changes as of #5082: when a with_polymorphic is in the middle + # of a path, the natural path makes sure it uses the base mappers, + # however when it's at the root, the with_polymorphic stays in + # the natural path + + # this behavior is the same as pre #5082, it was temporarily changed + # but this proved to be incorrect. The path starts on a + # with_polymorphic(), so a Query will "naturally" construct a path + # that comes from that wp. + eq_(p1.path, (e_poly_insp, emapper.attrs.machines)) + eq_(p1.natural_path, (e_poly_insp, emapper.attrs.machines)) + + # this behavior is new as of the final version of #5082. + # the path starts on a normal entity and has a with_polymorphic + # in the middle, for this to match what Query will generate it needs + # to use the non aliased mappers in the natural path. + p2 = PathRegistry.coerce( + ( + cmapper, + cmapper.attrs.employees, + p_poly_insp, + emapper.attrs.machines, + ) + ) + eq_( + p2.path, + ( + cmapper, + cmapper.attrs.employees, + e_poly_insp, + emapper.attrs.machines, + ), + ) + eq_( + p2.natural_path, + ( + cmapper, + cmapper.attrs.employees, + emapper, + emapper.attrs.machines, + ), + ) def test_with_poly_base(self): Person = _poly_fixtures.Person