Merge pull request #16322 from owncloud/trash-view

dont go trough the view when moving to trash

apps/files_trashbin/lib/trashbin.php

@@ -37,6 +37,7 @@
 namespace OCA\Files_Trashbin;
 
 use OC\Files\Filesystem;
+use OC\Files\View;
 use OCA\Files_Trashbin\Command\Expire;
 
 class Trashbin {
@@ -124,6 +125,7 @@ class Trashbin {
 
 	/**
 	 * copy file to owners trash
+	 *
 	 * @param string $sourcePath
 	 * @param string $owner
 	 * @param string $ownerPath
@@ -184,25 +186,32 @@ class Trashbin {
 
 		// disable proxy to prevent recursive calls
 		$trashPath = '/files_trashbin/files/' . $filename . '.d' . $timestamp;
+
+		/** @var \OC\Files\Storage\Storage $trashStorage */
+		list($trashStorage, $trashInternalPath) = $view->resolvePath($trashPath);
+		/** @var \OC\Files\Storage\Storage $sourceStorage */
+		list($sourceStorage, $sourceInternalPath) = $view->resolvePath('/files/' . $file_path);
 		try {
-			$sizeOfAddedFiles = $view->filesize('/files/' . $file_path);
-			if ($view->file_exists($trashPath)) {
-				$view->unlink($trashPath);
+			$sizeOfAddedFiles = $sourceStorage->filesize($sourceInternalPath);
+			if ($trashStorage->file_exists($trashInternalPath)) {
+				$trashStorage->unlink($trashInternalPath);
 			}
-			$view->rename('/files/' . $file_path, $trashPath);
+			$trashStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
 		} catch (\OCA\Files_Trashbin\Exceptions\CopyRecursiveException $e) {
 			$sizeOfAddedFiles = false;
-			if ($view->file_exists($trashPath)) {
-				$view->deleteAll($trashPath);
+			if ($trashStorage->file_exists($trashInternalPath)) {
+				$trashStorage->unlink($trashInternalPath);
 			}
 			\OC_Log::write('files_trashbin', 'Couldn\'t move ' . $file_path . ' to the trash bin', \OC_log::ERROR);
 		}
 
-		if ($view->file_exists('/files/' . $file_path)) { // failed to delete the original file, abort
-			$view->unlink($trashPath);
+		if ($sourceStorage->file_exists($sourceInternalPath)) { // failed to delete the original file, abort
+			$sourceStorage->unlink($sourceInternalPath);
 			return false;
 		}
 
+		$view->getUpdater()->rename('/files/' . $file_path, $trashPath);
+
 		if ($sizeOfAddedFiles !== false) {
 			$size = $sizeOfAddedFiles;
 			$query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)");
@@ -261,14 +270,15 @@ class Trashbin {
 				if ($owner !== $user) {
 					self::copy_recursive($owner . '/files_versions/' . $ownerPath, $owner . '/files_trashbin/versions/' . basename($ownerPath) . '.d' . $timestamp, $rootView);
 				}
-				$rootView->rename($owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . $filename . '.d' . $timestamp);
+				self::move($rootView, $owner . '/files_versions/' . $ownerPath, $user . '/files_trashbin/versions/' . $filename . '.d' . $timestamp);
 			} else if ($versions = \OCA\Files_Versions\Storage::getVersions($owner, $ownerPath)) {
+
 				foreach ($versions as $v) {
-					$size += $rootView->filesize($owner . '/files_versions' . $v['path'] . '.v' . $v['version']);
+					$size += $rootView->filesize($owner . '/files_versions/' . $v['path'] . '.v' . $v['version']);
 					if ($owner !== $user) {
-						$rootView->copy($owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $owner . '/files_trashbin/versions/' . $v['name'] . '.v' . $v['version'] . '.d' . $timestamp);
+						self::copy($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $owner . '/files_trashbin/versions/' . $v['name'] . '.v' . $v['version'] . '.d' . $timestamp);
 					}
-					$rootView->rename($owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . $filename . '.v' . $v['version'] . '.d' . $timestamp);
+					self::move($rootView, $owner . '/files_versions' . $v['path'] . '.v' . $v['version'], $user . '/files_trashbin/versions/' . $filename . '.v' . $v['version'] . '.d' . $timestamp);
 				}
 			}
 		}
@@ -276,6 +286,50 @@ class Trashbin {
 		return $size;
 	}
 
+	/**
+	 * Move a file or folder on storage level
+	 *
+	 * @param View $view
+	 * @param string $source
+	 * @param string $target
+	 * @return bool
+	 */
+	private static function move(View $view, $source, $target) {
+		/** @var \OC\Files\Storage\Storage $sourceStorage */
+		list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
+		/** @var \OC\Files\Storage\Storage $targetStorage */
+		list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
+		/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
+
+		$result = $targetStorage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
+		if ($result) {
+			$view->getUpdater()->rename($source, $target);
+		}
+		return $result;
+	}
+
+	/**
+	 * Copy a file or folder on storage level
+	 *
+	 * @param View $view
+	 * @param string $source
+	 * @param string $target
+	 * @return bool
+	 */
+	private static function copy(View $view, $source, $target) {
+		/** @var \OC\Files\Storage\Storage $sourceStorage */
+		list($sourceStorage, $sourceInternalPath) = $view->resolvePath($source);
+		/** @var \OC\Files\Storage\Storage $targetStorage */
+		list($targetStorage, $targetInternalPath) = $view->resolvePath($target);
+		/** @var \OC\Files\Storage\Storage $ownerTrashStorage */
+
+		$result = $targetStorage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
+		if ($result) {
+			$view->getUpdater()->update($target);
+		}
+		return $result;
+	}
+
 	/**
 	 * Restore a file or folder from trash bin
 	 *
@@ -299,7 +353,7 @@ class Trashbin {
 				// if location no longer exists, restore file in the root directory
 				if ($location !== '/' &&
 					(!$view->is_dir('files/' . $location) ||
-					!$view->isCreatable('files/' . $location))
+						!$view->isCreatable('files/' . $location))
 				) {
 					$location = '';
 				}
@@ -569,6 +623,7 @@ class Trashbin {
 
 	/**
 	 * resize trash bin if necessary after a new file was added to ownCloud
+	 *
 	 * @param string $user user id
 	 */
 	public static function resizeTrash($user) {
@@ -623,6 +678,7 @@ class Trashbin {
 	/**
 	 * if the size limit for the trash bin is reached, we delete the oldest
 	 * files in the trash bin until we meet the limit again
+	 *
 	 * @param array $files
 	 * @param string $user
 	 * @param int $availableSpace available disc space
@@ -725,7 +781,7 @@ class Trashbin {
 		//force rescan of versions, local storage may not have updated the cache
 		if (!self::$scannedVersions) {
 			/** @var \OC\Files\Storage\Storage $storage */
-			list($storage, ) = $view->resolvePath('/');
+			list($storage,) = $view->resolvePath('/');
 			$storage->getScanner()->scan('files_trashbin/versions');
 			self::$scannedVersions = true;
 		}
@@ -788,6 +844,7 @@ class Trashbin {
 
 	/**
 	 * get the size from a given root folder
+	 *
 	 * @param \OC\Files\View $view file view on the root folder
 	 * @return integer size of the folder
 	 */
@@ -799,7 +856,7 @@ class Trashbin {
 		$iterator = new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($root), \RecursiveIteratorIterator::CHILD_FIRST);
 		$size = 0;
 
-                /**
+		/**
 		 * RecursiveDirectoryIterator on an NFS path isn't iterable with foreach
 		 * This bug is fixed in PHP 5.5.9 or before
 		 * See #8376
@@ -845,6 +902,7 @@ class Trashbin {
 
 	/**
 	 * check if trash bin is empty for a given user
+	 *
 	 * @param string $user
 	 * @return bool
 	 */

apps/files_trashbin/tests/storage.php

@@ -316,9 +316,15 @@ class Storage extends \Test\TestCase {
 		 */
 		$storage = $this->getMockBuilder('\OC\Files\Storage\Temporary')
 			->setConstructorArgs([[]])
-			->setMethods(['rename', 'unlink'])
+			->setMethods(['rename', 'unlink', 'moveFromStorage'])
 			->getMock();
 
+		$storage->expects($this->any())
+			->method('rename')
+			->will($this->returnValue(false));
+		$storage->expects($this->any())
+			->method('moveFromStorage')
+			->will($this->returnValue(false));
 		$storage->expects($this->any())
 			->method('unlink')
 			->will($this->returnValue(false));

lib/private/files/storage/common.php

@@ -498,18 +498,18 @@ abstract class Common implements Storage {
 	 * @throws InvalidPathException
 	 */
 	private function scanForInvalidCharacters($fileName, $invalidChars) {
-		foreach(str_split($invalidChars) as $char) {
+		foreach (str_split($invalidChars) as $char) {
 			if (strpos($fileName, $char) !== false) {
 				throw new InvalidCharacterInPathException();
 			}
 		}
 
 		$sanitizedFileName = filter_var($fileName, FILTER_UNSAFE_RAW, FILTER_FLAG_STRIP_LOW);
-		if($sanitizedFileName !== $fileName) {
+		if ($sanitizedFileName !== $fileName) {
 			throw new InvalidCharacterInPathException();
 		}
 	}
-	
+
 	/**
 	 * @param array $options
 	 */
@@ -525,6 +525,7 @@ abstract class Common implements Storage {
 	public function getMountOption($name, $default = null) {
 		return isset($this->mountOptions[$name]) ? $this->mountOptions[$name] : $default;
 	}
+
 	/**
 	 * @param \OCP\Files\Storage $sourceStorage
 	 * @param string $sourceInternalPath
@@ -533,6 +534,10 @@ abstract class Common implements Storage {
 	 * @return bool
 	 */
 	public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath, $preserveMtime = false) {
+		if ($sourceStorage === $this) {
+			return $this->copy($sourceInternalPath, $targetInternalPath);
+		}
+
 		if ($sourceStorage->is_dir($sourceInternalPath)) {
 			$dh = $sourceStorage->opendir($sourceInternalPath);
 			$result = $this->mkdir($targetInternalPath);
@@ -575,6 +580,10 @@ abstract class Common implements Storage {
 	 * @return bool
 	 */
 	public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
+		if ($sourceStorage === $this) {
+			return $this->rename($sourceInternalPath, $targetInternalPath);
+		}
+
 		$result = $this->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath, true);
 		if ($result) {
 			if ($sourceStorage->is_dir($sourceInternalPath)) {

lib/private/files/storage/wrapper/wrapper.php

@@ -513,6 +513,10 @@ class Wrapper implements \OC\Files\Storage\Storage {
 	 * @return bool
 	 */
 	public function copyFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
+		if ($sourceStorage === $this) {
+			return $this->copy($sourceInternalPath, $targetInternalPath);
+		}
+
 		return $this->storage->copyFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
 	}
 
@@ -523,6 +527,10 @@ class Wrapper implements \OC\Files\Storage\Storage {
 	 * @return bool
 	 */
 	public function moveFromStorage(\OCP\Files\Storage $sourceStorage, $sourceInternalPath, $targetInternalPath) {
+		if ($sourceStorage === $this) {
+			return $this->rename($sourceInternalPath, $targetInternalPath);
+		}
+
 		return $this->storage->moveFromStorage($sourceStorage, $sourceInternalPath, $targetInternalPath);
 	}
 

tests/lib/files/storage/storage.php

@@ -176,7 +176,7 @@ abstract class Storage extends \Test\TestCase {
 		];
 	}
 
-	public function initSourceAndTarget ($source, $target = null) {
+	public function initSourceAndTarget($source, $target = null) {
 		$textFile = \OC::$SERVERROOT . '/tests/data/lorem.txt';
 		$this->instance->file_put_contents($source, file_get_contents($textFile));
 		if ($target) {
@@ -185,12 +185,12 @@ abstract class Storage extends \Test\TestCase {
 		}
 	}
 
-	public function assertSameAsLorem ($file) {
+	public function assertSameAsLorem($file) {
 		$textFile = \OC::$SERVERROOT . '/tests/data/lorem.txt';
 		$this->assertEquals(
 			file_get_contents($textFile),
 			$this->instance->file_get_contents($file),
-			'Expected '.$file.' to be a copy of '.$textFile
+			'Expected ' . $file . ' to be a copy of ' . $textFile
 		);
 	}
 
@@ -202,9 +202,9 @@ abstract class Storage extends \Test\TestCase {
 
 		$this->instance->copy($source, $target);
 
-		$this->assertTrue($this->instance->file_exists($target), $target.' was not created');
+		$this->assertTrue($this->instance->file_exists($target), $target . ' was not created');
 		$this->assertSameAsLorem($target);
-		$this->assertTrue($this->instance->file_exists($source), $source.' was deleted');
+		$this->assertTrue($this->instance->file_exists($source), $source . ' was deleted');
 	}
 
 	/**
@@ -216,8 +216,8 @@ abstract class Storage extends \Test\TestCase {
 		$this->instance->rename($source, $target);
 
 		$this->wait();
-		$this->assertTrue($this->instance->file_exists($target), $target.' was not created');
-		$this->assertFalse($this->instance->file_exists($source), $source.' still exists');
+		$this->assertTrue($this->instance->file_exists($target), $target . ' was not created');
+		$this->assertFalse($this->instance->file_exists($source), $source . ' still exists');
 		$this->assertSameAsLorem($target);
 	}
 
@@ -225,12 +225,12 @@ abstract class Storage extends \Test\TestCase {
 	 * @dataProvider copyAndMoveProvider
 	 */
 	public function testCopyOverwrite($source, $target) {
-		$this->initSourceAndTarget($source,$target);
+		$this->initSourceAndTarget($source, $target);
 
 		$this->instance->copy($source, $target);
 
-		$this->assertTrue($this->instance->file_exists($target), $target.' was not created');
-		$this->assertTrue($this->instance->file_exists($source), $source.' was deleted');
+		$this->assertTrue($this->instance->file_exists($target), $target . ' was not created');
+		$this->assertTrue($this->instance->file_exists($source), $source . ' was deleted');
 		$this->assertSameAsLorem($target);
 		$this->assertSameAsLorem($source);
 	}
@@ -243,8 +243,8 @@ abstract class Storage extends \Test\TestCase {
 
 		$this->instance->rename($source, $target);
 
-		$this->assertTrue($this->instance->file_exists($target), $target.' was not created');
-		$this->assertFalse($this->instance->file_exists($source), $source.' still exists');
+		$this->assertTrue($this->instance->file_exists($target), $target . ' was not created');
+		$this->assertFalse($this->instance->file_exists($source), $source . ' still exists');
 		$this->assertSameAsLorem($target);
 	}
 
@@ -535,4 +535,17 @@ abstract class Storage extends \Test\TestCase {
 		$this->assertTrue($this->instance->instanceOfStorage(get_class($this->instance)));
 		$this->assertFalse($this->instance->instanceOfStorage('\OC'));
 	}
+
+	/**
+	 * @dataProvider copyAndMoveProvider
+	 */
+	public function testCopyFromSameStorage($source, $target) {
+		$this->initSourceAndTarget($source);
+
+		$this->instance->copyFromStorage($this->instance, $source, $target);
+
+		$this->assertTrue($this->instance->file_exists($target), $target . ' was not created');
+		$this->assertSameAsLorem($target);
+		$this->assertTrue($this->instance->file_exists($source), $source . ' was deleted');
+	}
 }