- Removed errant many-to-many load in unitofwork

which triggered unnecessarily on expired/unloaded
collections. This load now takes place only if
passive_updates is False and the parent primary
key has changed, or if passive_deletes is False
and a delete of the parent has occurred.
[ticket:1845]

diff --git a/CHANGES b/CHANGES
index 11ae0e575134c6f9984086e7d71a831b5dac6501..6f188b10e00d8674b8ea828963209247bf0a4e1b 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -3,6 +3,17 @@
 =======
 CHANGES
 =======
+0.6.3
+=====
+- orm
+  - Removed errant many-to-many load in unitofwork 
+    which triggered unnecessarily on expired/unloaded 
+    collections. This load now takes place only if 
+    passive_updates is False and the parent primary 
+    key has changed, or if passive_deletes is False 
+    and a delete of the parent has occurred.
+    [ticket:1845]
+  
 0.6.2
 =====
 - orm

diff --git a/lib/sqlalchemy/orm/dependency.py b/lib/sqlalchemy/orm/dependency.py
index bd4473ef79a3a8d1dbad85440424316a20b52d86..baa7af645ec0bf302ec41e5911ca0adfcdde7f26 100644 (file)
--- a/lib/sqlalchemy/orm/dependency.py
+++ b/lib/sqlalchemy/orm/dependency.py
@@ -871,6 +871,9 @@ class ManyToManyDP(DependencyProcessor):
         
     def presort_deletes(self, uowcommit, states):
         if not self.passive_deletes:
+            # if no passive deletes, load history on 
+            # the collection, so that prop_has_changes()
+            # returns True
             for state in states:
                 history = uowcommit.get_attribute_history(
                                         state, 
@@ -878,16 +881,22 @@ class ManyToManyDP(DependencyProcessor):
                                         passive=self.passive_deletes)
         
     def presort_saves(self, uowcommit, states):
-        for state in states:
-            history = uowcommit.get_attribute_history(
-                                    state, 
-                                    self.key, 
-                                    False)
+        if not self.passive_updates:
+            # if no passive updates, load history on 
+            # each collection where parent has changed PK,
+            # so that prop_has_changes() returns True
+            for state in states:
+                if self._pks_changed(uowcommit, state):
+                    history = uowcommit.get_attribute_history(
+                                        state, 
+                                        self.key, 
+                                        False)
 
-        
         if not self.cascade.delete_orphan:
             return
-            
+        
+        # check for child items removed from the collection
+        # if delete_orphan check is turned on.
         for state in states:
             history = uowcommit.get_attribute_history(
                                         state, 
@@ -911,6 +920,8 @@ class ManyToManyDP(DependencyProcessor):
         processed = self._get_reversed_processed_set(uowcommit)
         tmp = set()
         for state in states:
+            # this history should be cached already, as 
+            # we loaded it in preprocess_deletes
             history = uowcommit.get_attribute_history(
                                     state, 
                                     self.key, 
@@ -947,7 +958,10 @@ class ManyToManyDP(DependencyProcessor):
         tmp = set()
 
         for state in states:
-            history = uowcommit.get_attribute_history(state, self.key)
+            need_cascade_pks = not self.passive_updates and \
+                                self._pks_changed(uowcommit, state) 
+            history = uowcommit.get_attribute_history(state, self.key,
+                                                passive=not need_cascade_pks)
             if history:
                 for child in history.added:
                     if child is None or \
@@ -976,28 +990,22 @@ class ManyToManyDP(DependencyProcessor):
                 tmp.update((c, state) 
                             for c in history.added + history.deleted)
                 
-            if not self.passive_updates and \
-                    self._pks_changed(uowcommit, state):
-                if not history:
-                    history = uowcommit.get_attribute_history(
-                                        state, 
-                                        self.key, 
-                                        passive=False)
-                
-                for child in history.unchanged:
-                    associationrow = {}
-                    sync.update(state, 
-                                self.parent, 
-                                associationrow, 
-                                "old_", 
-                                self.prop.synchronize_pairs)
-                    sync.update(child, 
-                                self.mapper, 
-                                associationrow, 
-                                "old_", 
-                                self.prop.secondary_synchronize_pairs)
-
-                    secondary_update.append(associationrow)
+                if need_cascade_pks:
+                    
+                    for child in history.unchanged:
+                        associationrow = {}
+                        sync.update(state, 
+                                    self.parent, 
+                                    associationrow, 
+                                    "old_", 
+                                    self.prop.synchronize_pairs)
+                        sync.update(child, 
+                                    self.mapper, 
+                                    associationrow, 
+                                    "old_", 
+                                    self.prop.secondary_synchronize_pairs)
+
+                        secondary_update.append(associationrow)
                     
         if processed is not None:
             processed.update(tmp)

diff --git a/test/orm/test_unitofworkv2.py b/test/orm/test_unitofworkv2.py
index 218d9b9cc4b68f6955efe5729b5bd9cd2ab5c501..d9179930533531bbad1e06650519fc53b487d4ec 100644 (file)
--- a/test/orm/test_unitofworkv2.py
+++ b/test/orm/test_unitofworkv2.py
@@ -223,6 +223,47 @@ class RudimentaryFlushTest(UOWTest):
                 ),
         )
     
