Annotate parentmapper in primaryjoin / secondaryjoin

This patch applies the "parentmapper" annotation to the columns
in the primaryjoin/secondaryjoin, but more dramatically,
also removes all the "deannotate" steps that were historically
applied to the relationship primaryjoin/secondaryjoin.
These deannotation steps were left over from the initial
implementations of annotations where the behaviors were not
as reliable.

By ensuring these annotations are present,
the evaluator no longer needs to do a name-based lookup
when it sees a column that has no "parentmapper",
because it can be assured this is not a mapped column.
This fixes the issue where the expression were based on
a relationship primaryjoin but the name of a column
in the join condition didn't match the attribute name.

Change-Id: I8c1d4594116d4109fef314a87c96a24d2efa0058
Fixes: #3366

4 files changed, 89 insertions, 17 deletions

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index f4fd6a3fe..daaa11cfe 100644
--- a/doc/build/changelog/changelog_12.rst
+++ b/doc/build/changelog/changelog_12.rst
@@ -13,6 +13,17 @@
 .. changelog::
     :version: 1.2.0b1
 
+    .. change:: 3366
+        :tags: bug, orm
+        :tickets: 3366
+
+        The "evaluate" strategy used by :meth:`.Query.update` and
+        :meth:`.Query.delete` can now accommodate a simple
+        object comparison from a many-to-one relationship to an instance,
+        when the attribute names of the primary key / foreign key columns
+        don't match the actual names of the columns.  Previously this would
+        do a simple name-based match and fail with an AttributeError.
+
     .. change:: 3938
         :tags: bug, engine
         :tickets: 3938
diff --git a/lib/sqlalchemy/orm/evaluator.py b/lib/sqlalchemy/orm/evaluator.py
index 95a9e9b91..aff6718d3 100644
--- a/lib/sqlalchemy/orm/evaluator.py
+++ b/lib/sqlalchemy/orm/evaluator.py
@@ -59,7 +59,9 @@ class EvaluatorCompiler(object):
                 )
             key = parentmapper._columntoproperty[clause].key
         else:
-            key = clause.key
+            raise UnevaluatableError(
+                "Cannot evaluate column: %s" % clause
+            )
 
         get_corresponding_attr = operator.attrgetter(key)
         return lambda obj: get_corresponding_attr(obj)
diff --git a/lib/sqlalchemy/orm/relationships.py b/lib/sqlalchemy/orm/relationships.py
index 1298e8880..dacb68b45 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -1727,8 +1727,8 @@ class RelationshipProperty(StrategizedProperty):
             support_sync=not self.viewonly,
             can_be_synced_fn=self._columns_are_mapped
         )
-        self.primaryjoin = jc.deannotated_primaryjoin
-        self.secondaryjoin = jc.deannotated_secondaryjoin
+        self.primaryjoin = jc.primaryjoin
+        self.secondaryjoin = jc.secondaryjoin
         self.direction = jc.direction
         self.local_remote_pairs = jc.local_remote_pairs
         self.remote_side = jc.remote_columns
@@ -1987,6 +1987,7 @@ class JoinCondition(object):
         self._annotate_fks()
         self._annotate_remote()
         self._annotate_local()
+        self._annotate_parentmapper()
         self._setup_pairs()
         self._check_foreign_cols(self.primaryjoin, True)
         if self.secondaryjoin is not None:
@@ -2450,6 +2451,19 @@ class JoinCondition(object):
             self.primaryjoin, {}, locals_
         )
 
+    def _annotate_parentmapper(self):
+        if self.prop is None:
+            return
+
+        def parentmappers_(elem):
+            if "remote" in elem._annotations:
+                return elem._annotate({"parentmapper": self.prop.mapper})
+            elif "local" in elem._annotations:
+                return elem._annotate({"parentmapper": self.prop.parent})
+        self.primaryjoin = visitors.replacement_traverse(
+            self.primaryjoin, {}, parentmappers_
+        )
+
     def _check_remote_side(self):
         if not self.local_remote_pairs:
             raise sa_exc.ArgumentError(
@@ -2707,17 +2721,6 @@ class JoinCondition(object):
     def foreign_key_columns(self):
         return self._gather_join_annotations("foreign")
 
-    @util.memoized_property
-    def deannotated_primaryjoin(self):
-        return _deep_deannotate(self.primaryjoin)
-
-    @util.memoized_property
-    def deannotated_secondaryjoin(self):
-        if self.secondaryjoin is not None:
-            return _deep_deannotate(self.secondaryjoin)
-        else:
-            return None
-
     def _gather_join_annotations(self, annotation):
         s = set(
             self._gather_columns_with_annotation(
@@ -2856,9 +2859,6 @@ class JoinCondition(object):
 
         bind_to_col = dict((binds[col].key, col) for col in binds)
 
-        # this is probably not necessary
-        lazywhere = _deep_deannotate(lazywhere)
-
         return lazywhere, bind_to_col, equated_columns
 
 
diff --git a/test/orm/test_evaluator.py b/test/orm/test_evaluator.py
index d31610f2b..ffb8d8701 100644
--- a/test/orm/test_evaluator.py
+++ b/test/orm/test_evaluator.py
@@ -9,6 +9,14 @@ from sqlalchemy import and_, or_, not_
 from sqlalchemy.orm import evaluator
 from sqlalchemy.orm import mapper
 
+from sqlalchemy.orm import relationship, Session
+from sqlalchemy import ForeignKey
+from sqlalchemy.orm import exc as orm_exc
+from sqlalchemy.testing import assert_raises_message
+from sqlalchemy.testing import is_
+from sqlalchemy import inspect
+
+
 compiler = evaluator.EvaluatorCompiler()
 
 
@@ -76,6 +84,21 @@ class EvaluateTest(fixtures.MappedTest):
         eval_eq(User.name == None,  # noqa
                 testcases=[(User(name='foo'), False), (User(name=None), True)])
 
+    def test_raise_on_unannotated_column(self):
+        User = self.classes.User
+
+        assert_raises_message(
+            evaluator.UnevaluatableError,
+            "Cannot evaluate column: foo",
+            compiler.process, User.id == Column('foo', Integer)
+        )
+
+        # if we let the above method through as we did
+        # prior to [ticket:3366], we would get
+        # AttributeError: 'User' object has no attribute 'foo'
+        # u1 = User(id=5)
+        # meth(u1)
+
     def test_true_false(self):
         User = self.classes.User
 
@@ -134,3 +157,39 @@ class EvaluateTest(fixtures.MappedTest):
             (User(id=None, name='foo'), None),
             (User(id=None, name=None), None),
         ])
+
+
+class M2OEvaluateTest(fixtures.DeclarativeMappedTest):
+    @classmethod
+    def setup_classes(cls):
+        Base = cls.DeclarativeBasic
+
+        class Parent(Base):
+            __tablename__ = "parent"
+            id = Column(Integer, primary_key=True)
+
+        class Child(Base):
+            __tablename__ = "child"
+            _id_parent = Column(
+                "id_parent", Integer, ForeignKey(Parent.id), primary_key=True)
+            name = Column(String(50), primary_key=True)
+            parent = relationship(Parent)
+
+    def test_delete(self):
+        Parent, Child = self.classes('Parent', 'Child')
+
+        session = Session()
+
+        p = Parent(id=1)
+        session.add(p)
+        session.commit()
+
+        c = Child(name="foo", parent=p)
+        session.add(c)
+        session.commit()
+
+        session.query(Child).filter(Child.parent == p).delete("evaluate")
+
+        is_(
+            inspect(c).deleted, True
+        )