andpp/mopidy
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Merge pull request #1349 from adamcik/feature/mpd-playlists-error-handling

Browse Source 
MPD playlist editing error handling
This commit is contained in:
Stein Magnus Jodal 2015-12-05 21:14:55 +01:00
commit 23d83a833f
5 changed files with 188 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
mopidy/mpd/exceptions.py
View File

@ -80,10 +80,23 @@ class MpdNoExistError(MpdAckError):
error_code = MpdAckError.ACK_ERROR_NO_EXIST
class MpdExistError(MpdAckError):
error_code = MpdAckError.ACK_ERROR_EXIST
class MpdSystemError(MpdAckError):
error_code = MpdAckError.ACK_ERROR_SYSTEM
class MpdInvalidPlaylistName(MpdAckError):
error_code = MpdAckError.ACK_ERROR_ARG
def __init__(self, *args, **kwargs):
super(MpdInvalidPlaylistName, self).__init__(*args, **kwargs)
self.message = ('playlist name is invalid: playlist names may not '
'contain slashes, newlines or carriage returns')
class MpdNotImplemented(MpdAckError):
error_code = 0

55
mopidy/mpd/protocol/stored_playlists.py
View File

@ -2,6 +2,7 @@ from __future__ import absolute_import, division, unicode_literals
import datetime
import logging
import re
import warnings
from mopidy.compat import urllib
@ -10,6 +11,11 @@ from mopidy.mpd import exceptions, protocol, translator
logger = logging.getLogger(__name__)
def _check_playlist_name(name):
if re.search('[/\n\r]', name):
raise exceptions.MpdInvalidPlaylistName()
@protocol.commands.add('listplaylist')
def listplaylist(context, name):
"""
@ -149,6 +155,7 @@ def playlistadd(context, name, track_uri):
``NAME.m3u`` will be created if it does not exist.
"""
_check_playlist_name(name)
uri = context.lookup_playlist_uri_from_name(name)
old_playlist = uri is not None and context.core.playlists.lookup(uri).get()
if not old_playlist:
@ -219,6 +226,7 @@ def playlistclear(context, name):
The playlist will be created if it does not exist.
"""
_check_playlist_name(name)
uri = context.lookup_playlist_uri_from_name(name)
playlist = uri is not None and context.core.playlists.lookup(uri).get()
if not playlist:
@ -240,14 +248,18 @@ def playlistdelete(context, name, songpos):
Deletes ``SONGPOS`` from the playlist ``NAME.m3u``.
"""
_check_playlist_name(name)
uri = context.lookup_playlist_uri_from_name(name)
playlist = uri is not None and context.core.playlists.lookup(uri).get()
if not playlist:
raise exceptions.MpdNoExistError('No such playlist')
# Convert tracks to list and remove requested
tracks = list(playlist.tracks)
tracks.pop(songpos)
try:
# Convert tracks to list and remove requested
tracks = list(playlist.tracks)
tracks.pop(songpos)
except IndexError:
raise exceptions.MpdArgError('Bad song index')
# Replace tracks and save playlist
playlist = playlist.replace(tracks=tracks)
@ -274,6 +286,10 @@ def playlistmove(context, name, from_pos, to_pos):
documentation, but just the ``SONGPOS`` to move *from*, i.e.
``playlistmove {NAME} {FROM_SONGPOS} {TO_SONGPOS}``.
"""
if from_pos == to_pos:
return
_check_playlist_name(name)
uri = context.lookup_playlist_uri_from_name(name)
playlist = uri is not None and context.core.playlists.lookup(uri).get()
if not playlist:
@ -281,10 +297,13 @@ def playlistmove(context, name, from_pos, to_pos):
if from_pos == to_pos:
return # Nothing to do
# Convert tracks to list and perform move
tracks = list(playlist.tracks)
track = tracks.pop(from_pos)
tracks.insert(to_pos, track)
try:
# Convert tracks to list and perform move
tracks = list(playlist.tracks)
track = tracks.pop(from_pos)
tracks.insert(to_pos, track)
except IndexError:
raise exceptions.MpdArgError('Bad song index')
# Replace tracks and save playlist
playlist = playlist.replace(tracks=tracks)
@ -303,16 +322,28 @@ def rename(context, old_name, new_name):
Renames the playlist ``NAME.m3u`` to ``NEW_NAME.m3u``.
"""
uri = context.lookup_playlist_uri_from_name(old_name)
uri_scheme = urllib.parse.urlparse(uri).scheme
old_playlist = uri is not None and context.core.playlists.lookup(uri).get()
_check_playlist_name(old_name)
_check_playlist_name(new_name)
old_uri = context.lookup_playlist_uri_from_name(old_name)
if not old_uri:
raise exceptions.MpdNoExistError('No such playlist')
old_playlist = context.core.playlists.lookup(old_uri).get()
if not old_playlist:
raise exceptions.MpdNoExistError('No such playlist')
new_uri = context.lookup_playlist_uri_from_name(new_name)
if new_uri and context.core.playlists.lookup(new_uri).get():
raise exceptions.MpdExistError('Playlist already exists')
# TODO: should we purge the mapping in an else?
# Create copy of the playlist and remove original
uri_scheme = urllib.parse.urlparse(old_uri).scheme
new_playlist = context.core.playlists.create(new_name, uri_scheme).get()
new_playlist = new_playlist.replace(tracks=old_playlist.tracks)
saved_playlist = context.core.playlists.save(new_playlist).get()
if saved_playlist is None:
raise exceptions.MpdFailedToSavePlaylist(uri_scheme)
context.core.playlists.delete(old_playlist.uri).get()
@ -327,7 +358,10 @@ def rm(context, name):
Removes the playlist ``NAME.m3u`` from the playlist directory.
"""
_check_playlist_name(name)
uri = context.lookup_playlist_uri_from_name(name)
if not uri:
raise exceptions.MpdNoExistError('No such playlist')
context.core.playlists.delete(uri).get()
@ -341,6 +375,7 @@ def save(context, name):
Saves the current playlist to ``NAME.m3u`` in the playlist
directory.
"""
_check_playlist_name(name)
tracks = context.core.tracklist.get_tracks().get()
uri = context.lookup_playlist_uri_from_name(name)
playlist = uri is not None and context.core.playlists.lookup(uri).get()

