- Fixed a very old behavior where the lazy load emitted for a one-to-many

could inappropriately pull in the parent table, and also return results
inconsistent based on what's in the parent table, when the primaryjoin
includes some kind of discriminator against the parent table, such
as ``and_(parent.id == child.parent_id, parent.deleted == False)``.
While this primaryjoin doesn't make that much sense for a one-to-many,
it is slightly more common when applied to the many-to-one side, and
the one-to-many comes as a result of a backref.
Loading rows from ``child`` in this case would keep ``parent.deleted == False``
as is within the query, thereby yanking it into the FROM clause
and doing a cartesian product.  The new behavior will now substitute
the value of the local "parent.deleted" for that parameter as is
appropriate.   Though typically, a real-world app probably wants to use a
different primaryjoin for the o2m side in any case.
fixes #2948

4 files changed, 191 insertions, 7 deletions

diff --git a/doc/build/changelog/changelog_09.rst b/doc/build/changelog/changelog_09.rst
index f2594b733..321deb647 100644
--- a/doc/build/changelog/changelog_09.rst
+++ b/doc/build/changelog/changelog_09.rst
@@ -16,6 +16,25 @@
 
     .. change::
         :tags: bug, orm
+        :tickets: 2948
+
+        Fixed a very old behavior where the lazy load emitted for a one-to-many
+        could inappropriately pull in the parent table, and also return results
+        inconsistent based on what's in the parent table, when the primaryjoin
+        includes some kind of discriminator against the parent table, such
+        as ``and_(parent.id == child.parent_id, parent.deleted == False)``.
+        While this primaryjoin doesn't make that much sense for a one-to-many,
+        it is slightly more common when applied to the many-to-one side, and
+        the one-to-many comes as a result of a backref.
+        Loading rows from ``child`` in this case would keep ``parent.deleted == False``
+        as is within the query, thereby yanking it into the FROM clause
+        and doing a cartesian product.  The new behavior will now substitute
+        the value of the local "parent.deleted" for that parameter as is
+        appropriate.   Though typically, a real-world app probably wants to use a
+        different primaryjoin for the o2m side in any case.
+
+    .. change::
+        :tags: bug, orm
         :tickets: 2965
 
         Improved the check for "how to join from A to B" such that when
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index a0b36ce57..311fba478 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -2587,6 +2587,7 @@ class JoinCondition(object):
         binds = util.column_dict()
         lookup = util.column_dict()
         equated_columns = util.column_dict()
+        being_replaced = set()
 
         if reverse_direction and self.secondaryjoin is None:
             for l, r in self.local_remote_pairs:
@@ -2594,16 +2595,22 @@ class JoinCondition(object):
                 _list.append((r, l))
                 equated_columns[l] = r
         else:
+            # replace all "local side" columns, which is
+            # anything that isn't marked "remote"
+            being_replaced.update(self.local_columns)
             for l, r in self.local_remote_pairs:
                 _list = lookup.setdefault(l, [])
                 _list.append((l, r))
                 equated_columns[r] = l
 
         def col_to_bind(col):
-            if col in lookup:
-                for tobind, equated in lookup[col]:
-                    if equated in binds:
-                        return None
+            if col in being_replaced or col in lookup:
+                if col in lookup:
+                    for tobind, equated in lookup[col]:
+                        if equated in binds:
+                            return None
+                else:
+                    assert not reverse_direction
                 if col not in binds:
                     binds[col] = sql.bindparam(
                         None, None, type_=col.type, unique=True)
diff --git a/test/orm/test_lazy_relations.py b/test/orm/test_lazy_relations.py
index 37d290b58..82e968ed0 100644
--- a/test/orm/test_lazy_relations.py
+++ b/test/orm/test_lazy_relations.py
@@ -5,15 +5,16 @@ import datetime
 from sqlalchemy import exc as sa_exc
 from sqlalchemy.orm import attributes, exc as orm_exc
 import sqlalchemy as sa
-from sqlalchemy import testing
-from sqlalchemy import Integer, String, ForeignKey, SmallInteger
+from sqlalchemy import testing, and_
+from sqlalchemy import Integer, String, ForeignKey, SmallInteger, Boolean
 from sqlalchemy.types import TypeDecorator
 from sqlalchemy.testing.schema import Table
 from sqlalchemy.testing.schema import Column
-from sqlalchemy.orm import mapper, relationship, create_session
+from sqlalchemy.orm import mapper, relationship, create_session, Session
 from sqlalchemy.testing import eq_
 from sqlalchemy.testing import fixtures
 from test.orm import _fixtures
+from sqlalchemy.testing.assertsql import AllOf, CompiledSQL
 
 
 class LazyTest(_fixtures.FixtureTest):
@@ -691,4 +692,117 @@ class CorrelatedTest(fixtures.MappedTest):
             User(name='user3', stuff=[Stuff(id=5, date=datetime.date(2007, 6, 15))])
         ])
 
+class O2MWOSideFixedTest(fixtures.MappedTest):
+    # test #2948 - o2m backref with a "m2o does/does not count"
+    # criteria doesn't scan the "o" table
 
