From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Sun, 24 Jul 2011 16:37:48 +0000 (-0400)
Subject: - Fixed regression from 0.6 where Session.add()
X-Git-Tag: rel_0_7_2~15
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0cc357c6caf8f5d4e33e3dedd799beeca0304518;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

- 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 a7da519f18..24ce33f287 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 c721519488..a24348c860 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 e7f473b675..494d94bb02 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 8c741f0a37..0ff30f906e 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