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