Merge pull request #13 from NextThought/p-changed-small-set-remove

Fix setting _p_changed when removing from small Python BTrees/TreeSets.

BTrees/_base.py

@@ -559,6 +559,7 @@ class Set(_BucketBase):
         index = self._search(key)
         if index < 0:
             index = -index - 1
+            self._p_changed = True
             self._keys.insert(index, key)
             return True, None
         return False, None
@@ -942,6 +943,12 @@ class _Tree(_Base):
 
         removed_first_bucket, value = child._del(key)
 
+        # See comment in _set about small trees
+        if (len(data) == 1 and
+            child.__class__ is self._bucket_type and
+            child._p_oid is None):
+            self._p_changed = True
+
         # fix up the node key, but not for the 0'th one.
         if index > 0 and child.size and key == data[index].key:
             self._p_changed = True

BTrees/tests/common.py

@@ -1146,6 +1146,40 @@ class BTreeTests(MappingBase):
         self.assertTrue(t._p_changed)
         self.assertEqual(t, t._p_jar.registered)
 
+    def testRemoveInSmallMapSetsChanged(self):
+        # A bug in the BTree Python implementation once caused
+        # deleting from a small btree to set _p_changed.
+        # There must be at least two objects so that _firstbucket doesn't
+        # get set
+        t = self._makeOne()
+        # Note that for the property to actually hold, we have to fake a
+        # _p_jar and _p_oid
+        t._p_oid = b'\0\0\0\0\0'
+        class Jar(object):
+            def __init__(self):
+                self._cache = self
+                self.registered = None
+
+            def mru(self, arg):
+                pass
+            def readCurrent(self, arg):
+                pass
+            def register(self, arg):
+                self.registered = arg
+
+        t._p_jar = Jar()
+        t[0] = 1
+        t[1] = 2
+        # reset these, setting _firstbucket triggered a change
+        t._p_changed = False
+        t._p_jar.registered = None
+
+        # now remove the second value
+        del t[1]
+        self.assertTrue(t._p_changed)
+        self.assertEqual(t, t._p_jar.registered)
+
+
 class NormalSetTests(Base):
     # Test common to all set types
 
@@ -1354,6 +1388,66 @@ class NormalSetTests(Base):
                 pass
             self.assertEqual(x, keys)
 
+    def testRemoveInSmallSetSetsChanged(self):
+        # A bug in the BTree TreeSet Python implementation once caused
+        # deleting an item in a small set to fail to set _p_changed.
+        # There must be at least two objects so that _firstbucket doesn't
+        # get set
+        t = self._makeOne()
+        # Note that for the property to actually hold, we have to fake a
+        # _p_jar and _p_oid
+        t._p_oid = b'\0\0\0\0\0'
+        class Jar(object):
+            def __init__(self):
+                self._cache = self
+                self.registered = None
+
+            def mru(self, arg):
+                pass
+            def readCurrent(self, arg):
+                pass
+            def register(self, arg):
+                self.registered = arg
+
+        t._p_jar = Jar()
+        t.add(0)
+        t.add(1)
+        # reset these, setting _firstbucket triggered a change
+        t._p_changed = False
+        t._p_jar.registered = None
+
+        # now remove the second value
+        t.remove(1)
+        self.assertTrue(t._p_changed)
+        self.assertEqual(t, t._p_jar.registered)
+
+    def testAddingOneSetsChanged(self):
+        # A bug in the BTree Set Python implementation once caused
+        # adding an object not to set _p_changed
+        t = self._makeOne()
+        # Note that for the property to actually hold, we have to fake a
+        # _p_jar and _p_oid
+        t._p_oid = b'\0\0\0\0\0'
+        class Jar(object):
+            def __init__(self):
+                self._cache = self
+                self.registered = None
+
+            def mru(self, arg):
+                pass
+            def readCurrent(self, arg):
+                pass
+            def register(self, arg):
+                self.registered = arg
+
+        t._p_jar = Jar()
+        t.add(0)
+        self.assertTrue(t._p_changed)
+        self.assertEqual(t, t._p_jar.registered)
+
+        # Whether or not doing `t.add(0)` again would result in
+        # _p_changed being set depends on whether this is a TreeSet or a plain Set
+
 class ExtendedSetTests(NormalSetTests):
 
     def testLen(self):

CHANGES.rst

@@ -4,7 +4,9 @@
 4.1.3 (unreleased)
 ------------------
 
-- TBD
+- Fix _p_changed when removing items from small pure-Python
+  BTrees/TreeSets and when adding to Sets. See:
+  https://github.com/zopefoundation/BTrees/issues/13
 
 
 4.1.2 (2015-04-07)
@@ -13,8 +15,9 @@
 - Suppress testing 64-bit values in OLBTrees on 32 bit machines.
   See:  https://github.com/zopefoundation/BTrees/issues/9
 
-- Fix _p_changed for small pure-Python BTrees.
-  See   https://github.com/zopefoundation/BTrees/issues/11
+- Fix _p_changed when adding items to small pure-Python
+  BTrees/TreeSets. See:
+  https://github.com/zopefoundation/BTrees/issues/11
 
 
 4.1.1 (2014-12-27)