Call __del() before remove()

The "remove" event for collections is now called before the item is removed
in the case of the ``collection.remove()`` method, as is consistent with the
behavior for most other forms of collection item removal (such as
``__delitem__``, replacement under ``__setitem__``).  The ``pop()`` methods
are now the only exception as the target item is not available until after
the pop operation proceeds.

This allows ``remove()`` to be consistent in its behavior with all
the other collection operations, allows the "before_delete" hook
to be local to "pop()" operations only, and removes some method overhead.

We are also looking here to gain some more predictability in terms
of the fix for #1103.

Change-Id: I4fdea911517d65cc300fae0e9c351a471e52e4ab

2 files changed, 36 insertions, 13 deletions

diff --git a/lib/sqlalchemy/orm/attributes.py b/lib/sqlalchemy/orm/attributes.py
index ff730d745..5ba8be439 100644
--- a/lib/sqlalchemy/orm/attributes.py
+++ b/lib/sqlalchemy/orm/attributes.py
@@ -973,6 +973,14 @@ class CollectionAttributeImpl(AttributeImpl):
         return value
 
     def fire_pre_remove_event(self, state, dict_, initiator):
+        """A special event used for pop() operations.
+
+        The "remove" event needs to have the item to be removed passed to
+        it, which in the case of pop from a set, we don't have a way to access
+        the item before the operation.   the event is used for all pop()
+        operations (even though set.pop is the one where it is really needed).
+
+        """
         state._modified_event(dict_, self, NEVER_SET, True)
 
     def fire_remove_event(self, state, dict_, value, initiator):
diff --git a/lib/sqlalchemy/orm/collections.py b/lib/sqlalchemy/orm/collections.py
index d6c23f5d2..54c29bb5e 100644
--- a/lib/sqlalchemy/orm/collections.py
+++ b/lib/sqlalchemy/orm/collections.py
@@ -1009,7 +1009,11 @@ def _instrument_membership_mutator(method, before, argument, after):
 
 
 def __set(collection, item, _sa_initiator=None):
-    """Run set events, may eventually be inlined into decorators."""
+    """Run set events.
+
+    This event always occurs before the collection is actually mutated.
+
+    """
 
     if _sa_initiator is not False:
         executor = collection._sa_adapter
@@ -1019,15 +1023,22 @@ def __set(collection, item, _sa_initiator=None):
 
 
 def __del(collection, item, _sa_initiator=None):
-    """Run del events, may eventually be inlined into decorators."""
+    """Run del events.
+
+    This event occurs before the collection is actually mutated, *except*
+    in the case of a pop operation, in which case it occurs afterwards.
+    For pop operations, the __before_pop hook is called before the
+    operation occurs.
+
+    """
     if _sa_initiator is not False:
         executor = collection._sa_adapter
         if executor:
             executor.fire_remove_event(item, _sa_initiator)
 
 
-def __before_delete(collection, _sa_initiator=None):
-    """Special method to run 'commit existing value' methods"""
+def __before_pop(collection, _sa_initiator=None):
+    """An event which occurs on a before a pop() operation occurs."""
     executor = collection._sa_adapter
     if executor:
         executor.fire_pre_remove_event(_sa_initiator)
@@ -1049,10 +1060,9 @@ def _list_decorators():
 
     def remove(fn):
         def remove(self, value, _sa_initiator=None):
-            __before_delete(self, _sa_initiator)
+            __del(self, value, _sa_initiator)
             # testlib.pragma exempt:__eq__
             fn(self, value)
-            __del(self, value, _sa_initiator)
         _tidy(remove)
         return remove
 
@@ -1156,7 +1166,7 @@ def _list_decorators():
 
     def pop(fn):
         def pop(self, index=-1):
-            __before_delete(self)
+            __before_pop(self)
             item = fn(self, index)
             __del(self, item)
             return item
@@ -1218,18 +1228,21 @@ def _dict_decorators():
 
     def pop(fn):
         def pop(self, key, default=Unspecified):
-            if key in self:
-                __del(self, self[key])
+            __before_pop(self)
+            _to_del = key in self
             if default is Unspecified:
-                return fn(self, key)
+                item = fn(self, key)
             else:
-                return fn(self, key, default)
+                item = fn(self, key, default)
+            if _to_del:
+                __del(self, item)
+            return item
         _tidy(pop)
         return pop
 
     def popitem(fn):
         def popitem(self):
-            __before_delete(self)
+            __before_pop(self)
             item = fn(self)
             __del(self, item[1])
             return item
@@ -1324,8 +1337,10 @@ def _set_decorators():
 
     def pop(fn):
         def pop(self):
-            __before_delete(self)
+            __before_pop(self)
             item = fn(self)
+            # for set in particular, we have no way to access the item
+            # that will be popped before pop is called.
             __del(self, item)
             return item
         _tidy(pop)