From beb6a38d8519ab75109e95e1ae33e5f212a8f9bf Mon Sep 17 00:00:00 2001 From: Vincent Petry Date: Mon, 11 May 2015 17:53:21 +0200 Subject: [PATCH] Added rmdir to trashbin storage wrapper This makes sure that folders are moved to trash when deleted with rmdir() instead of unlink(). This happens for example when deleting a folder over WebDAV. The web UI uses unlink() so it wasn't affected. --- apps/files_trashbin/lib/storage.php | 35 ++++- apps/files_trashbin/tests/storage.php | 179 ++++++++++++++++++++++++-- 2 files changed, 199 insertions(+), 15 deletions(-) diff --git a/apps/files_trashbin/lib/storage.php b/apps/files_trashbin/lib/storage.php index 418d7d2f1f..006971fb24 100644 --- a/apps/files_trashbin/lib/storage.php +++ b/apps/files_trashbin/lib/storage.php @@ -81,14 +81,39 @@ class Storage extends Wrapper { /** * Deletes the given file by moving it into the trashbin. * - * @param string $path + * @param string $path path of file or folder to delete + * + * @return bool true if the operation succeeded, false otherwise */ public function unlink($path) { + return $this->doDelete($path, 'unlink'); + } + + /** + * Deletes the given folder by moving it into the trashbin. + * + * @param string $path path of folder to delete + * + * @return bool true if the operation succeeded, false otherwise + */ + public function rmdir($path) { + return $this->doDelete($path, 'rmdir'); + } + + /** + * Run the delete operation with the given method + * + * @param string $path path of file or folder to delete + * @param string $method either "unlink" or "rmdir" + * + * @return bool true if the operation succeeded, false otherwise + */ + private function doDelete($path, $method) { if (self::$disableTrash || !\OC_App::isEnabled('files_trashbin') || (pathinfo($path, PATHINFO_EXTENSION) === 'part') ) { - return $this->storage->unlink($path); + return call_user_func_array([$this->storage, $method], [$path]); } $normalized = Filesystem::normalizePath($this->mountPoint . '/' . $path); $result = true; @@ -101,14 +126,14 @@ class Storage extends Wrapper { // in cross-storage cases the file will be copied // but not deleted, so we delete it here if ($result) { - $this->storage->unlink($path); + call_user_func_array([$this->storage, $method], [$path]); } } else { - $result = $this->storage->unlink($path); + $result = call_user_func_array([$this->storage, $method], [$path]); } unset($this->deletedFiles[$normalized]); } else if ($this->storage->file_exists($path)) { - $result = $this->storage->unlink($path); + $result = call_user_func_array([$this->storage, $method], [$path]); } return $result; diff --git a/apps/files_trashbin/tests/storage.php b/apps/files_trashbin/tests/storage.php index d1468522dc..7415aba09e 100644 --- a/apps/files_trashbin/tests/storage.php +++ b/apps/files_trashbin/tests/storage.php @@ -62,6 +62,8 @@ class Storage extends \Test\TestCase { $this->userView = new \OC\Files\View('/' . $this->user . '/files/'); $this->userView->file_put_contents('test.txt', 'foo'); + $this->userView->mkdir('folder'); + $this->userView->file_put_contents('folder/inside.txt', 'bar'); } protected function tearDown() { @@ -75,7 +77,7 @@ class Storage extends \Test\TestCase { /** * Test that deleting a file puts it into the trashbin. */ - public function testSingleStorageDelete() { + public function testSingleStorageDeleteFile() { $this->assertTrue($this->userView->file_exists('test.txt')); $this->userView->unlink('test.txt'); list($storage,) = $this->userView->resolvePath('test.txt'); @@ -89,13 +91,35 @@ class Storage extends \Test\TestCase { $this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.'))); } + /** + * Test that deleting a folder puts it into the trashbin. + */ + public function testSingleStorageDeleteFolder() { + $this->assertTrue($this->userView->file_exists('folder/inside.txt')); + $this->userView->rmdir('folder'); + list($storage,) = $this->userView->resolvePath('folder/inside.txt'); + $storage->getScanner()->scan(''); // make sure we check the storage + $this->assertFalse($this->userView->getFileInfo('folder')); + + // check if folder is in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('folder', substr($name, 0, strrpos($name, '.'))); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/' . $name . '/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('inside.txt', $name); + } + /** * Test that deleting a file from another mounted storage properly * lands in the trashbin. This is a cross-storage situation because * the trashbin folder is in the root storage while the mounted one * isn't. */ - public function testCrossStorageDelete() { + public function testCrossStorageDeleteFile() { $storage2 = new Temporary(array()); \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); @@ -115,10 +139,42 @@ class Storage extends \Test\TestCase { $this->assertEquals('subfile.txt', substr($name, 0, strrpos($name, '.'))); } + /** + * Test that deleting a folder from another mounted storage properly + * lands in the trashbin. This is a cross-storage situation because + * the trashbin folder is in the root storage while the mounted one + * isn't. + */ + public function testCrossStorageDeleteFolder() { + $storage2 = new Temporary(array()); + \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); + + $this->userView->mkdir('substorage/folder'); + $this->userView->file_put_contents('substorage/folder/subfile.txt', 'bar'); + $storage2->getScanner()->scan(''); + $this->assertTrue($storage2->file_exists('folder/subfile.txt')); + $this->userView->rmdir('substorage/folder'); + + $storage2->getScanner()->scan(''); + $this->assertFalse($this->userView->getFileInfo('substorage/folder')); + $this->assertFalse($storage2->file_exists('folder/subfile.txt')); + + // check if folder is in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('folder', substr($name, 0, strrpos($name, '.'))); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/' . $name . '/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('subfile.txt', $name); + } + /** * Test that deleted versions properly land in the trashbin. */ - public function testDeleteVersions() { + public function testDeleteVersionsOfFile() { \OCA\Files_Versions\Hooks::connectHooks(); // trigger a version (multiple would not work because of the expire logic) @@ -137,7 +193,38 @@ class Storage extends \Test\TestCase { $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions'); $this->assertEquals(1, count($results)); $name = $results[0]->getName(); - $this->assertEquals('test.txt', substr($name, 0, strlen('test.txt'))); + $this->assertEquals('test.txt.v', substr($name, 0, strlen('test.txt.v'))); + } + + /** + * Test that deleted versions properly land in the trashbin. + */ + public function testDeleteVersionsOfFolder() { + \OCA\Files_Versions\Hooks::connectHooks(); + + // trigger a version (multiple would not work because of the expire logic) + $this->userView->file_put_contents('folder/inside.txt', 'v1'); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_versions/folder/'); + $this->assertEquals(1, count($results)); + + $this->userView->rmdir('folder'); + + // rescan trash storage + list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + $rootStorage->getScanner()->scan(''); + + // check if versions are in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('folder.d', substr($name, 0, strlen('folder.d'))); + + // check if versions are in trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/' . $name . '/'); + $this->assertEquals(1, count($results)); + $name = $results[0]->getName(); + $this->assertEquals('inside.txt.v', substr($name, 0, strlen('inside.txt.v'))); } /** @@ -145,7 +232,7 @@ class Storage extends \Test\TestCase { * storages. This is because rename() between storages would call * unlink() which should NOT trigger the version deletion logic. */ - public function testKeepFileAndVersionsWhenMovingBetweenStorages() { + public function testKeepFileAndVersionsWhenMovingFileBetweenStorages() { \OCA\Files_Versions\Hooks::connectHooks(); $storage2 = new Temporary(array()); @@ -162,7 +249,7 @@ class Storage extends \Test\TestCase { // move to another storage $this->userView->rename('test.txt', 'substorage/test.txt'); - $this->userView->file_exists('substorage/test.txt'); + $this->assertTrue($this->userView->file_exists('substorage/test.txt')); // rescan trash storage list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); @@ -181,10 +268,51 @@ class Storage extends \Test\TestCase { $this->assertEquals(0, count($results)); } + /** + * Test that versions are not auto-trashed when moving a file between + * storages. This is because rename() between storages would call + * unlink() which should NOT trigger the version deletion logic. + */ + public function testKeepFileAndVersionsWhenMovingFolderBetweenStorages() { + \OCA\Files_Versions\Hooks::connectHooks(); + + $storage2 = new Temporary(array()); + \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); + + // trigger a version (multiple would not work because of the expire logic) + $this->userView->file_put_contents('folder/inside.txt', 'v1'); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $this->assertEquals(0, count($results)); + + $results = $this->rootView->getDirectoryContent($this->user . '/files_versions/folder/'); + $this->assertEquals(1, count($results)); + + // move to another storage + $this->userView->rename('folder', 'substorage/folder'); + $this->assertTrue($this->userView->file_exists('substorage/folder/inside.txt')); + + // rescan trash storage + list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); + $rootStorage->getScanner()->scan(''); + + // versions were moved too + $results = $this->rootView->getDirectoryContent($this->user . '/files_versions/substorage/folder/'); + $this->assertEquals(1, count($results)); + + // check that nothing got trashed by the rename's unlink() call + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files'); + $this->assertEquals(0, count($results)); + + // check that versions were moved and not trashed + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions/'); + $this->assertEquals(0, count($results)); + } + /** * Delete should fail is the source file cant be deleted */ - public function testSingleStorageDeleteFail() { + public function testSingleStorageDeleteFileFail() { /** * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage */ @@ -193,9 +321,6 @@ class Storage extends \Test\TestCase { ->setMethods(['rename', 'unlink']) ->getMock(); - $storage->expects($this->any()) - ->method('rename') - ->will($this->returnValue(false)); $storage->expects($this->any()) ->method('unlink') ->will($this->returnValue(false)); @@ -214,4 +339,38 @@ class Storage extends \Test\TestCase { $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); $this->assertEquals(0, count($results)); } + + /** + * Delete should fail is the source folder cant be deleted + */ + public function testSingleStorageDeleteFolderFail() { + /** + * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage + */ + $storage = $this->getMockBuilder('\OC\Files\Storage\Temporary') + ->setConstructorArgs([[]]) + ->setMethods(['rename', 'unlink', 'rmdir']) + ->getMock(); + + $storage->expects($this->any()) + ->method('rmdir') + ->will($this->returnValue(false)); + + $cache = $storage->getCache(); + + Filesystem::mount($storage, [], '/' . $this->user); + $storage->mkdir('files'); + $this->userView->mkdir('folder'); + $this->userView->file_put_contents('folder/test.txt', 'foo'); + $this->assertTrue($storage->file_exists('files/folder/test.txt')); + $this->assertFalse($this->userView->rmdir('files/folder')); + $this->assertTrue($storage->file_exists('files/folder')); + $this->assertTrue($storage->file_exists('files/folder/test.txt')); + $this->assertTrue($cache->inCache('files/folder')); + $this->assertTrue($cache->inCache('files/folder/test.txt')); + + // file should not be in the trashbin + $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); + $this->assertEquals(0, count($results)); + } }