Don't mutate old collection on bulk replace

For a bulk replace, assume the old collection is no longer
useful to the attribute system and only send the removal events,
not actually mutated the collection.

this changes behavior significantly and also means that dispose_collection
now receives the old collection intact.

Change-Id: Ic2685c85438191f07797d9ef97833a2cfdc4fcc2
Fixes: #3913

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index 55050c482f55f7a143974ca9840ed5b13947e0d0..f4fd6a3fe810f29adae343c973bd53b9cd5383ca 100644 (file)
--- a/doc/build/changelog/changelog_12.rst
+++ b/doc/build/changelog/changelog_12.rst
@@ -46,6 +46,20 @@
 
             :ref:`change_3891`
 
+    .. change:: 3913
+        :tags: bug, orm
+        :tickets: 3913
+
+        When assigning a collection to an attribute mapped by a relationship,
+        the previous collection is no longer mutated.  Previously, the old
+        collection would be emptied out in conjunction with the "item remove"
+        events that fire off; the events now fire off without affecting
+        the old collection.
+
+        .. seealso::
+
+            :ref:`change_3913`
+
     .. change:: 3932
         :tags: bug, oracle
         :tickets: 3932

diff --git a/doc/build/changelog/migration_12.rst b/doc/build/changelog/migration_12.rst
index d64200d120661e367bd3f8c527af05a6bc86b1df..da21f11b4fd90e8e1e5128e63b6424271c354b53 100644 (file)
--- a/doc/build/changelog/migration_12.rst
+++ b/doc/build/changelog/migration_12.rst
@@ -139,6 +139,33 @@ WHERE clause manually may need to be adjusted.
 
 :ticket:`3891`
 
+.. _change_3913:
+
+Previous collection is no longer mutated upon replacement
+---------------------------------------------------------
+
+The ORM emits events whenever the members of a mapped collection change.
+In the case of assigning a collection to an attribute that would replace
+the previous collection, a side effect of this was that the collection
+being replaced would also be mutated, which is misleading and unnecessary::
+
+    >>> a1, a2, a3 = Address('a1'), Address('a2'), Address('a3')
+    >>> user.addresses = [a1, a2]
+
+    >>> previous_collection = user.addresses
+
+    # replace the collection with a new one
+    >>> user.addresses = [a2, a3]
+
+    >>> previous_collection
+    [Address('a1'), Address('a2')]
+
+Above, prior to the change, the ``previous_collection`` would have had the
+"a1" member removed, corresponding to the member that's no longer in the
+new collection.
+
+:ticket:`3913`
+
 Key Behavioral Changes - Core
 =============================
 

diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py
index 2bb53e61e391619353ee6517fa94dbff4fc383f1..d949dc8a17f11f73e7a8a265c62501da4fe4a412 100644 (file)
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -764,9 +764,8 @@ def bulk_replace(values, existing_adapter, new_adapter):
             appender(member, _sa_initiator=False)
 
     if existing_adapter:
-        remover = existing_adapter.bulk_remover()
         for member in removals:
-            remover(member)
+            existing_adapter.fire_remove_event(member)
 
 
 def prepare_instrumentation(factory):

diff --git a/lib/sqlalchemy/orm/events.py b/lib/sqlalchemy/orm/events.py
index d2ccec432fc1b702b6f4f29b43d309e2c24ea6d6..ceeb31e9c34bd7a4e61ca68b2ad1a59b3ed44a05 100644 (file)
--- a/lib/sqlalchemy/orm/events.py
+++ b/lib/sqlalchemy/orm/events.py
@@ -2124,8 +2124,12 @@ class AttributeEvents(event.Events):
 
             u1.addresses = [a2, a3]  # <- old collection is disposed
 
-        The mechanics of the event will typically include that the given
-        collection is empty, even if it stored objects while being replaced.
+        The old collection received will contain its previous contents.
+
+        .. versionchanged:: 1.2 The collection passed to
+           :meth:`.AttributeEvents.dispose_collection` will now have its
+           contents before the dispose intact; previously, the collection
+           would be empty.
 
         .. versionadded:: 1.0.0 the :meth:`.AttributeEvents.init_collection`
            and :meth:`.AttributeEvents.dispose_collection` events supersede

diff --git a/test/orm/test_attributes.py b/test/orm/test_attributes.py
index d6f3caa0276ad8d7de4d6ae707e0650b5551379e..d3a63c3868042256ce4add365219983df0439ef3 100644 (file)
--- a/test/orm/test_attributes.py
+++ b/test/orm/test_attributes.py
@@ -2687,13 +2687,15 @@ class ListenerTest(fixtures.ORMTest):
         f1.barlist = [b2]
         adapter_two = f1.barlist._sa_adapter
         eq_(canary.init.mock_calls, [
-            call(f1, [], adapter_one),
+            call(f1, [b1], adapter_one),  # note the f1.barlist that
+                                          # we saved earlier has been mutated
+                                          # in place, new as of [ticket:3913]
             call(f1, [b2], adapter_two),
         ])
         eq_(
             canary.dispose.mock_calls,
             [
-                call(f1, [], adapter_one)
+                call(f1, [b1], adapter_one)
             ]
         )
 
@@ -2903,10 +2905,11 @@ class TestUnlink(fixtures.TestBase):
         coll = a1.bs
         a1.bs.append(B())
         a1.bs = []
-        # a bulk replace empties the old collection
-        assert len(coll) == 0
-        coll.append(B())
+        # a bulk replace no longer empties the old collection
+        # as of [ticket:3913]
         assert len(coll) == 1
+        coll.append(B())
+        assert len(coll) == 2
 
     def test_pop_existing(self):
         A, B = self.A, self.B

diff --git a/test/orm/test_collection.py b/test/orm/test_collection.py
index e3c69a9a6e1c7f127e1a8d0310ef82b8c6f50676..d059db6e57858b30415efa27fb33a8180155c75f 100644 (file)
--- a/test/orm/test_collection.py
+++ b/test/orm/test_collection.py
@@ -1551,7 +1551,11 @@ class CollectionsTest(fixtures.ORMTest):
         bulk1 = [e2]
         # empty & sever col1 from obj
         obj.attr = bulk1
-        self.assert_(len(col1) == 0)
+
+        # as of [ticket:3913] the old collection
+        # remains unchanged
+        self.assert_(len(col1) == 1)
+
         self.assert_(len(canary.data) == 1)
         self.assert_(obj.attr is not col1)
         self.assert_(obj.attr is not bulk1)