diff options
| -rw-r--r-- | doc/build/changelog/unreleased_12/4091.rst | 18 | ||||
| -rw-r--r-- | doc/build/orm/extensions/declarative/mixins.rst | 18 | ||||
| -rw-r--r-- | lib/sqlalchemy/ext/declarative/api.py | 21 | ||||
| -rw-r--r-- | lib/sqlalchemy/ext/declarative/base.py | 39 | ||||
| -rw-r--r-- | test/ext/declarative/test_mixin.py | 41 |
5 files changed, 125 insertions, 12 deletions
diff --git a/doc/build/changelog/unreleased_12/4091.rst b/doc/build/changelog/unreleased_12/4091.rst new file mode 100644 index 000000000..056843cfb --- /dev/null +++ b/doc/build/changelog/unreleased_12/4091.rst @@ -0,0 +1,18 @@ +.. change:: + :tags: bug, orm, declarative + :tickets: 4091 + + A warning is emitted if a subclass attempts to override an attribute + that was declared on a superclass using ``@declared_attr.cascading`` + that the overridden attribute will be ignored. This use + case cannot be fully supported down to further subclasses without more + complex development efforts, so for consistency the "cascading" is + honored all the way down regardless of overriding attributes. + +.. change:: + :tags: bug, orm, declarative + :tickets: 4092 + + A warning is emitted if the ``@declared_attr.cascading`` attribute is + used with a special declarative name such as ``__tablename__``, as this + has no effect.
\ No newline at end of file diff --git a/doc/build/orm/extensions/declarative/mixins.rst b/doc/build/orm/extensions/declarative/mixins.rst index 3b1146240..817295313 100644 --- a/doc/build/orm/extensions/declarative/mixins.rst +++ b/doc/build/orm/extensions/declarative/mixins.rst @@ -383,7 +383,8 @@ This is achieved using the :class:`.declared_attr` indicator in conjunction with a method named ``__tablename__()``. Declarative will always invoke :class:`.declared_attr` for the special names ``__tablename__``, ``__mapper_args__`` and ``__table_args__`` -function **for each mapped class in the hierarchy**. The function therefore +function **for each mapped class in the hierarchy, except if overridden +in a subclass**. The function therefore needs to expect to receive each class individually and to provide the correct answer for each. @@ -464,8 +465,8 @@ named columns on each subclass. However in this case, we may want to have an ``id`` column on every table, and have them refer to each other via foreign key. We can achieve this as a mixin by using the :attr:`.declared_attr.cascading` modifier, which indicates that the -function should be invoked **for each class in the hierarchy**, just like -it does for ``__tablename__``:: +function should be invoked **for each class in the hierarchy**, in *almost* +(see warning below) the same way as it does for ``__tablename__``:: class HasIdMixin(object): @declared_attr.cascading @@ -485,6 +486,17 @@ it does for ``__tablename__``:: primary_language = Column(String(50)) __mapper_args__ = {'polymorphic_identity': 'engineer'} +.. warning:: + + The :attr:`.declared_attr.cascading` feature currently does + **not** allow for a subclass to override the attribute with a different + function or value. This is a current limitation in the mechanics of + how ``@declared_attr`` is resolved, and a warning is emitted if + this condition is detected. This limitation does **not** + exist for the special attribute names such as ``__tablename__``, which + resolve in a different way internally than that of + :attr:`.declared_attr.cascading`. + .. versionadded:: 1.0.0 added :attr:`.declared_attr.cascading`. diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py index ee13a90f3..0773922d0 100644 --- a/lib/sqlalchemy/ext/declarative/api.py +++ b/lib/sqlalchemy/ext/declarative/api.py @@ -199,11 +199,24 @@ class declared_attr(interfaces._MappedAttribute, property): or MapperProperty-based declared attribute should be configured distinctly per mapped subclass, within a mapped-inheritance scenario. - .. note:: + .. warning:: - The :attr:`.declared_attr.cascading` modifier **only** applies - to the use of :class:`.declared_attr` on declarative mixin classes - and ``__abstract__`` classes. + The :attr:`.declared_attr.cascading` modifier has several + limitations: + + * The flag **only** applies to the use of :class:`.declared_attr` + on declarative mixin classes and ``__abstract__`` classes; it + currently has no effect when used on a mapped class directly. + + * The flag **only** applies to normally-named attributes, e.g. + not any special underscore attributes such as ``__tablename__``. + On these attributes it has **no** effect. + + * The flag currently **does not allow further overrides** down + the class hierarchy; if a subclass tries to override the + attribute, a warning is emitted and the overridden attribute + is skipped. This is a limitation that it is hoped will be + resolved at some point. Below, both MyClass as well as MySubClass will have a distinct ``id`` Column object established:: diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py index 3a2ac66ae..255373597 100644 --- a/lib/sqlalchemy/ext/declarative/base.py +++ b/lib/sqlalchemy/ext/declarative/base.py @@ -88,6 +88,19 @@ def _as_declarative(cls, classname, dict_): _MapperConfig.setup_mapping(cls, classname, dict_) +def _check_declared_props_nocascade(obj, name, cls): + + if isinstance(obj, declarative_props): + if getattr(obj, '_cascading', False): + util.warn( + "@declared_attr.cascading is not supported on the %s " + "attribute on class %s. This attribute invokes for " + "subclasses in any case." % (name, cls)) + return True + else: + return False + + class _MapperConfig(object): @classmethod @@ -167,9 +180,11 @@ class _MapperConfig(object): for name, obj in vars(base).items(): if name == '__mapper_args__': + check_decl = \ + _check_declared_props_nocascade(obj, name, cls) if not mapper_args_fn and ( not class_mapped or - isinstance(obj, declarative_props) + check_decl ): # don't even invoke __mapper_args__ until # after we've determined everything about the @@ -177,17 +192,21 @@ class _MapperConfig(object): # make a copy of it so a class-level dictionary # is not overwritten when we update column-based # arguments. - mapper_args_fn = lambda: dict(cls.__mapper_args__) + mapper_args_fn = lambda: dict(cls.__mapper_args__) # noqa elif name == '__tablename__': + check_decl = \ + _check_declared_props_nocascade(obj, name, cls) if not tablename and ( not class_mapped or - isinstance(obj, declarative_props) + check_decl ): tablename = cls.__tablename__ elif name == '__table_args__': + check_decl = \ + _check_declared_props_nocascade(obj, name, cls) if not table_args and ( not class_mapped or - isinstance(obj, declarative_props) + check_decl ): table_args = cls.__table_args__ if not isinstance( @@ -220,6 +239,18 @@ class _MapperConfig(object): elif isinstance(obj, declarative_props): oldclassprop = isinstance(obj, util.classproperty) if not oldclassprop and obj._cascading: + if name in dict_: + # unfortunately, while we can use the user- + # defined attribute here to allow a clean + # override, if there's another + # subclass below then it still tries to use + # this. not sure if there is enough information + # here to add this as a feature later on. + util.warn( + "Attribute '%s' on class %s cannot be " + "processed due to " + "@declared_attr.cascading; " + "skipping" % (name, cls)) dict_[name] = column_copies[obj] = \ ret = obj.__get__(obj, cls) setattr(cls, name, ret) diff --git a/test/ext/declarative/test_mixin.py b/test/ext/declarative/test_mixin.py index be169bcda..0b1b117fb 100644 --- a/test/ext/declarative/test_mixin.py +++ b/test/ext/declarative/test_mixin.py @@ -1,5 +1,5 @@ from sqlalchemy.testing import eq_, assert_raises, \ - assert_raises_message, is_ + assert_raises_message, is_, expect_warnings from sqlalchemy.ext import declarative as decl import sqlalchemy as sa from sqlalchemy import testing @@ -1613,6 +1613,30 @@ class DeclaredAttrTest(DeclarativeTestBase, testing.AssertsCompiledSQL): eq_(counter.mock_calls, [mock.call(A), mock.call(B)]) + def test_property_cascade_mixin_override(self): + counter = mock.Mock() + + class Mixin(object): + @declared_attr.cascading + def my_prop(cls): + counter(cls) + return column_property(cls.x + 2) + + class A(Base, Mixin): + __tablename__ = 'a' + + id = Column(Integer, primary_key=True) + x = Column(Integer) + + with expect_warnings( + "Attribute 'my_prop' on class .*B.* " + "cannot be processed due to @declared_attr.cascading; " + "skipping"): + class B(A): + my_prop = Column('foobar', Integer) + + eq_(counter.mock_calls, [mock.call(A), mock.call(B)]) + def test_property_cascade_abstract(self): counter = mock.Mock() @@ -1635,6 +1659,21 @@ class DeclaredAttrTest(DeclarativeTestBase, testing.AssertsCompiledSQL): eq_(counter.mock_calls, [mock.call(A), mock.call(B)]) + def test_warn_cascading_used_w_tablename(self): + class Mixin(object): + @declared_attr.cascading + def __tablename__(cls): + return "foo" + + with expect_warnings( + "@declared_attr.cascading is not supported on the " + "__tablename__ attribute on class .*A." + ): + class A(Mixin, Base): + id = Column(Integer, primary_key=True) + + eq_(A.__table__.name, "foo") + def test_col_prop_attrs_associated_w_class_for_mapper_args(self): from sqlalchemy import Column import collections |