annotate parentmapper in primaryjoin / secondaryjoin

Issue #3366 resolved
Mike Bayer repo owner created an issue

this is related to #3365 but is a more fundamental change.

First, test case from #3365:

from sqlalchemy import Column, Integer, Text, ForeignKey, create_engine
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.orm import Session, relationship

Base = declarative_base()


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(Text, primary_key=True)
    parent = relationship(Parent)

engine = create_engine('sqlite://')
Base.metadata.create_all(engine)
session = Session(engine)

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")

raises: "AttributeError: 'Child' object has no attribute 'id_parent'"

with the fix, raises: "sqlalchemy.exc.InvalidRequestError: Could not evaluate current criteria in Python. Specify 'fetch' or False for the synchronize_session parameter."

the patch involves adding ORM context to the primaryjoin/secondaryjoin:

diff --git a/lib/sqlalchemy/orm/evaluator.py b/lib/sqlalchemy/orm/evaluator.py
index 1e828ff..fb59186 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 b649c9e..291af09 100644
--- a/lib/sqlalchemy/orm/relationships.py
+++ b/lib/sqlalchemy/orm/relationships.py
@@ -1942,6 +1942,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:
@@ -2405,6 +2406,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(
@@ -2811,9 +2825,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

We'd just start tracking "parentmapper" throughout all PJ/SJ conditions.

Comments (9)

  1. Mike Bayer reporter
    • changed milestone to 1.2

    starting to wrap up scope for 1.1, remaining behavioral changes being pushed out

  2. Mike Bayer reporter

    this patch makes no sense. relationship.primaryjoin is the fully deannotated expression right now, that would have to change. but also, the filter condition (Child.parent = p) should be annotated and should not raise an error. For the test to work here we only need add the error throw, none of the annotation stuff is necessary.

  3. Mike Bayer reporter

    revised version of the patch ensures annotations are propagated all the way out to relationship.primaryjoin as well as in the _optimized_compare(), so that the many-to-one comparison works again.

  4. Mike Bayer reporter

    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

    → <<cset 63a7b2d2d940>>

  5. Log in to comment