- Some fixes to the state handling regarding

backrefs, typically when autoflush=False, where
the back-referenced collection wouldn't
properly handle add/removes with no net
change.  Thanks to Richard Murri for the
test case + patch.  [ticket:2123]
(also in 0.6.7).

diff --git a/CHANGES b/CHANGES
index a1d49f0368062586b9ecd35294c02df5bf6bc0fc..ddf6467c7b3be46c3eeee74741893bb42bd33d6a 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -46,6 +46,14 @@ CHANGES
     can't find the target entity.  Explain that the
     path must be from one of the root entities.
 
+  - Some fixes to the state handling regarding 
+    backrefs, typically when autoflush=False, where
+    the back-referenced collection wouldn't 
+    properly handle add/removes with no net 
+    change.  Thanks to Richard Murri for the 
+    test case + patch.  [ticket:2123]
+    (also in 0.6.7).
+
 - sql
   - Restored the "catchall" constructor on the base
     TypeEngine class, with a deprecation warning.

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index ead62d1fee88d0499936af905c5cc4adc0b03258..c084db8a8858cdb9c6c7d2297a0eb44a26f9cb68 100644 (file)
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -871,7 +871,6 @@ class CollectionAttributeImpl(AttributeImpl):
         state.commit(dict_, [self.key])
 
         if self.key in state.pending:
-
             # pending items exist.  issue a modified event,
             # add/remove new items.
             state.modified_event(dict_, self, user_data, True)

diff --git a/lib/sqlalchemy/orm/state.py b/lib/sqlalchemy/orm/state.py
index e29c5c03ad1ee37845faad4331c45ffc01a40ee0..00582fbdbd7d7231a46189c5202be4cc91d6ba37 100644 (file)
--- a/lib/sqlalchemy/orm/state.py
+++ b/lib/sqlalchemy/orm/state.py
@@ -547,10 +547,12 @@ class PendingCollection(object):
     def append(self, value):
         if value in self.deleted_items:
             self.deleted_items.remove(value)
-        self.added_items.add(value)
+        else:
+            self.added_items.add(value)
 
     def remove(self, value):
         if value in self.added_items:
             self.added_items.remove(value)
-        self.deleted_items.add(value)
+        else:
+            self.deleted_items.add(value)
 

diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index a05f5f70dffdbc62b490cfc9247301fd1e0f749a..b8002a446fba4f858366237fd17eb96db21a46ce 100644 (file)
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -1041,24 +1041,29 @@ class PendingBackrefTest(fixtures.ORMTest):
                             ([p, p4], [p1, p2, p3], []))
         assert called[0] == 1
 
-    def test_lazy_remove(self):
-        global lazy_load
-        called[0] = 0
-        lazy_load = []
-
+    def test_state_on_add_remove(self):
         b = Blog("blog 1")
         p = Post("post 1")
         p.blog = b
-        assert called[0] == 0
+        p.blog = None
 
-        lazy_load = [p]
+        eq_(called[0], 0)
+        eq_(b.posts, [])
+        eq_(called[0], 1)
 
-        p.blog = None
+    def test_pending_combines_with_lazy(self):
+        global lazy_load
+        b = Blog("blog 1")
+        p = Post("post 1")
         p2 = Post("post 2")
-        p2.blog = b
-        assert called[0] == 0
-        assert b.posts == [p2]
-        assert called[0] == 1
+        p.blog = b
+        lazy_load = [p, p2]
+        # lazy loaded + pending get added together.
+        # This isn't seen often with the ORM due
+        # to usual practices surrounding the 
+        # load/flush/load cycle.
+        eq_(b.posts, [p, p2, p])
+        eq_(called[0], 1)
 
     def test_normal_load(self):
         global lazy_load

diff --git a/test/orm/test_backref_mutations.py b/test/orm/test_backref_mutations.py
index 3d13ae02db6bdd1f41ce0eb5c4861693f0640fb6..0e11655044b5ebed146e223464b05b3863ef8870 100644 (file)
--- a/test/orm/test_backref_mutations.py
+++ b/test/orm/test_backref_mutations.py
@@ -13,7 +13,8 @@ from test.lib.testing import assert_raises, assert_raises_message
 from sqlalchemy import Integer, String, ForeignKey, Sequence, exc as sa_exc
 from test.lib.schema import Table
 from test.lib.schema import Column
-from sqlalchemy.orm import mapper, relationship, create_session, class_mapper, backref, sessionmaker
+from sqlalchemy.orm import mapper, relationship, create_session, \
+    class_mapper, backref, sessionmaker, Session
 from sqlalchemy.orm import attributes, exc as orm_exc
 from test.lib import testing
 from test.lib.testing import eq_
