`__declare_last__` and multiple mixins

[Jack Zhou]

Currently, declarative appears to only support a single mixin with __declare_last__ defined.

The relevant code from 0.9.8:

    for base in cls.__mro__:
        _is_declarative_inherits = hasattr(base, '_decl_class_registry')

        if '__declare_last__' in base.__dict__:
            @event.listens_for(mapper, "after_configured")
            def go():
                cls.__declare_last__()
        if '__declare_first__' in base.__dict__:
            @event.listens_for(mapper, "before_configured")
            def go():
                cls.__declare_first__()

As you can see, even though it inspects each class in the MRO, declarative calls cls.__declare_last__, which happens to be the first base class in the MRO that defines __declare_last__; it also calls it multiple times, as many times as there are bases that define __declare_last__. (master seems to have fixed the multiple calls issue, but is still limited to a single __declare_last__.)

I think it's better to allow each base class in the MRO to define its own __declare_last__:

    for base in cls.__mro__:
        _is_declarative_inherits = hasattr(base, '_decl_class_registry')

        if '__declare_last__' in base.__dict__:
            def closure(base):
                @event.listens_for(mapper, "after_configured")
                def go():
                    base.__dict__["__declare_last__"].__func__(cls)
            closure(base)
        if '__declare_first__' in base.__dict__:
            def closure(base):
                @event.listens_for(mapper, "before_configured")
                def go():
                    base.__dict__["__declare_first__"].__func__(cls)
            closure(base)

To me, this is more intuitive and useful for mixins. One downside I see is not being able to override the behavior of a mixin in a base class, which can potentially be troublesome. If you want to go with the multiple __declare_last__ approach, I'd be glad to provide a patch for master. Otherwise, feel free to close the issue; I can implement this in my own Base metaclass.

[Mike Bayer]

this seems intuitive to me:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class MixinOne(object):
    @classmethod
    def __declare_last__(cls):
        print "mixinone for %s" % cls


class A(MixinOne, Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)

    @classmethod
    def __declare_last__(cls):
        super(A, cls).__declare_last__()
        print "a"


class B(MixinOne, Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)

    @classmethod
    def __declare_last__(cls):
        print "b"


class C(MixinOne, Base):
    __tablename__ = 'c'
    id = Column(Integer, primary_key=True)

configure_mappers()

#!


mixinone for <class '__main__.A'>
a
b
mixinone for <class '__main__.C'>

works the same as Python:

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base

Base = declarative_base()


class MixinOne(object):
    @classmethod
    def propa(cls):
        print "mixinone for %s" % cls


class A(MixinOne, Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)

    @classmethod
    def propa(cls):
        super(A, cls).propa()
        print "a"


class B(MixinOne, Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)

    @classmethod
    def propa(cls):
        print "b"


class C(MixinOne, Base):
    __tablename__ = 'c'
    id = Column(Integer, primary_key=True)

for cls in (A, B, C):
    cls.propa()

#!

mixinone for <class '__main__.A'>
a
b
mixinone for <class '__main__.C'>

and more or less the same for @declared_attr, though we have to work a bit to get at "super":

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base, declared_attr

Base = declarative_base()


class MixinOne(object):
    @declared_attr
    def propa(cls):
        print "mixinone for %s" % cls


class A(MixinOne, Base):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)

    @declared_attr
    def propa(cls):
        prop = MixinOne.__dict__['propa']
        prop.__get__(None, cls)
        print "a"


class B(MixinOne, Base):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)

    @declared_attr
    def propa(cls):
        print "b"


class C(MixinOne, Base):
    __tablename__ = 'c'
    id = Column(Integer, primary_key=True)

#!


mixinone for <class '__main__.A'>
a
b
mixinone for <class '__main__.C'>

that is, in all these cases, when a class defines a method, that is now the single location that the method is callable from the outside. The class itself is responsible for calling upon super(). It's how any classmethod works in any system.

the fix that __declare_last__() and others is correctly evented only once is a currently undocumented part of the series of changes described in http://docs.sqlalchemy.org/en/latest/changelog/migration_10.html#improvements-to-declarative-mixins-declared-attr-and-related-features. I'd wait until 1.0 before using some of these declarative features with more power, the entire approach has been vastly improved.

[Mike Bayer]

also, if you truly want a mixin that has some declare_last thing that happens unconditionally, event.listen() for the "after_configured" event directly.

[Jack Zhou]

The super approach, while valid, unfortunately requires all mixins implement that method, and breaks as soon as one class in the MRO does not (e.g. Base), which seems a bit heavy:

class Base(object):
    pass


