Merge 73526de into 49cc7c0

CHANGES.rst

@@ -6,7 +6,22 @@
 1.0.0a3 (unreleased)
 ====================
 
-- Nothing changed yet.
+- Proxies around objects that implement ``toExternalObject`` are
+  allowed again; the proxied object's ``toExternalObject`` will be called.
+
+- The signature for ``updateFromExternalObject()`` has been tightened.
+  It should be ``(self, external_object, context, **kwargs)``, where
+  ``**kwargs`` is optional, as is context. ``**kwargs`` currently
+  contains nothing useful. Uses of ``dataserver=None`` in the
+  signature will generate a warning. This may be tightened further in
+  the future. See `issue 30
+  <https://github.com/NextThought/nti.externalization/issues/30>`_.
+
+- ``__ext_ignore_updateFromExternalObject__`` is officially
+  deprecated and generates a warning.
+
+- ``update_from_external_object`` caches certain information about the
+  types of the updater objects, making it 8-25% faster.
 
 
 1.0.0a2 (2018-07-05)

src/nti/externalization/datastructures.py

@@ -432,8 +432,8 @@ def _ext_accept_external_id(self, ext_self, parsed):
                 cache.ext_accept_external_id = False
         return cache.ext_accept_external_id
 
-    def updateFromExternalObject(self, parsed, *args, **kwargs):
-        result = super(InterfaceObjectIO, self).updateFromExternalObject(parsed, *args, **kwargs)
+    def updateFromExternalObject(self, parsed, *unused_args, **unused_kwargs):
+        result = AbstractDynamicObjectIO.updateFromExternalObject(self, parsed)
         # If we make it this far, then validate the object.
 
         # TODO: Should probably just make sure that there are no /new/

src/nti/externalization/externalization/_externalizer.pxd

@@ -91,7 +91,7 @@ cdef _usable_externalObject_cache_get
 @cython.locals(
     has_ext_obj=bint,
 )
-cdef _obj_has_usable_externalObject(obj)
+cpdef _obj_has_usable_externalObject(obj)
 
 cdef _externalize_object(obj, _ExternalizationState state)
 

src/nti/externalization/externalization/externalizer.py

@@ -184,9 +184,11 @@ def _externalize_sequence(obj, state):
 
 
 def _obj_has_usable_externalObject(obj):
-    # This is for legacy code support, to allow existing methods to move to adapters
-    # and call us without infinite recursion
-    kind = type(obj)
+    # This is for legacy code support, to allow existing methods to
+    # move to adapters and call us without infinite recursion.
+    # We use __class__ instead of type() to allow for proxies;
+    # The proxy itself cannot implement toExternalObject
+    kind = obj.__class__
     answer = _usable_externalObject_cache_get(kind)
     if answer is None:
         answer = False

src/nti/externalization/externalization/tests/test_externalizer.py

@@ -0,0 +1,49 @@
+# -*- coding: utf-8 -*-
+
+from __future__ import absolute_import
+from __future__ import division
+from __future__ import print_function
+
+# disable: accessing protected members, too many methods
+# pylint: disable=W0212,R0904
+
+import unittest
+
+from zope.proxy import ProxyBase
+
+from nti.externalization.externalization import to_external_object
+from ..externalizer import _obj_has_usable_externalObject
+
+
+class WithToExternalObject(object):
+
+    def toExternalObject(self, **kwargs):
+        return {'a': 42}
+
+
+
+class TestFunctions(unittest.TestCase):
+
+    def test_non_proxied(self):
+        self.assertTrue(_obj_has_usable_externalObject(WithToExternalObject()))
+
+    def test_has_usable_external_object_proxied(self):
+
+        obj = WithToExternalObject()
+        proxied = ProxyBase(obj)
+
+        self.assertTrue(_obj_has_usable_externalObject(proxied))
+        self.assertEqual({'a': 42}, to_external_object(proxied))
+
+    def test_proxy_has_usable_external_object_not_allowed(self):
+
+        class Proxy(ProxyBase):
+
+            def toExternalObject(self, **kwargs):
+                raise NotImplementedError
+
+        self.assertFalse(_obj_has_usable_externalObject(Proxy(object())))
+
+
+if __name__ == '__main__':
+    unittest.main()

src/nti/externalization/interfaces.py

@@ -288,15 +288,19 @@ class IInternalObjectUpdater(interface.Interface):
 		It should return the new value. Note that the function here is at most
 		a class or static method, not an instance method.""")
 
-    def updateFromExternalObject(externalObject, *args, **kwargs):
+    def updateFromExternalObject(externalObject, context, **kwargs):
         """
         Update the object this is adapting from the external object.
-        Two alternate signatures are supported, one with ``dataserver`` instead of
-        context, and one with no keyword args.
+
+        Alternately, the signature can be ``updateFromExternalObject(externalObject)``
+        or simply ``updateFromExternalObject(externalObject, **kwargs)``. In this
+        last case, ``context`` will be passed as a value in ``**kwargs``.
+
 
         :return: If not ``None``, a value that can be interpreted as a boolean,
                 indicating whether or not the internal object actually
