Merge pull request #13508 from owncloud/failed-delete-cache

Dont remove a file from cache if the delete operation failed

apps/files_trashbin/lib/storage.php

@@ -80,11 +80,15 @@ class Storage extends Wrapper {
 				$result = \OCA\Files_Trashbin\Trashbin::move2trash($filesPath);
 				// in cross-storage cases the file will be copied
 				// but not deleted, so we delete it here
-				$this->storage->unlink($path);
+				if ($result) {
+					$this->storage->unlink($path);
+				}
 			} else {
 				$result = $this->storage->unlink($path);
 			}
 			unset($this->deletedFiles[$normalized]);
+		} else if ($this->storage->file_exists($path)) {
+			$result = $this->storage->unlink($path);
 		}
 
 		return $result;

apps/files_trashbin/lib/trashbin.php

@@ -180,6 +180,11 @@ class Trashbin {
 		}
 		\OC_FileProxy::$enabled = $proxyStatus;
 
+		if ($view->file_exists('/files/' . $file_path)) { // failed to delete the original file, abort
+			$view->unlink($trashPath);
+			return false;
+		}
+
 		if ($sizeOfAddedFiles !== false) {
 			$size = $sizeOfAddedFiles;
 			$query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)");

apps/files_trashbin/tests/storage.php

@@ -71,7 +71,7 @@ class Storage extends \Test\TestCase {
 	public function testSingleStorageDelete() {
 		$this->assertTrue($this->userView->file_exists('test.txt'));
 		$this->userView->unlink('test.txt');
-		list($storage, ) = $this->userView->resolvePath('test.txt');
+		list($storage,) = $this->userView->resolvePath('test.txt');
 		$storage->getScanner()->scan(''); // make sure we check the storage
 		$this->assertFalse($this->userView->getFileInfo('test.txt'));
 
@@ -123,7 +123,7 @@ class Storage extends \Test\TestCase {
 		$this->userView->unlink('test.txt');
 
 		// rescan trash storage
-		list($rootStorage, ) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// check if versions are in trashbin
@@ -158,7 +158,7 @@ class Storage extends \Test\TestCase {
 		$this->userView->file_exists('substorage/test.txt');
 
 		// rescan trash storage
-		list($rootStorage, ) = $this->rootView->resolvePath($this->user . '/files_trashbin');
+		list($rootStorage,) = $this->rootView->resolvePath($this->user . '/files_trashbin');
 		$rootStorage->getScanner()->scan('');
 
 		// versions were moved too
@@ -173,4 +173,37 @@ class Storage extends \Test\TestCase {
 		$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() {
+		/**
+		 * @var \OC\Files\Storage\Temporary | \PHPUnit_Framework_MockObject_MockObject $storage
+		 */
+		$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
+			->setConstructorArgs([[]])
+			->setMethods(['rename', 'unlink'])
+			->getMock();
+
+		$storage->expects($this->any())
+			->method('rename')
+			->will($this->returnValue(false));
+		$storage->expects($this->any())
+			->method('unlink')
+			->will($this->returnValue(false));
+
+		$cache = $storage->getCache();
+
+		Filesystem::mount($storage, [], '/' . $this->user . '/files');
+		$this->userView->file_put_contents('test.txt', 'foo');
+		$this->assertTrue($storage->file_exists('test.txt'));
+		$this->assertFalse($this->userView->unlink('test.txt'));
+		$this->assertTrue($storage->file_exists('test.txt'));
+		$this->assertTrue($cache->inCache('test.txt'));
+
+		// file should not be in the trashbin
+		$results = $this->rootView->getDirectoryContent($this->user . '/files_trashbin/files/');
+		$this->assertEquals(0, count($results));
+	}
 }

lib/private/files/view.php

@@ -527,7 +527,7 @@ class View {
 						fclose($target);
 
 						if ($result !== false) {
-							$storage1->unlink($internalPath1);
+							$result &= $storage1->unlink($internalPath1);
 						}
 					}
 				}
@@ -537,7 +537,7 @@ class View {
 					if ($this->shouldEmitHooks()) {
 						$this->emit_file_hooks_post($exists, $path2);
 					}
-				} elseif ($result !== false) {
+				} elseif ($result) {
 					$this->updater->rename($path1, $path2);
 					if ($this->shouldEmitHooks($path1) and $this->shouldEmitHooks($path2)) {
 						\OC_Hook::emit(
@@ -803,7 +803,7 @@ class View {
 
 				$result = \OC_FileProxy::runPostProxies($operation, $this->getAbsolutePath($path), $result);
 
-				if (in_array('delete', $hooks)) {
+				if (in_array('delete', $hooks) and $result) {
 					$this->updater->remove($path);
 				}
 				if (in_array('write', $hooks)) {

tests/lib/files/view.php

@@ -849,4 +849,27 @@ class View extends \Test\TestCase {
 
 		$this->assertEquals($time, $view->filemtime('/test/sub/storage/foo/bar.txt'));
 	}
+
+	public function testDeleteFailKeepCache() {
+		/**
+		 * @var \PHPUnit_Framework_MockObject_MockObject | \OC\Files\Storage\Temporary $storage
+		 */
+		$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
+			->setConstructorArgs(array(array()))
+			->setMethods(array('unlink'))
+			->getMock();
+		$storage->expects($this->once())
+			->method('unlink')
+			->will($this->returnValue(false));
+		$scanner = $storage->getScanner();
+		$cache = $storage->getCache();
+		$storage->file_put_contents('foo.txt', 'asd');
+		$scanner->scan('');
+		\OC\Files\Filesystem::mount($storage, array(), '/test/');
+
+		$view = new \OC\Files\View('/test');
+
+		$this->assertFalse($view->unlink('foo.txt'));
+		$this->assertTrue($cache->inCache('foo.txt'));
+	}
 }