core: Validate core API calls

mopidy/core/library.py

@@ -5,8 +5,7 @@ import logging
 import operator
 import urlparse
 
-
+from mopidy.utils import deprecation, validation
-from mopidy.utils import deprecation
 
 
 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

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:

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
 

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:

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.models import TlTrack, Track
-from mopidy.utils import deprecation
+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):

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 '
+            _check_query_value(
-                               'string, not {arg!r}')
+                key, value, 'Expected "{key}" to be a string, not {arg!r}')
 
 
 def _check_query_value(key, arg, msg):

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'])
+        tracks = self.library.lookup(uris=['fake:/uri'])
-        self.assertEqual(tracks, {'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):

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()

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():