From dd4a8f3b785641edad70a89d5a7281011c3e2167 Mon Sep 17 00:00:00 2001 From: Thomas Adamcik Date: Tue, 5 May 2015 22:55:53 +0200 Subject: [PATCH] core: Make sure library can handle bad data from backends Note that None values are just ignored, while other bad data logs an error message and is ignored. --- mopidy/core/library.py | 93 ++++++++++++++++---------- tests/core/test_library.py | 130 ++++++++++++++++++++++++++++++++++--- 2 files changed, 181 insertions(+), 42 deletions(-) diff --git a/mopidy/core/library.py b/mopidy/core/library.py index d9803d3f..1ca21457 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -1,16 +1,30 @@ 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): + try: + yield + except exceptions.ValidationError as e: + logger.error('%s backend returned bad data: %s', + backend.actor_ref.actor_class.__name__, e) + except Exception: + logger.exception('%s backend caused an exception.', + backend.actor_ref.actor_class.__name__) + + class LibraryController(object): pykka_traversable = True @@ -79,22 +93,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: - directories.add(future.get()) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + root = future.get() + validation.check_instance(root, models.Ref) + directories.add(root) 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: - return backend.library.browse(uri).get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + + if not backend: + return [] + + with _backend_error_handling(backend): + result = backend.library.browse(uri).get() + validation.check_instances(result, models.Ref) + return result + return [] def get_distinct(self, field, query=None): @@ -120,11 +136,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: - result.update(future.get()) - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + values = future.get() + if values is not None: + validation.check_instances(values, compat.text_type) + result.update(values) return result def get_images(self, uris): @@ -152,12 +168,18 @@ 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: + name = backend.actor_ref.actor_class.__name__ + logger.warning( + '%s backend returned image for URI we did not ' + 'ask for: %s', name, 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: - result[u] = future.get() - except Exception: - logger.exception('%s backend caused an exception.', - backend.actor_ref.actor_class.__name__) + with _backend_error_handling(backend): + result = future.get() + if result is not None: + validation.check_instances(result, models.Track) + results[u] = result if uri: - return result[uri] - return result + return results[uri] + 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): """ @@ -313,8 +332,14 @@ class LibraryController(object): results = [] for backend, future in futures.items(): - try: - results.append(future.get()) + 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) except TypeError: backend_name = backend.actor_ref.actor_class.__name__ logger.warning( diff --git a/tests/core/test_library.py b/tests/core/test_library.py index 89f3b284..527e5272 100644 --- a/tests/core/test_library.py +++ b/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().get.return_value = {} self.library1.get_images.reset_mock() @@ -24,6 +25,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().get.return_value = {} self.library2.get_images.reset_mock() @@ -33,6 +35,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 @@ -156,11 +159,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] - self.library2.lookup().get.return_value = [5678] + track1 = Track(name='abc') + 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']) @@ -363,12 +369,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) @@ -443,28 +451,124 @@ class BackendFailuresCoreLibraryTest(unittest.TestCase): self.assertEqual([], self.core.library.browse(None)) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + def test_browse_backend_get_root_bad_value(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_browse_backend_get_root_none(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_browse_backend_browse_uri_exception_gets_ignored(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_browse_backend_browse_uri_bad_value(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) + def test_get_distinct_backend_exception_gets_ignored(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_distinct_backend_returns_string(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_get_distinct_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_get_distinct_backend_returns_list_if_ints(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) + def test_get_images_backend_exception_get_ignored(self, logger): + uri = 'dummy:/1' self.library.get_images.return_value.get.side_effect = Exception - self.assertEqual( - {'dummy:/1': tuple()}, self.core.library.get_images(['dummy:/1'])) + self.assertEqual({uri: tuple()}, self.core.library.get_images([uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') - def test_lookup_backend_exceptiosn_gets_ignores(self, logger): + def test_get_images_backend_returns_string(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_get_images_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_get_images_backend_returns_bad_dict(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_get_images_backend_returns_dict_with_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_get_images_backend_returns_wrong_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.warning.assert_called_with(mock.ANY, 'DummyBackend', 'foo') + + def test_lookup_backend_exceptions_gets_ignored(self, logger): + uri = 'dummy:/1' self.library.lookup.return_value.get.side_effect = Exception - self.assertEqual( - {'dummy:/1': []}, self.core.library.lookup(uris=['dummy:/1'])) + self.assertEqual({uri: []}, self.core.library.lookup(uris=[uri])) logger.exception.assert_called_with(mock.ANY, 'DummyBackend') + def test_lookup_uris_backend_returns_string(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_lookup_uris_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_lookup_uris_backend_returns_bad_list(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_lookup_uri_backend_returns_string(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_lookup_uri_backend_returns_none(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_lookup_uri_backend_returns_bad_list(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) + def test_refresh_backend_exception_gets_ignored(self, logger): self.library.refresh.return_value.get.side_effect = Exception self.core.library.refresh() @@ -486,3 +590,13 @@ class BackendFailuresCoreLibraryTest(unittest.TestCase): self.library.search.return_value.get.side_effect = LookupError with self.assertRaises(LookupError): self.core.library.search(query={'any': ['foo']}) + + def test_search_backend_returns_string(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) + + def test_search_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)