diff --git a/mopidy/core/library.py b/mopidy/core/library.py index 6fc1ce38..5971ec6e 100644 --- a/mopidy/core/library.py +++ b/mopidy/core/library.py @@ -5,8 +5,7 @@ import logging import operator import urlparse - -from mopidy.utils import deprecation +from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) @@ -70,6 +69,9 @@ class LibraryController(object): """ if uri is None: return self._roots() + elif not uri.strip(): + return [] + validation.check_uri(uri) return self._browse(uri) def _roots(self): @@ -111,6 +113,9 @@ class LibraryController(object): .. versionadded:: 1.0 """ + validation.check_choice(field, validation.DISTINCT_FIELDS) + query is None or validation.check_query(query) # TODO: normalize? + result = set() futures = {b: b.library.get_distinct(field, query) for b in self.backends.with_library.values()} @@ -137,6 +142,8 @@ class LibraryController(object): .. versionadded:: 1.0 """ + validation.check_uris(uris) + futures = { backend: backend.library.get_images(backend_uris) for (backend, backend_uris) @@ -187,6 +194,11 @@ class LibraryController(object): if none_set or both_set: raise ValueError("One of 'uri' or 'uris' must be set") + # TODO: validation.one_of(*args)? + + uris is None or validation.check_uris(uris) + uri is None or validation.check_uri(uri) + if uri: deprecation.warn('core.library.lookup:uri_arg') @@ -219,6 +231,8 @@ class LibraryController(object): :param uri: directory or track URI :type uri: string """ + uri is None or validation.check_uri(uri) + futures = {} backends = {} uri_scheme = urlparse.urlparse(uri).scheme if uri else None @@ -285,6 +299,10 @@ class LibraryController(object): """ query = _normalize_query(query or kwargs) + uris is None or validation.check_uris(uris) + query is None or validation.check_query(query) + validation.check_boolean(exact) + if kwargs: deprecation.warn('core.library.search:kwargs_query') @@ -319,6 +337,7 @@ class LibraryController(object): def _normalize_query(query): broken_client = False + # TODO: this breaks if query is not a dictionary like object... for (field, values) in query.items(): if isinstance(values, basestring): broken_client = True diff --git a/mopidy/core/mixer.py b/mopidy/core/mixer.py index 3388d706..fde7ee5a 100644 --- a/mopidy/core/mixer.py +++ b/mopidy/core/mixer.py @@ -2,6 +2,8 @@ from __future__ import absolute_import, unicode_literals import logging +from mopidy.utils import validation + logger = logging.getLogger(__name__) @@ -31,6 +33,8 @@ class MixerController(object): Returns :class:`True` if call is successful, otherwise :class:`False`. """ + validation.check_integer(volume, min=0, max=100) + if self._mixer is None: return False else: @@ -52,6 +56,7 @@ class MixerController(object): Returns :class:`True` if call is successful, otherwise :class:`False`. """ + validation.check_boolean(mute) if self._mixer is None: return False else: diff --git a/mopidy/core/playback.py b/mopidy/core/playback.py index abf9ae8a..135e1828 100644 --- a/mopidy/core/playback.py +++ b/mopidy/core/playback.py @@ -3,9 +3,10 @@ from __future__ import absolute_import, unicode_literals import logging import urlparse +from mopidy import models from mopidy.audio import PlaybackState from mopidy.core import listener -from mopidy.utils import deprecation +from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) @@ -96,6 +97,8 @@ class PlaybackController(object): "PAUSED" -> "PLAYING" [ label="resume" ] "PAUSED" -> "STOPPED" [ label="stop" ] """ + validation.check_choice(new_state, validation.PLAYBACK_STATES) + (old_state, self._state) = (self.get_state(), new_state) logger.debug('Changing state: %s -> %s', old_state, new_state) @@ -270,6 +273,7 @@ class PlaybackController(object): :param tl_track: track to play :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` """ + tl_track is None or validation.check_instance(tl_track, models.TlTrack) self._play(tl_track, on_error_step=1) def _play(self, tl_track=None, on_error_step=1): @@ -360,6 +364,8 @@ class PlaybackController(object): :type time_position: int :rtype: :class:`True` if successful, else :class:`False` """ + validation.check_integer(time_position, min=0) + if not self.core.tracklist.tracks: return False diff --git a/mopidy/core/playlists.py b/mopidy/core/playlists.py index 62001517..a430e3ff 100644 --- a/mopidy/core/playlists.py +++ b/mopidy/core/playlists.py @@ -5,7 +5,7 @@ import urlparse from mopidy.core import listener from mopidy.models import Playlist -from mopidy.utils import deprecation +from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) @@ -62,6 +62,8 @@ class PlaylistsController(object): .. versionadded:: 1.0 """ + validation.check_uri(uri) + uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) if backend: @@ -139,6 +141,8 @@ class PlaylistsController(object): :param uri: URI of the playlist to delete :type uri: string """ + validation.check_uri(uri) + uri_scheme = urlparse.urlparse(uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) if backend: @@ -172,6 +176,9 @@ class PlaylistsController(object): deprecation.warn('core.playlists.filter') criteria = criteria or kwargs + validation.check_query(criteria, list_values=False) + + # TODO: stop using self playlists matches = self.playlists for (key, value) in criteria.iteritems(): matches = filter(lambda p: getattr(p, key) == value, matches) @@ -207,6 +214,8 @@ class PlaylistsController(object): :param uri_scheme: limit to the backend matching the URI scheme :type uri_scheme: string """ + # TODO: check: uri_scheme is None or uri_scheme? + futures = {} backends = {} playlists_loaded = False @@ -251,8 +260,11 @@ class PlaylistsController(object): :type playlist: :class:`mopidy.models.Playlist` :rtype: :class:`mopidy.models.Playlist` or :class:`None` """ + validation.check_instance(playlist, Playlist) + if playlist.uri is None: - return + return # TODO: log this problem? + uri_scheme = urlparse.urlparse(playlist.uri).scheme backend = self.backends.with_playlists.get(uri_scheme, None) if backend: diff --git a/mopidy/core/tracklist.py b/mopidy/core/tracklist.py index 9a251b75..359efa0c 100644 --- a/mopidy/core/tracklist.py +++ b/mopidy/core/tracklist.py @@ -1,13 +1,11 @@ from __future__ import absolute_import, unicode_literals -import collections import logging import random -from mopidy import compat from mopidy.core import listener -from mopidy.models import TlTrack -from mopidy.utils import deprecation +from mopidy.models import TlTrack, Track +from mopidy.utils import deprecation, validation logger = logging.getLogger(__name__) @@ -93,6 +91,7 @@ class TracklistController(object): :class:`False` Tracks are not removed from the tracklist. """ + validation.check_boolean(value) if self.get_consume() != value: self._trigger_options_changed() return setattr(self, '_consume', value) @@ -121,7 +120,7 @@ class TracklistController(object): :class:`False` Tracks are played in the order of the tracklist. """ - + validation.check_boolean(value) if self.get_random() != value: self._trigger_options_changed() if value: @@ -157,7 +156,7 @@ class TracklistController(object): :class:`False` The tracklist is played once. """ - + validation.check_boolean(value) if self.get_repeat() != value: self._trigger_options_changed() return setattr(self, '_repeat', value) @@ -188,6 +187,7 @@ class TracklistController(object): :class:`False` Playback continues after current song. """ + validation.check_boolean(value) if self.get_single() != value: self._trigger_options_changed() return setattr(self, '_single', value) @@ -205,9 +205,10 @@ class TracklistController(object): The position of the given track in the tracklist. :param tl_track: the track to find the index of - :type tl_track: :class:`mopidy.models.TlTrack` + :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`int` or :class:`None` """ + tl_track is None or validation.check_instance(tl_track, TlTrack) try: return self._tl_tracks.index(tl_track) except ValueError: @@ -223,6 +224,7 @@ class TracklistController(object): :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`mopidy.models.TlTrack` or :class:`None` """ + tl_track is None or validation.check_instance(tl_track, TlTrack) if self.get_single() and self.get_repeat(): return tl_track elif self.get_single(): @@ -247,6 +249,7 @@ class TracklistController(object): :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`mopidy.models.TlTrack` or :class:`None` """ + tl_track is None or validation.check_instance(tl_track, TlTrack) if not self.get_tl_tracks(): return None @@ -288,6 +291,8 @@ class TracklistController(object): :type tl_track: :class:`mopidy.models.TlTrack` or :class:`None` :rtype: :class:`mopidy.models.TlTrack` or :class:`None` """ + tl_track is None or validation.check_instance(tl_track, TlTrack) + if self.get_repeat() or self.get_consume() or self.get_random(): return tl_track @@ -330,8 +335,13 @@ class TracklistController(object): """ assert tracks is not None or uri is not None or uris is not None, \ 'tracks, uri or uris must be provided' + # TODO: check that only one of tracks uri and uris is set... + # TODO: can at_position be negative? - # TODO: assert that tracks are track instances + tracks is None or validation.check_instances(tracks, Track) + uri is None or validation.check_uri(uri) + uris is None or validation.check_uris(uris) + validation.check_integer(at_position or 0) if tracks: deprecation.warn('core.tracklist.add:tracks_arg') @@ -412,12 +422,11 @@ class TracklistController(object): :rtype: list of :class:`mopidy.models.TlTrack` """ criteria = criteria or kwargs + validation.check_query(criteria) + # TODO: deprecate kwargs + matches = self._tl_tracks for (key, values) in criteria.items(): - if (not isinstance(values, collections.Iterable) or - isinstance(values, compat.string_types)): - # Fail hard if anyone is using the <0.17 calling style - raise ValueError('Filter values must be iterable: %r' % values) if key == 'tlid': matches = [ct for ct in matches if ct.tlid in values] else: @@ -443,6 +452,7 @@ class TracklistController(object): tl_tracks = self._tl_tracks + # TODO: use validation helpers? assert start < end, 'start must be smaller than end' assert start >= 0, 'start must be at least zero' assert end <= len(tl_tracks), \ @@ -470,6 +480,7 @@ class TracklistController(object): :type criteria: dict :rtype: list of :class:`mopidy.models.TlTrack` that was removed """ + # TODO: deprecate kwargs tl_tracks = self.filter(criteria, **kwargs) for tl_track in tl_tracks: position = self._tl_tracks.index(tl_track) @@ -491,6 +502,7 @@ class TracklistController(object): """ tl_tracks = self._tl_tracks + # TOOD: use validation helpers? if start is not None and end is not None: assert start < end, 'start must be smaller than end' @@ -519,6 +531,7 @@ class TracklistController(object): :type end: int :rtype: :class:`mopidy.models.TlTrack` """ + # TODO: validate slice? return self._tl_tracks[start:end] def _mark_playing(self, tl_track): diff --git a/mopidy/utils/validation.py b/mopidy/utils/validation.py index a0306564..25e9f227 100644 --- a/mopidy/utils/validation.py +++ b/mopidy/utils/validation.py @@ -74,12 +74,12 @@ def check_query(arg, list_values=True): check_choice(key, QUERY_FIELDS, msg='Expected query field to be one ' 'of {choices}, not {arg!r}') if list_values: - msg = 'Expected "{key}" values to be list of strings, not {arg!r}' + msg = 'Expected "{key}" to be list of strings, not {arg!r}' _check_iterable(value, msg, key=key) [_check_query_value(key, v, msg) for v in value] else: - _check_query_value(key, value, 'Expected "{key}" value to be a ' - 'string, not {arg!r}') + _check_query_value( + key, value, 'Expected "{key}" to be a string, not {arg!r}') def _check_query_value(key, arg, msg): diff --git a/tests/local/test_library.py b/tests/local/test_library.py index 0198ec9e..7763057f 100644 --- a/tests/local/test_library.py +++ b/tests/local/test_library.py @@ -9,7 +9,7 @@ import mock import pykka -from mopidy import core +from mopidy import core, exceptions from mopidy.local import actor, json from mopidy.models import Album, Artist, Image, Track @@ -137,8 +137,8 @@ class LocalLibraryProviderTest(unittest.TestCase): self.assertEqual(result[uri], self.tracks[0:1]) def test_lookup_unknown_track(self): - tracks = self.library.lookup(uris=['fake uri']) - self.assertEqual(tracks, {'fake uri': []}) + tracks = self.library.lookup(uris=['fake:/uri']) + self.assertEqual(tracks, {'fake:/uri': []}) # test backward compatibility with local libraries returning a # single Track @@ -343,42 +343,44 @@ class LocalLibraryProviderTest(unittest.TestCase): result = self.find_exact(any=['local:track:path1']) self.assertEqual(list(result[0].tracks), self.tracks[:1]) + # TODO: This is really just a test of the query validation code now, + # as this code path never even makes it to the local backend. def test_find_exact_wrong_type(self): - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(wrong=['test']) def test_find_exact_with_empty_query(self): - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(artist=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(albumartist=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(track_name=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(composer=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(performer=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(album=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(track_no=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(genre=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(date=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(comment=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.find_exact(any=['']) def test_search_no_hits(self): @@ -553,41 +555,41 @@ class LocalLibraryProviderTest(unittest.TestCase): self.assertEqual(list(result[0].tracks), self.tracks[:1]) def test_search_wrong_type(self): - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(wrong=['test']) def test_search_with_empty_query(self): - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(artist=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(albumartist=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(composer=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(performer=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(track_name=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(album=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(genre=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(date=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(comment=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(uri=['']) - with self.assertRaises(LookupError): + with self.assertRaises(exceptions.ValidationError): self.search(any=['']) def test_default_get_images_impl_no_images(self): diff --git a/tests/local/test_playback.py b/tests/local/test_playback.py index 20601d93..8aedcfbc 100644 --- a/tests/local/test_playback.py +++ b/tests/local/test_playback.py @@ -841,22 +841,6 @@ class LocalPlaybackProviderTest(unittest.TestCase): self.playback.seek(self.tracklist.tracks[-1].length * 100) self.assertEqual(self.playback.state, PlaybackState.STOPPED) - @unittest.SkipTest - @populate_tracklist - def test_seek_beyond_start_of_song(self): - # FIXME need to decide return value - self.playback.play() - result = self.playback.seek(-1000) - self.assert_(not result, 'Seek return value was %s' % result) - - @populate_tracklist - def test_seek_beyond_start_of_song_update_postion(self): - self.playback.play() - self.playback.seek(-1000) - position = self.playback.time_position - self.assertGreaterEqual(position, 0) - self.assertEqual(self.playback.state, PlaybackState.PLAYING) - @populate_tracklist def test_stop_when_stopped(self): self.playback.stop() diff --git a/tests/utils/test_validation.py b/tests/utils/test_validation.py index d55a918e..fdebd4d2 100644 --- a/tests/utils/test_validation.py +++ b/tests/utils/test_validation.py @@ -118,7 +118,7 @@ def test_check_query_invalid_values(): def test_check_values_error_message(): with raises(exceptions.ValidationError) as excinfo: validation.check_query({'any': 'abc'}) - assert 'Expected "any" values to be list of strings' in str(excinfo.value) + assert 'Expected "any" to be list of strings, not' in str(excinfo.value) def test_check_uri_with_valid_values():