- edited description
declared_attr.cascading unconditionally sets attribute
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)
Comments (5)
-
reporter -
repo owner 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.
-
repo owner - changed milestone to 1.2
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.
-
repo owner by "this" i mean, "don't replace the key that's there if it isnt the cascading object"
-
repo owner 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.
- Log in to comment