diff options
| -rw-r--r-- | doc/build/changelog/changelog_10.rst | 19 | ||||
| -rw-r--r-- | lib/sqlalchemy/ext/declarative/api.py | 21 | ||||
| -rw-r--r-- | lib/sqlalchemy/ext/declarative/base.py | 10 | ||||
| -rw-r--r-- | lib/sqlalchemy/orm/mapper.py | 14 | ||||
| -rw-r--r-- | test/ext/declarative/test_basic.py | 31 | ||||
| -rw-r--r-- | test/orm/test_events.py | 40 |
6 files changed, 110 insertions, 25 deletions
diff --git a/doc/build/changelog/changelog_10.rst b/doc/build/changelog/changelog_10.rst index 5b410e74f..891981fe2 100644 --- a/doc/build/changelog/changelog_10.rst +++ b/doc/build/changelog/changelog_10.rst @@ -20,6 +20,25 @@ .. change:: :tags: bug, orm + :tickets: 3388 + + Fixed a regression regarding the :meth:`.MapperEvents.instrument_class` + event where its invocation was moved to be after the class manager's + instrumentation of the class, which is the opposite of what the + documentation for the event explicitly states. The rationale for the + switch was due to Declarative taking the step of setting up + the full "instrumentation manager" for a class before it was mapped + for the purpose of the new ``@declared_attr`` features + described in :ref:`feature_3150`, but the change was also made + against the classical use of :func:`.mapper` for consistency. + However, SQLSoup relies upon the instrumentation event happening + before any instrumentation under classical mapping. + The behavior is reverted in the case of classical and declarative + mapping, the latter implemented by using a simple memoization + without using class manager. + + .. change:: + :tags: bug, orm :tickets: 3387 Fixed issue in new :meth:`.QueryEvents.before_compile` event where diff --git a/lib/sqlalchemy/ext/declarative/api.py b/lib/sqlalchemy/ext/declarative/api.py index 713ea0aba..3d46bd4cb 100644 --- a/lib/sqlalchemy/ext/declarative/api.py +++ b/lib/sqlalchemy/ext/declarative/api.py @@ -163,21 +163,16 @@ class declared_attr(interfaces._MappedAttribute, property): self._cascading = cascading def __get__(desc, self, cls): - # use the ClassManager for memoization of values. This is better than - # adding yet another attribute onto the class, or using weakrefs - # here which are slow and take up memory. It also allows us to - # warn for non-mapped use of declared_attr. - - manager = attributes.manager_of_class(cls) - if manager is None: - util.warn( - "Unmanaged access of declarative attribute %s from " - "non-mapped class %s" % - (desc.fget.__name__, cls.__name__)) + reg = cls.__dict__.get('_sa_declared_attr_reg', None) + if reg is None: + manager = attributes.manager_of_class(cls) + if manager is None: + util.warn( + "Unmanaged access of declarative attribute %s from " + "non-mapped class %s" % + (desc.fget.__name__, cls.__name__)) return desc.fget(cls) - reg = manager.info.get('declared_attr_reg', None) - if reg is None: return desc.fget(cls) elif desc in reg: diff --git a/lib/sqlalchemy/ext/declarative/base.py b/lib/sqlalchemy/ext/declarative/base.py index 062936ea7..57eb54f63 100644 --- a/lib/sqlalchemy/ext/declarative/base.py +++ b/lib/sqlalchemy/ext/declarative/base.py @@ -115,10 +115,10 @@ class _MapperConfig(object): self.column_copies = {} self._setup_declared_events() - # register up front, so that @declared_attr can memoize - # function evaluations in .info - manager = instrumentation.register_class(self.cls) - manager.info['declared_attr_reg'] = {} + # temporary registry. While early 1.0 versions + # set up the ClassManager here, by API contract + # we can't do that until there's a mapper. + self.cls._sa_declared_attr_reg = {} self._scan_attributes() @@ -529,7 +529,7 @@ class _MapperConfig(object): self.local_table, **self.mapper_args ) - del mp_.class_manager.info['declared_attr_reg'] + del self.cls._sa_declared_attr_reg return mp_ diff --git a/lib/sqlalchemy/orm/mapper.py b/lib/sqlalchemy/orm/mapper.py index 924130874..468846d40 100644 --- a/lib/sqlalchemy/orm/mapper.py +++ b/lib/sqlalchemy/orm/mapper.py @@ -1096,8 +1096,6 @@ class Mapper(InspectionAttr): """ - # when using declarative as of 1.0, the register_class has - # already happened from within declarative. manager = attributes.manager_of_class(self.class_) if self.non_primary: @@ -1120,14 +1118,20 @@ class Mapper(InspectionAttr): "create a non primary Mapper. clear_mappers() will " "remove *all* current mappers from all classes." % self.class_) - - if manager is None: - manager = instrumentation.register_class(self.class_) + # else: + # a ClassManager may already exist as + # ClassManager.instrument_attribute() creates + # new managers for each subclass if they don't yet exist. _mapper_registry[self] = True + # note: this *must be called before instrumentation.register_class* + # to maintain the documented behavior of instrument_class self.dispatch.instrument_class(self, self.class_) + if manager is None: + manager = instrumentation.register_class(self.class_) + self.class_manager = manager manager.mapper = self diff --git a/test/ext/declarative/test_basic.py b/test/ext/declarative/test_basic.py index 3fac39cac..ab0de801c 100644 --- a/test/ext/declarative/test_basic.py +++ b/test/ext/declarative/test_basic.py @@ -13,7 +13,10 @@ from sqlalchemy.orm import relationship, create_session, class_mapper, \ column_property, composite, Session, properties from sqlalchemy.util import with_metaclass from sqlalchemy.ext.declarative import declared_attr, synonym_for -from sqlalchemy.testing import fixtures +from sqlalchemy.testing import fixtures, mock +from sqlalchemy.orm.events import MapperEvents +from sqlalchemy.orm import mapper +from sqlalchemy import event Base = None @@ -1671,6 +1674,32 @@ class DeclarativeTest(DeclarativeTestBase): )) ) + @testing.teardown_events(MapperEvents) + def test_instrument_class_before_instrumentation(self): + # test #3388 + + canary = mock.Mock() + + @event.listens_for(mapper, "instrument_class") + def instrument_class(mp, cls): + canary.instrument_class(mp, cls) + + @event.listens_for(object, "class_instrument") + def class_instrument(cls): + canary.class_instrument(cls) + + class Test(Base): + __tablename__ = 'test' + id = Column(Integer, primary_key=True) + # MARKMARK + eq_( + canary.mock_calls, + [ + mock.call.instrument_class(Test.__mapper__, Test), + mock.call.class_instrument(Test) + ] + ) + def _produce_test(inline, stringbased): diff --git a/test/orm/test_events.py b/test/orm/test_events.py index 89cc54eb8..51a0bb3a2 100644 --- a/test/orm/test_events.py +++ b/test/orm/test_events.py @@ -189,13 +189,13 @@ class MapperEventsTest(_RemoveListeners, _fixtures.FixtureTest): ] ) - def test_merge(self): users, User = self.tables.users, self.classes.User mapper(User, users) canary = [] + def load(obj, ctx): canary.append('load') event.listen(mapper, 'load', load) @@ -212,6 +212,7 @@ class MapperEventsTest(_RemoveListeners, _fixtures.FixtureTest): s.query(User).order_by(User.id).first() eq_(canary, ['load', 'load', 'load']) + def test_inheritance(self): users, addresses, User = (self.tables.users, self.tables.addresses, @@ -385,6 +386,43 @@ class MapperEventsTest(_RemoveListeners, _fixtures.FixtureTest): mapper(Address, addresses) eq_(canary, [User, Address]) + def test_instrument_class_precedes_class_instrumentation(self): + users = self.tables.users + + class MyClass(object): + pass + + canary = Mock() + + def my_init(self): + canary.init() + + # mapper level event + @event.listens_for(mapper, "instrument_class") + def instrument_class(mp, class_): + canary.instrument_class(class_) + class_.__init__ = my_init + + # instrumentationmanager event + @event.listens_for(object, "class_instrument") + def class_instrument(class_): + canary.class_instrument(class_) + + mapper(MyClass, users) + + m1 = MyClass() + assert attributes.instance_state(m1) + + eq_( + [ + call.instrument_class(MyClass), + call.class_instrument(MyClass), + call.init() + ], + canary.mock_calls + ) + + class DeclarativeEventListenTest(_RemoveListeners, fixtures.DeclarativeMappedTest): run_setup_classes = "each" run_deletes = None |