Merge pull request #13032 from nextcloud/objectstore-write-exists

upload new files in objectstore to a .part path first

lib/private/Files/ObjectStore/Azure.php

@@ -115,4 +115,17 @@ class Azure implements IObjectStore {
 	public function deleteObject($urn) {
 		$this->getBlobClient()->deleteBlob($this->containerName, $urn);
 	}
+
+	public function objectExists($urn) {
+		try {
+			$this->getBlobClient()->getBlobMetadata($this->containerName, $urn);
+			return true;
+		} catch (ServiceException $e) {
+			if ($e->getCode() === 404) {
+				return false;
+			} else {
+				throw $e;
+			}
+		}
+	}
 }

lib/private/Files/ObjectStore/ObjectStoreStorage.php

@@ -436,7 +436,10 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common {
 		$stat['mimetype'] = $mimetype;
 		$stat['etag'] = $this->getETag($path);
 
-		$fileId = $this->getCache()->put($path, $stat);
+		$exists = $this->getCache()->inCache($path);
+		$uploadPath = $exists ? $path : $path . '.part';
+		$fileId = $this->getCache()->put($uploadPath, $stat);
+		$urn = $this->getURN($fileId);
 		try {
 			//upload to object storage
 			if ($size === null) {
@@ -446,22 +449,31 @@ class ObjectStoreStorage extends \OC\Files\Storage\Common {
 					]);
 					$size = $writtenSize;
 				});
-				$this->objectStore->writeObject($this->getURN($fileId), $countStream);
+				$this->objectStore->writeObject($urn, $countStream);
 				if (is_resource($countStream)) {
 					fclose($countStream);
 				}
 			} else {
-				$this->objectStore->writeObject($this->getURN($fileId), $stream);
+				$this->objectStore->writeObject($urn, $stream);
 			}
 		} catch (\Exception $ex) {
-			$this->getCache()->remove($path);
+			$this->getCache()->remove($uploadPath);
 			$this->logger->logException($ex, [
 				'app' => 'objectstore',
-				'message' => 'Could not create object ' . $this->getURN($fileId) . ' for ' . $path,
+				'message' => 'Could not create object ' . $urn . ' for ' . $path,
 			]);
 			throw $ex; // make this bubble up
 		}
 
+		if (!$exists) {
+			if ($this->objectStore->objectExists($urn)) {
+				$this->getCache()->move($uploadPath, $path);
+			} else {
+				$this->getCache()->remove($uploadPath);
+				throw new \Exception("Object not found after writing (urn: $urn, path: $path)", 404);
+			}
+		}
+
 		return $size;
 	}
 }

lib/private/Files/ObjectStore/S3ObjectTrait.php

@@ -90,4 +90,8 @@ trait S3ObjectTrait {
 			'Key' => $urn
 		]);
 	}
+
+	public function objectExists($urn) {
+		return $this->getConnection()->doesObjectExist($this->bucket, $urn);
+	}
 }

lib/private/Files/ObjectStore/StorageObjectStore.php

@@ -89,4 +89,7 @@ class StorageObjectStore implements IObjectStore {
 		$this->storage->unlink($urn);
 	}
 
+	public function objectExists($urn) {
+		return $this->storage->file_exists($urn);
+	}
 }

lib/private/Files/ObjectStore/Swift.php

@@ -128,4 +128,7 @@ class Swift implements IObjectStore {
 		$this->getContainer()->delete();
 	}
 
+	public function objectExists($urn) {
+		return $this->getContainer()->objectExists($urn);
+	}
 }

lib/public/Files/ObjectStore/IObjectStore.php

@@ -63,4 +63,13 @@ interface IObjectStore {
 	 * @since 7.0.0
 	 */
 	public function deleteObject($urn);
+
+	/**
+	 * Check if an object exists in the object store
+	 *
+	 * @param string $urn
+	 * @return bool
+	 * @since 16.0.0
+	 */
+	public function objectExists($urn);
 }