-                underwent updates. If ``None``, no meaning is assigned (to allow older
+                underwent updates. If ``None``, the caller should assume that the object
+                was updated (to allow older
                 code that doesn't return at all.)
         """
 

src/nti/externalization/internalization/_updater.pxd

@@ -31,9 +31,9 @@ cdef dict _EMPTY_DICT
 cdef class _RecallArgs(object):
     cdef registry
     cdef context
-    cdef require_updater
-    cdef notify
     cdef pre_hook
+    cdef bint require_updater
+    cdef bint notify
 
 
 cdef _recall(k, obj, ext_obj, _RecallArgs kwargs)
@@ -42,6 +42,15 @@ cpdef update_from_external_object(containedObject,
                                   externalObject,
                                   registry=*,
                                   context=*,
-                                  require_updater=*,
-                                  notify=*,
+                                  bint require_updater=*,
+                                  bint notify=*,
                                   pre_hook=*)
+
+cdef dict _argspec_cacheg
+cdef str _UPDATE_ARGS_TWO
+cdef str _UPDATE_ARGS_ONE
+cdef str _UPDATE_ARGS_CONTEXT_KW
+cdef _get_update_signature(updater)
+
+cdef dict _upsable_updateFromExternalObject_cache
+cdef _obj_has_usable_updateFromExternalObject(obj)

src/nti/externalization/internalization/updater.py

@@ -41,16 +41,16 @@ class _RecallArgs(object):
         'pre_hook',
     )
 
-    def __init__(self, registry,
-                 context,
-                 require_updater,
-                 notify,
-                 pre_hook):
-        self.registry = registry
-        self.context = context
-        self.require_updater = require_updater
-        self.notify = notify
-        self.pre_hook = pre_hook
+    # We don't have an __init__, we ask the caller
+    # to fill us in. In cython, this avoids some
+    # unneeded bint->object->bint conversions.
+
+    def __init__(self):
+        self.registry = None
+        self.context = None
+        self.require_updater = False
+        self.notify = True
+        self.pre_hook = None
 
 
 def _recall(k, obj, ext_obj, kwargs):
@@ -63,9 +63,105 @@ def _recall(k, obj, ext_obj, kwargs):
                                       notify=kwargs.notify,
                                       pre_hook=kwargs.pre_hook)
     if IPersistent_providedBy(obj): # pragma: no cover
-        obj._v_updated_from_external_source = ext_obj
+        obj._v_updated_from_external_source = ext_obj # pylint:disable=protected-access
     return obj
 
+##
+# Note on caching: We do not expect the updater objects to be proxied.
+# So we directly use type() instead of .__class__, which is faster.
+# We also do not expect them to be unloaded/updated/unbounded,
+# so we use a regular dict to cache info about them, which is faster
+# than a WeakKeyDictionary. For the same reason, we use dynamic warning
+# strings.
+
+# Support for varying signatures of the updater. This is slow and
+# cumbersome and needs to go; we are in the deprecation period now.
+# See https://github.com/NextThought/nti.externalization/issues/30
+
+_argspec_cache = {}
+
+# update(ext, context) or update(ext, context=None) or update(ext, dataserver)
+# exactly two arguments. It doesn't matter what the name is, we'll call it
+# positional.
+_UPDATE_ARGS_TWO = "update args two"
+_UPDATE_ARGS_CONTEXT_KW = "update args **kwargs"
+_UPDATE_ARGS_ONE = "update args external only"
+
+
+def _get_update_signature(updater):
+    kind = type(updater)
+
+    spec = _argspec_cache.get(kind)
+    if spec is None:
+        try:
+            argspec = inspect.getargspec(updater.updateFromExternalObject)
+        except TypeError: # pragma: no cover (This is hard to catch in pure-python coverage mode)
+            # Cython functions and other extension types are "not a Python function"
+            # and don't work with this. We assume they use the standard form accepting
+            # 'context' as kwarg
+            spec = _UPDATE_ARGS_CONTEXT_KW
+        else:
+            # argspec.args contains the names of all the parameters.
+            # argspec.keywords, if not none, is the name of the **kwarg
+            # These all must be methods (or at least classmethods), having
+            # an extra 'self' argument.
+            if not argspec.keywords:
+                # No **kwarg, good!
+                if len(argspec.args) == 3:
+                    # update(ext, context) or update(ext, context=None) or update(ext, dataserver)
+                    spec = _UPDATE_ARGS_TWO
+                else:
+                    # update(ext)
+                    spec = _UPDATE_ARGS_ONE
+            else:
+                if len(argspec.args) == 3:
+                    # update(ext, context, **kwargs) or update(ext, dataserver, **kwargs)
+                    spec = _UPDATE_ARGS_TWO
+                elif argspec.keywords.startswith("unused") or argspec.keywords.startswith('_'):
+                    spec = _UPDATE_ARGS_ONE
+                else:
+                    spec = _UPDATE_ARGS_CONTEXT_KW
+
+            if 'dataserver' in argspec.args and argspec.defaults and len(argspec.defaults) >= 1:
+                warnings.warn("The type %r still uses updateFromExternalObject(dataserver=None). "
+                              "Please change to context=None." % (kind,),
+                              FutureWarning)
+
+        _argspec_cache[kind] = spec
+
+    return spec
+
+_usable_updateFromExternalObject_cache = {}
+
+def _obj_has_usable_updateFromExternalObject(obj):
+    kind = type(obj)
+
+    usable_from = _usable_updateFromExternalObject_cache.get(kind)
+    if usable_from is None:
+        has_update = hasattr(obj, 'updateFromExternalObject')
+        if not has_update:
+            usable_from = False
+        else:
+            wants_ignore = getattr(obj, '__ext_ignore_updateFromExternalObject__', False)
+            usable_from = not wants_ignore
+            if wants_ignore:
+                warnings.warn("The type %r has __ext_ignore_updateFromExternalObject__=True. "
+                              "Please remove updateFromExternalObject from the type." % (kind,),
+                              FutureWarning)
+
+
+        _usable_updateFromExternalObject_cache[kind] = usable_from
+
+    return usable_from
+
+try:
+    from zope.testing import cleanup # pylint:disable=ungrouped-imports
+except ImportError: # pragma: no cover
+    raise
+else:
+    cleanup.addCleanUp(_argspec_cache.clear)
+    cleanup.addCleanUp(_usable_updateFromExternalObject_cache.clear)
+
 
 def update_from_external_object(containedObject, externalObject,
                                 registry=component, context=None,
@@ -102,7 +198,7 @@ def update_from_external_object(containedObject, externalObject,
         Signature ``f(k,x)`` where ``k`` is either the key name, or
         None in the case of a sequence and ``x`` is the external
         object. Deprecated.
-    :return: `containedObject` after updates from `externalObject`
+    :return: *containedObject* after updates from *externalObject*
 
     .. versionchanged:: 1.0.0a2
        Remove the ``object_hook`` parameter.
@@ -112,13 +208,12 @@ def update_from_external_object(containedObject, externalObject,
         for i in range(3):
             warnings.warn('pre_hook is deprecated', FutureWarning, stacklevel=i)
 
-    kwargs = _RecallArgs(
-        registry,
-        context,
-        require_updater,
-        notify,
-        pre_hook
-    )
+    kwargs = _RecallArgs()
+    kwargs.registry = registry
+    kwargs.context = context
+    kwargs.require_updater = require_updater
+    kwargs.notify = notify
+    kwargs.pre_hook = pre_hook
 
 
     # Parse any contained objects
@@ -168,8 +263,7 @@ def update_from_external_object(containedObject, externalObject,
                 externalObject[k] = _recall(k, factory(), v, kwargs)
 
     updater = None
-    if hasattr(containedObject, 'updateFromExternalObject') \
-        and not getattr(containedObject, '__ext_ignore_updateFromExternalObject__', False):
+    if _obj_has_usable_updateFromExternalObject(containedObject):
         # legacy support. The __ext_ignore_updateFromExternalObject__
         # allows a transition to an adapter without changing
         # existing callers and without triggering infinite recursion
@@ -189,24 +283,14 @@ def update_from_external_object(containedObject, externalObject,
 
         updated = None
         # The signature may vary.
-        # XXX: This is slow and cumbersome and needs to go.
-        # See https://github.com/NextThought/nti.externalization/issues/30
-        try:
-            argspec = inspect.getargspec(updater.updateFromExternalObject)
-        except TypeError: # pragma: no cover (This is hard to catch in pure-python coverage mode)
-            # Cython functions and other extension types are "not a Python function"
-            # and don't work with this. We assume they use the standard form accepting
-            # 'context' as kwarg
-            updated = updater.updateFromExternalObject(externalObject, context=context)
+        arg_kind = _get_update_signature(updater)
+        if arg_kind is _UPDATE_ARGS_TWO:
+            updated = updater.updateFromExternalObject(externalObject, context)
+        elif arg_kind is _UPDATE_ARGS_ONE:
+            updated = updater.updateFromExternalObject(externalObject)
         else:
-            if 'context' in argspec.args or (argspec.keywords and 'dataserver' not in argspec.args):
-                updated = updater.updateFromExternalObject(externalObject,
-                                                           context=context)
-            elif argspec.keywords or 'dataserver' in argspec.args:
-                updated = updater.updateFromExternalObject(externalObject,
-                                                           dataserver=context)
-            else:
-                updated = updater.updateFromExternalObject(externalObject)
+            updated = updater.updateFromExternalObject(externalObject,
+                                                       context=context)
 
         # Broadcast a modified event if the object seems to have changed.
         if notify and (updated is None or updated):
@@ -216,5 +300,6 @@ def update_from_external_object(containedObject, externalObject,
     return containedObject
 
 
+
 from nti.externalization._compat import import_c_accel # pylint:disable=wrong-import-position,wrong-import-order
 import_c_accel(globals(), 'nti.externalization.internalization._updater')