From f3cca5255b6dfaa0771a443c0119f5463ce9d56f Mon Sep 17 00:00:00 2001
From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Fri, 5 Sep 2008 17:16:11 +0000
Subject: [PATCH] - rearranged delete() so that the object is attached before
 cascades fire off [ticket:5058] - after_attach() only fires if the object was
 not previously attached

---
 lib/sqlalchemy/orm/session.py | 66 ++++++++++++++++++++---------------
 test/orm/session.py           |  8 +++--
 2 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index f2d4e150a8..66009be01c 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -1144,12 +1144,28 @@ class Session(object):
         except exc.NO_STATE:
             raise exc.UnmappedInstanceError(instance)
 
-        # grab the full cascade list first, since lazyloads/autoflush
-        # may be triggered by this operation (delete cascade lazyloads by default)
+        if state.key is None:
+            raise sa_exc.InvalidRequestError(
+                "Instance '%s' is not persisted" %
+                mapperutil.state_str(state))
+
+        if state in self._deleted:
+            return
+            
+        # ensure object is attached to allow the 
+        # cascade operation to load deferred attributes
+        # and collections
+        self._attach(state)
+
+        # grab the cascades before adding the item to the deleted list
+        # so that autoflush does not delete the item
         cascade_states = list(_cascade_state_iterator('delete', state))
-        self._delete_impl(state)
+
+        self._deleted[state] = state.obj()
+        self.identity_map.add(state)
+
         for state, m, o in cascade_states:
-            self._delete_impl(state, ignore_transient=True)
+            self._delete_impl(state)
 
     def merge(self, instance, dont_load=False,
               _recursive=None):
@@ -1252,18 +1268,12 @@ class Session(object):
         if (self.identity_map.contains_state(state) and
             state not in self._deleted):
             return
+            
         if state.key is None:
             raise sa_exc.InvalidRequestError(
                 "Instance '%s' is not persisted" %
                 mapperutil.state_str(state))
 
-        if (state.key in self.identity_map and
-            not self.identity_map.contains_state(state)):
-            raise sa_exc.InvalidRequestError(
-                "Could not update instance '%s', identity key %s; a different "
-                "instance with the same identity key already exists in this "
-                "session." % (mapperutil.state_str(state), state.key))
-
         self._attach(state)
         self._deleted.pop(state, None)
         self.identity_map.add(state)
@@ -1274,38 +1284,36 @@ class Session(object):
         else:
             self._update_impl(state)
 
-    def _delete_impl(self, state, ignore_transient=False):
-        if self.identity_map.contains_state(state) and state in self._deleted:
+    def _delete_impl(self, state):
+        if state in self._deleted:
             return
 
         if state.key is None:
-            if ignore_transient:
-                return
-            else:
-                raise sa_exc.InvalidRequestError(
-                    "Instance '%s' is not persisted" %
-                    mapperutil.state_str(state))
-        if (state.key in self.identity_map and
-            not self.identity_map.contains_state(state)):
-            raise sa_exc.InvalidRequestError(
-                "Instance '%s' is with key %s already persisted with a "
-                "different identity" % (mapperutil.state_str(state),
-                                        state.key))
-
+            return
+                    
         self._attach(state)
         self._deleted[state] = state.obj()
         self.identity_map.add(state)
-
+    
     def _attach(self, state):
+        if state.key and \
+            state.key in self.identity_map and \
+            not self.identity_map.contains_state(state):
+            raise sa_exc.InvalidRequestError(
+                "Can't attach instance %s; another instance with key %s is already present in this session." % 
+                    (mapperutil.state_str(state), state.key)
+                )
+                
         if state.session_id and state.session_id is not self.hash_key:
             raise sa_exc.InvalidRequestError(
                 "Object '%s' is already attached to session '%s' "
                 "(this is '%s')" % (mapperutil.state_str(state),
                                     state.session_id, self.hash_key))
+                                    
         if state.session_id != self.hash_key:
             state.session_id = self.hash_key
-        for ext in self.extensions:
-            ext.after_attach(self, state.obj())
+            for ext in self.extensions:
+                ext.after_attach(self, state.obj())
 
     def __contains__(self, instance):
         """Return True if the instance is associated with this session.
diff --git a/test/orm/session.py b/test/orm/session.py
index c2e6e9d15c..8dd98512e3 100644
--- a/test/orm/session.py
+++ b/test/orm/session.py
@@ -621,7 +621,10 @@ class SessionTest(_fixtures.FixtureTest):
     def test_save_update_delete(self):
 
         s = create_session()
-        mapper(User, users)
+        mapper(User, users, properties={
+            'addresses':relation(Address, cascade="all, delete")
+        })
+        mapper(Address, addresses)
 
         user = User(name='u1')
 
@@ -658,8 +661,9 @@ class SessionTest(_fixtures.FixtureTest):
         self.assertRaisesMessage(sa.exc.InvalidRequestError, "is already attached to session", s2.delete, user)
 
         u2 = s2.query(User).get(user.id)
-        self.assertRaisesMessage(sa.exc.InvalidRequestError, "already persisted with a different identity", s.delete, u2)
+        self.assertRaisesMessage(sa.exc.InvalidRequestError, "another instance with key", s.delete, u2)
 
+        s.expire(user)
         s.expunge(user)
         assert user not in s
         s.delete(user)
-- 
2.47.3