APICallWrapper.__get__ must return a bound method (#218)

This magic method was previously returning a plain, unbound
function. This tricks some introspection code, which expects it
to be a bound method.

Also add unit test coverage for that module.

Bump version to 2.0.0a6.

Author: jmoldow

HISTORY.rst

@@ -108,6 +108,11 @@ Release History
   whether the caller reads or streams the content.
 - Add more information to the request/response logs from ``LoggingNetwork``.
 - Add logging for request exceptions in ``LoggingNetwork``.
+- Bugfix so that the return value of ``JWTAuth.refresh()`` correctly matches
+  that of the auth interface (by returning a tuple of
+  ((access token), (refresh token or None)), instead of just the access token).
+  In particular, this fixes an exception in ``BoxSession`` that always occurred
+  when it tried to refresh any ``JWTAuth`` object.
 - Fixed an exception that was being raised from ``ExtendableEnumMeta.__dir__()``.
 - CPython 3.6 support.
 

boxsdk/util/api_call_decorator.py

@@ -4,6 +4,8 @@
 
 from functools import update_wrapper, wraps
 
+from ..object.cloneable import Cloneable
+
 
 def api_call(method):
     """
@@ -33,26 +35,42 @@ class APICallWrapper(object):
     def __init__(self, func_that_makes_an_api_call):
         super(APICallWrapper, self).__init__()
         self._func_that_makes_an_api_call = func_that_makes_an_api_call
+        self.__name__ = func_that_makes_an_api_call.__name__
         update_wrapper(self, func_that_makes_an_api_call)
 
+    def __call__(self, cloneable_instance, *args, **kwargs):
+        return self.__get__(cloneable_instance, type(cloneable_instance))(*args, **kwargs)
+
     def __get__(self, _instance, owner):
+        # `APICallWrapper` is imitating a function. For native functions,
+        # ```func.__get__(None, cls)``` always returns `func`.
+        if _instance is None:
+            return self
+
+        if isinstance(owner, type) and not issubclass(owner, Cloneable):
+            raise TypeError(
+                "descriptor {name!r} must be owned by a 'Cloneable' subclass, not {owner.__name__}"
+                .format(name=self.__name__, owner=owner)
+            )
+        expected_type = owner or Cloneable
+        if not isinstance(_instance, expected_type):
+            raise TypeError(
+                "descriptor {name!r} for {expected_type.__name__!r} objects doesn't apply to {instance.__class__.__name__!r} object"
+                .format(name=self.__name__, expected_type=expected_type, instance=_instance)
+            )
+
         @wraps(self._func_that_makes_an_api_call)
-        def call(*args, **kwargs):
-            instance = _instance
-            if instance is None:
-                # If this is being called as an unbound method, the instance is the first arg.
-                if owner is not None and args and isinstance(args[0], owner):
-                    instance = args[0]
-                    args = args[1:]
-                else:
-                    raise TypeError
+        def call(instance, *args, **kwargs):
             extra_network_parameters = kwargs.pop('extra_network_parameters', None)
             if extra_network_parameters:
                 # If extra_network_parameters is specified, then clone the instance, and specify the parameters
                 # as the defaults to be used.
-                # pylint: disable=protected-access
-                instance = instance.clone(instance._session.with_default_network_request_kwargs(extra_network_parameters))
-                # pylint: enable=protected-access
-            response = self._func_that_makes_an_api_call(instance, *args, **kwargs)
-            return response
-        return call
+                instance = instance.clone(instance.session.with_default_network_request_kwargs(extra_network_parameters))
+
+            method = self._func_that_makes_an_api_call.__get__(instance, owner)
+            return method(*args, **kwargs)
+
+        # Since the caller passed a non-`None` instance to `__get__()`, they
+        # want a bound method back, not an unbound function. Thus, we must bind
+        # `call()` to `_instance` and then return that bound method.
+        return call.__get__(_instance, owner)

boxsdk/version.py

@@ -3,4 +3,4 @@
 from __future__ import unicode_literals, absolute_import
 
 
-__version__ = '2.0.0a5'
+__version__ = '2.0.0a6'

test/unit/util/test_api_call_decorator.py

@@ -0,0 +1,116 @@
+# coding: utf-8
+
+from __future__ import absolute_import, unicode_literals
+
+from boxsdk.object.cloneable import Cloneable
+from boxsdk.util.api_call_decorator import api_call
+from mock import NonCallableMock
+import pytest
+
+
+@pytest.fixture
+def api_call_result():
+    return {'bar': 'ƒøø'}
+
+
+@pytest.fixture(name='api_call_method')
+def api_call_method_fixture(api_call_result):
+
+    @api_call
+    def api_call_method(self, *args, **kwargs):
+        return self, args, kwargs, api_call_result
+
+    return api_call_method
+
+
+@pytest.fixture
+def cloneable_subclass_with_api_call_method(api_call_method):
+    api_call_method_fixture = api_call_method
+
+    # pylint:disable=abstract-method
+    class CloneableSubclass(Cloneable):
+        api_call_method = api_call_method_fixture
+
+    return CloneableSubclass
+
+
+@pytest.fixture
+def mock_cloneable(cloneable_subclass_with_api_call_method):
+
+    # pylint:disable=abstract-method
+    class MockCloneable(cloneable_subclass_with_api_call_method, NonCallableMock):
+        pass
+
+    return MockCloneable(spec_set=cloneable_subclass_with_api_call_method, name='Cloneable')
+
+
+def test_api_call_is_decorator():
+
+    @api_call
+    def func():
+        pass
+
+    assert callable(func)
+    assert hasattr(func, '__get__')
+
+
+def test_api_call_decorated_function_must_be_a_method():
+
+    @api_call
+    def func():
+        pass
+
+    with pytest.raises(TypeError):
+        func()
+
+
+def test_api_call_decorated_method_must_be_a_cloneable_method():
+
+    class Cls(object):
+        @api_call
+        def func(self):
+            pass
+
+    obj = Cls()
+    with pytest.raises(TypeError):
+        obj.func()
+
+
+def test_api_call_decorated_method_must_be_bound_to_an_instance_of_the_owner(mock_cloneable, api_call_method):
+    # pylint:disable=abstract-method
+    class CloneableSubclass2(Cloneable):
+        pass
+
+    with pytest.raises(TypeError):
+        api_call_method.__get__(mock_cloneable, CloneableSubclass2)
+
+
+def test_api_call_decorated_method_returns_itself_when_bound_to_none(api_call_method, cloneable_subclass_with_api_call_method):
+    assert api_call_method.__get__(None, Cloneable) is api_call_method
+    assert not hasattr(api_call_method.__get__(None, Cloneable), '__self__')
+    assert cloneable_subclass_with_api_call_method.api_call_method is api_call_method
+    assert not hasattr(cloneable_subclass_with_api_call_method.api_call_method, '__self__')
+
+
+def test_api_call_decorated_method_binds_to_instance(mock_cloneable, api_call_method):
+    assert api_call_method.__get__(mock_cloneable, Cloneable) is not api_call_method
+    assert api_call_method.__get__(mock_cloneable, Cloneable).__self__ is mock_cloneable
+    assert mock_cloneable.api_call_method is not api_call_method
+    assert mock_cloneable.api_call_method.__self__ is mock_cloneable
+
+
+def test_api_call_decorated_method_delegates_to_wrapped_method(mock_cloneable, api_call_result):
+    args = (1, 2, 'ƒøø', 'bar')
+    kwargs = {'bar': 'ƒøø'}
+    assert mock_cloneable.api_call_method(*args, **kwargs) == (mock_cloneable, args, kwargs, api_call_result)
+
+
+def test_api_call_decorated_method_can_be_called_as_an_unbound_method_with_an_instance_as_the_first_argument(
+        mock_cloneable,
+        api_call_result,
+        cloneable_subclass_with_api_call_method,
+):
+    args = (1, 2, 'ƒøø', 'bar')
+    kwargs = {'bar': 'ƒøø'}
+    api_call_method = cloneable_subclass_with_api_call_method.api_call_method
+    assert api_call_method(mock_cloneable, *args, **kwargs) == (mock_cloneable, args, kwargs, api_call_result)