+    @classmethod
+    def define_tables(self, meta):
+        Table('city', meta,
+            Column('id', Integer, primary_key=True),
+            Column('deleted', Boolean),
+        )
+        Table('person', meta,
+              Column('id', Integer, primary_key=True),
+              Column('city_id', ForeignKey('city.id'))
+              )
+
+
+    @classmethod
+    def setup_classes(cls):
+        class Person(cls.Basic):
+            pass
+
+        class City(cls.Basic):
+            pass
+
+    @classmethod
+    def setup_mappers(cls):
+        Person, City = cls.classes.Person, cls.classes.City
+        city, person = cls.tables.city, cls.tables.person
+
+        mapper(Person, person, properties={
+                'city':relationship(City,
+                            primaryjoin=and_(
+                                        person.c.city_id==city.c.id,
+                                        city.c.deleted == False),
+                            backref='people'
+                        )
+            })
+        mapper(City, city)
+
+    def _fixture(self, include_other):
+        city, person = self.tables.city, self.tables.person
+
+        if include_other:
+            city.insert().execute(
+                {"id": 1, "deleted": False},
+            )
+
+            person.insert().execute(
+                {"id": 1, "city_id": 1},
+                {"id": 2, "city_id": 1},
+            )
+
+        city.insert().execute(
+            {"id": 2, "deleted": True},
+        )
+
+        person.insert().execute(
+            {"id": 3, "city_id": 2},
+            {"id": 4, "city_id": 2},
+        )
+
+
+    def test_lazyload_assert_expected_sql(self):
+        self._fixture(True)
+        Person, City = self.classes.Person, self.classes.City
+        sess = Session(testing.db)
+        c1, c2 = sess.query(City).order_by(City.id).all()
+
+        def go():
+            eq_(
+                [p.id for p in c2.people],
+                []
+            )
+
+        self.assert_sql_execution(
+                testing.db,
+                go,
+                CompiledSQL(
+                    "SELECT person.id AS person_id, person.city_id AS "
+                    "person_city_id FROM person "
+                    "WHERE person.city_id = :param_1 AND :param_2 = 0",
+                    {"param_1": 2, "param_2": 1}
+                )
+        )
+
+    def test_lazyload_people_other_exists(self):
+        self._fixture(True)
+        Person, City = self.classes.Person, self.classes.City
+        sess = Session(testing.db)
+        c1, c2 = sess.query(City).order_by(City.id).all()
+        eq_(
+            [p.id for p in c1.people],
+            [1, 2]
+        )
+
+        eq_(
+            [p.id for p in c2.people],
+            []
+        )
+
+    def test_lazyload_people_no_other_exists(self):
+        # note that if we revert #2948, *this still passes!*
+        # e.g. due to the scan of the "o" table, whether or not *another*
+        # row exists determines if this works.
+
+        self._fixture(False)
+        Person, City = self.classes.Person, self.classes.City
+        sess = Session(testing.db)
+        c2, = sess.query(City).order_by(City.id).all()
+
+        eq_(
+            [p.id for p in c2.people],
+            []
+        )
diff --git a/test/orm/test_rel_fn.py b/test/orm/test_rel_fn.py
index 0b35a44eb..c4d811d53 100644
--- a/test/orm/test_rel_fn.py
+++ b/test/orm/test_rel_fn.py
@@ -416,6 +416,30 @@ class _JoinFixtures(object):
                     **kw
                 )
 
+
+        cls.left = Table('lft', m,
+            Column('id', Integer, primary_key=True),
+            Column('x', Integer),
+            Column('y', Integer),
+        )
+        cls.right = Table('rgt', m,
+            Column('id', Integer, primary_key=True),
+            Column('lid', Integer, ForeignKey('lft.id')),
+            Column('x', Integer),
+            Column('y', Integer),
+        )
+
+    def _join_fixture_o2m_o_side_none(self, **kw):
+        return relationships.JoinCondition(
+                    self.left,
+                    self.right,
+                    self.left,
+                    self.right,
+                    primaryjoin=and_(self.left.c.id == self.right.c.lid,
+                                        self.left.c.x == 5),
+                    **kw
+                    )
+
     def _assert_non_simple_warning(self, fn):
         assert_raises_message(
             exc.SAWarning,
@@ -1062,3 +1086,23 @@ class LazyClauseTest(_JoinFixtures, fixtures.TestBase, AssertsCompiledSQL):
             "lft.id = :param_1"
         )
 
+    def test_lazy_clause_o2m_o_side_none(self):
+        # test for #2948.  When the join is "o.id == m.oid AND o.something == something",
+        # we don't want 'o' brought into the lazy load for 'm'
+        joincond = self._join_fixture_o2m_o_side_none()
+        lazywhere, bind_to_col, equated_columns = joincond.create_lazy_clause()
+        self.assert_compile(
+            lazywhere,
+            ":param_1 = rgt.lid AND :param_2 = :x_1",
+            checkparams={'param_1': None, 'param_2': None, 'x_1': 5}
+        )
+
+    def test_lazy_clause_o2m_o_side_none_reverse(self):
+        # continued test for #2948.
+        joincond = self._join_fixture_o2m_o_side_none()
+        lazywhere, bind_to_col, equated_columns = joincond.create_lazy_clause(reverse_direction=True)
+        self.assert_compile(
+            lazywhere,
+            "lft.id = :param_1 AND lft.x = :x_1",
+            checkparams= {'param_1': None, 'x_1': 5}
+        )