+    def test_many_to_many(self):
+        mapper(Item, items, properties={
+            'keywords':relationship(Keyword, secondary=item_keywords)
+        })
+        mapper(Keyword, keywords)
+        
+        sess = create_session()
+        k1 = Keyword(name='k1')
+        i1 = Item(description='i1', keywords=[k1])
+        sess.add(i1)
+        self.assert_sql_execution(
+                testing.db,
+                sess.flush,
+                AllOf(
+                    CompiledSQL(
+                    "INSERT INTO keywords (name) VALUES (:name)",
+                    {'name':'k1'}
+                    ),
+                    CompiledSQL(
+                    "INSERT INTO items (description) VALUES (:description)",
+                    {'description':'i1'}
+                    ),
+                ),
+                CompiledSQL(
+                    "INSERT INTO item_keywords (item_id, keyword_id) "
+                    "VALUES (:item_id, :keyword_id)",
+                    lambda ctx:{'item_id':i1.id, 'keyword_id':k1.id}
+                )
+        )
+        
+        # test that keywords collection isn't loaded
+        sess.expire(i1, ['keywords'])
+        i1.description = 'i2'
+        self.assert_sql_execution(
+                testing.db,
+                sess.flush,
+                CompiledSQL("UPDATE items SET description=:description "
+                            "WHERE items.id = :items_id", 
+                            lambda ctx:{'description':'i2', 'items_id':i1.id})
+        )
+        
     def test_m2o_flush_size(self):
         mapper(User, users)
         mapper(Address, addresses, properties={
@@ -619,12 +660,22 @@ class SingleCycleM2MTest(_base.MappedTest,
                     (n3.id, n5.id), (n3.id, n4.id)
                 ])
         )
-
+        
         sess.delete(n1)
         
         self.assert_sql_execution(
                 testing.db,
                 sess.flush,
+                # this is n1.parents firing off, as it should, since
+                # passive_deletes is False for n1.parents
+                CompiledSQL(
+                    "SELECT nodes.id AS nodes_id, nodes.data AS nodes_data, "
+                    "nodes.favorite_node_id AS nodes_favorite_node_id FROM "
+                    "nodes, node_to_nodes WHERE :param_1 = "
+                    "node_to_nodes.right_node_id AND nodes.id = "
+                    "node_to_nodes.left_node_id" ,
+                    lambda ctx:{u'param_1': n1.id},
+                ),    
                 CompiledSQL(
                     "DELETE FROM node_to_nodes WHERE "
                     "node_to_nodes.left_node_id = :left_node_id AND "