class MixinOne(object):
    @classmethod
    def propa(cls):
        print "mixinone for %s" % cls
        super(MixinOne, cls).propa()


class MixinTwo(object):
    @classmethod
    def propa(cls):
        print "mixintwo for %s" % cls
        super(MixinTwo, cls).propa()


class A(MixinOne, MixinTwo, Base):
    @classmethod
    def propa(cls):
        print "a"
        super(A, cls).propa()


A().propa()  # AttributeError: 'super' object has no attribute 'propa'

Re: your second point, I want to event.listen() for the after_configured event for all classes that inherit my mixin, so this has to be done in __declare_last__ (or a similar per-class hook).

[Mike Bayer]

The super approach, while valid, unfortunately requires all mixins implement that method, and breaks as soon as one class in the MRO does not (e.g. Base), which seems a bit heavy

nevertheless, that's how Python works. If you're running unittest, or nose, or pytest, your test classes may define a @classmethod setUpClass. What if you have a hierarchy of fixtures, and some of the "super" fixtures also define setUpClass? People would not expect the test runners to redefine Python's behavior here:

import unittest


class TestBaseOne(object):
    @classmethod
    def setUpClass(cls):
        print "testbaseone setupclass"


class TestBaseTwo(object):
    @classmethod
    def setUpClass(cls):
        print "testbasetwo setupclass"


class MyTestCase(TestBaseOne, TestBaseTwo, unittest.TestCase):
    @classmethod
    def setUpClass(cls):
        print "MyTestCase setupclass"

    def test_something(self):
        print "test something"


if __name__ == '__main__':
    unittest.main()

is the above a bug in unittest? It doesn't make sense for just certain @classmethods to act in a surprising way, totally different than any other classmethod, and more people would report this as a bug than not. If we decide that __declare_last__() and such just aren't useful this way, then I'd rather just deprecate them, because they're only confusing and misleading that this is the best way to create fine-grained mapper configuration behavior; they are probably the worst - the only reason they exist is so that I could build AbstractConcreteBase / ConcreteBase and have them set up an event automatically, but i could just as well do that nowadays using mapper_configured() with propagate=True, as events can be placed on mixins now. Those base classes would suffer from the same problem, if the user redefined these methods, so there is an issue here, which is that these methods even exist as documented.

Re: your second point, I want to event.listen() for the after_configured event for all classes that inherit my mixin, so this has to be done in declare_last (or a similar per-class hook).

you can find all classes that inherit from your mixin using cls.subclasses() recursively. But it also might be more appropriate to use the "mapper_configured" event instead, as that is per-mapper/class, and with propagate=True can be made to be called for all classes.

[Mike Bayer]

heh. I just looked into seeing if I could do away with __declare_first__() for the concretebases, and as it turns out they are actually making use of this method as it works right now, abstractconcretebase is superseding that of concretebase and it assumes it is an overriding method :).

[Jack Zhou]

Okay, fair enough. It really comes down to preference anyway. :)

[Mike Bayer]

taking a page from unittest, there's always this also:

diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py
index 66fe05f..03d1406 100644
--- a/lib/sqlalchemy/ext/declarative/api.py
+++ b/lib/sqlalchemy/ext/declarative/api.py
@@ -251,7 +251,18 @@ class _stateful_declared_attr(declared_attr):
         return declared_attr(fn, **self.kw)


-def declarative_base(bind=None, metadata=None, mapper=None, cls=object,
+class BaseDeclarative(object):
+    @classmethod
+    def __declare_last__(cls):
+        pass
+
+    @classmethod
+    def __declare_first__(cls):
+        pass
+
+
+def declarative_base(bind=None, metadata=None, mapper=None,
+                     cls=BaseDeclarative,
                      name='Base', constructor=_declarative_constructor,
                      class_registry=None,
                      metaclass=DeclarativeMeta):
diff --git a/test/ext/declarative/test_inheritance.py b/test/ext/declarative/test_inheritance.py
index 6ea37e4..74da9a4 100644
--- a/test/ext/declarative/test_inheritance.py
+++ b/test/ext/declarative/test_inheritance.py
@@ -1389,7 +1389,7 @@ class ConcreteExtensionConfigTest(
         )

     def test_abstract_in_hierarchy(self):
-        class Document(Base, AbstractConcreteBase):
+        class Document(AbstractConcreteBase, Base):
             doctype = Column(String)

         class ContactDocument(Document):

I don't like the idea of changing the default base of "Base" but that would make this case automatic. You can of course specify this base class yourself, but if your use case is like that of a 3rd party mixin then i can see why you might be going for something more auto.