Commits

Mike Bayer  committed 79b0b8c

Added a new exception to detect the case where two
subclasses are being loaded using with_polymorphic(),
each subclass contains a relationship attribute of the same
name, and eager loading is being applied to one or both.
This is an ongoing bug which can't be immediately fixed,
so since the results are usually wrong in any case it raises an
exception for now. 0.7 has the same issue, so an exception
raise here probably means the code was returning the wrong
data in 0.7. [ticket:2614]

  • Participants
  • Parent commits a55d5f3

Comments (0)

Files changed (5)

File doc/build/changelog/changelog_08.rst

     :version: 0.8.0b2
 
     .. change::
+        :tags: orm, bug
+        :ticket: 2614
+
+      Added a new exception to detect the case where two
+      subclasses are being loaded using with_polymorphic(),
+      each subclass contains a relationship attribute of the same
+      name, and eager loading is being applied to one or both.
+      This is an ongoing bug which can't be immediately fixed,
+      so since the results are usually wrong in any case it raises an
+      exception for now.   0.7 has the same issue, so an exception
+      raise here probably means the code was returning the wrong
+      data in 0.7.
+
+    .. change::
         :tags: sql, bug
 
       Fixed a gotcha where inadvertently calling list() on a

File lib/sqlalchemy/orm/strategies.py

         self.uselist = self.parent_property.uselist
 
 
+    def _warn_existing_path(self):
+        raise sa_exc.InvalidRequestError(
+            "Eager loading cannot currently function correctly when two or "
+            "more "
+            "same-named attributes associated with multiple polymorphic "
+            "classes "
+            "of the same base are present.   Encountered more than one "
+            r"eager path for attribute '%s' on mapper '%s'." %
+            (self.key, self.parent.base_mapper, ))
+
+
 class NoLoader(AbstractRelationshipLoader):
     """Provide loading behavior for a :class:`.RelationshipProperty`
     with "lazy=None".
 
         # add new query to attributes to be picked up
         # by create_row_processor
-        path.set(context, "subquery", q)
+        existing = path.replace(context, "subquery", q)
+        if existing:
+            self._warn_existing_path()
 
     def _get_leftmost(self, subq_path):
         subq_path = subq_path.path
         )
 
         add_to_collection = context.secondary_columns
-        path.set(context, "eager_row_processor", clauses)
+        existing = path.replace(context, "eager_row_processor", clauses)
+        if existing:
+            self._warn_existing_path()
         return clauses, adapter, add_to_collection, allow_innerjoin
 
     def _create_eager_join(self, context, entity,

File lib/sqlalchemy/orm/util.py

     def set(self, reg, key, value):
         reg._attributes[(key, self.reduced_path)] = value
 
+    def replace(self, reg, key, value):
+        path_key = (key, self.reduced_path)
+        existing = reg._attributes.get(path_key, None)
+        reg._attributes[path_key] = value
+        return existing
+
     def setdefault(self, reg, key, value):
         reg._attributes.setdefault((key, self.reduced_path), value)
 

File test/orm/test_eager_relations.py

 
         session.close_all()
         d = session.query(Director).options(joinedload('*')).first()
-        assert len(list(session)) == 3
+        assert len(list(session)) == 3
+
+
+
+class WarnFor2614TestBase(object):
+
+    @classmethod
+    def define_tables(cls, metadata):
+        Table('a', metadata,
+            Column('id', Integer, primary_key=True),
+            Column('type', String(50)),
+        )
+        Table('b', metadata,
+            Column('id', Integer, ForeignKey('a.id'), primary_key=True),
+        )
+        Table('c', metadata,
+            Column('id', Integer, ForeignKey('a.id'), primary_key=True),
+        )
+        Table('d', metadata,
+            Column('id', Integer, primary_key=True),
+            Column('bid', Integer, ForeignKey('b.id')),
+            Column('cid', Integer, ForeignKey('c.id')),
+        )
+
+    def _mapping(self, lazy_b=True, lazy_c=True):
+        class A(object):
+            pass
+
+        class B(A):
+            pass
+
+        class C(A):
+            pass
+
+        class D(object):
+            pass
+
+        mapper(A, self.tables.a, polymorphic_on=self.tables.a.c.type)
+        mapper(B, self.tables.b, inherits=A, polymorphic_identity='b',
+                    properties={
+                        'ds': relationship(D, lazy=lazy_b)
+                    })
+        mapper(C, self.tables.c, inherits=A, polymorphic_identity='c',
+                    properties={
+                        'ds': relationship(D, lazy=lazy_c)
+                    })
+        mapper(D, self.tables.d)
+
+
+        return  A, B, C, D
+
+    def _assert_raises(self, fn):
+        assert_raises_message(
+            sa.exc.InvalidRequestError,
+            "Eager loading cannot currently function correctly when two or more "
+            r"same\-named attributes associated with multiple polymorphic classes "
+            "of the same base are present.   Encountered more than one "
+            r"eager path for attribute 'ds' on mapper 'Mapper\|A\|a'.",
+            fn
+        )
+
+    def test_poly_both_eager(self):
+        A, B, C, D = self._mapping(lazy_b=self.eager_name,
+                                    lazy_c=self.eager_name)
+
+        session = Session()
+        self._assert_raises(
+            session.query(A).with_polymorphic('*').all
+        )
+
+    def test_poly_one_eager(self):
+        A, B, C, D = self._mapping(lazy_b=self.eager_name, lazy_c=True)
+
+        session = Session()
+        session.query(A).with_polymorphic('*').all()
+
+    def test_poly_both_option(self):
+        A, B, C, D = self._mapping()
+
+        session = Session()
+        self._assert_raises(
+            session.query(A).with_polymorphic('*').options(
+                        self.eager_option(B.ds), self.eager_option(C.ds)).all
+        )
+
+    def test_poly_one_option_bs(self):
+        A, B, C, D = self._mapping()
+
+        session = Session()
+
+        # sucks, can't even do eager() on just one of them, as B.ds
+        # hits for both
+        self._assert_raises(
+            session.query(A).with_polymorphic('*').\
+                options(self.eager_option(B.ds)).all
+        )
+
+    def test_poly_one_option_cs(self):
+        A, B, C, D = self._mapping()
+
+        session = Session()
+
+        # sucks, can't even do eager() on just one of them, as B.ds
+        # hits for both
+        self._assert_raises(
+            session.query(A).with_polymorphic('*').\
+                options(self.eager_option(C.ds)).all
+        )
+
+    def test_single_poly_one_option_bs(self):
+        A, B, C, D = self._mapping()
+
+        session = Session()
+
+        session.query(A).with_polymorphic(B).\
+            options(self.eager_option(B.ds)).all()
+
+    def test_lazy_True(self):
+        A, B, C, D = self._mapping()
+
+        session = Session()
+        session.query(A).with_polymorphic('*').all()
+
+class WarnFor2614Test(WarnFor2614TestBase, fixtures.MappedTest):
+    eager_name = "joined"
+
+    def eager_option(self, arg):
+        return joinedload(arg)

File test/orm/test_subquery_relations.py

 
         session.close_all()
         d = session.query(Director).options(subqueryload('*')).first()
-        assert len(list(session)) == 3
+        assert len(list(session)) == 3
+
+from . import test_eager_relations
+
+class WarnFor2614Test(test_eager_relations.WarnFor2614TestBase, fixtures.MappedTest):
+    eager_name = "subquery"
+
+    def eager_option(self, arg):
+        return subqueryload(arg)