From: Mike Bayer Date: Sat, 24 Nov 2012 21:14:58 +0000 (-0500) Subject: Added a new exception to detect the case where two X-Git-Tag: rel_0_8_0b2~24 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e2697d547ec8c24c9a37a72fc60abe73b7dee81b;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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] --- diff --git a/doc/build/changelog/changelog_08.rst b/doc/build/changelog/changelog_08.rst index 8046a88920..45a612b491 100644 --- a/doc/build/changelog/changelog_08.rst +++ b/doc/build/changelog/changelog_08.rst @@ -6,6 +6,20 @@ .. changelog:: :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 diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 39329f9b11..586ec4b4e9 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -303,6 +303,17 @@ class AbstractRelationshipLoader(LoaderStrategy): 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". @@ -746,7 +757,9 @@ class SubqueryLoader(AbstractRelationshipLoader): # 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 @@ -1091,7 +1104,9 @@ class JoinedLoader(AbstractRelationshipLoader): ) 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, diff --git a/lib/sqlalchemy/orm/util.py b/lib/sqlalchemy/orm/util.py index 97baffc9aa..e5e725138c 100644 --- a/lib/sqlalchemy/orm/util.py +++ b/lib/sqlalchemy/orm/util.py @@ -279,6 +279,12 @@ class PathRegistry(object): 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) diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 39a7a7301d..ae3853e75f 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -2659,4 +2659,131 @@ class CyclicalInheritingEagerTestTwo(fixtures.DeclarativeMappedTest, session.close_all() d = session.query(Director).options(joinedload('*')).first() - assert len(list(session)) == 3 \ No newline at end of file + 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) diff --git a/test/orm/test_subquery_relations.py b/test/orm/test_subquery_relations.py index f6e2299b42..000f4abaf8 100644 --- a/test/orm/test_subquery_relations.py +++ b/test/orm/test_subquery_relations.py @@ -1382,4 +1382,12 @@ class CyclicalInheritingEagerTestTwo(fixtures.DeclarativeMappedTest, session.close_all() d = session.query(Director).options(subqueryload('*')).first() - assert len(list(session)) == 3 \ No newline at end of file + 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)