Merge pull request #105 from zopefoundation/issue102

Fully test the C implementation of the PickleCache

CHANGES.rst

 ``persistent`` Changelog
 ========================
 
-4.4.4 (unreleased)
+4.5.0 (unreleased)
 ------------------
 
-- Nothing changed yet.
+- Fully test the C implementation of the PickleCache, and fix
+  discrepancies between it and the Python implementation:
+
+  - The C implementation now raises ``ValueError`` instead of
+    ``AssertionError`` for certain types of bad inputs.
+  - The Python implementation uses the C wording for error messages.
+  - The C implementation properly implements ``IPickleCache``; methods
+    unique to the Python implementation were moved to
+    ``IExtendedPickleCache``.
+  - The Python implementation raises ``AttributeError`` if a
+    persistent class doesn't have a ``p_jar`` attribute.
 
 
 4.4.3 (2018-10-22)

persistent/__init__.py

@@ -30,6 +30,7 @@ __all__ = [
 ]
 from persistent._compat import PURE_PYTHON
 from persistent.interfaces import IPersistent
+from persistent.interfaces import IPickleCache
 
 import persistent.timestamp as TimeStamp
 
@@ -48,6 +49,7 @@ else:
     # Note that the Python version already does this.
     from zope.interface import classImplements
     classImplements(_cPersistence.Persistent, IPersistent)
+    classImplements(_cPickleCache.PickleCache, IPickleCache)
 
 
 _persistence = pyPersistence if PURE_PYTHON or _cPersistence is None else _cPersistence

persistent/cPickleCache.c

@@ -727,7 +727,7 @@ cc_new_ghost(ccobject *self, PyObject *args)
     Py_DECREF(tmp);
     if (tmp != Py_None)
     {
-        PyErr_SetString(PyExc_AssertionError,
+        PyErr_SetString(PyExc_ValueError,
                         "New ghost object must not have an oid");
         return NULL;
     }
@@ -739,7 +739,7 @@ cc_new_ghost(ccobject *self, PyObject *args)
     Py_DECREF(tmp);
     if (tmp != Py_None)
     {
-        PyErr_SetString(PyExc_AssertionError,
+        PyErr_SetString(PyExc_ValueError,
                         "New ghost object must not have a jar");
         return NULL;
     }
@@ -748,7 +748,7 @@ cc_new_ghost(ccobject *self, PyObject *args)
     if (tmp)
     {
         Py_DECREF(tmp);
-        PyErr_SetString(PyExc_AssertionError,
+        PyErr_SetString(PyExc_ValueError,
                         "The given oid is already in the cache");
         return NULL;
     }

persistent/interfaces.py

@@ -439,12 +439,6 @@ class IPickleCache(Interface):
         o Return 'default' if not found.
         """
 
-    def mru(oid):
-        """ Move the element corresonding to 'oid' to the head.
-
-        o Raise KeyError if no element is found.
-        """
-
     def __len__():
         """ -> the number of OIDs in the cache.
         """
@@ -516,23 +510,6 @@ class IPickleCache(Interface):
         If 'oid' is already in the cache, raise.
         """
 
-    def reify(to_reify):
-        """ Reify the indicated objects.
-
-        o If 'to_reify' is a string, treat it as an OID.
-
-        o Otherwise, iterate over it as a sequence of OIDs.
-
-        o For each OID, if present in 'data' and in GHOST state:
-
-            o Call '_p_activate' on the object.
-
-            o Add it to the ring.
-
-        o If any OID is present but not in GHOST state, skip it.
-
-        o Raise KeyErrory if any OID is not present.
-        """
 
     def invalidate(to_invalidate):
         """ Invalidate the indicated objects.
@@ -566,3 +543,33 @@ class IPickleCache(Interface):
                                         'ringlen?')
     cache_data = Attribute("Property:  copy of our 'data' dict")
     cache_klass_count = Attribute("Property: len of 'persistent_classes'")
+
+
+class IExtendedPickleCache(IPickleCache):
+    """
+    Extra operations for a pickle cache.
+    """
+
+    def reify(to_reify):
+        """ Reify the indicated objects.
+
+        o If 'to_reify' is a string, treat it as an OID.
+
+        o Otherwise, iterate over it as a sequence of OIDs.
+
+        o For each OID, if present in 'data' and in GHOST state:
+
+            o Call '_p_activate' on the object.
+
+            o Add it to the ring.
+
+        o If any OID is present but not in GHOST state, skip it.
+
+        o Raise KeyErrory if any OID is not present.
+        """
+
+    def mru(oid):
+        """ Move the element corresonding to 'oid' to the head.
+
+        o Raise KeyError if no element is found.
+        """

persistent/picklecache.py

@@ -18,7 +18,7 @@ import weakref
 from zope.interface import implementer
 
 from persistent.interfaces import GHOST
-from persistent.interfaces import IPickleCache
+from persistent.interfaces import IExtendedPickleCache
 from persistent.interfaces import OID_TYPE
 from persistent.interfaces import UPTODATE
 from persistent.persistence import Persistent
@@ -53,7 +53,7 @@ def _sweeping_ring(f):
 
 from .ring import Ring
 
-@implementer(IPickleCache)
+@implementer(IExtendedPickleCache)
 class PickleCache(object):
 
     total_estimated_size = 0
@@ -126,16 +126,17 @@ class PickleCache(object):
                 # Raise the same type of exception as the C impl with the same
                 # message.
                 raise ValueError('A different object already has the same oid')
-        # Match the C impl: it requires a jar
-        jar = getattr(value, '_p_jar', None)
-        if jar is None and not isinstance(value, type):
+        # Match the C impl: it requires a jar. Let this raise AttributeError
+        # if no jar is found.
+        jar = getattr(value, '_p_jar')
+        if jar is None:
             raise ValueError("Cached object jar missing")
         # It also requires that it cannot be cached more than one place
         existing_cache = getattr(jar, '_cache', None)
         if (existing_cache is not None
             and existing_cache is not self
             and existing_cache.data.get(oid) is not None):
-            raise ValueError("Object already in another cache")
+            raise ValueError("Cache values may only be in one cache.")
 
         if isinstance(value, type): # ZODB.persistentclass.PersistentMetaClass
             self.persistent_classes[oid] = value
@@ -301,7 +302,10 @@ class PickleCache(object):
 
             self.total_estimated_size += new_est_size_in_bytes
 
-    cache_drain_resistance = property(lambda self: self.drain_resistance)
+    cache_drain_resistance = property(
+        lambda self: self.drain_resistance,
+        lambda self, nv: setattr(self, 'drain_resistance', nv)
+    )
     cache_non_ghost_count = property(lambda self: self.non_ghost_count)
     cache_data = property(lambda self: dict(self.data.items()))
     cache_klass_count = property(lambda self: len(self.persistent_classes))