Commits

Mike Bayer  committed 858e49f

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

  • Participants
  • Parent commits 112024e

Comments (0)

Files changed (7)

     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"

File lib/sqlalchemy/orm/attributes.py

         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
     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:

File lib/sqlalchemy/orm/collections.py

         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(
         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(
 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)
 
 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)

File lib/sqlalchemy/orm/dependency.py

                 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):
                             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:
                                                 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 = {}
                         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 = {}
         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(
                     (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,

File lib/sqlalchemy/orm/unitofwork.py

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

File test/orm/test_attributes.py

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

File test/orm/test_cascade.py

 
         })
 
-    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, [])
-
-    def test_none_skipped_append(self):
-        # [ticket:2229] proposes warning/raising on None
-        # for 0.8
+        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_o2m_collection_append(self):
         User, Address = self.classes.User, self.classes.Address
         s = Session()
 
         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
         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):