Merge pull request #1163 from adamcik/feature/core-dont-trust-backends

Don't trust backends...

docs/changelog.rst

@@ -30,6 +30,8 @@ Core API
 - Add :meth:`mopidy.core.playback.PlaybackController.get_current_tlid`.
   (Part of: :issue:`1137`)
 
+- Update core to handle backend crashes and bad data. (Fixes: :issue:`1161`)
+
 Models
 ------
 

mopidy/core/library.py

@@ -1,16 +1,32 @@
 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, 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 LibraryController(object):
     pykka_traversable = True
 
@@ -79,22 +95,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:
+            with _backend_error_handling(backend):
-                directories.add(future.get())
+                root = future.get()
-            except Exception:
+                validation.check_instance(root, models.Ref)
-                logger.exception('%s backend caused an exception.',
+                directories.add(root)
-                                 backend.actor_ref.actor_class.__name__)
         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:
+        if not backend:
-                return backend.library.browse(uri).get()
+            return []
-        except Exception:
+
-            logger.exception('%s backend caused an exception.',
+        with _backend_error_handling(backend):
-                             backend.actor_ref.actor_class.__name__)
+            result = backend.library.browse(uri).get()
+            validation.check_instances(result, models.Ref)
+            return result
+
         return []
 
     def get_distinct(self, field, query=None):
@@ -120,11 +138,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:
+            with _backend_error_handling(backend):
-                result.update(future.get())
+                values = future.get()
-            except Exception:
+                if values is not None:
-                logger.exception('%s backend caused an exception.',
+                    validation.check_instances(values, compat.text_type)
-                                 backend.actor_ref.actor_class.__name__)
+                    result.update(values)
         return result
 
     def get_images(self, uris):
@@ -152,12 +170,16 @@ 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:
+                        raise exceptions.ValidationError(
+                            'Got unknown image URI: %s' % 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:
+            with _backend_error_handling(backend):
-                result[u] = future.get()
+                result = future.get()
-            except Exception:
+                if result is not None:
-                logger.exception('%s backend caused an exception.',
+                    validation.check_instances(result, models.Track)
-                                 backend.actor_ref.actor_class.__name__)
+                    results[u] = result
 
         if uri:
-            return result[uri]
+            return results[uri]
-        return result
+        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):
         """
@@ -311,25 +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:
-                results.append(future.get())
+                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):

mopidy/core/mixer.py

@@ -1,13 +1,27 @@
 from __future__ import absolute_import, unicode_literals
 
+import contextlib
 import logging
 
+from mopidy import exceptions
 from mopidy.utils import validation
 
 
 logger = logging.getLogger(__name__)
 
 
+@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__)
+
+
 class MixerController(object):
     pykka_traversable = True
 
@@ -21,8 +35,15 @@ class MixerController(object):
 
         The volume scale is linear.
         """
-        if self._mixer is not None:
+        if self._mixer is None:
-            return self._mixer.get_volume().get()
+            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.
@@ -36,9 +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
-        else:
+
-            return self._mixer.set_volume(volume).get()
+        with _mixer_error_handling(self._mixer):
+            result = self._mixer.set_volume(volume).get()
+            validation.check_instance(result, bool)
+            return result
+
+        return False
 
     def get_mute(self):
         """Get mute state.
@@ -46,8 +72,15 @@ class MixerController(object):
         :class:`True` if muted, :class:`False` unmuted, :class:`None` if
         unknown.
         """
-        if self._mixer is not None:
+        if self._mixer is None:
-            return self._mixer.get_mute().get()
+            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.
@@ -58,6 +91,11 @@ class MixerController(object):
         """
         validation.check_boolean(mute)
         if self._mixer is None:
-            return False
+            return False  # TODO: 2.0 return None
-        else:
+
-            return self._mixer.set_mute(bool(mute)).get()
+        with _mixer_error_handling(self._mixer):
+            result = self._mixer.set_mute(bool(mute)).get()
+            validation.check_instance(result, bool)
+            return result
+
+        return False

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
 
@@ -34,17 +50,18 @@ 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:
-                results.extend(future.get())
+                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)
-            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):
         """
@@ -120,22 +145,23 @@ 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:
             backends = [self.backends.with_playlists[uri_scheme]]
         else:
             backends = self.backends.with_playlists.values()
+
         for backend in backends:
-            try:
+            with _backend_error_handling(backend):
-                playlist = backend.playlists.create(name).get()
+                result = backend.playlists.create(name).get()
-            except Exception:
+                if result is None:
-                playlist = None
+                    continue
-            # Workaround for playlist providers that return None from create()
+                validation.check_instance(result, Playlist)
-            if not playlist:
+                listener.CoreListener.send('playlist_changed', playlist=result)
-                continue
+                return result
-            listener.CoreListener.send('playlist_changed', playlist=playlist)
+
-            return playlist
+        return None
 
     def delete(self, uri):
         """
@@ -151,8 +177,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):
         """