tests/lib/Files/ObjectStore/FailWriteObjectStore.php

@@ -0,0 +1,53 @@
+<?php declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2018 Robin Appelman <robin@icewind.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Test\Files\ObjectStore;
+
+use OCP\Files\ObjectStore\IObjectStore;
+
+class FailWriteObjectStore implements IObjectStore {
+	private $objectStore;
+
+	public function __construct(IObjectStore $objectStore) {
+		$this->objectStore = $objectStore;
+	}
+
+	public function getStorageId() {
+		return $this->objectStore->getStorageId();
+	}
+
+	public function readObject($urn) {
+		return $this->objectStore->readObject($urn);
+	}
+
+	public function writeObject($urn, $stream) {
+		// emulate a failed write that didn't throw an error
+		return true;
+	}
+
+	public function deleteObject($urn) {
+		$this->objectStore->deleteObject($urn);
+	}
+
+	public function objectExists($urn) {
+		return $this->objectStore->objectExists($urn);
+	}
+}

tests/lib/Files/ObjectStore/ObjectStoreStorageOverwrite.php

@@ -0,0 +1,38 @@
+<?php declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2018 Robin Appelman <robin@icewind.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace Test\Files\ObjectStore;
+
+use OC\Files\ObjectStore\ObjectStoreStorage;
+use OCP\Files\ObjectStore\IObjectStore;
+
+/**
+ * Allow overwriting the object store instance for test purposes
+ */
+class ObjectStoreStorageOverwrite extends ObjectStoreStorage {
+	public function setObjectStore(IObjectStore $objectStore) {
+		$this->objectStore = $objectStore;
+	}
+
+	public function getObjectStore(): IObjectStore {
+		return $this->objectStore;
+	}
+}

tests/lib/Files/ObjectStore/ObjectStoreStorageTest.php

@@ -30,6 +30,8 @@ use Test\Files\Storage\Storage;
  * @group DB
  */
 class ObjectStoreStorageTest extends Storage {
+	/** @var ObjectStoreStorageOverwrite */
+	protected $instance;
 
 	/**
 	 * @var IObjectStore
@@ -42,7 +44,7 @@ class ObjectStoreStorageTest extends Storage {
 		$baseStorage = new Temporary();
 		$this->objectStorage = new StorageObjectStore($baseStorage);
 		$config['objectstore'] = $this->objectStorage;
-		$this->instance = new ObjectStoreStorage($config);
+		$this->instance = new ObjectStoreStorageOverwrite($config);
 	}
 
 	protected function tearDown() {
@@ -166,4 +168,17 @@ class ObjectStoreStorageTest extends Storage {
 		$targetId = $this->instance->getCache()->getId('target');
 		$this->assertSame($sourceId, $targetId, 'fileid must be stable on move or shares will break');
 	}
+
+	public function testWriteObjectSilentFailure() {
+		$objectStore = $this->instance->getObjectStore();
+		$this->instance->setObjectStore(new FailWriteObjectStore($objectStore));
+
+		try {
+			$this->instance->file_put_contents('test.txt', 'foo');
+			$this->fail('expected exception');
+		} catch (\Exception $e) {
+			$this->assertStringStartsWith('Object not found after writing', $e->getMessage());
+		}
+		$this->assertFalse($this->instance->file_exists('test.txt'));
+	}
 }

tests/lib/Files/ObjectStore/ObjectStoreTest.php

@@ -94,4 +94,19 @@ abstract class ObjectStoreTest extends TestCase {
 			$this->assertEquals(1, 1);
 		}
 	}
+
+	public function testExists() {
+		$stream = $this->stringToStream('foobar');
+
+		$instance = $this->getInstance();
+		$this->assertFalse($instance->objectExists('2'));
+
+		$instance->writeObject('2', $stream);
+
+		$this->assertTrue($instance->objectExists('2'));
+
+		$instance->deleteObject('2');
+
+		$this->assertFalse($instance->objectExists('2'));
+	}
 }