From dd4a8f3b785641edad70a89d5a7281011c3e2167 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 5 May 2015 22:55:53 +0200 Subject: [PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] 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 5/7] 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 c01f8679bc6e1140c46e5b02e488e709c93d3a8e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 22:34:44 +0200 Subject: [PATCH 6/7] 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 29c66f7bc8856c28bb1f3dae2a8c44a22e412ed7 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Thu, 7 May 2015 00:13:58 +0200 Subject: [PATCH 7/7] 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))