From 4c6092203e38e804f84b63f60afe03901b5e9b46 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Sat, 21 Jul 2018 09:48:33 -0500 Subject: [PATCH 1/3] A failing test case. --- setup.py | 1 + .../externalization/tests/test_persistence.py | 99 +++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/setup.py b/setup.py index 8a43a88..776e588 100755 --- a/setup.py +++ b/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', diff --git a/src/nti/externalization/tests/test_persistence.py b/src/nti/externalization/tests/test_persistence.py index cb88dae..9fc50f4 100644 --- a/src/nti/externalization/tests/test_persistence.py +++ b/src/nti/externalization/tests/test_persistence.py @@ -17,6 +17,7 @@ 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 @@ -24,6 +25,7 @@ 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 +188,100 @@ def toExternalOID(self, **kwargs): wref = PWeakRef(P()) assert_that(wref.toExternalOID(), is_(b'abc')) + +@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: + # 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): + self._all_persists_fail(GlobalNoPicklePersistentMixin2) + + def test_persistent_mixin3(self): + self._all_persists_fail(GlobalNoPicklePersistentMixin3) + + def test_persistent_emits_warning(self): + import warnings + with warnings.catch_warnings(record=True) as w: + class P(Persistent): + pass + NoPickle(P) + + assert_that(w, has_length(1)) + # XXX Fill in details From a267e2721d8e49c76131a56d18feae04da830bca Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Sat, 21 Jul 2018 10:07:01 -0500 Subject: [PATCH 2/3] @NoPickle catches Persistent subclasses too. --- CHANGES.rst | 7 ++++ src/nti/externalization/persistence.py | 32 +++++++++++++++---- .../externalization/tests/test_persistence.py | 25 +++++++++++---- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 52a16bc..ee6125c 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -6,6 +6,13 @@ 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. + - 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 diff --git a/src/nti/externalization/persistence.py b/src/nti/externalization/persistence.py index 94a0371..4a67d3f 100644 --- a/src/nti/externalization/persistence.py +++ b/src/nti/externalization/persistence.py @@ -251,10 +251,25 @@ 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 @@ -262,9 +277,14 @@ def NoPickle(cls): def __reduce_ex__(self, protocol=0): raise TypeError(msg) - __reduce__ = __reduce_ex__ - cls.__reduce__ = __reduce__ cls.__reduce_ex__ = __reduce_ex__ + cls.__reduce__ = __reduce_ex__ + cls.__getstate__ = __reduce_ex__ + + if issubclass(cls, persistent.Persistent): + import warnings + warnings.warn(RuntimeWarning("Using @NoPickle an a Persistent subclass"), + stacklevel=2) return cls diff --git a/src/nti/externalization/tests/test_persistence.py b/src/nti/externalization/tests/test_persistence.py index 9fc50f4..4d66ecd 100644 --- a/src/nti/externalization/tests/test_persistence.py +++ b/src/nti/externalization/tests/test_persistence.py @@ -7,6 +7,7 @@ # stdlib imports import unittest +import warnings import persistent from persistent import Persistent @@ -189,9 +190,12 @@ def toExternalOID(self, **kwargs): assert_that(wref.toExternalOID(), is_(b'abc')) -@NoPickle -class GlobalPersistentNoPickle(Persistent): - pass +with warnings.catch_warnings(): + warnings.simplefilter('ignore') + + @NoPickle + class GlobalPersistentNoPickle(Persistent): + pass class GlobalSubclassPersistentNoPickle(GlobalPersistentNoPickle): pass @@ -240,7 +244,7 @@ def _persist_pickle(self, obj): def _persist_cpickle(self, obj): try: import cPickle - except ImportError: + except ImportError: # pragma: no cover # Python 3 raise TypeError("Not allowed to pickle") else: @@ -271,17 +275,24 @@ def test_persistent_mixin1(self): self._all_persists_fail(GlobalNoPicklePersistentMixin1) def test_persistent_mixin2(self): - self._all_persists_fail(GlobalNoPicklePersistentMixin2) + # 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 test_persistent_emits_warning(self): - import warnings with warnings.catch_warnings(record=True) as w: class P(Persistent): pass NoPickle(P) assert_that(w, has_length(1)) - # XXX Fill in details + assert_that(w[0].message, is_(RuntimeWarning)) + self.assertIn("Using @NoPickle", + str(w[0].message)) From e11ad8a39837e4ceb282dbb06cdaf16d2271f853 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Sat, 21 Jul 2018 10:41:35 -0500 Subject: [PATCH 3/3] Also emit a warning if the class decorated by @NoPickle directly implements one of the methods we override. --- CHANGES.rst | 4 ++- src/nti/externalization/persistence.py | 13 +++++--- .../externalization/tests/test_persistence.py | 32 ++++++++++++++++--- 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index ee6125c..27ed33e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,7 +11,9 @@ 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. + 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 diff --git a/src/nti/externalization/persistence.py b/src/nti/externalization/persistence.py index 4a67d3f..8056813 100644 --- a/src/nti/externalization/persistence.py +++ b/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 @@ -277,14 +278,16 @@ class CanPickle(Persistent, Root): def __reduce_ex__(self, protocol=0): raise TypeError(msg) - - cls.__reduce_ex__ = __reduce_ex__ - cls.__reduce__ = __reduce_ex__ - cls.__getstate__ = __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): - import warnings warnings.warn(RuntimeWarning("Using @NoPickle an a Persistent subclass"), stacklevel=2) + + return cls diff --git a/src/nti/externalization/tests/test_persistence.py b/src/nti/externalization/tests/test_persistence.py index 4d66ecd..5048743 100644 --- a/src/nti/externalization/tests/test_persistence.py +++ b/src/nti/externalization/tests/test_persistence.py @@ -286,13 +286,37 @@ def test_persistent_mixin2(self): def test_persistent_mixin3(self): self._all_persists_fail(GlobalNoPicklePersistentMixin3) - def test_persistent_emits_warning(self): + def _check_emits_warning(self, kind): with warnings.catch_warnings(record=True) as w: - class P(Persistent): - pass - NoPickle(P) + 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)