2
mopidy/mpd/uri_mapper.py
View File

@ -71,7 +71,7 @@ class MpdUriMapper(object):
"""
Helper function to retrieve a playlist URI from its unique MPD name.
"""
if not self._uri_from_name:
if name not in self._uri_from_name:
self.refresh_playlists_mapping()
return self._uri_from_name.get(name)

21
tests/mpd/protocol/test_regression.py
View File

@ -233,3 +233,24 @@ class IssueGH1120RegressionTest(protocol.BaseTestCase):
response2 = self.send_request('lsinfo "/"')
self.assertEqual(response1, response2)
class IssueGH1348RegressionTest(protocol.BaseTestCase):
"""
The issue: http://github.com/mopidy/mopidy/issues/1348
"""
def test(self):
self.backend.library.dummy_library = [Track(uri='dummy:a')]
# Create a dummy playlist and trigger population of mapping
self.send_request('playlistadd "testing1" "dummy:a"')
self.send_request('listplaylists')
# Create an other playlist which isn't in the map
self.send_request('playlistadd "testing2" "dummy:a"')
self.assertEqual(['OK'], self.send_request('rm "testing2"'))
playlists = self.backend.playlists.as_list().get()
self.assertEqual(['testing1'], [ref.name for ref in playlists])

108
tests/mpd/protocol/test_stored_playlists.py
View File

