From 94a089bc2b425f7896659868630836c013e80dd0 Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Fri, 12 May 2017 12:01:53 -0400 Subject: [PATCH] Demote innerjoin to outerjoin coming from with_polymorphic a with_polymorphic, regardless of inheritance type, represents multiple classes. A subclass that wants to joinedload with innerjoin=True needs to be demoted to an outerjoin because the parent entity rows might not be of that type. Looks more intuitive with a joined inheritance load, but applies just as well to single or concrete. Change-Id: I4d3d76106ae20032269f8848aad70a8e2f9422f9 Fixes: #3988 --- doc/build/changelog/changelog_12.rst | 11 +++++ lib/sqlalchemy/orm/mapper.py | 2 + lib/sqlalchemy/orm/query.py | 3 +- lib/sqlalchemy/orm/strategies.py | 8 +++- lib/sqlalchemy/orm/util.py | 19 ++++++--- test/orm/inheritance/test_relationship.py | 52 +++++++++++++++++++++++ 6 files changed, 86 insertions(+), 9 deletions(-) diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst index 24c525f850..3d0b7fa088 100644 --- a/doc/build/changelog/changelog_12.rst +++ b/doc/build/changelog/changelog_12.rst @@ -13,6 +13,17 @@ .. changelog:: :version: 1.2.0b1 + .. change:: 3988 + :tags: bug, orm + :tickets: 3988 + + Fixed bug where combining a "with_polymorphic" load in conjunction + with subclass-linked relationships that specify joinedload with + innerjoin=True, would fail to demote those "innerjoins" to + "outerjoins" to suit the other polymorphic classes that don't + support that relationship. This applies to both a single and a + joined inheritance polymorphic load. + .. change:: 3984 :tags: bug, orm :tickets: 3984 diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 3fdba44825..25aef3af79 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -687,6 +687,8 @@ class Mapper(InspectionAttr): is_mapper = True """Part of the inspection API.""" + represents_outer_join = False + @property def mapper(self): """Part of the inspection API. diff --git a/lib/sqlalchemy/orm/query.py b/lib/sqlalchemy/orm/query.py index a2f83818c8..f1734194a4 100644 --- a/lib/sqlalchemy/orm/query.py +++ b/lib/sqlalchemy/orm/query.py @@ -3729,7 +3729,8 @@ class _MapperEntity(_QueryEntity): self.path, adapter, context.primary_columns, with_polymorphic=self._with_polymorphic, only_load_props=query._only_load_props, - polymorphic_discriminator=self._polymorphic_discriminator) + polymorphic_discriminator=self._polymorphic_discriminator + ) def __str__(self): return str(self.mapper) diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 00aabed0bb..05bb55d58d 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1502,7 +1502,9 @@ class JoinedLoader(AbstractRelationshipLoader): attach_on_outside = ( not chained_from_outerjoin or - not innerjoin or innerjoin == 'unnested') + not innerjoin or innerjoin == 'unnested' or + entity.entity_zero.represents_outer_join + ) if attach_on_outside: # this is the "classic" eager join case. @@ -1510,7 +1512,9 @@ class JoinedLoader(AbstractRelationshipLoader): towrap, clauses.aliased_class, onclause, - isouter=not innerjoin or ( + isouter=not innerjoin or + entity.entity_zero.represents_outer_join or + ( chained_from_outerjoin and isinstance(towrap, sql.Join) ), _left_memo=self.parent, _right_memo=self.mapper ) diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 5bcb28f319..eebe188378 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -376,7 +376,8 @@ class AliasedClass(object): with_polymorphic_mappers=(), with_polymorphic_discriminator=None, base_alias=None, - use_mapper_path=False): + use_mapper_path=False, + represents_outer_join=False): mapper = _class_to_mapper(cls) if alias is None: alias = mapper._with_polymorphic_selectable.alias( @@ -395,7 +396,8 @@ class AliasedClass(object): else mapper.polymorphic_on, base_alias, use_mapper_path, - adapt_on_names + adapt_on_names, + represents_outer_join ) self.__name__ = 'AliasedClass_%s' % mapper.class_.__name__ @@ -478,7 +480,8 @@ class AliasedInsp(InspectionAttr): def __init__(self, entity, mapper, selectable, name, with_polymorphic_mappers, polymorphic_on, - _base_alias, _use_mapper_path, adapt_on_names): + _base_alias, _use_mapper_path, adapt_on_names, + represents_outer_join): self.entity = entity self.mapper = mapper self.selectable = selectable @@ -487,6 +490,7 @@ class AliasedInsp(InspectionAttr): self.polymorphic_on = polymorphic_on self._base_alias = _base_alias or self self._use_mapper_path = _use_mapper_path + self.represents_outer_join = represents_outer_join self._adapter = sql_util.ColumnAdapter( selectable, equivalents=mapper._equivalent_columns, @@ -530,7 +534,8 @@ class AliasedInsp(InspectionAttr): 'with_polymorphic_discriminator': self.polymorphic_on, 'base_alias': self._base_alias, - 'use_mapper_path': self._use_mapper_path + 'use_mapper_path': self._use_mapper_path, + 'represents_outer_join': self.represents_outer_join } def __setstate__(self, state): @@ -543,7 +548,8 @@ class AliasedInsp(InspectionAttr): state['with_polymorphic_discriminator'], state['base_alias'], state['use_mapper_path'], - state['adapt_on_names'] + state['adapt_on_names'], + state['represents_outer_join'] ) def _adapt_element(self, elem): @@ -772,7 +778,8 @@ def with_polymorphic(base, classes, selectable=False, selectable, with_polymorphic_mappers=mappers, with_polymorphic_discriminator=polymorphic_on, - use_mapper_path=_use_mapper_path) + use_mapper_path=_use_mapper_path, + represents_outer_join=not innerjoin) def _orm_annotate(element, exclude=None): diff --git a/test/orm/inheritance/test_relationship.py b/test/orm/inheritance/test_relationship.py index 74956930cb..246cf214b8 100644 --- a/test/orm/inheritance/test_relationship.py +++ b/test/orm/inheritance/test_relationship.py @@ -1662,6 +1662,58 @@ class JoinedloadOverWPolyAliased( "LEFT OUTER JOIN link AS link_2 ON parent_1.id = link_2.parent_id" ) + def test_local_wpoly_innerjoins(self): + # test for issue #3988 + Sub1 = self._fixture_from_subclass() + Parent = self.classes.Parent + Link = self.classes.Link + + poly = with_polymorphic(Parent, [Sub1]) + + session = Session() + q = session.query(poly).options( + joinedload(poly.Sub1.links, innerjoin=True). + joinedload(Link.child.of_type(Sub1), innerjoin=True). + joinedload(poly.Sub1.links, innerjoin=True) + ) + self.assert_compile( + q, + "SELECT parent.id AS parent_id, parent.type AS parent_type, " + "link_1.parent_id AS link_1_parent_id, " + "link_1.child_id AS link_1_child_id, " + "parent_1.id AS parent_1_id, parent_1.type AS parent_1_type, " + "link_2.parent_id AS link_2_parent_id, " + "link_2.child_id AS link_2_child_id FROM parent " + "LEFT OUTER JOIN link AS link_1 ON parent.id = link_1.parent_id " + "LEFT OUTER JOIN parent AS parent_1 " + "ON link_1.child_id = parent_1.id " + "LEFT OUTER JOIN link AS link_2 ON parent_1.id = link_2.parent_id" + ) + + def test_local_wpoly_innerjoins_roundtrip(self): + # test for issue #3988 + Sub1 = self._fixture_from_subclass() + Parent = self.classes.Parent + Link = self.classes.Link + + session = Session() + session.add_all([ + Parent(), + Parent() + ]) + + # represents "Parent" and "Sub1" rows + poly = with_polymorphic(Parent, [Sub1]) + + # innerjoin for Sub1 only, but this needs + # to be cancelled because the Parent rows + # would be omitted + q = session.query(poly).options( + joinedload(poly.Sub1.links, innerjoin=True). + joinedload(Link.child.of_type(Sub1), innerjoin=True) + ) + eq_(len(q.all()), 2) + class JoinAcrossJoinedInhMultiPath(fixtures.DeclarativeMappedTest, testing.AssertsCompiledSQL): -- 2.39.5