From 8b82369347641d1c9d64406462fa5527132c4880 Mon Sep 17 00:00:00 2001
From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 26 May 2017 13:26:40 -0400
Subject: [PATCH] Don't hard-evaluate non-ORM @declared_attr for
 AbstractConcreteBase

Fixed bug where using :class:`.declared_attr` on an
:class:`.AbstractConcreteBase` where a particular return value were some
non-mapped symbol, including ``None``, would cause the attribute
to hard-evaluate just once and store the value to the object
dictionary, not allowing it to invoke for subclasses.   This behavior
is normal when :class:`.declared_attr` is on a mapped class, and
does not occur on a mixin or abstract class.  Since
:class:`.AbstractConcreteBase` is both "abstract" and actually
"mapped", a special exception case is made here so that the
"abstract" behavior takes precedence for :class:`.declared_attr`.

Change-Id: I6160ebb3a52c441d6a4b663c8c9bbac6d37fa417
Fixes: #3848
---
 doc/build/changelog/changelog_12.rst   | 15 ++++++
 lib/sqlalchemy/ext/declarative/base.py |  6 ++-
 test/ext/declarative/test_mixin.py     | 63 ++++++++++++++++++++++++++
 3 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index e7d15d72d9..314fe675c1 100644
--- a/doc/build/changelog/changelog_12.rst
+++ b/doc/build/changelog/changelog_12.rst
@@ -26,6 +26,21 @@
         and the UPDATE now sets the version_id value to itself, so that
         concurrency checks still take place.
 
+    .. change:: 3848
+        :tags: bug, orm, declarative
+        :tickets: 3848
+
+        Fixed bug where using :class:`.declared_attr` on an
+        :class:`.AbstractConcreteBase` where a particular return value were some
+        non-mapped symbol, including ``None``, would cause the attribute
+        to hard-evaluate just once and store the value to the object
+        dictionary, not allowing it to invoke for subclasses.   This behavior
+        is normal when :class:`.declared_attr` is on a mapped class, and
+        does not occur on a mixin or abstract class.  Since
+        :class:`.AbstractConcreteBase` is both "abstract" and actually
+        "mapped", a special exception case is made here so that the
+        "abstract" behavior takes precedence for :class:`.declared_attr`.
+
     .. change:: 3673
         :tags: bug, orm
         :tickets: 3673
diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 95f02ea968..d9433e6928 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -273,6 +273,9 @@ class _MapperConfig(object):
 
         our_stuff = self.properties
 
+        late_mapped = _get_immediate_cls_attr(
+                    cls, '_sa_decl_prepare_nocascade', strict=True)
+
         for k in list(dict_):
 
             if k in ('__table__', '__tablename__', '__mapper_args__'):
@@ -302,7 +305,8 @@ class _MapperConfig(object):
                 # and place the evaluated value onto the class.
                 if not k.startswith('__'):
                     dict_.pop(k)
-                    setattr(cls, k, value)
+                    if not late_mapped:
+                        setattr(cls, k, value)
                 continue
             # we expect to see the name 'metadata' in some valid cases;
             # however at this point we see it's assigned to something trying
diff --git a/test/ext/declarative/test_mixin.py b/test/ext/declarative/test_mixin.py
index 1f9fa1dfaf..3272e0753f 100644
--- a/test/ext/declarative/test_mixin.py
+++ b/test/ext/declarative/test_mixin.py
@@ -1188,6 +1188,69 @@ class DeclarativeMixinTest(DeclarativeTestBase):
         TypeA(filters=['foo'])
         TypeB(filters=['foo'])
 
+    def test_arbitrary_attrs_three(self):
+        class Mapped(Base):
+            __tablename__ = 't'
+            id = Column(Integer, primary_key=True)
+
+            @declared_attr
+            def some_attr(cls):
+                return cls.__name__ + "SOME ATTR"
+
+        eq_(Mapped.some_attr, "MappedSOME ATTR")
+        eq_(Mapped.__dict__['some_attr'], "MappedSOME ATTR")
+
+    def test_arbitrary_attrs_doesnt_apply_to_abstract_declared_attr(self):
+        names = ["name1", "name2", "name3"]
+
+        class SomeAbstract(Base):
+            __abstract__ = True
+
+            @declared_attr
+            def some_attr(cls):
+                return names.pop(0)
+
+        class M1(SomeAbstract):
+            __tablename__ = 't1'
+            id = Column(Integer, primary_key=True)
+
+        class M2(SomeAbstract):
+            __tablename__ = 't2'
+            id = Column(Integer, primary_key=True)
+
+        eq_(M1.__dict__['some_attr'], 'name1')
+        eq_(M2.__dict__['some_attr'], 'name2')
+
+    def test_arbitrary_attrs_doesnt_apply_to_prepare_nocascade(self):
+        names = ["name1", "name2", "name3"]
+
+        class SomeAbstract(Base):
+            __tablename__ = 't0'
+            __no_table__ = True
+
+            # used by AbstractConcreteBase
+            _sa_decl_prepare_nocascade = True
+
+            id = Column(Integer, primary_key=True)
+
+            @declared_attr
+            def some_attr(cls):
+                return names.pop(0)
+
+        class M1(SomeAbstract):
+            __tablename__ = 't1'
+            id = Column(Integer, primary_key=True)
+
+        class M2(SomeAbstract):
+            __tablename__ = 't2'
+            id = Column(Integer, primary_key=True)
+
+        eq_(M1.some_attr, "name2")
+        eq_(M2.some_attr, "name3")
+        eq_(M1.__dict__['some_attr'], 'name2')
+        eq_(M2.__dict__['some_attr'], 'name3')
+        assert isinstance(SomeAbstract.__dict__['some_attr'], declared_attr)
+
 
 class DeclarativeMixinPropertyTest(DeclarativeTestBase):
 
-- 
2.47.2