Cleanup part file after upload exception

Added unit tests for checking for stray part files.
Convert exception to sabre exception in upload put method.

Also added unit test for exception mapping, which also indirectly tests
that the part file is being deleted on exception.

This applies to both chunking and non-chunking mode.

Added some unit tests for chunk upload.

1 files changed, 314 insertions, 29 deletions

diff --git a/tests/lib/connector/sabre/file.php b/tests/lib/connector/sabre/file.php
index 834698d90cb..246f7f6d0a2 100644
--- a/tests/lib/connector/sabre/file.php
+++ b/tests/lib/connector/sabre/file.php
@@ -10,6 +10,7 @@ namespace Test\Connector\Sabre;
 
 use Test\HookHelper;
 use OC\Files\Filesystem;
+use OCP\Lock\ILockingProvider;
 
 class File extends \Test\TestCase {
 
@@ -33,6 +34,7 @@ class File extends \Test\TestCase {
 	public function tearDown() {
 		$userManager = \OC::$server->getUserManager();
 		$userManager->get($this->user)->delete();
+		unset($_SERVER['HTTP_OC_CHUNKED']);
 
 		parent::tearDown();
 	}
@@ -44,23 +46,92 @@ class File extends \Test\TestCase {
 		return $stream;
 	}
 
+
+	public function fopenFailuresProvider() {
+		return [
+			[
+				// return false
+				null,
+				'\Sabre\Dav\Exception',
+				false
+			],
+			[
+				new \OCP\Files\NotPermittedException(),
+				'Sabre\DAV\Exception\Forbidden'
+			],
+			[
+				new \OCP\Files\EntityTooLargeException(),
+				'OC\Connector\Sabre\Exception\EntityTooLarge'
+			],
+			[
+				new \OCP\Files\InvalidContentException(),
+				'OC\Connector\Sabre\Exception\UnsupportedMediaType'
+			],
+			[
+				new \OCP\Files\InvalidPathException(),
+				'Sabre\DAV\Exception\Forbidden'
+			],
+			[
+				new \OCP\Files\LockNotAcquiredException('/test.txt', 1),
+				'OC\Connector\Sabre\Exception\FileLocked'
+			],
+			[
+				new \OCP\Lock\LockedException('/test.txt'),
+				'OC\Connector\Sabre\Exception\FileLocked'
+			],
+			[
+				new \OCP\Encryption\Exceptions\GenericEncryptionException(),
+				'Sabre\DAV\Exception\ServiceUnavailable'
+			],
+			[
+				new \OCP\Files\StorageNotAvailableException(),
+				'Sabre\DAV\Exception\ServiceUnavailable'
+			],
+			[
+				new \Sabre\DAV\Exception('Generic sabre exception'),
+				'Sabre\DAV\Exception',
+				false
+			],
+			[
+				new \Exception('Generic exception'),
+				'Sabre\DAV\Exception'
+			],
+		];
+	}
+
 	/**
-	 * @expectedException \Sabre\DAV\Exception
+	 * @dataProvider fopenFailuresProvider
 	 */
-	public function testSimplePutFails() {
+	public function testSimplePutFails($thrownException, $expectedException, $checkPreviousClass = true) {
 		// setup
-		$storage = $this->getMock('\OC\Files\Storage\Local', ['fopen'], [['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]);
+		$storage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['fopen'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
 		$view = $this->getMock('\OC\Files\View', array('getRelativePath', 'resolvePath'), array());
-		$view->expects($this->any())
+		$view->expects($this->atLeastOnce())
 			->method('resolvePath')
-			->will($this->returnValue(array($storage, '')));
-		$storage->expects($this->once())
-			->method('fopen')
-			->will($this->returnValue(false));
+			->will($this->returnCallback(
+				function($path) use ($storage){
+					return [$storage, $path];
+				}
+			));
+
+		if ($thrownException !== null) {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->throwException($thrownException));
+		} else {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->returnValue(false));
+		}
 
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
 
 		$info = new \OC\Files\FileInfo('/test.txt', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -69,9 +140,97 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put('test data');
+		$caughtException = null;
+		try {
+			$file->put('test data');
+		} catch (\Exception $e) {
+			$caughtException = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $caughtException);
+		if ($checkPreviousClass) {
+			$this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious());
+		}
+
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
+	/**
+	 * Test putting a file using chunking
+	 *
+	 * @dataProvider fopenFailuresProvider
+	 */
+	public function testChunkedPutFails($thrownException, $expectedException, $checkPreviousClass = false) {
+		// setup
+		$storage = $this->getMock(
+			'\OC\Files\Storage\Local',
+			['fopen'],
+			[['datadir' => \OC::$server->getTempManager()->getTemporaryFolder()]]
+		);
+		\OC\Files\Filesystem::mount($storage, [], $this->user . '/');
+		$view = $this->getMock('\OC\Files\View', ['getRelativePath', 'resolvePath'], []);
+		$view->expects($this->atLeastOnce())
+			->method('resolvePath')
+			->will($this->returnCallback(
+				function($path) use ($storage){
+					return [$storage, $path];
+				}
+			));
+
+		if ($thrownException !== null) {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->throwException($thrownException));
+		} else {
+			$storage->expects($this->once())
+				->method('fopen')
+				->will($this->returnValue(false));
+		}
+
+		$view->expects($this->any())
+			->method('getRelativePath')
+			->will($this->returnArgument(0));
+
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+
+		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-0', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// put first chunk
+		$this->assertNull($file->put('test data one'));
+
+		$info = new \OC\Files\FileInfo('/test.txt-chunking-12345-2-1', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$caughtException = null;
+		try {
+			// last chunk
+			$file->put('test data two');
+		} catch (\Exception $e) {
+			$caughtException = $e;
+		}
+
+		$this->assertInstanceOf($expectedException, $caughtException);
+		if ($checkPreviousClass) {
+			$this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious());
+		}
+
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Simulate putting a file to the given path.
+	 *
+	 * @param string $path path to put the file into
+	 * @param string $viewRoot root to use for the view
+	 *
+	 * @return result of the PUT operaiton which is usually the etag
+	 */
 	private function doPut($path, $viewRoot = null) {
 		$view = \OC\Files\Filesystem::getView();
 		if (!is_null($viewRoot)) {
@@ -90,14 +249,23 @@ class File extends \Test\TestCase {
 
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
-		$this->assertNotEmpty($file->put($this->getStream('test data')));
+		return $file->put($this->getStream('test data'));
 	}
 
 	/**
 	 * Test putting a single file
 	 */
 	public function testPutSingleFile() {
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
+	}
+
+	/**
+	 * Test putting a file using chunking
+	 */
+	public function testChunkedPut() {
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+		$this->assertNull($this->doPut('/test.txt-chunking-12345-2-0'));
+		$this->assertNotEmpty($this->doPut('/test.txt-chunking-12345-2-1'));
 	}
 
 	/**
@@ -106,7 +274,7 @@ class File extends \Test\TestCase {
 	public function testPutSingleFileTriggersHooks() {
 		HookHelper::setUpHooks();
 
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -140,7 +308,7 @@ class File extends \Test\TestCase {
 
 		HookHelper::setUpHooks();
 
-		$this->doPut('/foo.txt');
+		$this->assertNotEmpty($this->doPut('/foo.txt'));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -177,7 +345,7 @@ class File extends \Test\TestCase {
 		HookHelper::setUpHooks();
 
 		// happens with public webdav where the view root is the share root
-		$this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot');
+		$this->assertNotEmpty($this->doPut('/foo.txt', '/' . $this->user . '/files/noderoot'));
 
 		$this->assertCount(4, HookHelper::$hookCalls);
 		$this->assertHookCall(
@@ -211,8 +379,6 @@ class File extends \Test\TestCase {
 
 	/**
 	 * Test put file with cancelled hook
-	 *
-	 * @expectedException \Sabre\DAV\Exception
 	 */
 	public function testPutSingleFileCancelPreHook() {
 		\OCP\Util::connectHook(
@@ -222,13 +388,22 @@ class File extends \Test\TestCase {
 			'cancellingCallback'
 		);
 
-		$this->doPut('/foo.txt');
+		// action
+		$thrown = false;
+		try {
+			$this->doPut('/foo.txt');
+		} catch (\Sabre\DAV\Exception $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles(), 'No stray part files');
 	}
 
 	/**
-	 * @expectedException \Sabre\DAV\Exception
+	 * Test exception when the uploaded size did not match
 	 */
-	public function testSimplePutFailsOnRename() {
+	public function testSimplePutFailsSizeCheck() {
 		// setup
 		$view = $this->getMock('\OC\Files\View',
 			array('rename', 'getRelativePath', 'filesize'));
@@ -238,7 +413,8 @@ class File extends \Test\TestCase {
 			->will($this->returnValue(false));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
+
 		$view->expects($this->any())
 			->method('filesize')
 			->will($this->returnValue(123456));
@@ -253,18 +429,87 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\Sabre\DAV\Exception\BadRequest $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
 	/**
-	 * @expectedException \OC\Connector\Sabre\Exception\InvalidPath
+	 * Test exception during final rename in simple upload mode
+	 */
+	public function testSimplePutFailsMoveFromStorage() {
+		$view = new \OC\Files\View('/' . $this->user . '/files');
+
+		// simulate situation where the target file is locked
+		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt', null, null, array(
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		), null);
+
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\FileLocked $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Test exception during final rename in chunk upload mode
+	 */
+	public function testChunkedPutFailsFinalRename() {
+		$view = new \OC\Files\View('/' . $this->user . '/files');
+
+		// simulate situation where the target file is locked
+		$view->lockFile('/test.txt', ILockingProvider::LOCK_EXCLUSIVE);
+
+		$_SERVER['HTTP_OC_CHUNKED'] = true;
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-0', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+		$this->assertNull($file->put('test data one'));
+
+		$info = new \OC\Files\FileInfo('/' . $this->user . '/files/test.txt-chunking-12345-2-1', null, null, [
+			'permissions' => \OCP\Constants::PERMISSION_ALL
+		], null);
+		$file = new \OC\Connector\Sabre\File($view, $info);
+
+		// action
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\FileLocked $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
+	}
+
+	/**
+	 * Test put file with invalid chars
 	 */
 	public function testSimplePutInvalidChars() {
 		// setup
 		$view = $this->getMock('\OC\Files\View', array('getRelativePath'));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/*'));
+			->will($this->returnArgument(0));
 
 		$info = new \OC\Files\FileInfo('/*', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -272,7 +517,15 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\OC\Connector\Sabre\Exception\InvalidPath $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
 	/**
@@ -286,7 +539,7 @@ class File extends \Test\TestCase {
 
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/*'));
+			->will($this->returnArgument(0));
 
 		$info = new \OC\Files\FileInfo('/*', null, null, array(
 			'permissions' => \OCP\Constants::PERMISSION_ALL
@@ -296,7 +549,6 @@ class File extends \Test\TestCase {
 	}
 
 	/**
-	 * @expectedException \Sabre\DAV\Exception\BadRequest
 	 */
 	public function testUploadAbort() {
 		// setup
@@ -308,7 +560,7 @@ class File extends \Test\TestCase {
 			->will($this->returnValue(false));
 		$view->expects($this->any())
 			->method('getRelativePath')
-			->will($this->returnValue('/test.txt'));
+			->will($this->returnArgument(0));
 		$view->expects($this->any())
 			->method('filesize')
 			->will($this->returnValue(123456));
@@ -323,7 +575,15 @@ class File extends \Test\TestCase {
 		$file = new \OC\Connector\Sabre\File($view, $info);
 
 		// action
-		$file->put($this->getStream('test data'));
+		$thrown = false;
+		try {
+			$file->put($this->getStream('test data'));
+		} catch (\Sabre\DAV\Exception\BadRequest $e) {
+			$thrown = true;
+		}
+
+		$this->assertTrue($thrown);
+		$this->assertEmpty($this->listPartFiles($view, ''), 'No stray part files');
 	}
 
 	/**
@@ -484,4 +744,29 @@ class File extends \Test\TestCase {
 		);
 	}
 
+	/**
+	 * Returns part files in the given path
+	 *
+	 * @param \OC\Files\View view which root is the current user's "files" folder
+	 * @param string $path path for which to list part files
+	 *
+	 * @return array list of part files
+	 */
+	private function listPartFiles(\OC\Files\View $userView = null, $path = '') {
+		if ($userView === null) {
+			$userView = \OC\Files\Filesystem::getView();
+		}
+		$files = [];
+		list($storage, $internalPath) = $userView->resolvePath($path);
+		$realPath = $storage->getSourcePath($internalPath);
+		$dh = opendir($realPath);
+		while (($file = readdir($dh)) !== false) {
+			if (substr($file, strlen($file) - 5, 5) === '.part') {
+				$files[] = $file;
+			}
+		}
+		closedir($dh);
+		return $files;
+	}
+
 }