Merge e11ad8a into 5fa344c

CHANGES.rst

@@ -6,6 +6,15 @@
 1.0.0a3 (unreleased)
 ====================
 
+- The ``@NoPickle`` decorator also works with ``Persistent``
+  subclasses (and may or may not work with multiple-inheritance
+  subclasses of ``Persistent``, depending on the MRO,
+  but that's always been the case for regular objects). A
+  ``Persistent`` subclass being decorated with ``@NoPickle`` doesn't
+  make much sense, so a ``RuntimeWarning`` is issued. A warning is
+  also issued if the class directly implements one of the pickle
+  protocol methods.
+
 - Updating objects that use ``createFieldProperties`` or otherwise
   have ``FieldProperty`` objects in their type is at least 10% faster
   thanks to avoiding double-validation due to a small monkey-patch on

setup.py

@@ -161,6 +161,7 @@ def _c(m):
         'PyYAML',
         'pytz',
         'simplejson',
+        'transaction >= 2.2',
         'six >= 1.11.0', # for the reference cycle fix in reraise()
         'ZODB',
         'zope.component',

src/nti/externalization/persistence.py

@@ -18,6 +18,7 @@
     from itertools import izip
 except ImportError: # pragma: no cover
     izip = zip
+import warnings
 
 import persistent
 from persistent.list import PersistentList
@@ -251,20 +252,42 @@ def NoPickle(cls):
     objects do not get pickled and thus avoiding
     ZODB backward compatibility concerns.
 
-    .. warning:: If you subclass something that used this
-            decorator, you should override ``__reduce_ex__``
-            (or both it and ``__reduce__``).
-
+    .. warning::
+        Subclasses of such decorated classes are
+        also not capable of being pickled, without
+        appropriate overrides of ``__reduce_ex__`` and
+        ``__getstate__``. A "working" subclass, but only
+        for ZODB, looks like this:
+
+            @NoPickle
+            class Root(object):
+                pass
+
+            class CanPickle(Persistent, Root):
+                pass
+
+    .. warning::
+       This decorator emits a warning when it is used
+       directly on a ``Persistent`` subclass, as that rather
+       defeats the point (but it is still allowed for
+       backwards compatibility).
     """
 
     msg = "Not allowed to pickle %s" % cls
 
     def __reduce_ex__(self, protocol=0):
         raise TypeError(msg)
 
-    __reduce__ = __reduce_ex__
+    for meth in '__reduce_ex__', '__reduce__', '__getstate__':
+        if vars(cls).get(meth) is not None:
+            warnings.warn(RuntimeWarning("Using @NoPickle an a class that implements " + meth),
+                          stacklevel=2)
+        setattr(cls, meth, __reduce_ex__)
+
+    if issubclass(cls, persistent.Persistent):
+        warnings.warn(RuntimeWarning("Using @NoPickle an a Persistent subclass"),
+                      stacklevel=2)
+
 
-    cls.__reduce__ = __reduce__
-    cls.__reduce_ex__ = __reduce_ex__
 
     return cls

src/nti/externalization/tests/test_persistence.py

@@ -7,6 +7,7 @@
 
 # stdlib imports
 import unittest
+import warnings
 
 import persistent
 from persistent import Persistent
@@ -17,13 +18,15 @@
 from nti.externalization.persistence import PersistentExternalizableWeakList
 from nti.externalization.persistence import getPersistentState
 from nti.externalization.persistence import setPersistentStateChanged
+from nti.externalization.persistence import NoPickle
 from nti.externalization.tests import ExternalizationLayerTest
 
 from hamcrest import assert_that
 from hamcrest import calling
 from hamcrest import is_
 from hamcrest import is_not
 from hamcrest import raises
+from hamcrest import has_length
 
 # disable: accessing protected members, too many methods
 # pylint: disable=W0212,R0904
@@ -186,3 +189,134 @@ def toExternalOID(self, **kwargs):
         wref = PWeakRef(P())
 
         assert_that(wref.toExternalOID(), is_(b'abc'))
+
+with warnings.catch_warnings():
+    warnings.simplefilter('ignore')
+
+    @NoPickle
+    class GlobalPersistentNoPickle(Persistent):
+        pass
+
+class GlobalSubclassPersistentNoPickle(GlobalPersistentNoPickle):
+    pass
+
+@NoPickle
+class GlobalNoPickle(object):
+    pass
+
+class GlobalSubclassNoPickle(GlobalNoPickle):
+    pass
+
+class GlobalNoPicklePersistentMixin1(GlobalNoPickle,
+                                     Persistent):
+    pass
+
+class GlobalNoPicklePersistentMixin2(Persistent,
+                                     GlobalNoPickle):
+    pass
+
+class GlobalNoPicklePersistentMixin3(GlobalSubclassNoPickle,
+                                     Persistent):
+    pass
+
+class TestNoPickle(unittest.TestCase):
+
+    def _persist_zodb(self, obj):
+        from ZODB import DB
+        from ZODB.MappingStorage import MappingStorage
+        import transaction
+
+        db = DB(MappingStorage())
+        conn = db.open()
+        try:
+            conn.root.key = obj
+
+            transaction.commit()
+        finally:
+            conn.close()
+            db.close()
+            transaction.abort()
+
+    def _persist_pickle(self, obj):
+        import pickle
+        pickle.dumps(obj)
+
+    def _persist_cpickle(self, obj):
+        try:
+            import cPickle
+        except ImportError: # pragma: no cover
+            # Python 3
+            raise TypeError("Not allowed to pickle")
+        else:
+            cPickle.dumps(obj)
+
+    def _all_persists_fail(self, factory):
+
+        for meth in (self._persist_zodb,
+                     self._persist_pickle,
+                     self._persist_cpickle):
+            __traceback_info__ = meth
+            assert_that(calling(meth).with_args(factory()),
+                        raises(TypeError, "Not allowed to pickle"))
+
+    def test_plain_object(self):
+        self._all_persists_fail(GlobalNoPickle)
+
+    def test_subclass_plain_object(self):
+        self._all_persists_fail(GlobalSubclassNoPickle)
+
+    def test_persistent(self):
+        self._all_persists_fail(GlobalPersistentNoPickle)
+
+    def test_subclass_persistent(self):
+        self._all_persists_fail(GlobalSubclassPersistentNoPickle)
+
+    def test_persistent_mixin1(self):
+        self._all_persists_fail(GlobalNoPicklePersistentMixin1)
+
+    def test_persistent_mixin2(self):
+        # Putting Persistent first works for zodb.
+        factory = GlobalNoPicklePersistentMixin2
+        self._persist_zodb(factory())
+        # But plain pickle still fails
+        with self.assertRaises(TypeError):
+            self._persist_pickle(factory())
+
+
+    def test_persistent_mixin3(self):
+        self._all_persists_fail(GlobalNoPicklePersistentMixin3)
+
+    def _check_emits_warning(self, kind):
+        with warnings.catch_warnings(record=True) as w:
+            NoPickle(kind)
+
+        assert_that(w, has_length(1))
+        assert_that(w[0].message, is_(RuntimeWarning))
+        self.assertIn("Using @NoPickle",
+                      str(w[0].message))
+
+    def test_persistent_emits_warning(self):
+        class P(Persistent):
+            pass
+        self._check_emits_warning(P)
+
+    def test_getstate_emits_warning(self):
+        class P(object):
+            def __getstate__(self):
+                "Does nothing"
+
+        self._check_emits_warning(P)
+
+    def test_reduce_emits_warning(self):
+        class P(object):
+            def __reduce__(self):
+                "Does nothing"
+
+        self._check_emits_warning(P)
+
+    def test_reduce_ex_emits_warning(self):
+        class P(object):
+            def __reduce_ex__(self):
+                "Does nothing"
+
+        self._check_emits_warning(P)