declared_attr.cascading doesn't work for __tablename__

[David Lord]

I noticed this while fixing tablename generation in Flask-SQLAlchemy. The reason I use the metaclass instead of declared_attr is because the cascading option doesn't work for __tablename__. Instead, it's treated like a normal declared attr, where once an intermediate class sets a value, that's used going forward.

You also showed declared_attr.cascading for __tablename__ in the docs for __table_cls__, which won't work as intended: https://gerrit.sqlalchemy.org/plugins/gitiles/zzzeek/sqlalchemy/+/refs/changes/27/527/2/doc/build/orm/extensions/declarative/api.rst#137

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

class Base:
    @declared_attr.cascading
    def __tablename__(cls):
        return cls.__name__.lower()

Base = declarative_base(cls=Base)

class User(Base):
    id = sa.Column(sa.Integer, primary_key=True)

class User2(User):
    __tablename__ = 'user2'
    id = sa.Column(sa.ForeignKey(User.id), primary_key=True)

class User3(User2):
    # I would expect __tablename__ = 'user3' here
    # but sqla keeps 'user2'
    id = sa.Column(sa.ForeignKey(User2.id), primary_key=True)

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

Traceback (most recent call last):
  File "/home/david/Projects/flask-sqlalchemy/example.py", line 23, in <module>
    class User3(User2):
  File "/home/david/.virtualenvs/flask-sqlalchemy/lib/python3.6/site-packages/sqlalchemy/ext/declarative/api.py", line 64, in __init__
    _as_declarative(cls, classname, cls.__dict__)
  File "/home/david/.virtualenvs/flask-sqlalchemy/lib/python3.6/site-packages/sqlalchemy/ext/declarative/base.py", line 88, in _as_declarative
    _MapperConfig.setup_mapping(cls, classname, dict_)
  File "/home/david/.virtualenvs/flask-sqlalchemy/lib/python3.6/site-packages/sqlalchemy/ext/declarative/base.py", line 103, in setup_mapping
    cfg_cls(cls_, classname, dict_)
  File "/home/david/.virtualenvs/flask-sqlalchemy/lib/python3.6/site-packages/sqlalchemy/ext/declarative/base.py", line 131, in __init__
    self._setup_table()
  File "/home/david/.virtualenvs/flask-sqlalchemy/lib/python3.6/site-packages/sqlalchemy/ext/declarative/base.py", line 395, in _setup_table
    **table_kw)
  File "/home/david/.virtualenvs/flask-sqlalchemy/lib/python3.6/site-packages/sqlalchemy/sql/schema.py", line 421, in __new__
    "existing Table object." % key)
sqlalchemy.exc.InvalidRequestError: Table 'user2' is already defined for this MetaData instance.  Specify 'extend_existing=True' to redefine options and columns on an existing Table object.

[Mike Bayer]

the glitch in my doc is that you don't actually need the .cascading part: http://docs.sqlalchemy.org/en/latest/orm/extensions/declarative/mixins.html#controlling-table-inheritance-with-mixins "Declarative will always invoke declared_attr for the special names __tablename__, __mapper_args__ and __table_args__ function for each mapped class in the hierarchy. ".

in your example, you are replacing __tablename__ with a fixed string value on User2 which is why it isn't getting propagated. remove that and it works, doesn't matter if .cascading is present or not.

we don't have the option to "ignore" that a class overrode __tablename__, I think people would expect that if you override an attribute in a subclass, that's the new behavior going forward. that it's a straight string and not a new function signals that this is likely not what the user really wants here but i don't think declarative should be trying to guess the intent.

i dont think there's a bug so far.

[David Lord]

This and #4091 are connected. Since cascading applies to further subclasses even if an intermediate class sets the attr manually, I'd expect that behavior with __tablename__ too. What I'm observing seems to go against "Declarative will always invoke declared_attr for the special names ... for each mapped class." cascading seems to fit that description.

[David Lord]

If this worked the way I want (tablename attr can cascade like other attrs, skipping manually set attrs), I could remove the tablename generation from the metaclass init entirely. The _should_set_tablename detection would go away completely and I'd just have a simple cascading declared attr tablename. (Although I could only drop it if it was backported to 1.0, since that's the lowest version I support.)

[Mike Bayer]

This and #4091 are connected. Since cascading applies to further subclasses even if an intermediate class sets the attr manually, I'd expect that behavior with tablename too.

in the case of #4091, that is a limitation of @declared_attr.cascading that I don't have the energy to fix right now. I don't see why you'd expect __tablename__ to act this way if ".cascading" is not present. if it is present, then yes, it's hard to know this has no effect.

So fix here would be just another warning. "You used @cascading with tablename and this has no effect".

"Declarative will always invoke declared_attr for the special names ... for each mapped class."

then we change that doc too.

If this worked the way I want (tablename attr can cascade like other attrs, skipping manually set attrs)

I think to as much a degree as possible, the normal behavior of "the most recent class attribute in the __mro__ takes precedence" should be enforced. Once you override an attribute in Python, that's it, you don't have a previous version of the attribute hopping over the one you have. If you want flask-sqlalchemy to have arbitrary __tablename__ on any class in the hierarchy, but if the attribute is not immediately present and then it would use the superclass __tablename__ function, then you should tell users to not use __tablename__ and use something like __flask_tablename__.

put another way, I think __tablename__ does it right and @declared_attr.cascading does it wrong. I just have it warning in #4091 that hey, this doesn't really work, sorry.

[Mike Bayer]

same gerrit https://gerrit.sqlalchemy.org/530

[David Lord]

OK, fair enough, as long as the behavior is clear I'm ok with it.