Raise more informative KeyError subclasses.

CHANGES.rst

@@ -4,6 +4,12 @@ Changes
 4.2.0 (unreleased)
 ------------------
 
+- Raise more informative KeyError subclasses from the utility when intids
+  or objects cannot be found. This distinguishes them from errors
+  raised by normal dictionaries or BTrees, and is useful in unit
+  testing or when persisting intids or sharing them among processes
+  for later or concurrent use.
+
 - Add support for Python 3.5.
 
 - Drop support for Python 2.6.

src/zope/intid/__init__.py

@@ -34,6 +34,9 @@
 
 from zope.intid.interfaces import IIntIds, IIntIdEvent
 from zope.intid.interfaces import IntIdAddedEvent, IntIdRemovedEvent
+from zope.intid.interfaces import IntIdMissingError, IntIdsCorruptedError, ObjectMissingError
+
+
 
 @implementer(IIntIds, IContained)
 class IntIds(Persistent):
@@ -67,7 +70,10 @@ def __iter__(self):
         return self.refs.iterkeys()
 
     def getObject(self, id):
-        return self.refs[id]()
+        try:
+            return self.refs[id]()
+        except KeyError:
+            raise ObjectMissingError(id)
 
     def queryObject(self, id, default=None):
         r = self.refs.get(id)
@@ -79,12 +85,17 @@ def getId(self, ob):
         try:
             key = IKeyReference(ob)
         except (NotYet, TypeError, ValueError):
-            raise KeyError(ob)
+            raise IntIdMissingError(ob)
 
         try:
             return self.ids[key]
         except KeyError:
-            raise KeyError(ob)
+            # In theory, if the ZODB and our self.ids BTree is
+            # corrupted, this could raise
+            # ZODB.POSException.POSKeyError, which we should probably
+            # let propagate. But since that's a KeyError, we've always
+            # caught it and transformed the error message and type.
+            raise IntIdMissingError(ob)
 
     def queryId(self, ob, default=None):
         try:
@@ -131,10 +142,26 @@ def unregister(self, ob):
         if key is None:
             return
 
-        uid = self.ids[key]
-        del self.refs[uid]
-        del self.ids[key]
+        # In theory, any of the KeyErrors we're catching here could be
+        # a ZODB POSKeyError if the ZODB and our BTrees are corrupt.
+        # We used to let those propagate (where they would probably be
+        # ignored, see removeIntIdSubscriber), but now we transform
+        # them and ignore that possibility because the chances of that
+        # happening are (probably) extremely remote.
 
+        try:
+            uid = self.ids[key]
+        except KeyError:
+            raise IntIdMissingError(ob)
+
+        try:
+            del self.refs[uid]
+        except KeyError:
+            # It was in self.ids, but not self.refs. Something is corrupted.
+            # We've always let this KeyError propagate, before cleaning up self.ids,
+            # meaning that getId(ob) will continue to work, but getObject(uid) will not.
+            raise IntIdsCorruptedError(ob, uid)
+        del self.ids[key]
 
 @adapter(ILocation, IObjectRemovedEvent)
 def removeIntIdSubscriber(ob, event):
@@ -154,6 +181,7 @@ def removeIntIdSubscriber(ob, event):
                 try:
                     utility.unregister(key)
                 except KeyError:
+                    # Silently ignoring corruption here
                     pass
 
 @adapter(ILocation, IObjectAddedEvent)

src/zope/intid/interfaces.py

@@ -2,6 +2,23 @@
 """
 from zope.interface import Interface, Attribute, implementer
 
+class IntIdMissingError(KeyError):
+    """
+    Raised when ``getId`` cannot find an intid.
+    """
+
+class ObjectMissingError(KeyError):
+    """
+    Raised when ``getObject`` cannot find an object.
+    """
+
+class IntIdsCorruptedError(KeyError):
+    """
+    Raised when internal corruption is detected in the utility.
+
+    Users should not need to catch this because this situation should
+    not happen.
+    """
 
 class IIntIdsQuery(Interface):
     "Query for IDs and objects"
@@ -43,7 +60,7 @@ def register(ob):
     def unregister(ob):
         """Remove the object from the indexes.
 
