declared_attr.cascading unconditionally sets attribute

[David Lord]

I'm not sure if this is a bug or just a result of me misunderstanding the documentation. I would expect that a cascading declared attr would not overwrite an attribute that's manually defined on a class, but that's not the case. Instead, the declared attr always replaces the existing attribute.

This makes it less useful for things like __tablename__ (with #4090), where I would like the user to be able to set their own tablename to override the default.

import sqlalchemy as sa
from sqlalchemy.ext.declarative import declarative_base, declared_attr, \
    has_inherited_table

class Base:
    @declared_attr.cascading
    def id(cls):
        if has_inherited_table(cls):
            type = sa.ForeignKey(cls.__mro__[1].id)
        else:
            type = sa.Integer

        return sa.Column(type, primary_key=True)

Base = declarative_base(cls=Base)

class User(Base):
    __tablename__ = 'user'

class User2(User):
    __tablename__ = 'user2'
    # due to cascading, the created name is 'id' instead of 'user_id'
    id = sa.Column('user_id', sa.ForeignKey(User.id), primary_key=True)

engine = sa.create_engine('sqlite://', echo=True)
Base.metadata.create_all(engine)

[Mike Bayer]

haha OK yes, so with #4090 you found this confusing thing which is that .cascading isn't something declarative checks with __tablename__, but with columns, it's important. this is because "cascading" was added later on to accommodate for this use case with columns. the logic for normally-named attributes is much more elaborate than it is for simple things like __tablename__ because these attributes can point to anything, whereas __tablename__ is a special name.

in this case the behavior you have found was probably not intended, though I have a feeling you don't actually need this use case? an easy way to not break people's code for the moment would be to have it emit a warning for this scenario. would need to think about if people are actually trying to do this and if just changing it is safe.

[Mike Bayer]

welp, this case is totally uncovered in the tests, if i assert the assumption made here, nothing fails:

diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 16cb05e9c..b7d30c31f 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -220,6 +220,8 @@ class _MapperConfig(object):
                     elif isinstance(obj, declarative_props):
                         oldclassprop = isinstance(obj, util.classproperty)
                         if not oldclassprop and obj._cascading:
+                            if name in dict_:
+                                assert dict_.get(name)  is obj
                             dict_[name] = column_copies[obj] = \
                                 ret = obj.__get__(obj, cls)
                             setattr(cls, name, ret)

but your case of course does! will see if i can stick this in.

[Mike Bayer]

by "this" i mean, "don't replace the key that's there if it isnt the cascading object"

[Mike Bayer]

unfortunately while I can get your example to work, I can't get it to work if there's a further subclass below User2, it wants to go back to the @declared_attr.cascading. I can't figure out the class mechanics for this right now or if i need to propagate more information throughout the declarative API, etc. so in https://gerrit.sqlalchemy.org/530 there is just the warning to prevent people from doing this for now.