Repository: mopidy/mopidy
Issue: mpd: Playlists rm crashes for new playlists

Steps to reproduce:
- Open ncmcpp
- Create a new playlist called `testing`
- Run `mpc rm testing`

```
2015-12-04 23:41:33,959 ERROR    [MpdSession-13] /home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/actor.py:269
  pykka Unhandled exception in MpdSession (urn:uuid:093fbff0-33df-4e39-ba0b-c7259431372c):
Traceback (most recent call last):
  File "/home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/actor.py", line 201, in _actor_loop
    response = self._handle_receive(message)
  File "/home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/actor.py", line 304, in _handle_receive
    return self.on_receive(message)
  File "/home/adamcik/dev/mopidy/mopidy/internal/network.py", line 370, in on_receive
    self.on_line_received(line)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/session.py", line 34, in on_line_received
    response = self.dispatcher.handle_request(line)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 47, in handle_request
    return self._call_next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 68, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 76, in _catch_mpd_ack_errors_filter
    return self._call_next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 68, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 89, in _authenticate_filter
    return self._call_next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 68, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 105, in _command_list_filter
    response = self._call_next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 68, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 134, in _idle_filter
    response = self._call_next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 68, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 147, in _add_ok_filter
    response = self._call_next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 68, in _call_next_filter
    return next_filter(request, response, filter_chain)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 159, in _call_handler_filter
    response = self._format_response(self._call_handler(request))
  File "/home/adamcik/dev/mopidy/mopidy/mpd/dispatcher.py", line 174, in _call_handler
    return protocol.commands.call(tokens, context=self.context)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/protocol/__init__.py", line 180, in call
    return self.handlers[tokens[0]](context, *tokens[1:])
  File "/home/adamcik/dev/mopidy/mopidy/mpd/protocol/__init__.py", line 158, in validate
    return func(**callargs)
  File "/home/adamcik/dev/mopidy/mopidy/mpd/protocol/stored_playlists.py", line 331, in rm
    context.core.playlists.delete(uri).get()
  File "/home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/threading.py", line 52, in get
    compat.reraise(*self._data['exc_info'])
  File "/home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/compat.py", line 12, in reraise
    exec('raise tp, value, tb')
  File "/home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/actor.py", line 201, in _actor_loop
    response = self._handle_receive(message)
  File "/home/adamcik/dev/mopidy-virtualenv/local/lib/python2.7/site-packages/pykka/actor.py", line 295, in _handle_receive
    return callee(*message['args'], **message['kwargs'])
  File "/home/adamcik/dev/mopidy/mopidy/core/playlists.py", line 176, in delete
    validation.check_uri(uri)
  File "/home/adamcik/dev/mopidy/mopidy/internal/validation.py", line 98, in check_uri
    raise exceptions.ValidationError(msg.format(arg=arg))
ValidationError: Expected a valid URI, not None
```

To "fix":
- Run `mpc lsplaylists`
- Run `mpc rm testing`


Changed files:
mopidy/mpd/uri_mapper.py, mopidy/mpd/exceptions.py, tests/mpd/protocol/test_stored_playlists.py, tests/mpd/protocol/test_regression.py, mopidy/mpd/protocol/stored_playlists.py

diff --git a/mopidy/mpd/exceptions.py b/mopidy/mpd/exceptions.py
index b64a6cf0..05762683 100644
--- a/mopidy/mpd/exceptions.py
+++ b/mopidy/mpd/exceptions.py
@@ -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
 
diff --git a/mopidy/mpd/protocol/stored_playlists.py b/mopidy/mpd/protocol/stored_playlists.py
index 647a1464..68ae1e9e 100644
--- a/mopidy/mpd/protocol/stored_playlists.py
+++ b/mopidy/mpd/protocol/stored_playlists.py
@@ -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()
diff --git a/mopidy/mpd/uri_mapper.py b/mopidy/mpd/uri_mapper.py
index 9e7ec2dd..bb627a47 100644
--- a/mopidy/mpd/uri_mapper.py
+++ b/mopidy/mpd/uri_mapper.py
@@ -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)
 
diff --git a/tests/mpd/protocol/test_regression.py b/tests/mpd/protocol/test_regression.py
index 1688d064..40a3f103 100644
--- a/tests/mpd/protocol/test_regression.py
+++ b/tests/mpd/protocol/test_regression.py
@@ -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])
diff --git a/tests/mpd/protocol/test_stored_playlists.py b/tests/mpd/protocol/test_stored_playlists.py
index e212af09..fc3e8214 100644
--- a/tests/mpd/protocol/test_stored_playlists.py
+++ b/tests/mpd/protocol/test_stored_playlists.py
@@ -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')