From: Mike Bayer <mike_mp@zzzcomputing.com>
Date: Wed, 3 Oct 2012 15:06:22 +0000 (-0400)
Subject:   - [bug] Fixed Session accounting bug whereby replacing
X-Git-Tag: rel_0_7_10~35
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=49703bf01a4fd7889d00ed24209569c65aa86b64;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git

  - [bug] Fixed Session accounting bug whereby replacing
    a deleted object in the identity map with another
    object of the same primary key would raise a
    "conflicting state" error on rollback(),
    if the replaced primary key were established either
    via non-unitofwork-established INSERT statement
    or by primary key switch of another instance.
    [ticket:2583]
---

diff --git a/CHANGES b/CHANGES
index 09e5d6f50b..0e16cc35a1 100644
--- a/CHANGES
+++ b/CHANGES
@@ -3,6 +3,18 @@
 =======
 CHANGES
 =======
+0.7.10
+======
+- orm
+  - [bug] Fixed Session accounting bug whereby replacing
+    a deleted object in the identity map with another
+    object of the same primary key would raise a
+    "conflicting state" error on rollback(),
+    if the replaced primary key were established either
+    via non-unitofwork-established INSERT statement
+    or by primary key switch of another instance.
+    [ticket:2583]
+
 0.7.9
 =====
 - orm
diff --git a/lib/sqlalchemy/orm/session.py b/lib/sqlalchemy/orm/session.py
index 188c3b2e76..1651648acd 100644
--- a/lib/sqlalchemy/orm/session.py
+++ b/lib/sqlalchemy/orm/session.py
@@ -251,6 +251,7 @@ class SessionTransaction(object):
         if not self._is_transaction_boundary:
             self._new = self._parent._new
             self._deleted = self._parent._deleted
+            self._key_switches = self._parent._key_switches
             return
 
         if not self.session._flushing:
@@ -258,6 +259,7 @@ class SessionTransaction(object):
 
         self._new = weakref.WeakKeyDictionary()
         self._deleted = weakref.WeakKeyDictionary()
+        self._key_switches = weakref.WeakKeyDictionary()
 
     def _restore_snapshot(self):
         assert self._is_transaction_boundary
@@ -267,11 +269,16 @@ class SessionTransaction(object):
             if s.key:
                 del s.key
 
+        for s, (oldkey, newkey) in self._key_switches.items():
+            self.session.identity_map.discard(s)
+            s.key = oldkey
+            self.session.identity_map.replace(s)
+
         for s in set(self._deleted).union(self.session._deleted):
             if s.deleted:
                 #assert s in self._deleted
                 del s.deleted
-            self.session._update_impl(s)
+            self.session._update_impl(s, discard_existing=True)
 
         assert not self.session._deleted
 
@@ -1323,6 +1330,11 @@ class Session(object):
                 # state has already replaced this one in the identity
                 # map (see test/orm/test_naturalpks.py ReversePKsTest)
                 self.identity_map.discard(state)
+                if state in self.transaction._key_switches:
+                    orig_key = self.transaction._key_switches[0]
+                else:
+                    orig_key = state.key
+                self.transaction._key_switches[state] = (orig_key, instance_key)
                 state.key = instance_key
 
             self.identity_map.replace(state)
@@ -1601,7 +1613,7 @@ class Session(object):
             self._new[state] = state.obj()
             state.insert_order = len(self._new)
 
-    def _update_impl(self, state):
+    def _update_impl(self, state, discard_existing=False):
         if (self.identity_map.contains_state(state) and
             state not in self._deleted):
             return
@@ -1617,6 +1629,10 @@ class Session(object):
                 "function to send this object back to the transient state." %
                 mapperutil.state_str(state)
             )
+        if discard_existing:
+            existing = self.identity_map.get(state.key)
+            if existing is not None:
+                self.identity_map.discard(attributes.instance_state(existing))
         self._attach(state)
         self._deleted.pop(state, None)
         self.identity_map.add(state)
diff --git a/test/orm/test_transaction.py b/test/orm/test_transaction.py
index 30bbd91946..12a995805b 100644
--- a/test/orm/test_transaction.py
+++ b/test/orm/test_transaction.py
@@ -1127,7 +1127,56 @@ class NaturalPKRollbackTest(fixtures.MappedTest):
 
         session.rollback()
 
+    def test_key_replaced_by_update(self):
+        users, User = self.tables.users, self.classes.User
+
+        mapper(User, users)
+
+        u1 = User(name='u1')
+        u2 = User(name='u2')
+
+        s = Session()
+        s.add_all([u1, u2])
+        s.commit()
+
+        s.delete(u1)
+        s.flush()
+
+        u2.name = 'u1'
+        s.flush()
+
+        assert u1 not in s
+        s.rollback()
+
+        assert u1 in s
+        assert u2 in s
 
+        assert s.identity_map[(User, ('u1',))] is u1
+        assert s.identity_map[(User, ('u2',))] is u2
+
+    def test_key_replaced_by_oob_insert(self):
+        users, User = self.tables.users, self.classes.User
+
+        mapper(User, users)
+
+        u1 = User(name='u1')
+
+        s = Session()
+        s.add(u1)
+        s.commit()
+
+        s.delete(u1)
+        s.flush()
+
+        s.execute(users.insert().values(name='u1'))
+        u2 = s.query(User).get('u1')
+
+        assert u1 not in s
+        s.rollback()
+
+        assert u1 in s
+        assert u2 not in s
 
+        assert s.identity_map[(User, ('u1',))] is u1