- Fixed regression from 0.6 where Session.add()

against an object which contained None in a
collection would raise an internal exception.
Reverted this to 0.6's behavior which is to
accept the None but obviously nothing is
persisted.  Ideally, collections with None
present or on append() should at least emit a
warning, which is being considered for 0.8.
[ticket:2205]

4 files changed, 88 insertions, 4 deletions

diff --git a/CHANGES b/CHANGES
index a7da519f1..24ce33f28 100644
--- a/CHANGES
+++ b/CHANGES
@@ -32,6 +32,16 @@ CHANGES
     _with_invoke_all_eagers()
     which selects old/new behavior [ticket:2213]
 
+  - Fixed regression from 0.6 where Session.add()
+    against an object which contained None in a
+    collection would raise an internal exception.
+    Reverted this to 0.6's behavior which is to 
+    accept the None but obviously nothing is
+    persisted.  Ideally, collections with None 
+    present or on append() should at least emit a 
+    warning, which is being considered for 0.8.
+    [ticket:2205]
+
   - Fixed regression from 0.6 where a get history
     operation on some relationship() based attributes
     would fail when a lazyload would emit; this could 
diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index c72151948..a24348c86 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -722,8 +722,12 @@ class CollectionAttributeImpl(AttributeImpl):
         if self.key in state.committed_state:
             original = state.committed_state[self.key]
             if original is not NO_VALUE:
-                current_states = [(instance_state(c), c) for c in current]
-                original_states = [(instance_state(c), c) for c in original]
+                current_states = [((c is not None) and 
+                                    instance_state(c) or None, c) 
+                                    for c in current]
+                original_states = [((c is not None) and 
+                                    instance_state(c) or None, c) 
+                                    for c in original]
 
                 current_set = dict(current_states)
                 original_set = dict(original_states)
@@ -1114,8 +1118,13 @@ class History(tuple):
         elif original is _NO_HISTORY:
             return cls((), list(current), ())
         else:
-            current_states = [(instance_state(c), c) for c in current]
-            original_states = [(instance_state(c), c) for c in original]
+
+            current_states = [((c is not None) and instance_state(c) or None, c) 
+                                for c in current 
+                                ]
+            original_states = [((c is not None) and instance_state(c) or None, c) 
+                                for c in original 
+                                ]
 
             current_set = dict(current_states)
             original_set = dict(original_states)
diff --git a/lib/sqlalchemy/orm/properties.py b/lib/sqlalchemy/orm/properties.py
index e7f473b67..494d94bb0 100644
--- a/lib/sqlalchemy/orm/properties.py
+++ b/lib/sqlalchemy/orm/properties.py
@@ -818,6 +818,13 @@ class RelationshipProperty(StrategizedProperty):
             if instance_state in visited_states:
                 continue
 
+            if c is None:
+                # would like to emit a warning here, but
+                # would not be consistent with collection.append(None)
+                # current behavior of silently skipping.
+                # see [ticket:2229]
+                continue
+
             instance_dict = attributes.instance_dict(c)
 
             if halt_on and halt_on(instance_state):
diff --git a/test/orm/test_cascade.py b/test/orm/test_cascade.py
index 8c741f0a3..0ff30f906 100644
--- a/test/orm/test_cascade.py
+++ b/test/orm/test_cascade.py
@@ -307,6 +307,64 @@ class O2MCascadeDeleteOrphanTest(fixtures.MappedTest):
         assert users.count().scalar() == 1
         assert orders.count().scalar() == 0
 
+class O2MCascadeTest(fixtures.MappedTest):
+    run_inserts = None
+
+    @classmethod
+    def define_tables(cls, metadata):
+        Table('users', metadata,
+              Column('id', Integer, primary_key=True, test_needs_autoincrement=True),
+              Column('name', String(30), nullable=False),
+        )
+        Table('addresses', metadata,
+              Column('id', Integer, primary_key=True, test_needs_autoincrement=True),
+              Column('user_id', Integer, ForeignKey('users.id')),
+              Column('email_address', String(50), nullable=False),
+        )
+
+    @classmethod
+    def setup_classes(cls):
+        class User(cls.Comparable):
+            pass
+        class Address(cls.Comparable):
+            pass
+
+    @classmethod
+    def setup_mappers(cls):
+        users, User, Address, addresses = (
+                    cls.tables.users, cls.classes.User, 
+                    cls.classes.Address, cls.tables.addresses)
+
+        mapper(Address, addresses)
+        mapper(User, users, properties={
+           'addresses':relationship(Address, backref="user"),
+
+        })
+
+    def test_none_skipped_assignment(self):
+        # [ticket:2229] proposes warning/raising on None
+        # for 0.8
+        User, Address = self.classes.User, self.classes.Address
+        s = Session()
+        u1 = User(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
+        User, Address = self.classes.User, self.classes.Address
+        s = Session()
+
+        u1 = User()
+        s.add(u1)
+        u1.addresses.append(None)
+        eq_(u1.addresses, [None])
+        s.commit()
+        eq_(u1.addresses, [])
+
 class O2MCascadeDeleteNoOrphanTest(fixtures.MappedTest):
     run_inserts = None