Test attributes for being non-mapped column properties more closely

Fixed bug in concrete inheritance mapping where user-defined
attributes such as hybrid properties that mirror the names
of mapped attributes from sibling classes would be overwritten by
the mapper as non-accessible at the instance level.  Also
ensured that user-bound descriptors are not implicitly invoked at the class
level during the mapper configuration stage.

Change-Id: I52b84a15c296b14efeaffb72941fc941d1d52c0d
Fixes: #4188

diff --git a/doc/build/changelog/unreleased_12/4188.rst b/doc/build/changelog/unreleased_12/4188.rst
new file mode 100644 (file)
index 0000000..fd10854
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4188.rst
@@ -0,0 +1,10 @@
+.. change::
+    :tags: bug, orm
+    :tickets: 4188
+
+    Fixed bug in concrete inheritance mapping where user-defined
+    attributes such as hybrid properties that mirror the names
+    of mapped attributes from sibling classes would be overwritten by
+    the mapper as non-accessible at the instance level.   Additionally
+    ensured that user-bound descriptors are not implicitly invoked at the class
+    level during the mapper configuration stage.

diff --git a/lib/sqlalchemy/orm/instrumentation.py b/lib/sqlalchemy/orm/instrumentation.py
index d5422b0d002bd904e7350b9e81736a3ffa8f03c3..1b839cf5c351d4b467a23895bfb138f02df31ddf 100644 (file)
--- a/lib/sqlalchemy/orm/instrumentation.py
+++ b/lib/sqlalchemy/orm/instrumentation.py
@@ -168,6 +168,15 @@ class ClassManager(dict):
                 if isinstance(val, interfaces.InspectionAttr):
                     yield key, val
 
+    def _get_class_attr_mro(self, key, default=None):
+        """return an attribute on the class without tripping it."""
+
+        for supercls in self.class_.__mro__:
+            if key in supercls.__dict__:
+                return supercls.__dict__[key]
+        else:
+            return default
+
     def _attr_has_impl(self, key):
         """Return True if the given attribute is fully initialized.
 

diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index cd9e00b8b0155fe53f4e96a8ce3b2217f01c4c6c..a30a8c2433b656aca476d20f326e4763ffe835c6 100644 (file)
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -1613,10 +1613,23 @@ class Mapper(InspectionAttr):
         if not self.concrete:
             self._configure_property(key, prop, init=False, setparent=False)
         elif key not in self._props:
-            self._configure_property(
-                key,
-                properties.ConcreteInheritedProperty(),
-                init=init, setparent=True)
+            # determine if the class implements this attribute; if not,
+            # or if it is implemented by the attribute that is handling the
+            # given superclass-mapped property, then we need to report that we
+            # can't use this at the instance level since we are a concrete
+            # mapper and we don't map this.  don't trip user-defined
+            # descriptors that might have side effects when invoked.
+            implementing_attribute = self.class_manager._get_class_attr_mro(
+                key, prop)
+            if implementing_attribute is prop or (isinstance(
+                    implementing_attribute,
+                    attributes.InstrumentedAttribute) and
+                implementing_attribute._parententity is prop.parent
+            ):
+                self._configure_property(
+                    key,
+                    properties.ConcreteInheritedProperty(),
+                    init=init, setparent=True)
 
     def _configure_property(self, key, prop, init=True, setparent=True):
         self._log("_configure_property(%s, %s)", key, prop.__class__.__name__)
@@ -2413,9 +2426,8 @@ class Mapper(InspectionAttr):
                     self.class_.__dict__[assigned_name]):
                 return True
         else:
-            if getattr(self.class_, assigned_name, None) is not None \
-                    and self._is_userland_descriptor(
-                    getattr(self.class_, assigned_name)):
+            attr = self.class_manager._get_class_attr_mro(assigned_name, None)
+            if attr is not None and self._is_userland_descriptor(attr):
                 return True
 
         if self.include_properties is not None and \

diff --git a/test/orm/inheritance/test_concrete.py b/test/orm/inheritance/test_concrete.py
index b103bad7beb4c858edea8eb886da9dd36893c822..edf2c4bdc31ca024e018b70edd33779ca5162381 100644 (file)
--- a/test/orm/inheritance/test_concrete.py
+++ b/test/orm/inheritance/test_concrete.py
@@ -10,6 +10,8 @@ from sqlalchemy.testing import fixtures
 from sqlalchemy.orm import attributes
 from sqlalchemy.testing import eq_
 from sqlalchemy.testing.schema import Table, Column
+from sqlalchemy.ext.hybrid import hybrid_property
+from sqlalchemy.testing import mock
 
 
 class Employee(object):
@@ -217,6 +219,73 @@ class ConcreteTest(fixtures.MappedTest):
         assert set([repr(x) for x in session.query(Hacker).all()]) \
             == set(["Hacker Kurt 'Badass' knows how to hack"])
 
+    def test_multi_level_no_base_w_hybrid(self):
+        pjoin = polymorphic_union(
+            {'manager': managers_table, 'engineer': engineers_table,
+             'hacker': hackers_table},
+            'type', 'pjoin')
+
+        test_calls = mock.Mock()
+
+        class ManagerWHybrid(Employee):
+
+            def __init__(self, name, manager_data):
+                self.name = name
+                self.manager_data = manager_data
+
+            @hybrid_property
+            def engineer_info(self):
+                test_calls.engineer_info_instance()
+                return self.manager_data
+
+            @engineer_info.expression
+            def engineer_info(cls):
+                test_calls.engineer_info_class()
+                return cls.manager_data
+
+        employee_mapper = mapper(Employee, pjoin,
+                                 polymorphic_on=pjoin.c.type)
+        mapper(
+            ManagerWHybrid, managers_table,
+            inherits=employee_mapper,
+            concrete=True,
+            polymorphic_identity='manager')
+        mapper(
+            Engineer,
+            engineers_table,
+            inherits=employee_mapper,
+            concrete=True,
+            polymorphic_identity='engineer')
+
+        session = create_session()
+        tom = ManagerWHybrid('Tom', 'mgrdata')
+
+        # mapping did not impact the engineer_info
+        # hybrid in any way
+        eq_(test_calls.mock_calls, [])
+
+        eq_(
+            tom.engineer_info, "mgrdata"
+        )
+        eq_(test_calls.mock_calls, [mock.call.engineer_info_instance()])
+
+        session.add(tom)
+        session.flush()
+
+        session.close()
+
+        tom = session.query(ManagerWHybrid).filter(
+            ManagerWHybrid.engineer_info == 'mgrdata').one()
+        eq_(
+            test_calls.mock_calls,
+            [
+                mock.call.engineer_info_instance(),
+                mock.call.engineer_info_class()]
+        )
+        eq_(
+            tom.engineer_info, "mgrdata"
+        )
+
     def test_multi_level_with_base(self):
         pjoin = polymorphic_union({
             'employee': employees_table,