returning None (or anything) from @declared_attr means it won't be called further, though this usage may be intuitive on AbstractConcreteBase

[Mike Bayer]

this is a conceptual one that might not be possible to fix. With AbstractConcreteBase, we're both "abstract" and "mapped", so the scheme below is intuitive. But because it returns None, the logic in extract_mappable_attributes ("# using @declared_attr for some object that isn't Column/MapperProperty; remove from the dict and place the evaluated value onto the class.") blows it away.

need to re-identify the rationale for the "remove the attribute" logic, determine if the "might return None" style is likely only with AbstractConcreteBase or in a more general sense, then identify a potential warning, or even behavioral change, to make this more intuitive. A special symbol like "don't map" might serve as a more explicit replacement for None.

from sqlalchemy import *
from sqlalchemy.orm import *
from sqlalchemy.ext.declarative import declarative_base
from sqlalchemy.ext.declarative import declared_attr
from sqlalchemy.ext.declarative import AbstractConcreteBase

Base = declarative_base()


class FooBase(AbstractConcreteBase, Base):
    @declared_attr
    def foo(cls):
        print "FOO! ", cls.__name__
        if cls.__name__ == 'A':
            #return Column(String)  # gets discarded, but affects B
            return None # causes attr to not be called again
        else:
            return Column(Integer)

class A(FooBase):
    __tablename__ = 'a'
    id = Column(Integer, primary_key=True)
    __mapper_args__ = {
        'polymorphic_identity': 'a'
    }


class B(FooBase):
    __tablename__ = 'b'
    id = Column(Integer, primary_key=True)
    __mapper_args__ = {
        'polymorphic_identity': 'b'
    }

configure_mappers()
print FooBase.__mapper__.local_table

e = create_engine("sqlite://", echo=True)
Base.metadata.create_all(e)

output:

SELECT b.id, 'b' AS type 
FROM b UNION ALL SELECT a.id, 'a' AS type 
FROM a

CREATE TABLE a (
    id INTEGER NOT NULL, 
    PRIMARY KEY (id)
)

CREATE TABLE b (
    id INTEGER NOT NULL, 
    PRIMARY KEY (id)
)

bug is "foo" is missing from "b", "foo" missing from polymorphic selectable.

[Mike Bayer]

diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py
index 7c503d4..fb27343 100644
--- a/lib/sqlalchemy/ext/declarative/api.py
+++ b/lib/sqlalchemy/ext/declarative/api.py
@@ -166,6 +166,16 @@ class declared_attr(interfaces._MappedAttribute, property):

     """

+    SKIP_ATTRIBUTE = util.symbol("SKIP_ATTRIBUTE")
+    """A special return value that indicates the declarative system
+    should skip mapping this attribute.
+
+    Usually, SKIP_ATTRIBUTE is returned conditionally, such that some
+    subclasses of a mapping should map this attribute, and others should
+    not.
+
+    """
+
     def __init__(self, fget, cascading=False):
         super(declared_attr, self).__init__(fget)
         self.__doc__ = fget.__doc__
diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 16cb05e..410ae8e 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -301,8 +301,27 @@ class _MapperConfig(object):
                 # isn't Column/MapperProperty; remove from the dict_
                 # and place the evaluated value onto the class.
                 if not k.startswith('__'):
+                    if value is None:
+                        util.warn(
+                            "Attribute %s uses declared_attr but returns "
+                            "non-SQLAlchemy value None; to indicate skipping "
+                            "this attribute, use "
+                            "declared_attr.SKIP_ATTRIBUTE" %
+                            (k, ))
+                    elif value is declared_attr.SKIP_ATTRIBUTE:
+                        # for mapper properties, etc., SKIP_ATTRIBUTE means
+                        # we'll skip this
+                        continue
+                    else:
+                        util.warn(
+                            "Attribute %s uses declared_attr but "
+                            "returns a non-SQLAlchemy value %r" % (k, value))
                     dict_.pop(k)
                     setattr(cls, k, value)
+                elif value is declared_attr.SKIP_ATTRIBUTE:
+                    # for __tablename__, etc., SKIP_ATTRIBUTE is the same
+                    # as None
+                    value = None
                 continue
             # we expect to see the name 'metadata' in some valid cases;
             # however at this point we see it's assigned to something trying

will put this up in a gerrit for 1.2

[Mike Bayer]

https://gerrit.sqlalchemy.org/290 - add a new constant, warn for ambiguous values

[Mike Bayer]

the "setattr" logic just needs to not take place for declared attrs:

diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 95f02ea..653c57c 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -278,8 +278,10 @@ class _MapperConfig(object):
             if k in ('__table__', '__tablename__', '__mapper_args__'):
                 continue

+            FLAG = False
             value = dict_[k]
             if isinstance(value, declarative_props):
+                FLAG = True
                 value = getattr(cls, k)

             elif isinstance(value, QueryableAttribute) and \
@@ -302,7 +304,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 FLAG:
+                        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

[Mike Bayer]

nope

    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")

[Mike Bayer]

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

→ <<cset 8b8236934764>>