Search through mapper superclass hierarchy for owner

Fixed regression caused by fix for issue :ticket:`4116` affecting versions
1.2.2 as well as 1.1.15, which had the effect of mis-calculation of the
"owning class" of an :class:`.AssociationProxy` as the ``NoneType`` class
in some declarative mixin/inheritance situations as well as if the
association proxy were accessed off of an un-mapped class.  The "figure out
the owner" logic has been replaced by an in-depth routine that searches
through the complete mapper hierarchy assigned to the class or subclass to
determine the correct (we hope) match; will not assign the owner if no
match is found.  An exception is now raised if the proxy is used
against an un-mapped instance.

Change-Id: I611b590df2babe077ce6c19bea89e84251d1a7f4
Fixes: #4185
(cherry picked from commit 650b9eddae0eb198c8f8dc2d1e1e3c6ac53b18f3)

diff --git a/doc/build/changelog/unreleased_11/4185.rst b/doc/build/changelog/unreleased_11/4185.rst
new file mode 100644 (file)
index 0000000..8be1f77
--- /dev/null
+++ b/doc/build/changelog/unreleased_11/4185.rst
@@ -0,0 +1,16 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4185
+    :versions: 1.2.3
+
+    Fixed regression caused by fix for issue :ticket:`4116` affecting versions
+    1.2.2 as well as 1.1.15, which had the effect of mis-calculation of the
+    "owning class" of an :class:`.AssociationProxy` as the ``NoneType`` class
+    in some declarative mixin/inheritance situations as well as if the
+    association proxy were accessed off of an un-mapped class.  The "figure out
+    the owner" logic has been replaced by an in-depth routine that searches
+    through the complete mapper hierarchy assigned to the class or subclass to
+    determine the correct (we hope) match; will not assign the owner if no
+    match is found.  An exception is now raised if the proxy is used
+    against an un-mapped instance.
+

diff --git a/lib/sqlalchemy/ext/associationproxy.py b/lib/sqlalchemy/ext/associationproxy.py
index a104b868d181613035c731ffeb614a47559c49bf..e701f5908596e1eb2e675b35b9661d2151254955 100644 (file)
--- a/lib/sqlalchemy/ext/associationproxy.py
+++ b/lib/sqlalchemy/ext/associationproxy.py
@@ -212,7 +212,12 @@ class AssociationProxy(interfaces.InspectionAttrInfo):
         return (self.local_attr, self.remote_attr)
 
     def _get_property(self):
-        return (orm.class_mapper(self.owning_class).
+        owning_class = self.owning_class
+        if owning_class is None:
+            raise exc.InvalidRequestError(
+                "This association proxy has no mapped owning class; "
+                "can't locate a mapped property")
+        return (orm.class_mapper(owning_class).
                 get_property(self.target_collection))
 
     @util.memoized_property
@@ -244,18 +249,33 @@ class AssociationProxy(interfaces.InspectionAttrInfo):
     def _target_is_object(self):
         return getattr(self.target_class, self.value_attr).impl.uses_objects
 
+    def _calc_owner(self, obj, class_):
+        if obj is not None and class_ is None:
+            target_cls = type(obj)
+        elif class_ is not None:
+            target_cls = class_
+        else:
+            return
+
+        # we might be getting invoked for a subclass
+        # that is not mapped yet, in some declarative situations.
+        # save until we are mapped
+        try:
+            insp = inspect(target_cls)
+        except exc.NoInspectionAvailable:
+            # can't find a mapper, don't set owner. if we are a not-yet-mapped
+            # subclass, we can also scan through __mro__ to find a mapped
+            # class, but instead just wait for us to be called again against a
+            # mapped class normally.
+            return
+
+        # note we can get our real .key here too
+        owner = insp.mapper.class_manager._locate_owning_manager(self)
+        self.owning_class = owner.class_
+
     def __get__(self, obj, class_):
         if self.owning_class is None:
-            try:
-                insp = inspect(class_)
-            except exc.NoInspectionAvailable:
-                pass
-            else:
-                if hasattr(insp, 'mapper'):
-                    self.owning_class = insp.mapper.class_
-
-            if self.owning_class is None:
-                self.owning_class = type(obj)
+            self._calc_owner(obj, class_)
 
         if obj is None:
             return self
@@ -278,7 +298,7 @@ class AssociationProxy(interfaces.InspectionAttrInfo):
 
     def __set__(self, obj, values):
         if self.owning_class is None:
-            self.owning_class = type(obj)
+            self._calc_owner(obj, None)
 
         if self.scalar:
             creator = self.creator and self.creator or self.target_class
@@ -295,7 +315,8 @@ class AssociationProxy(interfaces.InspectionAttrInfo):
 
     def __delete__(self, obj):
         if self.owning_class is None:
-            self.owning_class = type(obj)
+            self._calc_owner(obj, None)
+
         delattr(obj, self.key)
 
     def _initialize_scalar_accessors(self):
@@ -464,6 +485,7 @@ class AssociationProxy(interfaces.InspectionAttrInfo):
             getattr(self.target_class, self.value_attr) != obj)
 
 
