From c01f8679bc6e1140c46e5b02e488e709c93d3a8e Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Wed, 6 May 2015 22:34:44 +0200 Subject: [PATCH] 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)