Expire memoizations on setattr/delattr, check in delattr

Fixed bug where declarative would not update the state of the
:class:`.Mapper` as far as what attributes were present, when additional
attributes were added or removed after the mapper attribute collections had
already been called and memoized.  Addtionally, a ``NotImplementedError``
is now raised if a fully mapped attribute (e.g. column, relationship, etc.)
is deleted from a class that is currently mapped, since the mapper will not
function correctly if the attribute has been removed.

Change-Id: Idaca8e0237b31aa1d6564d94c3a179d7dc6b5df9
Fixes: #4133

5 files changed, 99 insertions, 2 deletions

diff --git a/doc/build/changelog/unreleased_13/4133.rst b/doc/build/changelog/unreleased_13/4133.rst
new file mode 100644
index 000000000..74c7e7098
--- /dev/null
+++ b/doc/build/changelog/unreleased_13/4133.rst
@@ -0,0 +1,11 @@
+.. change::
+    :tags: bug, orm, declarative
+    :tickets: 4133
+
+    Fixed bug where declarative would not update the state of the
+    :class:`.Mapper` as far as what attributes were present, when additional
+    attributes were added or removed after the mapper attribute collections had
+    already been called and memoized.  Addtionally, a ``NotImplementedError``
+    is now raised if a fully mapped attribute (e.g. column, relationship, etc.)
+    is deleted from a class that is currently mapped, since the mapper will not
+    function correctly if the attribute has been removed.
diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py
index b08d3ce30..865cd16f0 100644
--- a/lib/sqlalchemy/ext/declarative/api.py
+++ b/lib/sqlalchemy/ext/declarative/api.py
@@ -21,7 +21,7 @@ import re
 
 from .base import _as_declarative, \
     _declarative_constructor,\
-    _DeferredMapperConfig, _add_attribute
+    _DeferredMapperConfig, _add_attribute, _del_attribute
 from .clsregistry import _class_resolver
 
 
@@ -68,6 +68,8 @@ class DeclarativeMeta(type):
     def __setattr__(cls, key, value):
         _add_attribute(cls, key, value)
 
+    def __delattr__(cls, key):
+        _del_attribute(cls, key)
 
 def synonym_for(name, map_column=False):
     """Decorator that produces an :func:`.orm.synonym` attribute in conjunction
diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py
index 5d0eab34e..544bb2497 100644
--- a/lib/sqlalchemy/ext/declarative/base.py
+++ b/lib/sqlalchemy/ext/declarative/base.py
@@ -677,10 +677,29 @@ def _add_attribute(cls, key, value):
             )
         else:
             type.__setattr__(cls, key, value)
+            cls.__mapper__._expire_memoizations()
     else:
         type.__setattr__(cls, key, value)
 
 
+def _del_attribute(cls, key):
+
+    if '__mapper__' in cls.__dict__ and \
+            key in cls.__dict__ and not cls.__mapper__._dispose_called:
+        value = cls.__dict__[key]
+        if isinstance(
+                value,
+                (Column, ColumnProperty, MapperProperty, QueryableAttribute)
+        ):
+            raise NotImplementedError(
+                "Can't un-map individual mapped attributes on a mapped class.")
+        else:
+            type.__delattr__(cls, key)
+            cls.__mapper__._expire_memoizations()
+    else:
+        type.__delattr__(cls, key)
+
+
 def _declarative_constructor(self, **kwargs):
     """A simple constructor that allows initialization from kwargs.
 
diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py
index a30a8c243..e856c6c79 100644
--- a/lib/sqlalchemy/orm/mapper.py
+++ b/lib/sqlalchemy/orm/mapper.py
@@ -86,6 +86,7 @@ class Mapper(InspectionAttr):
     """
 
     _new_mappers = False
+    _dispose_called = False
 
     def __init__(self,
                  class_,
@@ -1274,6 +1275,7 @@ class Mapper(InspectionAttr):
     def dispose(self):
         # Disable any attribute-based compilation.
         self.configured = True
+        self._dispose_called = True
 
         if hasattr(self, '_configure_failed'):
             del self._configure_failed
diff --git a/test/ext/declarative/test_basic.py b/test/ext/declarative/test_basic.py
index 5d1655e87..d2dc1a425 100644
--- a/test/ext/declarative/test_basic.py
+++ b/test/ext/declarative/test_basic.py
@@ -2,6 +2,7 @@
 from sqlalchemy.testing import eq_, assert_raises, \
     assert_raises_message, expect_warnings, is_
 from sqlalchemy.ext import declarative as decl
+from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy import exc
 import sqlalchemy as sa
 from sqlalchemy import testing, util
@@ -1768,7 +1769,7 @@ class DeclarativeTest(DeclarativeTestBase):
         class Test(Base):
             __tablename__ = 'test'
             id = Column(Integer, primary_key=True)
-        # MARKMARK
+
         eq_(
             canary.mock_calls,
             [
@@ -1786,6 +1787,68 @@ class DeclarativeTest(DeclarativeTestBase):
 
         eq_(Base.__doc__, MyBase.__doc__)
 
+    def test_delattr_mapped_raises(self):
+        Base = decl.declarative_base()
+
+        class Foo(Base):
+            __tablename__ = 'foo'
+
+            id = Column(Integer, primary_key=True)
+            data = Column(String)
+
+        def go():
+            del Foo.data
+
+        assert_raises_message(
+            NotImplementedError,
+            "Can't un-map individual mapped attributes on a mapped class.",
+            go
+        )
+
+    def test_delattr_hybrid_fine(self):
+        Base = decl.declarative_base()
+
+        class Foo(Base):
+            __tablename__ = 'foo'
+
+            id = Column(Integer, primary_key=True)
+            data = Column(String)
+
+            @hybrid_property
+            def data_hybrid(self):
+                return self.data
+
+        assert "data_hybrid" in Foo.__mapper__.all_orm_descriptors.keys()
+
+        del Foo.data_hybrid
+
+        assert "data_hybrid" not in Foo.__mapper__.all_orm_descriptors.keys()
+
+        assert not hasattr(Foo, "data_hybrid")
+
+    def test_setattr_hybrid_updates_descriptors(self):
+        Base = decl.declarative_base()
+
+        class Foo(Base):
+            __tablename__ = 'foo'
+
+            id = Column(Integer, primary_key=True)
+            data = Column(String)
+
+        assert "data_hybrid" not in Foo.__mapper__.all_orm_descriptors.keys()
+
+        @hybrid_property
+        def data_hybrid(self):
+            return self.data
+        Foo.data_hybrid = data_hybrid
+        assert "data_hybrid" in Foo.__mapper__.all_orm_descriptors.keys()
+
+        del Foo.data_hybrid
+
+        assert "data_hybrid" not in Foo.__mapper__.all_orm_descriptors.keys()
+
+        assert not hasattr(Foo, "data_hybrid")
+
 
 def _produce_test(inline, stringbased):