From: Mike Bayer Date: Wed, 17 Feb 2016 22:54:43 +0000 (-0500) Subject: - Fixed bug which would cause an eagerly loaded many-to-one attribute X-Git-Tag: rel_1_1_0b1~98^2~19 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=594a974ede97941a61c2e657a63cb11a7137c1ae;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - Fixed bug which would cause an eagerly loaded many-to-one attribute to not be loaded, if the joined eager load were from a row where the same entity were present multiple times, some calling for the attribute to be eagerly loaded and others not. The logic here is revised to take in the attribute even though a different loader path has handled the parent entity already. fixes #3431 --- diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst index 37ed3470c9..ccf99ee988 100644 --- a/doc/build/changelog/changelog_11.rst +++ b/doc/build/changelog/changelog_11.rst @@ -21,6 +21,21 @@ .. changelog:: :version: 1.1.0b1 + .. change:: + :tags: bug, orm + :tickets: 3431 + + Fixed bug which would cause an eagerly loaded many-to-one attribute + to not be loaded, if the joined eager load were from a row where the + same entity were present multiple times, some calling for the attribute + to be eagerly loaded and others not. The logic here is revised to + take in the attribute even though a different loader path has + handled the parent entity already. + + .. seealso:: + + :ref:`change_3431` + .. change:: :tags: feature, engine :tickets: 2837 diff --git a/doc/build/changelog/migration_11.rst b/doc/build/changelog/migration_11.rst index 8ed6ad6b56..fb0b72de7b 100644 --- a/doc/build/changelog/migration_11.rst +++ b/doc/build/changelog/migration_11.rst @@ -487,6 +487,95 @@ associated with any bound :class:`.Engine`, then the fallback to the :ticket:`3081` +.. _change_3431: + +Joined eager loading where the same entity is present multiple times in one row +------------------------------------------------------------------------------- + +A fix has been made to the case has been made whereby an attribute will be +loaded via joined eager loading, even if the entity was already loaded from the +row on a different "path" that doesn't include the attribute. This is a +deep use case that's hard to reproduce, but the general idea is as follows:: + + class A(Base): + __tablename__ = 'a' + id = Column(Integer, primary_key=True) + b_id = Column(ForeignKey('b.id')) + c_id = Column(ForeignKey('c.id')) + + b = relationship("B") + c = relationship("C") + + + class B(Base): + __tablename__ = 'b' + id = Column(Integer, primary_key=True) + c_id = Column(ForeignKey('c.id')) + + c = relationship("C") + + + class C(Base): + __tablename__ = 'c' + id = Column(Integer, primary_key=True) + d_id = Column(ForeignKey('d.id')) + d = relationship("D") + + + class D(Base): + __tablename__ = 'd' + id = Column(Integer, primary_key=True) + + + c_alias_1 = aliased(C) + c_alias_2 = aliased(C) + + q = s.query(A) + q = q.join(A.b).join(c_alias_1, B.c).join(c_alias_1.d) + q = q.options(contains_eager(A.b).contains_eager(B.c, alias=c_alias_1).contains_eager(C.d)) + q = q.join(c_alias_2, A.c) + q = q.options(contains_eager(A.c, alias=c_alias_2)) + +The above query emits SQL like this:: + + SELECT + d.id AS d_id, + c_1.id AS c_1_id, c_1.d_id AS c_1_d_id, + b.id AS b_id, b.c_id AS b_c_id, + c_2.id AS c_2_id, c_2.d_id AS c_2_d_id, + a.id AS a_id, a.b_id AS a_b_id, a.c_id AS a_c_id + FROM + a + JOIN b ON b.id = a.b_id + JOIN c AS c_1 ON c_1.id = b.c_id + JOIN d ON d.id = c_1.d_id + JOIN c AS c_2 ON c_2.id = a.c_id + +We can see that the ``c`` table is selected from twice; once in the context +of ``A.b.c -> c_alias_1`` and another in the context of ``A.c -> c_alias_2``. +Also, we can see that it is quite possible that the ``C`` identity for a +single row is the **same** for both ``c_alias_1`` and ``c_alias_2``, meaning +two sets of columns in one row result in only one new object being added +to the identity map. + +The query options above only call for the attribute ``C.d`` to be loaded +in the context of ``c_alias_1``, and not ``c_alias_2``. So whether or not +the final ``C`` object we get in the identity map has the ``C.d`` attribute +loaded depends on how the mappings are traversed, which while not completely +random, is essentially non-deterministic. The fix is that even if the +loader for ``c_alias_1`` is processed after that of ``c_alias_2`` for a +single row where they both refer to the same identity, the ``C.d`` +element will still be loaded. Previously, the loader did not seek to +modify the load of an entity that was already loaded via a different path. +The loader that reaches the entity first has always been non-deterministic, +so this fix may be detectable as a behavioral change in some situations and +not others. + +The fix includes tests for two variants of the "multiple paths to one entity" +case, and the fix should hopefully cover all other scenarios of this nature. + +:ticket:`3431` + .. _change_3641: Columns no longer added redundantly with DISTINCT + ORDER BY diff --git a/lib/sqlalchemy/orm/strategies.py b/lib/sqlalchemy/orm/strategies.py index 7942b14d41..7d816e6261 100644 --- a/lib/sqlalchemy/orm/strategies.py +++ b/lib/sqlalchemy/orm/strategies.py @@ -1573,13 +1573,19 @@ class JoinedLoader(AbstractRelationshipLoader): # call _instance on the row, even though the object has # been created, so that we further descend into properties existing = _instance(row) - if existing is not None \ - and key in dict_ \ - and existing is not dict_[key]: - util.warn( - "Multiple rows returned with " - "uselist=False for eagerly-loaded attribute '%s' " - % self) + if existing is not None: + # conflicting value already loaded, this shouldn't happen + if key in dict_: + if existing is not dict_[key]: + util.warn( + "Multiple rows returned with " + "uselist=False for eagerly-loaded attribute '%s' " + % self) + else: + # this case is when one row has multiple loads of the + # same entity (e.g. via aliasing), one has an attribute + # that the other doesn't. + dict_[key] = existing def load_scalar_from_joined_exec(state, dict_, row): _instance(row) diff --git a/test/orm/test_eager_relations.py b/test/orm/test_eager_relations.py index 1c3b57690a..3ad641b8fc 100644 --- a/test/orm/test_eager_relations.py +++ b/test/orm/test_eager_relations.py @@ -1,11 +1,11 @@ """tests of joined-eager loaded attributes""" -from sqlalchemy.testing import eq_, is_, is_not_ +from sqlalchemy.testing import eq_, is_, is_not_, in_ import sqlalchemy as sa from sqlalchemy import testing from sqlalchemy.orm import joinedload, deferred, undefer, \ joinedload_all, backref, Session,\ - defaultload, Load, load_only + defaultload, Load, load_only, contains_eager from sqlalchemy import Integer, String, Date, ForeignKey, and_, select, \ func, text from sqlalchemy.testing.schema import Table, Column @@ -4178,3 +4178,148 @@ class EnsureColumnsAddedTest( "ON m2mchild_1.id = parent_to_m2m_1.child_id) " "ON parent.arb = parent_to_m2m_1.parent_id" ) + + +class EntityViaMultiplePathTestOne(fixtures.DeclarativeMappedTest): + """test for [ticket:3431]""" + + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class A(Base): + __tablename__ = 'a' + id = Column(Integer, primary_key=True) + b_id = Column(ForeignKey('b.id')) + c_id = Column(ForeignKey('c.id')) + + b = relationship("B") + c = relationship("C") + + class B(Base): + __tablename__ = 'b' + id = Column(Integer, primary_key=True) + c_id = Column(ForeignKey('c.id')) + + c = relationship("C") + + class C(Base): + __tablename__ = 'c' + id = Column(Integer, primary_key=True) + d_id = Column(ForeignKey('d.id')) + d = relationship("D") + + class D(Base): + __tablename__ = 'd' + id = Column(Integer, primary_key=True) + + @classmethod + def define_tables(cls, metadata): + Table( + 'a', metadata, + Column('id', Integer, primary_key=True), + Column('bid', ForeignKey('b.id')) + ) + + def test_multi_path_load(self): + A, B, C, D = self.classes('A', 'B', 'C', 'D') + + s = Session() + + c = C(d=D()) + + s.add( + A(b=B(c=c), c=c) + ) + s.commit() + + c_alias_1 = aliased(C) + c_alias_2 = aliased(C) + + q = s.query(A) + q = q.join(A.b).join(c_alias_1, B.c).join(c_alias_1.d) + q = q.options( + contains_eager(A.b). + contains_eager(B.c, alias=c_alias_1). + contains_eager(C.d)) + q = q.join(c_alias_2, A.c) + q = q.options(contains_eager(A.c, alias=c_alias_2)) + + a1 = q.all()[0] + + # ensure 'd' key was populated in dict. Varies based on + # PYTHONHASHSEED + in_('d', a1.c.__dict__) + + +class EntityViaMultiplePathTestTwo(fixtures.DeclarativeMappedTest): + """test for [ticket:3431]""" + + @classmethod + def setup_classes(cls): + Base = cls.DeclarativeBasic + + class User(Base): + __tablename__ = 'cs_user' + + id = Column(Integer, primary_key=True) + data = Column(Integer) + + class LD(Base): + """Child. The column we reference 'A' with is an integer.""" + + __tablename__ = 'cs_ld' + + id = Column(Integer, primary_key=True) + user_id = Column(Integer, ForeignKey('cs_user.id')) + user = relationship(User, primaryjoin=user_id == User.id) + + class A(Base): + """Child. The column we reference 'A' with is an integer.""" + + __tablename__ = 'cs_a' + + id = Column(Integer, primary_key=True) + ld_id = Column(Integer, ForeignKey('cs_ld.id')) + ld = relationship(LD, primaryjoin=ld_id == LD.id) + + class LDA(Base): + """Child. The column we reference 'A' with is an integer.""" + + __tablename__ = 'cs_lda' + + id = Column(Integer, primary_key=True) + ld_id = Column(Integer, ForeignKey('cs_ld.id')) + a_id = Column(Integer, ForeignKey('cs_a.id')) + a = relationship(A, primaryjoin=a_id == A.id) + ld = relationship(LD, primaryjoin=ld_id == LD.id) + + def test_multi_path_load(self): + User, LD, A, LDA = self.classes('User', 'LD', 'A', 'LDA') + + s = Session() + + u0 = User(data=42) + l0 = LD(user=u0) + z0 = A(ld=l0) + lz0 = LDA(ld=l0, a=z0) + s.add_all([ + u0, l0, z0, lz0 + ]) + s.commit() + + l_ac = aliased(LD) + u_ac = aliased(User) + + lz_test = (s.query(LDA) + .join('ld') + .options(contains_eager('ld')) + .join('a', (l_ac, 'ld'), (u_ac, 'user')) + .options(contains_eager('a') + .contains_eager('ld', alias=l_ac) + .contains_eager('user', alias=u_ac)) + .first()) + + in_( + 'user', lz_test.a.ld.__dict__ + )