Bad error on model with __table__ and mixin with different column

[Former user]

With a declarative model and mixin like this:

        class ColumnMixin:
            data = Column(Integer)

        class Model(Base, ColumnMixin):
            __table__ = Table(
                'foo',
                Base.metadata,
                Column('data', Integer),
                Column('id', Integer, primary_key=True) 
                )

… this undescriptive and unnecessary error comes up:

    […](…)
      File "/Users/gthb/extsvn/sqlalchemy/lib/sqlalchemy/ext/declarative.py", line 832, in __init__
        _as_declarative(cls, classname, cls.__dict__)
      File "/Users/gthb/extsvn/sqlalchemy/lib/sqlalchemy/ext/declarative.py", line 683, in _as_declarative
        if tablename or k not in parent_columns:
    TypeError: Error when calling the metaclass bases
        argument of type 'NoneType' is not iterable

The real reason is that the case of a model having __table__ and a mixin is not supported.

The attached patch lets that case work, and also prevents this error from being raised in the case of a mixin containing a column not represented in table (an already defined ArgumentError then gets raised instead).

[Former user]

Patch resolving this ticket, with tests

[Former user]

Sorry, formatting all screwed up and I can't fix because I'm “guest” :)

[Mike Bayer]

I would expect that the Column being present on the mixin, and then using __table__, should raise an error unconditionally. Otherwise it feels like two configurational styles are being mixed, and "ignoring" the column on the mixin may be perceived as silent failure.

[Former user]

I regard it as the __table__ fulfilling the mixin's promise of that column being present (and possibly overriding its other properties) — and if it doesn't, then you get an error. I think this is a valid use case you are proposing to break; my system generates table definitions dynamically, and then maps them declaratively with a mix-in containing the columns that are common to all those dynamically created tables. This is the natural way to do that.

I get that you want to avoid the silent failure if people do this inadvertently. Would a warning be an acceptable compromise, so that the user can just set a warning filter to assert that they are doing this consciously?

[Mike Bayer]

Replying to guest:

I regard it as the __table__ fulfilling the mixin's promise of that column being present (and possibly overriding its other properties) — and if it doesn't, then you get an error. I think this is a valid use case you are proposing to break; my system generates table definitions dynamically, and then maps them declaratively with a mix-in containing the columns that are common to all those dynamically created tables. This is the natural way to do that.

if you are generating Table objects that have a set of columns that are common to all of them what is the purpose of the mixin ?

[Former user]

It defines the common columns and shared methods involving them. Other code references the mix-in (base class) without needing to know about the models extending it. I was doing this with a metaclass extending DeclarativeMeta, but changed to the mix-in approach because it's cleaner and doesn't throw off IDE autocompletion and code validation tools.

[Former user]

Even if you end up deciding not to support this, you should still apply that first line change of the patch (the empty tuple () instead of a None). It averts the bad error, letting a clearer one (ArgumentError with descriptive message) be raised instead.

[Mike Bayer]

agreed, improving the error message is non-controversial.

I guess you want the columns on your mixin as a way of raising an error if the __table__ fails to provide them, is that it ? It just seems a little strange so far. Probably OK, I'm not sure the warning would be needed.

[Former user]

Yeah, that's it (or half of it really: there are also compile-time-defined models extending the mixin with __tablename__ and extra Columns, but I needed this for the dynamic ones.).

It is a little strange, true. Most apps will not dynamically add and map tables at runtime, but that's kind of the nature of our domain. (By the same token there will probably be few people bugging you about any issues arising from supporting this!)

I did work around this (rather than go install a patched version of SQLAlchemy in production) by splitting apart the mixin, something like:

class MixinSansColumns(object):
    # define all the other stuff
class Mixin(MixinSansColumns):
    # define the Column attributes

and deriving the generated class directly from MixinSansColumns, while the statically-defined ones extend Mixin and get the shared columns from there.

It's a bit of a kludgy workaround though. Using the unchanged mixin would be a tad cleaner, and this patch seems fairly safe to me (fwiw) — but full ack that you are the judge of that.

[Mike Bayer]

6de7db07cecd7f6f86452cdc64270c83b5bef212

[Mike Bayer]

Removing milestone: 0.6.2 (automated comment)