Accommodate for classically mapped base classes in declarative

Fixed issue in previously untested use case, allowing a declarative mapped
class to inherit from a classically-mapped class outside of the declarative
base, including that it accommodates for unmapped intermediate classes. An
unmapped intermediate class may specify ``__abstract__``, which is now
interpreted correctly, or the intermediate class can remain unmarked, and
the classically mapped base class will be detected within the hierarchy
regardless. In order to anticipate existing scenarios which may be mixing
in classical mappings into existing declarative hierarchies, an error is
now raised if multiple mapped bases are detected for a given class.

Fixes: #4321
Change-Id: I8604ecfd170d2589d9d1b1c87ba303762071fc30

diff --git a/doc/build/changelog/unreleased_12/4321.rst b/doc/build/changelog/unreleased_12/4321.rst
new file mode 100644 (file)
index 0000000..edf65f2
--- /dev/null
+++ b/doc/build/changelog/unreleased_12/4321.rst
@@ -0,0 +1,13 @@
+.. change::
+    :tags: bug, orm, declarative
+    :tickets: 4321
+
+    Fixed issue in previously untested use case, allowing a declarative mapped
+    class to inherit from a classically-mapped class outside of the declarative
+    base, including that it accommodates for unmapped intermediate classes. An
+    unmapped intermediate class may specify ``__abstract__``, which is now
+    interpreted correctly, or the intermediate class can remain unmarked, and
+    the classically mapped base class will be detected within the hierarchy
+    regardless. In order to anticipate existing scenarios which may be mixing
+    in classical mappings into existing declarative hierarchies, an error is
+    now raised if multiple mapped bases are detected for a given class.

diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 544bb2497965fc7ffa0470299533bf585da03693..818b92c98a763750b3e72a280b61db119d64df60 100644 (file)
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -35,26 +35,50 @@ def _declared_mapping_info(cls):
         return None
 
 
-def _resolve_for_abstract(cls):
+def _resolve_for_abstract_or_classical(cls):
     if cls is object:
         return None
 
     if _get_immediate_cls_attr(cls, '__abstract__', strict=True):
         for sup in cls.__bases__:
-            sup = _resolve_for_abstract(sup)
+            sup = _resolve_for_abstract_or_classical(sup)
             if sup is not None:
                 return sup
         else:
             return None
     else:
+        classical = _dive_for_classically_mapped_class(cls)
+        if classical is not None:
+            return classical
+        else:
+            return cls
+
+
+def _dive_for_classically_mapped_class(cls):
+    # support issue #4321
+
+    # if we are within a base hierarchy, don't
+    # search at all for classical mappings
+    if hasattr(cls, '_decl_class_registry'):
+        return None
+
+    manager = instrumentation.manager_of_class(cls)
+    if manager is not None:
         return cls