@@ -198,11 +230,16 @@ class PlaylistsController(object):
         """
         uri_scheme = urlparse.urlparse(uri).scheme
         backend = self.backends.with_playlists.get(uri_scheme, None)
-        if backend:
+        if not backend:
-            return backend.playlists.lookup(uri).get()
-        else:
             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):
@@ -231,12 +268,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')
@@ -270,7 +304,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, reraise=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

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.return_value.get.return_value = {}
         self.library1.root_directory.get.return_value = dummy1_root
@@ -23,6 +24,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.return_value.get.return_value = {}
         self.library2.root_directory.get.return_value = dummy2_root
@@ -31,6 +33,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
 
@@ -148,11 +151,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]
+        track1 = Track(name='abc')
-        self.library2.lookup().get.return_value = [5678]
+        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'])
@@ -349,12 +355,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)
@@ -407,8 +415,7 @@ class LegacyFindExactToSearchLibraryTest(unittest.TestCase):
         # We are just testing that this doesn't fail.
 
 
-@mock.patch('mopidy.core.library.logger')
+class MockBackendCoreLibraryBase(unittest.TestCase):
-class BackendFailuresCoreLibraryTest(unittest.TestCase):
 
     def setUp(self):  # noqa: N802
         dummy_root = Ref.directory(uri='dummy:directory', name='dummy')
@@ -423,52 +430,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_browse_uri_exception_gets_ignored(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_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_get_distinct_backend_exception_gets_ignored(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)
+
+
+@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_images_backend_exception_get_ignored(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_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)
+
+
+@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(
+        self.assertEqual({uri: tuple()}, self.core.library.get_images([uri]))
-            {'dummy:/1': tuple()}, self.core.library.get_images(['dummy:/1']))
         logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
 
-    def test_lookup_backend_exceptiosn_gets_ignores(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_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_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_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.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY)
+
+
+@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(
+        self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri]))
-            {'dummy:/1': []}, self.core.library.lookup(uris=['dummy:/1']))
         logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
 
-    def test_refresh_backend_exception_gets_ignored(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_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_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_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)
+
+
+@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_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)

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)
@@ -92,3 +94,63 @@ 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 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=[])
+
+
+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_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_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_backend_returns_wrong_type(self):
+        self.mixer.get_volume.return_value.get.return_value = '12'
+        self.assertEqual(self.core.mixer.get_volume(), None)
+
+
+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_backend_returns_wrong_type(self):
+        self.mixer.set_volume.return_value.get.return_value = 'done'
+        self.assertFalse(self.core.mixer.set_volume(30))
+
+
+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_backend_returns_wrong_type(self):
+        self.mixer.get_mute.return_value.get.return_value = '12'
+        self.assertEqual(self.core.mixer.get_mute(), None)
+
+
+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_backend_returns_wrong_type(self):
+        self.mixer.set_mute.return_value.get.return_value = 'done'
+        self.assertFalse(self.core.mixer.set_mute(True))

tests/core/test_playlists.py

@@ -298,8 +298,7 @@ class DeprecatedGetPlaylistsTest(BasePlaylistsTest):
         self.assertEqual(len(result[1].tracks), 0)
 
 
-@mock.patch('mopidy.core.playlists.logger')
+class MockBackendCorePlaylistsBase(unittest.TestCase):
-class BackendFailuresCorePlaylistsTest(unittest.TestCase):
 
     def setUp(self):  # noqa: N802
         self.playlists = mock.Mock(spec=backend.PlaylistsProvider)
@@ -311,27 +310,127 @@ 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
+@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_get_items_backend_exception_gets_through(self, logger):
+    def test_backend_returns_none(self, logger):
-        # TODO: is this behavior desired?
+        self.playlists.as_list.return_value.get.return_value = None
+        self.assertEqual([], self.core.playlists.as_list())
+        self.assertFalse(logger.error.called)
+
+    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)
+
+
+@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
-        with self.assertRaises(Exception):
+        self.assertIsNone(self.core.playlists.get_items('dummy:/1'))
-            self.core.playlists.get_items('dummy:/1')
+        logger.exception.assert_called_with(mock.ANY, 'DummyBackend')
+
+    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_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)
+
+
+@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_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_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)
+
+
+@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')
+
+
+@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_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_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')
+
+
+@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_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_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))
+        logger.error.assert_called_with(mock.ANY, 'DummyBackend', mock.ANY)