From: Mike Bayer Date: Tue, 14 Feb 2017 16:39:44 +0000 (-0500) Subject: Don't mutate old collection on bulk replace X-Git-Tag: rel_1_2_0b1~149 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2bfe19152d49b969acdc4607bf7c33727f796f5a;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git 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 55050c482f..f4fd6a3fe8 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 d64200d120..da21f11b4f 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 2bb53e61e3..d949dc8a17 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 d2ccec432f..ceeb31e9c3 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 d6f3caa027..d3a63c3868 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 e3c69a9a6e..d059db6e57 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)