Warnings for @declared_attr.cascadingticket_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.

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.

Ensure that documenation refers to the current inconsistency that
__tablename__ can be overridden by subclasses however
@declared_attr.cascading cannot.

Fixes: #4091
Fixes: #4092
Change-Id: I3aecdb2f99d408e404a1223f5ad86ae3c7fdf036

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