From f34f634824da18a71f3c898a2e19e19e5f26bede Mon Sep 17 00:00:00 2001 From: Mike Bayer Date: Mon, 9 Jul 2018 18:24:12 -0400 Subject: [PATCH] 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 --- doc/build/changelog/unreleased_13/4133.rst | 11 ++++ lib/sqlalchemy/ext/declarative/api.py | 4 +- lib/sqlalchemy/ext/declarative/base.py | 19 +++++++ lib/sqlalchemy/orm/mapper.py | 2 + test/ext/declarative/test_basic.py | 65 +++++++++++++++++++++- 5 files changed, 99 insertions(+), 2 deletions(-) create mode 100644 doc/build/changelog/unreleased_13/4133.rst diff --git a/doc/build/changelog/unreleased_13/4133.rst b/doc/build/changelog/unreleased_13/4133.rst new file mode 100644 index 0000000000..74c7e70986 --- /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 b08d3ce30c..865cd16f0f 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 5d0eab34ed..544bb24979 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 a30a8c2433..e856c6c797 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 5d1655e873..d2dc1a4250 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): -- 2.47.2