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>

diff --git a/lib/private/Files/Node/File.php b/lib/private/Files/Node/File.php
index c4430b9181daf0bd2fc68f9685326c3039c4d66d..4bfa5d583f78938abf86fec10c3601dfe6a52eef 100644 (file)
--- a/lib/private/Files/Node/File.php
+++ b/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

diff --git a/lib/private/Files/Node/Folder.php b/lib/private/Files/Node/Folder.php
index 288a02ef2071807962b520a6fc28167158101fa0..fd907f708f39376810c7dba042780c2aca568c83 100644 (file)
--- a/lib/private/Files/Node/Folder.php
+++ b/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
         *

diff --git a/lib/private/Files/Node/Node.php b/lib/private/Files/Node/Node.php
index afda495cb20b7a695e7b22e211d61083261469ac..e00debe69032663af17f80359b19a33d79d90be8 100644 (file)
--- a/lib/private/Files/Node/Node.php
+++ b/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);
+               }
+       }
+
 }

diff --git a/tests/lib/Files/Node/FileTest.php b/tests/lib/Files/Node/FileTest.php
index 87402cd5f5298e7607c18f303d610fc81a7ede65..a17cc1d1a3ad07db5afdedf757c1ac0a7240383b 100644 (file)
--- a/tests/lib/Files/Node/FileTest.php
+++ b/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);
        }

diff --git a/tests/lib/Files/Node/NodeTest.php b/tests/lib/Files/Node/NodeTest.php
index ce7250dc064182656e65c0db98412101ef3ed835..03f9118d9e4ee8791806b9d0df920d989f260edb 100644 (file)
--- a/tests/lib/Files/Node/NodeTest.php
+++ b/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');
+       }
 }