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

6 files changed, 186 insertions, 7 deletions

diff --git a/doc/build/changelog/changelog_11.rst b/doc/build/changelog/changelog_11.rst
index 88f1ebb24..669f08188 100644
--- a/doc/build/changelog/changelog_11.rst
+++ b/doc/build/changelog/changelog_11.rst
@@ -22,6 +22,21 @@
     :version: 1.1.0
 
     .. change::
+        :tags: bug, orm
+        :tickets: 3788
+
+        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::
         :tags: bug, sql
         :tickets: 3786
 
diff --git a/lib/sqlalchemy/sql/elements.py b/lib/sqlalchemy/sql/elements.py
index 75d5368d5..cff57372c 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
 
diff --git a/lib/sqlalchemy/testing/__init__.py b/lib/sqlalchemy/testing/__init__.py
index f4a23d238..8f3d063be 100644
--- a/lib/sqlalchemy/testing/__init__.py
+++ b/lib/sqlalchemy/testing/__init__.py
@@ -22,7 +22,7 @@ from .assertions import emits_warning, emits_warning_on, uses_deprecated, \
     eq_, ne_, le_, is_, is_not_, startswith_, assert_raises, \
     assert_raises_message, AssertsCompiledSQL, ComparesTables, \
     AssertsExecutionResults, expect_deprecated, expect_warnings, \
-    in_, not_in_, eq_ignore_whitespace, eq_regex
+    in_, not_in_, eq_ignore_whitespace, eq_regex, is_true, is_false
 
 from .util import run_as_contextmanager, rowset, fail, \
     provide_metadata, adict, force_drop_names, \
diff --git a/lib/sqlalchemy/testing/assertions.py b/lib/sqlalchemy/testing/assertions.py
index bd1ccaa51..84653da5c 100644
--- a/lib/sqlalchemy/testing/assertions.py
+++ b/lib/sqlalchemy/testing/assertions.py
@@ -224,6 +224,14 @@ def le_(a, b, msg=None):
     assert a <= b, msg or "%r != %r" % (a, b)
 
 
+def is_true(a, msg=None):
+    is_(a, True, msg=msg)
+
+
+def is_false(a, msg=None):
+    is_(a, False, msg=msg)
+
+
 def is_(a, b, msg=None):
     """Assert a is b, with repr messaging on failure."""
     assert a is b, msg or "%r is not %r" % (a, b)
diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py
index f2e1db2da..56d1b8323 100644
--- a/test/orm/test_lazy_relations.py
+++ b/test/orm/test_lazy_relations.py
@@ -6,12 +6,13 @@ from sqlalchemy.orm import attributes, exc as orm_exc, configure_mappers
 import sqlalchemy as sa
 from sqlalchemy import testing, and_
 from sqlalchemy import Integer, String, ForeignKey, SmallInteger, Boolean
+from sqlalchemy import ForeignKeyConstraint
 from sqlalchemy.types import TypeDecorator
 from sqlalchemy.testing.schema import Table
 from sqlalchemy.testing.schema import Column
 from sqlalchemy import orm
 from sqlalchemy.orm import mapper, relationship, create_session, Session
-from sqlalchemy.testing import eq_
+from sqlalchemy.testing import eq_, is_true, is_false
 from sqlalchemy.testing import fixtures
 from test.orm import _fixtures
 from sqlalchemy.testing.assertsql import CompiledSQL
@@ -1148,3 +1149,69 @@ class TypeCoerceTest(fixtures.MappedTest, testing.AssertsExecutionResults,):
                 [{'param_1': 5}]
             )
         )
