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

6 files changed, 61 insertions, 10 deletions

diff --git a/doc/build/changelog/changelog_12.rst b/doc/build/changelog/changelog_12.rst
index 55050c482..f4fd6a3fe 100644
--- 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 d64200d12..da21f11b4 100644
--- 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 2bb53e61e..d949dc8a1 100644
--- 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 d2ccec432..ceeb31e9c 100644
--- 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 d6f3caa02..d3a63c386 100644
--- 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 e3c69a9a6..d059db6e5 100644
--- 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)