Skip to content

simple many-to-one comparison is sensitive to order of clauses #3788

Closed
@sqlalchemy-bot

Description

@sqlalchemy-bot
Collaborator

Migrated issue, originally created by Michael Bayer (@zzzeek)

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base
Base = declarative_base()


class A(Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)

    assoc = relationship("AB")


class B(Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)


class AB(Base):
    __tablename__ = 'ab'

    a_id = Column(ForeignKey('a.id'), primary_key=True)
    b_id = Column(ForeignKey('b.id'), primary_key=True)

    a = relationship("A")
    b = relationship("B")


bug = True


class C(Base):
    __tablename__ = 'c'

    id = Column(Integer, primary_key=True)

    if bug:
        k2 = Column(Integer)
        k1 = Column(Integer)
    else:
        k1 = Column(Integer)
        k2 = Column(Integer)

    assoc = relationship("AB")

    __table_args__ = (
        ForeignKeyConstraint(['k1', 'k2'], ['ab.a_id', 'ab.b_id']), {})

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

s = Session(e)
a, b = A(id=100), B(id=1000)

ab = AB(a=a, b=b)

c1 = C(id=1, assoc=ab)
c2 = C(id=2, assoc=ab)
s.add_all([a, b, ab, c1, c2])
s.commit()
s.close()  # clears everything

c1 = s.query(C).get(1)

print "#1 EMITS LAZYLOAD:"
assoc = c1.assoc  # note we keep a strong reference here

c2 = s.query(C).get(2)

print "SHOULD NOT EMIT LAZYLOAD"

assert c2.assoc is assoc


what happens here is mapper.get_clause() is determined as an and() using the order of columns in primary key only. The relationship join is based on Join._join_condition which iterates through columns in the source selectable, so the clauses are reversed.

the fix would be in the comparison:

diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index 75d5368..cff5737 100644
--- a/lib/sqlalchemy/sql/elements.py
+++ b/lib/sqlalchemy/sql/elements.py
@@ -1828,12 +1828,23 @@ class ClauseList(ClauseElement):
         if not isinstance(other, ClauseList) and len(self.clauses) == 1:
             return self.clauses[0].compare(other, **kw)
         elif isinstance(other, ClauseList) and \
-                len(self.clauses) == len(other.clauses):
-            for i in range(0, len(self.clauses)):
-                if not self.clauses[i].compare(other.clauses[i], **kw):
-                    return False
+                len(self.clauses) == len(other.clauses) and \
+                self.operator is other.operator:
+
+            if self.operator in (operators.and_, operators.or_):
+                completed = set()
+                for clause in self.clauses:
+                    for other_clause in set(other.clauses).difference(completed):
+                        if clause.compare(other_clause, **kw):
+                            completed.add(other_clause)
+                            break
+                return len(completed) == len(other.clauses)
             else:
-                return self.operator == other.operator
+                for i in range(0, len(self.clauses)):
+                    if not self.clauses[i].compare(other.clauses[i], **kw):
+                        return False
+                else:
+                    return True
         else:
             return False
 

Activity

sqlalchemy-bot

sqlalchemy-bot commented on Sep 2, 2016

@sqlalchemy-bot
CollaboratorAuthor

Michael Bayer (@zzzeek) wrote:

Repair clauselist comparison to account for clause ordering

Fixed bug where the "simple many-to-one" condition that allows lazy
loading to use get() from identity map would fail to be invoked if the
primaryjoin of the relationship had multiple clauses separated by AND
which were not in the same order as that of the primary key columns
being compared in each clause. This ordering
difference occurs for a composite foreign key where the table-bound
columns on the referencing side were not in the same order in the .c
collection as the primary key columns on the referenced side....which
in turn occurs a lot if one is using declarative mixins and/or
declared_attr to set up columns.

Change-Id: I66cce74f614c04ed693dc0d58ac8c952b2f8ae54
Fixes: #3788

ce577d4

sqlalchemy-bot

sqlalchemy-bot commented on Sep 2, 2016

@sqlalchemy-bot
CollaboratorAuthor

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
added this to the 1.1 milestone on Nov 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @sqlalchemy-bot

        Issue actions

          simple many-to-one comparison is sensitive to order of clauses · Issue #3788 · sqlalchemy/sqlalchemy