Fix methods get, pop, setdefault and __contains__ to not hide POSKeyError

CHANGES.rst

@@ -19,8 +19,9 @@
 - Fix ``Tree.__setstate__`` to no longer accept children besides
   tree or bucket types to prevent crashes. See `PR 143
   <https://github.com/zopefoundation/BTrees/pull/143>`_ for details.
-- BTrees, TreeSet, Set and Buckets implements the ``__and__``,
-  ``__or__`` and ``__sub__`` as shortcuts for
+
+- Make BTrees, TreeSet, Set and Buckets implements the ``__and__``,
+  ``__or__`` and ``__sub__`` special methods as shortcuts for
   ``BTrees.Interfaces.IMerge.intersection``,
   ``BTrees.Interfaces.IMerge.union`` and
   ``BTrees.Interfaces.IMerge.difference``.
@@ -59,6 +60,12 @@
   <https://github.com/zopefoundation/BTrees/pull/159>`_ for all the
   interface updates.
 
+- Fix the ``get``, ``setdefault`` and ``pop`` methods, as well as the
+  ``in`` operator, to not suppress ``POSKeyError`` if the object or
+  subobjects are corrupted. Previously, such errors were logged by
+  ZODB, but not propagated. See `issue 161
+  <https://github.com/zopefoundation/BTrees/issues/161>`_.
+
 4.7.2 (2020-04-07)
 ==================
 

src/BTrees/BTreeModuleTemplate.c

@@ -508,6 +508,23 @@ static char *search_keywords[] = {"min", "max",
                                   "excludemin", "excludemax",
                                   0};
 
+/**
+ * Call this instead of using `PyErr_ExceptionMatches(PyExc_KeyError)`
+ * when you intend to suppress the KeyError.
+ *
+ * We want to match only exactly ``PyExc_KeyError``, and not subclasses
+ * such as ``ZODB.POSException.POSKeyError``.
+ */
+static int
+BTree_ShouldSuppressKeyError()
+{
+    PyObject* exc_type = PyErr_Occurred(); /* Returns borrowed reference. */
+    if (exc_type && exc_type == PyExc_KeyError) {
+        return 1;
+    }
+    return 0;
+}
+
 #include "BTreeItemsTemplate.c"
 #include "BucketTemplate.c"
 #include "SetTemplate.c"

src/BTrees/BTreeTemplate.c

@@ -1205,8 +1205,7 @@ _BTree_setstate(BTree *self, PyObject *state, int noval)
     }
 
     len = PyTuple_Size(items);
-    if (len < 0)
-        return -1;
+    ASSERT(len >= 0, "_BTree_setstate: items tuple has negative size", -1);
     len = (len + 1) / 2;
 
     assert(len > 0); /* If the BTree is empty, it's state is None. */
@@ -1975,7 +1974,7 @@ BTree_getm(BTree *self, PyObject *args)
         return NULL;
     if ((r=_BTree_get(self, key, 0, _BGET_REPLACE_TYPE_ERROR)))
         return r;
-    UNLESS (PyErr_ExceptionMatches(PyExc_KeyError))
+    UNLESS (BTree_ShouldSuppressKeyError())
         return NULL;
     PyErr_Clear();
     Py_INCREF(d);
@@ -1999,7 +1998,7 @@ BTree_setdefault(BTree *self, PyObject *args)
     /* The key isn't in the tree.  If that's not due to a KeyError exception,
     * pass back the unexpected exception.
     */
-    if (! PyErr_ExceptionMatches(PyExc_KeyError))
+    if (! BTree_ShouldSuppressKeyError())
         return NULL;
     PyErr_Clear();
 
@@ -2040,7 +2039,7 @@ BTree_pop(BTree *self, PyObject *args)
     /* The key isn't in the tree.  If that's not due to a KeyError exception,
     * pass back the unexpected exception.
     */
-    if (! PyErr_ExceptionMatches(PyExc_KeyError))
+    if (! BTree_ShouldSuppressKeyError())
         return NULL;
 
     if (failobj != NULL)
@@ -2078,7 +2077,7 @@ BTree_contains(BTree *self, PyObject *key)
         result = INT_AS_LONG(asobj) ? 1 : 0;
         Py_DECREF(asobj);
     }
-    else if (PyErr_ExceptionMatches(PyExc_KeyError))
+    else if (BTree_ShouldSuppressKeyError())
     {
         PyErr_Clear();
         result = 0;

src/BTrees/BucketTemplate.c

@@ -1300,8 +1300,7 @@ _bucket_setstate(Bucket *self, PyObject *state)
     }
 
     len = PyTuple_Size(items);
