From d389a79d5b10f8d53af4aba3af65bcea965b6aaa Mon Sep 17 00:00:00 2001 From: Nicolas MURE Date: Mon, 1 Jul 2019 18:28:46 +0200 Subject: [PATCH] do not create container for AzureBlobStorage Related to #618. Container creation is out of Gaufrette scope. The container should be created by the deleveloper on its own. Thus, the multi container mode has been removed, as it was creating containers on the fly. --- CHANGELOG.md | 7 +- UPGRADE.md | 11 + doc/adapters/azure-blob-storage.md | 24 +- .../Adapter/AzureBlobStorageSpec.php | 32 +-- src/Gaufrette/Adapter/AzureBlobStorage.php | 182 +----------- .../Adapter/AzureBlobStorageTest.php | 27 +- .../AzureMultiContainerBlobStorageTest.php | 266 ------------------ 7 files changed, 54 insertions(+), 495 deletions(-) delete mode 100644 tests/Gaufrette/Functional/Adapter/AzureMultiContainerBlobStorageTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 171688f1d..084870a6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,18 @@ which is the latest supported version of the SDK for OpenStack instead of https://github.com/rackspace/php-opencloud (#533). - Google Cloud Storage Adapter (#557) -## Removed +## Removed (introduces BC breaks) - The [OpenCloud adapter](https://github.com/KnpLabs/Gaufrette/blob/v0.5.0/src/Gaufrette/Adapter/OpenCloud.php) has been removed. - The [ObjectStoreFactory](https://github.com/KnpLabs/Gaufrette/blob/v0.5.0/src/Gaufrette/Adapter/OpenStackCloudFiles/ObjectStoreFactory.php) has been removed. +## Changes (introduces BC breaks) + +- Gaufrette is no longer responsible for bucket / container creation. This +should be done prior to any adapter usage (#618). + Thank you @nicolasmure and @PanzerLlama for your contributions ! v0.8.3 diff --git a/UPGRADE.md b/UPGRADE.md index 11d716416..24e8d2535 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,6 +1,17 @@ 1.0 === +**Gaufrette\Adapter\AzureblobStorage:** +As container management is out of Gaufrette scope (see #618), this adapter has +the following BC breaks : +* The `createContainer` public method has been removed. +* The `deleteContainer` public method has been removed. +* The `getCreateContainerOptions` public method has been removed. +* The `setCreateContainerOptions` public method has been removed. +* Drop support for [multi continer mode](https://github.com/KnpLabs/Gaufrette/blob/b488cf8f595c3c7a35005f72b60692e14c69398c/doc/adapters/azure-blob-storage.md#multi-container-mode). +* The constructor's `create` parameter has been removed. +* The constructor's `containerName` parameter is now mandatory (string). + **Gaufrette\Adapter\OpenStackCloudFiles\ObjectStoreFactory:** * This factory has been removed diff --git a/doc/adapters/azure-blob-storage.md b/doc/adapters/azure-blob-storage.md index 38a3df98e..8e636440a 100644 --- a/doc/adapters/azure-blob-storage.md +++ b/doc/adapters/azure-blob-storage.md @@ -20,36 +20,16 @@ You should be able to find your **endpoint**, **account name** and **account key Thanks to the blob proxy factory, the adapter lazy loads the connection to the endpoint, so it will not create any connection until it's really needed (eg. when a read or write operation is issued). -## Multi-container mode - -If you specify a container name, adapter will use only that container for all blobs. - -If you omit specifying a container, it will use a so-called multi-container mode in which container name is determined -directly from key. This allows for more flexibility if you're using dedicated storage accounts per asset type -(ie. one for images, one for videos) as you get to group assets logically, use container-level privileges, etc. +Note that your container should be created by your own needs in order to use +this adapter. ## Example ```php -write('my/stuff.txt', 'This is my stuff'); - -// multi-container mode -$adapter = new Gaufrette\Adapter\AzureBlobStorage($factory); -// make auto-created containers public by default -$containerOptions = new MicrosoftAzure\Storage\Blob\Models\CreateContainerOptions; -$containerOptions->setPublicAccess(true); -$adapter->setCreateContainerOptions($containerOptions); -$filesystem = new Gaufrette\Filesystem($adapter); -// container=my (auto-created), path=stuff.txt -$filesystem->write('my/stuff.txt', 'This is my stuff'); - ``` diff --git a/spec/Gaufrette/Adapter/AzureBlobStorageSpec.php b/spec/Gaufrette/Adapter/AzureBlobStorageSpec.php index 80e0603a7..5bb5ab5f5 100644 --- a/spec/Gaufrette/Adapter/AzureBlobStorageSpec.php +++ b/spec/Gaufrette/Adapter/AzureBlobStorageSpec.php @@ -31,6 +31,13 @@ function it_should_be_initializable() $this->shouldHaveType('Gaufrette\Adapter\MetadataSupporter'); } + function it_should_require_a_container_name(BlobProxyFactoryInterface $blobFactory) + { + $this->beConstructedWith($blobFactory); + + $this->shouldThrow(\ArgumentCountError::class)->duringInstantiation(); + } + function it_reads_file(BlobProxyFactoryInterface $blobFactory, IBlob $blob, GetBlobResult $blobContent) { $blobFactory->create()->willReturn($blob); @@ -302,29 +309,4 @@ function it_throws_storage_failure_when_it_fails_to_get_keys( $this->shouldThrow(StorageFailure::class)->duringKeys(); } - - function it_creates_container(BlobProxyFactoryInterface $blobFactory, IBlob $blob) - { - $blobFactory->create()->willReturn($blob); - - $blob->createContainer('containerName', null)->shouldBeCalled(); - - $this->createContainer('containerName'); - } - - function it_throws_storage_failure_when_it_fails_to_create_container( - BlobProxyFactoryInterface $blobFactory, - IBlob $blob, - ServiceException $azureException, - ResponseInterface $response - ) { - $blobFactory->create()->willReturn($blob); - - $blob->createContainer('containerName', null)->willThrow($azureException->getWrappedObject()); - $azureException->getResponse()->willReturn($response); - $response->getBody()->willReturn('SomeErrorCode'); - $azureException->getErrorText()->willReturn(Argument::type('string')); - - $this->shouldThrow(StorageFailure::class)->duringCreateContainer('containerName'); - } } diff --git a/src/Gaufrette/Adapter/AzureBlobStorage.php b/src/Gaufrette/Adapter/AzureBlobStorage.php index 4506c463a..bff9aefc4 100644 --- a/src/Gaufrette/Adapter/AzureBlobStorage.php +++ b/src/Gaufrette/Adapter/AzureBlobStorage.php @@ -9,7 +9,6 @@ use Gaufrette\Adapter\AzureBlobStorage\BlobProxyFactoryInterface; use MicrosoftAzure\Storage\Blob\Models\Blob; use MicrosoftAzure\Storage\Blob\Models\CreateBlockBlobOptions; -use MicrosoftAzure\Storage\Blob\Models\CreateContainerOptions; use MicrosoftAzure\Storage\Common\Exceptions\ServiceException; /** @@ -46,104 +45,18 @@ class AzureBlobStorage implements Adapter, MetadataSupporter, SizeCalculator, Ch */ protected $blobProxy; - /** - * @var bool - */ - protected $multiContainerMode = false; - - /** - * @var CreateContainerOptions - */ - protected $createContainerOptions; - /** * @param AzureBlobStorage\BlobProxyFactoryInterface $blobProxyFactory - * @param string|null $containerName - * @param bool $create + * @param string $containerName * @param bool $detectContentType * * @throws \RuntimeException */ - public function __construct(BlobProxyFactoryInterface $blobProxyFactory, $containerName = null, $create = false, $detectContentType = true) + public function __construct(BlobProxyFactoryInterface $blobProxyFactory, string $containerName, bool $detectContentType = true) { $this->blobProxyFactory = $blobProxyFactory; $this->containerName = $containerName; $this->detectContentType = $detectContentType; - - if (null === $containerName) { - $this->multiContainerMode = true; - } elseif ($create) { - $this->createContainer($containerName); - } - } - - /** - * @return CreateContainerOptions - */ - public function getCreateContainerOptions() - { - return $this->createContainerOptions; - } - - /** - * @param CreateContainerOptions $options - */ - public function setCreateContainerOptions(CreateContainerOptions $options) - { - $this->createContainerOptions = $options; - } - - /** - * Creates a new container. - * - * @param string $containerName - * @param \MicrosoftAzure\Storage\Blob\Models\CreateContainerOptions $options - * - * @throws StorageFailure if cannot create the container - */ - public function createContainer($containerName, CreateContainerOptions $options = null) - { - $this->init(); - - if (null === $options) { - $options = $this->getCreateContainerOptions(); - } - - try { - $this->blobProxy->createContainer($containerName, $options); - } catch (ServiceException $e) { - $errorCode = $this->getErrorCodeFromServiceException($e); - - // We don't care if the container was created between check and creation attempt - // it might be due to a parallel execution creating it. - if ($errorCode !== self::ERROR_CONTAINER_ALREADY_EXISTS) { - throw StorageFailure::unexpectedFailure('createContainer', [ - 'containerName' => $containerName, - 'options' => $options, - ], $e); - } - } - } - - /** - * Deletes a container. - * - * @param string $containerName - * @param DeleteContainerOptions $options - * - * @throws StorageFailure if cannot delete the container - */ - public function deleteContainer($containerName, DeleteContainerOptions $options = null) - { - $this->init(); - - try { - $this->blobProxy->deleteContainer($containerName, $options); - } catch (ServiceException $e) { - throw StorageFailure::unexpectedFailure('deleteContainer', [ - 'containerName' => $containerName, - ], $e); - } } /** @@ -187,10 +100,6 @@ public function write($key, $content) } try { - if ($this->multiContainerMode) { - $this->createContainer($containerName); - } - $this->blobProxy->createBlockBlob($containerName, $key, $content, $options); } catch (ServiceException $e) { throw StorageFailure::unexpectedFailure('write', [ @@ -231,76 +140,10 @@ public function keys() { $this->init(); - if ($this->multiContainerMode) { - return $this->keysForMultiContainerMode(); - } - - return $this->keysForSingleContainerMode(); - } - - /** - * List objects stored when the adapter is used in multi container mode. - * - * @return array - * - * @throws \RuntimeException - */ - private function keysForMultiContainerMode() - { - try { - $containersList = $this->blobProxy->listContainers()->getContainers(); - $lists = []; - - foreach ($containersList as $container) { - $lists[] = $this->fetchContainerKeysAndIgnore404($container->getName()); - } - - return !empty($lists) ? array_merge(...$lists) : []; - } catch (ServiceException $e) { - throw StorageFailure::unexpectedFailure('keys', [ - 'multiContainerMode' => $this->multiContainerMode, - 'containerName' => $this->containerName, - ], $e); - } - } - - /** - * List the keys in a container and ignore any 404 error. - * - * This prevent race conditions happening when a container is deleted after calling listContainers() and - * before the container keys are fetched. - * - * @param string $containerName - * - * @return array - */ - private function fetchContainerKeysAndIgnore404($containerName) - { - try { - return $this->fetchBlobs($containerName, $containerName); - } catch (ServiceException $e) { - if ($e->getResponse()->getStatusCode() === 404) { - return []; - } - - throw $e; - } - } - - /** - * List objects stored when the adapter is not used in multi container mode. - * - * @return array - * - * @throws \RuntimeException - */ - private function keysForSingleContainerMode() - { try { return $this->fetchBlobs($this->containerName); } catch (ServiceException $e) { throw StorageFailure::unexpectedFailure('keys', [ - 'multiContainerMode' => $this->multiContainerMode, 'containerName' => $this->containerName, ], $e); } @@ -413,10 +256,6 @@ public function rename($sourceKey, $targetKey) list($targetContainerName, $targetKey) = $this->tokenizeKey($targetKey); try { - if ($this->multiContainerMode) { - $this->createContainer($targetContainerName); - } - $this->blobProxy->copyBlob($targetContainerName, $targetKey, $sourceContainerName, $sourceKey); $this->blobProxy->deleteBlob($sourceContainerName, $sourceKey); } catch (ServiceException $e) { @@ -535,22 +374,7 @@ private function guessContentType($content) */ private function tokenizeKey($key) { - $containerName = $this->containerName; - if (false === $this->multiContainerMode) { - return [$containerName, $key]; - } - - if (false === ($index = strpos($key, '/'))) { - throw new InvalidKey(sprintf( - 'Failed to establish container name from key "%s", container name is required in multi-container mode', - $key - )); - } - - $containerName = substr($key, 0, $index); - $key = substr($key, $index + 1); - - return [$containerName, $key]; + return [$this->containerName, $key]; } /** diff --git a/tests/Gaufrette/Functional/Adapter/AzureBlobStorageTest.php b/tests/Gaufrette/Functional/Adapter/AzureBlobStorageTest.php index e53e669e1..d00b7b3d9 100644 --- a/tests/Gaufrette/Functional/Adapter/AzureBlobStorageTest.php +++ b/tests/Gaufrette/Functional/Adapter/AzureBlobStorageTest.php @@ -18,6 +18,9 @@ class AzureBlobStorageTest extends FunctionalTestCase /** @var AzureBlobStorage */ private $adapter; + /** @var \MicrosoftAzure\Storage\Blob\Internal\IBlob */ + private $blobProxy; + public function setUp() { $account = getenv('AZURE_ACCOUNT'); @@ -30,8 +33,13 @@ public function setUp() $connection = sprintf('BlobEndpoint=http://%1$s.blob.core.windows.net/;AccountName=%1$s;AccountKey=%2$s', $account, $key); + $blobProxyFactory = new BlobProxyFactory($connection); + $this->blobProxy = $blobProxyFactory->create(); + $this->container = uniqid($containerName); - $this->adapter = new AzureBlobStorage(new BlobProxyFactory($connection), $this->container, true); + $this->blobProxy->createContainer($this->container); + + $this->adapter = new AzureBlobStorage($blobProxyFactory, $this->container); $this->filesystem = new Filesystem($this->adapter); } @@ -41,6 +49,21 @@ public function tearDown() return; } - $this->adapter->deleteContainer($this->container); + $this->blobProxy->deleteContainer($this->container); + } + + /** + * @test + * @group functional + * @expectedException \Gaufrette\Exception\StorageFailure + */ + public function shouldThrowWhenUsingAnUnexistingContainer() + { + $this->blobProxy->deleteContainer($this->container); + + $this->filesystem->write('foo', 'Some content'); + + // will not delete the container again, see self::tearDown function + $this->adapter = null; } } diff --git a/tests/Gaufrette/Functional/Adapter/AzureMultiContainerBlobStorageTest.php b/tests/Gaufrette/Functional/Adapter/AzureMultiContainerBlobStorageTest.php deleted file mode 100644 index 328996594..000000000 --- a/tests/Gaufrette/Functional/Adapter/AzureMultiContainerBlobStorageTest.php +++ /dev/null @@ -1,266 +0,0 @@ -markTestSkipped('Either AZURE_ACCOUNT and/or AZURE_KEY env variables are not defined.'); - } - - $connection = sprintf('BlobEndpoint=http://%1$s.blob.core.windows.net/;AccountName=%1$s;AccountKey=%2$s', $account, $key); - - $this->adapter = new AzureBlobStorage(new BlobProxyFactory($connection)); - $this->filesystem = new Filesystem($this->adapter); - } - - /** - * @test - * @group functional - */ - public function shouldWriteAndRead() - { - $path1 = $this->createUniqueContainerName('container') . '/foo'; - $path2 = $this->createUniqueContainerName('test') . '/subdir/foo'; - - $this->filesystem->write($path1, 'Some content'); - $this->filesystem->write($path2, 'Some content1', true); - - $this->assertEquals('Some content', $this->filesystem->read($path1)); - $this->assertEquals('Some content1', $this->filesystem->read($path2)); - } - - /** - * @test - * @group functional - */ - public function shouldUpdateFileContent() - { - $path = $this->createUniqueContainerName('container') . '/foo'; - - $this->filesystem->write($path, 'Some content'); - $this->filesystem->write($path, 'Some content updated', true); - - $this->assertEquals('Some content updated', $this->filesystem->read($path)); - } - - /** - * @test - * @group functional - */ - public function shouldCheckIfFileExists() - { - $path1 = $this->createUniqueContainerName('container') . '/foo'; - $path2 = $this->createUniqueContainerName('test') . '/somefile'; - - $this->assertFalse($this->filesystem->has($path1)); - - $this->filesystem->write($path1, 'Some content'); - - $this->assertTrue($this->filesystem->has($path1)); - $this->assertFalse($this->filesystem->has($path2)); - } - - /** - * @test - * @group functional - */ - public function shouldGetMtime() - { - $path = $this->createUniqueContainerName('container') . '/foo'; - - $this->filesystem->write($path, 'Some content'); - - $this->assertGreaterThan(0, $this->filesystem->mtime($path)); - } - - /** - * @test - * @group functional - */ - public function shouldGetSize() - { - $path = $this->createUniqueContainerName('container') . '/foo'; - - $this->filesystem->write($path, 'Some content'); - - $this->assertEquals(12, $this->filesystem->size($path)); - } - - /** - * @test - * @group functional - */ - public function shouldGetMd5Hash() - { - $path = $this->createUniqueContainerName('container') . '/foo'; - - $content = 'Some content'; - $this->filesystem->write($path, $content); - - $this->assertEquals(\md5($content), $this->filesystem->checksum($path)); - } - - /** - * @test - * @group functional - * @expectedException \RuntimeException - * @expectedMessage Could not get mtime for the "foo" key - */ - public function shouldFailWhenTryMtimeForKeyWhichDoesNotExist() - { - $this->assertFalse($this->filesystem->mtime('container5/foo')); - } - - /** - * @test - * @group functional - */ - public function shouldRenameFile() - { - $somedir = $this->createUniqueContainerName('somedir'); - $path1 = $this->createUniqueContainerName('container') . '/foo'; - $path2 = $this->createUniqueContainerName('container-new') . '/boo'; - $path3 = $somedir . '/sub/boo'; - - $this->filesystem->write($path1, 'Some content'); - $this->filesystem->rename($path1, $path2); - - $this->assertFalse($this->filesystem->has($path1)); - $this->assertEquals('Some content', $this->filesystem->read($path2)); - - $this->filesystem->write($path1, 'Some content'); - $this->filesystem->rename($path1, $path3); - - $this->assertFalse($this->filesystem->has($somedir . '/sub/foo')); - $this->assertEquals('Some content', $this->filesystem->read($path3)); - } - - /** - * @test - * @group functional - */ - public function shouldDeleteFile() - { - $path = $this->createUniqueContainerName('container') . '/foo'; - - $this->filesystem->write($path, 'Some content'); - - $this->assertTrue($this->filesystem->has($path)); - - $this->filesystem->delete($path); - - $this->assertFalse($this->filesystem->has($path)); - } - - /** - * @test - * @group functional - */ - public function shouldFetchKeys() - { - $path1 = $this->createUniqueContainerName('container-1') . '/foo'; - $path2 = $this->createUniqueContainerName('container-2') . '/bar'; - $path3 = $this->createUniqueContainerName('container-3') . '/baz'; - - $this->filesystem->write($path1, 'Some content'); - $this->filesystem->write($path2, 'Some content'); - $this->filesystem->write($path3, 'Some content'); - - $actualKeys = $this->filesystem->keys(); - foreach ([$path1, $path2, $path3] as $key) { - $this->assertContains($key, $actualKeys); - } - } - - /** - * @test - * @group functional - */ - public function shouldWorkWithHiddenFiles() - { - $path = $this->createUniqueContainerName('container') . '/.foo'; - - $this->filesystem->write($path, 'hidden'); - $this->assertTrue($this->filesystem->has($path)); - $this->assertContains($path, $this->filesystem->keys()); - $this->filesystem->delete($path); - $this->assertFalse($this->filesystem->has($path)); - } - - /** - * @test - * @group functional - */ - public function shouldKeepFileObjectInRegister() - { - $path = $this->createUniqueContainerName('container') . '/somefile'; - - $FileObjectA = $this->filesystem->createFile($path); - $FileObjectB = $this->filesystem->createFile($path); - - $this->assertSame($FileObjectB, $FileObjectA); - } - - /** - * @test - * @group functional - */ - public function shouldWriteToSameFile() - { - $path = $this->createUniqueContainerName('container') . '/somefile'; - - $FileObjectA = $this->filesystem->createFile($path); - $FileObjectA->setContent('ABC'); - - $FileObjectB = $this->filesystem->createFile($path); - $FileObjectB->setContent('DEF'); - - $this->assertEquals('DEF', $FileObjectA->getContent()); - } - - private function createUniqueContainerName($prefix) - { - $this->containers[] = $container = uniqid($prefix); - - return $container; - } - - public function tearDown() - { - foreach ($this->containers as $container) { - try { - $this->adapter->deleteContainer($container); - } catch (StorageFailure $e) { - if (null !== ($previous = $e->getPrevious()) - && $previous instanceof ServiceException - && $previous->getResponse()->getStatusCode() === 404 - ) { - continue; - } - - throw $e; - } - } - } -}