From dd3a574ab618e39e9ed14e39aebef8d1033c4190 Mon Sep 17 00:00:00 2001 From: Jordan Moldow Date: Fri, 2 Sep 2016 12:03:06 -0700 Subject: [PATCH 1/2] Refactor Translator to use less global state In the past, there was one Translator, which was a global singeton, and would be permanently modified any time a new object class with an _item_type was created. Recently, in v2.0.0a1, we tried out a change where the global translator would be modified any time a new object class with an _item_type OR with a baseclass with an _item_type was created. This means that every subclass operation mutated global state. We thought this would make it easier for developers to add their own classes to the SDK. In retrospect, it makes it much harder to write tests (because any temporary subclasses created, including by a mock library, will modify global state for the rest of the test run), and makes it impossible to intentionally create a subclass that shouldn't be registered in the translator. So we are reverting this behavior for v2.0.0a2. Furthermore, this adds the ability to create non-global translators (which are, by default, used on `BoxSession` objects), and the ability to add non-global registrations to these non-global translators. This is now the publicly recommended way for developers to register types, outside of the SDK itself. For now, the old mechanism of implicitly registering the official SDK classes with _item_type is retained. But we can experiment with the new system, and see if we prefer to switch to the explicit registration, and delete the implicit registration system, in v2.0.0a3. Also fix a bug that I discovered in ExtendableEnumMeta. --- HISTORY.rst | 13 +- boxsdk/client/client.py | 27 ++-- boxsdk/object/base_api_json_object.py | 58 ++++++++- boxsdk/object/base_object.py | 3 +- boxsdk/object/events.py | 3 +- boxsdk/object/folder.py | 7 +- boxsdk/object/group.py | 5 +- boxsdk/object/group_membership.py | 5 +- boxsdk/object/search.py | 3 +- boxsdk/session/box_session.py | 44 ++++++- boxsdk/util/chain_map.py | 19 +++ boxsdk/util/enum.py | 2 +- boxsdk/util/singleton.py | 31 ----- boxsdk/util/translator.py | 119 +++++++++++++++--- boxsdk/version.py | 2 +- docs/source/boxsdk.util.rst | 16 +-- setup.py | 1 + test/unit/conftest.py | 37 +++++- test/unit/object/test_base_api_json_object.py | 35 ++++++ test/unit/session/test_box_session.py | 35 +++++- test/unit/util/test_enum.py | 4 + test/unit/util/test_singleton.py | 91 -------------- test/unit/util/test_translator.py | 88 ++++++++++++- tox.ini | 1 + 24 files changed, 456 insertions(+), 193 deletions(-) create mode 100644 boxsdk/util/chain_map.py delete mode 100644 boxsdk/util/singleton.py delete mode 100644 test/unit/util/test_singleton.py diff --git a/HISTORY.rst b/HISTORY.rst index 0e72e0d0d..37e4a672b 100644 --- a/HISTORY.rst +++ b/HISTORY.rst @@ -24,7 +24,17 @@ Release History **Features** -- Added ability to create custom subclasses of SDK objects with ``_item_type`` defined. +- Added more flexibility to the object translation system: + + - Can create non-global ``Translator`` instances, which can extend or + not-extend the global default ``Translator``. + - Can initialize ``BoxSession`` with a custom ``Translator``. + - Can register custom subclasses on the ``Translator`` which is associated + with a ``BoxSession`` or a ``Client``. + - All translation of API responses now use the ``Translator`` that is + referenced by the ``BoxSession``, instead of directly using the global + default ``Translator``. + - Added an ``Event`` class. **Other** @@ -36,6 +46,7 @@ Release History ``BaseObject`` is the parent of all objects that are a part of the REST API. Another subclass of ``BaseAPIJSONObject``, ``APIJSONObject``, was created to represent pseudo-smart objects such as ``Event`` that are not directly accessible through an API endpoint. +- Fixed an exception that was being raised from ``ExtendableEnumMeta.__dir__()``. 1.5.3 (2016-05-26) ++++++++++++++++++ diff --git a/boxsdk/client/client.py b/boxsdk/client/client.py index 0df5fea85..a2fe4fcec 100644 --- a/boxsdk/client/client.py +++ b/boxsdk/client/client.py @@ -11,7 +11,6 @@ from ..object.search import Search from ..object.events import Events from ..util.shared_link import get_shared_link_header -from ..util.translator import Translator class Client(Cloneable): @@ -62,6 +61,10 @@ def session(self): """ return self._session + @property + def translator(self): + return self._session.translator + def folder(self, folder_id): """ Initialize a :class:`Folder` object, whose box id is folder_id. @@ -75,7 +78,7 @@ def folder(self, folder_id): :rtype: :class:`Folder` """ - return Translator().translate('folder')(session=self._session, object_id=folder_id) + return self.translator.translate('folder')(session=self._session, object_id=folder_id) def file(self, file_id): """ @@ -90,7 +93,7 @@ def file(self, file_id): :rtype: :class:`File` """ - return Translator().translate('file')(session=self._session, object_id=file_id) + return self.translator.translate('file')(session=self._session, object_id=file_id) def user(self, user_id='me'): """ @@ -105,7 +108,7 @@ def user(self, user_id='me'): :rtype: :class:`User` """ - return Translator().translate('user')(session=self._session, object_id=user_id) + return self.translator.translate('user')(session=self._session, object_id=user_id) def group(self, group_id): """ @@ -120,7 +123,7 @@ def group(self, group_id): :rtype: :class:`Group` """ - return Translator().translate('group')(session=self._session, object_id=group_id) + return self.translator.translate('group')(session=self._session, object_id=group_id) def collaboration(self, collab_id): """ @@ -135,7 +138,7 @@ def collaboration(self, collab_id): :rtype: :class:`Collaboration` """ - return Translator().translate('collaboration')(session=self._session, object_id=collab_id) + return self.translator.translate('collaboration')(session=self._session, object_id=collab_id) @api_call def users(self, limit=None, offset=0, filter_term=None): @@ -167,7 +170,7 @@ def users(self, limit=None, offset=0, filter_term=None): params['filter_term'] = filter_term box_response = self._session.get(url, params=params) response = box_response.json() - user_class = Translator().translate('user') + user_class = self.translator.translate('user') return [user_class( session=self._session, object_id=item['id'], @@ -256,7 +259,7 @@ def group_membership(self, group_membership_id): :rtype: :class:`GroupMembership` """ - return Translator().translate('group_membership')( + return self.translator.translate('group_membership')( session=self._session, object_id=group_membership_id, ) @@ -274,7 +277,7 @@ def groups(self): url = '{0}/groups'.format(API.BASE_API_URL) box_response = self._session.get(url) response = box_response.json() - group_class = Translator().translate('group') + group_class = self.translator.translate('group') return [group_class( session=self._session, object_id=item['id'], @@ -303,7 +306,7 @@ def create_group(self, name): } box_response = self._session.post(url, data=json.dumps(body_attributes)) response = box_response.json() - return Translator().translate('group')( + return self.translator.translate('group')( session=self._session, object_id=response['id'], response_object=response, @@ -334,7 +337,7 @@ def get_shared_item(self, shared_link, password=None): '{0}/shared_items'.format(API.BASE_API_URL), headers=get_shared_link_header(shared_link, password), ).json() - return Translator().translate(response['type'])( + return self.translator.translate(response['type'])( session=self._session.with_shared_link(shared_link, password), object_id=response['id'], response_object=response, @@ -389,7 +392,7 @@ def create_user(self, name, login=None, **user_attributes): user_attributes['is_platform_access_only'] = True box_response = self._session.post(url, data=json.dumps(user_attributes)) response = box_response.json() - return Translator().translate('user')( + return self.translator.translate('user')( session=self._session, object_id=response['id'], response_object=response, diff --git a/boxsdk/object/base_api_json_object.py b/boxsdk/object/base_api_json_object.py index 1fbf9eaab..f22d9781c 100644 --- a/boxsdk/object/base_api_json_object.py +++ b/boxsdk/object/base_api_json_object.py @@ -9,22 +9,68 @@ class BaseAPIJSONObjectMeta(type): """ - Metaclass for Box API objects. Registers classes so that API responses can be translated to the correct type. - Relies on the _item_type field defined on the classes to match the type property of the response json. - But the type-class mapping will only be registered if the module of the class is imported. - So it's also important to add the module name to __all__ in object/__init__.py. + Metaclass for Box API objects. + + Registers classes with the default translator, so that API responses can be + translated to the correct type. This relies on the _item_type field, which + must be defined in the class's namespace dict (and must be re-defined, in + order to register a custom subclass), to match the 'type' field of the + response json. But the type-class mapping will only be registered if the + module of the class is imported. + + For example, events returned from the API look like + + .. code-block:: json + + {'type': 'event', ...} + + so a class for that type could be created and registered with the default + translator like this: + + .. code-block:: python + + class Event(BaseAPIJSONObject): + _item_type = 'event' + ... + + NOTE: The default translator registration functionality is a private + implementation detail of the SDK, to make it easy to register the default + API object classes with the default translator. For convenience and + backwards-compatability, developers are allowed to re-define the _item_type + field in their own custom subclasses in order to take advantage of this + functionality, but are encouraged not to. Since this is a private + implementation detail, it may change or be removed in any major or minor + release. Additionally, it has the usual hazards of mutable global state. + The supported and recommended ways for registering custom subclasses are: + + - Constructing a new :class:`Translator`, calling `Translator.register()` + as necessary, and passing it to the :class:`BoxSession` constructor. + - Calling `session.translator.register()` on an existing + :class:`BoxSession`. + - Calling `client.translator.register()` on an existing :class:`Client`. """ + def __init__(cls, name, bases, attrs): super(BaseAPIJSONObjectMeta, cls).__init__(name, bases, attrs) - item_type = getattr(cls, '_item_type', None) + item_type = attrs.get('_item_type', None) if item_type is not None: - Translator().register(item_type, cls) + Translator._default_translator.register(item_type, cls) # pylint:disable=protected-access @six.add_metaclass(BaseAPIJSONObjectMeta) class BaseAPIJSONObject(object): """Base class containing basic logic shared between true REST objects and other objects (such as an Event)""" + # :attr _item_type: + # (protected) The Box resource type that this class represents. + # For API object/resource classes this should equal the expected value + # of the 'type' field in API JSON responses. Otherwise, this should be + # `None`. + # :type _item_type: `unicode` or `None` + # + # NOTE: When defining a leaf class with an _item_type in this SDK, it's + # also important to add the module name to __all__ in object/__init__.py, + # so that it will be imported and registered with the default translator. _item_type = None def __init__(self, response_object=None, **kwargs): diff --git a/boxsdk/object/base_object.py b/boxsdk/object/base_object.py index 442a792d1..1606af7c9 100644 --- a/boxsdk/object/base_object.py +++ b/boxsdk/object/base_object.py @@ -5,7 +5,6 @@ from .base_endpoint import BaseEndpoint from .base_api_json_object import BaseAPIJSONObject -from ..util.translator import Translator from ..util.api_call_decorator import api_call @@ -208,7 +207,7 @@ def _paging_wrapper(self, url, starting_index, limit, factory=None): for index_in_current_page, item in enumerate(response['entries']): instance_factory = factory if not instance_factory: - instance_factory = Translator().translate(item['type']) + instance_factory = self._session.translator.translate(item['type']) instance = instance_factory(self._session, item['id'], item) yield instance, current_page_size, index_in_current_page diff --git a/boxsdk/object/events.py b/boxsdk/object/events.py index 456582457..ab15e1909 100644 --- a/boxsdk/object/events.py +++ b/boxsdk/object/events.py @@ -9,7 +9,6 @@ from ..util.enum import ExtendableEnumMeta from ..util.lru_cache import LRUCache from ..util.text_enum import TextEnum -from ..util.translator import Translator # pylint:disable=too-many-ancestors @@ -94,7 +93,7 @@ def get_events(self, limit=100, stream_position=0, stream_type=UserEventsStreamT box_response = self._session.get(url, params=params) response = box_response.json().copy() if 'entries' in response: - response['entries'] = [Translator().translate(item['type'])(item) for item in response['entries']] + response['entries'] = [self._session.translator.translate(item['type'])(item) for item in response['entries']] return response @api_call diff --git a/boxsdk/object/folder.py b/boxsdk/object/folder.py index 38d10b224..a9bd076ff 100644 --- a/boxsdk/object/folder.py +++ b/boxsdk/object/folder.py @@ -11,7 +11,6 @@ from boxsdk.object.user import User from boxsdk.util.api_call_decorator import api_call from boxsdk.util.text_enum import TextEnum -from boxsdk.util.translator import Translator class FolderSyncState(TextEnum): @@ -153,7 +152,7 @@ def get_items(self, limit, offset=0, fields=None): params['fields'] = ','.join(fields) box_response = self._session.get(url, params=params) response = box_response.json() - return [Translator().translate(item['type'])(self._session, item['id'], item) for item in response['entries']] + return [self._session.translator.translate(item['type'])(self._session, item['id'], item) for item in response['entries']] @api_call def upload_stream( @@ -218,7 +217,7 @@ def upload_stream( box_response = self._session.post(url, data=data, files=files, expect_json_response=False) file_response = box_response.json()['entries'][0] file_id = file_response['id'] - return Translator().translate(file_response['type'])( + return self._session.translator.translate(file_response['type'])( session=self._session, object_id=file_id, response_object=file_response, @@ -364,7 +363,7 @@ def add_collaborator(self, collaborator, role, notify=False): box_response = self._session.post(url, expect_json_response=True, data=data, params=params) collaboration_response = box_response.json() collab_id = collaboration_response['id'] - return Translator().translate(collaboration_response['type'])( + return self._session.translator.translate(collaboration_response['type'])( session=self._session, object_id=collab_id, response_object=collaboration_response, diff --git a/boxsdk/object/group.py b/boxsdk/object/group.py index 13bf17fa4..f4f4da964 100644 --- a/boxsdk/object/group.py +++ b/boxsdk/object/group.py @@ -6,7 +6,6 @@ from .base_object import BaseObject from ..config import API -from ..util.translator import Translator from ..util.api_call_decorator import api_call @@ -48,7 +47,7 @@ def membership(self, starting_index=0, limit=100, include_page_info=False): """ url = self.get_url('memberships') - membership_factory = partial(Translator().translate("group_membership"), group=self) + membership_factory = partial(self._session.translator.translate("group_membership"), group=self) for group_membership_tuple in self._paging_wrapper(url, starting_index, limit, membership_factory): if include_page_info: yield group_membership_tuple @@ -83,4 +82,4 @@ def add_member(self, user, role): box_response = self._session.post(url, data=json.dumps(body_attributes)) response = box_response.json() - return Translator().translate(response['type'])(self._session, response['id'], response, user=user, group=self) + return self._session.translator.translate(response['type'])(self._session, response['id'], response, user=user, group=self) diff --git a/boxsdk/object/group_membership.py b/boxsdk/object/group_membership.py index 9d58814f0..50ced62a8 100644 --- a/boxsdk/object/group_membership.py +++ b/boxsdk/object/group_membership.py @@ -3,7 +3,6 @@ from __future__ import unicode_literals, absolute_import from .base_object import BaseObject -from ..util.translator import Translator class GroupMembership(BaseObject): @@ -70,8 +69,8 @@ def _init_user_and_group_instances(session, response_object, user, group): user_info = response_object.get('user') group_info = response_object.get('group') - user = user or Translator().translate(user_info['type'])(session, user_info['id'], user_info) - group = group or Translator().translate(group_info['type'])(session, group_info['id'], group_info) + user = user or session.translator.translate(user_info['type'])(session, user_info['id'], user_info) + group = group or session.translator.translate(group_info['type'])(session, group_info['id'], group_info) return user, group diff --git a/boxsdk/object/search.py b/boxsdk/object/search.py index 14fb59afd..f34462620 100644 --- a/boxsdk/object/search.py +++ b/boxsdk/object/search.py @@ -5,7 +5,6 @@ import json from .base_endpoint import BaseEndpoint -from ..util.translator import Translator from ..util.api_call_decorator import api_call @@ -231,4 +230,4 @@ def search( params.update(kwargs) box_response = self._session.get(url, params=params) response = box_response.json() - return [Translator().translate(item['type'])(self._session, item['id'], item) for item in response['entries']] + return [self._session.translator.translate(item['type'])(self._session, item['id'], item) for item in response['entries']] diff --git a/boxsdk/session/box_session.py b/boxsdk/session/box_session.py index ad0284100..fbccabf34 100644 --- a/boxsdk/session/box_session.py +++ b/boxsdk/session/box_session.py @@ -6,6 +6,7 @@ from boxsdk.exception import BoxAPIException from boxsdk.util.multipart_stream import MultipartStream from boxsdk.util.shared_link import get_shared_link_header +from ..util.translator import Translator class BoxResponse(object): @@ -65,7 +66,7 @@ class BoxSession(object): Box API session. Provides auth, automatic retry of failed requests, and session renewal. """ - def __init__(self, oauth, network_layer, default_headers=None, default_network_request_kwargs=None): + def __init__(self, oauth, network_layer, default_headers=None, translator=None, default_network_request_kwargs=None): """ :param oauth: OAuth2 object used by the session to authorize requests. @@ -79,22 +80,39 @@ def __init__(self, oauth, network_layer, default_headers=None, default_network_r A dictionary containing default values to be used as headers when this session makes an API request. :type default_headers: `dict` or None + :param translator: + (optional) The translator to use for translating Box API JSON + responses into :class:`BaseAPIJSONObject` smart objects. + Defaults to a new :class:`Translator` that inherits the + registrations of the default translator. + :type translator: :class:`Translator` :param default_network_request_kwargs: A dictionary containing default values to be passed to the network layer when this session makes an API request. :type default_network_request_kwargs: `dict` or None """ + if translator is None: + translator = Translator(extend_default_translator=True, new_child=True) super(BoxSession, self).__init__() self._oauth = oauth self._network_layer = network_layer self._default_headers = {'User-Agent': Client.USER_AGENT_STRING} + self._translator = translator self._default_network_request_kwargs = {} if default_headers: self._default_headers.update(default_headers) if default_network_request_kwargs: self._default_network_request_kwargs.update(default_network_request_kwargs) + @property + def translator(self): + """The translator used for translating Box API JSON responses into `BaseAPIJSONObject` smart objects. + + :rtype: :class:`Translator` + """ + return self._translator + def get_url(self, endpoint, *args): """ Return the URL for the given Box API endpoint. @@ -126,7 +144,13 @@ def as_user(self, user): """ headers = self._default_headers.copy() headers['As-User'] = user.object_id - return self.__class__(self._oauth, self._network_layer, headers, self._default_network_request_kwargs.copy()) + return self.__class__( + self._oauth, + self._network_layer, + default_headers=headers, + translator=self._translator, + default_network_request_kwargs=self._default_network_request_kwargs.copy(), + ) def with_shared_link(self, shared_link, shared_link_password=None): """ @@ -143,10 +167,22 @@ def with_shared_link(self, shared_link, shared_link_password=None): """ headers = self._default_headers.copy() headers.update(get_shared_link_header(shared_link, shared_link_password)) - return self.__class__(self._oauth, self._network_layer, headers, self._default_network_request_kwargs.copy()) + return self.__class__( + self._oauth, + self._network_layer, + default_headers=headers, + translator=self._translator, + default_network_request_kwargs=self._default_network_request_kwargs.copy(), + ) def with_default_network_request_kwargs(self, extra_network_parameters): - return self.__class__(self._oauth, self._network_layer, self._default_headers.copy(), extra_network_parameters) + return self.__class__( + self._oauth, + self._network_layer, + default_headers=self._default_headers.copy(), + translator=self._translator, + default_network_request_kwargs=extra_network_parameters, + ) def _renew_session(self, access_token_used): """ diff --git a/boxsdk/util/chain_map.py b/boxsdk/util/chain_map.py new file mode 100644 index 000000000..d4ef6e6a9 --- /dev/null +++ b/boxsdk/util/chain_map.py @@ -0,0 +1,19 @@ +# coding: utf-8 + +from __future__ import absolute_import, unicode_literals + +from functools import wraps + + +__all__ = [str('ChainMap')] + + +try: + from collections import ChainMap +except ImportError: + from chainmap import ChainMap as _ChainMap + + # Make sure that `ChainMap` is a new-style class. + @wraps(_ChainMap, updated=()) + class ChainMap(_ChainMap, object): + __slots__ = () diff --git a/boxsdk/util/enum.py b/boxsdk/util/enum.py index 109103393..a5a986311 100644 --- a/boxsdk/util/enum.py +++ b/boxsdk/util/enum.py @@ -99,7 +99,7 @@ def in_(subclass): return any(map(in_, cls.__subclasses__())) def __dir__(cls): - return list(set(super(ExtendableEnumMeta, cls).__dir__()).union(set(map(dir, cls.__subclasses__())))) + return list(set(super(ExtendableEnumMeta, cls).__dir__()).union(*map(dir, cls.__subclasses__()))) def __getitem__(cls, name): try: diff --git a/boxsdk/util/singleton.py b/boxsdk/util/singleton.py deleted file mode 100644 index 1a9a4992f..000000000 --- a/boxsdk/util/singleton.py +++ /dev/null @@ -1,31 +0,0 @@ -# coding: utf-8 - -from __future__ import unicode_literals -from threading import RLock - - -class Singleton(type): - """ - Metaclass for implementing the singleton pattern. - - Sample usage: - @add_metaclass(Singleton) - class my_singleton(object): - def __init__(self): - pass - """ - _instances = {} - _lock = RLock() - - def __call__(cls, *args, **kwargs): - """ - When the class is instantiated, return the singleton if it exists; else create it and store for the next use. - """ - if cls not in Singleton._instances: - # Using an RLock, in case the singleton being initialized itself initializes another singleton. - # RLock's are slow... but the 'if' just above ensures that slowness is only suffered when any - # Singleton subclass is first initialized. After that the outer 'if' above will skip this code. - with Singleton._lock: - if cls not in Singleton._instances: - Singleton._instances[cls] = super(Singleton, cls).__call__(*args, **kwargs) - return Singleton._instances[cls] diff --git a/boxsdk/util/translator.py b/boxsdk/util/translator.py index 31d7d6ff0..fe456ab5f 100644 --- a/boxsdk/util/translator.py +++ b/boxsdk/util/translator.py @@ -1,21 +1,91 @@ # coding: utf-8 -from __future__ import unicode_literals -from six import add_metaclass -from boxsdk.util.singleton import Singleton +from __future__ import absolute_import, unicode_literals +from .chain_map import ChainMap -@add_metaclass(Singleton) -class Translator(object): + +__all__ = [str('Translator')] + + +class Translator(ChainMap): """ Translate item responses from the Box API to Box objects. + + Also acts as a :class:`Mapping` from type names to Box object classes. + + There exists a global default `Translator`, containing the default API + object classes defined by the SDK. Custom `Translator` instances can be + created to extend the default `Translator` with custom subclasses. + + A `Translator` is a :class:`ChainMap`, so that one translator can "extend" + others. The most common scenario would be a custom, non-global + `Translator` that extends only the default translator, to register 0 or + more new classes. But more complex inheritance is also allowed, in case + that is useful. """ - def __init__(self): - self._type_to_class_mapping = {} - def register(self, type_name, box_cls): + __slots__ = () + + # :attr _default_translator: + # A global `Translator` containing the default API object classes + # defined by the SDK. By default, new `Translator` instances will + # "extend" this one, so that the global registrations are reflected + # automatically. + # + # NOTE: For convenience and backwards-compatability, developers are + # allowed to register their own custom subclasses with + # `_default_translator`, but are encouraged not to. The default + # translator may change or be removed in any major or minor release. + # Additionally, it has the usual hazards of mutable global state. + # The supported and recommended ways for registering custom subclasses + # are: + # + # - Constructing a new `Translator`, calling `Translator.register()` as + # necessary, and passing it to the `BoxSession` constructor. + # - Calling `session.translator.register()` on an existing + # `BoxSession`. + # - Calling `client.translator.register()` on an existing `Client`. + # :type _default_translator: :class:`Translator` + _default_translator = {} # Will be set to a `Translator` instance below, after the class is defined. + + def __init__(self, *translation_maps, **kwargs): + """Baseclass override. + + :param translation_maps: + (variadic) The same as the `maps` variadic parameter to + :class:`ChainMap`, except restricted to maps from type names to Box + object classes. + :type translation_maps: + `tuple` of (:class:`Mapping` of `unicode` to :class:`BaseAPIJSONObjectMeta`) + :param extend_default_translator: + (optional, keyword-only) If `True` (the default), + `_default_translator` is appended to the end of `translation_maps`. + When this functionality is used, the new `Translator` will inherit + all of the global registrations. + :type extend_default_translator: `bool` + :param new_child: + (optional, keyword-only) If `True` (the default), a new empty + `dict` is prepended to the front of `translation_maps`. Either way, + the resulting `Translator` starts out with the same key-value + pairs. But when this is `False`, the first item in + `translation_maps` will be mutated by `__setitem__()` and + `__delitem()__` calls, which will affect other references to it. + Whereas when this is `True`, all items in `translation_maps` are + safe from mutation in normal usage scenarios. + :type new_child: `bool` """ - Associate a Box object class to handle Box API item responses with the given type name. + translation_maps = list(translation_maps) + extend_default_translator = kwargs.pop('extend_default_translator', True) + new_child = kwargs.pop('new_child', True) + if extend_default_translator: + translation_maps.append(self._default_translator) + if new_child: + translation_maps.insert(0, {}) + super(Translator, self).__init__(*translation_maps, **kwargs) + + def register(self, type_name, box_cls): + """Associate a Box object class to handle Box API item responses with the given type name. :param type_name: The type name to be registered. @@ -24,11 +94,27 @@ def register(self, type_name, box_cls): :param box_cls: The Box object class, which will be associated with the type name provided. :type box_cls: - `type` + :class:`BaseAPIJSONObjectMeta` + """ + self[type_name] = box_cls + + def get(self, key, default=None): + """Get the box object class associated with the given type name. + + :param key: + The type name to be translated. + :type key: + `unicode` + :param default: + (optional) The default Box object class to return. + Defaults to `BaseObject`. + :type default: :class:`BaseAPIJSONObjectMeta` + :rtype: :class:`BaseAPIJSONObjectMeta` """ - self._type_to_class_mapping.update({ - type_name: box_cls, - }) + from boxsdk.object.base_object import BaseObject + if default is None: + default = BaseObject + return super(Translator, self).get(key, default) def translate(self, type_name): """ @@ -38,6 +124,9 @@ def translate(self, type_name): The type name to be translated. :type type_name: `unicode` + :rtype: :class:`BaseAPIJSONObjectMeta` """ - from boxsdk.object.base_object import BaseObject - return self._type_to_class_mapping.get(type_name, BaseObject) + return self.get(type_name) + + +Translator._default_translator = Translator(extend_default_translator=False) # pylint:disable=protected-access diff --git a/boxsdk/version.py b/boxsdk/version.py index 207cf8c03..a729a80b3 100644 --- a/boxsdk/version.py +++ b/boxsdk/version.py @@ -3,4 +3,4 @@ from __future__ import unicode_literals, absolute_import -__version__ = '2.0.0a1' +__version__ = '2.0.0a2' diff --git a/docs/source/boxsdk.util.rst b/docs/source/boxsdk.util.rst index e70628a8e..4e7bf0735 100644 --- a/docs/source/boxsdk.util.rst +++ b/docs/source/boxsdk.util.rst @@ -12,6 +12,14 @@ boxsdk.util.api_call_decorator module :undoc-members: :show-inheritance: +boxsdk.util.chain_map module +---------------------------- + +.. automodule:: boxsdk.util.chain_map + :members: + :undoc-members: + :show-inheritance: + boxsdk.util.compat module ------------------------- @@ -68,14 +76,6 @@ boxsdk.util.shared_link module :undoc-members: :show-inheritance: -boxsdk.util.singleton module ----------------------------- - -.. automodule:: boxsdk.util.singleton - :members: - :undoc-members: - :show-inheritance: - boxsdk.util.text_enum module ---------------------------- diff --git a/setup.py b/setup.py index ef4fb24ea..0ad2056f7 100644 --- a/setup.py +++ b/setup.py @@ -72,6 +72,7 @@ def main(): # # [1] # [2] + 'chainmap>=1.0.2': ['2.6', '2.7'], # <'3.3' 'enum34>=1.0.4': ['2.6', '2.7', '3.3'], # <'3.4' 'ordereddict>=1.1': ['2.6'], # <'2.7' } diff --git a/test/unit/conftest.py b/test/unit/conftest.py index 6c82453e4..08d59fcc3 100644 --- a/test/unit/conftest.py +++ b/test/unit/conftest.py @@ -1,19 +1,52 @@ # coding: utf-8 from __future__ import unicode_literals + +import copy import json + from mock import Mock, MagicMock import pytest from boxsdk.auth.oauth2 import DefaultNetwork from boxsdk.network import default_network from boxsdk.network.default_network import DefaultNetworkResponse from boxsdk.session.box_session import BoxResponse, BoxSession +from boxsdk.util.translator import Translator -@pytest.fixture() -def mock_box_session(): +@pytest.fixture(scope='function', autouse=True) +def original_default_translator(): + """A reference to the default translator, before the reference is changed by `default_translator` below.""" + return Translator._default_translator # pylint:disable=protected-access + + +@pytest.yield_fixture(scope='function', autouse=True) +def default_translator(original_default_translator): + """The default translator to use during the test. + + We don't want global state to mutate across tests. So before each test + (because of autouse=True), we make a copy of the default translator, and + assign this copy to Translator._default_translator. At the end of the test, + we reset the reference. + """ + try: + translator = Translator(dict(copy.deepcopy(original_default_translator)), extend_default_translator=False, new_child=False) + Translator._default_translator = translator # pylint:disable=protected-access + yield translator + finally: + Translator._default_translator = original_default_translator # pylint:disable=protected-access + + +@pytest.fixture(scope='function') +def translator(default_translator): + return Translator(extend_default_translator=True, new_child=True) + + +@pytest.fixture(scope='function') +def mock_box_session(translator): mock_session = MagicMock(BoxSession) mock_session.get_url.side_effect = lambda *args, **kwargs: BoxSession.get_url(mock_session, *args, **kwargs) + mock_session.translator = translator return mock_session diff --git a/test/unit/object/test_base_api_json_object.py b/test/unit/object/test_base_api_json_object.py index 3032f1147..483f3b45e 100644 --- a/test/unit/object/test_base_api_json_object.py +++ b/test/unit/object/test_base_api_json_object.py @@ -4,6 +4,8 @@ import pytest from boxsdk.object.base_api_json_object import BaseAPIJSONObject +from boxsdk.object.base_object import BaseObject +from boxsdk.object.folder import Folder @pytest.fixture(params=[{'foo': 'bar'}, {'a': {'b': 'c'}}]) @@ -22,3 +24,36 @@ def test_getitem(base_api_json_object): assert isinstance(test_object, BaseAPIJSONObject) for key in dictionary_response: assert test_object[key] == dictionary_response[key] + + +def test_meta_registers_new_item_type_in_default_translator(default_translator, original_default_translator): + item_type = u'ƒøø' + + class Foo(BaseAPIJSONObject): + _item_type = item_type + + assert default_translator.translate(item_type) is Foo + assert (set(default_translator) - set(original_default_translator)) == set([item_type]) + + +@pytest.mark.parametrize('subclass', [BaseAPIJSONObject, BaseObject, Folder]) +def test_meta_does_not_register_new_subclass_in_default_translator_if_item_type_is_not_defined_in_namespace( + subclass, + default_translator, + original_default_translator, +): + + class Foo(subclass): + pass + + assert Foo not in default_translator.values() + assert default_translator == original_default_translator + + +def test_meta_overrides_registration_if_subclass_redefines_item_type(default_translator, original_default_translator): + + class FolderSubclass(Folder): + _item_type = 'folder' + + assert default_translator.translate('folder') is FolderSubclass + assert default_translator.keys() == original_default_translator.keys() diff --git a/test/unit/session/test_box_session.py b/test/unit/session/test_box_session.py index cdef96cef..54ecdfd9a 100644 --- a/test/unit/session/test_box_session.py +++ b/test/unit/session/test_box_session.py @@ -1,6 +1,6 @@ # coding: utf-8 -from __future__ import unicode_literals +from __future__ import absolute_import, unicode_literals from functools import partial from io import IOBase @@ -12,17 +12,24 @@ from boxsdk.auth.oauth2 import OAuth2 from boxsdk.exception import BoxAPIException from boxsdk.network.default_network import DefaultNetwork, DefaultNetworkResponse -from boxsdk.session.box_session import BoxSession, BoxResponse +from boxsdk.session.box_session import BoxSession, BoxResponse, Translator + + +@pytest.fixture(scope='function', params=[False, True]) +def translator(default_translator, request): + if request.param: + return Translator(extend_default_translator=True, new_child=True) + return None @pytest.fixture -def box_session(): +def box_session(translator): mock_oauth = Mock(OAuth2) mock_oauth.access_token = 'fake_access_token' mock_network_layer = Mock(DefaultNetwork) - return BoxSession(mock_oauth, mock_network_layer) + return BoxSession(mock_oauth, mock_network_layer, translator=translator) @pytest.mark.parametrize('test_method', [ @@ -165,3 +172,23 @@ def test_box_response_properties_pass_through_to_network_response_properties(): assert box_result.ok == mock_network_response.ok assert box_result.status_code == mock_network_response.status_code assert box_result.network_response == mock_network_response + + +def test_translator(box_session, translator, default_translator, original_default_translator): + assert isinstance(box_session.translator, Translator) + assert box_session.translator == default_translator + if translator: + assert box_session.translator is translator + + # Test that adding new registrations works. + + class Foo(object): + pass + + item_type = u'ƒøø' + box_session.translator.register(item_type, Foo) + assert box_session.translator.translate(item_type) is Foo + + # Test that adding new registrations does not affect global state. + assert default_translator == original_default_translator + assert (set(box_session.translator) - set(default_translator)) == set([item_type]) diff --git a/test/unit/util/test_enum.py b/test/unit/util/test_enum.py index 5275636a9..fdb5d5eff 100644 --- a/test/unit/util/test_enum.py +++ b/test/unit/util/test_enum.py @@ -169,3 +169,7 @@ def test_len(EnumBaseWithSubclassesDefined, enum_members): def test_reversed(EnumBaseWithSubclassesDefined): EnumBase = EnumBaseWithSubclassesDefined assert list(reversed(list(reversed(EnumBase)))) == list(EnumBase) + + +def test_dir(EnumBaseWithSubclassesDefined): + assert set(enum_member_names).issubset(dir(EnumBaseWithSubclassesDefined)) diff --git a/test/unit/util/test_singleton.py b/test/unit/util/test_singleton.py deleted file mode 100644 index 4f9a45c80..000000000 --- a/test/unit/util/test_singleton.py +++ /dev/null @@ -1,91 +0,0 @@ -# coding: utf-8 - -from __future__ import unicode_literals -from threading import Event, Thread -import pytest -from six import add_metaclass -from boxsdk.util.singleton import Singleton - -# pylint:disable=redefined-outer-name - - -@pytest.fixture -def singleton_class(): - @add_metaclass(Singleton) - class MySingleton(object): - def __init__(self, on_enter=None, block_on=None): - if on_enter: - on_enter.set() - if block_on: - block_on.wait() - return MySingleton - - -@pytest.fixture -def nested_singleton(singleton_class): - @add_metaclass(Singleton) - class Nested(object): - def __init__(self): - self.single = singleton_class() - return Nested - - -def test_singleton_returns_singleton(singleton_class): - instance1 = singleton_class() - instance2 = singleton_class() - - assert instance1 is instance2 - - -def test_nested_singletons_are_safely_initialized(nested_singleton, singleton_class): - """Ensure that the init of one Singleton can itself init another singleton.""" - nested = nested_singleton() - single = singleton_class() - - assert nested.single is single - - -def test_initializing_singleton_is_atomic(singleton_class): - """ - Forces a race condition on initialization and ensures that only 1 instances is created - - This test is setting up the race condition by creating 2 threads, each which will ask for a reference - to the target singleton. The __init__ method of that singleton can be controlled by and communicate with - this test code via interaction with 2 Event objects. The first thread will be launched and block - (via Event.wait) inside the __init__ of the Singleton. And then the 2nd thread will be unleashed and the - test code can ensure that this second thread can't get into the __init__, because of the atomicity of - the singleton. - - Some my content that doing this test by mocking out the Lock is better/simpler. This implementation uses - real threads and *zero* knowledge of the internals of the Singleton implementation. Thus it is less fragile. - Singleton can change implementation to use some other Locking scheme... and nothing here changes. A mock version - would break in that eventuality. - """ - results = [] - thread1_inside_singleton, thread1_is_released = Event(), Event() - thread2_inside_singleton, thread2_has_started = Event(), Event() - - def entry(on_enter, block_on, thread_has_started): - if thread_has_started: - thread_has_started.set() - single = singleton_class(on_enter, block_on) - results.append(single) - - thread1 = Thread(target=entry, args=(thread1_inside_singleton, thread1_is_released, None)) - thread1.start() - - thread1_inside_singleton.wait() - - thread2 = Thread(target=entry, args=(thread2_inside_singleton, None, thread2_has_started)) - thread2.start() - - thread2_has_started.wait() # We block until the thread is known to have started. - assert not thread2_inside_singleton.wait(0.01) # Wait a bit ensuring that the thread never makes it inside - assert thread2.is_alive() # And yet the thread is still alive. - - thread1_is_released.set() # releasing thread1 will allow both threads to complete. - - thread1.join() - thread2.join() - assert not thread2_inside_singleton.wait(0) # Re-ensure that thread2 never made it inside the singleton __init__ - assert results[0] is results[1] # And of course there's only 1 instance of the singleton diff --git a/test/unit/util/test_translator.py b/test/unit/util/test_translator.py index 3e67bbdff..f5a000ba2 100644 --- a/test/unit/util/test_translator.py +++ b/test/unit/util/test_translator.py @@ -1,7 +1,11 @@ # coding: utf-8 -from __future__ import unicode_literals +from __future__ import absolute_import, unicode_literals + +from itertools import product + import pytest + from boxsdk.object.base_object import BaseObject from boxsdk.object.file import File from boxsdk.object.folder import Folder @@ -53,3 +57,85 @@ def translator_response( def test_translator_converts_response_to_correct_type(response_type): response, object_class = _response_to_class_mapping[response_type] assert type(Translator().translate(response.json()['type']) == object_class) + + +def test_default_translator(): + assert isinstance(Translator._default_translator, Translator) # pylint:disable=protected-access + + +@pytest.mark.parametrize(('extend_default_translator', 'new_child'), list(product([None, True], [False, True]))) +def test_with_extend_default_translator(default_translator, extend_default_translator, new_child): + item_type = 'foo' + + class Foo(object): + pass + + kwargs = {} + if extend_default_translator is not None: + kwargs['extend_default_translator'] = extend_default_translator + translator = Translator({item_type: Foo}, new_child=new_child, **kwargs) + assert set(translator.items()).issuperset(default_translator.items()) + + +@pytest.mark.parametrize('new_child', [False, True]) +def test_without_extend_default_translator(new_child): + item_type = 'foo' + + class Foo(object): + pass + + mapping = {item_type: Foo} + + translator = Translator(mapping, extend_default_translator=False, new_child=new_child) + assert translator == mapping + + +@pytest.mark.parametrize(('new_child', 'extend_default_translator'), list(product([None, True], [False, True]))) +def test_with_new_child(new_child, extend_default_translator): + item_type = 'foo' + + class Foo(object): + pass + + mapping = {item_type: Foo} + + kwargs = {} + if new_child is not None: + kwargs['new_child'] = new_child + + translator = Translator(mapping, extend_default_translator=extend_default_translator, **kwargs) + assert item_type in translator + assert translator.maps[0] == {} + with pytest.raises(KeyError): + del translator[item_type] + + class Bar(Foo): + pass + + translator.register(item_type, Bar) + assert translator.translate(item_type) is Bar + assert mapping == {item_type: Foo} + + +@pytest.mark.parametrize('extend_default_translator', [False, True]) +def test_without_new_child(extend_default_translator): + item_type = 'foo' + + class Foo(object): + pass + + mapping = {item_type: Foo} + + translator = Translator(mapping, new_child=False, extend_default_translator=extend_default_translator) + assert item_type in translator + assert translator.maps[0] is mapping + + class Bar(Foo): + pass + + translator.register(item_type, Bar) + assert translator.translate(item_type) is Bar + assert mapping == {item_type: Bar} + + del translator[item_type] + assert not mapping diff --git a/tox.ini b/tox.ini index 6ebc3d35b..bfba0947b 100644 --- a/tox.ini +++ b/tox.ini @@ -52,6 +52,7 @@ deps = -rrequirements-dev.txt [testenv:docs] changedir = docs deps = + -rrequirements-dev.txt sphinx commands = sphinx-apidoc -f -o source ../boxsdk From bf0b7c34b04ae99ff21dc88a6bebbf0d0712b156 Mon Sep 17 00:00:00 2001 From: Jordan Moldow Date: Fri, 2 Sep 2016 13:25:53 -0700 Subject: [PATCH 2/2] Improvements for PR #165 Add a `translator` property to `BaseEndpoint`, for use in API call methods. Fix test and lint failures. --- .pylintrc | 2 +- boxsdk/client/client.py | 4 ++++ boxsdk/object/base_endpoint.py | 8 ++++++++ boxsdk/object/base_object.py | 2 +- boxsdk/object/events.py | 2 +- boxsdk/object/folder.py | 6 +++--- boxsdk/object/group.py | 4 ++-- boxsdk/object/search.py | 2 +- boxsdk/util/chain_map.py | 3 ++- boxsdk/util/translator.py | 2 +- test/unit/conftest.py | 2 +- test/unit/object/test_base_api_json_object.py | 2 +- test/unit/session/test_box_session.py | 2 +- 13 files changed, 27 insertions(+), 14 deletions(-) diff --git a/.pylintrc b/.pylintrc index f5888bd5e..46e9c35e9 100644 --- a/.pylintrc +++ b/.pylintrc @@ -226,7 +226,7 @@ max-branches=12 max-statements=50 # Maximum number of parents for a class (see R0901). -max-parents=7 +max-parents=14 # Maximum number of attributes for a class (see R0902). max-attributes=15 diff --git a/boxsdk/client/client.py b/boxsdk/client/client.py index a2fe4fcec..6c51aba3c 100644 --- a/boxsdk/client/client.py +++ b/boxsdk/client/client.py @@ -63,6 +63,10 @@ def session(self): @property def translator(self): + """The translator used for translating Box API JSON responses into `BaseAPIJSONObject` smart objects. + + :rtype: :class:`Translator` + """ return self._session.translator def folder(self, folder_id): diff --git a/boxsdk/object/base_endpoint.py b/boxsdk/object/base_endpoint.py index 724801864..b87f5259d 100644 --- a/boxsdk/object/base_endpoint.py +++ b/boxsdk/object/base_endpoint.py @@ -32,6 +32,14 @@ def session(self): """ return self._session + @property + def translator(self): + """The translator used for translating Box API JSON responses into `BaseAPIJSONObject` smart objects. + + :rtype: :class:`Translator` + """ + return self._session.translator + def get_url(self, endpoint, *args): """ Return the URL used to access the endpoint. diff --git a/boxsdk/object/base_object.py b/boxsdk/object/base_object.py index 1606af7c9..48ee12922 100644 --- a/boxsdk/object/base_object.py +++ b/boxsdk/object/base_object.py @@ -207,7 +207,7 @@ def _paging_wrapper(self, url, starting_index, limit, factory=None): for index_in_current_page, item in enumerate(response['entries']): instance_factory = factory if not instance_factory: - instance_factory = self._session.translator.translate(item['type']) + instance_factory = self.translator.translate(item['type']) instance = instance_factory(self._session, item['id'], item) yield instance, current_page_size, index_in_current_page diff --git a/boxsdk/object/events.py b/boxsdk/object/events.py index ab15e1909..7c1299003 100644 --- a/boxsdk/object/events.py +++ b/boxsdk/object/events.py @@ -93,7 +93,7 @@ def get_events(self, limit=100, stream_position=0, stream_type=UserEventsStreamT box_response = self._session.get(url, params=params) response = box_response.json().copy() if 'entries' in response: - response['entries'] = [self._session.translator.translate(item['type'])(item) for item in response['entries']] + response['entries'] = [self.translator.translate(item['type'])(item) for item in response['entries']] return response @api_call diff --git a/boxsdk/object/folder.py b/boxsdk/object/folder.py index a9bd076ff..f75c35cd2 100644 --- a/boxsdk/object/folder.py +++ b/boxsdk/object/folder.py @@ -152,7 +152,7 @@ def get_items(self, limit, offset=0, fields=None): params['fields'] = ','.join(fields) box_response = self._session.get(url, params=params) response = box_response.json() - return [self._session.translator.translate(item['type'])(self._session, item['id'], item) for item in response['entries']] + return [self.translator.translate(item['type'])(self._session, item['id'], item) for item in response['entries']] @api_call def upload_stream( @@ -217,7 +217,7 @@ def upload_stream( box_response = self._session.post(url, data=data, files=files, expect_json_response=False) file_response = box_response.json()['entries'][0] file_id = file_response['id'] - return self._session.translator.translate(file_response['type'])( + return self.translator.translate(file_response['type'])( session=self._session, object_id=file_id, response_object=file_response, @@ -363,7 +363,7 @@ def add_collaborator(self, collaborator, role, notify=False): box_response = self._session.post(url, expect_json_response=True, data=data, params=params) collaboration_response = box_response.json() collab_id = collaboration_response['id'] - return self._session.translator.translate(collaboration_response['type'])( + return self.translator.translate(collaboration_response['type'])( session=self._session, object_id=collab_id, response_object=collaboration_response, diff --git a/boxsdk/object/group.py b/boxsdk/object/group.py index f4f4da964..0a324ad87 100644 --- a/boxsdk/object/group.py +++ b/boxsdk/object/group.py @@ -47,7 +47,7 @@ def membership(self, starting_index=0, limit=100, include_page_info=False): """ url = self.get_url('memberships') - membership_factory = partial(self._session.translator.translate("group_membership"), group=self) + membership_factory = partial(self.translator.translate("group_membership"), group=self) for group_membership_tuple in self._paging_wrapper(url, starting_index, limit, membership_factory): if include_page_info: yield group_membership_tuple @@ -82,4 +82,4 @@ def add_member(self, user, role): box_response = self._session.post(url, data=json.dumps(body_attributes)) response = box_response.json() - return self._session.translator.translate(response['type'])(self._session, response['id'], response, user=user, group=self) + return self.translator.translate(response['type'])(self._session, response['id'], response, user=user, group=self) diff --git a/boxsdk/object/search.py b/boxsdk/object/search.py index f34462620..dbc78e238 100644 --- a/boxsdk/object/search.py +++ b/boxsdk/object/search.py @@ -230,4 +230,4 @@ def search( params.update(kwargs) box_response = self._session.get(url, params=params) response = box_response.json() - return [self._session.translator.translate(item['type'])(self._session, item['id'], item) for item in response['entries']] + return [self.translator.translate(item['type'])(self._session, item['id'], item) for item in response['entries']] diff --git a/boxsdk/util/chain_map.py b/boxsdk/util/chain_map.py index d4ef6e6a9..7df702d00 100644 --- a/boxsdk/util/chain_map.py +++ b/boxsdk/util/chain_map.py @@ -5,9 +5,10 @@ from functools import wraps -__all__ = [str('ChainMap')] +__all__ = list(map(str, ['ChainMap'])) +# pylint:disable=unused-import try: from collections import ChainMap except ImportError: diff --git a/boxsdk/util/translator.py b/boxsdk/util/translator.py index fe456ab5f..8cc3260c3 100644 --- a/boxsdk/util/translator.py +++ b/boxsdk/util/translator.py @@ -5,7 +5,7 @@ from .chain_map import ChainMap -__all__ = [str('Translator')] +__all__ = list(map(str, ['Translator'])) class Translator(ChainMap): diff --git a/test/unit/conftest.py b/test/unit/conftest.py index 08d59fcc3..6f9b9d4c3 100644 --- a/test/unit/conftest.py +++ b/test/unit/conftest.py @@ -38,7 +38,7 @@ def default_translator(original_default_translator): @pytest.fixture(scope='function') -def translator(default_translator): +def translator(default_translator): # pylint:disable=unused-argument return Translator(extend_default_translator=True, new_child=True) diff --git a/test/unit/object/test_base_api_json_object.py b/test/unit/object/test_base_api_json_object.py index 483f3b45e..9fdb00f59 100644 --- a/test/unit/object/test_base_api_json_object.py +++ b/test/unit/object/test_base_api_json_object.py @@ -56,4 +56,4 @@ class FolderSubclass(Folder): _item_type = 'folder' assert default_translator.translate('folder') is FolderSubclass - assert default_translator.keys() == original_default_translator.keys() + assert set(default_translator.keys()) == set(original_default_translator.keys()) diff --git a/test/unit/session/test_box_session.py b/test/unit/session/test_box_session.py index 54ecdfd9a..9b837af2b 100644 --- a/test/unit/session/test_box_session.py +++ b/test/unit/session/test_box_session.py @@ -16,7 +16,7 @@ @pytest.fixture(scope='function', params=[False, True]) -def translator(default_translator, request): +def translator(default_translator, request): # pylint:disable=unused-argument if request.param: return Translator(extend_default_translator=True, new_child=True) return None