From: Mike Bayer Date: Mon, 13 Aug 2012 15:00:58 +0000 (-0400) Subject: - [feature] Adding/removing None from a mapped collection X-Git-Tag: rel_0_8_0b1~256 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=96650157083f9c691a7a8a737724159cd6a1d668;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git - [feature] Adding/removing None from a mapped collection now generates attribute events. Previously, a None append would be ignored in some cases. Related to [ticket:2229]. - [feature] The presence of None in a mapped collection now raises an error during flush. Previously, None values in collections would be silently ignored. [ticket:2229] --- diff --git a/CHANGES b/CHANGES index be1d4e82e7..c898769289 100644 --- a/CHANGES +++ b/CHANGES @@ -123,6 +123,16 @@ underneath "0.7.xx". Both features should be avoided, however. [ticket:2372] + - [feature] Adding/removing None from a mapped collection + now generates attribute events. Previously, a None + append would be ignored in some cases. Related + to [ticket:2229]. + + - [feature] The presence of None in a mapped collection + now raises an error during flush. Previously, + None values in collections would be silently ignored. + [ticket:2229] + - [bug] Improvements to joined/subquery eager loading dealing with chains of subclass entities sharing a common base, with no specific "join depth" diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py index d26ee61c34..08e536f712 100644 --- a/lib/sqlalchemy/orm/attributes.py +++ b/lib/sqlalchemy/orm/attributes.py @@ -1022,6 +1022,9 @@ def backref_listeners(attribute, key, uselist): return child def emit_backref_from_collection_append_event(state, child, initiator): + if child is None: + return + child_state, child_dict = instance_state(child), \ instance_dict(child) child_impl = child_state.manager[key].impl @@ -1207,7 +1210,6 @@ class History(History): def from_collection(cls, attribute, state, current): original = state.committed_state.get(attribute.key, _NO_HISTORY) current = getattr(current, '_sa_adapter') - if original is NO_VALUE: return cls(list(current), (), ()) elif original is _NO_HISTORY: diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py index 93b99d83c3..a57ef5e680 100644 --- a/lib/sqlalchemy/orm/collections.py +++ b/lib/sqlalchemy/orm/collections.py @@ -716,7 +716,7 @@ class CollectionAdapter(object): operation. """ - if initiator is not False and item is not None: + if initiator is not False: if self.invalidated: self._warn_invalidated() return self.attr.fire_append_event( @@ -734,7 +734,7 @@ class CollectionAdapter(object): an initiator value from a chained operation. """ - if initiator is not False and item is not None: + if initiator is not False: if self.invalidated: self._warn_invalidated() self.attr.fire_remove_event( @@ -1032,7 +1032,7 @@ def _instrument_membership_mutator(method, before, argument, after): def __set(collection, item, _sa_initiator=None): """Run set events, may eventually be inlined into decorators.""" - if _sa_initiator is not False and item is not None: + if _sa_initiator is not False: executor = getattr(collection, '_sa_adapter', None) if executor: item = getattr(executor, 'fire_append_event')(item, _sa_initiator) @@ -1040,7 +1040,7 @@ def __set(collection, item, _sa_initiator=None): def __del(collection, item, _sa_initiator=None): """Run del events, may eventually be inlined into decorators.""" - if _sa_initiator is not False and item is not None: + if _sa_initiator is not False: executor = getattr(collection, '_sa_adapter', None) if executor: getattr(executor, 'fire_remove_event')(item, _sa_initiator) diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py index 881a7bb625..5c9efb3985 100644 --- a/lib/sqlalchemy/orm/dependency.py +++ b/lib/sqlalchemy/orm/dependency.py @@ -247,7 +247,11 @@ class DependencyProcessor(object): self.mapper in uowcommit.mappers def _verify_canload(self, state): - if state is not None and \ + if self.prop.uselist and state is None: + raise exc.FlushError( + "Can't flush None value found in " + "collection %s" % (self.prop, )) + elif state is not None and \ not self.mapper._canload(state, allow_subtypes=not self.enable_typechecks): if self.mapper._canload(state, allow_subtypes=True): @@ -559,10 +563,10 @@ class OneToManyDP(DependencyProcessor): pks_changed): source = state dest = child + self._verify_canload(child) if dest is None or \ (not self.post_update and uowcommit.is_deleted(dest)): return - self._verify_canload(child) if clearkeys: sync.clear(dest, self.mapper, self.prop.synchronize_pairs) else: @@ -1032,8 +1036,7 @@ class ManyToManyDP(DependencyProcessor): passive) if history: for child in history.added: - if child is None or \ - (processed is not None and + if (processed is not None and (state, child) in processed): continue associationrow = {} @@ -1044,8 +1047,7 @@ class ManyToManyDP(DependencyProcessor): continue secondary_insert.append(associationrow) for child in history.deleted: - if child is None or \ - (processed is not None and + if (processed is not None and (state, child) in processed): continue associationrow = {} @@ -1130,6 +1132,8 @@ class ManyToManyDP(DependencyProcessor): if associationrow is None: return + self._verify_canload(child) + if child is not None and not uowcommit.session._contains_state(child): if not child.deleted: util.warn( @@ -1138,8 +1142,6 @@ class ManyToManyDP(DependencyProcessor): (mapperutil.state_class_str(child), operation, self.prop)) return False - self._verify_canload(child) - sync.populate_dict(state, self.parent, associationrow, self.prop.synchronize_pairs) sync.populate_dict(child, self.mapper, associationrow, diff --git a/lib/sqlalchemy/orm/unitofwork.py b/lib/sqlalchemy/orm/unitofwork.py index c0423939f9..84c9f647ca 100644 --- a/lib/sqlalchemy/orm/unitofwork.py +++ b/lib/sqlalchemy/orm/unitofwork.py @@ -29,6 +29,9 @@ def track_cascade_events(descriptor, prop): # process "save_update" cascade rules for when # an instance is appended to the list of another instance + if item is None: + return + sess = sessionlib._state_session(state) if sess: prop = state.manager.mapper._props[key] @@ -40,6 +43,9 @@ def track_cascade_events(descriptor, prop): return item def remove(state, item, initiator): + if item is None: + return + sess = sessionlib._state_session(state) if sess: prop = state.manager.mapper._props[key] diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py index 546822f5dc..c38c721ccf 100644 --- a/test/orm/test_attributes.py +++ b/test/orm/test_attributes.py @@ -2308,6 +2308,46 @@ class ListenerTest(fixtures.ORMTest): f1.barset.add(b1) assert f1.barset.pop().data == 'some bar appended' + def test_none_on_collection_event(self): + """test that append/remove of None in collections emits events. + + This is new behavior as of 0.8. + + """ + class Foo(object): + pass + class Bar(object): + pass + instrumentation.register_class(Foo) + instrumentation.register_class(Bar) + attributes.register_attribute(Foo, 'barlist', uselist=True, + useobject=True) + canary = [] + def append(state, child, initiator): + canary.append((state, child)) + def remove(state, child, initiator): + canary.append((state, child)) + event.listen(Foo.barlist, 'append', append) + event.listen(Foo.barlist, 'remove', remove) + + b1, b2 = Bar(), Bar() + f1 = Foo() + f1.barlist.append(None) + eq_(canary, [(f1, None)]) + + canary[:] = [] + f1 = Foo() + f1.barlist = [None, b2] + eq_(canary, [(f1, None), (f1, b2)]) + + canary[:] = [] + f1 = Foo() + f1.barlist = [b1, None, b2] + eq_(canary, [(f1, b1), (f1, None), (f1, b2)]) + + f1.barlist.remove(None) + eq_(canary, [(f1, b1), (f1, None), (f1, b2), (f1, None)]) + def test_propagate(self): classes = [None, None, None] canary = [] diff --git a/test/orm/test_cascade.py b/test/orm/test_cascade.py index 20088c070e..ee3c7b63ee 100644 --- a/test/orm/test_cascade.py +++ b/test/orm/test_cascade.py @@ -397,20 +397,20 @@ class O2MCascadeTest(fixtures.MappedTest): }) - def test_none_skipped_assignment(self): - # [ticket:2229] proposes warning/raising on None - # for 0.8 + def test_none_o2m_collection_assignment(self): User, Address = self.classes.User, self.classes.Address s = Session() u1 = User(name='u', addresses=[None]) s.add(u1) eq_(u1.addresses, [None]) - s.commit() - eq_(u1.addresses, []) + assert_raises_message( + orm_exc.FlushError, + "Can't flush None value found in collection User.addresses", + s.commit + ) + eq_(u1.addresses, [None]) - def test_none_skipped_append(self): - # [ticket:2229] proposes warning/raising on None - # for 0.8 + def test_none_o2m_collection_append(self): User, Address = self.classes.User, self.classes.Address s = Session() @@ -418,8 +418,13 @@ class O2MCascadeTest(fixtures.MappedTest): s.add(u1) u1.addresses.append(None) eq_(u1.addresses, [None]) - s.commit() - eq_(u1.addresses, []) + assert_raises_message( + orm_exc.FlushError, + "Can't flush None value found in collection User.addresses", + s.commit + ) + eq_(u1.addresses, [None]) + class O2MCascadeDeleteNoOrphanTest(fixtures.MappedTest): run_inserts = None @@ -1797,6 +1802,57 @@ class M2MCascadeTest(fixtures.MappedTest): assert b1 not in a1.bs assert b1 in a2.bs + def test_none_m2m_collection_assignment(self): + a, A, B, b, atob = (self.tables.a, + self.classes.A, + self.classes.B, + self.tables.b, + self.tables.atob) + + + mapper(A, a, properties={ + 'bs': relationship(B, + secondary=atob, backref="as") + }) + mapper(B, b) + + s = Session() + a1 = A(bs=[None]) + s.add(a1) + eq_(a1.bs, [None]) + assert_raises_message( + orm_exc.FlushError, + "Can't flush None value found in collection A.bs", + s.commit + ) + eq_(a1.bs, [None]) + + def test_none_m2m_collection_append(self): + a, A, B, b, atob = (self.tables.a, + self.classes.A, + self.classes.B, + self.tables.b, + self.tables.atob) + + + mapper(A, a, properties={ + 'bs': relationship(B, + secondary=atob, backref="as") + }) + mapper(B, b) + + s = Session() + a1 = A() + a1.bs.append(None) + s.add(a1) + eq_(a1.bs, [None]) + assert_raises_message( + orm_exc.FlushError, + "Can't flush None value found in collection A.bs", + s.commit + ) + eq_(a1.bs, [None]) + class O2MSelfReferentialDetelOrphanTest(fixtures.MappedTest): @classmethod def define_tables(cls, metadata):