-    if (len < 0)
-        return -1;
+    ASSERT(len >= 0, "_bucket_setstate: items tuple has negative size", -1);
     len /= 2;
 
     for (i = self->len; --i >= 0; ) {
@@ -1407,7 +1406,7 @@ bucket_setdefault(Bucket *self, PyObject *args)
     /* The key isn't in the bucket.  If that's not due to a KeyError exception,
      * pass back the unexpected exception.
      */
-    if (! PyErr_ExceptionMatches(PyExc_KeyError))
+    if (! BTree_ShouldSuppressKeyError())
         return NULL;
     PyErr_Clear();
 
@@ -1448,7 +1447,7 @@ bucket_pop(Bucket *self, PyObject *args)
     /* The key isn't in the bucket.  If that's not due to a KeyError exception,
      * pass back the unexpected exception.
      */
-    if (! PyErr_ExceptionMatches(PyExc_KeyError))
+    if (! BTree_ShouldSuppressKeyError())
         return NULL;
 
     if (failobj != NULL) {
@@ -1484,7 +1483,7 @@ bucket_contains(Bucket *self, PyObject *key)
         result = INT_AS_LONG(asobj) ? 1 : 0;
         Py_DECREF(asobj);
     }
-    else if (PyErr_ExceptionMatches(PyExc_KeyError)) {
+    else if (BTree_ShouldSuppressKeyError()) {
         PyErr_Clear();
         result = 0;
     }
@@ -1523,7 +1522,7 @@ bucket_getm(Bucket *self, PyObject *args)
         PyErr_Clear();
         PyErr_SetObject(PyExc_KeyError, key);
     }
-    if (!PyErr_ExceptionMatches(PyExc_KeyError))
+    if (!BTree_ShouldSuppressKeyError())
         return NULL;
     PyErr_Clear();
     Py_INCREF(d);

src/BTrees/tests/common.py

@@ -1169,6 +1169,40 @@ def trial(i):
                 # of twos complement binary integers
                 self.assertEqual(9, len(b))
 
+    @_skip_wo_ZODB
+    def testAccessRaisesPOSKeyErrorOnSelf(self):
+        # We don't hide a POSKeyError that happens when
+        # accessing the object itself in `get()`.
+        # See https://github.com/zopefoundation/BTrees/issues/161
+        from ZODB.POSException import POSKeyError
+        import transaction
+        transaction.begin()
+        m = self._makeOne()
+        root = self._getRoot()
+        root.m = m
+        transaction.commit()
+        conn = root._p_jar
+        # Ghost the object
+        conn.cacheMinimize()
+        self.assertEqual(m._p_status, "ghost")
+        # Remove the backing data
+        self.db._storage._data.clear()
+
+        transaction.begin()
+        try:
+            with self.assertRaises(POSKeyError):
+                m.get(1)
+
+            with self.assertRaises(POSKeyError):
+                m.setdefault(1, 1)
+
+            with self.assertRaises(POSKeyError):
+                _ = 1 in m
+
+            with self.assertRaises(POSKeyError):
+                m.pop(1)
+        finally:
+            self._closeRoot(root)
 
 class BTreeTests(MappingBase):
     # Tests common to all BTrees
@@ -1191,6 +1225,45 @@ def _checkIt(self, t):
         t._check()
         check(t)
 
+    @_skip_wo_ZODB
+    def testAccessRaisesPOSKeyErrorOnNested(self):
+        # We don't hide a POSKeyError that happens when
+        # accessing sub objects in `get()`.
+        # See https://github.com/zopefoundation/BTrees/issues/161
+        from ZODB.POSException import POSKeyError
+        import transaction
+        transaction.begin()
+        m = self._makeOne()
+        root = self._getRoot()
+        root.m = m
+        self._populate(m, 1000)
+        transaction.commit()
+        conn = root._p_jar
+        # Ghost the objects...
+        conn.cacheMinimize()
+        # ...but activate the tree itself, leaving the
+        # buckets ghosted
+        m._p_activate()
+
+        # Remove the backing data
+        self.db._storage._data.clear()
+
+        transaction.begin()
+        try:
+            with self.assertRaises(POSKeyError):
+                m.get(1)
+
+            with self.assertRaises(POSKeyError):
+                m.setdefault(1, 1)
+
+            with self.assertRaises(POSKeyError):
+                _ = 1 in m
+
+            with self.assertRaises(POSKeyError):
+                m.pop(1)
+        finally:
+            self._closeRoot(root)
+
     def testDeleteNoChildrenWorks(self):
         t = self._makeOne()
         t[5] = 6