@ -232,6 +232,10 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
self.assertEqual(0, len(self.core.tracklist.tracks.get()))
self.assertEqualResponse('ACK [50@0] {load} No such playlist')
# No invalid name check for load.
self.send_request('load "unknown/playlist"')
self.assertEqualResponse('ACK [50@0] {load} No such playlist')
def test_playlistadd(self):
tracks = [
Track(uri='dummy:a'),
@ -259,6 +263,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
self.assertInResponse('OK')
self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
def test_playlistadd_invalid_name_acks(self):
self.send_request('playlistadd "foo/bar" "dummy:a"')
self.assertInResponse('ACK [2@0] {playlistadd} playlist name is '
'invalid: playlist names may not contain '
'slashes, newlines or carriage returns')
def test_playlistclear(self):
self.backend.playlists.set_dummy_playlists([
Playlist(
@ -276,6 +286,12 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
self.assertInResponse('OK')
self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
def test_playlistclear_invalid_name_acks(self):
self.send_request('playlistclear "foo/bar"')
self.assertInResponse('ACK [2@0] {playlistclear} playlist name is '
'invalid: playlist names may not contain '
'slashes, newlines or carriage returns')
def test_playlistdelete(self):
tracks = [
Track(uri='dummy:a'),
@ -292,6 +308,21 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
self.assertEqual(
2, len(self.backend.playlists.get_items('dummy:a1').get()))
def test_playlistdelete_invalid_name_acks(self):
self.send_request('playlistdelete "foo/bar" "0"')
self.assertInResponse('ACK [2@0] {playlistdelete} playlist name is '
'invalid: playlist names may not contain '
'slashes, newlines or carriage returns')
def test_playlistdelete_unknown_playlist_acks(self):
self.send_request('playlistdelete "foobar" "0"')
self.assertInResponse('ACK [50@0] {playlistdelete} No such playlist')
def test_playlistdelete_unknown_index_acks(self):
self.send_request('save "foobar"')
self.send_request('playlistdelete "foobar" "0"')
self.assertInResponse('ACK [2@0] {playlistdelete} Bad song index')
def test_playlistmove(self):
tracks = [
Track(uri='dummy:a'),
@ -309,6 +340,42 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
"dummy:c",
self.backend.playlists.get_items('dummy:a1').get()[0].uri)
def test_playlistmove_invalid_name_acks(self):
self.send_request('playlistmove "foo/bar" "0" "1"')
self.assertInResponse('ACK [2@0] {playlistmove} playlist name is '
'invalid: playlist names may not contain '
'slashes, newlines or carriage returns')
def test_playlistmove_unknown_playlist_acks(self):
self.send_request('playlistmove "foobar" "0" "1"')
self.assertInResponse('ACK [50@0] {playlistmove} No such playlist')
def test_playlistmove_unknown_position_acks(self):
self.send_request('save "foobar"')
self.send_request('playlistmove "foobar" "0" "1"')
self.assertInResponse('ACK [2@0] {playlistmove} Bad song index')
def test_playlistmove_same_index_shortcircuits_everything(self):
# Bad indexes on unknown playlist:
self.send_request('playlistmove "foobar" "0" "0"')
self.assertInResponse('OK')
self.send_request('playlistmove "foobar" "100000" "100000"')
self.assertInResponse('OK')
# Bad indexes on known playlist:
self.send_request('save "foobar"')
self.send_request('playlistmove "foobar" "0" "0"')
self.assertInResponse('OK')
self.send_request('playlistmove "foobar" "10" "10"')
self.assertInResponse('OK')
# Invalid playlist name:
self.send_request('playlistmove "foo/bar" "0" "0"')
self.assertInResponse('OK')
def test_rename(self):
self.backend.playlists.set_dummy_playlists([
Playlist(
@ -320,6 +387,31 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
self.assertIsNotNone(
self.backend.playlists.lookup('dummy:new_name').get())
def test_rename_unknown_playlist_acks(self):
self.send_request('rename "foo" "bar"')
self.assertInResponse('ACK [50@0] {rename} No such playlist')
def test_rename_to_existing_acks(self):
self.send_request('save "foo"')
self.send_request('save "bar"')
self.send_request('rename "foo" "bar"')
self.assertInResponse('ACK [56@0] {rename} Playlist already exists')
def test_rename_invalid_name_acks(self):
expected = ('ACK [2@0] {rename} playlist name is invalid: playlist '
'names may not contain slashes, newlines or carriage '
'returns')
self.send_request('rename "foo/bar" "bar"')
self.assertInResponse(expected)
self.send_request('rename "foo" "foo/bar"')
self.assertInResponse(expected)
self.send_request('rename "bar/foo" "foo/bar"')
self.assertInResponse(expected)
def test_rm(self):
self.backend.playlists.set_dummy_playlists([
Playlist(
@ -330,8 +422,24 @@ class PlaylistsHandlerTest(protocol.BaseTestCase):
self.assertInResponse('OK')
self.assertIsNone(self.backend.playlists.lookup('dummy:a1').get())
def test_rm_unknown_playlist_acks(self):
self.send_request('rm "name"')
self.assertInResponse('ACK [50@0] {rm} No such playlist')
def test_rm_invalid_name_acks(self):
self.send_request('rm "foo/bar"')
self.assertInResponse('ACK [2@0] {rm} playlist name is invalid: '
'playlist names may not contain slashes, '
'newlines or carriage returns')
def test_save(self):
self.send_request('save "name"')
self.assertInResponse('OK')
self.assertIsNotNone(self.backend.playlists.lookup('dummy:name').get())
def test_save_invalid_name_acks(self):
self.send_request('save "foo/bar"')
self.assertInResponse('ACK [2@0] {save} playlist name is invalid: '
'playlist names may not contain slashes, '
'newlines or carriage returns')