- Fixed bug in lazy loading SQL construction whereby a complex

primaryjoin that referred to the same "local" column multiple
times in the "column that points to itself" style of self-referential
join would not be substituted in all cases.   The logic to determine
substitutions here has been reworked to be more open-ended.
fixes #3300

4 files changed, 101 insertions, 17 deletions

diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index 10d003f09..99201ea01 100644
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -15,6 +15,16 @@
     :version: 0.9.9
 
     .. change::
+        :tags: bug, orm
+        :tickets: 3300
+
+        Fixed bug in lazy loading SQL construction whereby a complex
+        primaryjoin that referred to the same "local" column multiple
+        times in the "column that points to itself" style of self-referential
+        join would not be substituted in all cases.   The logic to determine
+        substitutions here has been reworked to be more open-ended.
+
+    .. change::
         :tags: bug, postgresql
         :tickets: 2940
 
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index df2250a4c..969b231ec 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -2692,27 +2692,31 @@ class JoinCondition(object):
 
     def create_lazy_clause(self, reverse_direction=False):
         binds = util.column_dict()
-        lookup = collections.defaultdict(list)
         equated_columns = util.column_dict()
 
-        if reverse_direction and self.secondaryjoin is None:
-            for l, r in self.local_remote_pairs:
-                lookup[r].append((r, l))
-                equated_columns[l] = r
-        else:
-            # replace all "local side" columns, which is
-            # anything that isn't marked "remote"
+        has_secondary = self.secondaryjoin is not None
+
+        if has_secondary:
+            lookup = collections.defaultdict(list)
             for l, r in self.local_remote_pairs:
                 lookup[l].append((l, r))
                 equated_columns[r] = l
+        elif not reverse_direction:
+            for l, r in self.local_remote_pairs:
+                equated_columns[r] = l
+        else:
+            for l, r in self.local_remote_pairs:
+                equated_columns[l] = r
 
         def col_to_bind(col):
-            if (reverse_direction and col in lookup) or \
-                    (not reverse_direction and "local" in col._annotations):
-                if col in lookup:
-                    for tobind, equated in lookup[col]:
-                        if equated in binds:
-                            return None
+
+            if (
+                (not reverse_direction and 'local' in col._annotations) or
+                reverse_direction and (
+                    (has_secondary and col in lookup) or
+                    (not has_secondary and 'remote' in col._annotations)
+                )
+            ):
                 if col not in binds:
                     binds[col] = sql.bindparam(
                         None, None, type_=col.type, unique=True)
diff --git a/test/orm/test_rel_fn.py b/test/orm/test_rel_fn.py
index 150b59b75..230f3b18a 100644
--- a/test/orm/test_rel_fn.py
+++ b/test/orm/test_rel_fn.py
@@ -490,6 +490,19 @@ class _JoinFixtures(object):
                         )
                 )
 
+    def _join_fixture_remote_local_multiple_ref(self, **kw):
+        fn = lambda a, b: ((a == b) | (b == a))
+        return relationships.JoinCondition(
+            self.selfref, self.selfref,
+            self.selfref, self.selfref,
+            support_sync=False,
+            primaryjoin=fn(
+                # we're putting a do-nothing annotation on
+                # "a" so that the left/right is preserved;
+                # annotation vs. non seems to affect __eq__ behavior
+                self.selfref.c.sid._annotate({"foo": "bar"}),
+                foreign(remote(self.selfref.c.sid)))
+        )
 
     def _assert_non_simple_warning(self, fn):
         assert_raises_message(
@@ -1175,3 +1188,13 @@ class LazyClauseTest(_JoinFixtures, fixtures.TestBase, AssertsCompiledSQL):
             "lft.id = :param_1 AND lft.x = :x_1",
             checkparams= {'param_1': None, 'x_1': 5}
         )
+
+    def test_lazy_clause_remote_local_multiple_ref(self):
+        joincond = self._join_fixture_remote_local_multiple_ref()
+        lazywhere, bind_to_col, equated_columns = joincond.create_lazy_clause()
+
+        self.assert_compile(
+            lazywhere,
+            ":param_1 = selfref.sid OR selfref.sid = :param_1",
+            checkparams={'param_1': None}
+        )
diff --git a/test/orm/test_relationships.py b/test/orm/test_relationships.py
index 2a15ce666..9e4b38a90 100644
--- a/test/orm/test_relationships.py
+++ b/test/orm/test_relationships.py
@@ -436,6 +436,33 @@ class DirectSelfRefFKTest(fixtures.MappedTest, AssertsCompiledSQL):
         ])
         return sess
 