-        KeyError is raised if ob is not registered previously.
+        IntIdMissingError is raised if ob is not registered previously.
         """
 
 class IIntIdsManage(Interface):

src/zope/intid/tests.py

@@ -43,6 +43,7 @@
 
 from zope.intid import IntIds, intIdEventNotify
 from zope.intid.interfaces import IIntIds
+from zope.intid.interfaces import IntIdMissingError, IntIdsCorruptedError, ObjectMissingError
 
 
 # Local Utility Addition
@@ -125,35 +126,46 @@ def test_non_keyreferences(self):
 
         self.assertTrue(u.queryId(obj) is None)
         self.assertTrue(u.unregister(obj) is None)
-        self.assertRaises(KeyError, u.getId, obj)
+        self.assertRaises(IntIdMissingError, u.getId, obj)
 
     def test(self):
         u = self.createIntIds()
         obj = P()
 
         obj._p_jar = ConnectionStub()
 
-        self.assertRaises(KeyError, u.getId, obj)
-        self.assertRaises(KeyError, u.getId, P())
+        self.assertRaises(IntIdMissingError, u.getId, obj)
+        self.assertRaises(IntIdMissingError, u.getId, P())
 
-        self.assertTrue(u.queryId(obj) is None)
-        self.assertTrue(u.queryId(obj, 42) is 42)
-        self.assertTrue(u.queryId(P(), 42) is 42)
-        self.assertTrue(u.queryObject(42) is None)
-        self.assertTrue(u.queryObject(42, obj) is obj)
+        self.assertIsNone(u.queryId(obj))
+        self.assertIs(u.queryId(obj, self), self)
+        self.assertIs(u.queryId(P(), self), self)
+        self.assertIsNone(u.queryObject(42))
+        self.assertIs(u.queryObject(42, obj), obj)
 
         uid = u.register(obj)
-        self.assertTrue(u.getObject(uid) is obj)
-        self.assertTrue(u.queryObject(uid) is obj)
+        self.assertIs(u.getObject(uid), obj)
+        self.assertIs(u.queryObject(uid), obj)
         self.assertEqual(u.getId(obj), uid)
         self.assertEqual(u.queryId(obj), uid)
 
         uid2 = u.register(obj)
         self.assertEqual(uid, uid2)
 
         u.unregister(obj)
-        self.assertRaises(KeyError, u.getObject, uid)
-        self.assertRaises(KeyError, u.getId, obj)
+        self.assertRaises(ObjectMissingError, u.getObject, uid)
+        self.assertRaises(IntIdMissingError, u.getId, obj)
+
+        # Unregistering again fails
+        self.assertRaises(IntIdMissingError, u.unregister, obj)
+
+        # Let's manually generate corruption
+        uid = u.register(obj)
+        del u.refs[uid]
+
+        self.assertRaises(IntIdsCorruptedError, u.unregister, obj)
+        # But we can still ask for its id
+        self.assertEqual(u.getId(obj), uid)
 
     def test_btree_long(self):
         # This is a somewhat arkward test, that *simulates* the border case
@@ -286,8 +298,8 @@ def appendObjectEvent(obj, event):
         # nearest one.
         removeIntIdSubscriber(folder, ObjectRemovedEvent(parent_folder))
 
-        self.assertRaises(KeyError, self.utility.getObject, id)
-        self.assertRaises(KeyError, self.utility1.getObject, id1)
+        self.assertRaises(ObjectMissingError, self.utility.getObject, id)
+        self.assertRaises(ObjectMissingError, self.utility1.getObject, id1)
 
         self.assertEqual(len(events), 1)
         self.assertEqual(events[0].object, folder)