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

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index f4fd6a3fe810f29adae343c973bd53b9cd5383ca..daaa11cfed9da15dddee3b643d60e4ee4139ca12 100644 (file)
--- 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 95a9e9b9153035a392c7051ee3c64705ece7c348..aff6718d311a41809913e1e6aea84706331f802c 100644 (file)
--- 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 1298e8880a5bb4c1a2229affa8a4616f249764f1..dacb68b453764de62d549355bf26001190468d66 100644 (file)
--- 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 d31610f2b6262e0dfab0d283a36657a2af9e095c..ffb8d8701b2e71dad232a241997be93397f25cd6 100644 (file)
--- 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
+        )