From 6c9f2644cf8bac151b9af743802eb72a51278e8b Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 10 Dec 2018 17:20:45 +0100 Subject: Add objectExists to objectstore interface Signed-off-by: Robin Appelman --- lib/private/Files/ObjectStore/Azure.php | 13 +++++++++++++ lib/private/Files/ObjectStore/S3ObjectTrait.php | 4 ++++ lib/private/Files/ObjectStore/StorageObjectStore.php | 3 +++ lib/private/Files/ObjectStore/Swift.php | 3 +++ lib/public/Files/ObjectStore/IObjectStore.php | 9 +++++++++ tests/lib/Files/ObjectStore/ObjectStoreTest.php | 15 +++++++++++++++ 6 files changed, 47 insertions(+) diff --git a/lib/private/Files/ObjectStore/Azure.php b/lib/private/Files/ObjectStore/Azure.php index 899c826ec19..e162c510b98 100644 --- a/lib/private/Files/ObjectStore/Azure.php +++ b/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; + } + } + } } diff --git a/lib/private/Files/ObjectStore/S3ObjectTrait.php b/lib/private/Files/ObjectStore/S3ObjectTrait.php index 280a8efa81c..a1110d87c8f 100644 --- a/lib/private/Files/ObjectStore/S3ObjectTrait.php +++ b/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); + } } diff --git a/lib/private/Files/ObjectStore/StorageObjectStore.php b/lib/private/Files/ObjectStore/StorageObjectStore.php index 0d35ba2ed7a..f9fc1b5a4aa 100644 --- a/lib/private/Files/ObjectStore/StorageObjectStore.php +++ b/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); + } } diff --git a/lib/private/Files/ObjectStore/Swift.php b/lib/private/Files/ObjectStore/Swift.php index e379e54d018..3d6bf9d69da 100644 --- a/lib/private/Files/ObjectStore/Swift.php +++ b/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); + } } diff --git a/lib/public/Files/ObjectStore/IObjectStore.php b/lib/public/Files/ObjectStore/IObjectStore.php index 628fd5852da..83c4b1065d6 100644 --- a/lib/public/Files/ObjectStore/IObjectStore.php +++ b/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); } diff --git a/tests/lib/Files/ObjectStore/ObjectStoreTest.php b/tests/lib/Files/ObjectStore/ObjectStoreTest.php index 2116306053e..1383c0149a2 100644 --- a/tests/lib/Files/ObjectStore/ObjectStoreTest.php +++ b/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')); + } } -- cgit v1.2.3 From d6bf5d43841f8d80ed54137db75c8ca6d2396cf4 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 12 Dec 2018 15:24:40 +0100 Subject: upload new files in objectstore to a .part path first This prevent the object store and cache from getting out of sync when an objectstore silently fails or the php process get's killed during the upload without giving us the chance to cleanup Signed-off-by: Robin Appelman --- .../Files/ObjectStore/ObjectStoreStorage.php | 22 +++++++-- .../lib/Files/ObjectStore/FailWriteObjectStore.php | 53 ++++++++++++++++++++++ .../ObjectStore/ObjectStoreStorageOverwrite.php | 38 ++++++++++++++++ .../Files/ObjectStore/ObjectStoreStorageTest.php | 17 ++++++- 4 files changed, 124 insertions(+), 6 deletions(-) create mode 100644 tests/lib/Files/ObjectStore/FailWriteObjectStore.php create mode 100644 tests/lib/Files/ObjectStore/ObjectStoreStorageOverwrite.php diff --git a/lib/private/Files/ObjectStore/ObjectStoreStorage.php b/lib/private/Files/ObjectStore/ObjectStoreStorage.php index 26db551a384..7ee1c8e2055 100644 --- a/lib/private/Files/ObjectStore/ObjectStoreStorage.php +++ b/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; } } diff --git a/tests/lib/Files/ObjectStore/FailWriteObjectStore.php b/tests/lib/Files/ObjectStore/FailWriteObjectStore.php new file mode 100644 index 00000000000..4310d8ba27c --- /dev/null +++ b/tests/lib/Files/ObjectStore/FailWriteObjectStore.php @@ -0,0 +1,53 @@ + + * + * @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 . + * + */ + +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); + } +} diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStorageOverwrite.php b/tests/lib/Files/ObjectStore/ObjectStoreStorageOverwrite.php new file mode 100644 index 00000000000..5e8faed3347 --- /dev/null +++ b/tests/lib/Files/ObjectStore/ObjectStoreStorageOverwrite.php @@ -0,0 +1,38 @@ + + * + * @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 . + * + */ + +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; + } +} diff --git a/tests/lib/Files/ObjectStore/ObjectStoreStorageTest.php b/tests/lib/Files/ObjectStore/ObjectStoreStorageTest.php index c9d6c1bd922..3b3827f460a 100644 --- a/tests/lib/Files/ObjectStore/ObjectStoreStorageTest.php +++ b/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')); + } } -- cgit v1.2.3