Fix files node API failed rename/copy

Whenever a rename or copy operation failed on the view, we must throw
an exception instead of just ignoring.

Signed-off-by: Vincent Petry <pvince81@owncloud.com>

lib/private/Files/Node/File.php

@@ -29,6 +29,16 @@ namespace OC\Files\Node;
 use OCP\Files\NotPermittedException;
 
 class File extends Node implements \OCP\Files\File {
+	/**
+	 * Creates a Folder that represents a non-existing path
+	 *
+	 * @param string $path path
+	 * @return string non-existing node class
+	 */
+	protected function createNonExistingNode($path) {
+		return new NonExistingFile($this->root, $this->view, $path);
+	}
+
 	/**
 	 * @return string
 	 * @throws \OCP\Files\NotPermittedException
@@ -113,52 +123,6 @@ class File extends Node implements \OCP\Files\File {
 		}
 	}
 
-	/**
-	 * @param string $targetPath
-	 * @throws \OCP\Files\NotPermittedException
-	 * @return \OC\Files\Node\Node
-	 */
-	public function copy($targetPath) {
-		$targetPath = $this->normalizePath($targetPath);
-		$parent = $this->root->get(dirname($targetPath));
-		if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) {
-			$nonExisting = new NonExistingFile($this->root, $this->view, $targetPath);
-			$this->root->emit('\OC\Files', 'preCopy', array($this, $nonExisting));
-			$this->root->emit('\OC\Files', 'preWrite', array($nonExisting));
-			$this->view->copy($this->path, $targetPath);
-			$targetNode = $this->root->get($targetPath);
-			$this->root->emit('\OC\Files', 'postCopy', array($this, $targetNode));
-			$this->root->emit('\OC\Files', 'postWrite', array($targetNode));
-			return $targetNode;
-		} else {
-			throw new NotPermittedException();
-		}
-	}
-
-	/**
-	 * @param string $targetPath
-	 * @throws \OCP\Files\NotPermittedException
-	 * @return \OC\Files\Node\Node
-	 */
-	public function move($targetPath) {
-		$targetPath = $this->normalizePath($targetPath);
-		$parent = $this->root->get(dirname($targetPath));
-		if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) {
-			$nonExisting = new NonExistingFile($this->root, $this->view, $targetPath);
-			$this->root->emit('\OC\Files', 'preRename', array($this, $nonExisting));
-			$this->root->emit('\OC\Files', 'preWrite', array($nonExisting));
-			$this->view->rename($this->path, $targetPath);
-			$targetNode = $this->root->get($targetPath);
-			$this->root->emit('\OC\Files', 'postRename', array($this, $targetNode));
-			$this->root->emit('\OC\Files', 'postWrite', array($targetNode));
-			$this->path = $targetPath;
-			$this->fileInfo = null;
-			return $targetNode;
-		} else {
-			throw new NotPermittedException();
-		}
-	}
-
 	/**
 	 * @param string $type
 	 * @param bool $raw

lib/private/Files/Node/Folder.php

@@ -35,6 +35,16 @@ use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
 
 class Folder extends Node implements \OCP\Files\Folder {
+	/**
+	 * Creates a Folder that represents a non-existing path
+	 *
+	 * @param string $path path
+	 * @return string non-existing node class
+	 */
+	protected function createNonExistingNode($path) {
+		return new NonExistingFolder($this->root, $this->view, $path);
+	}
+
 	/**
 	 * @param string $path path relative to the folder
 	 * @return string
@@ -325,51 +335,6 @@ class Folder extends Node implements \OCP\Files\Folder {
 		}
 	}
 
-	/**
-	 * @param string $targetPath
-	 * @throws \OCP\Files\NotPermittedException
-	 * @return \OC\Files\Node\Node
-	 */
-	public function copy($targetPath) {
-		$targetPath = $this->normalizePath($targetPath);
-		$parent = $this->root->get(dirname($targetPath));
-		if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) {
-			$nonExisting = new NonExistingFolder($this->root, $this->view, $targetPath);
-			$this->root->emit('\OC\Files', 'preCopy', array($this, $nonExisting));
-			$this->root->emit('\OC\Files', 'preWrite', array($nonExisting));
-			$this->view->copy($this->path, $targetPath);
-			$targetNode = $this->root->get($targetPath);
-			$this->root->emit('\OC\Files', 'postCopy', array($this, $targetNode));
-			$this->root->emit('\OC\Files', 'postWrite', array($targetNode));
-			return $targetNode;
-		} else {
-			throw new NotPermittedException('No permission to copy to path');
-		}
-	}
-
-	/**
-	 * @param string $targetPath
-	 * @throws \OCP\Files\NotPermittedException
-	 * @return \OC\Files\Node\Node
-	 */
-	public function move($targetPath) {
-		$targetPath = $this->normalizePath($targetPath);
-		$parent = $this->root->get(dirname($targetPath));
-		if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) {
-			$nonExisting = new NonExistingFolder($this->root, $this->view, $targetPath);
-			$this->root->emit('\OC\Files', 'preRename', array($this, $nonExisting));
-			$this->root->emit('\OC\Files', 'preWrite', array($nonExisting));
-			$this->view->rename($this->path, $targetPath);
-			$targetNode = $this->root->get($targetPath);
-			$this->root->emit('\OC\Files', 'postRename', array($this, $targetNode));
-			$this->root->emit('\OC\Files', 'postWrite', array($targetNode));
-			$this->path = $targetPath;
-			return $targetNode;
-		} else {
-			throw new NotPermittedException('No permission to move to path');
-		}
-	}
-
 	/**
 	 * Add a suffix to the name in case the file exists
 	 *

lib/private/Files/Node/Node.php

@@ -33,6 +33,7 @@ use OCP\Files\InvalidPathException;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
 
+// FIXME: this class really should be abstract
 class Node implements \OCP\Files\Node {
 	/**
 	 * @var \OC\Files\View $view
@@ -67,6 +68,16 @@ class Node implements \OCP\Files\Node {
 		$this->fileInfo = $fileInfo;
 	}
 
+	/**
+	 * Creates a Node of the same type that represents a non-existing path
+	 *
+	 * @param string $path path
+	 * @return string non-existing node class
+	 */
+	protected function createNonExistingNode($path) {
+		throw new \Exception('Must be implemented by subclasses');
+	}
+
 	/**
 	 * Returns the matching file info
 	 *
@@ -106,27 +117,10 @@ class Node implements \OCP\Files\Node {
 		return ($this->getPermissions() & $permissions) === $permissions;
 	}
 
-	/**
-	 * @param string $targetPath
-	 * @throws \OCP\Files\NotPermittedException
-	 * @return \OC\Files\Node\Node
-	 */
-	public function move($targetPath) {
-		return;
-	}
-
 	public function delete() {
 		return;
 	}
 