@@ -238,6 +239,7 @@ class O2MCollectionTest(_fixtures.FixtureTest):
         assert a1 not in u1.addresses
         assert a1 in u2.addresses
 
+
 class O2OScalarBackrefMoveTest(_fixtures.FixtureTest):
     run_inserts = None
 
@@ -477,6 +479,118 @@ class O2OScalarOrphanTest(_fixtures.FixtureTest):
         assert sess.query(User).count() == 1
 
 
+class M2MCollectionMoveTest(_fixtures.FixtureTest):
+    run_inserts = None
+
+    @classmethod
+    def setup_mappers(cls):
+        keywords, items, item_keywords, Keyword, Item = (cls.tables.keywords,
+                                cls.tables.items,
+                                cls.tables.item_keywords,
+                                cls.classes.Keyword,
+                                cls.classes.Item)
+
+        mapper(Item, items, properties={
+            'keywords':relationship(Keyword, secondary=item_keywords, 
+                                    backref='items')
+        })
+        mapper(Keyword, keywords)
+
+    def test_add_remove_pending_backref(self):
+        """test that pending doesn't add an item that's not a net add."""
+
+        Item, Keyword = (self.classes.Item, self.classes.Keyword)
+
+        session = Session(autoflush=False)
+
+        i1 = Item(description='i1')
+        session.add(i1)
+        session.commit()
+
+        session.expire(i1, ['keywords'])
+
+        k1= Keyword(name='k1')
+        k1.items.append(i1)
+        k1.items.remove(i1)
+        eq_(i1.keywords, [])
+
+    def test_remove_add_pending_backref(self):
+        """test that pending doesn't remove an item that's not a net remove."""
+
+        Item, Keyword = (self.classes.Item, self.classes.Keyword)
+
+        session = Session(autoflush=False)
+
+        k1= Keyword(name='k1')
+        i1 = Item(description='i1', keywords=[k1])
+        session.add(i1)
+        session.commit()
+
+        session.expire(i1, ['keywords'])
+
+        k1.items.remove(i1)
+        k1.items.append(i1)
+        eq_(i1.keywords, [k1])
+
+    def test_pending_combines_with_flushed(self):
+        """test the combination of unflushed pending + lazy loaded from DB."""
+
+        Item, Keyword = (self.classes.Item, self.classes.Keyword)
+
+        session = Session(testing.db, autoflush=False)
+
+        k1 = Keyword(name='k1')
+        k2 = Keyword(name='k2')
+        i1 = Item(description='i1', keywords=[k1])
+        session.add(i1)
+        session.add(k2)
+        session.commit()
+
+        k2.items.append(i1)
+        # the pending
+        # list is still here.
+        eq_(
+            set(attributes.instance_state(i1).
+                pending['keywords'].added_items),
+            set([k2])
+        )
+        # because autoflush is off, k2 is still
+        # coming in from pending
+        eq_(i1.keywords, [k1, k2])
+
+        # prove it didn't flush
+        eq_(session.scalar("select count(*) from item_keywords"), 1)
+
+        # the pending collection was removed
+        assert 'keywords' not in attributes.\
+                                instance_state(i1).\
+                                pending
+
+    def test_duplicate_adds(self):
+        Item, Keyword = (self.classes.Item, self.classes.Keyword)
+
+        session = Session(testing.db, autoflush=False)
+
+        k1 = Keyword(name='k1')
+        i1 = Item(description='i1', keywords=[k1])
+        session.add(i1)
+        session.commit()
+
+        k1.items.append(i1)
+        eq_(i1.keywords, [k1, k1])
+
+        session.expire(i1, ['keywords'])
+        k1.items.append(i1)
+        eq_(i1.keywords, [k1, k1])
+
+        session.expire(i1, ['keywords'])
+        k1.items.append(i1)
+        eq_(i1.keywords, [k1, k1])
+
+        eq_(k1.items, [i1, i1, i1, i1])
+        session.commit()
+        eq_(k1.items, [i1])
+
 class M2MScalarMoveTest(_fixtures.FixtureTest):
     run_inserts = None
 
@@ -489,7 +603,9 @@ class M2MScalarMoveTest(_fixtures.FixtureTest):
                                 cls.classes.Item)
 
         mapper(Item, items, properties={
-            'keyword':relationship(Keyword, secondary=item_keywords, uselist=False, backref=backref("item", uselist=False))
+            'keyword':relationship(Keyword, secondary=item_keywords, 
+                                    uselist=False, 
+                                    backref=backref("item", uselist=False))
         })
         mapper(Keyword, keywords)