From dbd5b5bb6e329ceb216fc2c13540e9f482129ccd Mon Sep 17 00:00:00 2001 From: nupplaPhil Date: Mon, 6 Jan 2020 17:42:28 +0100 Subject: [PATCH] - Fixing SystemResource - Adding tests for StorageManager - Updating doc --- doc/AddonStorageBackend.md | 9 +- src/Core/StorageManager.php | 70 ++++-- src/Model/Photo.php | 5 +- src/Model/Storage/AbstractStorage.php | 32 +++ src/Model/Storage/Database.php | 33 ++- src/Model/Storage/Filesystem.php | 12 +- src/Model/Storage/IStorage.php | 7 + src/Model/Storage/SystemResource.php | 42 +++- tests/Util/SampleStorageBackend.php | 94 ++++++++ tests/datasets/storage/database.fixture.php | 40 ++++ tests/src/Core/StorageManagerTest.php | 252 ++++++++++++++++++++ update.php | 10 + 12 files changed, 547 insertions(+), 59 deletions(-) create mode 100644 src/Model/Storage/AbstractStorage.php create mode 100644 tests/Util/SampleStorageBackend.php create mode 100644 tests/datasets/storage/database.fixture.php create mode 100644 tests/src/Core/StorageManagerTest.php diff --git a/doc/AddonStorageBackend.md b/doc/AddonStorageBackend.md index 896ea49bf7..115bfd0035 100644 --- a/doc/AddonStorageBackend.md +++ b/doc/AddonStorageBackend.md @@ -215,6 +215,11 @@ class SampleStorageBackend implements IStorage { return self::NAME; } + + public static function getName() + { + return self::NAME; + } } ``` @@ -239,13 +244,13 @@ function samplestorage_install() // on addon install, we register our class with name "Sample Storage". // note: we use `::class` property, which returns full class name as string // this save us the problem of correctly escape backslashes in class name - DI::facStorage()->register("Sample Storage", SampleStorageBackend::class); + DI::facStorage()->register(SampleStorageBackend::class); } function samplestorage_unistall() { // when the plugin is uninstalled, we unregister the backend. - DI::facStorage()->unregister("Sample Storage"); + DI::facStorage()->unregister(SampleStorageBackend::class); } ``` diff --git a/src/Core/StorageManager.php b/src/Core/StorageManager.php index ede8966f41..6a67a06ed7 100644 --- a/src/Core/StorageManager.php +++ b/src/Core/StorageManager.php @@ -45,6 +45,7 @@ class StorageManager * @param Database $dba * @param IConfiguration $config * @param LoggerInterface $logger + * @param Dice $dice */ public function __construct(Database $dba, IConfiguration $config, LoggerInterface $logger, Dice $dice) { @@ -76,27 +77,39 @@ class StorageManager /** * @brief Return storage backend class by registered name * - * @param string $name Backend name + * @param string|null $name Backend name * * @return Storage\IStorage|null null if no backend registered at $name */ - public function getByName(string $name) + public function getByName(string $name = null) { - if (!$this->isValidBackend($name)) { + if (!$this->isValidBackend($name) && + $name !== Storage\SystemResource::getName()) { return null; } - return $this->dice->create($this->backends[$name]); + /** @var Storage\IStorage $storage */ + $storage = null; + + // If the storage of the file is a system resource, + // create it directly since it isn't listed in the registered backends + if ($name === Storage\SystemResource::getName()) { + $storage = $this->dice->create(Storage\SystemResource::class); + } else { + $storage = $this->dice->create($this->backends[$name]); + } + + return $storage; } /** * Checks, if the storage is a valid backend * - * @param string $name The name or class of the backend + * @param string|null $name The name or class of the backend * * @return boolean True, if the backend is a valid backend */ - public function isValidBackend(string $name) + public function isValidBackend(string $name = null) { return array_key_exists($name, $this->backends); } @@ -108,7 +121,7 @@ class StorageManager * * @return boolean True, if the set was successful */ - public function setBackend(string $name) + public function setBackend(string $name = null) { if (!$this->isValidBackend($name)) { return false; @@ -135,23 +148,24 @@ class StorageManager /** * @brief Register a storage backend class * - * @param string $name User readable backend name * @param string $class Backend class name * * @return boolean True, if the registration was successful */ - public function register(string $name, string $class) + public function register(string $class) { - if (!is_subclass_of($class, Storage\IStorage::class)) { - return false; - } + if (is_subclass_of($class, Storage\IStorage::class)) { + /** @var Storage\IStorage $class */ - $backends = $this->backends; - $backends[$name] = $class; + $backends = $this->backends; + $backends[$class::getName()] = $class; - if ($this->config->set('storage', 'backends', $this->backends)) { - $this->backends = $backends; - return true; + if ($this->config->set('storage', 'backends', $backends)) { + $this->backends = $backends; + return true; + } else { + return false; + } } else { return false; } @@ -160,14 +174,26 @@ class StorageManager /** * @brief Unregister a storage backend class * - * @param string $name User readable backend name + * @param string $class Backend class name * * @return boolean True, if unregistering was successful */ - public function unregister(string $name) + public function unregister(string $class) { - unset($this->backends[$name]); - return $this->config->set('storage', 'backends', $this->backends); + if (is_subclass_of($class, Storage\IStorage::class)) { + /** @var Storage\IStorage $class */ + + unset($this->backends[$class::getName()]); + + if ($this->currentBackend instanceof $class) { + $this->config->set('storage', 'name', null); + $this->currentBackend = null; + } + + return $this->config->set('storage', 'backends', $this->backends); + } else { + return false; + } } /** @@ -196,7 +222,7 @@ class StorageManager $resources = $this->dba->select( $table, ['id', 'data', 'backend-class', 'backend-ref'], - ['`backend-class` IS NULL or `backend-class` != ?', $destination], + ['`backend-class` IS NULL or `backend-class` != ?', $destination::getName()], ['limit' => $limit] ); diff --git a/src/Model/Photo.php b/src/Model/Photo.php index 7aa6ffdb3d..f8f50824bd 100644 --- a/src/Model/Photo.php +++ b/src/Model/Photo.php @@ -16,6 +16,7 @@ use Friendica\Database\DBA; use Friendica\Database\DBStructure; use Friendica\DI; use Friendica\Model\Storage\IStorage; +use Friendica\Model\Storage\SystemResource; use Friendica\Object\Image; use Friendica\Util\DateTimeFormat; use Friendica\Util\Images; @@ -223,7 +224,7 @@ class Photo $values = array_fill(0, count($fields), ""); $photo = array_combine($fields, $values); - $photo["backend-class"] = Storage\SystemResource::class; + $photo["backend-class"] = SystemResource::NAME; $photo["backend-ref"] = $filename; $photo["type"] = $mimetype; $photo["cacheable"] = false; @@ -275,7 +276,7 @@ class Photo if (DBA::isResult($existing_photo)) { $backend_ref = (string)$existing_photo["backend-ref"]; - $storage = DI::facStorage()->getByName((string)$existing_photo["backend-class"]); + $storage = DI::facStorage()->getByName($existing_photo["backend-class"] ?? ''); } else { $storage = DI::storage(); } diff --git a/src/Model/Storage/AbstractStorage.php b/src/Model/Storage/AbstractStorage.php new file mode 100644 index 0000000000..270d675623 --- /dev/null +++ b/src/Model/Storage/AbstractStorage.php @@ -0,0 +1,32 @@ +l10n = $l10n; + $this->logger = $logger; + } + + public function __toString() + { + return static::getName(); + } +} diff --git a/src/Model/Storage/Database.php b/src/Model/Storage/Database.php index 84dd116b52..182add8614 100644 --- a/src/Model/Storage/Database.php +++ b/src/Model/Storage/Database.php @@ -6,35 +6,32 @@ namespace Friendica\Model\Storage; -use Friendica\Core\L10n; +use Friendica\Core\L10n\L10n; use Psr\Log\LoggerInterface; +use Friendica\Database\Database as DBA; /** * @brief Database based storage system * * This class manage data stored in database table. */ -class Database implements IStorage +class Database extends AbstractStorage { const NAME = 'Database'; - /** @var \Friendica\Database\Database */ + /** @var DBA */ private $dba; - /** @var LoggerInterface */ - private $logger; - /** @var L10n\L10n */ - private $l10n; /** - * @param \Friendica\Database\Database $dba - * @param LoggerInterface $logger - * @param L10n\L10n $l10n + * @param DBA $dba + * @param LoggerInterface $logger + * @param L10n $l10n */ - public function __construct(\Friendica\Database\Database $dba, LoggerInterface $logger, L10n\L10n $l10n) + public function __construct(DBA $dba, LoggerInterface $logger, L10n $l10n) { - $this->dba = $dba; - $this->logger = $logger; - $this->l10n = $l10n; + parent::__construct($l10n, $logger); + + $this->dba = $dba; } /** @@ -58,15 +55,15 @@ class Database implements IStorage if ($reference !== '') { $result = $this->dba->update('storage', ['data' => $data], ['id' => $reference]); if ($result === false) { - $this->logger->warning('Failed to update data.', ['id' => $reference, 'errorCode' => $this->dba->errorNo(), 'errorMessage' => $this->dba->errorMessage()]); + $this->logger->warning('Failed to update data.', ['id' => $reference, 'errorCode' => $this->dba->errorNo(), 'errorMessage' => $this->dba->errorMessage()]); throw new StorageException($this->l10n->t('Database storage failed to update %s', $reference)); - } + } return $reference; } else { $result = $this->dba->insert('storage', ['data' => $data]); if ($result === false) { - $this->logger->warning('Failed to insert data.', ['errorCode' => $this->dba->errorNo(), 'errorMessage' => $this->dba->errorMessage()]); + $this->logger->warning('Failed to insert data.', ['errorCode' => $this->dba->errorNo(), 'errorMessage' => $this->dba->errorMessage()]); throw new StorageException($this->l10n->t('Database storage failed to insert data')); } @@ -101,7 +98,7 @@ class Database implements IStorage /** * @inheritDoc */ - public function __toString() + public static function getName() { return self::NAME; } diff --git a/src/Model/Storage/Filesystem.php b/src/Model/Storage/Filesystem.php index e954352bc6..bfc474a5aa 100644 --- a/src/Model/Storage/Filesystem.php +++ b/src/Model/Storage/Filesystem.php @@ -21,7 +21,7 @@ use Psr\Log\LoggerInterface; * Each new resource gets a value as reference and is saved in a * folder tree stucture created from that value. */ -class Filesystem implements IStorage +class Filesystem extends AbstractStorage { const NAME = 'Filesystem'; @@ -30,10 +30,6 @@ class Filesystem implements IStorage /** @var IConfiguration */ private $config; - /** @var LoggerInterface */ - private $logger; - /** @var L10n */ - private $l10n; /** @var string */ private $basePath; @@ -47,9 +43,9 @@ class Filesystem implements IStorage */ public function __construct(IConfiguration $config, LoggerInterface $logger, L10n $l10n) { + parent::__construct($l10n, $logger); + $this->config = $config; - $this->logger = $logger; - $this->l10n = $l10n; $path = $this->config->get('storage', 'filesystem_path', self::DEFAULT_BASE_FOLDER); $this->basePath = rtrim($path, '/'); @@ -185,7 +181,7 @@ class Filesystem implements IStorage /** * @inheritDoc */ - public function __toString() + public static function getName() { return self::NAME; } diff --git a/src/Model/Storage/IStorage.php b/src/Model/Storage/IStorage.php index a7b7f6f566..c3ec3197c9 100644 --- a/src/Model/Storage/IStorage.php +++ b/src/Model/Storage/IStorage.php @@ -96,4 +96,11 @@ interface IStorage * @return string */ public function __toString(); + + /** + * The name of the backend + * + * @return string + */ + public static function getName(); } diff --git a/src/Model/Storage/SystemResource.php b/src/Model/Storage/SystemResource.php index 3afe8ee6f5..43514c1dcd 100644 --- a/src/Model/Storage/SystemResource.php +++ b/src/Model/Storage/SystemResource.php @@ -16,10 +16,15 @@ use \BadMethodCallException; */ class SystemResource implements IStorage { + const NAME = 'SystemResource'; + // Valid folders to look for resources const VALID_FOLDERS = ["images"]; - public static function get($filename) + /** + * @inheritDoc + */ + public function get(string $filename) { $folder = dirname($filename); if (!in_array($folder, self::VALID_FOLDERS)) { @@ -31,25 +36,48 @@ class SystemResource implements IStorage return file_get_contents($filename); } - - public static function put($data, $filename = "") + /** + * @inheritDoc + */ + public function put(string $data, string $filename = '') { throw new BadMethodCallException(); } - public static function delete($filename) + public function delete(string $filename) { throw new BadMethodCallException(); } - public static function getOptions() + /** + * @inheritDoc + */ + public function getOptions() { return []; } - public static function saveOptions($data) + /** + * @inheritDoc + */ + public function saveOptions(array $data) { return []; } + + /** + * @inheritDoc + */ + public function __toString() + { + return self::NAME; + } + + /** + * @inheritDoc + */ + public static function getName() + { + return self::NAME; + } } - diff --git a/tests/Util/SampleStorageBackend.php b/tests/Util/SampleStorageBackend.php new file mode 100644 index 0000000000..3f4eb90ada --- /dev/null +++ b/tests/Util/SampleStorageBackend.php @@ -0,0 +1,94 @@ + [ + 'input', // will use a simple text input + 'The file to return', // the label + 'sample', // the current value + 'Enter the path to a file', // the help text + // no extra data for 'input' type.. + ], + ]; + /** @var array Just save the data in memory */ + private $data = []; + + /** + * SampleStorageBackend constructor. + * + * @param L10n $l10n The configuration of Friendica + * + * You can add here every dynamic class as dependency you like and add them to a private field + * Friendica automatically creates these classes and passes them as argument to the constructor + */ + public function __construct(L10n $l10n) + { + $this->l10n = $l10n; + } + + public function get(string $reference) + { + // we return always the same image data. Which file we load is defined by + // a config key + return $this->data[$reference] ?? null; + } + + public function put(string $data, string $reference = '') + { + if ($reference === '') { + $reference = 'sample'; + } + + $this->data[$reference] = $data; + + return $reference; + } + + public function delete(string $reference) + { + if (isset($this->data[$reference])) { + unset($this->data[$reference]); + } + + return true; + } + + public function getOptions() + { + return $this->options; + } + + public function saveOptions(array $data) + { + $this->options = $data; + + // no errors, return empty array + return $this->options; + } + + public function __toString() + { + return self::NAME; + } + + public static function getName() + { + return self::NAME; + } +} diff --git a/tests/datasets/storage/database.fixture.php b/tests/datasets/storage/database.fixture.php new file mode 100644 index 0000000000..780bfa1402 --- /dev/null +++ b/tests/datasets/storage/database.fixture.php @@ -0,0 +1,40 @@ + [ + // move from data-attribute to storage backend + [ + 'id' => 1, + 'backend-class' => null, + 'backend-ref' => 'f0c0d0i2', + 'data' => 'without class', + ], + // move from storage-backend to maybe filesystem backend, skip at database backend + [ + 'id' => 2, + 'backend-class' => 'Database', + 'backend-ref' => 1, + 'data' => '', + ], + // move data if invalid storage + [ + 'id' => 3, + 'backend-class' => 'invalid!', + 'backend-ref' => 'unimported', + 'data' => 'invalid data moved', + ], + // skip everytime because of invalid storage and no data + [ + 'id' => 3, + 'backend-class' => 'invalid!', + 'backend-ref' => 'unimported', + 'data' => '', + ], + ], + 'storage' => [ + [ + 'id' => 1, + 'data' => 'inside database', + ], + ], +]; diff --git a/tests/src/Core/StorageManagerTest.php b/tests/src/Core/StorageManagerTest.php new file mode 100644 index 0000000000..82af38b4aa --- /dev/null +++ b/tests/src/Core/StorageManagerTest.php @@ -0,0 +1,252 @@ +setUpVfsDir(); + + $this->logger = new NullLogger(); + $this->dice = (new Dice()) + ->addRules(include __DIR__ . '/../../../static/dependencies.config.php') + ->addRule(Database::class, ['instanceOf' => StaticDatabase::class, 'shared' => true]) + ->addRule(ISession::class, ['instanceOf' => Session\Memory::class, 'shared' => true, 'call' => null]); + + $profiler = \Mockery::mock(Profiler::class); + $profiler->shouldReceive('saveTimestamp')->withAnyArgs()->andReturn(true); + + // load real config to avoid mocking every config-entry which is related to the Database class + $configFactory = new ConfigFactory(); + $loader = new ConfigFileLoader($this->root->url()); + $configCache = $configFactory->createCache($loader); + + $this->dba = new StaticDatabase($configCache, $profiler, $this->logger); + + $configModel = new Config($this->dba); + $this->config = new PreloadConfiguration($configCache, $configModel); + } + + /** + * Test plain instancing first + */ + public function testInstance() + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + $this->assertInstanceOf(StorageManager::class, $storageManager); + } + + public function dataStorages() + { + return [ + 'empty' => [ + 'name' => '', + 'assert' => null, + 'assertName' => '', + 'userBackend' => false, + ], + 'database' => [ + 'name' => Storage\Database::NAME, + 'assert' => Storage\Database::class, + 'assertName' => Storage\Database::NAME, + 'userBackend' => true, + ], + 'filesystem' => [ + 'name' => Storage\Filesystem::NAME, + 'assert' => Storage\Filesystem::class, + 'assertName' => Storage\Filesystem::NAME, + 'userBackend' => true, + ], + 'systemresource' => [ + 'name' => Storage\SystemResource::NAME, + 'assert' => Storage\SystemResource::class, + 'assertName' => Storage\SystemResource::NAME, + // false here, because SystemResource isn't meant to be a user backend, + // it's for system resources only + 'userBackend' => false, + ], + 'invalid' => [ + 'name' => 'invalid', + 'assert' => null, + 'assertName' => '', + 'userBackend' => false, + ], + ]; + } + + /** + * Test the getByName() method + * + * @dataProvider dataStorages + */ + public function testGetByName($name, $assert, $assertName) + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + $storage = $storageManager->getByName($name); + + if (!empty($assert)) { + $this->assertInstanceOf(Storage\IStorage::class, $storage); + $this->assertInstanceOf($assert, $storage); + $this->assertEquals($name, $storage::getName()); + } else { + $this->assertNull($storage); + } + $this->assertEquals($assertName, $storage); + } + + /** + * Test the isValidBackend() method + * + * @dataProvider dataStorages + */ + public function testIsValidBackend($name, $assert, $assertName, $userBackend) + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + $this->assertEquals($userBackend, $storageManager->isValidBackend($name)); + } + + /** + * Test the method listBackends() with default setting + */ + public function testListBackends() + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + $this->assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends()); + } + + /** + * Test the method getBackend() + * + * @dataProvider dataStorages + */ + public function testGetBackend($name, $assert, $assertName, $userBackend) + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + $this->assertNull($storageManager->getBackend()); + + if ($userBackend) { + $storageManager->setBackend($name); + + $this->assertInstanceOf($assert, $storageManager->getBackend()); + } + } + + /** + * Test the method getBackend() with a pre-configured backend + * + * @dataProvider dataStorages + */ + public function testPresetBackend($name, $assert, $assertName, $userBackend) + { + $this->config->set('storage', 'name', $name); + + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + if ($userBackend) { + $this->assertInstanceOf($assert, $storageManager->getBackend()); + } else { + $this->assertNull($storageManager->getBackend()); + } + } + + /** + * Tests the register and unregister methods for a new backend storage class + * + * Uses a sample storage for testing + * + * @see SampleStorageBackend + */ + public function testRegisterUnregisterBackends() + { + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + + $this->assertTrue($storageManager->register(SampleStorageBackend::class)); + + $this->assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [ + 'Sample Storage' => SampleStorageBackend::class, + ]), $storageManager->listBackends()); + $this->assertEquals(array_merge(StorageManager::DEFAULT_BACKENDS, [ + 'Sample Storage' => SampleStorageBackend::class, + ]), $this->config->get('storage', 'backends')); + + $this->assertTrue($storageManager->setBackend(SampleStorageBackend::NAME)); + $this->assertEquals(SampleStorageBackend::NAME, $this->config->get('storage', 'name')); + + $this->assertInstanceOf(SampleStorageBackend::class, $storageManager->getBackend()); + + $this->assertTrue($storageManager->unregister(SampleStorageBackend::class)); + $this->assertEquals(StorageManager::DEFAULT_BACKENDS, $this->config->get('storage', 'backends')); + $this->assertEquals(StorageManager::DEFAULT_BACKENDS, $storageManager->listBackends()); + + $this->assertNull($storageManager->getBackend()); + $this->assertNull($this->config->get('storage', 'name')); + } + + /** + * Test moving data to a new storage (currently testing db & filesystem) + * + * @dataProvider dataStorages + */ + public function testMoveStorage($name, $assert, $assertName, $userBackend) + { + if (!$userBackend) { + return; + } + + $this->loadFixture(__DIR__ . '/../../datasets/storage/database.fixture.php', $this->dba); + + $storageManager = new StorageManager($this->dba, $this->config, $this->logger, $this->dice); + $storage = $storageManager->getByName($name); + $storageManager->move($storage); + + $photos = $this->dba->select('photo', ['backend-ref', 'backend-class', 'id', 'data']); + + while ($photo = $this->dba->fetch($photos)) { + + $this->assertEmpty($photo['data']); + + $storage = $storageManager->getByName($photo['backend-class']); + $data = $storage->get($photo['backend-ref']); + + $this->assertNotEmpty($data); + } + } +} diff --git a/update.php b/update.php index 90018b62d6..145446bae0 100644 --- a/update.php +++ b/update.php @@ -418,5 +418,15 @@ function update_1329() Config::delete('storage', 'class'); } + $photos = DBA::select('photos', ['backend-class', 'id'], ['backend-class IS NOT NULL']); + foreach ($photos as $photo) { + DBA::update('photos', ['backend-class' => $photo['backend-class']::NAME], ['id' => $photo['id']]); + } + + $attachs = DBA::select('attach', ['backend-class', 'id'], ['backend-class IS NOT NULL']); + foreach ($attachs as $attach) { + DBA::update('photos', ['backend-class' => $attach['backend-class']::NAME], ['id' => $attach['id']]); + } + return Update::SUCCESS; }