core: Address review comments for do not trust backends PR

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__
+                        raise exceptions.ValidationError(
-                        logger.warning(
+                            'Got unknown image URI: %s' % uri)
-                            '%s backend returned image for URI we did not '
-                            'ask for: %s', name, 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
+            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 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(
                     '%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):

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?
+            result = self._mixer.set_volume(volume).get()
-            return bool(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?
+            result = self._mixer.set_mute(bool(mute)).get()
-            return bool(self._mixer.set_mute(bool(mute)).get())
+            validation.check_instance(result, bool)
+            return result
 
         return None

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:

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')

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)