+    def test_descendants_lazyload_clause(self):
+        self._descendants_fixture(data=False)
+        Entity = self.classes.Entity
+        self.assert_compile(
+            Entity.descendants.property.strategy._lazywhere,
+            "entity.path LIKE (:param_1 || :path_1)"
+        )
+
+        self.assert_compile(
+            Entity.descendants.property.strategy._rev_lazywhere,
+            ":param_1 LIKE (entity.path || :path_1)"
+        )
+
+    def test_ancestors_lazyload_clause(self):
+        self._anscestors_fixture(data=False)
+        Entity = self.classes.Entity
+        # :param_1 LIKE (:param_1 || :path_1)
+        self.assert_compile(
+            Entity.anscestors.property.strategy._lazywhere,
+            ":param_1 LIKE (entity.path || :path_1)"
+        )
+
+        self.assert_compile(
+            Entity.anscestors.property.strategy._rev_lazywhere,
+            "entity.path LIKE (:param_1 || :path_1)"
+        )
+
     def test_descendants_lazyload(self):
         sess = self._descendants_fixture()
         Entity = self.classes.Entity
@@ -500,7 +527,7 @@ class DirectSelfRefFKTest(fixtures.MappedTest, AssertsCompiledSQL):
         )
 
 
-class CompositeSelfRefFKTest(fixtures.MappedTest):
+class CompositeSelfRefFKTest(fixtures.MappedTest, AssertsCompiledSQL):
 
     """Tests a composite FK where, in
     the relationship(), one col points
@@ -523,6 +550,8 @@ class CompositeSelfRefFKTest(fixtures.MappedTest):
 
     """
 
+    __dialect__ = 'default'
+
     @classmethod
     def define_tables(cls, metadata):
         Table('company_t', metadata,
@@ -670,6 +699,7 @@ class CompositeSelfRefFKTest(fixtures.MappedTest):
             )
         })
 
+        self._assert_lazy_clauses()
         self._test()
 
     def test_overlapping_warning(self):
@@ -718,6 +748,7 @@ class CompositeSelfRefFKTest(fixtures.MappedTest):
             )
         })
 
+        self._assert_lazy_clauses()
         self._test_no_warning()
 
     def _test_no_overwrite(self, sess, expect_failure):
@@ -749,6 +780,7 @@ class CompositeSelfRefFKTest(fixtures.MappedTest):
         self._test_no_warning(overwrites=True)
 
     def _test_no_warning(self, overwrites=False):
+        configure_mappers()
         self._test_relationships()
         sess = Session()
         self._setup_data(sess)
@@ -756,16 +788,31 @@ class CompositeSelfRefFKTest(fixtures.MappedTest):
         self._test_join_aliasing(sess)
         self._test_no_overwrite(sess, expect_failure=overwrites)
 
-    def _test_relationships(self):
+    @testing.emits_warning("relationship .* will copy column ")
+    def _assert_lazy_clauses(self):
         configure_mappers()
         Employee = self.classes.Employee
+        self.assert_compile(
+            Employee.employees.property.strategy._lazywhere,
+            ":param_1 = employee_t.reports_to_id AND "
+            ":param_2 = employee_t.company_id"
+        )
+
+        self.assert_compile(
+            Employee.employees.property.strategy._rev_lazywhere,
+            "employee_t.emp_id = :param_1 AND "
+            "employee_t.company_id = :param_2"
+        )
+
+    def _test_relationships(self):
+        Employee = self.classes.Employee
         employee_t = self.tables.employee_t
         eq_(
             set(Employee.employees.property.local_remote_pairs),
             set([
                 (employee_t.c.company_id, employee_t.c.company_id),
                 (employee_t.c.emp_id, employee_t.c.reports_to_id),
-                ])
+            ])
         )
         eq_(
             Employee.employees.property.remote_side,