- [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]

7 files changed, 139 insertions, 23 deletions

diff --git a/CHANGES b/CHANGES
index be1d4e82e..c89876928 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 d26ee61c3..08e536f71 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 93b99d83c..a57ef5e68 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 881a7bb62..5c9efb398 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 c0423939f..84c9f647c 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 546822f5d..c38c721cc 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 20088c070..ee3c7b63e 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):