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