core: Update playlists to handle bad data from backends and exceptions
This commit is contained in:
Thomas Adamcik
parent
3426633c78
commit
e7b241e18b
@ -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
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
Reference in New Issue
Block a user