From 7938ef48eddb55bd0865bd0df48b5b9d99cc338a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 29 Apr 2015 21:27:57 +0200 Subject: [PATCH 01/42] audio: Stop tweaking tee queue sizes --- docs/changelog.rst | 14 ++++++++++++-- mopidy/audio/actor.py | 4 ---- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index d51b4bf1..f7b7beb4 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,6 +4,16 @@ Changelog This changelog is used to track all major changes to Mopidy. +v1.0.4 (unreleased) +=================== + +Bug fix release. + +- Audio: Since all previous attempts at tweaking the queuing for :issue:`1097` + seems to break things in subtle ways for different users. We are giving up + on tweaking the defaults and just going to live with a bit more lag on + software volume changes. (Fixes: :issue:`1147`) + v1.0.3 (2015-04-28) =================== @@ -16,7 +26,7 @@ Bug fix release. - Audio: Follow-up fix for :issue:`1097` still exhibits issues for certain setups. We are giving this get an other go by setting the buffer size to - maximum 100ms instead of a fixed number of buffers. (Fixes: :issue:`1147`, + maximum 100ms instead of a fixed number of buffers. (Addresses: :issue:`1147`, PR: :issue:`1154`) @@ -30,7 +40,7 @@ Bug fix release. - Audio: Fix for :issue:`1097` tuned down the buffer size in the queue. Turns out this can cause distortions in certain cases. Give this an other go with - a more generous buffer size. (Fixes: :issue:`1147`, PR: :issue:`1152`) + a more generous buffer size. (Addresses: :issue:`1147`, PR: :issue:`1152`) - Audio: Make sure mute events get emitted by software mixer. (Fixes: :issue:`1146`, PR: :issue:`1152`) diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 7cca954a..fcd1e233 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -161,11 +161,7 @@ class _Outputs(gst.Bin): logger.info('Audio output set to "%s"', description) def _add(self, element): - # All tee branches need a queue in front of them. - # But keep the queue short so the volume change isn't to slow: queue = gst.element_factory_make('queue') - queue.set_property('max-size-time', 100 * gst.MSECOND) - self.add(element) self.add(queue) queue.link(element) From 94039e06dc95ba7f994c28363e6b4693fbe3505f Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 29 Apr 2015 21:32:43 +0200 Subject: [PATCH 02/42] models: Make sure sub-classes can extend models --- mopidy/models/immutable.py | 3 ++- tests/models/test_models.py | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 2b7bfa5b..8e282c91 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -22,7 +22,8 @@ class ImmutableObjectMeta(type): attrs['_fields'] = fields attrs['_instances'] = weakref.WeakValueDictionary() - attrs['__slots__'] = ['_hash'] + fields.values() + attrs['__slots__'] = list(attrs.get('__slots__', [])) + attrs['__slots__'].extend(['_hash'] + fields.values()) for ancestor in [b for base in bases for b in inspect.getmro(base)]: if '__weakref__' in getattr(ancestor, '__slots__', []): diff --git a/tests/models/test_models.py b/tests/models/test_models.py index bdfd1896..f0f5ff6c 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -18,6 +18,14 @@ class InheritanceTest(unittest.TestCase): class Foo(Track): pass + def test_sub_class_can_have_its_own_slots(self): + + class Foo(Track): + __slots__ = ('_foo',) + + f = Foo() + f._foo = 123 + class CachingTest(unittest.TestCase): From b1475f7d060b60f0794dbc90f9aae10e0657ec04 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 30 Apr 2015 08:40:13 +0200 Subject: [PATCH 03/42] docs: Update changelog for v1.0.4 --- docs/changelog.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index f7b7beb4..10c413ec 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -4,7 +4,8 @@ Changelog This changelog is used to track all major changes to Mopidy. -v1.0.4 (unreleased) + +v1.0.4 (2015-04-30) =================== Bug fix release. From 2f96dacae82a4a903dfac761a8c9a4307812efee Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 30 Apr 2015 08:41:03 +0200 Subject: [PATCH 04/42] Bump version to 1.0.4 --- mopidy/__init__.py | 2 +- tests/test_version.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/mopidy/__init__.py b/mopidy/__init__.py index c2823270..0bc5410e 100644 --- a/mopidy/__init__.py +++ b/mopidy/__init__.py @@ -30,4 +30,4 @@ except ImportError: warnings.filterwarnings('ignore', 'could not open display') -__version__ = '1.0.3' +__version__ = '1.0.4' diff --git a/tests/test_version.py b/tests/test_version.py index 1bdcfe01..37d0b459 100644 --- a/tests/test_version.py +++ b/tests/test_version.py @@ -58,5 +58,6 @@ class VersionTest(unittest.TestCase): self.assertVersionLess('0.19.5', '1.0.0') self.assertVersionLess('1.0.0', '1.0.1') self.assertVersionLess('1.0.1', '1.0.2') - self.assertVersionLess('1.0.2', __version__) - self.assertVersionLess(__version__, '1.0.4') + self.assertVersionLess('1.0.2', '1.0.3') + self.assertVersionLess('1.0.3', __version__) + self.assertVersionLess(__version__, '1.0.5') From 624a69251febdb164d57082fdbec2a9d96b8aa9c Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 3 May 2015 21:54:34 +0200 Subject: [PATCH 05/42] docs: Move Backend API docs to after Core API --- docs/api/index.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/index.rst b/docs/api/index.rst index 2402186e..3e008f00 100644 --- a/docs/api/index.rst +++ b/docs/api/index.rst @@ -15,8 +15,8 @@ API reference concepts models - backends core + backends audio mixer frontends From bb95dc3b9bf8d03b37142336de720582cd893c25 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Sun, 3 May 2015 22:58:43 +0200 Subject: [PATCH 06/42] models: Make sure parent fields are used by children. Without this change any sub-class would end up with an empty _fields and none of the actual fields would be writable even internally. --- mopidy/models/immutable.py | 4 +++- tests/models/test_models.py | 12 ++++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 8e282c91..485480a9 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -15,7 +15,9 @@ class ImmutableObjectMeta(type): def __new__(cls, name, bases, attrs): fields = {} - for key, value in attrs.items(): + for base in bases: # Copy parent fields over to our state + fields.update(getattr(base, '_fields', {})) + for key, value in attrs.items(): # Add our own fields if isinstance(value, Field): fields[key] = '_' + key value._name = key diff --git a/tests/models/test_models.py b/tests/models/test_models.py index f0f5ff6c..be82d3b2 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -19,6 +19,7 @@ class InheritanceTest(unittest.TestCase): pass def test_sub_class_can_have_its_own_slots(self): + # Needed for things like SpotifyTrack in mopidy-spotify 1.x class Foo(Track): __slots__ = ('_foo',) @@ -26,6 +27,17 @@ class InheritanceTest(unittest.TestCase): f = Foo() f._foo = 123 + def test_sub_class_can_be_initialized(self): + # Fails with following error if fields are not handled across classes. + # TypeError: __init__() got an unexpected keyword argument "type" + # Essentially this is testing that sub-classes take parent _fields into + # account. + + class Foo(Ref): + pass + + Foo.directory() + class CachingTest(unittest.TestCase): From 83c2bf9bcdcbea3e01843c8d444a34a37aecafff Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Sun, 3 May 2015 23:32:35 +0200 Subject: [PATCH 07/42] docs: Fix Sphinx syntax error --- mopidy/core/library.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index e6da95f1..d9803d3f 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -106,7 +106,7 @@ class LibraryController(object): recommended to use this method. :param string field: One of ``artist``, ``albumartist``, ``album``, - ``composer``, ``performer``, ``date``or ``genre``. + ``composer``, ``performer``, ``date`` or ``genre``. :param dict query: Query to use for limiting results, see :meth:`search` for details about the query format. :rtype: set of values corresponding to the requested field type. From a8b15e6af57258972fea085487838b66a8237e94 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 4 May 2015 08:37:09 +0200 Subject: [PATCH 08/42] mpd: Include more context in >=INFO logging --- mopidy/mpd/dispatcher.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index 5abc1b4b..10a05fcb 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -167,7 +167,7 @@ class MpdDispatcher(object): # TODO: check that blacklist items are valid commands? blacklist = self.config['mpd'].get('command_blacklist', []) if tokens and tokens[0] in blacklist: - logger.warning('Client sent us blacklisted command: %s', tokens[0]) + logger.warning('MPD client used blacklisted command: %s', tokens[0]) raise exceptions.MpdDisabled(command=tokens[0]) try: return protocol.commands.call(tokens, context=self.context) From 9b4d76866b60f31a37c5e4ad3d3492c2457e61e9 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Mon, 4 May 2015 08:44:45 +0200 Subject: [PATCH 09/42] mpd: Fix flake8 warning --- mopidy/mpd/dispatcher.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/mopidy/mpd/dispatcher.py b/mopidy/mpd/dispatcher.py index 10a05fcb..a8e2c05c 100644 --- a/mopidy/mpd/dispatcher.py +++ b/mopidy/mpd/dispatcher.py @@ -167,7 +167,8 @@ class MpdDispatcher(object): # TODO: check that blacklist items are valid commands? blacklist = self.config['mpd'].get('command_blacklist', []) if tokens and tokens[0] in blacklist: - logger.warning('MPD client used blacklisted command: %s', tokens[0]) + logger.warning( + 'MPD client used blacklisted command: %s', tokens[0]) raise exceptions.MpdDisabled(command=tokens[0]) try: return protocol.commands.call(tokens, context=self.context) From 07159f69c269ffadf0269d4cb16d649ec27092e4 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 4 May 2015 21:37:17 +0200 Subject: [PATCH 10/42] models: Decouple fields tests from the model metaclass --- tests/models/test_fields.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/models/test_fields.py b/tests/models/test_fields.py index 6ef10f18..bf842fd5 100644 --- a/tests/models/test_fields.py +++ b/tests/models/test_fields.py @@ -3,15 +3,14 @@ from __future__ import absolute_import, unicode_literals import unittest from mopidy.models.fields import * # noqa: F403 -from mopidy.models.immutable import ImmutableObjectMeta def create_instance(field): """Create an instance of a dummy class for testing fields.""" class Dummy(object): - __metaclass__ = ImmutableObjectMeta attr = field + attr._name = 'attr' return Dummy() From 7f6809aebb3eed98ea46d2fc1f33aeda8cd70711 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 4 May 2015 22:36:27 +0200 Subject: [PATCH 11/42] models: Explicitly define which models can be deserialized --- mopidy/models/serialize.py | 12 +++++++----- tests/models/test_models.py | 6 ------ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/mopidy/models/serialize.py b/mopidy/models/serialize.py index 1c438efb..5002a8f7 100644 --- a/mopidy/models/serialize.py +++ b/mopidy/models/serialize.py @@ -2,7 +2,9 @@ from __future__ import absolute_import, unicode_literals import json -from mopidy.models.immutable import ImmutableObject +from mopidy.models import immutable + +_MODELS = ['Ref', 'Artist', 'Album', 'Track', 'TlTrack', 'Playlist'] class ModelJSONEncoder(json.JSONEncoder): @@ -19,7 +21,7 @@ class ModelJSONEncoder(json.JSONEncoder): """ def default(self, obj): - if isinstance(obj, ImmutableObject): + if isinstance(obj, immutable.ImmutableObject): return obj.serialize() return json.JSONEncoder.default(self, obj) @@ -38,8 +40,8 @@ def model_json_decoder(dct): """ if '__model__' in dct: - models = {c.__name__: c for c in ImmutableObject.__subclasses__()} + from mopidy import models model_name = dct.pop('__model__') - if model_name in models: - return models[model_name](**dct) + if model_name in _MODELS: + return getattr(models, model_name)(**dct) return dct diff --git a/tests/models/test_models.py b/tests/models/test_models.py index be82d3b2..5108411a 100644 --- a/tests/models/test_models.py +++ b/tests/models/test_models.py @@ -1168,9 +1168,3 @@ class SearchResultTest(unittest.TestCase): self.assertDictEqual( {'__model__': 'SearchResult', 'uri': 'uri'}, SearchResult(uri='uri').serialize()) - - def test_to_json_and_back(self): - result1 = SearchResult(uri='uri') - serialized = json.dumps(result1, cls=ModelJSONEncoder) - result2 = json.loads(serialized, object_hook=model_json_decoder) - self.assertEqual(result1, result2) From 5989d3a01707ce3b6e75192aedb7037e946bd600 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 4 May 2015 22:39:36 +0200 Subject: [PATCH 12/42] models: Simplify how we add __weakref__ to slots --- mopidy/models/immutable.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 485480a9..0b6ef2f7 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -1,7 +1,6 @@ from __future__ import absolute_import, unicode_literals import copy -import inspect import itertools import weakref @@ -27,12 +26,6 @@ class ImmutableObjectMeta(type): attrs['__slots__'] = list(attrs.get('__slots__', [])) attrs['__slots__'].extend(['_hash'] + fields.values()) - for ancestor in [b for base in bases for b in inspect.getmro(base)]: - if '__weakref__' in getattr(ancestor, '__slots__', []): - break - else: - attrs['__slots__'].append('__weakref__') - return super(ImmutableObjectMeta, cls).__new__(cls, name, bases, attrs) def __call__(cls, *args, **kwargs): # noqa: N805 @@ -56,6 +49,7 @@ class ImmutableObject(object): """ __metaclass__ = ImmutableObjectMeta + __slots__ = ['__weakref__'] def __init__(self, *args, **kwargs): for key, value in kwargs.items(): From b480311d66d8135b53a8e692c1c12786733175fe Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Mon, 4 May 2015 23:41:11 +0200 Subject: [PATCH 13/42] models: Add ValidatedImmutableObject and "revert" ImmutableObject Testing with extension that use custom models it was discovered that the changes to have type safe models were a bit to invasive to be suitable for a minor release. This change fixes this by bringing back ImmutableObjects in their old form, and moving the shinny new features to ValidatedImmutableObject. A subset of the tests for ImmutableObjects have been resurrected to have some confidence in this working the way we think it should. --- mopidy/models/__init__.py | 21 ++-- mopidy/models/fields.py | 7 +- mopidy/models/immutable.py | 206 +++++++++++++++++++++++------------- tests/models/test_legacy.py | 164 ++++++++++++++++++++++++++++ 4 files changed, 312 insertions(+), 86 deletions(-) create mode 100644 tests/models/test_legacy.py diff --git a/mopidy/models/__init__.py b/mopidy/models/__init__.py index e4e8528a..231a472a 100644 --- a/mopidy/models/__init__.py +++ b/mopidy/models/__init__.py @@ -1,15 +1,16 @@ from __future__ import absolute_import, unicode_literals from mopidy.models import fields -from mopidy.models.immutable import ImmutableObject +from mopidy.models.immutable import ImmutableObject, ValidatedImmutableObject from mopidy.models.serialize import ModelJSONEncoder, model_json_decoder __all__ = [ 'ImmutableObject', 'Ref', 'Image', 'Artist', 'Album', 'track', 'TlTrack', - 'Playlist', 'SearchResult', 'model_json_decoder', 'ModelJSONEncoder'] + 'Playlist', 'SearchResult', 'model_json_decoder', 'ModelJSONEncoder', + 'ValidatedImmutableObject'] -class Ref(ImmutableObject): +class Ref(ValidatedImmutableObject): """ Model to represent URI references with a human friendly name and type @@ -81,7 +82,7 @@ class Ref(ImmutableObject): return cls(**kwargs) -class Image(ImmutableObject): +class Image(ValidatedImmutableObject): """ :param string uri: URI of the image @@ -99,7 +100,7 @@ class Image(ImmutableObject): height = fields.Integer(min=0) -class Artist(ImmutableObject): +class Artist(ValidatedImmutableObject): """ :param uri: artist URI @@ -120,7 +121,7 @@ class Artist(ImmutableObject): musicbrainz_id = fields.Identifier() -class Album(ImmutableObject): +class Album(ValidatedImmutableObject): """ :param uri: album URI @@ -169,7 +170,7 @@ class Album(ImmutableObject): # actual usage of this field with more than one image. -class Track(ImmutableObject): +class Track(ValidatedImmutableObject): """ :param uri: track URI @@ -253,7 +254,7 @@ class Track(ImmutableObject): last_modified = fields.Integer(min=0) -class TlTrack(ImmutableObject): +class TlTrack(ValidatedImmutableObject): """ A tracklist track. Wraps a regular track and it's tracklist ID. @@ -292,7 +293,7 @@ class TlTrack(ImmutableObject): return iter([self.tlid, self.track]) -class Playlist(ImmutableObject): +class Playlist(ValidatedImmutableObject): """ :param uri: playlist URI @@ -329,7 +330,7 @@ class Playlist(ImmutableObject): return len(self.tracks) -class SearchResult(ImmutableObject): +class SearchResult(ValidatedImmutableObject): """ :param uri: search result URI diff --git a/mopidy/models/fields.py b/mopidy/models/fields.py index 23154df5..bd0ba9f9 100644 --- a/mopidy/models/fields.py +++ b/mopidy/models/fields.py @@ -4,8 +4,9 @@ from __future__ import absolute_import, unicode_literals class Field(object): """ - Base field for use in :class:`ImmutableObject`. These fields are - responsible for type checking and other data sanitation in our models. + Base field for use in + :class:`~mopidy.models.immutable.ValidatedImmutableObject`. These fields + are responsible for type checking and other data sanitation in our models. For simplicity fields use the Python descriptor protocol to store the values in the instance dictionary. Also note that fields are mutable if @@ -19,7 +20,7 @@ class Field(object): """ def __init__(self, default=None, type=None, choices=None): - self._name = None # Set by ImmutableObjectMeta + self._name = None # Set by ValidatedImmutableObjectMeta self._choices = choices self._default = default self._type = type diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 0b6ef2f7..98cd8b5b 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -8,71 +8,54 @@ from mopidy.models.fields import Field from mopidy.utils import deprecation -class ImmutableObjectMeta(type): - - """Helper to automatically assign field names to descriptors.""" - - def __new__(cls, name, bases, attrs): - fields = {} - for base in bases: # Copy parent fields over to our state - fields.update(getattr(base, '_fields', {})) - for key, value in attrs.items(): # Add our own fields - if isinstance(value, Field): - fields[key] = '_' + key - value._name = key - - attrs['_fields'] = fields - attrs['_instances'] = weakref.WeakValueDictionary() - attrs['__slots__'] = list(attrs.get('__slots__', [])) - attrs['__slots__'].extend(['_hash'] + fields.values()) - - return super(ImmutableObjectMeta, cls).__new__(cls, name, bases, attrs) - - def __call__(cls, *args, **kwargs): # noqa: N805 - instance = super(ImmutableObjectMeta, cls).__call__(*args, **kwargs) - return cls._instances.setdefault(weakref.ref(instance), instance) - - class ImmutableObject(object): - """ Superclass for immutable objects whose fields can only be modified via the - constructor. Fields should be :class:`Field` instances to ensure type - safety in our models. + constructor. - Note that since these models can not be changed, we heavily memoize them - to save memory. So constructing a class with the same arguments twice will - give you the same instance twice. + This version of this class has been retained to avoid breaking any clients + relying on it's behavior. Internally in Mopidy we now use + :class:`ValidatedImmutableObject` for type safety and it's much smaller + memory footprint. :param kwargs: kwargs to set as fields on the object :type kwargs: any """ - __metaclass__ = ImmutableObjectMeta + # Any sub-classes that don't set slots won't be effected by the base using + # slots as they will still get an instance dict. __slots__ = ['__weakref__'] def __init__(self, *args, **kwargs): for key, value in kwargs.items(): - if key not in self._fields: + if not self._is_valid_field(key): raise TypeError( - '__init__() got an unexpected keyword argument "%s"' % - key) - super(ImmutableObject, self).__setattr__(key, value) + '__init__() got an unexpected keyword argument "%s"' % key) + self._set_field(key, value) def __setattr__(self, name, value): - if name in self.__slots__: - return super(ImmutableObject, self).__setattr__(name, value) - raise AttributeError('Object is immutable.') + if name.startswith('_'): + object.__setattr__(self, name, value) + else: + raise AttributeError('Object is immutable.') def __delattr__(self, name): - if name in self.__slots__: - return super(ImmutableObject, self).__delattr__(name) - raise AttributeError('Object is immutable.') + if name.startswith('_'): + object.__delattr__(self, name) + else: + raise AttributeError('Object is immutable.') + + def _is_valid_field(self, name): + return hasattr(self, name) and not callable(getattr(self, name)) + + def _set_field(self, name, value): + if value == getattr(self.__class__, name): + self.__dict__.pop(name, None) + else: + self.__dict__[name] = value def _items(self): - for field, key in self._fields.items(): - if hasattr(self, key): - yield field, getattr(self, key) + return self.__dict__.iteritems() def __repr__(self): kwarg_pairs = [] @@ -88,12 +71,10 @@ class ImmutableObject(object): } def __hash__(self): - if not hasattr(self, '_hash'): - hash_sum = 0 - for key, value in self._items(): - hash_sum += hash(key) + hash(value) - super(ImmutableObject, self).__setattr__('_hash', hash_sum) - return self._hash + hash_sum = 0 + for key, value in self._items(): + hash_sum += hash(key) + hash(value) + return hash_sum def __eq__(self, other): if not isinstance(other, self.__class__): @@ -107,11 +88,108 @@ class ImmutableObject(object): def copy(self, **values): """ .. deprecated:: 1.1 - Use :meth:`replace` instead. Note that we no longer return copies. + Use :meth:`replace` instead. """ deprecation.warn('model.immutable.copy') return self.replace(**values) + def replace(self, **kwargs): + """ + Replace the fields in the model and return a new instance + + Examples:: + + # Returns a track with a new name + Track(name='foo').replace(name='bar') + # Return an album with a new number of tracks + Album(num_tracks=2).replace(num_tracks=5) + + :param kwargs: kwargs to set as fields on the object + :type kwargs: any + :rtype: instance of the model with replaced fields + """ + other = copy.copy(self) + for key, value in kwargs.items(): + if not self._is_valid_field(key): + raise TypeError( + 'copy() got an unexpected keyword argument "%s"' % key) + other._set_field(key, value) + return other + + def serialize(self): + data = {} + data['__model__'] = self.__class__.__name__ + for key, value in self._items(): + if isinstance(value, (set, frozenset, list, tuple)): + value = [ + v.serialize() if isinstance(v, ImmutableObject) else v + for v in value] + elif isinstance(value, ImmutableObject): + value = value.serialize() + if not (isinstance(value, list) and len(value) == 0): + data[key] = value + return data + + +class _ValidatedImmutableObjectMeta(type): + + """Helper that initializes fields, slots and memoizes instance creation.""" + + def __new__(cls, name, bases, attrs): + fields = {} + + for base in bases: # Copy parent fields over to our state + fields.update(getattr(base, '_fields', {})) + + for key, value in attrs.items(): # Add our own fields + if isinstance(value, Field): + fields[key] = '_' + key + value._name = key + + attrs['_fields'] = fields + attrs['_instances'] = weakref.WeakValueDictionary() + attrs['__slots__'] = list(attrs.get('__slots__', [])) + fields.values() + + return super(_ValidatedImmutableObjectMeta, cls).__new__( + cls, name, bases, attrs) + + def __call__(cls, *args, **kwargs): # noqa: N805 + instance = super(_ValidatedImmutableObjectMeta, cls).__call__( + *args, **kwargs) + return cls._instances.setdefault(weakref.ref(instance), instance) + + +class ValidatedImmutableObject(ImmutableObject): + """ + Superclass for immutable objects whose fields can only be modified via the + constructor. Fields should be :class:`Field` instances to ensure type + safety in our models. + + Note that since these models can not be changed, we heavily memoize them + to save memory. So constructing a class with the same arguments twice will + give you the same instance twice. + """ + + __metaclass__ = _ValidatedImmutableObjectMeta + __slots__ = ['_hash'] + + def __hash__(self): + if not hasattr(self, '_hash'): + hash_sum = super(ValidatedImmutableObject, self).__hash__() + object.__setattr__(self, '_hash', hash_sum) + return self._hash + + def _is_valid_field(self, name): + return name in self._fields + + def _set_field(self, name, value): + object.__setattr__(self, name, value) + + def _items(self): + for field, key in self._fields.items(): + if hasattr(self, key): + yield field, getattr(self, key) + def replace(self, **kwargs): """ Replace the fields in the model and return a new instance @@ -133,25 +211,7 @@ class ImmutableObject(object): """ if not kwargs: return self - other = copy.copy(self) - for key, value in kwargs.items(): - if key not in self._fields: - raise TypeError( - 'copy() got an unexpected keyword argument "%s"' % key) - super(ImmutableObject, other).__setattr__(key, value) - super(ImmutableObject, other).__delattr__('_hash') + other = super(ValidatedImmutableObject, self).replace(**kwargs) + if hasattr(self, '_hash'): + object.__delattr__(other, '_hash') return self._instances.setdefault(weakref.ref(other), other) - - def serialize(self): - data = {} - data['__model__'] = self.__class__.__name__ - for key, value in self._items(): - if isinstance(value, (set, frozenset, list, tuple)): - value = [ - v.serialize() if isinstance(v, ImmutableObject) else v - for v in value] - elif isinstance(value, ImmutableObject): - value = value.serialize() - if not (isinstance(value, list) and len(value) == 0): - data[key] = value - return data diff --git a/tests/models/test_legacy.py b/tests/models/test_legacy.py new file mode 100644 index 00000000..d837d738 --- /dev/null +++ b/tests/models/test_legacy.py @@ -0,0 +1,164 @@ +from __future__ import absolute_import, unicode_literals + +import unittest + +from mopidy.models import ImmutableObject + + +class Model(ImmutableObject): + uri = None + name = None + models = frozenset() + + def __init__(self, *args, **kwargs): + self.__dict__['models'] = frozenset(kwargs.pop('models', None) or []) + super(Model, self).__init__(self, *args, **kwargs) + + +class SubModel(ImmutableObject): + uri = None + name = None + + +class GenericCopyTest(unittest.TestCase): + def compare(self, orig, other): + self.assertEqual(orig, other) + self.assertNotEqual(id(orig), id(other)) + + def test_copying_model(self): + model = Model() + self.compare(model, model.replace()) + + def test_copying_model_with_basic_values(self): + model = Model(name='foo', uri='bar') + other = model.replace(name='baz') + self.assertEqual('baz', other.name) + self.assertEqual('bar', other.uri) + + def test_copying_model_with_missing_values(self): + model = Model(uri='bar') + other = model.replace(name='baz') + self.assertEqual('baz', other.name) + self.assertEqual('bar', other.uri) + + def test_copying_model_with_private_internal_value(self): + model = Model(models=[SubModel(name=123)]) + other = model.replace(models=[SubModel(name=345)]) + self.assertIn(SubModel(name=345), other.models) + + def test_copying_model_with_invalid_key(self): + with self.assertRaises(TypeError): + Model().replace(invalid_key=True) + + def test_copying_model_to_remove(self): + model = Model(name='foo').replace(name=None) + self.assertEqual(model, Model()) + + +class ModelTest(unittest.TestCase): + def test_uri(self): + uri = 'an_uri' + model = Model(uri=uri) + self.assertEqual(model.uri, uri) + with self.assertRaises(AttributeError): + model.uri = None + + def test_name(self): + name = 'a name' + model = Model(name=name) + self.assertEqual(model.name, name) + with self.assertRaises(AttributeError): + model.name = None + + def test_submodels(self): + models = [SubModel(name=123), SubModel(name=456)] + model = Model(models=models) + self.assertEqual(set(model.models), set(models)) + with self.assertRaises(AttributeError): + model.models = None + + def test_models_none(self): + self.assertEqual(set(), Model(models=None).models) + + def test_invalid_kwarg(self): + with self.assertRaises(TypeError): + Model(foo='baz') + + def test_repr_without_models(self): + self.assertEqual( + "Model(name=u'name', uri=u'uri')", + repr(Model(uri='uri', name='name'))) + + def test_repr_with_models(self): + self.assertEqual( + "Model(models=[SubModel(name=123)], name=u'name', uri=u'uri')", + repr(Model(uri='uri', name='name', models=[SubModel(name=123)]))) + + def test_serialize_without_models(self): + self.assertDictEqual( + {'__model__': 'Model', 'uri': 'uri', 'name': 'name'}, + Model(uri='uri', name='name').serialize()) + + def test_serialize_with_models(self): + submodel = SubModel(name=123) + self.assertDictEqual( + {'__model__': 'Model', 'uri': 'uri', 'name': 'name', + 'models': [submodel.serialize()]}, + Model(uri='uri', name='name', models=[submodel]).serialize()) + + def test_eq_uri(self): + model1 = Model(uri='uri1') + model2 = Model(uri='uri1') + self.assertEqual(model1, model2) + self.assertEqual(hash(model1), hash(model2)) + + def test_eq_name(self): + model1 = Model(name='name1') + model2 = Model(name='name1') + self.assertEqual(model1, model2) + self.assertEqual(hash(model1), hash(model2)) + + def test_eq_models(self): + models = [SubModel()] + model1 = Model(models=models) + model2 = Model(models=models) + self.assertEqual(model1, model2) + self.assertEqual(hash(model1), hash(model2)) + + def test_eq_models_order(self): + submodel1 = SubModel(name='name1') + submodel2 = SubModel(name='name2') + model1 = Model(models=[submodel1, submodel2]) + model2 = Model(models=[submodel2, submodel1]) + self.assertEqual(model1, model2) + self.assertEqual(hash(model1), hash(model2)) + + def test_eq_none(self): + self.assertNotEqual(Model(), None) + + def test_eq_other(self): + self.assertNotEqual(Model(), 'other') + + def test_ne_uri(self): + model1 = Model(uri='uri1') + model2 = Model(uri='uri2') + self.assertNotEqual(model1, model2) + self.assertNotEqual(hash(model1), hash(model2)) + + def test_ne_name(self): + model1 = Model(name='name1') + model2 = Model(name='name2') + self.assertNotEqual(model1, model2) + self.assertNotEqual(hash(model1), hash(model2)) + + def test_ne_models(self): + model1 = Model(models=[SubModel(name='name1')]) + model2 = Model(models=[SubModel(name='name2')]) + self.assertNotEqual(model1, model2) + self.assertNotEqual(hash(model1), hash(model2)) + + def test_ignores_values_with_default_value_none(self): + model1 = Model(name='name1') + model2 = Model(name='name1', uri=None) + self.assertEqual(model1, model2) + self.assertEqual(hash(model1), hash(model2)) From 85871fb33d86595c1b0a6c0c1f4191d8635f8d2a Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 5 May 2015 00:00:22 +0200 Subject: [PATCH 14/42] docs: Improve fields documentation --- docs/api/models.rst | 25 +++++++++++++++++++++++-- mopidy/models/fields.py | 24 ++++++++++++++++++++++-- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/docs/api/models.rst b/docs/api/models.rst index d2d8ec0a..d25e7f34 100644 --- a/docs/api/models.rst +++ b/docs/api/models.rst @@ -77,8 +77,29 @@ Data model helpers .. autoclass:: mopidy.models.ImmutableObject :members: -.. autoclass:: mopidy.models.Field +.. autoclass:: mopidy.models.ValidatedImmutableObject + :members: replace + +Data model (de)serialization +---------------------------- + +.. autofunction:: mopidy.models.model_json_decoder .. autoclass:: mopidy.models.ModelJSONEncoder -.. autofunction:: mopidy.models.model_json_decoder +Data model field types +---------------------- + +.. autoclass:: mopidy.models.fields.Field + +.. autoclass:: mopidy.models.fields.String + +.. autoclass:: mopidy.models.fields.Identifier + +.. autoclass:: mopidy.models.fields.URI + +.. autoclass:: mopidy.models.fields.Date + +.. autoclass:: mopidy.models.fields.Integer + +.. autoclass:: mopidy.models.fields.Collection diff --git a/mopidy/models/fields.py b/mopidy/models/fields.py index bd0ba9f9..01a03a75 100644 --- a/mopidy/models/fields.py +++ b/mopidy/models/fields.py @@ -73,20 +73,41 @@ class String(Field): class Date(String): + """ + :class:`Field` for storing ISO 8601 dates as a string. + + Supported formats are ``YYYY-MM-DD``, ``YYYY-MM`` and ``YYYY``, currently + not validated. + + :param default: default value for field + """ pass # TODO: make this check for YYYY-MM-DD, YYYY-MM, YYYY using strptime. class Identifier(String): + """ + :class:`Field` for storing ASCII values such as GUIDs or other identifiers. + + Values will be interned. + + :param default: default value for field + """ def validate(self, value): return intern(str(super(Identifier, self).validate(value))) class URI(Identifier): + """ + :class`Field` for storing URIs + + Values will be interned, currently not validated. + + :param default: default value for field + """ pass # TODO: validate URIs? class Integer(Field): - """ :class:`Field` for storing integer numbers. @@ -112,7 +133,6 @@ class Integer(Field): class Collection(Field): - """ :class:`Field` for storing collections of a given type. From dd4a8f3b785641edad70a89d5a7281011c3e2167 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 5 May 2015 22:55:53 +0200 Subject: [PATCH 15/42] core: Make sure library can handle bad data from backends Note that None values are just ignored, while other bad data logs an error message and is ignored. --- mopidy/core/library.py | 93 ++++++++++++++++---------- tests/core/test_library.py | 130 ++++++++++++++++++++++++++++++++++--- 2 files changed, 181 insertions(+), 42 deletions(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index d9803d3f..1ca21457 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -1,16 +1,30 @@ from __future__ import absolute_import, unicode_literals import collections +import contextlib import logging import operator import urlparse +from mopidy import compat, exceptions, models from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _backend_error_handling(backend): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s backend returned bad data: %s', + backend.actor_ref.actor_class.__name__, e) + except Exception: + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) + + class LibraryController(object): pykka_traversable = True @@ -79,22 +93,24 @@ class LibraryController(object): backends = self.backends.with_library_browse.values() futures = {b: b.library.root_directory for b in backends} for backend, future in futures.items(): - try: - directories.add(future.get()) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + root = future.get() + validation.check_instance(root, models.Ref) + directories.add(root) return sorted(directories, key=operator.attrgetter('name')) def _browse(self, uri): scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_library_browse.get(scheme) - try: - if backend: - return backend.library.browse(uri).get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + + if not backend: + return [] + + with _backend_error_handling(backend): + result = backend.library.browse(uri).get() + validation.check_instances(result, models.Ref) + return result + return [] def get_distinct(self, field, query=None): @@ -120,11 +136,11 @@ class LibraryController(object): futures = {b: b.library.get_distinct(field, query) for b in self.backends.with_library.values()} for backend, future in futures.items(): - try: - result.update(future.get()) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + values = future.get() + if values is not None: + validation.check_instances(values, compat.text_type) + result.update(values) return result def get_images(self, uris): @@ -152,12 +168,18 @@ class LibraryController(object): results = {uri: tuple() for uri in uris} for backend, future in futures.items(): - try: + with _backend_error_handling(backend): + if future.get() is None: + continue + validation.check_instance(future.get(), collections.Mapping) for uri, images in future.get().items(): + if uri not in uris: + name = backend.actor_ref.actor_class.__name__ + logger.warning( + '%s backend returned image for URI we did not ' + 'ask for: %s', name, uri) + validation.check_instances(images, models.Image) results[uri] += tuple(images) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) return results def find_exact(self, query=None, uris=None, **kwargs): @@ -202,7 +224,7 @@ class LibraryController(object): uris = [uri] futures = {} - result = {u: [] for u in uris} + results = {u: [] for u in uris} # TODO: lookup(uris) to backend APIs for backend, backend_uris in self._get_backends_to_uris(uris).items(): @@ -210,15 +232,15 @@ class LibraryController(object): futures[(backend, u)] = backend.library.lookup(u) for (backend, u), future in futures.items(): - try: - result[u] = future.get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + result = future.get() + if result is not None: + validation.check_instances(result, models.Track) + results[u] = result if uri: - return result[uri] - return result + return results[uri] + return results def refresh(self, uri=None): """ @@ -241,11 +263,8 @@ class LibraryController(object): futures[backend] = backend.library.refresh(uri) for backend, future in futures.items(): - try: + with _backend_error_handling(backend): future.get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) def search(self, query=None, uris=None, exact=False, **kwargs): """ @@ -313,8 +332,14 @@ class LibraryController(object): results = [] for backend, future in futures.items(): - try: - results.append(future.get()) + try: # TODO: fix all these cases so we can use common helper + result = future.get() + if result is not None: + validation.check_instance(result, models.SearchResult) + results.append(result) + except exceptions.ValidationError as e: + logger.error('%s backend returned bad data: %s', + backend.actor_ref.actor_class.__name__, e) except TypeError: backend_name = backend.actor_ref.actor_class.__name__ logger.warning( diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 89f3b284..527e5272 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -15,6 +15,7 @@ class BaseCoreLibraryTest(unittest.TestCase): dummy1_root = Ref.directory(uri='dummy1:directory', name='dummy1') self.backend1 = mock.Mock() self.backend1.uri_schemes.get.return_value = ['dummy1'] + self.backend1.actor_ref.actor_class.__name__ = 'DummyBackend1' self.library1 = mock.Mock(spec=backend.LibraryProvider) self.library1.get_images().get.return_value = {} self.library1.get_images.reset_mock() @@ -24,6 +25,7 @@ class BaseCoreLibraryTest(unittest.TestCase): dummy2_root = Ref.directory(uri='dummy2:directory', name='dummy2') self.backend2 = mock.Mock() self.backend2.uri_schemes.get.return_value = ['dummy2', 'du2'] + self.backend2.actor_ref.actor_class.__name__ = 'DummyBackend2' self.library2 = mock.Mock(spec=backend.LibraryProvider) self.library2.get_images().get.return_value = {} self.library2.get_images.reset_mock() @@ -33,6 +35,7 @@ class BaseCoreLibraryTest(unittest.TestCase): # A backend without the optional library provider self.backend3 = mock.Mock() self.backend3.uri_schemes.get.return_value = ['dummy3'] + self.backend3.actor_ref.actor_class.__name__ = 'DummyBackend3' self.backend3.has_library().get.return_value = False self.backend3.has_library_browse().get.return_value = False @@ -156,11 +159,14 @@ class CoreLibraryTest(BaseCoreLibraryTest): self.core.library.lookup('dummy1:a', ['dummy2:a']) def test_lookup_can_handle_uris(self): - self.library1.lookup().get.return_value = [1234] - self.library2.lookup().get.return_value = [5678] + track1 = Track(name='abc') + track2 = Track(name='def') + + self.library1.lookup().get.return_value = [track1] + self.library2.lookup().get.return_value = [track2] result = self.core.library.lookup(uris=['dummy1:a', 'dummy2:a']) - self.assertEqual(result, {'dummy2:a': [5678], 'dummy1:a': [1234]}) + self.assertEqual(result, {'dummy2:a': [track2], 'dummy1:a': [track1]}) def test_lookup_uris_returns_empty_list_for_dummy3_track(self): result = self.core.library.lookup(uris=['dummy3:a']) @@ -363,12 +369,14 @@ class DeprecatedLookupCoreLibraryTest(BaseCoreLibraryTest): return super(DeprecatedLookupCoreLibraryTest, self).run(result) def test_lookup_selects_dummy1_backend(self): + self.library1.lookup.return_value.get.return_value = [] self.core.library.lookup('dummy1:a') self.library1.lookup.assert_called_once_with('dummy1:a') self.assertFalse(self.library2.lookup.called) def test_lookup_selects_dummy2_backend(self): + self.library2.lookup.return_value.get.return_value = [] self.core.library.lookup('dummy2:a') self.assertFalse(self.library1.lookup.called) @@ -443,28 +451,124 @@ class BackendFailuresCoreLibraryTest(unittest.TestCase): self.assertEqual([], self.core.library.browse(None)) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + def test_browse_backend_get_root_bad_value(self, logger): + self.library.root_directory.get.return_value = 123 + self.assertEqual([], self.core.library.browse(None)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_browse_backend_get_root_none(self, logger): + self.library.root_directory.get.return_value = None + self.assertEqual([], self.core.library.browse(None)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + def test_browse_backend_browse_uri_exception_gets_ignored(self, logger): self.library.browse.return_value.get.side_effect = Exception self.assertEqual([], self.core.library.browse('dummy:directory')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + def test_browse_backend_browse_uri_bad_value(self, logger): + self.library.browse.return_value.get.return_value = [123] + self.assertEqual([], self.core.library.browse('dummy:directory')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + def test_get_distinct_backend_exception_gets_ignored(self, logger): self.library.get_distinct.return_value.get.side_effect = Exception self.assertEqual(set(), self.core.library.get_distinct('artist')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + def test_get_distinct_backend_returns_string(self, logger): + self.library.get_distinct.return_value.get.return_value = 'abc' + self.assertEqual(set(), self.core.library.get_distinct('artist')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_get_distinct_backend_returns_none(self, logger): + self.library.get_distinct.return_value.get.return_value = None + self.assertEqual(set(), self.core.library.get_distinct('artist')) + self.assertFalse(logger.error.called) + + def test_get_distinct_backend_returns_list_if_ints(self, logger): + self.library.get_distinct.return_value.get.return_value = [1, 2, 3] + self.assertEqual(set(), self.core.library.get_distinct('artist')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + def test_get_images_backend_exception_get_ignored(self, logger): + uri = 'dummy:/1' self.library.get_images.return_value.get.side_effect = Exception - self.assertEqual( - {'dummy:/1': tuple()}, self.core.library.get_images(['dummy:/1'])) + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_lookup_backend_exceptiosn_gets_ignores(self, logger): + def test_get_images_backend_returns_string(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = 'abc' + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_get_images_backend_returns_none(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = None + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + self.assertFalse(logger.error.called) + + def test_get_images_backend_returns_bad_dict(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = {uri: 'abc'} + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_get_images_backend_returns_dict_with_none(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = {uri: None} + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_get_images_backend_returns_wrong_uri(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = {'foo': []} + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.warning.assert_called_with(mock.ANY, 'DummyBackend', 'foo') + + def test_lookup_backend_exceptions_gets_ignored(self, logger): + uri = 'dummy:/1' self.library.lookup.return_value.get.side_effect = Exception - self.assertEqual( - {'dummy:/1': []}, self.core.library.lookup(uris=['dummy:/1'])) + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + def test_lookup_uris_backend_returns_string(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = 'abc' + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_lookup_uris_backend_returns_none(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = None + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + self.assertFalse(logger.error.called) + + def test_lookup_uris_backend_returns_bad_list(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = [123] + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_lookup_uri_backend_returns_string(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.library.lookup(uri)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_lookup_uri_backend_returns_none(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = None + self.assertEqual([], self.core.library.lookup(uri)) + self.assertFalse(logger.error.called) + + def test_lookup_uri_backend_returns_bad_list(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = [123] + self.assertEqual([], self.core.library.lookup(uri)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + def test_refresh_backend_exception_gets_ignored(self, logger): self.library.refresh.return_value.get.side_effect = Exception self.core.library.refresh() @@ -486,3 +590,13 @@ class BackendFailuresCoreLibraryTest(unittest.TestCase): self.library.search.return_value.get.side_effect = LookupError with self.assertRaises(LookupError): self.core.library.search(query={'any': ['foo']}) + + def test_search_backend_returns_string(self, logger): + self.library.search.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.library.search(query={'any': ['foo']})) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_search_backend_returns_none(self, logger): + self.library.search.return_value.get.return_value = None + self.assertEqual([], self.core.library.search(query={'any': ['foo']})) + self.assertFalse(logger.error.called) From 3426633c78545f7daaf6c5cca26964b65245496e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 5 May 2015 23:41:46 +0200 Subject: [PATCH 16/42] core: Make sure we handle bad mixer data and exceptions. --- mopidy/core/mixer.py | 52 +++++++++++++++++++++++++++++++++------- tests/core/test_mixer.py | 48 +++++++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/mopidy/core/mixer.py b/mopidy/core/mixer.py index fde7ee5a..94188d50 100644 --- a/mopidy/core/mixer.py +++ b/mopidy/core/mixer.py @@ -1,10 +1,24 @@ from __future__ import absolute_import, unicode_literals +import contextlib import logging +from mopidy import exceptions from mopidy.utils import validation +@contextlib.contextmanager +def _mixer_error_handling(mixer): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s mixer returned bad data: %s', + mixer.actor_ref.actor_class.__name__, e) + except Exception: + logger.exception('%s mixer caused an exception.', + mixer.actor_ref.actor_class.__name__) + + logger = logging.getLogger(__name__) @@ -21,8 +35,15 @@ class MixerController(object): The volume scale is linear. """ - if self._mixer is not None: - return self._mixer.get_volume().get() + if self._mixer is None: + return None + + with _mixer_error_handling(self._mixer): + volume = self._mixer.get_volume().get() + volume is None or validation.check_integer(volume, min=0, max=100) + return volume + + return None def set_volume(self, volume): """Set the volume. @@ -37,8 +58,12 @@ class MixerController(object): if self._mixer is None: return False - else: - return self._mixer.set_volume(volume).get() + + with _mixer_error_handling(self._mixer): + # TODO: log non-bool return values? + return bool(self._mixer.set_volume(volume).get()) + + return False def get_mute(self): """Get mute state. @@ -46,8 +71,15 @@ class MixerController(object): :class:`True` if muted, :class:`False` unmuted, :class:`None` if unknown. """ - if self._mixer is not None: - return self._mixer.get_mute().get() + if self._mixer is None: + return None + + with _mixer_error_handling(self._mixer): + mute = self._mixer.get_mute().get() + mute is None or validation.check_instance(mute, bool) + return mute + + return None def set_mute(self, mute): """Set mute state. @@ -59,5 +91,9 @@ class MixerController(object): validation.check_boolean(mute) if self._mixer is None: return False - else: - return self._mixer.set_mute(bool(mute)).get() + + with _mixer_error_handling(self._mixer): + # TODO: log non-bool return values? + return bool(self._mixer.set_mute(bool(mute)).get()) + + return None diff --git a/tests/core/test_mixer.py b/tests/core/test_mixer.py index c4ef7fe9..61e054d0 100644 --- a/tests/core/test_mixer.py +++ b/tests/core/test_mixer.py @@ -92,3 +92,51 @@ class CoreNoneMixerListenerTest(unittest.TestCase): def test_forwards_mixer_mute_changed_event_to_frontends(self, send): self.core.mixer.set_mute(mute=True) self.assertEqual(send.call_count, 0) + + +class CoreBadMixerTest(unittest.TestCase): + + def setUp(self): # noqa: N802 + self.mixer = mock.Mock() + self.mixer.actor_ref.actor_class.__name__ = 'DummyMixer' + self.core = core.Core(mixer=self.mixer, backends=[]) + + def test_get_volume_raises_exception(self): + self.mixer.get_volume.return_value.get.side_effect = Exception + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_get_volume_returns_negative(self): + self.mixer.get_volume.return_value.get.return_value = -1 + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_get_volume_returns_out_of_bound(self): + self.mixer.get_volume.return_value.get.return_value = 1000 + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_get_volume_returns_wrong_type(self): + self.mixer.get_volume.return_value.get.return_value = '12' + self.assertEqual(self.core.mixer.get_volume(), None) + + def test_set_volume_exception(self): + self.mixer.set_volume.return_value.get.side_effect = Exception + self.assertFalse(self.core.mixer.set_volume(30)) + + def test_set_volume_non_bool_return_value(self): + self.mixer.set_volume.return_value.get.return_value = 'done' + self.assertIs(self.core.mixer.set_volume(30), True) + + def test_get_mute_raises_exception(self): + self.mixer.get_mute.return_value.get.side_effect = Exception + self.assertEqual(self.core.mixer.get_mute(), None) + + def test_get_mute_returns_wrong_type(self): + self.mixer.get_mute.return_value.get.return_value = '12' + self.assertEqual(self.core.mixer.get_mute(), None) + + def test_set_mute_exception(self): + self.mixer.set_mute.return_value.get.side_effect = Exception + self.assertFalse(self.core.mixer.set_mute(True)) + + def test_set_mute_non_bool_return_value(self): + self.mixer.set_mute.return_value.get.return_value = 'done' + self.assertIs(self.core.mixer.set_mute(True), True) From e7b241e18b74ad4984b372b6ae1d6fdb91f9361e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 00:28:58 +0200 Subject: [PATCH 17/42] core: Update playlists to handle bad data from backends and exceptions --- mopidy/core/playlists.py | 95 +++++++++++++++++++++++++++--------- tests/core/test_playlists.py | 82 +++++++++++++++++++++++++++++-- 2 files changed, 150 insertions(+), 27 deletions(-) diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index aa5befaf..d14f2fcd 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -1,15 +1,31 @@ from __future__ import absolute_import, unicode_literals +import contextlib import logging import urlparse +from mopidy import exceptions from mopidy.core import listener -from mopidy.models import Playlist +from mopidy.models import Playlist, Ref from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) +@contextlib.contextmanager +def _backend_error_handling(backend, reraise=None): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s backend returned bad data: %s', + backend.actor_ref.actor_class.__name__, e) + except Exception as e: + if reraise and isinstance(e, reraise): + raise + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) + + class PlaylistsController(object): pykka_traversable = True @@ -36,15 +52,16 @@ class PlaylistsController(object): results = [] for backend, future in futures.items(): try: - results.extend(future.get()) + with _backend_error_handling(backend, NotImplementedError): + playlists = future.get() + if playlists is not None: + validation.check_instances(playlists, Ref) + results.extend(playlists) except NotImplementedError: backend_name = backend.actor_ref.actor_class.__name__ logger.warning( '%s does not implement playlists.as_list(). ' 'Please upgrade it.', backend_name) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) return results @@ -66,8 +83,16 @@ class PlaylistsController(object): uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: - return backend.playlists.get_items(uri).get() + + if not backend: + return None + + with _backend_error_handling(backend): + items = backend.playlists.get_items(uri).get() + items is None or validation.check_instances(items, Ref) + return items + + return None def get_playlists(self, include_tracks=True): """ @@ -85,7 +110,7 @@ class PlaylistsController(object): """ deprecation.warn('core.playlists.get_playlists') - playlist_refs = self.as_list() + playlist_refs = self.as_list() or [] if include_tracks: playlists = {r.uri: self.lookup(r.uri) for r in playlist_refs} @@ -125,11 +150,20 @@ class PlaylistsController(object): if uri_scheme in self.backends.with_playlists: backend = self.backends.with_playlists[uri_scheme] else: - # TODO: this fallback looks suspicious + # TODO: loop over backends until one of them doesn't return None backend = list(self.backends.with_playlists.values())[0] - playlist = backend.playlists.create(name).get() - listener.CoreListener.send('playlist_changed', playlist=playlist) - return playlist + + with _backend_error_handling(backend): + playlist = backend.playlists.create(name).get() + + if playlist is None: + return None + + validation.check_instance(playlist, Playlist) + listener.CoreListener.send('playlist_changed', playlist=playlist) + return playlist + + return None def delete(self, uri): """ @@ -145,8 +179,14 @@ class PlaylistsController(object): uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: + if not backend: + return + + with _backend_error_handling(backend): backend.playlists.delete(uri).get() + # TODO: emit playlist changed? + + # TODO: return value? def filter(self, criteria=None, **kwargs): """ @@ -192,11 +232,16 @@ class PlaylistsController(object): """ uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: - return backend.playlists.lookup(uri).get() - else: + if not backend: return None + with _backend_error_handling(backend): + playlist = backend.playlists.lookup(uri).get() + playlist is None or validation.check_instance(playlist, Playlist) + return playlist + + return None + # TODO: there is an inconsistency between library.refresh(uri) and this # call, not sure how to sort this out. def refresh(self, uri_scheme=None): @@ -225,12 +270,9 @@ class PlaylistsController(object): futures[backend] = backend.playlists.refresh() for backend, future in futures.items(): - try: + with _backend_error_handling(backend): future.get() playlists_loaded = True - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) if playlists_loaded: listener.CoreListener.send('playlists_loaded') @@ -264,7 +306,16 @@ class PlaylistsController(object): uri_scheme = urlparse.urlparse(playlist.uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) - if backend: + if not backend: + return None + + # TODO: we let AssertionError error through due to legacy tests :/ + with _backend_error_handling(backend, AssertionError): playlist = backend.playlists.save(playlist).get() - listener.CoreListener.send('playlist_changed', playlist=playlist) + playlist is None or validation.check_instance(playlist, Playlist) + if playlist: + listener.CoreListener.send( + 'playlist_changed', playlist=playlist) return playlist + + return None diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index febff62b..0f73ac14 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -295,15 +295,69 @@ class BackendFailuresCorePlaylistsTest(unittest.TestCase): self.core = core.Core(mixer=None, backends=[self.backend]) def test_as_list_backend_exception_gets_ignored(self, logger): - self.playlists.as_list.get.side_effect = Exception + self.playlists.as_list.return_value.get.side_effect = Exception self.assertEqual([], self.core.playlists.as_list()) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_items_backend_exception_gets_through(self, logger): - # TODO: is this behavior desired? + def test_as_list_backend_returns_none(self, logger): + self.playlists.as_list.return_value.get.return_value = None + self.assertEqual([], self.core.playlists.as_list()) + self.assertFalse(logger.error.called) + + def test_as_list_backend_bad_value(self, logger): + self.playlists.as_list.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.playlists.as_list()) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_get_items_backend_exception_gets_caught(self, logger): self.playlists.get_items.return_value.get.side_effect = Exception - with self.assertRaises(Exception): - self.core.playlists.get_items('dummy:/1') + self.assertIsNone(self.core.playlists.get_items('dummy:/1')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_get_items_backend_returns_none(self, logger): + self.playlists.get_items.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.get_items('dummy:/1')) + self.assertFalse(logger.error.called) + + def test_get_items_backend_returns_bad_value(self, logger): + self.playlists.get_items.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.get_items('dummy:/1')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_create_backend_exception_gets_caught(self, logger): + self.playlists.create.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.create('foobar')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_create_backend_returns_none(self, logger): + self.playlists.create.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.create('foobar')) + self.assertFalse(logger.error.called) + + def test_create_backend_returns_bad_value(self, logger): + self.playlists.create.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.create('foobar')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_delete_backend_exception_gets_caught(self, logger): + self.playlists.delete.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.delete('dummy:/1')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_lookup_backend_exception_gets_caught(self, logger): + self.playlists.lookup.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.lookup('dummy:/1')) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_lookup_backend_returns_none(self, logger): + self.playlists.lookup.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.lookup('dummy:/1')) + self.assertFalse(logger.error.called) + + def test_lookup_backend_returns_bad_value(self, logger): + self.playlists.lookup.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.lookup('dummy:/1')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) @mock.patch('mopidy.core.listener.CoreListener.send') def test_refresh_backend_exception_gets_ignored(self, send, logger): @@ -318,3 +372,21 @@ class BackendFailuresCorePlaylistsTest(unittest.TestCase): self.core.playlists.refresh('dummy') self.assertFalse(send.called) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_save_backend_exception_gets_caught(self, logger): + playlist = Playlist(uri='dummy:/1') + self.playlists.save.return_value.get.side_effect = Exception + self.assertIsNone(self.core.playlists.save(playlist)) + logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + + def test_save_backend_returns_none(self, logger): + playlist = Playlist(uri='dummy:/1') + self.playlists.save.return_value.get.return_value = None + self.assertIsNone(self.core.playlists.save(playlist)) + self.assertFalse(logger.error.called) + + def test_save_backend_returns_bad_value(self, logger): + playlist = Playlist(uri='dummy:/1') + self.playlists.save.return_value.get.return_value = 'abc' + self.assertIsNone(self.core.playlists.save(playlist)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) From 4aa984207b1ccef3b1163cace009b582dea2ba42 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 01:01:12 +0200 Subject: [PATCH 18/42] tests: Split up core bad backend tests and unify naming --- tests/core/test_library.py | 137 ++++++++++++++++++++--------------- tests/core/test_mixer.py | 34 ++++++--- tests/core/test_playlists.py | 67 ++++++++++++----- 3 files changed, 150 insertions(+), 88 deletions(-) diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 527e5272..525425ca 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -429,8 +429,7 @@ class LegacyFindExactToSearchLibraryTest(unittest.TestCase): # We are just testing that this doesn't fail. -@mock.patch('mopidy.core.library.logger') -class BackendFailuresCoreLibraryTest(unittest.TestCase): +class MockBackendCoreLibraryBase(unittest.TestCase): def setUp(self): # noqa: N802 dummy_root = Ref.directory(uri='dummy:directory', name='dummy') @@ -445,158 +444,182 @@ class BackendFailuresCoreLibraryTest(unittest.TestCase): self.core = core.Core(mixer=None, backends=[self.backend]) - def test_browse_backend_get_root_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class BrowseBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception_for_root(self, logger): # Might happen if root_directory is a property for some weird reason. self.library.root_directory.get.side_effect = Exception self.assertEqual([], self.core.library.browse(None)) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_browse_backend_get_root_bad_value(self, logger): - self.library.root_directory.get.return_value = 123 - self.assertEqual([], self.core.library.browse(None)) - logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - - def test_browse_backend_get_root_none(self, logger): + def test_backend_returns_none_for_root(self, logger): self.library.root_directory.get.return_value = None self.assertEqual([], self.core.library.browse(None)) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_browse_backend_browse_uri_exception_gets_ignored(self, logger): + def test_backend_returns_wrong_type_for_root(self, logger): + self.library.root_directory.get.return_value = 123 + self.assertEqual([], self.core.library.browse(None)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_raises_exception_for_browse(self, logger): self.library.browse.return_value.get.side_effect = Exception self.assertEqual([], self.core.library.browse('dummy:directory')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_browse_backend_browse_uri_bad_value(self, logger): + def test_backend_returns_wrong_type_for_browse(self, logger): self.library.browse.return_value.get.return_value = [123] self.assertEqual([], self.core.library.browse('dummy:directory')) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_get_distinct_backend_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class GetDistinctBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): self.library.get_distinct.return_value.get.side_effect = Exception self.assertEqual(set(), self.core.library.get_distinct('artist')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_distinct_backend_returns_string(self, logger): - self.library.get_distinct.return_value.get.return_value = 'abc' - self.assertEqual(set(), self.core.library.get_distinct('artist')) - logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - - def test_get_distinct_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): self.library.get_distinct.return_value.get.return_value = None self.assertEqual(set(), self.core.library.get_distinct('artist')) self.assertFalse(logger.error.called) - def test_get_distinct_backend_returns_list_if_ints(self, logger): + def test_backend_returns_wrong_type(self, logger): + self.library.get_distinct.return_value.get.return_value = 'abc' + self.assertEqual(set(), self.core.library.get_distinct('artist')) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_iterable_containing_wrong_types(self, logger): self.library.get_distinct.return_value.get.return_value = [1, 2, 3] self.assertEqual(set(), self.core.library.get_distinct('artist')) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_get_images_backend_exception_get_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class GetImagesBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): uri = 'dummy:/1' self.library.get_images.return_value.get.side_effect = Exception self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_images_backend_returns_string(self, logger): - uri = 'dummy:/1' - self.library.get_images.return_value.get.return_value = 'abc' - self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) - logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - - def test_get_images_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): uri = 'dummy:/1' self.library.get_images.return_value.get.return_value = None self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) self.assertFalse(logger.error.called) - def test_get_images_backend_returns_bad_dict(self, logger): + def test_backend_returns_wrong_type(self, logger): + uri = 'dummy:/1' + self.library.get_images.return_value.get.return_value = 'abc' + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_mapping_containing_wrong_types(self, logger): uri = 'dummy:/1' self.library.get_images.return_value.get.return_value = {uri: 'abc'} self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_get_images_backend_returns_dict_with_none(self, logger): + def test_backend_returns_mapping_containing_none(self, logger): uri = 'dummy:/1' self.library.get_images.return_value.get.return_value = {uri: None} self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_get_images_backend_returns_wrong_uri(self, logger): + def test_backend_returns_unknown_uri(self, logger): uri = 'dummy:/1' self.library.get_images.return_value.get.return_value = {'foo': []} self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.warning.assert_called_with(mock.ANY, 'DummyBackend', 'foo') - def test_lookup_backend_exceptions_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class LookupByUrisBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): uri = 'dummy:/1' self.library.lookup.return_value.get.side_effect = Exception self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_lookup_uris_backend_returns_string(self, logger): - uri = 'dummy:/1' - self.library.lookup.return_value.get.return_value = 'abc' - self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) - logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - - def test_lookup_uris_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): uri = 'dummy:/1' self.library.lookup.return_value.get.return_value = None self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) self.assertFalse(logger.error.called) - def test_lookup_uris_backend_returns_bad_list(self, logger): + def test_backend_returns_wrong_type(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = 'abc' + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_iterable_containing_wrong_types(self, logger): uri = 'dummy:/1' self.library.lookup.return_value.get.return_value = [123] self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_lookup_uri_backend_returns_string(self, logger): - uri = 'dummy:/1' - self.library.lookup.return_value.get.return_value = 'abc' - self.assertEqual([], self.core.library.lookup(uri)) - logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - - def test_lookup_uri_backend_returns_none(self, logger): + def test_backend_returns_none_with_uri(self, logger): uri = 'dummy:/1' self.library.lookup.return_value.get.return_value = None self.assertEqual([], self.core.library.lookup(uri)) self.assertFalse(logger.error.called) - def test_lookup_uri_backend_returns_bad_list(self, logger): + def test_backend_returns_wrong_type_with_uri(self, logger): + uri = 'dummy:/1' + self.library.lookup.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.library.lookup(uri)) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + + def test_backend_returns_iterable_wrong_types_with_uri(self, logger): uri = 'dummy:/1' self.library.lookup.return_value.get.return_value = [123] self.assertEqual([], self.core.library.lookup(uri)) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_refresh_backend_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class RefreshBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): self.library.refresh.return_value.get.side_effect = Exception self.core.library.refresh() logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_refresh_uri_backend_exception_gets_ignored(self, logger): + def test_backend_raises_exception_with_uri(self, logger): self.library.refresh.return_value.get.side_effect = Exception self.core.library.refresh('dummy:/1') logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_search_backend_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.library.logger') +class SearchBadBackendTest(MockBackendCoreLibraryBase): + + def test_backend_raises_exception(self, logger): self.library.search.return_value.get.side_effect = Exception self.assertEqual([], self.core.library.search(query={'any': ['foo']})) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_search_backend_lookup_error_gets_through(self, logger): + def test_backend_raises_lookuperror(self, logger): # TODO: is this behavior desired? Do we need to continue handling # LookupError case specially. self.library.search.return_value.get.side_effect = LookupError with self.assertRaises(LookupError): self.core.library.search(query={'any': ['foo']}) - def test_search_backend_returns_string(self, logger): - self.library.search.return_value.get.return_value = 'abc' - self.assertEqual([], self.core.library.search(query={'any': ['foo']})) - logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - - def test_search_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): self.library.search.return_value.get.return_value = None self.assertEqual([], self.core.library.search(query={'any': ['foo']})) self.assertFalse(logger.error.called) + + def test_backend_returns_wrong_type(self, logger): + self.library.search.return_value.get.return_value = 'abc' + self.assertEqual([], self.core.library.search(query={'any': ['foo']})) + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) diff --git a/tests/core/test_mixer.py b/tests/core/test_mixer.py index 61e054d0..5444cae6 100644 --- a/tests/core/test_mixer.py +++ b/tests/core/test_mixer.py @@ -94,49 +94,61 @@ class CoreNoneMixerListenerTest(unittest.TestCase): self.assertEqual(send.call_count, 0) -class CoreBadMixerTest(unittest.TestCase): +class MockBackendCoreMixerBase(unittest.TestCase): def setUp(self): # noqa: N802 self.mixer = mock.Mock() self.mixer.actor_ref.actor_class.__name__ = 'DummyMixer' self.core = core.Core(mixer=self.mixer, backends=[]) - def test_get_volume_raises_exception(self): + +class GetVolumeBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): self.mixer.get_volume.return_value.get.side_effect = Exception self.assertEqual(self.core.mixer.get_volume(), None) - def test_get_volume_returns_negative(self): + def test_backend_returns_too_small_value(self): self.mixer.get_volume.return_value.get.return_value = -1 self.assertEqual(self.core.mixer.get_volume(), None) - def test_get_volume_returns_out_of_bound(self): + def test_backend_returns_too_large_value(self): self.mixer.get_volume.return_value.get.return_value = 1000 self.assertEqual(self.core.mixer.get_volume(), None) - def test_get_volume_returns_wrong_type(self): + def test_backend_returns_wrong_type(self): self.mixer.get_volume.return_value.get.return_value = '12' self.assertEqual(self.core.mixer.get_volume(), None) - def test_set_volume_exception(self): + +class SetVolumeBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): self.mixer.set_volume.return_value.get.side_effect = Exception self.assertFalse(self.core.mixer.set_volume(30)) - def test_set_volume_non_bool_return_value(self): + def test_backend_returns_wrong_type(self): self.mixer.set_volume.return_value.get.return_value = 'done' self.assertIs(self.core.mixer.set_volume(30), True) - def test_get_mute_raises_exception(self): + +class GetMuteBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): self.mixer.get_mute.return_value.get.side_effect = Exception self.assertEqual(self.core.mixer.get_mute(), None) - def test_get_mute_returns_wrong_type(self): + def test_backend_returns_wrong_type(self): self.mixer.get_mute.return_value.get.return_value = '12' self.assertEqual(self.core.mixer.get_mute(), None) - def test_set_mute_exception(self): + +class SetMuteBadBackendTest(MockBackendCoreMixerBase): + + def test_backend_raises_exception(self): self.mixer.set_mute.return_value.get.side_effect = Exception self.assertFalse(self.core.mixer.set_mute(True)) - def test_set_mute_non_bool_return_value(self): + def test_backend_returns_wrong_type(self): self.mixer.set_mute.return_value.get.return_value = 'done' self.assertIs(self.core.mixer.set_mute(True), True) diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 0f73ac14..d4e075ad 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -281,8 +281,7 @@ class DeprecatedGetPlaylistsTest(BasePlaylistsTest): self.assertEqual(len(result[1].tracks), 0) -@mock.patch('mopidy.core.playlists.logger') -class BackendFailuresCorePlaylistsTest(unittest.TestCase): +class MockBackendCorePlaylistsBase(unittest.TestCase): def setUp(self): # noqa: N802 self.playlists = mock.Mock(spec=backend.PlaylistsProvider) @@ -294,98 +293,126 @@ class BackendFailuresCorePlaylistsTest(unittest.TestCase): self.core = core.Core(mixer=None, backends=[self.backend]) - def test_as_list_backend_exception_gets_ignored(self, logger): + +@mock.patch('mopidy.core.playlists.logger') +class AsListBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): self.playlists.as_list.return_value.get.side_effect = Exception self.assertEqual([], self.core.playlists.as_list()) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_as_list_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): self.playlists.as_list.return_value.get.return_value = None self.assertEqual([], self.core.playlists.as_list()) self.assertFalse(logger.error.called) - def test_as_list_backend_bad_value(self, logger): + def test_backend_returns_wrong_type(self, logger): self.playlists.as_list.return_value.get.return_value = 'abc' self.assertEqual([], self.core.playlists.as_list()) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_get_items_backend_exception_gets_caught(self, logger): + +@mock.patch('mopidy.core.playlists.logger') +class GetItemsBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): self.playlists.get_items.return_value.get.side_effect = Exception self.assertIsNone(self.core.playlists.get_items('dummy:/1')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_get_items_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): self.playlists.get_items.return_value.get.return_value = None self.assertIsNone(self.core.playlists.get_items('dummy:/1')) self.assertFalse(logger.error.called) - def test_get_items_backend_returns_bad_value(self, logger): + def test_backend_returns_wrong_type(self, logger): self.playlists.get_items.return_value.get.return_value = 'abc' self.assertIsNone(self.core.playlists.get_items('dummy:/1')) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_create_backend_exception_gets_caught(self, logger): + +@mock.patch('mopidy.core.playlists.logger') +class CreateBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): self.playlists.create.return_value.get.side_effect = Exception self.assertIsNone(self.core.playlists.create('foobar')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_create_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): self.playlists.create.return_value.get.return_value = None self.assertIsNone(self.core.playlists.create('foobar')) self.assertFalse(logger.error.called) - def test_create_backend_returns_bad_value(self, logger): + def test_backend_returns_wrong_type(self, logger): self.playlists.create.return_value.get.return_value = 'abc' self.assertIsNone(self.core.playlists.create('foobar')) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) - def test_delete_backend_exception_gets_caught(self, logger): + +@mock.patch('mopidy.core.playlists.logger') +class DeleteBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): self.playlists.delete.return_value.get.side_effect = Exception self.assertIsNone(self.core.playlists.delete('dummy:/1')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_lookup_backend_exception_gets_caught(self, logger): + +@mock.patch('mopidy.core.playlists.logger') +class LookupBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): self.playlists.lookup.return_value.get.side_effect = Exception self.assertIsNone(self.core.playlists.lookup('dummy:/1')) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_lookup_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): self.playlists.lookup.return_value.get.return_value = None self.assertIsNone(self.core.playlists.lookup('dummy:/1')) self.assertFalse(logger.error.called) - def test_lookup_backend_returns_bad_value(self, logger): + def test_backend_returns_wrong_type(self, logger): self.playlists.lookup.return_value.get.return_value = 'abc' self.assertIsNone(self.core.playlists.lookup('dummy:/1')) logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) + +@mock.patch('mopidy.core.playlists.logger') +class RefreshBadBackendsTest(MockBackendCorePlaylistsBase): + @mock.patch('mopidy.core.listener.CoreListener.send') - def test_refresh_backend_exception_gets_ignored(self, send, logger): + def test_backend_raises_exception(self, send, logger): self.playlists.refresh.return_value.get.side_effect = Exception self.core.playlists.refresh() self.assertFalse(send.called) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') @mock.patch('mopidy.core.listener.CoreListener.send') - def test_refresh_uri_backend_exception_gets_ignored(self, send, logger): + def test_backend_raises_exception_called_with_uri(self, send, logger): self.playlists.refresh.return_value.get.side_effect = Exception self.core.playlists.refresh('dummy') self.assertFalse(send.called) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_save_backend_exception_gets_caught(self, logger): + +@mock.patch('mopidy.core.playlists.logger') +class SaveBadBackendsTest(MockBackendCorePlaylistsBase): + + def test_backend_raises_exception(self, logger): playlist = Playlist(uri='dummy:/1') self.playlists.save.return_value.get.side_effect = Exception self.assertIsNone(self.core.playlists.save(playlist)) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_save_backend_returns_none(self, logger): + def test_backend_returns_none(self, logger): playlist = Playlist(uri='dummy:/1') self.playlists.save.return_value.get.return_value = None self.assertIsNone(self.core.playlists.save(playlist)) self.assertFalse(logger.error.called) - def test_save_backend_returns_bad_value(self, logger): + def test_backend_returns_wrong_type(self, logger): playlist = Playlist(uri='dummy:/1') self.playlists.save.return_value.get.return_value = 'abc' self.assertIsNone(self.core.playlists.save(playlist)) From 9f64a8719aae6e04876d80d3d0d32a04f82913f9 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 01:02:49 +0200 Subject: [PATCH 19/42] docs: Add core not trusting backends entry to changelog --- docs/changelog.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 63f432ba..b410fd39 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -27,6 +27,8 @@ Core API ``tl_track`` versions of the calls. (Fixes: :issue:`1131` PR: :issue:`1136`, :issue:`1140`) +- Update core to handle backend crashes and bad data. (Fixes: :issue:`1161`) + Models ------ From 636639a201bd9cfea36af53e7e4ec2c72d2d479f Mon Sep 17 00:00:00 2001 From: Thomas Kemmer Date: Wed, 6 May 2015 14:50:21 +0200 Subject: [PATCH 20/42] Fix #1162: Ignore None results and exceptions from PlaylistsProvider.create(). --- docs/changelog.rst | 9 +++++++++ mopidy/core/playlists.py | 18 ++++++++++++------ tests/core/test_playlists.py | 26 ++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 10c413ec..00cee7b8 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -5,6 +5,15 @@ Changelog This changelog is used to track all major changes to Mopidy. +v1.0.5 (UNRELEASED) +=================== + +Bug fix release. + +- Core: Add workaround for playlist providers that do not support + creating playlists. (Fixes: :issue:`1162`, PR :issue:`1165`) + + v1.0.4 (2015-04-30) =================== diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index 669e1f35..1b4c2692 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -118,13 +118,19 @@ class PlaylistsController(object): :rtype: :class:`mopidy.models.Playlist` """ if uri_scheme in self.backends.with_playlists: - backend = self.backends.with_playlists[uri_scheme] + backends = [self.backends.with_playlists[uri_scheme]] else: - # TODO: this fallback looks suspicious - backend = list(self.backends.with_playlists.values())[0] - playlist = backend.playlists.create(name).get() - listener.CoreListener.send('playlist_changed', playlist=playlist) - return playlist + backends = self.backends.with_playlists.values() + for backend in backends: + try: + playlist = backend.playlists.create(name).get() + except Exception: + playlist = None + # Workaround for playlist providers that return None from create() + if not playlist: + continue + listener.CoreListener.send('playlist_changed', playlist=playlist) + return playlist def delete(self, uri): """ diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 081f73e6..e02f6204 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -118,6 +118,32 @@ class PlaylistsTest(unittest.TestCase): self.sp1.create.assert_called_once_with('foo') self.assertFalse(self.sp2.create.called) + def test_create_without_uri_scheme_ignores_none_result(self): + playlist = Playlist() + self.sp1.create().get.return_value = None + self.sp1.reset_mock() + self.sp2.create().get.return_value = playlist + self.sp2.reset_mock() + + result = self.core.playlists.create('foo') + + self.assertEqual(playlist, result) + self.sp1.create.assert_called_once_with('foo') + self.sp2.create.assert_called_once_with('foo') + + def test_create_without_uri_scheme_ignores_exception(self): + playlist = Playlist() + self.sp1.create().get.side_effect = Exception + self.sp1.reset_mock() + self.sp2.create().get.return_value = playlist + self.sp2.reset_mock() + + result = self.core.playlists.create('foo') + + self.assertEqual(playlist, result) + self.sp1.create.assert_called_once_with('foo') + self.sp2.create.assert_called_once_with('foo') + def test_create_with_uri_scheme_selects_the_matching_backend(self): playlist = Playlist() self.sp2.create().get.return_value = playlist From 6d82cdb611902bfae7ddbeec39992cc2b5534670 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Wed, 6 May 2015 21:06:30 +0200 Subject: [PATCH 21/42] tests: Cleanup reset_mock() usage --- tests/core/test_events.py | 13 ----------- tests/core/test_library.py | 42 ++++++++++++------------------------ tests/core/test_playback.py | 6 ++---- tests/core/test_playlists.py | 27 ++++++++--------------- tests/http/test_events.py | 4 ---- 5 files changed, 25 insertions(+), 67 deletions(-) diff --git a/tests/core/test_events.py b/tests/core/test_events.py index 157acffd..b6cd25b9 100644 --- a/tests/core/test_events.py +++ b/tests/core/test_events.py @@ -45,15 +45,12 @@ class BackendEventsTest(unittest.TestCase): self.assertEqual(send.call_args[1]['mute'], True) def test_tracklist_add_sends_tracklist_changed_event(self, send): - send.reset_mock() - self.core.tracklist.add(uris=['dummy:a']).get() self.assertEqual(send.call_args[0][0], 'tracklist_changed') def test_tracklist_clear_sends_tracklist_changed_event(self, send): self.core.tracklist.add(uris=['dummy:a']).get() - send.reset_mock() self.core.tracklist.clear().get() @@ -61,7 +58,6 @@ class BackendEventsTest(unittest.TestCase): def test_tracklist_move_sends_tracklist_changed_event(self, send): self.core.tracklist.add(uris=['dummy:a', 'dummy:b']).get() - send.reset_mock() self.core.tracklist.move(0, 1, 1).get() @@ -69,7 +65,6 @@ class BackendEventsTest(unittest.TestCase): def test_tracklist_remove_sends_tracklist_changed_event(self, send): self.core.tracklist.add(uris=['dummy:a']).get() - send.reset_mock() self.core.tracklist.remove({'uri': ['dummy:a']}).get() @@ -77,29 +72,22 @@ class BackendEventsTest(unittest.TestCase): def test_tracklist_shuffle_sends_tracklist_changed_event(self, send): self.core.tracklist.add(uris=['dummy:a', 'dummy:b']).get() - send.reset_mock() self.core.tracklist.shuffle().get() self.assertEqual(send.call_args[0][0], 'tracklist_changed') def test_playlists_refresh_sends_playlists_loaded_event(self, send): - send.reset_mock() - self.core.playlists.refresh().get() self.assertEqual(send.call_args[0][0], 'playlists_loaded') def test_playlists_refresh_uri_sends_playlists_loaded_event(self, send): - send.reset_mock() - self.core.playlists.refresh(uri_scheme='dummy').get() self.assertEqual(send.call_args[0][0], 'playlists_loaded') def test_playlists_create_sends_playlist_changed_event(self, send): - send.reset_mock() - self.core.playlists.create('foo').get() self.assertEqual(send.call_args[0][0], 'playlist_changed') @@ -112,7 +100,6 @@ class BackendEventsTest(unittest.TestCase): def test_playlists_save_sends_playlist_changed_event(self, send): playlist = self.core.playlists.create('foo').get() playlist = playlist.replace(name='bar') - send.reset_mock() self.core.playlists.save(playlist).get() diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 89f3b284..4a8937c3 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -16,8 +16,7 @@ class BaseCoreLibraryTest(unittest.TestCase): self.backend1 = mock.Mock() self.backend1.uri_schemes.get.return_value = ['dummy1'] self.library1 = mock.Mock(spec=backend.LibraryProvider) - self.library1.get_images().get.return_value = {} - self.library1.get_images.reset_mock() + self.library1.get_images.return_value.get.return_value = {} self.library1.root_directory.get.return_value = dummy1_root self.backend1.library = self.library1 @@ -25,8 +24,7 @@ class BaseCoreLibraryTest(unittest.TestCase): self.backend2 = mock.Mock() self.backend2.uri_schemes.get.return_value = ['dummy2', 'du2'] self.library2 = mock.Mock(spec=backend.LibraryProvider) - self.library2.get_images().get.return_value = {} - self.library2.get_images.reset_mock() + self.library2.get_images.return_value.get.return_value = {} self.library2.root_directory.get.return_value = dummy2_root self.backend2.library = self.library2 @@ -65,20 +63,17 @@ class CoreLibraryTest(BaseCoreLibraryTest): self.library2.get_images.assert_called_once_with(['dummy2:track']) def test_get_images_returns_images(self): - self.library1.get_images().get.return_value = { + self.library1.get_images.return_value.get.return_value = { 'dummy1:track': [Image(uri='uri')]} - self.library1.get_images.reset_mock() result = self.core.library.get_images(['dummy1:track']) self.assertEqual({'dummy1:track': (Image(uri='uri'),)}, result) def test_get_images_merges_results(self): - self.library1.get_images().get.return_value = { + self.library1.get_images.return_value.get.return_value = { 'dummy1:track': [Image(uri='uri1')]} - self.library1.get_images.reset_mock() - self.library2.get_images().get.return_value = { + self.library2.get_images.return_value.get.return_value = { 'dummy2:track': [Image(uri='uri2')]} - self.library2.get_images.reset_mock() result = self.core.library.get_images( ['dummy1:track', 'dummy2:track', 'dummy3:track', 'dummy4:track']) @@ -106,11 +101,10 @@ class CoreLibraryTest(BaseCoreLibraryTest): self.assertFalse(self.library2.browse.called) def test_browse_dummy1_selects_dummy1_backend(self): - self.library1.browse().get.return_value = [ + self.library1.browse.return_value.get.return_value = [ Ref.directory(uri='dummy1:directory:/foo/bar', name='bar'), Ref.track(uri='dummy1:track:/foo/baz.mp3', name='Baz'), ] - self.library1.browse.reset_mock() self.core.library.browse('dummy1:directory:/foo') @@ -119,11 +113,10 @@ class CoreLibraryTest(BaseCoreLibraryTest): self.library1.browse.assert_called_with('dummy1:directory:/foo') def test_browse_dummy2_selects_dummy2_backend(self): - self.library2.browse().get.return_value = [ + self.library2.browse.return_value.get.return_value = [ Ref.directory(uri='dummy2:directory:/bar/baz', name='quux'), Ref.track(uri='dummy2:track:/bar/foo.mp3', name='Baz'), ] - self.library2.browse.reset_mock() self.core.library.browse('dummy2:directory:/bar') @@ -139,11 +132,10 @@ class CoreLibraryTest(BaseCoreLibraryTest): self.assertEqual(self.library2.browse.call_count, 0) def test_browse_dir_returns_subdirs_and_tracks(self): - self.library1.browse().get.return_value = [ + self.library1.browse.return_value.get.return_value = [ Ref.directory(uri='dummy1:directory:/foo/bar', name='Bar'), Ref.track(uri='dummy1:track:/foo/baz.mp3', name='Baz'), ] - self.library1.browse.reset_mock() result = self.core.library.browse('dummy1:directory:/foo') self.assertEqual(result, [ @@ -199,10 +191,8 @@ class CoreLibraryTest(BaseCoreLibraryTest): result1 = SearchResult(tracks=[track1]) result2 = SearchResult(tracks=[track2]) - self.library1.search().get.return_value = result1 - self.library1.search.reset_mock() - self.library2.search().get.return_value = result2 - self.library2.search.reset_mock() + self.library1.search.return_value.get.return_value = result1 + self.library2.search.return_value.get.return_value = result2 result = self.core.library.search({'any': ['a']}) @@ -234,10 +224,8 @@ class CoreLibraryTest(BaseCoreLibraryTest): track1 = Track(uri='dummy1:a') result1 = SearchResult(tracks=[track1]) - self.library1.search().get.return_value = result1 - self.library1.search.reset_mock() - self.library2.search().get.return_value = None - self.library2.search.reset_mock() + self.library1.search.return_value.get.return_value = result1 + self.library2.search.return_value.get.return_value = None result = self.core.library.search({'any': ['a']}) @@ -254,10 +242,8 @@ class CoreLibraryTest(BaseCoreLibraryTest): result1 = SearchResult(tracks=[track1]) result2 = SearchResult(tracks=[track2]) - self.library1.search().get.return_value = result1 - self.library1.search.reset_mock() - self.library2.search().get.return_value = result2 - self.library2.search.reset_mock() + self.library1.search.return_value.get.return_value = result1 + self.library2.search.return_value.get.return_value = result2 result = self.core.library.search({'any': ['a']}) diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 1837ac80..7896ed4e 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -21,15 +21,13 @@ class CorePlaybackTest(unittest.TestCase): self.backend1 = mock.Mock() self.backend1.uri_schemes.get.return_value = ['dummy1'] self.playback1 = mock.Mock(spec=backend.PlaybackProvider) - self.playback1.get_time_position().get.return_value = 1000 - self.playback1.reset_mock() + self.playback1.get_time_position.return_value.get.return_value = 1000 self.backend1.playback = self.playback1 self.backend2 = mock.Mock() self.backend2.uri_schemes.get.return_value = ['dummy2'] self.playback2 = mock.Mock(spec=backend.PlaybackProvider) - self.playback2.get_time_position().get.return_value = 2000 - self.playback2.reset_mock() + self.playback2.get_time_position.return_value.get.return_value = 2000 self.backend2.playback = self.playback2 # A backend without the optional playback provider diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 9d52efd4..063b4057 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -90,8 +90,7 @@ class PlaylistTest(BasePlaylistsTest): def test_create_without_uri_scheme_uses_first_backend(self): playlist = Playlist() - self.sp1.create().get.return_value = playlist - self.sp1.reset_mock() + self.sp1.create.return_value.get.return_value = playlist result = self.core.playlists.create('foo') @@ -101,10 +100,8 @@ class PlaylistTest(BasePlaylistsTest): def test_create_without_uri_scheme_ignores_none_result(self): playlist = Playlist() - self.sp1.create().get.return_value = None - self.sp1.reset_mock() - self.sp2.create().get.return_value = playlist - self.sp2.reset_mock() + self.sp1.create.return_value.get.return_value = None + self.sp2.create.return_value.get.return_value = playlist result = self.core.playlists.create('foo') @@ -114,10 +111,8 @@ class PlaylistTest(BasePlaylistsTest): def test_create_without_uri_scheme_ignores_exception(self): playlist = Playlist() - self.sp1.create().get.side_effect = Exception - self.sp1.reset_mock() - self.sp2.create().get.return_value = playlist - self.sp2.reset_mock() + self.sp1.create.return_value.get.side_effect = Exception + self.sp2.create.return_value.get.return_value = playlist result = self.core.playlists.create('foo') @@ -127,8 +122,7 @@ class PlaylistTest(BasePlaylistsTest): def test_create_with_uri_scheme_selects_the_matching_backend(self): playlist = Playlist() - self.sp2.create().get.return_value = playlist - self.sp2.reset_mock() + self.sp2.create.return_value.get.return_value = playlist result = self.core.playlists.create('foo', uri_scheme='dummy2') @@ -138,8 +132,7 @@ class PlaylistTest(BasePlaylistsTest): def test_create_with_unsupported_uri_scheme_uses_first_backend(self): playlist = Playlist() - self.sp1.create().get.return_value = playlist - self.sp1.reset_mock() + self.sp1.create.return_value.get.return_value = playlist result = self.core.playlists.create('foo', uri_scheme='dummy3') @@ -216,8 +209,7 @@ class PlaylistTest(BasePlaylistsTest): def test_save_selects_the_dummy1_backend(self): playlist = Playlist(uri='dummy1:a') - self.sp1.save().get.return_value = playlist - self.sp1.reset_mock() + self.sp1.save.return_value.get.return_value = playlist result = self.core.playlists.save(playlist) @@ -227,8 +219,7 @@ class PlaylistTest(BasePlaylistsTest): def test_save_selects_the_dummy2_backend(self): playlist = Playlist(uri='dummy2:a') - self.sp2.save().get.return_value = playlist - self.sp2.reset_mock() + self.sp2.save.return_value.get.return_value = playlist result = self.core.playlists.save(playlist) diff --git a/tests/http/test_events.py b/tests/http/test_events.py index 43d9db58..dd1760a3 100644 --- a/tests/http/test_events.py +++ b/tests/http/test_events.py @@ -12,8 +12,6 @@ from mopidy.http import actor class HttpEventsTest(unittest.TestCase): def test_track_playback_paused_is_broadcasted(self, broadcast): - broadcast.reset_mock() - actor.on_event('track_playback_paused', foo='bar') self.assertDictEqual( @@ -23,8 +21,6 @@ class HttpEventsTest(unittest.TestCase): }) def test_track_playback_resumed_is_broadcasted(self, broadcast): - broadcast.reset_mock() - actor.on_event('track_playback_resumed', foo='bar') self.assertDictEqual( From c01f8679bc6e1140c46e5b02e488e709c93d3a8e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 22:34:44 +0200 Subject: [PATCH 22/42] core: Address review comments for do not trust backends PR --- mopidy/core/library.py | 41 +++++++++++++++++--------------------- mopidy/core/mixer.py | 18 +++++++++-------- mopidy/core/playlists.py | 12 +++++------ tests/core/test_library.py | 2 +- tests/core/test_mixer.py | 6 ++++-- 5 files changed, 39 insertions(+), 40 deletions(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index 1ca21457..8e6e4015 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -14,13 +14,15 @@ logger = logging.getLogger(__name__) @contextlib.contextmanager -def _backend_error_handling(backend): +def _backend_error_handling(backend, reraise=None): try: yield except exceptions.ValidationError as e: logger.error('%s backend returned bad data: %s', backend.actor_ref.actor_class.__name__, e) - except Exception: + except Exception as e: + if reraise and isinstance(e, reraise): + raise logger.exception('%s backend caused an exception.', backend.actor_ref.actor_class.__name__) @@ -174,10 +176,8 @@ class LibraryController(object): validation.check_instance(future.get(), collections.Mapping) for uri, images in future.get().items(): if uri not in uris: - name = backend.actor_ref.actor_class.__name__ - logger.warning( - '%s backend returned image for URI we did not ' - 'ask for: %s', name, uri) + raise exceptions.ValidationError( + 'Got unknown image URI: %s' % uri) validation.check_instances(images, models.Image) results[uri] += tuple(images) return results @@ -330,31 +330,26 @@ class LibraryController(object): futures[backend] = backend.library.search( query=query, uris=backend_uris, exact=exact) + # Some of our tests check for LookupError to catch bad queries. This is + # silly and should be replaced with query validation before passing it + # to the backends. + reraise = (TypeError, LookupError) + results = [] for backend, future in futures.items(): - try: # TODO: fix all these cases so we can use common helper - result = future.get() - if result is not None: - validation.check_instance(result, models.SearchResult) - results.append(result) - except exceptions.ValidationError as e: - logger.error('%s backend returned bad data: %s', - backend.actor_ref.actor_class.__name__, e) + try: + with _backend_error_handling(backend, reraise=reraise): + result = future.get() + if result is not None: + validation.check_instance(result, models.SearchResult) + results.append(result) except TypeError: backend_name = backend.actor_ref.actor_class.__name__ logger.warning( '%s does not implement library.search() with "exact" ' 'support. Please upgrade it.', backend_name) - except LookupError: - # Some of our tests check for this to catch bad queries. This - # is silly and should be replaced with query validation before - # passing it to the backends. - raise - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) - return [r for r in results if r] + return results def _normalize_query(query): diff --git a/mopidy/core/mixer.py b/mopidy/core/mixer.py index 94188d50..d68cb842 100644 --- a/mopidy/core/mixer.py +++ b/mopidy/core/mixer.py @@ -7,6 +7,9 @@ from mopidy import exceptions from mopidy.utils import validation +logger = logging.getLogger(__name__) + + @contextlib.contextmanager def _mixer_error_handling(mixer): try: @@ -19,9 +22,6 @@ def _mixer_error_handling(mixer): mixer.actor_ref.actor_class.__name__) -logger = logging.getLogger(__name__) - - class MixerController(object): pykka_traversable = True @@ -60,10 +60,11 @@ class MixerController(object): return False with _mixer_error_handling(self._mixer): - # TODO: log non-bool return values? - return bool(self._mixer.set_volume(volume).get()) + result = self._mixer.set_volume(volume).get() + validation.check_instance(result, bool) + return result - return False + return None def get_mute(self): """Get mute state. @@ -93,7 +94,8 @@ class MixerController(object): return False with _mixer_error_handling(self._mixer): - # TODO: log non-bool return values? - return bool(self._mixer.set_mute(bool(mute)).get()) + result = self._mixer.set_mute(bool(mute)).get() + validation.check_instance(result, bool) + return result return None diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index d14f2fcd..af983872 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -50,15 +50,15 @@ class PlaylistsController(object): for backend in set(self.backends.with_playlists.values())} results = [] - for backend, future in futures.items(): + for b, future in futures.items(): try: - with _backend_error_handling(backend, NotImplementedError): + with _backend_error_handling(b, reraise=NotImplementedError): playlists = future.get() if playlists is not None: validation.check_instances(playlists, Ref) results.extend(playlists) except NotImplementedError: - backend_name = backend.actor_ref.actor_class.__name__ + backend_name = b.actor_ref.actor_class.__name__ logger.warning( '%s does not implement playlists.as_list(). ' 'Please upgrade it.', backend_name) @@ -110,7 +110,7 @@ class PlaylistsController(object): """ deprecation.warn('core.playlists.get_playlists') - playlist_refs = self.as_list() or [] + playlist_refs = self.as_list() if include_tracks: playlists = {r.uri: self.lookup(r.uri) for r in playlist_refs} @@ -145,7 +145,7 @@ class PlaylistsController(object): :type name: string :param uri_scheme: use the backend matching the URI scheme :type uri_scheme: string - :rtype: :class:`mopidy.models.Playlist` + :rtype: :class:`mopidy.models.Playlist` or :class:`None` """ if uri_scheme in self.backends.with_playlists: backend = self.backends.with_playlists[uri_scheme] @@ -310,7 +310,7 @@ class PlaylistsController(object): return None # TODO: we let AssertionError error through due to legacy tests :/ - with _backend_error_handling(backend, AssertionError): + with _backend_error_handling(backend, reraise=AssertionError): playlist = backend.playlists.save(playlist).get() playlist is None or validation.check_instance(playlist, Playlist) if playlist: diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 525425ca..c368baf2 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -536,7 +536,7 @@ class GetImagesBadBackendTest(MockBackendCoreLibraryBase): uri = 'dummy:/1' self.library.get_images.return_value.get.return_value = {'foo': []} self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) - logger.warning.assert_called_with(mock.ANY, 'DummyBackend', 'foo') + logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY) @mock.patch('mopidy.core.library.logger') diff --git a/tests/core/test_mixer.py b/tests/core/test_mixer.py index 5444cae6..40e13aea 100644 --- a/tests/core/test_mixer.py +++ b/tests/core/test_mixer.py @@ -23,6 +23,7 @@ class CoreMixerTest(unittest.TestCase): self.mixer.get_volume.assert_called_once_with() def test_set_volume(self): + self.mixer.set_volume.return_value.get.return_value = True self.core.mixer.set_volume(30) self.mixer.set_volume.assert_called_once_with(30) @@ -34,6 +35,7 @@ class CoreMixerTest(unittest.TestCase): self.mixer.get_mute.assert_called_once_with() def test_set_mute(self): + self.mixer.set_mute.return_value.get.return_value = True self.core.mixer.set_mute(True) self.mixer.set_mute.assert_called_once_with(True) @@ -129,7 +131,7 @@ class SetVolumeBadBackendTest(MockBackendCoreMixerBase): def test_backend_returns_wrong_type(self): self.mixer.set_volume.return_value.get.return_value = 'done' - self.assertIs(self.core.mixer.set_volume(30), True) + self.assertIs(self.core.mixer.set_volume(30), None) class GetMuteBadBackendTest(MockBackendCoreMixerBase): @@ -151,4 +153,4 @@ class SetMuteBadBackendTest(MockBackendCoreMixerBase): def test_backend_returns_wrong_type(self): self.mixer.set_mute.return_value.get.return_value = 'done' - self.assertIs(self.core.mixer.set_mute(True), True) + self.assertIs(self.core.mixer.set_mute(True), None) From 4d608dd431262eb16461ed65fdfc6015b7e9e339 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 23:00:53 +0200 Subject: [PATCH 23/42] core: Add get_current_tlid shortcut --- docs/changelog.rst | 3 +++ mopidy/core/playback.py | 16 +++++++++++++--- tests/core/test_playback.py | 11 +++++++++++ 3 files changed, 27 insertions(+), 3 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 2850a27a..0a19e834 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -27,6 +27,9 @@ Core API ``tl_track`` versions of the calls. (Fixes: :issue:`1131` PR: :issue:`1136`, :issue:`1140`) +- Add :meth:`mopidy.core.playback.PlaybackController.get_current_tlid`. + (Part of: :issue:`1137`) + Models ------ diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 3605db0f..5836ff68 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -61,9 +61,7 @@ class PlaybackController(object): Returns a :class:`mopidy.models.Track` or :class:`None`. """ - tl_track = self.get_current_tl_track() - if tl_track is not None: - return tl_track.track + return getattr(self.get_current_tl_track(), 'track', None) current_track = deprecation.deprecated_property(get_current_track) """ @@ -71,6 +69,18 @@ class PlaybackController(object): Use :meth:`get_current_track` instead. """ + def get_current_tlid(self): + """ + Get the currently playing or selected TLID. + + Extracted from :meth:`get_current_tl_track` for convenience. + + Returns a :class:`int` or :class:`None`. + + .. versionadded:: 1.1 + """ + return getattr(self.get_current_tl_track(), 'tlid', None) + def get_stream_title(self): """Get the current stream title or :class:`None`.""" return self._stream_title diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 7896ed4e..4b81dd8b 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -121,6 +121,17 @@ class CorePlaybackTest(unittest.TestCase): self.assertEqual( self.core.playback.get_current_track(), self.tracks[0]) + def test_get_current_tlid_none(self): + self.set_current_tl_track(None) + + self.assertEqual(self.core.playback.get_current_tlid(), None) + + def test_get_current_tlid_play(self): + self.core.playback.play(self.tl_tracks[0]) + + self.assertEqual( + self.core.playback.get_current_tlid(), self.tl_tracks[0].tlid) + # TODO Test state def test_play_selects_dummy1_backend(self): From 29c66f7bc8856c28bb1f3dae2a8c44a22e412ed7 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 7 May 2015 00:13:58 +0200 Subject: [PATCH 24/42] core: Correct volume/mute return values --- mopidy/core/mixer.py | 8 ++++---- tests/core/test_mixer.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mopidy/core/mixer.py b/mopidy/core/mixer.py index d68cb842..2815fb0c 100644 --- a/mopidy/core/mixer.py +++ b/mopidy/core/mixer.py @@ -57,14 +57,14 @@ class MixerController(object): validation.check_integer(volume, min=0, max=100) if self._mixer is None: - return False + return False # TODO: 2.0 return None with _mixer_error_handling(self._mixer): result = self._mixer.set_volume(volume).get() validation.check_instance(result, bool) return result - return None + return False def get_mute(self): """Get mute state. @@ -91,11 +91,11 @@ class MixerController(object): """ validation.check_boolean(mute) if self._mixer is None: - return False + return False # TODO: 2.0 return None with _mixer_error_handling(self._mixer): result = self._mixer.set_mute(bool(mute)).get() validation.check_instance(result, bool) return result - return None + return False diff --git a/tests/core/test_mixer.py b/tests/core/test_mixer.py index 40e13aea..45241fec 100644 --- a/tests/core/test_mixer.py +++ b/tests/core/test_mixer.py @@ -131,7 +131,7 @@ class SetVolumeBadBackendTest(MockBackendCoreMixerBase): def test_backend_returns_wrong_type(self): self.mixer.set_volume.return_value.get.return_value = 'done' - self.assertIs(self.core.mixer.set_volume(30), None) + self.assertFalse(self.core.mixer.set_volume(30)) class GetMuteBadBackendTest(MockBackendCoreMixerBase): @@ -153,4 +153,4 @@ class SetMuteBadBackendTest(MockBackendCoreMixerBase): def test_backend_returns_wrong_type(self): self.mixer.set_mute.return_value.get.return_value = 'done' - self.assertIs(self.core.mixer.set_mute(True), None) + self.assertFalse(self.core.mixer.set_mute(True)) From ae07603da09d007e0aa76e781085f062da44e704 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:02:23 +0200 Subject: [PATCH 25/42] docs: Add module name to API docs headers In the same way the Python stdlib docs does. --- docs/api/audio.rst | 6 +++--- docs/api/backends.rst | 6 +++--- docs/api/commands.rst | 6 +++--- docs/api/config.rst | 6 +++--- docs/api/core.rst | 6 +++--- docs/api/ext.rst | 6 +++--- docs/api/http.rst | 3 --- docs/api/mixer.rst | 6 +++--- docs/api/models.rst | 6 +++--- docs/api/zeroconf.rst | 6 +++--- 10 files changed, 27 insertions(+), 30 deletions(-) diff --git a/docs/api/audio.rst b/docs/api/audio.rst index 76389fb4..1e86625c 100644 --- a/docs/api/audio.rst +++ b/docs/api/audio.rst @@ -1,8 +1,8 @@ .. _audio-api: -********* -Audio API -********* +********************************* +:mod:`mopidy.audio` --- Audio API +********************************* .. module:: mopidy.audio :synopsis: Thin wrapper around the parts of GStreamer we use diff --git a/docs/api/backends.rst b/docs/api/backends.rst index 5e938357..f7218876 100644 --- a/docs/api/backends.rst +++ b/docs/api/backends.rst @@ -1,8 +1,8 @@ .. _backend-api: -*********** -Backend API -*********** +************************************* +:mod:`mopidy.backend` --- Backend API +************************************* .. module:: mopidy.backend :synopsis: The API implemented by backends diff --git a/docs/api/commands.rst b/docs/api/commands.rst index f0469350..216c4d46 100644 --- a/docs/api/commands.rst +++ b/docs/api/commands.rst @@ -1,8 +1,8 @@ .. _commands-api: -************ -Commands API -************ +*************************************** +:mod:`mopidy.commands` --- Commands API +*************************************** .. automodule:: mopidy.commands :synopsis: Commands API for Mopidy CLI. diff --git a/docs/api/config.rst b/docs/api/config.rst index 8b005a9d..289bda5a 100644 --- a/docs/api/config.rst +++ b/docs/api/config.rst @@ -1,8 +1,8 @@ .. _config-api: -********** -Config API -********** +*********************************** +:mod:`mopidy.config` --- Config API +*********************************** .. automodule:: mopidy.config :synopsis: Config API for config loading and validation diff --git a/docs/api/core.rst b/docs/api/core.rst index 38703222..9134afed 100644 --- a/docs/api/core.rst +++ b/docs/api/core.rst @@ -1,8 +1,8 @@ .. _core-api: -******** -Core API -******** +******************************* +:mod:`mopidy.core` --- Core API +******************************* .. module:: mopidy.core :synopsis: Core API for use by frontends diff --git a/docs/api/ext.rst b/docs/api/ext.rst index 11908920..220c763b 100644 --- a/docs/api/ext.rst +++ b/docs/api/ext.rst @@ -1,8 +1,8 @@ .. _ext-api: -************* -Extension API -************* +********************************** +:mod:`mopidy.ext` -- Extension API +********************************** If you want to learn how to make Mopidy extensions, read :ref:`extensiondev`. diff --git a/docs/api/http.rst b/docs/api/http.rst index 9a7d56bb..e428e471 100644 --- a/docs/api/http.rst +++ b/docs/api/http.rst @@ -4,9 +4,6 @@ HTTP JSON-RPC API ***************** -.. module:: mopidy.http - :synopsis: The HTTP frontend APIs - The :ref:`ext-http` extension makes Mopidy's :ref:`core-api` available using JSON-RPC over HTTP using HTTP POST and WebSockets. We also provide a JavaScript wrapper, called :ref:`Mopidy.js `, around the JSON-RPC over diff --git a/docs/api/mixer.rst b/docs/api/mixer.rst index 6f02e3c9..272bf3c7 100644 --- a/docs/api/mixer.rst +++ b/docs/api/mixer.rst @@ -1,8 +1,8 @@ .. _mixer-api: -*************** -Audio mixer API -*************** +*************************************** +:mod:`mopidy.mixer` --- Audio mixer API +*************************************** .. module:: mopidy.mixer :synopsis: The audio mixer API diff --git a/docs/api/models.rst b/docs/api/models.rst index d25e7f34..07702555 100644 --- a/docs/api/models.rst +++ b/docs/api/models.rst @@ -1,6 +1,6 @@ -*********** -Data models -*********** +************************************ +:mod:`mopidy.models` --- Data models +************************************ These immutable data models are used for all data transfer within the Mopidy backends and between the backends and the MPD frontend. All fields are optional diff --git a/docs/api/zeroconf.rst b/docs/api/zeroconf.rst index 7cdd93f0..552c5771 100644 --- a/docs/api/zeroconf.rst +++ b/docs/api/zeroconf.rst @@ -1,8 +1,8 @@ .. _zeroconf-api: -************ -Zeroconf API -************ +*************************************** +:mod:`mopidy.zeroconf` --- Zeroconf API +*************************************** .. module:: mopidy.zeroconf :synopsis: Helper for publishing of services on Zeroconf From ccecf6b6bf4176cbbae0cb631386508d21e97659 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:08:02 +0200 Subject: [PATCH 26/42] docs: Remove plurality from backends/frontends API docs --- docs/api/{backends.rst => backend.rst} | 0 docs/api/{frontends.rst => frontend.rst} | 0 docs/api/index.rst | 4 ++-- 3 files changed, 2 insertions(+), 2 deletions(-) rename docs/api/{backends.rst => backend.rst} (100%) rename docs/api/{frontends.rst => frontend.rst} (100%) diff --git a/docs/api/backends.rst b/docs/api/backend.rst similarity index 100% rename from docs/api/backends.rst rename to docs/api/backend.rst diff --git a/docs/api/frontends.rst b/docs/api/frontend.rst similarity index 100% rename from docs/api/frontends.rst rename to docs/api/frontend.rst diff --git a/docs/api/index.rst b/docs/api/index.rst index 3e008f00..3c57d963 100644 --- a/docs/api/index.rst +++ b/docs/api/index.rst @@ -16,10 +16,10 @@ API reference concepts models core - backends + backend audio mixer - frontends + frontend commands ext config From d02f7dca18f500cc7c44e5a957c55e2c5bce6c48 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:15:06 +0200 Subject: [PATCH 27/42] docs: Move frontend API between core and backend --- docs/api/index.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/index.rst b/docs/api/index.rst index 3c57d963..bdb803ee 100644 --- a/docs/api/index.rst +++ b/docs/api/index.rst @@ -16,10 +16,10 @@ API reference concepts models core + frontend backend audio mixer - frontend commands ext config From 526216b61b437caf8adb3c1879339d62c1461500 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:15:16 +0200 Subject: [PATCH 28/42] docs: Remove note header --- docs/api/index.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/api/index.rst b/docs/api/index.rst index bdb803ee..b19d9e21 100644 --- a/docs/api/index.rst +++ b/docs/api/index.rst @@ -4,7 +4,7 @@ API reference ************* -.. note:: What is public? +.. note:: Only APIs documented here are public and open for use by Mopidy extensions. From 1d82bd704350ad64ffaf8682dba8d55fe94ccc11 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:15:28 +0200 Subject: [PATCH 29/42] docs: Use consistent syntax for module headers --- docs/modules/local.rst | 6 +++--- docs/modules/mpd.rst | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/modules/local.rst b/docs/modules/local.rst index 31ca6498..014bac2a 100644 --- a/docs/modules/local.rst +++ b/docs/modules/local.rst @@ -1,6 +1,6 @@ -************************************ -:mod:`mopidy.local` -- Local backend -************************************ +************************************* +:mod:`mopidy.local` --- Local backend +************************************* For details on how to use Mopidy's local backend, see :ref:`ext-local`. diff --git a/docs/modules/mpd.rst b/docs/modules/mpd.rst index 1826e535..83650c39 100644 --- a/docs/modules/mpd.rst +++ b/docs/modules/mpd.rst @@ -1,6 +1,6 @@ -******************************* -:mod:`mopidy.mpd` -- MPD server -******************************* +******************************** +:mod:`mopidy.mpd` --- MPD server +******************************** For details on how to use Mopidy's MPD server, see :ref:`ext-mpd`. From 7c57c51b2eebb5d5e397cef2988b3c80b9fa760b Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:16:37 +0200 Subject: [PATCH 30/42] docs: Fix unexpected indentation error --- docs/changelog.rst | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/docs/changelog.rst b/docs/changelog.rst index 182db916..d010ba5f 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -12,10 +12,11 @@ Core API - Calling the following methods with ``kwargs`` is being deprecated. (PR: :issue:`1090`) - - :meth:`mopidy.core.library.LibraryController.search` - - :meth:`mopidy.core.library.PlaylistsController.filter` - - :meth:`mopidy.core.library.TracklistController.filter` - - :meth:`mopidy.core.library.TracklistController.remove` + + - :meth:`mopidy.core.library.LibraryController.search` + - :meth:`mopidy.core.library.PlaylistsController.filter` + - :meth:`mopidy.core.library.TracklistController.filter` + - :meth:`mopidy.core.library.TracklistController.remove` - Updated core controllers to handle backend exceptions in all calls that rely on multiple backends. (Issue: :issue:`667`) From 3d051e1a248ff7fd39e4b5a401c4d24775aa6952 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:20:05 +0200 Subject: [PATCH 31/42] docs: Remove old deps from list of mocked modules --- docs/conf.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index e970bdee..e91318cc 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -47,7 +47,6 @@ class Mock(object): return Mock() MOCK_MODULES = [ - 'cherrypy', 'dbus', 'dbus.mainloop', 'dbus.mainloop.glib', @@ -61,12 +60,6 @@ MOCK_MODULES = [ 'pykka.actor', 'pykka.future', 'pykka.registry', - 'pylast', - 'ws4py', - 'ws4py.messaging', - 'ws4py.server', - 'ws4py.server.cherrypyserver', - 'ws4py.websocket', ] for mod_name in MOCK_MODULES: sys.modules[mod_name] = Mock() From 622a3c549442609b5a5b21c1950b92826b566527 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:39:54 +0200 Subject: [PATCH 32/42] docs: Group API docs in sections --- docs/api/{concepts.rst => architecture.rst} | 6 +-- docs/api/index.rst | 46 +++++++++++++++++---- 2 files changed, 41 insertions(+), 11 deletions(-) rename docs/api/{concepts.rst => architecture.rst} (97%) diff --git a/docs/api/concepts.rst b/docs/api/architecture.rst similarity index 97% rename from docs/api/concepts.rst rename to docs/api/architecture.rst index 9c542777..b0789f49 100644 --- a/docs/api/concepts.rst +++ b/docs/api/architecture.rst @@ -1,8 +1,8 @@ .. _concepts: -************************* -Architecture and concepts -************************* +************ +Architecture +************ The overall architecture of Mopidy is organized around multiple frontends and backends. The frontends use the core API. The core actor makes multiple backends diff --git a/docs/api/index.rst b/docs/api/index.rst index b19d9e21..3a79af5d 100644 --- a/docs/api/index.rst +++ b/docs/api/index.rst @@ -10,20 +10,50 @@ API reference extensions. -.. toctree:: - :glob: +Concepts +======== - concepts +.. toctree:: + + architecture models + + +Basics +====== + +.. toctree:: + core frontend backend - audio - mixer - commands ext - config - zeroconf + + +Web/JavaScript +============== + +.. toctree:: + http-server http js + + +Audio +===== + +.. toctree:: + + audio + mixer + + +Utilities +========= + +.. toctree:: + + config + commands + zeroconf From f96a22e5cbc778ef555dc9c241a6906a411af390 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:40:52 +0200 Subject: [PATCH 33/42] docs: Remove note on how to access core attributes The corresponding methods are now fully documented and the old attributes are deprecated. --- docs/api/http.rst | 11 +++-------- docs/api/js.rst | 11 ++++------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/docs/api/http.rst b/docs/api/http.rst index e428e471..f2a50b27 100644 --- a/docs/api/http.rst +++ b/docs/api/http.rst @@ -62,14 +62,9 @@ JSON-RPC 2.0 messages can be recognized by checking for the key named please refer to the `JSON-RPC 2.0 spec `_. -All methods (not attributes) in the :ref:`core-api` is made available through -JSON-RPC calls over the WebSocket. For example, -:meth:`mopidy.core.PlaybackController.play` is available as the JSON-RPC method -``core.playback.play``. - -The core API's attributes is made available through setters and getters. For -example, the attribute :attr:`mopidy.core.PlaybackController.current_track` is -available as the JSON-RPC method ``core.playback.get_current_track``. +All methods in the :ref:`core-api` is made available through JSON-RPC calls +over the WebSocket. For example, :meth:`mopidy.core.PlaybackController.play` is +available as the JSON-RPC method ``core.playback.play``. Example JSON-RPC request:: diff --git a/docs/api/js.rst b/docs/api/js.rst index 29866d14..8771d48c 100644 --- a/docs/api/js.rst +++ b/docs/api/js.rst @@ -189,13 +189,10 @@ you've hooked up an errback (more on that a bit later) to the promise returned from the call, the errback will be called with a ``Mopidy.ConnectionError`` instance. -All methods in Mopidy's :ref:`core-api` is available via Mopidy.js. The core -API attributes is *not* available, but that shouldn't be a problem as we've -added (undocumented) getters and setters for all of them, so you can access the -attributes as well from JavaScript. For example, the -:attr:`mopidy.core.PlaybackController.state` attribute is available in -JSON-RPC as the method ``core.playback.get_state`` and in Mopidy.js as -``mopidy.playback.getState()``. +All methods in Mopidy's :ref:`core-api` is available via Mopidy.js. For +example, the :meth:`mopidy.core.PlaybackController.get_state` method is +available in JSON-RPC as the method ``core.playback.get_state`` and in +Mopidy.js as ``mopidy.playback.getState()``. Both the WebSocket API and the JavaScript API are based on introspection of the core Python API. Thus, they will always be up to date and immediately reflect From d0418d625be360951a4c6c4d4f2c67fb50496ec4 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 21:42:37 +0200 Subject: [PATCH 34/42] docs: Link from JS docs to static web client example --- docs/api/http-server.rst | 2 ++ docs/api/js.rst | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/api/http-server.rst b/docs/api/http-server.rst index 317a77c5..463abf5a 100644 --- a/docs/api/http-server.rst +++ b/docs/api/http-server.rst @@ -25,6 +25,8 @@ For details on how to make a Mopidy extension, see the :ref:`extensiondev` guide. +.. _static-web-client: + Static web client example ========================= diff --git a/docs/api/js.rst b/docs/api/js.rst index 8771d48c..b98eb566 100644 --- a/docs/api/js.rst +++ b/docs/api/js.rst @@ -21,9 +21,9 @@ available at: You may need to adjust hostname and port for your local setup. -Thus, if you use Mopidy to host your web client, like described above, you can -load the latest version of Mopidy.js by adding the following script tag to your -HTML file: +Thus, if you use Mopidy to host your web client, like described in +:ref:`static-web-client`, you can load the latest version of Mopidy.js by +adding the following script tag to your HTML file: .. code-block:: html From 6fe382f37e4469fe6af8f8dfca522b0ab6f2febe Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 22:34:13 +0200 Subject: [PATCH 35/42] docs: Mopidy.js supports by-name parameters Since Mopidy 0.19 / Mopidy.js 0.4 --- docs/api/js.rst | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/api/js.rst b/docs/api/js.rst index b98eb566..62f7d438 100644 --- a/docs/api/js.rst +++ b/docs/api/js.rst @@ -215,8 +215,7 @@ by looking at the method's ``description`` and ``params`` attributes: JSON-RPC 2.0 limits method parameters to be sent *either* by-position or by-name. Combinations of both, like we're used to from Python, isn't supported -by JSON-RPC 2.0. To further limit this, Mopidy.js currently only supports -passing parameters by-position. +by JSON-RPC 2.0. Obviously, you'll want to get a return value from many of your method calls. Since everything is happening across the WebSocket and maybe even across the From 4c8c8cd9279590518454d519d07652015e431b77 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 22:36:56 +0200 Subject: [PATCH 36/42] docs: Don't refer to when.js before it's introduced --- docs/api/js.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/api/js.rst b/docs/api/js.rst index 62f7d438..6a8e0fcd 100644 --- a/docs/api/js.rst +++ b/docs/api/js.rst @@ -268,8 +268,9 @@ passing it as the second argument to ``done()``: .done(printCurrentTrack, console.error.bind(console)); If you don't hook up an error handler function and never call ``done()`` on the -promise object, when.js will log warnings to the console that you have -unhandled errors. In general, unhandled errors will not go silently missing. +promise object, warnings will be logged to the console complaining that you +have unhandled errors. In general, unhandled errors will not go silently +missing. The promise objects returned by Mopidy.js adheres to the `CommonJS Promises/A `_ standard. We use the From d8bcd7f273ecdf8f5ad3967de76fbee099e62ecf Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Thu, 7 May 2015 23:15:56 +0200 Subject: [PATCH 37/42] Rename mopidy.utils to mopidy.internal --- docs/conf.py | 2 +- docs/extensiondev.rst | 4 ++-- mopidy/__main__.py | 4 ++-- mopidy/audio/actor.py | 2 +- mopidy/audio/scan.py | 4 ++-- mopidy/commands.py | 2 +- mopidy/config/__init__.py | 2 +- mopidy/config/types.py | 2 +- mopidy/core/actor.py | 4 ++-- mopidy/core/library.py | 2 +- mopidy/core/mixer.py | 2 +- mopidy/core/playback.py | 2 +- mopidy/core/playlists.py | 2 +- mopidy/core/tracklist.py | 2 +- mopidy/http/actor.py | 2 +- mopidy/http/handlers.py | 2 +- mopidy/{utils => internal}/__init__.py | 0 mopidy/{utils => internal}/deprecation.py | 0 mopidy/{utils => internal}/deps.py | 2 +- mopidy/{utils => internal}/encoding.py | 0 mopidy/{utils => internal}/formatting.py | 0 mopidy/{utils => internal}/jsonrpc.py | 0 mopidy/{utils => internal}/log.py | 0 mopidy/{utils => internal}/network.py | 2 +- mopidy/{utils => internal}/path.py | 2 +- mopidy/{utils => internal}/process.py | 0 mopidy/{utils => internal}/timer.py | 0 mopidy/{utils => internal}/validation.py | 0 mopidy/{utils => internal}/versioning.py | 0 mopidy/{utils => internal}/xdg.py | 0 mopidy/local/commands.py | 2 +- mopidy/local/json.py | 2 +- mopidy/local/storage.py | 2 +- mopidy/local/translator.py | 6 +++--- mopidy/m3u/actor.py | 2 +- mopidy/m3u/translator.py | 15 +++++++-------- mopidy/models/immutable.py | 2 +- mopidy/mpd/actor.py | 2 +- mopidy/mpd/protocol/current_playlist.py | 2 +- mopidy/mpd/protocol/music_db.py | 2 +- mopidy/mpd/protocol/playback.py | 2 +- mopidy/mpd/session.py | 2 +- tests/audio/test_actor.py | 8 ++++---- tests/audio/test_scan.py | 2 +- tests/config/test_types.py | 2 +- tests/core/test_actor.py | 2 +- tests/core/test_events.py | 2 +- tests/core/test_library.py | 2 +- tests/core/test_playback.py | 2 +- tests/core/test_playlists.py | 2 +- tests/core/test_tracklist.py | 2 +- tests/{utils => internal}/__init__.py | 0 tests/{utils => internal}/network/__init__.py | 0 .../network/test_connection.py | 2 +- .../network/test_lineprotocol.py | 2 +- tests/{utils => internal}/network/test_server.py | 2 +- tests/{utils => internal}/network/test_utils.py | 10 +++++----- tests/{utils => internal}/test_deps.py | 2 +- tests/{utils => internal}/test_encoding.py | 12 ++++++------ tests/{utils => internal}/test_jsonrpc.py | 2 +- tests/{utils => internal}/test_path.py | 2 +- tests/{utils => internal}/test_validation.py | 2 +- tests/{utils => internal}/test_xdg.py | 2 +- tests/local/__init__.py | 2 +- tests/local/test_playback.py | 2 +- tests/local/test_tracklist.py | 2 +- tests/m3u/test_playlists.py | 2 +- tests/m3u/test_translator.py | 2 +- tests/mpd/protocol/__init__.py | 2 +- tests/mpd/protocol/test_current_playlist.py | 2 +- tests/mpd/protocol/test_playback.py | 2 +- tests/mpd/test_dispatcher.py | 2 +- tests/mpd/test_status.py | 2 +- tests/mpd/test_translator.py | 6 +++--- tests/stream/test_library.py | 6 +++--- 75 files changed, 90 insertions(+), 91 deletions(-) rename mopidy/{utils => internal}/__init__.py (100%) rename mopidy/{utils => internal}/deprecation.py (100%) rename mopidy/{utils => internal}/deps.py (99%) rename mopidy/{utils => internal}/encoding.py (100%) rename mopidy/{utils => internal}/formatting.py (100%) rename mopidy/{utils => internal}/jsonrpc.py (100%) rename mopidy/{utils => internal}/log.py (100%) rename mopidy/{utils => internal}/network.py (99%) rename mopidy/{utils => internal}/path.py (99%) rename mopidy/{utils => internal}/process.py (100%) rename mopidy/{utils => internal}/timer.py (100%) rename mopidy/{utils => internal}/validation.py (100%) rename mopidy/{utils => internal}/versioning.py (100%) rename mopidy/{utils => internal}/xdg.py (100%) rename tests/{utils => internal}/__init__.py (100%) rename tests/{utils => internal}/network/__init__.py (100%) rename tests/{utils => internal}/network/test_connection.py (99%) rename tests/{utils => internal}/network/test_lineprotocol.py (99%) rename tests/{utils => internal}/network/test_server.py (99%) rename tests/{utils => internal}/network/test_utils.py (87%) rename tests/{utils => internal}/test_deps.py (99%) rename tests/{utils => internal}/test_encoding.py (80%) rename tests/{utils => internal}/test_jsonrpc.py (99%) rename tests/{utils => internal}/test_path.py (99%) rename tests/{utils => internal}/test_validation.py (99%) rename tests/{utils => internal}/test_xdg.py (98%) diff --git a/docs/conf.py b/docs/conf.py index e91318cc..cc760720 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -95,7 +95,7 @@ master_doc = 'index' project = 'Mopidy' copyright = '2009-2015, Stein Magnus Jodal and contributors' -from mopidy.utils.versioning import get_version +from mopidy.internal.versioning import get_version release = get_version() version = '.'.join(release.split('.')[:2]) diff --git a/docs/extensiondev.rst b/docs/extensiondev.rst index a2a5f463..1e25f48b 100644 --- a/docs/extensiondev.rst +++ b/docs/extensiondev.rst @@ -434,8 +434,8 @@ Use of Mopidy APIs ================== When writing an extension, you should only use APIs documented at -:ref:`api-ref`. Other parts of Mopidy, like :mod:`mopidy.utils`, may change at -any time and are not something extensions should use. +:ref:`api-ref`. Other parts of Mopidy, like :mod:`mopidy.internal`, may change +at any time and are not something extensions should use. Logging in extensions diff --git a/mopidy/__main__.py b/mopidy/__main__.py index 9ec9769f..6584146f 100644 --- a/mopidy/__main__.py +++ b/mopidy/__main__.py @@ -41,7 +41,7 @@ sys.argv[1:] = [] from mopidy import commands, config as config_lib, ext -from mopidy.utils import encoding, log, path, process, versioning +from mopidy.internal import encoding, log, path, process, versioning logger = logging.getLogger(__name__) @@ -137,7 +137,7 @@ def main(): extension.setup(registry) # Anything that wants to exit after this point must use - # mopidy.utils.process.exit_process as actors can have been started. + # mopidy.internal.process.exit_process as actors can have been started. try: return args.command.run(args, proxied_config) except NotImplementedError: diff --git a/mopidy/audio/actor.py b/mopidy/audio/actor.py index 4577c3f7..72750bdf 100644 --- a/mopidy/audio/actor.py +++ b/mopidy/audio/actor.py @@ -16,7 +16,7 @@ from mopidy import exceptions from mopidy.audio import playlists, utils from mopidy.audio.constants import PlaybackState from mopidy.audio.listener import AudioListener -from mopidy.utils import deprecation, process +from mopidy.internal import deprecation, process logger = logging.getLogger(__name__) diff --git a/mopidy/audio/scan.py b/mopidy/audio/scan.py index 385c41af..cf370052 100644 --- a/mopidy/audio/scan.py +++ b/mopidy/audio/scan.py @@ -10,7 +10,7 @@ import gst.pbutils # noqa from mopidy import exceptions from mopidy.audio import utils -from mopidy.utils import encoding +from mopidy.internal import encoding _missing_plugin_desc = gst.pbutils.missing_plugin_message_get_description @@ -182,7 +182,7 @@ if __name__ == '__main__': import gobject - from mopidy.utils import path + from mopidy.internal import path gobject.threads_init() diff --git a/mopidy/commands.py b/mopidy/commands.py index 2414348b..29564779 100644 --- a/mopidy/commands.py +++ b/mopidy/commands.py @@ -13,7 +13,7 @@ import gobject from mopidy import config as config_lib, exceptions from mopidy.audio import Audio from mopidy.core import Core -from mopidy.utils import deps, process, timer, versioning +from mopidy.internal import deps, process, timer, versioning logger = logging.getLogger(__name__) diff --git a/mopidy/config/__init__.py b/mopidy/config/__init__.py index fd914994..fc6dcb60 100644 --- a/mopidy/config/__init__.py +++ b/mopidy/config/__init__.py @@ -11,7 +11,7 @@ from mopidy.compat import configparser from mopidy.config import keyring from mopidy.config.schemas import * # noqa from mopidy.config.types import * # noqa -from mopidy.utils import path, versioning +from mopidy.internal import path, versioning logger = logging.getLogger(__name__) diff --git a/mopidy/config/types.py b/mopidy/config/types.py index 8359766f..9d673c43 100644 --- a/mopidy/config/types.py +++ b/mopidy/config/types.py @@ -6,7 +6,7 @@ import socket from mopidy import compat from mopidy.config import validators -from mopidy.utils import log, path +from mopidy.internal import log, path def decode(value): diff --git a/mopidy/core/actor.py b/mopidy/core/actor.py index c3967f6a..b6318492 100644 --- a/mopidy/core/actor.py +++ b/mopidy/core/actor.py @@ -14,8 +14,8 @@ from mopidy.core.mixer import MixerController from mopidy.core.playback import PlaybackController from mopidy.core.playlists import PlaylistsController from mopidy.core.tracklist import TracklistController -from mopidy.utils import versioning -from mopidy.utils.deprecation import deprecated_property +from mopidy.internal import versioning +from mopidy.internal.deprecation import deprecated_property class Core( diff --git a/mopidy/core/library.py b/mopidy/core/library.py index 8e6e4015..f801836a 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -7,7 +7,7 @@ import operator import urlparse from mopidy import compat, exceptions, models -from mopidy.utils import deprecation, validation +from mopidy.internal import deprecation, validation logger = logging.getLogger(__name__) diff --git a/mopidy/core/mixer.py b/mopidy/core/mixer.py index 2815fb0c..649ff270 100644 --- a/mopidy/core/mixer.py +++ b/mopidy/core/mixer.py @@ -4,7 +4,7 @@ import contextlib import logging from mopidy import exceptions -from mopidy.utils import validation +from mopidy.internal import validation logger = logging.getLogger(__name__) diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index 5836ff68..6d17620a 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -6,7 +6,7 @@ import urlparse from mopidy import models from mopidy.audio import PlaybackState from mopidy.core import listener -from mopidy.utils import deprecation, validation +from mopidy.internal import deprecation, validation logger = logging.getLogger(__name__) diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index c5e6d0cc..086806cc 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -6,8 +6,8 @@ import urlparse from mopidy import exceptions from mopidy.core import listener +from mopidy.internal import deprecation, validation from mopidy.models import Playlist, Ref -from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 692762ef..028e7c02 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -4,8 +4,8 @@ import logging import random from mopidy.core import listener +from mopidy.internal import deprecation, validation from mopidy.models import TlTrack, Track -from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) diff --git a/mopidy/http/actor.py b/mopidy/http/actor.py index 200ef833..5fe29134 100644 --- a/mopidy/http/actor.py +++ b/mopidy/http/actor.py @@ -16,7 +16,7 @@ import tornado.websocket from mopidy import exceptions, models, zeroconf from mopidy.core import CoreListener from mopidy.http import handlers -from mopidy.utils import encoding, formatting, network +from mopidy.internal import encoding, formatting, network logger = logging.getLogger(__name__) diff --git a/mopidy/http/handlers.py b/mopidy/http/handlers.py index 228c245c..a752a4f0 100644 --- a/mopidy/http/handlers.py +++ b/mopidy/http/handlers.py @@ -12,7 +12,7 @@ import tornado.websocket import mopidy from mopidy import core, models -from mopidy.utils import encoding, jsonrpc +from mopidy.internal import encoding, jsonrpc logger = logging.getLogger(__name__) diff --git a/mopidy/utils/__init__.py b/mopidy/internal/__init__.py similarity index 100% rename from mopidy/utils/__init__.py rename to mopidy/internal/__init__.py diff --git a/mopidy/utils/deprecation.py b/mopidy/internal/deprecation.py similarity index 100% rename from mopidy/utils/deprecation.py rename to mopidy/internal/deprecation.py diff --git a/mopidy/utils/deps.py b/mopidy/internal/deps.py similarity index 99% rename from mopidy/utils/deps.py rename to mopidy/internal/deps.py index aafede9d..1f363657 100644 --- a/mopidy/utils/deps.py +++ b/mopidy/internal/deps.py @@ -11,7 +11,7 @@ import pygst pygst.require('0.10') import gst # noqa -from mopidy.utils import formatting +from mopidy.internal import formatting def format_dependency_list(adapters=None): diff --git a/mopidy/utils/encoding.py b/mopidy/internal/encoding.py similarity index 100% rename from mopidy/utils/encoding.py rename to mopidy/internal/encoding.py diff --git a/mopidy/utils/formatting.py b/mopidy/internal/formatting.py similarity index 100% rename from mopidy/utils/formatting.py rename to mopidy/internal/formatting.py diff --git a/mopidy/utils/jsonrpc.py b/mopidy/internal/jsonrpc.py similarity index 100% rename from mopidy/utils/jsonrpc.py rename to mopidy/internal/jsonrpc.py diff --git a/mopidy/utils/log.py b/mopidy/internal/log.py similarity index 100% rename from mopidy/utils/log.py rename to mopidy/internal/log.py diff --git a/mopidy/utils/network.py b/mopidy/internal/network.py similarity index 99% rename from mopidy/utils/network.py rename to mopidy/internal/network.py index 000382e3..4b8b35fe 100644 --- a/mopidy/utils/network.py +++ b/mopidy/internal/network.py @@ -11,7 +11,7 @@ import gobject import pykka -from mopidy.utils import encoding +from mopidy.internal import encoding logger = logging.getLogger(__name__) diff --git a/mopidy/utils/path.py b/mopidy/internal/path.py similarity index 99% rename from mopidy/utils/path.py rename to mopidy/internal/path.py index 37b6cdb1..3a41d930 100644 --- a/mopidy/utils/path.py +++ b/mopidy/internal/path.py @@ -10,7 +10,7 @@ import urlparse from mopidy import compat, exceptions from mopidy.compat import queue -from mopidy.utils import encoding, xdg +from mopidy.internal import encoding, xdg logger = logging.getLogger(__name__) diff --git a/mopidy/utils/process.py b/mopidy/internal/process.py similarity index 100% rename from mopidy/utils/process.py rename to mopidy/internal/process.py diff --git a/mopidy/utils/timer.py b/mopidy/internal/timer.py similarity index 100% rename from mopidy/utils/timer.py rename to mopidy/internal/timer.py diff --git a/mopidy/utils/validation.py b/mopidy/internal/validation.py similarity index 100% rename from mopidy/utils/validation.py rename to mopidy/internal/validation.py diff --git a/mopidy/utils/versioning.py b/mopidy/internal/versioning.py similarity index 100% rename from mopidy/utils/versioning.py rename to mopidy/internal/versioning.py diff --git a/mopidy/utils/xdg.py b/mopidy/internal/xdg.py similarity index 100% rename from mopidy/utils/xdg.py rename to mopidy/internal/xdg.py diff --git a/mopidy/local/commands.py b/mopidy/local/commands.py index c8c70216..7033f3aa 100644 --- a/mopidy/local/commands.py +++ b/mopidy/local/commands.py @@ -7,8 +7,8 @@ import time from mopidy import commands, compat, exceptions from mopidy.audio import scan, utils +from mopidy.internal import path from mopidy.local import translator -from mopidy.utils import path logger = logging.getLogger(__name__) diff --git a/mopidy/local/json.py b/mopidy/local/json.py index 22fcfa5b..715b5c5d 100644 --- a/mopidy/local/json.py +++ b/mopidy/local/json.py @@ -11,8 +11,8 @@ import tempfile import mopidy from mopidy import compat, local, models +from mopidy.internal import encoding, timer from mopidy.local import search, storage, translator -from mopidy.utils import encoding, timer logger = logging.getLogger(__name__) diff --git a/mopidy/local/storage.py b/mopidy/local/storage.py index 21d278e5..1808c4a2 100644 --- a/mopidy/local/storage.py +++ b/mopidy/local/storage.py @@ -3,7 +3,7 @@ from __future__ import absolute_import, unicode_literals import logging import os -from mopidy.utils import encoding, path +from mopidy.internal import encoding, path logger = logging.getLogger(__name__) diff --git a/mopidy/local/translator.py b/mopidy/local/translator.py index 92b20a7b..6e5c9c01 100644 --- a/mopidy/local/translator.py +++ b/mopidy/local/translator.py @@ -5,20 +5,20 @@ import os import urllib from mopidy import compat -from mopidy.utils.path import path_to_uri, uri_to_path +from mopidy.internal import path logger = logging.getLogger(__name__) def local_track_uri_to_file_uri(uri, media_dir): - return path_to_uri(local_track_uri_to_path(uri, media_dir)) + return path.path_to_uri(local_track_uri_to_path(uri, media_dir)) def local_track_uri_to_path(uri, media_dir): if not uri.startswith('local:track:'): raise ValueError('Invalid URI.') - file_path = uri_to_path(uri).split(b':', 1)[1] + file_path = path.uri_to_path(uri).split(b':', 1)[1] return os.path.join(media_dir, file_path) diff --git a/mopidy/m3u/actor.py b/mopidy/m3u/actor.py index 3908d938..fe959d86 100644 --- a/mopidy/m3u/actor.py +++ b/mopidy/m3u/actor.py @@ -5,9 +5,9 @@ import logging import pykka from mopidy import backend +from mopidy.internal import encoding, path from mopidy.m3u.library import M3ULibraryProvider from mopidy.m3u.playlists import M3UPlaylistsProvider -from mopidy.utils import encoding, path logger = logging.getLogger(__name__) diff --git a/mopidy/m3u/translator.py b/mopidy/m3u/translator.py index 177ab6c3..96a47fdc 100644 --- a/mopidy/m3u/translator.py +++ b/mopidy/m3u/translator.py @@ -7,9 +7,8 @@ import urllib import urlparse from mopidy import compat +from mopidy.internal import encoding, path from mopidy.models import Track -from mopidy.utils.encoding import locale_decode -from mopidy.utils.path import path_to_uri, uri_to_path M3U_EXTINF_RE = re.compile(r'#EXTINF:(-1|\d+),(.*)') @@ -20,7 +19,7 @@ logger = logging.getLogger(__name__) def playlist_uri_to_path(uri, playlists_dir): if not uri.startswith('m3u:'): raise ValueError('Invalid URI %s' % uri) - file_path = uri_to_path(uri) + file_path = path.uri_to_path(uri) return os.path.join(playlists_dir, file_path) @@ -80,7 +79,7 @@ def parse_m3u(file_path, media_dir=None): with open(file_path) as m3u: contents = m3u.readlines() except IOError as error: - logger.warning('Couldn\'t open m3u: %s', locale_decode(error)) + logger.warning('Couldn\'t open m3u: %s', encoding.locale_decode(error)) return tracks if not contents: @@ -100,11 +99,11 @@ def parse_m3u(file_path, media_dir=None): if urlparse.urlsplit(line).scheme: tracks.append(track.replace(uri=line)) elif os.path.normpath(line) == os.path.abspath(line): - path = path_to_uri(line) - tracks.append(track.replace(uri=path)) + uri = path.path_to_uri(line) + tracks.append(track.replace(uri=uri)) elif media_dir is not None: - path = path_to_uri(os.path.join(media_dir, line)) - tracks.append(track.replace(uri=path)) + uri = path.path_to_uri(os.path.join(media_dir, line)) + tracks.append(track.replace(uri=uri)) track = Track() return tracks diff --git a/mopidy/models/immutable.py b/mopidy/models/immutable.py index 98cd8b5b..8bbf568b 100644 --- a/mopidy/models/immutable.py +++ b/mopidy/models/immutable.py @@ -4,8 +4,8 @@ import copy import itertools import weakref +from mopidy.internal import deprecation from mopidy.models.fields import Field -from mopidy.utils import deprecation class ImmutableObject(object): diff --git a/mopidy/mpd/actor.py b/mopidy/mpd/actor.py index 36775578..8eb59c1f 100644 --- a/mopidy/mpd/actor.py +++ b/mopidy/mpd/actor.py @@ -6,8 +6,8 @@ import pykka from mopidy import exceptions, zeroconf from mopidy.core import CoreListener +from mopidy.internal import encoding, network, process from mopidy.mpd import session, uri_mapper -from mopidy.utils import encoding, network, process logger = logging.getLogger(__name__) diff --git a/mopidy/mpd/protocol/current_playlist.py b/mopidy/mpd/protocol/current_playlist.py index f93722ee..f44abb95 100644 --- a/mopidy/mpd/protocol/current_playlist.py +++ b/mopidy/mpd/protocol/current_playlist.py @@ -2,8 +2,8 @@ from __future__ import absolute_import, unicode_literals import urlparse +from mopidy.internal import deprecation from mopidy.mpd import exceptions, protocol, translator -from mopidy.utils import deprecation @protocol.commands.add('add') diff --git a/mopidy/mpd/protocol/music_db.py b/mopidy/mpd/protocol/music_db.py index 0350fc21..510d3ac1 100644 --- a/mopidy/mpd/protocol/music_db.py +++ b/mopidy/mpd/protocol/music_db.py @@ -3,9 +3,9 @@ from __future__ import absolute_import, unicode_literals import functools import itertools +from mopidy.internal import deprecation from mopidy.models import Track from mopidy.mpd import exceptions, protocol, translator -from mopidy.utils import deprecation _SEARCH_MAPPING = { 'album': 'album', diff --git a/mopidy/mpd/protocol/playback.py b/mopidy/mpd/protocol/playback.py index ce3174d7..333e1ccb 100644 --- a/mopidy/mpd/protocol/playback.py +++ b/mopidy/mpd/protocol/playback.py @@ -1,8 +1,8 @@ from __future__ import absolute_import, unicode_literals from mopidy.core import PlaybackState +from mopidy.internal import deprecation from mopidy.mpd import exceptions, protocol -from mopidy.utils import deprecation @protocol.commands.add('consume', state=protocol.BOOL) diff --git a/mopidy/mpd/session.py b/mopidy/mpd/session.py index adbf6cc3..68550f3b 100644 --- a/mopidy/mpd/session.py +++ b/mopidy/mpd/session.py @@ -2,8 +2,8 @@ from __future__ import absolute_import, unicode_literals import logging +from mopidy.internal import formatting, network from mopidy.mpd import dispatcher, protocol -from mopidy.utils import formatting, network logger = logging.getLogger(__name__) diff --git a/tests/audio/test_actor.py b/tests/audio/test_actor.py index 7d5f6148..046971a8 100644 --- a/tests/audio/test_actor.py +++ b/tests/audio/test_actor.py @@ -16,7 +16,7 @@ import pykka from mopidy import audio from mopidy.audio.constants import PlaybackState -from mopidy.utils.path import path_to_uri +from mopidy.internal import path from tests import dummy_audio, path_to_data_dir @@ -36,8 +36,8 @@ class BaseTest(unittest.TestCase): } } - uris = [path_to_uri(path_to_data_dir('song1.wav')), - path_to_uri(path_to_data_dir('song2.wav'))] + uris = [path.path_to_uri(path_to_data_dir('song1.wav')), + path.path_to_uri(path_to_data_dir('song2.wav'))] audio_class = audio.Audio @@ -53,7 +53,7 @@ class BaseTest(unittest.TestCase): 'hostname': '', }, } - self.song_uri = path_to_uri(path_to_data_dir('song1.wav')) + self.song_uri = path.path_to_uri(path_to_data_dir('song1.wav')) self.audio = self.audio_class.start(config=config, mixer=None).proxy() def tearDown(self): # noqa diff --git a/tests/audio/test_scan.py b/tests/audio/test_scan.py index ff5a4641..c558835e 100644 --- a/tests/audio/test_scan.py +++ b/tests/audio/test_scan.py @@ -8,7 +8,7 @@ gobject.threads_init() from mopidy import exceptions from mopidy.audio import scan -from mopidy.utils import path as path_lib +from mopidy.internal import path as path_lib from tests import path_to_data_dir diff --git a/tests/config/test_types.py b/tests/config/test_types.py index be1ab829..40226c51 100644 --- a/tests/config/test_types.py +++ b/tests/config/test_types.py @@ -373,7 +373,7 @@ class ExpandedPathTest(unittest.TestCase): expanded = b'expanded_path' self.assertEqual(expanded, types.ExpandedPath(original, expanded)) - @mock.patch('mopidy.utils.path.expand_path') + @mock.patch('mopidy.internal.path.expand_path') def test_orginal_stores_unexpanded(self, expand_path_mock): original = b'~' expanded = b'expanded_path' diff --git a/tests/core/test_actor.py b/tests/core/test_actor.py index 520c5026..410933d2 100644 --- a/tests/core/test_actor.py +++ b/tests/core/test_actor.py @@ -7,7 +7,7 @@ import mock import pykka from mopidy.core import Core -from mopidy.utils import versioning +from mopidy.internal import versioning class CoreActorTest(unittest.TestCase): diff --git a/tests/core/test_events.py b/tests/core/test_events.py index b6cd25b9..7c8eba1d 100644 --- a/tests/core/test_events.py +++ b/tests/core/test_events.py @@ -7,8 +7,8 @@ import mock import pykka from mopidy import core +from mopidy.internal import deprecation from mopidy.models import Track -from mopidy.utils import deprecation from tests import dummy_backend diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 2f244ce7..941f1831 100644 --- a/tests/core/test_library.py +++ b/tests/core/test_library.py @@ -5,8 +5,8 @@ import unittest import mock from mopidy import backend, core +from mopidy.internal import deprecation from mopidy.models import Image, Ref, SearchResult, Track -from mopidy.utils import deprecation class BaseCoreLibraryTest(unittest.TestCase): diff --git a/tests/core/test_playback.py b/tests/core/test_playback.py index 4b81dd8b..67f5841e 100644 --- a/tests/core/test_playback.py +++ b/tests/core/test_playback.py @@ -7,8 +7,8 @@ import mock import pykka from mopidy import backend, core +from mopidy.internal import deprecation from mopidy.models import Track -from mopidy.utils import deprecation from tests import dummy_audio as audio diff --git a/tests/core/test_playlists.py b/tests/core/test_playlists.py index 2dabf93b..029254a8 100644 --- a/tests/core/test_playlists.py +++ b/tests/core/test_playlists.py @@ -5,8 +5,8 @@ import unittest import mock from mopidy import backend, core +from mopidy.internal import deprecation from mopidy.models import Playlist, Ref, Track -from mopidy.utils import deprecation class BasePlaylistsTest(unittest.TestCase): diff --git a/tests/core/test_tracklist.py b/tests/core/test_tracklist.py index 6339a18c..83b576ea 100644 --- a/tests/core/test_tracklist.py +++ b/tests/core/test_tracklist.py @@ -5,8 +5,8 @@ import unittest import mock from mopidy import backend, core +from mopidy.internal import deprecation from mopidy.models import TlTrack, Track -from mopidy.utils import deprecation class TracklistTest(unittest.TestCase): diff --git a/tests/utils/__init__.py b/tests/internal/__init__.py similarity index 100% rename from tests/utils/__init__.py rename to tests/internal/__init__.py diff --git a/tests/utils/network/__init__.py b/tests/internal/network/__init__.py similarity index 100% rename from tests/utils/network/__init__.py rename to tests/internal/network/__init__.py diff --git a/tests/utils/network/test_connection.py b/tests/internal/network/test_connection.py similarity index 99% rename from tests/utils/network/test_connection.py rename to tests/internal/network/test_connection.py index 3ad1df6b..8ae7d15c 100644 --- a/tests/utils/network/test_connection.py +++ b/tests/internal/network/test_connection.py @@ -11,7 +11,7 @@ from mock import Mock, call, patch, sentinel import pykka -from mopidy.utils import network +from mopidy.internal import network from tests import any_int, any_unicode diff --git a/tests/utils/network/test_lineprotocol.py b/tests/internal/network/test_lineprotocol.py similarity index 99% rename from tests/utils/network/test_lineprotocol.py rename to tests/internal/network/test_lineprotocol.py index d3548117..586d180e 100644 --- a/tests/utils/network/test_lineprotocol.py +++ b/tests/internal/network/test_lineprotocol.py @@ -8,7 +8,7 @@ import unittest from mock import Mock, sentinel from mopidy import compat -from mopidy.utils import network +from mopidy.internal import network from tests import any_unicode diff --git a/tests/utils/network/test_server.py b/tests/internal/network/test_server.py similarity index 99% rename from tests/utils/network/test_server.py rename to tests/internal/network/test_server.py index 5ea64fca..af8effd2 100644 --- a/tests/utils/network/test_server.py +++ b/tests/internal/network/test_server.py @@ -8,7 +8,7 @@ import gobject from mock import Mock, patch, sentinel -from mopidy.utils import network +from mopidy.internal import network from tests import any_int diff --git a/tests/utils/network/test_utils.py b/tests/internal/network/test_utils.py similarity index 87% rename from tests/utils/network/test_utils.py rename to tests/internal/network/test_utils.py index 55d68a99..a769ff93 100644 --- a/tests/utils/network/test_utils.py +++ b/tests/internal/network/test_utils.py @@ -5,18 +5,18 @@ import unittest from mock import Mock, patch -from mopidy.utils import network +from mopidy.internal import network class FormatHostnameTest(unittest.TestCase): - @patch('mopidy.utils.network.has_ipv6', True) + @patch('mopidy.internal.network.has_ipv6', True) def test_format_hostname_prefixes_ipv4_addresses_when_ipv6_available(self): network.has_ipv6 = True self.assertEqual(network.format_hostname('0.0.0.0'), '::ffff:0.0.0.0') self.assertEqual(network.format_hostname('1.0.0.1'), '::ffff:1.0.0.1') - @patch('mopidy.utils.network.has_ipv6', False) + @patch('mopidy.internal.network.has_ipv6', False) def test_format_hostname_does_nothing_when_only_ipv4_available(self): network.has_ipv6 = False self.assertEqual(network.format_hostname('0.0.0.0'), '0.0.0.0') @@ -43,14 +43,14 @@ class TryIPv6SocketTest(unittest.TestCase): class CreateSocketTest(unittest.TestCase): - @patch('mopidy.utils.network.has_ipv6', False) + @patch('mopidy.internal.network.has_ipv6', False) @patch('socket.socket') def test_ipv4_socket(self, socket_mock): network.create_socket() self.assertEqual( socket_mock.call_args[0], (socket.AF_INET, socket.SOCK_STREAM)) - @patch('mopidy.utils.network.has_ipv6', True) + @patch('mopidy.internal.network.has_ipv6', True) @patch('socket.socket') def test_ipv6_socket(self, socket_mock): network.create_socket() diff --git a/tests/utils/test_deps.py b/tests/internal/test_deps.py similarity index 99% rename from tests/utils/test_deps.py rename to tests/internal/test_deps.py index 394fba85..27e6f629 100644 --- a/tests/utils/test_deps.py +++ b/tests/internal/test_deps.py @@ -12,7 +12,7 @@ import pygst pygst.require('0.10') import gst # noqa -from mopidy.utils import deps +from mopidy.internal import deps class DepsTest(unittest.TestCase): diff --git a/tests/utils/test_encoding.py b/tests/internal/test_encoding.py similarity index 80% rename from tests/utils/test_encoding.py rename to tests/internal/test_encoding.py index 2ec7e529..cc8987ce 100644 --- a/tests/utils/test_encoding.py +++ b/tests/internal/test_encoding.py @@ -4,16 +4,16 @@ import unittest import mock -from mopidy.utils.encoding import locale_decode +from mopidy.internal import encoding -@mock.patch('mopidy.utils.encoding.locale.getpreferredencoding') +@mock.patch('mopidy.internal.encoding.locale.getpreferredencoding') class LocaleDecodeTest(unittest.TestCase): def test_can_decode_utf8_strings_with_french_content(self, mock): mock.return_value = 'UTF-8' - result = locale_decode( + result = encoding.locale_decode( b'[Errno 98] Adresse d\xc3\xa9j\xc3\xa0 utilis\xc3\xa9e') self.assertEqual('[Errno 98] Adresse d\xe9j\xe0 utilis\xe9e', result) @@ -22,7 +22,7 @@ class LocaleDecodeTest(unittest.TestCase): mock.return_value = 'UTF-8' error = IOError(98, b'Adresse d\xc3\xa9j\xc3\xa0 utilis\xc3\xa9e') - result = locale_decode(error) + result = encoding.locale_decode(error) expected = '[Errno 98] Adresse d\xe9j\xe0 utilis\xe9e' self.assertEqual( @@ -33,13 +33,13 @@ class LocaleDecodeTest(unittest.TestCase): def test_does_not_use_locale_to_decode_unicode_strings(self, mock): mock.return_value = 'UTF-8' - locale_decode('abc') + encoding.locale_decode('abc') self.assertFalse(mock.called) def test_does_not_use_locale_to_decode_ascii_bytestrings(self, mock): mock.return_value = 'UTF-8' - locale_decode('abc') + encoding.locale_decode('abc') self.assertFalse(mock.called) diff --git a/tests/utils/test_jsonrpc.py b/tests/internal/test_jsonrpc.py similarity index 99% rename from tests/utils/test_jsonrpc.py rename to tests/internal/test_jsonrpc.py index 160afc4d..b2103caa 100644 --- a/tests/utils/test_jsonrpc.py +++ b/tests/internal/test_jsonrpc.py @@ -8,7 +8,7 @@ import mock import pykka from mopidy import core, models -from mopidy.utils import deprecation, jsonrpc +from mopidy.internal import deprecation, jsonrpc from tests import dummy_backend diff --git a/tests/utils/test_path.py b/tests/internal/test_path.py similarity index 99% rename from tests/utils/test_path.py rename to tests/internal/test_path.py index 1acd7271..503d2490 100644 --- a/tests/utils/test_path.py +++ b/tests/internal/test_path.py @@ -10,7 +10,7 @@ import unittest import glib from mopidy import compat, exceptions -from mopidy.utils import path +from mopidy.internal import path import tests diff --git a/tests/utils/test_validation.py b/tests/internal/test_validation.py similarity index 99% rename from tests/utils/test_validation.py rename to tests/internal/test_validation.py index f211c003..a46a3b59 100644 --- a/tests/utils/test_validation.py +++ b/tests/internal/test_validation.py @@ -3,7 +3,7 @@ from __future__ import absolute_import, unicode_literals from pytest import raises from mopidy import compat, exceptions -from mopidy.utils import validation +from mopidy.internal import validation def test_check_boolean_with_valid_values(): diff --git a/tests/utils/test_xdg.py b/tests/internal/test_xdg.py similarity index 98% rename from tests/utils/test_xdg.py rename to tests/internal/test_xdg.py index eab595a4..521447f7 100644 --- a/tests/utils/test_xdg.py +++ b/tests/internal/test_xdg.py @@ -6,7 +6,7 @@ import mock import pytest -from mopidy.utils import xdg +from mopidy.internal import xdg @pytest.yield_fixture diff --git a/tests/local/__init__.py b/tests/local/__init__.py index 3841a1e4..7f3cfb33 100644 --- a/tests/local/__init__.py +++ b/tests/local/__init__.py @@ -1,6 +1,6 @@ from __future__ import absolute_import, unicode_literals -from mopidy.utils import deprecation +from mopidy.internal import deprecation def generate_song(i): diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py index 8aedcfbc..23e427d9 100644 --- a/tests/local/test_playback.py +++ b/tests/local/test_playback.py @@ -9,9 +9,9 @@ import pykka from mopidy import core from mopidy.core import PlaybackState +from mopidy.internal import deprecation from mopidy.local import actor from mopidy.models import TlTrack, Track -from mopidy.utils import deprecation from tests import dummy_audio, path_to_data_dir from tests.local import generate_song, populate_tracklist diff --git a/tests/local/test_tracklist.py b/tests/local/test_tracklist.py index a0add637..63ef8fde 100644 --- a/tests/local/test_tracklist.py +++ b/tests/local/test_tracklist.py @@ -7,9 +7,9 @@ import pykka from mopidy import core from mopidy.core import PlaybackState +from mopidy.internal import deprecation from mopidy.local import actor from mopidy.models import Playlist, Track -from mopidy.utils import deprecation from tests import dummy_audio, path_to_data_dir from tests.local import generate_song, populate_tracklist diff --git a/tests/m3u/test_playlists.py b/tests/m3u/test_playlists.py index a8caf8fd..f9a7f04a 100644 --- a/tests/m3u/test_playlists.py +++ b/tests/m3u/test_playlists.py @@ -8,10 +8,10 @@ import unittest import pykka from mopidy import core +from mopidy.internal import deprecation from mopidy.m3u import actor from mopidy.m3u.translator import playlist_uri_to_path from mopidy.models import Playlist, Track -from mopidy.utils import deprecation from tests import dummy_audio, path_to_data_dir from tests.m3u import generate_song diff --git a/tests/m3u/test_translator.py b/tests/m3u/test_translator.py index 32eb9f3b..cf0bf69f 100644 --- a/tests/m3u/test_translator.py +++ b/tests/m3u/test_translator.py @@ -6,9 +6,9 @@ import os import tempfile import unittest +from mopidy.internal import path from mopidy.m3u import translator from mopidy.models import Track -from mopidy.utils import path from tests import path_to_data_dir diff --git a/tests/mpd/protocol/__init__.py b/tests/mpd/protocol/__init__.py index 4b009407..e66bf88a 100644 --- a/tests/mpd/protocol/__init__.py +++ b/tests/mpd/protocol/__init__.py @@ -7,8 +7,8 @@ import mock import pykka from mopidy import core +from mopidy.internal import deprecation from mopidy.mpd import session, uri_mapper -from mopidy.utils import deprecation from tests import dummy_backend, dummy_mixer diff --git a/tests/mpd/protocol/test_current_playlist.py b/tests/mpd/protocol/test_current_playlist.py index 6ec53adc..3b7540b5 100644 --- a/tests/mpd/protocol/test_current_playlist.py +++ b/tests/mpd/protocol/test_current_playlist.py @@ -1,7 +1,7 @@ from __future__ import absolute_import, unicode_literals +from mopidy.internal import deprecation from mopidy.models import Ref, Track -from mopidy.utils import deprecation from tests.mpd import protocol diff --git a/tests/mpd/protocol/test_playback.py b/tests/mpd/protocol/test_playback.py index 6121f540..b9adb646 100644 --- a/tests/mpd/protocol/test_playback.py +++ b/tests/mpd/protocol/test_playback.py @@ -3,8 +3,8 @@ from __future__ import absolute_import, unicode_literals import unittest from mopidy.core import PlaybackState +from mopidy.internal import deprecation from mopidy.models import Track -from mopidy.utils import deprecation from tests.mpd import protocol diff --git a/tests/mpd/test_dispatcher.py b/tests/mpd/test_dispatcher.py index be2bf608..e5eec0f9 100644 --- a/tests/mpd/test_dispatcher.py +++ b/tests/mpd/test_dispatcher.py @@ -5,9 +5,9 @@ import unittest import pykka from mopidy import core +from mopidy.internal import deprecation from mopidy.mpd.dispatcher import MpdDispatcher from mopidy.mpd.exceptions import MpdAckError -from mopidy.utils import deprecation from tests import dummy_backend diff --git a/tests/mpd/test_status.py b/tests/mpd/test_status.py index f6390e53..d36ad4dc 100644 --- a/tests/mpd/test_status.py +++ b/tests/mpd/test_status.py @@ -6,10 +6,10 @@ import pykka from mopidy import core from mopidy.core import PlaybackState +from mopidy.internal import deprecation from mopidy.models import Track from mopidy.mpd import dispatcher from mopidy.mpd.protocol import status -from mopidy.utils import deprecation from tests import dummy_backend, dummy_mixer diff --git a/tests/mpd/test_translator.py b/tests/mpd/test_translator.py index 055932fc..646a22b2 100644 --- a/tests/mpd/test_translator.py +++ b/tests/mpd/test_translator.py @@ -2,9 +2,9 @@ from __future__ import absolute_import, unicode_literals import unittest +from mopidy.internal import path from mopidy.models import Album, Artist, Playlist, TlTrack, Track from mopidy.mpd import translator -from mopidy.utils.path import mtime class TrackMpdFormatTest(unittest.TestCase): @@ -27,10 +27,10 @@ class TrackMpdFormatTest(unittest.TestCase): def setUp(self): # noqa: N802 self.media_dir = '/dir/subdir' - mtime.set_fake_time(1234567) + path.mtime.set_fake_time(1234567) def tearDown(self): # noqa: N802 - mtime.undo_fake() + path.mtime.undo_fake() def test_track_to_mpd_format_for_empty_track(self): # TODO: this is likely wrong, see: diff --git a/tests/stream/test_library.py b/tests/stream/test_library.py index b2410bb7..3962159c 100644 --- a/tests/stream/test_library.py +++ b/tests/stream/test_library.py @@ -11,9 +11,9 @@ import pygst pygst.require('0.10') import gst # noqa: pygst magic is needed to import correct gst +from mopidy.internal import path from mopidy.models import Track from mopidy.stream import actor -from mopidy.utils.path import path_to_uri from tests import path_to_data_dir @@ -23,7 +23,7 @@ class LibraryProviderTest(unittest.TestCase): def setUp(self): # noqa: N802 self.backend = mock.Mock() self.backend.uri_schemes = ['file'] - self.uri = path_to_uri(path_to_data_dir('song1.wav')) + self.uri = path.path_to_uri(path_to_data_dir('song1.wav')) def test_lookup_ignores_unknown_scheme(self): library = actor.StreamLibraryProvider(self.backend, 1000, [], {}) @@ -34,7 +34,7 @@ class LibraryProviderTest(unittest.TestCase): self.assertEqual([Track(uri=self.uri)], library.lookup(self.uri)) def test_lookup_respects_blacklist_globbing(self): - blacklist = [path_to_uri(path_to_data_dir('')) + '*'] + blacklist = [path.path_to_uri(path_to_data_dir('')) + '*'] library = actor.StreamLibraryProvider(self.backend, 100, blacklist, {}) self.assertEqual([Track(uri=self.uri)], library.lookup(self.uri)) From e30cd2cfa59f125279b6bb9ba5abe8926519f5ec Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 8 May 2015 00:32:09 +0200 Subject: [PATCH 38/42] local: Rename local_{track_ => }uri_to_file_uri() --- mopidy/local/playback.py | 2 +- mopidy/local/translator.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mopidy/local/playback.py b/mopidy/local/playback.py index 24038426..a851239d 100644 --- a/mopidy/local/playback.py +++ b/mopidy/local/playback.py @@ -7,5 +7,5 @@ from mopidy.local import translator class LocalPlaybackProvider(backend.PlaybackProvider): def translate_uri(self, uri): - return translator.local_track_uri_to_file_uri( + return translator.local_uri_to_file_uri( uri, self.backend.config['local']['media_dir']) diff --git a/mopidy/local/translator.py b/mopidy/local/translator.py index 6e5c9c01..4e248fd1 100644 --- a/mopidy/local/translator.py +++ b/mopidy/local/translator.py @@ -11,7 +11,7 @@ from mopidy.internal import path logger = logging.getLogger(__name__) -def local_track_uri_to_file_uri(uri, media_dir): +def local_uri_to_file_uri(uri, media_dir): return path.path_to_uri(local_track_uri_to_path(uri, media_dir)) From 4d5b48576073e04f675f6b24adf3ad57a2ef96df Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 8 May 2015 00:33:13 +0200 Subject: [PATCH 39/42] local: Add local_uri_to_file_uri() Which replaces local_track_uri_to_file_uri() and also handles local:directory: URIs. --- mopidy/local/translator.py | 15 ++++++-- tests/local/test_translator.py | 68 ++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 tests/local/test_translator.py diff --git a/mopidy/local/translator.py b/mopidy/local/translator.py index 4e248fd1..371643e4 100644 --- a/mopidy/local/translator.py +++ b/mopidy/local/translator.py @@ -12,16 +12,25 @@ logger = logging.getLogger(__name__) def local_uri_to_file_uri(uri, media_dir): - return path.path_to_uri(local_track_uri_to_path(uri, media_dir)) + """Convert local track or directory URI to file URI.""" + return path.path_to_uri(local_uri_to_path(uri, media_dir)) -def local_track_uri_to_path(uri, media_dir): - if not uri.startswith('local:track:'): +def local_uri_to_path(uri, media_dir): + """Convert local track or directory URI to absolute path.""" + if ( + not uri.startswith('local:directory:') and + not uri.startswith('local:track:')): raise ValueError('Invalid URI.') file_path = path.uri_to_path(uri).split(b':', 1)[1] return os.path.join(media_dir, file_path) +def local_track_uri_to_path(uri, media_dir): + # Deprecated version to keep old versions of Mopidy-Local-Sqlite working. + return local_uri_to_path(uri, media_dir) + + def path_to_local_track_uri(relpath): """Convert path relative to media_dir to local track URI.""" if isinstance(relpath, compat.text_type): diff --git a/tests/local/test_translator.py b/tests/local/test_translator.py new file mode 100644 index 00000000..1a54ae83 --- /dev/null +++ b/tests/local/test_translator.py @@ -0,0 +1,68 @@ +# encoding: utf-8 + +from __future__ import unicode_literals + +import pytest + +from mopidy.local import translator + + +@pytest.mark.parametrize('local_uri,file_uri', [ + ('local:directory:A/B', 'file:///home/alice/Music/A/B'), + ('local:directory:A%20B', 'file:///home/alice/Music/A%20B'), + ('local:directory:A+B', 'file:///home/alice/Music/A%2BB'), + ( + 'local:directory:%C3%A6%C3%B8%C3%A5', + 'file:///home/alice/Music/%C3%A6%C3%B8%C3%A5'), + ('local:track:A/B.mp3', 'file:///home/alice/Music/A/B.mp3'), + ('local:track:A%20B.mp3', 'file:///home/alice/Music/A%20B.mp3'), + ('local:track:A+B.mp3', 'file:///home/alice/Music/A%2BB.mp3'), + ( + 'local:track:%C3%A6%C3%B8%C3%A5.mp3', + 'file:///home/alice/Music/%C3%A6%C3%B8%C3%A5.mp3'), +]) +def test_local_uri_to_file_uri(local_uri, file_uri): + media_dir = b'/home/alice/Music' + + assert translator.local_uri_to_file_uri(local_uri, media_dir) == file_uri + + +@pytest.mark.parametrize('uri', [ + 'A/B', + 'local:foo:A/B', +]) +def test_local_uri_to_file_uri_errors(uri): + media_dir = b'/home/alice/Music' + + with pytest.raises(ValueError): + translator.local_uri_to_file_uri(uri, media_dir) + + +@pytest.mark.parametrize('uri,path', [ + ('local:directory:A/B', b'/home/alice/Music/A/B'), + ('local:directory:A%20B', b'/home/alice/Music/A B'), + ('local:directory:A+B', b'/home/alice/Music/A+B'), + ('local:directory:%C3%A6%C3%B8%C3%A5', b'/home/alice/Music/æøå'), + ('local:track:A/B.mp3', b'/home/alice/Music/A/B.mp3'), + ('local:track:A%20B.mp3', b'/home/alice/Music/A B.mp3'), + ('local:track:A+B.mp3', b'/home/alice/Music/A+B.mp3'), + ('local:track:%C3%A6%C3%B8%C3%A5.mp3', b'/home/alice/Music/æøå.mp3'), +]) +def test_local_uri_to_path(uri, path): + media_dir = b'/home/alice/Music' + + assert translator.local_uri_to_path(uri, media_dir) == path + + # Legacy version to keep old versions of Mopidy-Local-Sqlite working + assert translator.local_track_uri_to_path(uri, media_dir) == path + + +@pytest.mark.parametrize('uri', [ + 'A/B', + 'local:foo:A/B', +]) +def test_local_uri_to_path_errors(uri): + media_dir = b'/home/alice/Music' + + with pytest.raises(ValueError): + translator.local_uri_to_path(uri, media_dir) From 56cffa00892438110e0d7c51ca667a3cf51a58ad Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 8 May 2015 00:37:07 +0200 Subject: [PATCH 40/42] local: Test path_to_local_{directory,track}() --- tests/local/test_translator.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/local/test_translator.py b/tests/local/test_translator.py index 1a54ae83..afe0b340 100644 --- a/tests/local/test_translator.py +++ b/tests/local/test_translator.py @@ -66,3 +66,23 @@ def test_local_uri_to_path_errors(uri): with pytest.raises(ValueError): translator.local_uri_to_path(uri, media_dir) + + +@pytest.mark.parametrize('path,uri', [ + ('foo', 'local:track:foo'), + (b'foo', 'local:track:foo'), + ('æøå', 'local:track:%C3%A6%C3%B8%C3%A5'), + (b'\x00\x01\x02', 'local:track:%00%01%02'), +]) +def test_path_to_local_track_uri(path, uri): + assert translator.path_to_local_track_uri(path) == uri + + +@pytest.mark.parametrize('path,uri', [ + ('foo', 'local:directory:foo'), + (b'foo', 'local:directory:foo'), + ('æøå', 'local:directory:%C3%A6%C3%B8%C3%A5'), + (b'\x00\x01\x02', 'local:directory:%00%01%02'), +]) +def test_path_to_local_directory_uri(path, uri): + assert translator.path_to_local_directory_uri(path) == uri From c59784c1e84f3d520c4cd7d3d3fe668f39348e54 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 8 May 2015 00:42:25 +0200 Subject: [PATCH 41/42] local: Add path_to_file_uri() --- mopidy/local/translator.py | 8 +++++++- tests/local/test_translator.py | 10 ++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/mopidy/local/translator.py b/mopidy/local/translator.py index 371643e4..856e9d4c 100644 --- a/mopidy/local/translator.py +++ b/mopidy/local/translator.py @@ -13,7 +13,7 @@ logger = logging.getLogger(__name__) def local_uri_to_file_uri(uri, media_dir): """Convert local track or directory URI to file URI.""" - return path.path_to_uri(local_uri_to_path(uri, media_dir)) + return path_to_file_uri(local_uri_to_path(uri, media_dir)) def local_uri_to_path(uri, media_dir): @@ -31,6 +31,12 @@ def local_track_uri_to_path(uri, media_dir): return local_uri_to_path(uri, media_dir) +def path_to_file_uri(abspath): + """Convert absolute path to file URI.""" + # Re-export internal method for use by Mopidy-Local-* extensions. + return path.path_to_uri(abspath) + + def path_to_local_track_uri(relpath): """Convert path relative to media_dir to local track URI.""" if isinstance(relpath, compat.text_type): diff --git a/tests/local/test_translator.py b/tests/local/test_translator.py index afe0b340..124766dd 100644 --- a/tests/local/test_translator.py +++ b/tests/local/test_translator.py @@ -68,6 +68,16 @@ def test_local_uri_to_path_errors(uri): translator.local_uri_to_path(uri, media_dir) +@pytest.mark.parametrize('path,uri', [ + ('/foo', 'file:///foo'), + (b'/foo', 'file:///foo'), + ('/æøå', 'file:///%C3%A6%C3%B8%C3%A5'), + (b'/\x00\x01\x02', 'file:///%00%01%02'), +]) +def test_path_to_file_uri(path, uri): + assert translator.path_to_file_uri(path) == uri + + @pytest.mark.parametrize('path,uri', [ ('foo', 'local:track:foo'), (b'foo', 'local:track:foo'), From 64b5342c51a927b5fa32150d00e26968a6651af0 Mon Sep 17 00:00:00 2001 From: Stein Magnus Jodal Date: Fri, 8 May 2015 00:48:31 +0200 Subject: [PATCH 42/42] docs: Document mopidy.local.translator --- docs/modules/local.rst | 14 ++++++++++++++ mopidy/local/translator.py | 3 ++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/docs/modules/local.rst b/docs/modules/local.rst index 014bac2a..395e8802 100644 --- a/docs/modules/local.rst +++ b/docs/modules/local.rst @@ -6,4 +6,18 @@ For details on how to use Mopidy's local backend, see :ref:`ext-local`. .. automodule:: mopidy.local :synopsis: Local backend + + +Local library API +================= + +.. autoclass:: mopidy.local.Library + :members: + + +Translation utils +================= + +.. automodule:: mopidy.local.translator + :synopsis: Translators for local library extensions :members: diff --git a/mopidy/local/translator.py b/mopidy/local/translator.py index 856e9d4c..6fc53f63 100644 --- a/mopidy/local/translator.py +++ b/mopidy/local/translator.py @@ -38,7 +38,8 @@ def path_to_file_uri(abspath): def path_to_local_track_uri(relpath): - """Convert path relative to media_dir to local track URI.""" + """Convert path relative to :confval:`local/media_dir` to local track + URI.""" if isinstance(relpath, compat.text_type): relpath = relpath.encode('utf-8') return b'local:track:%s' % urllib.quote(relpath)