-	/**
-	 * @param string $targetPath
-	 * @return \OC\Files\Node\Node
-	 */
-	public function copy($targetPath) {
-		return;
-	}
-
 	/**
 	 * @param int $mtime
 	 * @throws \OCP\Files\NotPermittedException
@@ -381,4 +375,54 @@ class Node implements \OCP\Files\Node {
 	public function unlock($type) {
 		$this->view->unlockFile($this->path, $type);
 	}
+
+	/**
+	 * @param string $targetPath
+	 * @throws \OCP\Files\NotPermittedException if copy not allowed or failed
+	 * @return \OC\Files\Node\Node
+	 */
+	public function copy($targetPath) {
+		$targetPath = $this->normalizePath($targetPath);
+		$parent = $this->root->get(dirname($targetPath));
+		if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) {
+			$nonExisting = $this->createNonExistingNode($targetPath);
+			$this->root->emit('\OC\Files', 'preCopy', [$this, $nonExisting]);
+			$this->root->emit('\OC\Files', 'preWrite', [$nonExisting]);
+			if (!$this->view->copy($this->path, $targetPath)) {
+				throw new NotPermittedException('Could not copy ' . $this->path . ' to ' . $targetPath);
+			}
+			$targetNode = $this->root->get($targetPath);
+			$this->root->emit('\OC\Files', 'postCopy', [$this, $targetNode]);
+			$this->root->emit('\OC\Files', 'postWrite', [$targetNode]);
+			return $targetNode;
+		} else {
+			throw new NotPermittedException('No permission to copy to path ' . $targetPath);
+		}
+	}
+
+	/**
+	 * @param string $targetPath
+	 * @throws \OCP\Files\NotPermittedException if move not allowed or failed
+	 * @return \OC\Files\Node\Node
+	 */
+	public function move($targetPath) {
+		$targetPath = $this->normalizePath($targetPath);
+		$parent = $this->root->get(dirname($targetPath));
+		if ($parent instanceof Folder and $this->isValidPath($targetPath) and $parent->isCreatable()) {
+			$nonExisting = $this->createNonExistingNode($targetPath);
+			$this->root->emit('\OC\Files', 'preRename', [$this, $nonExisting]);
+			$this->root->emit('\OC\Files', 'preWrite', [$nonExisting]);
+			if (!$this->view->rename($this->path, $targetPath)) {
+				throw new NotPermittedException('Could not move ' . $this->path . ' to ' . $targetPath);
+			}
+			$targetNode = $this->root->get($targetPath);
+			$this->root->emit('\OC\Files', 'postRename', [$this, $targetNode]);
+			$this->root->emit('\OC\Files', 'postWrite', [$targetNode]);
+			$this->path = $targetPath;
+			return $targetNode;
+		} else {
+			throw new NotPermittedException('No permission to move to path ' . $targetPath);
+		}
+	}
+
 }

