declared_attr with abstractconcretebase

Issue #2670 resolved
Mike Bayer repo owner created an issue

because it's not mapped up front @declared_attr isn't evaluated as expected. might need the plugin to run them again.

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import (declarative_base, declared_attr,
    AbstractConcreteBase)

engine = create_engine('sqlite://', echo=True)
Base = declarative_base()

class Something(Base):
    __tablename__ = u'something'
    id = Column(Integer, primary_key=True)


class AbstractConcreteAbstraction(AbstractConcreteBase, Base):
    id = Column(Integer, primary_key=True)
    derpa = Column(Integer)
    derp = Column(Integer)

    @declared_attr
    def something_id(cls):
        return Column(ForeignKey(Something.id))

    @declared_attr
    def something(cls):
        return relationship(Something)


class ConcreteConcreteAbstraction(AbstractConcreteAbstraction):
    __tablename__ = u'cca'
    __mapper_args__ = {'polymorphic_identity': 'ccb',
        'concrete': True}


Base.metadata.create_all(engine)

session = Session(engine)
print session.query(ConcreteConcreteAbstraction).filter(
        ConcreteConcreteAbstraction.something.has(id=1)).all()

# workaround needed
# AbstractConcreteAbstraction.something = relationship(Something)

print session.query(AbstractConcreteAbstraction).filter(
        AbstractConcreteAbstraction.something.has(id=1)).all()

Comments (16)

  1. Mike Bayer reporter

    a patch is attached but its kind of lame, I'm actually not sure if we are handling the relationship here correctly, in terms of it being created twice against abstract + subclass. the whole way abstractconcretebase works might not actually work fully.

  2. Mike Bayer reporter

    to distinguish between those attrs that get mixed in to subclasses, and those that are on the base, we might need new API for this, like:

    class AbstractConcreteAbstraction(AbstractConcreteBase, Base):
        id = Column(Integer, primary_key=True)
        derpa = Column(Integer)
        derp = Column(Integer)
    
        @declared_attr
        def something_id(cls):
            return Column(ForeignKey(Something.id))
    
        @classmethod
        def base_properties(cls):
            return {
                "something": relationship(Something)
            }
    
  3. Mike Bayer reporter

    here's the absolute least intrusive way to do this, a new directive explicit for those attribs we want in the base also:

    diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py
    index 941f02b..30c3a69 100644
    --- a/lib/sqlalchemy/ext/declarative/api.py
    +++ b/lib/sqlalchemy/ext/declarative/api.py
    @@ -12,6 +12,7 @@ from ...orm import synonym as _orm_synonym, mapper,\
                                     interfaces, properties
     from ...orm.util import polymorphic_union
     from ...orm.base import _mapper_or_none
    +from ...orm.interfaces import MapperProperty
     from ...util import OrderedDict
     from ... import exc
     import weakref
    @@ -370,6 +371,15 @@ class AbstractConcreteBase(ConcreteBase):
         __abstract__ = True
    
         @classmethod
    +    def apply_to_base(cls, fn):
    +        if not isinstance(fn, declared_attr):
    +            raise exc.InvalidRequestError(
    +                            "apply_to_base argument must also be "
    +                            "@declared_attr")
    +        fn._sa_apply_to_base = True
    +        return fn
    +
    +    @classmethod
         def __declare_first__(cls):
             if hasattr(cls, '__mapper__'):
                 return
    @@ -388,7 +398,21 @@ class AbstractConcreteBase(ConcreteBase):
                 if mn is not None:
                     mappers.append(mn)
             pjoin = cls._create_polymorphic_union(mappers)
    -        cls.__mapper__ = m = mapper(cls, pjoin, polymorphic_on=pjoin.c.type)
    +
    +        props = {}
    +        for base in cls.__mro__:
    +            for name, obj in vars(base).items():
    +                if hasattr(obj, '_sa_apply_to_base'):
    +                    attr = getattr(cls, name)
    +                    if not isinstance(attr, MapperProperty):
    +                        raise exc.InvalidRequestError(
    +                                    "Expected mapped property for "
    +                                    "attribute '%s'" % name)
    +                    props[name] = attr
    +
    +        cls.__mapper__ = m = mapper(cls, pjoin,
    +                                polymorphic_on=pjoin.c.type,
    +                                properties=props)
    
             for scls in cls.__subclasses__():
                 sm = _mapper_or_none(scls)
    

    e.g.

    class AbstractConcreteAbstraction(AbstractConcreteBase, Base):
        # ...
        @AbstractConcreteBase.apply_to_base
        @declared_attr
        def something(cls):
            return relationship(Something)
    
  4. Mike Bayer reporter
    • changed status to open

    funny, what we're doing in #3150 is one way to go but as I worked through this, I've come back to a fix that doesn't need #3150's feature again :)

  5. Mike Bayer reporter
    • refactor of declarative, break up into indiviudal methods that are now affixed to _MapperConfig
    • declarative now creates column copies ahead of time so that they are ready to go for a declared_attr
    • overhaul of declared_attr; memoization, cascading modifier
    • A relationship set up with :class:.declared_attr on a :class:.AbstractConcreteBase base class will now be configured on the abstract base mapping automatically, in addition to being set up on descendant concrete classes as usual. fixes #2670
    • The :class:.declared_attr construct has newly improved behaviors and features in conjunction with declarative. The decorated function will now have access to the final column copies present on the local mixin when invoked, and will also be invoked exactly once for each mapped class, the returned result being memoized. A new modifier :attr:.declared_attr.cascading is added as well. fixes #3150
    • the original plan for #3150 has been scaled back; by copying mixin columns up front and memoizing, we don't actually need the "map properties later" thing.
    • full docs + migration notes

    → <<cset 7f82c55fa764>>

  6. Log in to comment