Merge pull request #21628 from nextcloud/external-to-s3-trashbin-fix

fix moving files from external storage to object store trashbin

5 files changed, 40 insertions, 27 deletions

diff --git a/apps/files_trashbin/lib/Trashbin.php b/apps/files_trashbin/lib/Trashbin.php
index f73cc1f4aa6..db00a7ed272 100644
--- a/apps/files_trashbin/lib/Trashbin.php
+++ b/apps/files_trashbin/lib/Trashbin.php
@@ -278,6 +278,10 @@ class Trashbin {
 		/** @var \OC\Files\Storage\Storage $sourceStorage */
 		[$sourceStorage, $sourceInternalPath] = $ownerView->resolvePath('/files/' . $ownerPath);
 
+		$connection = \OC::$server->getDatabaseConnection();
+		$connection->beginTransaction();
+		$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
+
 		try {
 			$moveSuccessful = true;
 			if ($trashStorage->file_exists($trashInternalPath)) {
@@ -298,10 +302,11 @@ class Trashbin {
 			} else {
 				$sourceStorage->unlink($sourceInternalPath);
 			}
+			$connection->rollBack();
 			return false;
 		}
 
-		$trashStorage->getUpdater()->renameFromStorage($sourceStorage, $sourceInternalPath, $trashInternalPath);
+		$connection->commit();
 
 		if ($moveSuccessful) {
 			$query = \OC_DB::prepare("INSERT INTO `*PREFIX*files_trash` (`id`,`timestamp`,`location`,`user`) VALUES (?,?,?,?)");
diff --git a/lib/private/Files/Cache/Updater.php b/lib/private/Files/Cache/Updater.php
index 60534a153dd..b6a0e62a88a 100644
--- a/lib/private/Files/Cache/Updater.php
+++ b/lib/private/Files/Cache/Updater.php
@@ -28,6 +28,7 @@
 
 namespace OC\Files\Cache;
 
+use OC\Files\FileInfo;
 use OCP\Files\Cache\ICacheEntry;
 use OCP\Files\Cache\IUpdater;
 use OCP\Files\Storage\IStorage;
@@ -187,7 +188,9 @@ class Updater implements IUpdater {
 		$sourceUpdater = $sourceStorage->getUpdater();
 		$sourcePropagator = $sourceStorage->getPropagator();
 
-		if ($sourceCache->inCache($source)) {
+		$sourceInfo = $sourceCache->get($source);
+
+		if ($sourceInfo !== false) {
 			if ($this->cache->inCache($target)) {
 				$this->cache->remove($target);
 			}
@@ -197,13 +200,13 @@ class Updater implements IUpdater {
 			} else {
 				$this->cache->moveFromCache($sourceCache, $source, $target);
 			}
-		}
 
-		if (pathinfo($source, PATHINFO_EXTENSION) !== pathinfo($target, PATHINFO_EXTENSION)) {
-			// handle mime type change
-			$mimeType = $this->storage->getMimeType($target);
-			$fileId = $this->cache->getId($target);
-			$this->cache->update($fileId, ['mimetype' => $mimeType]);
+			if (pathinfo($source, PATHINFO_EXTENSION) !== pathinfo($target, PATHINFO_EXTENSION) && $sourceInfo->getMimeType() !== FileInfo::MIMETYPE_FOLDER) {
+				// handle mime type change
+				$mimeType = $this->storage->getMimeType($target);
+				$fileId = $this->cache->getId($target);
+				$this->cache->update($fileId, ['mimetype' => $mimeType]);
+			}
 		}
 
 		if ($sourceCache instanceof Cache) {
diff --git a/lib/private/Files/Storage/Common.php b/lib/private/Files/Storage/Common.php
index a62b7d727fb..958d09832c4 100644
--- a/lib/private/Files/Storage/Common.php
+++ b/lib/private/Files/Storage/Common.php
@@ -53,6 +53,7 @@ use OC\Files\Storage\Wrapper\Jail;
 use OC\Files\Storage\Wrapper\Wrapper;
 use OCP\Files\EmptyFileNameException;
 use OCP\Files\FileNameTooLongException;
+use OCP\Files\GenericFileException;
 use OCP\Files\InvalidCharacterInPathException;
 use OCP\Files\InvalidDirectoryException;
 use OCP\Files\InvalidPathException;
@@ -620,18 +621,19 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 			}
 		} else {
 			$source = $sourceStorage->fopen($sourceInternalPath, 'r');
-			// TODO: call fopen in a way that we execute again all storage wrappers
-			// to avoid that we bypass storage wrappers which perform important actions
-			// for this operation. Same is true for all other operations which
-			// are not the same as the original one.Once this is fixed we also
-			// need to adjust the encryption wrapper.
-			$target = $this->fopen($targetInternalPath, 'w');
-			[, $result] = \OC_Helper::streamCopy($source, $target);
+			$result = false;
+			if ($source) {
+				try {
+					$this->writeStream($targetInternalPath, $source);
+					$result = true;
+				} catch (\Exception $e) {
+					\OC::$server->getLogger()->logException($e, ['level' => ILogger::WARN, 'message' => 'Failed to copy stream to storage']);
+				}
+			}
+
 			if ($result and $preserveMtime) {
 				$this->touch($targetInternalPath, $sourceStorage->filemtime($sourceInternalPath));
 			}
-			fclose($source);
-			fclose($target);
 
 			if (!$result) {
 				// delete partially written target file
@@ -858,10 +860,13 @@ abstract class Common implements Storage, ILockingStorage, IWriteStreamStorage {
 	public function writeStream(string $path, $stream, int $size = null): int {
 		$target = $this->fopen($path, 'w');
 		if (!$target) {
-			return 0;
+			throw new GenericFileException("Failed to open $path for writing");
 		}
 		try {
 			[$count, $result] = \OC_Helper::streamCopy($stream, $target);
+			if (!$result) {
+				throw new GenericFileException("Failed to copy stream");
+			}
 		} finally {
 			fclose($target);
 			fclose($stream);
diff --git a/lib/private/Files/Storage/Local.php b/lib/private/Files/Storage/Local.php
index 4cf3ac4799f..0b636d06bde 100644
--- a/lib/private/Files/Storage/Local.php
+++ b/lib/private/Files/Storage/Local.php
@@ -44,6 +44,7 @@ use OC\Files\Filesystem;
 use OC\Files\Storage\Wrapper\Jail;
 use OCP\Constants;
 use OCP\Files\ForbiddenException;
+use OCP\Files\GenericFileException;
 use OCP\Files\Storage\IStorage;
 use OCP\ILogger;
 
@@ -553,6 +554,11 @@ class Local extends \OC\Files\Storage\Common {
 	}
 
 	public function writeStream(string $path, $stream, int $size = null): int {
-		return (int)file_put_contents($this->getSourcePath($path), $stream);
+		$result = file_put_contents($this->getSourcePath($path), $stream);
+		if ($result === false) {
+			throw new GenericFileException("Failed write steam to $path");
+		} else {
+			return $result;
+		}
 	}
 }
diff --git a/tests/lib/Files/ViewTest.php b/tests/lib/Files/ViewTest.php
index 036fc038a60..fad2217bfdb 100644
--- a/tests/lib/Files/ViewTest.php
+++ b/tests/lib/Files/ViewTest.php
@@ -13,7 +13,6 @@ use OC\Files\Filesystem;
 use OC\Files\Mount\MountPoint;
 use OC\Files\Storage\Common;
 use OC\Files\Storage\Temporary;
-use OC\Files\Stream\Quota;
 use OC\Files\View;
 use OCP\Constants;
 use OCP\Files\Config\IMountProvider;
@@ -1167,13 +1166,8 @@ class ViewTest extends \Test\TestCase {
 			->setMethods(['fopen'])
 			->getMock();
 
-		$storage2->expects($this->any())
-			->method('fopen')
-			->willReturnCallback(function ($path, $mode) use ($storage2) {
-				/** @var \PHPUnit_Framework_MockObject_MockObject | \OC\Files\Storage\Temporary $storage2 */
-				$source = fopen($storage2->getSourcePath($path), $mode);
-				return Quota::wrap($source, 9);
-			});
+		$storage2->method('writeStream')
+			->willReturn(0);
 
 		$storage1->mkdir('sub');
 		$storage1->file_put_contents('foo.txt', '0123456789ABCDEFGH');