Skip to content
Snippets Groups Projects
Commit beb6a38d authored by Vincent Petry's avatar Vincent Petry
Browse files

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.
parent 20d2d8d3
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
apps/files_trashbin/lib/storage.php
+ 30
− 5
View file @ beb6a38d
...@@ -81,14 +81,39 @@ class Storage extends Wrapper { ...@@ -81,14 +81,39 @@ class Storage extends Wrapper {
/** /**
* Deletes the given file by moving it into the trashbin. * 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) { 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 if (self::$disableTrash
|| !\OC_App::isEnabled('files_trashbin') || !\OC_App::isEnabled('files_trashbin')
|| (pathinfo($path, PATHINFO_EXTENSION) === 'part') || (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); $normalized = Filesystem::normalizePath($this->mountPoint . '/' . $path);
$result = true; $result = true;
...@@ -101,14 +126,14 @@ class Storage extends Wrapper { ...@@ -101,14 +126,14 @@ class Storage extends Wrapper {
// in cross-storage cases the file will be copied // in cross-storage cases the file will be copied
// but not deleted, so we delete it here // but not deleted, so we delete it here
if ($result) { if ($result) {
$this->storage->unlink($path); call_user_func_array([$this->storage, $method], [$path]);
} }
} else { } else {
$result = $this->storage->unlink($path); $result = call_user_func_array([$this->storage, $method], [$path]);
} }
unset($this->deletedFiles[$normalized]); unset($this->deletedFiles[$normalized]);
} else if ($this->storage->file_exists($path)) { } else if ($this->storage->file_exists($path)) {
$result = $this->storage->unlink($path); $result = call_user_func_array([$this->storage, $method], [$path]);
} }
return $result; return $result;
......
apps/files_trashbin/tests/storage.php
+ 169
− 10
View file @ beb6a38d
...@@ -62,6 +62,8 @@ class Storage extends \Test\TestCase { ...@@ -62,6 +62,8 @@ class Storage extends \Test\TestCase {
$this->userView = new \OC\Files\View('/' . $this->user . '/files/'); $this->userView = new \OC\Files\View('/' . $this->user . '/files/');
$this->userView->file_put_contents('test.txt', 'foo'); $this->userView->file_put_contents('test.txt', 'foo');
$this->userView->mkdir('folder');
$this->userView->file_put_contents('folder/inside.txt', 'bar');
} }
protected function tearDown() { protected function tearDown() {
...@@ -75,7 +77,7 @@ class Storage extends \Test\TestCase { ...@@ -75,7 +77,7 @@ class Storage extends \Test\TestCase {
/** /**
* Test that deleting a file puts it into the trashbin. * 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->assertTrue($this->userView->file_exists('test.txt'));
$this->userView->unlink('test.txt'); $this->userView->unlink('test.txt');
list($storage,) = $this->userView->resolvePath('test.txt'); list($storage,) = $this->userView->resolvePath('test.txt');
...@@ -89,13 +91,35 @@ class Storage extends \Test\TestCase { ...@@ -89,13 +91,35 @@ class Storage extends \Test\TestCase {
$this->assertEquals('test.txt', substr($name, 0, strrpos($name, '.'))); $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 * Test that deleting a file from another mounted storage properly
* lands in the trashbin. This is a cross-storage situation because * lands in the trashbin. This is a cross-storage situation because
* the trashbin folder is in the root storage while the mounted one * the trashbin folder is in the root storage while the mounted one
* isn't. * isn't.
*/ */
public function testCrossStorageDelete() { public function testCrossStorageDeleteFile() {
$storage2 = new Temporary(array()); $storage2 = new Temporary(array());
\OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage'); \OC\Files\Filesystem::mount($storage2, array(), $this->user . '/files/substorage');
...@@ -115,10 +139,42 @@ class Storage extends \Test\TestCase { ...@@ -115,10 +139,42 @@ class Storage extends \Test\TestCase {
$this->assertEquals('subfile.txt', substr($name, 0, strrpos($name, '.'))); $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. * Test that deleted versions properly land in the trashbin.
*/ */
public function testDeleteVersions() { public function testDeleteVersionsOfFile() {
\OCA\Files_Versions\Hooks::connectHooks(); \OCA\Files_Versions\Hooks::connectHooks();
// trigger a version (multiple would not work because of the expire logic) // trigger a version (multiple would not work because of the expire logic)
...@@ -137,7 +193,38 @@ class Storage extends \Test\TestCase { ...@@ -137,7 +193,38 @@ class Storage extends \Test\TestCase {
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions'); $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/versions');
$this->assertEquals(1, count($results)); $this->assertEquals(1, count($results));
$name = $results[0]->getName(); $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 { ...@@ -145,7 +232,7 @@ class Storage extends \Test\TestCase {
* storages. This is because rename() between storages would call * storages. This is because rename() between storages would call
* unlink() which should NOT trigger the version deletion logic. * unlink() which should NOT trigger the version deletion logic.
*/ */
public function testKeepFileAndVersionsWhenMovingBetweenStorages() { public function testKeepFileAndVersionsWhenMovingFileBetweenStorages() {
\OCA\Files_Versions\Hooks::connectHooks(); \OCA\Files_Versions\Hooks::connectHooks();
$storage2 = new Temporary(array()); $storage2 = new Temporary(array());
...@@ -162,7 +249,7 @@ class Storage extends \Test\TestCase { ...@@ -162,7 +249,7 @@ class Storage extends \Test\TestCase {
// move to another storage // move to another storage
$this->userView->rename('test.txt', 'substorage/test.txt'); $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 // rescan trash storage
list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin'); list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
...@@ -181,10 +268,51 @@ class Storage extends \Test\TestCase { ...@@ -181,10 +268,51 @@ class Storage extends \Test\TestCase {
$this->assertEquals(0, count($results)); $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 * 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 * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage
*/ */
...@@ -193,9 +321,6 @@ class Storage extends \Test\TestCase { ...@@ -193,9 +321,6 @@ class Storage extends \Test\TestCase {
->setMethods(['rename', 'unlink']) ->setMethods(['rename', 'unlink'])
->getMock(); ->getMock();
$storage->expects($this->any())
->method('rename')
->will($this->returnValue(false));
$storage->expects($this->any()) $storage->expects($this->any())
->method('unlink') ->method('unlink')
->will($this->returnValue(false)); ->will($this->returnValue(false));
...@@ -214,4 +339,38 @@ class Storage extends \Test\TestCase { ...@@ -214,4 +339,38 @@ class Storage extends \Test\TestCase {
$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/'); $results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
$this->assertEquals(0, count($results)); $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));
}
} }
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment