mpd: Make sure rename error handling is correct

mopidy/mpd/exceptions.py

@@ -80,6 +80,10 @@ 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
 

mopidy/mpd/protocol/stored_playlists.py

@@ -322,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()

tests/mpd/protocol/test_stored_playlists.py

@@ -387,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(