+
 class _lazy_collection(object):
     def __init__(self, obj, target):
         self.ref = weakref.ref(obj)

diff --git a/lib/sqlalchemy/orm/instrumentation.py b/lib/sqlalchemy/orm/instrumentation.py
index c17f8ab8d772c53e17140eb82c0c6f4923cdbf91..aa388484705be6f7fde1efab342a69e1c2d33769 100644 (file)
--- a/lib/sqlalchemy/orm/instrumentation.py
+++ b/lib/sqlalchemy/orm/instrumentation.py
@@ -115,6 +115,41 @@ class ClassManager(dict):
         # raises unless self.mapper has been assigned
         raise exc.UnmappedClassError(self.class_)
 
+    def _locate_owning_manager(self, attribute):
+        """Scan through all instrumented classes in our hierarchy
+        searching for the given object as an attribute, and return
+        the bottommost owner.
+
+        E.g.::
+
+            foo = foobar()
+
+            class Parent:
+                attr = foo
+
+            class Child(Parent):
+                pass
+
+        Child.manager._locate_owning_manager(foo) would
+        give us Parent.
+
+        Needed by association proxy to correctly figure out the
+        owning class when the attribute is accessed.
+
+        """
+
+        stack = [None]
+        for supercls in self.class_.__mro__:
+            mgr = manager_of_class(supercls)
+            if not mgr:
+                continue
+            for key in set(supercls.__dict__):
+                val = supercls.__dict__[key]
+                if val is attribute:
+                    stack.append(mgr)
+                    continue
+        return stack[-1]
+
     def _all_sqla_attributes(self, exclude=None):
         """return an iterator of all classbound attributes that are
         implement :class:`.InspectionAttr`.

diff --git a/test/ext/test_associationproxy.py b/test/ext/test_associationproxy.py
index 3ef09599cbec704f4c5791ead3707a85f1c58ded..d215bbc8d98288bcf8bf4834ffb6b9a8ef0f84c2 100644 (file)
--- a/test/ext/test_associationproxy.py
+++ b/test/ext/test_associationproxy.py
@@ -15,6 +15,7 @@ from sqlalchemy.testing.schema import Table, Column
 from sqlalchemy.testing.mock import Mock, call
 from sqlalchemy.testing.assertions import expect_warnings
 from sqlalchemy.ext.declarative import declarative_base
+from sqlalchemy.ext.declarative import declared_attr
 
 
 class DictCollection(dict):
@@ -1576,6 +1577,9 @@ class DictOfTupleUpdateTest(fixtures.TestBase):
 
 
 class AttributeAccessTest(fixtures.TestBase):
+    def teardown(self):
+        clear_mappers()
+
     def test_resolve_aliased_class(self):
         Base = declarative_base()
 
@@ -1599,6 +1603,160 @@ class AttributeAccessTest(fixtures.TestBase):
 
         is_(spec.owning_class, B)
 
+    def test_resolved_w_subclass(self):
+        # test for issue #4185, as well as several below
+
+        Base = declarative_base()
+
+        class Mixin(object):
+            @declared_attr
+            def children(cls):
+                return association_proxy('_children', 'value')
+
+        # 1. build parent, Mixin.children gets invoked, we add
+        # Parent.children
+        class Parent(Mixin, Base):
+            __tablename__ = 'parent'
+            id = Column(Integer, primary_key=True)
+
+            _children = relationship("Child")
+
+        class Child(Base):
+            __tablename__ = 'child'
+            parent_id = Column(
+                Integer, ForeignKey(Parent.id), primary_key=True)
+
+        # 2. declarative builds up SubParent, scans through all attributes
+        # over all classes.  Hits Mixin, hits "children", accesses "children"
+        # in terms of the class, e.g. SubParent.children.  SubParent isn't
+        # mapped yet.  association proxy then sets up "owning_class"
+        # as NoneType.
+        class SubParent(Parent):
+            __tablename__ = 'subparent'
+            id = Column(Integer, ForeignKey(Parent.id), primary_key=True)
+
+        configure_mappers()
+
+        # 3. which would break here.
+        p1 = Parent()
+        eq_(p1.children, [])
+
+    def test_resolved_to_correct_class_one(self):
+        Base = declarative_base()
+
+        class Parent(Base):
+            __tablename__ = 'parent'
+            id = Column(Integer, primary_key=True)
+            _children = relationship("Child")
+            children = association_proxy('_children', 'value')
+
+        class Child(Base):
+            __tablename__ = 'child'
+            parent_id = Column(
+                Integer, ForeignKey(Parent.id), primary_key=True)
+
+        class SubParent(Parent):
+            __tablename__ = 'subparent'
+            id = Column(Integer, ForeignKey(Parent.id), primary_key=True)
+
+        is_(SubParent.children.owning_class, Parent)
+
+    def test_resolved_to_correct_class_two(self):
+        Base = declarative_base()
+
+        class Parent(Base):
+            __tablename__ = 'parent'
+            id = Column(Integer, primary_key=True)
+            _children = relationship("Child")
+
+        class Child(Base):
+            __tablename__ = 'child'
+            parent_id = Column(
+                Integer, ForeignKey(Parent.id), primary_key=True)
+
+        class SubParent(Parent):
+            __tablename__ = 'subparent'
+            id = Column(Integer, ForeignKey(Parent.id), primary_key=True)
+            children = association_proxy('_children', 'value')
+
+        is_(SubParent.children.owning_class, SubParent)
+
+    def test_resolved_to_correct_class_three(self):
+        Base = declarative_base()
+
+        class Parent(Base):
+            __tablename__ = 'parent'
+            id = Column(Integer, primary_key=True)
+            _children = relationship("Child")
+
+        class Child(Base):
+            __tablename__ = 'child'
+            parent_id = Column(
+                Integer, ForeignKey(Parent.id), primary_key=True)
+
+        class SubParent(Parent):
+            __tablename__ = 'subparent'
+            id = Column(Integer, ForeignKey(Parent.id), primary_key=True)
+            children = association_proxy('_children', 'value')
+
+        class SubSubParent(SubParent):
+            __tablename__ = 'subsubparent'
+            id = Column(Integer, ForeignKey(SubParent.id), primary_key=True)
+
+        is_(SubSubParent.children.owning_class, SubParent)
+
+    def test_resolved_to_correct_class_four(self):
+        Base = declarative_base()
+
+        class Parent(Base):
+            __tablename__ = 'parent'
+            id = Column(Integer, primary_key=True)
+            _children = relationship("Child")
+            children = association_proxy(
+                '_children', 'value', creator=lambda value: Child(value=value))
+
+        class Child(Base):
+            __tablename__ = 'child'
+            parent_id = Column(
+                Integer, ForeignKey(Parent.id), primary_key=True)
+            value = Column(String)
+
+        class SubParent(Parent):
+            __tablename__ = 'subparent'
+            id = Column(Integer, ForeignKey(Parent.id), primary_key=True)
+
+        sp = SubParent()
+        sp.children = 'c'
+        is_(SubParent.children.owning_class, Parent)
+
+    def test_never_assign_nonetype(self):
+        foo = association_proxy('x', 'y')
+        foo._calc_owner(None, None)
+        is_(foo.owning_class, None)
+
+        class Bat(object):
+            foo = association_proxy('x', 'y')
+
+        Bat.foo
+        is_(Bat.foo.owning_class, None)
+
+        b1 = Bat()
+        assert_raises_message(
+            exc.InvalidRequestError,
+            "This association proxy has no mapped owning class; "
+            "can't locate a mapped property",
+            getattr, b1, "foo"
+        )
+        is_(Bat.foo.owning_class, None)
+
+        # after all that, we can map it
+        mapper(
+            Bat,
+            Table('bat', MetaData(), Column('x', Integer, primary_key=True)))
+
+        # answer is correct
+        is_(Bat.foo.owning_class, Bat)
+
 
 class InfoTest(fixtures.TestBase):
     def test_constructor(self):