From 417838dbf8762008adb512307dc3cad8168206c7 Mon Sep 17 00:00:00 2001 From: Jordan Moldow Date: Wed, 26 Jul 2017 18:26:06 -0700 Subject: [PATCH 1/2] APICallWrapper.__get__ must return a bound method 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. --- HISTORY.rst | 5 + boxsdk/util/api_call_decorator.py | 48 ++++++--- boxsdk/version.py | 2 +- test/unit/util/test_api_call_decorator.py | 113 ++++++++++++++++++++++ 4 files changed, 152 insertions(+), 16 deletions(-) create mode 100644 test/unit/util/test_api_call_decorator.py diff --git a/HISTORY.rst b/HISTORY.rst index 4c6b744fd..c3a0078ba 100644 --- a/HISTORY.rst +++ b/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. diff --git a/boxsdk/util/api_call_decorator.py b/boxsdk/util/api_call_decorator.py index 2af7c5f42..44bab8058 100644 --- a/boxsdk/util/api_call_decorator.py +++ b/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) diff --git a/boxsdk/version.py b/boxsdk/version.py index a43741a53..696b23e76 100644 --- a/boxsdk/version.py +++ b/boxsdk/version.py @@ -3,4 +3,4 @@ from __future__ import unicode_literals, absolute_import -__version__ = '2.0.0a5' +__version__ = '2.0.0a6' diff --git a/test/unit/util/test_api_call_decorator.py b/test/unit/util/test_api_call_decorator.py new file mode 100644 index 000000000..ca685063e --- /dev/null +++ b/test/unit/util/test_api_call_decorator.py @@ -0,0 +1,113 @@ +# 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 + + class CloneableSubclass(Cloneable): + api_call_method = api_call_method_fixture + + return CloneableSubclass + + +@pytest.fixture +def mock_cloneable(cloneable_subclass_with_api_call_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): + 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) From 120ca02a62746668834b3a51206f32bb7ee1d763 Mon Sep 17 00:00:00 2001 From: Jordan Moldow Date: Wed, 26 Jul 2017 19:11:00 -0700 Subject: [PATCH 2/2] Fix pylint warnings --- test/unit/util/test_api_call_decorator.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/unit/util/test_api_call_decorator.py b/test/unit/util/test_api_call_decorator.py index ca685063e..9c0209274 100644 --- a/test/unit/util/test_api_call_decorator.py +++ b/test/unit/util/test_api_call_decorator.py @@ -27,6 +27,7 @@ def api_call_method(self, *args, **kwargs): 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 @@ -36,6 +37,7 @@ class CloneableSubclass(Cloneable): @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 @@ -75,6 +77,7 @@ def func(self): 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 @@ -82,7 +85,7 @@ class CloneableSubclass2(Cloneable): 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): +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