From: Mike Bayer Date: Fri, 17 Aug 2018 15:37:30 +0000 (-0400) Subject: Accommodate for classically mapped base classes in declarative X-Git-Tag: rel_1_3_0b1~104^2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=4c931b2ec7e0f09ac8c3ebe28c794f5858d54efb;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 index 0000000000..edf65f2f9e --- /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 544bb24979..818b92c98a 100644 --- 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 7ced8ec157..c310b725f4 100644 --- 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):