+
+
+class CompositeSimpleM2OTest(fixtures.MappedTest):
+    """ORM-level test for [ticket:3788]"""
+
+    @classmethod
+    def define_tables(cls, metadata):
+        Table(
+            'a', metadata,
+            Column("id1", Integer, primary_key=True),
+            Column("id2", Integer, primary_key=True),
+        )
+
+        Table(
+            "b_sameorder", metadata,
+            Column("id", Integer, primary_key=True),
+            Column('a_id1', Integer),
+            Column('a_id2', Integer),
+            ForeignKeyConstraint(['a_id1', 'a_id2'], ['a.id1', 'a.id2'])
+        )
+
+        Table(
+            "b_differentorder", metadata,
+            Column("id", Integer, primary_key=True),
+            Column('a_id1', Integer),
+            Column('a_id2', Integer),
+            ForeignKeyConstraint(['a_id1', 'a_id2'], ['a.id1', 'a.id2'])
+        )
+
+    @classmethod
+    def setup_classes(cls):
+        class A(cls.Basic):
+            pass
+
+        class B(cls.Basic):
+            pass
+
+    def test_use_get_sameorder(self):
+        mapper(self.classes.A, self.tables.a)
+        m_b = mapper(self.classes.B, self.tables.b_sameorder, properties={
+            'a': relationship(self.classes.A)
+        })
+
+        configure_mappers()
+        is_true(m_b.relationships.a.strategy.use_get)
+
+    def test_use_get_reverseorder(self):
+        mapper(self.classes.A, self.tables.a)
+        m_b = mapper(self.classes.B, self.tables.b_differentorder, properties={
+            'a': relationship(self.classes.A)
+        })
+
+        configure_mappers()
+        is_true(m_b.relationships.a.strategy.use_get)
+
+    def test_dont_use_get_pj_is_different(self):
+        mapper(self.classes.A, self.tables.a)
+        m_b = mapper(self.classes.B, self.tables.b_sameorder, properties={
+            'a': relationship(self.classes.A, primaryjoin=and_(
+                self.tables.a.c.id1 == self.tables.b_sameorder.c.a_id1,
+                self.tables.a.c.id2 == 12
+            ))
+        })
+
+        configure_mappers()
+        is_false(m_b.relationships.a.strategy.use_get)
diff --git a/test/sql/test_utils.py b/test/sql/test_utils.py
new file mode 100644
index 000000000..09d7e98af
--- /dev/null
+++ b/test/sql/test_utils.py
@@ -0,0 +1,78 @@
+from sqlalchemy.testing import fixtures, is_true, is_false
+from sqlalchemy import MetaData, Table, Column, Integer
+from sqlalchemy import and_, or_
+from sqlalchemy.sql.elements import ClauseList
+from sqlalchemy.sql import operators
+
+
+class CompareClausesTest(fixtures.TestBase):
+    def setup(self):
+        m = MetaData()
+        self.a = Table(
+            'a', m,
+            Column('x', Integer),
+            Column('y', Integer)
+        )
+
+        self.b = Table(
+            'b', m,
+            Column('y', Integer),
+            Column('z', Integer)
+        )
+
+    def test_compare_clauselist_associative(self):
+
+        l1 = and_(
+            self.a.c.x == self.b.c.y,
+            self.a.c.y == self.b.c.z
+        )
+
+        l2 = and_(
+            self.a.c.y == self.b.c.z,
+            self.a.c.x == self.b.c.y,
+        )
+
+        l3 = and_(
+            self.a.c.x == self.b.c.z,
+            self.a.c.y == self.b.c.y
+        )
+
+        is_true(l1.compare(l1))
+        is_true(l1.compare(l2))
+        is_false(l1.compare(l3))
+
+    def test_compare_clauselist_not_associative(self):
+
+        l1 = ClauseList(
+            self.a.c.x, self.a.c.y, self.b.c.y, operator=operators.sub)
+
+        l2 = ClauseList(
+            self.b.c.y, self.a.c.x, self.a.c.y, operator=operators.sub)
+
+        is_true(l1.compare(l1))
+        is_false(l1.compare(l2))
+
+    def test_compare_clauselist_assoc_different_operator(self):
+
+        l1 = and_(
+            self.a.c.x == self.b.c.y,
+            self.a.c.y == self.b.c.z
+        )
+
+        l2 = or_(
+            self.a.c.y == self.b.c.z,
+            self.a.c.x == self.b.c.y,
+        )
+
+        is_false(l1.compare(l2))
+
+    def test_compare_clauselist_not_assoc_different_operator(self):
+
+        l1 = ClauseList(
+            self.a.c.x, self.a.c.y, self.b.c.y, operator=operators.sub)
+
+        l2 = ClauseList(
+            self.a.c.x, self.a.c.y, self.b.c.y, operator=operators.div)
+
+        is_false(l1.compare(l2))
+