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

diff --git a/CHANGES b/CHANGES
index a7da519f18e23cfbe25e0f9d972b1ed493b76075..24ce33f287da1f37d65ac6037a755de9db6a469e 100644 (file)
--- 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 c7215194887520377ee28320e53306bf92a4c78c..a24348c8606b83942ea360dbae44c414a64c0235 100644 (file)
--- 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 e7f473b6753c80075ac2b1eaeeccec2662ba805a..494d94bb02bf4bcad20e5c146cc9d5e4a9ecaf1f 100644 (file)
--- 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 8c741f0a372517fd365926ba84854f7664696ffd..0ff30f906eca5e2725ecfd39010c87e143255505 100644 (file)
--- 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