+    else:
+        for sup in cls.__bases__:
+            mapper = _dive_for_classically_mapped_class(sup)
+            if mapper is not None:
+                return sup
+        else:
+            return None
 
 
 def _get_immediate_cls_attr(cls, attrname, strict=False):
     """return an attribute of the class that is either present directly
     on the class, e.g. not on a superclass, or is from a superclass but
-    this superclass is a mixin, that is, not a descendant of
-    the declarative base.
+    this superclass is a non-mapped mixin, that is, not a descendant of
+    the declarative base and is also not classically mapped.
 
     This is used to detect attributes that indicate something about
     a mapped class independently from any mapped classes that it may
@@ -66,10 +90,14 @@ def _get_immediate_cls_attr(cls, attrname, strict=False):
 
     for base in cls.__mro__:
         _is_declarative_inherits = hasattr(base, '_decl_class_registry')
+        _is_classicial_inherits = not _is_declarative_inherits and \
+            _dive_for_classically_mapped_class(base) is not None
+
         if attrname in base.__dict__ and (
             base is cls or
             ((base in cls.__bases__ if strict else True)
-                and not _is_declarative_inherits)
+                and not _is_declarative_inherits
+                and not _is_classicial_inherits)
         ):
             return getattr(base, attrname)
     else:
@@ -451,15 +479,24 @@ class _MapperConfig(object):
         cls = self.cls
         table_args = self.table_args
         declared_columns = self.declared_columns
+
+        # since we search for classical mappings now, search for
+        # multiple mapped bases as well and raise an error.
+        inherits = []
         for c in cls.__bases__:
-            c = _resolve_for_abstract(c)
+            c = _resolve_for_abstract_or_classical(c)
             if c is None:
                 continue
             if _declared_mapping_info(c) is not None and \
                     not _get_immediate_cls_attr(
                         c, '_sa_decl_prepare_nocascade', strict=True):
-                self.inherits = c
-                break
+                inherits.append(c)
+
+        if inherits:
+            if len(inherits) > 1:
+                raise exc.InvalidRequestError(
+                    "Class %s has multiple mapped bases: %r" % (cls, inherits))
+            self.inherits = inherits[0]
         else:
             self.inherits = None
 

diff --git a/test/ext/declarative/test_inheritance.py b/test/ext/declarative/test_inheritance.py
index 7ced8ec157bb672b99c04c4a62a63167554f1e1b..c310b725f4feaaaa850ee9860b395d9f2c822130 100644 (file)
--- a/test/ext/declarative/test_inheritance.py
+++ b/test/ext/declarative/test_inheritance.py
@@ -1,5 +1,5 @@
 
-from sqlalchemy.testing import eq_, assert_raises, \
+from sqlalchemy.testing import eq_, le_, assert_raises, \
     assert_raises_message, is_, is_true, is_false
 from sqlalchemy.ext import declarative as decl
 import sqlalchemy as sa
@@ -8,7 +8,7 @@ from sqlalchemy import Integer, String, ForeignKey
 from sqlalchemy.testing.schema import Table, Column
 from sqlalchemy.orm import relationship, create_session, class_mapper, \
     configure_mappers, clear_mappers, \
-    polymorphic_union, deferred, Session
+    polymorphic_union, deferred, Session, mapper
 from sqlalchemy.ext.declarative import declared_attr, AbstractConcreteBase, \
     ConcreteBase, has_inherited_table
 from sqlalchemy.testing import fixtures, mock
@@ -290,6 +290,95 @@ class DeclarativeInheritanceTest(DeclarativeTestBase):
 
         assert class_mapper(Engineer).inherits is class_mapper(Person)
 
+    def test_intermediate_abstract_class_on_classical(self):
+        class Person(object):
+            pass
+
+        person_table = Table('people', Base.metadata,
+                             Column('id', Integer, primary_key=True),
+                             Column('kind', String(50)))
+
+        mapper(Person, person_table,
+               polymorphic_on='kind', polymorphic_identity='person')
+
+        class SpecialPerson(Person):
+            __abstract__ = True
+
+        class Manager(SpecialPerson, Base):
+            __tablename__ = 'managers'
+            id = Column(Integer, ForeignKey(Person.id), primary_key=True)
+            __mapper_args__ = {
+                'polymorphic_identity': 'manager'
+            }
+
+        from sqlalchemy import inspect
+        assert inspect(Manager).inherits is inspect(Person)
+
+        eq_(set(class_mapper(Person).class_manager), {'id', 'kind'})
+        eq_(set(class_mapper(Manager).class_manager), {'id', 'kind'})
+
+    def test_intermediate_unmapped_class_on_classical(self):
+        class Person(object):
+            pass
+
+        person_table = Table('people', Base.metadata,
+                             Column('id', Integer, primary_key=True),
+                             Column('kind', String(50)))
+
+        mapper(Person, person_table,
+               polymorphic_on='kind', polymorphic_identity='person')
+
+        class SpecialPerson(Person):
+            pass
+
+        class Manager(SpecialPerson, Base):
+            __tablename__ = 'managers'
+            id = Column(Integer, ForeignKey(Person.id), primary_key=True)
+            __mapper_args__ = {
+                'polymorphic_identity': 'manager'
+            }
+
+        from sqlalchemy import inspect
+        assert inspect(Manager).inherits is inspect(Person)
+
+        eq_(set(class_mapper(Person).class_manager), {'id', 'kind'})
+        eq_(set(class_mapper(Manager).class_manager), {'id', 'kind'})
+
+    def test_class_w_invalid_multiple_bases(self):
+        class Person(object):
+            pass
+
+        person_table = Table('people', Base.metadata,
+                             Column('id', Integer, primary_key=True),
+                             Column('kind', String(50)))
+
+        mapper(Person, person_table,
+               polymorphic_on='kind', polymorphic_identity='person')
+
+        class DeclPerson(Base):
+            __tablename__ = 'decl_people'
+            id = Column(Integer, primary_key=True)
+            kind = Column(String(50))
+
+        class SpecialPerson(Person):
+            pass
+
+        def go():
+            class Manager(SpecialPerson, DeclPerson):
+                __tablename__ = 'managers'
+                id = Column(Integer,
+                            ForeignKey(DeclPerson.id), primary_key=True)
+                __mapper_args__ = {
+                    'polymorphic_identity': 'manager'
+                }
+
+        assert_raises_message(
+            sa.exc.InvalidRequestError,
+            r"Class .*Manager.* has multiple mapped "
+            r"bases: \[.*Person.*DeclPerson.*\]",
+            go
+        )
+
     def test_with_undefined_foreignkey(self):
 
         class Parent(Base):