tests/lib/Files/Node/FileTest.php

@@ -16,9 +16,6 @@ namespace Test\Files\Node;
  * @package Test\Files\Node
  */
 class FileTest extends NodeTest {
-
-	public $viewDeleteMethod = 'unlink';
-
 	protected function createTestNode($root, $view, $path) {
 		return new \OC\Files\Node\File($root, $view, $path);
 	}

tests/lib/Files/Node/NodeTest.php

@@ -614,7 +614,6 @@ abstract class NodeTest extends \Test\TestCase {
 			->method('rename');
 
 		$node = $this->createTestNode($this->root, $this->view, '/bar/foo');
-		$parentNode = new \OC\Files\Node\Folder($this->root, $this->view, '/bar');
 
 		$this->root->expects($this->once())
 			->method('get')
@@ -641,4 +640,50 @@ abstract class NodeTest extends \Test\TestCase {
 
 		$node->move('/bar/asd');
 	}
+
+	/**
+	 * @expectedException \OCP\Files\NotPermittedException
+	 */
+	public function testMoveFailed() {
+		$this->view->expects($this->any())
+			->method('rename')
+			->with('/bar/foo', '/bar/asd')
+			->will($this->returnValue(false));
+
+		$this->view->expects($this->any())
+			->method('getFileInfo')
+			->will($this->returnValue($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL, 'fileid' => 1])));
+
+		$node = $this->createTestNode($this->root, $this->view, '/bar/foo');
+		$parentNode = new \OC\Files\Node\Folder($this->root, $this->view, '/bar');
+
+		$this->root->expects($this->any())
+			->method('get')
+			->will($this->returnValueMap([['/bar', $parentNode], ['/bar/asd', $node]]));
+
+		$node->move('/bar/asd');
+	}
+
+	/**
+	 * @expectedException \OCP\Files\NotPermittedException
+	 */
+	public function testCopyFailed() {
+		$this->view->expects($this->any())
+			->method('copy')
+			->with('/bar/foo', '/bar/asd')
+			->will($this->returnValue(false));
+
+		$this->view->expects($this->any())
+			->method('getFileInfo')
+			->will($this->returnValue($this->getFileInfo(['permissions' => \OCP\Constants::PERMISSION_ALL, 'fileid' => 1])));
+
+		$node = $this->createTestNode($this->root, $this->view, '/bar/foo');
+		$parentNode = new \OC\Files\Node\Folder($this->root, $this->view, '/bar');
+
+		$this->root->expects($this->any())
+			->method('get')
+			->will($this->returnValueMap([['/bar', $parentNode], ['/bar/asd', $node]]));
+
+		$node->copy('/bar/asd');
+	}
 }