From fd882abd8003c39787b640b8f2accc887431400c Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 11 Jan 2023 21:10:59 +0100 Subject: [PATCH 1/3] Revert Cache delete() behavior to stable version --- src/Core/Config/ValueObject/Cache.php | 73 ++++++++++------------- tests/src/Core/Config/Cache/CacheTest.php | 34 +++++++++-- 2 files changed, 58 insertions(+), 49 deletions(-) diff --git a/src/Core/Config/ValueObject/Cache.php b/src/Core/Config/ValueObject/Cache.php index 61a65c3e70..db972431ac 100644 --- a/src/Core/Config/ValueObject/Cache.php +++ b/src/Core/Config/ValueObject/Cache.php @@ -87,10 +87,11 @@ class Cache $keys = array_keys($config[$category]); foreach ($keys as $key) { - $this->set($category, $key, $config[$category][$key] ?? null, $source); + $value = $config[$category][$key]; + if (isset($value)) { + $this->set($category, $key, $value, $source); + } } - } else { - $this->set($category, null, $config[$category], $source); } } } @@ -149,8 +150,6 @@ class Cache $data[$category][$key] = $this->config[$category][$key]; } } - } elseif (is_int($this->source[$category])) { - $data[$category] = null; } } @@ -160,49 +159,40 @@ class Cache /** * Sets a value in the config cache. Accepts raw output from the config table * - * @param string $cat Config category - * @param ?string $key Config key - * @param ?mixed $value Value to set - * @param int $source The source of the current config key + * @param string $cat Config category + * @param string $key Config key + * @param mixed $value Value to set + * @param int $source The source of the current config key * * @return bool True, if the value is set */ - public function set(string $cat, string $key = null, $value = null, int $source = self::SOURCE_DEFAULT): bool + public function set(string $cat, string $key, $value, int $source = self::SOURCE_DEFAULT): bool { - if (!isset($this->config[$cat]) && $key !== null) { + if (!isset($this->config[$cat])) { $this->config[$cat] = []; $this->source[$cat] = []; } - if ((isset($this->source[$cat][$key]) && $source < $this->source[$cat][$key]) || - (is_int($this->source[$cat] ?? null) && $source < $this->source[$cat])) { + if (isset($this->source[$cat][$key]) && + $source < $this->source[$cat][$key]) { return false; } if ($this->hidePasswordOutput && $key == 'password' && is_string($value)) { - $this->setCatKeyValueSource($cat, $key, new HiddenString($value), $source); + $this->config[$cat][$key] = new HiddenString((string)$value); } else if (is_string($value)) { - $this->setCatKeyValueSource($cat, $key, self::toConfigValue($value), $source); + $this->config[$cat][$key] = self::toConfigValue($value); } else { - $this->setCatKeyValueSource($cat, $key, $value, $source); + $this->config[$cat][$key] = $value; } + $this->source[$cat][$key] = $source; + return true; } - private function setCatKeyValueSource(string $cat, string $key = null, $value = null, int $source = self::SOURCE_DEFAULT) - { - if (isset($key)) { - $this->config[$cat][$key] = $value; - $this->source[$cat][$key] = $source; - } else { - $this->config[$cat] = $value; - $this->source[$cat] = $source; - } - } - /** * Formats a DB value to a config value * - null = The db-value isn't set @@ -214,7 +204,7 @@ class Cache * * @param string|null $value * - * @return null|array|string + * @return mixed */ public static function toConfigValue(?string $value) { @@ -232,27 +222,24 @@ class Cache /** * Deletes a value from the config cache. * - * @param string $cat Config category - * @param ?string $key Config key (if not set, the whole category will get deleted) - * @param int $source The source of the current config key + * @param string $cat Config category + * @param string $key Config key * * @return bool true, if deleted */ - public function delete(string $cat, string $key = null, int $source = self::SOURCE_DEFAULT): bool + public function delete(string $cat, string $key): bool { if (isset($this->config[$cat][$key])) { - $this->config[$cat][$key] = null; - $this->source[$cat][$key] = $source; - if (empty(array_filter($this->config[$cat], function($value) { return !is_null($value); }))) { - $this->config[$cat] = null; - $this->source[$cat] = $source; + unset($this->config[$cat][$key]); + unset($this->source[$cat][$key]); + if (count($this->config[$cat]) == 0) { + unset($this->config[$cat]); + unset($this->source[$cat]); } - } elseif (isset($this->config[$cat])) { - $this->config[$cat] = null; - $this->source[$cat] = $source; + return true; + } else { + return false; } - - return true; } /** @@ -283,7 +270,7 @@ class Cache $keys = array_keys($config[$category]); foreach ($keys as $key) { - if (!key_exists($key, $this->config[$category] ?? [])) { + if (!isset($this->config[$category][$key])) { $return[$category][$key] = $config[$category][$key]; } } diff --git a/tests/src/Core/Config/Cache/CacheTest.php b/tests/src/Core/Config/Cache/CacheTest.php index e14b7998c5..a4e12e23a6 100644 --- a/tests/src/Core/Config/Cache/CacheTest.php +++ b/tests/src/Core/Config/Cache/CacheTest.php @@ -40,7 +40,6 @@ class CacheTest extends MockedTest 'int' => 235, 'dec' => 2.456, 'array' => ['1', 2, '3', true, false], - 'null' => null, ], 'config' => [ 'a' => 'value', @@ -124,7 +123,7 @@ class CacheTest extends MockedTest // wrong dataset $configCache->load(['system' => 'not_array']); - self::assertEquals(['system' => 'not_array'], $configCache->getAll()); + self::assertEquals([], $configCache->getAll()); // incomplete dataset (key is integer ID of the array) $configCache = new Cache(); @@ -209,16 +208,13 @@ class CacheTest extends MockedTest { $configCache = new Cache($data); - $assertion = []; - foreach ($data as $cat => $values) { - $assertion[$cat] = null; foreach ($values as $key => $value) { $configCache->delete($cat, $key); } } - self::assertEquals($assertion, $configCache->getAll()); + self::assertEmpty($configCache->getAll()); } /** @@ -554,4 +550,30 @@ class CacheTest extends MockedTest self::assertEquals('new_value', $newCache->get($category, 'new_key')); } + + /** + * Test that keys are removed after a deletion + * + * @dataProvider dataTests + * + */ + public function testDeleteRemovesKey($data) + { + $cache = new Cache(); + $cache->load($data, Cache::SOURCE_FILE); + + $cache->set('system', 'test', 'overwrite!', Cache::SOURCE_DATA); + self::assertEquals('overwrite!', $cache->get('system', 'test')); + + // array should now be removed + $cache->delete('system', 'test'); + self::assertArrayNotHasKey('test', $cache->getAll()['system']); + + self::assertArrayHasKey('config', $cache->getAll()); + self::assertArrayHasKey('a', $cache->getAll()['config']); + + // category should now be removed + $cache->delete('config', 'a'); + self::assertArrayNotHasKey('config', $cache->getAll()); + } } From 11a8bd17e3e6a66d1df6ffee22355c9f95398b2b Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 11 Jan 2023 21:53:34 +0100 Subject: [PATCH 2/3] Assure that deleted cat/keys are working as expected - A deleted cache-key would delete a merged cache-key as well - Deleting a key in the Model results in reloading the config to assure any value from underlying files --- src/Core/Config/Model/Config.php | 4 +- src/Core/Config/ValueObject/Cache.php | 19 ++++++++ tests/src/Core/Config/Cache/CacheTest.php | 21 +++++++++ .../Config/Cache/ConfigFileManagerTest.php | 43 ++----------------- tests/src/Core/Config/ConfigTest.php | 20 ++++++++- 5 files changed, 65 insertions(+), 42 deletions(-) diff --git a/src/Core/Config/Model/Config.php b/src/Core/Config/Model/Config.php index c96750694b..5258033a75 100644 --- a/src/Core/Config/Model/Config.php +++ b/src/Core/Config/Model/Config.php @@ -71,6 +71,8 @@ class Config implements IManageConfigValues { try { $this->configFileManager->saveData($this->configCache); + // reload after the save to possible reload default values of lower source-priorities again + $this->reload(); } catch (ConfigFileException $e) { throw new ConfigPersistenceException('Cannot save config', $e); } @@ -116,7 +118,7 @@ class Config implements IManageConfigValues /** {@inheritDoc} */ public function delete(string $cat, string $key): bool { - if ($this->configCache->delete($cat, $key, Cache::SOURCE_DATA)) { + if ($this->configCache->delete($cat, $key)) { $this->save(); return true; } else { diff --git a/src/Core/Config/ValueObject/Cache.php b/src/Core/Config/ValueObject/Cache.php index db972431ac..be42ae1181 100644 --- a/src/Core/Config/ValueObject/Cache.php +++ b/src/Core/Config/ValueObject/Cache.php @@ -55,6 +55,11 @@ class Cache */ private $source = []; + /** + * @var array + */ + private $delConfig = []; + /** * @var bool */ @@ -232,6 +237,7 @@ class Cache if (isset($this->config[$cat][$key])) { unset($this->config[$cat][$key]); unset($this->source[$cat][$key]); + $this->delConfig[$cat][$key] = true; if (count($this->config[$cat]) == 0) { unset($this->config[$cat]); unset($this->source[$cat]); @@ -313,6 +319,19 @@ class Cache } } + $delCategories = array_keys($cache->delConfig); + + foreach ($delCategories as $category) { + if (is_array($cache->delConfig[$category])) { + $keys = array_keys($cache->delConfig[$category]); + + foreach ($keys as $key) { + unset($newConfig[$category][$key]); + unset($newSource[$category][$key]); + } + } + } + $newCache = new Cache(); $newCache->config = $newConfig; $newCache->source = $newSource; diff --git a/tests/src/Core/Config/Cache/CacheTest.php b/tests/src/Core/Config/Cache/CacheTest.php index a4e12e23a6..c4e536871b 100644 --- a/tests/src/Core/Config/Cache/CacheTest.php +++ b/tests/src/Core/Config/Cache/CacheTest.php @@ -576,4 +576,25 @@ class CacheTest extends MockedTest $cache->delete('config', 'a'); self::assertArrayNotHasKey('config', $cache->getAll()); } + + /** + * Test that deleted keys are working with merge + * + * @dataProvider dataTests + */ + public function testDeleteAndMergeWithDefault($data) + { + $cache = new Cache(); + $cache->load($data, Cache::SOURCE_FILE); + + $cache2 = new Cache(); + $cache2->set('system', 'test', 'overrride'); + $cache2->delete('system', 'test'); + + self::assertEquals('it', $cache->get('system', 'test')); + self::assertNull($cache2->get('system', 'test')); + + $mergedCache = $cache->merge($cache2); + self::assertNull($mergedCache->get('system', 'test')); + } } diff --git a/tests/src/Core/Config/Cache/ConfigFileManagerTest.php b/tests/src/Core/Config/Cache/ConfigFileManagerTest.php index 6c2c41ef65..e5b630a200 100644 --- a/tests/src/Core/Config/Cache/ConfigFileManagerTest.php +++ b/tests/src/Core/Config/Cache/ConfigFileManagerTest.php @@ -443,7 +443,7 @@ class ConfigFileManagerTest extends MockedTest } /** - * If we delete something with the Cache::delete() functionality, be sure to override the underlying source as well + * If we delete something with the Cache::delete() functionality, be sure to probably reset it to the underlying key */ public function testDeleteKeyOverwrite() { @@ -465,51 +465,16 @@ class ConfigFileManagerTest extends MockedTest $configFileManager->setupCache($configCache); - $configCache->delete('system', 'default_timezone', Cache::SOURCE_DATA); + $configCache->delete('system', 'default_timezone'); $configFileManager->saveData($configCache); - // assert that system.default_timezone is now null, even it's set with settings.conf.php + // assert that system.default_timezone is now the restored 'UTC' from the defaults $configCache = new Cache(); $configFileManager->setupCache($configCache); - self::assertNull($configCache->get('system', 'default_timezone')); - } - - /** - * If we delete something with the Cache::delete() functionality, be sure to override the underlying source as well - */ - public function testDeleteCategoryOverwrite() - { - $this->delConfigFile('node.config.php'); - - $fileDir = dirname(__DIR__) . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - '..' . DIRECTORY_SEPARATOR . - 'datasets' . DIRECTORY_SEPARATOR . - 'config' . DIRECTORY_SEPARATOR; - - vfsStream::newFile('B.config.php') - ->at($this->root->getChild('config')) - ->setContent(file_get_contents($fileDir . 'B.config.php')); - - $configFileManager = (new Config())->createConfigFileManager($this->root->url()); - $configCache = new Cache(); - - $configFileManager->setupCache($configCache); - - $configCache->delete('system'); - - $configFileManager->saveData($configCache); - - // assert that system.default_timezone is now null, even it's set with settings.conf.php - $configCache = new Cache(); - - $configFileManager->setupCache($configCache); - - self::assertNull($configCache->get('system', 'default_timezone')); + self::assertEquals('UTC', $configCache->get('system', 'default_timezone')); } /** diff --git a/tests/src/Core/Config/ConfigTest.php b/tests/src/Core/Config/ConfigTest.php index f7d7c070d3..f244044743 100644 --- a/tests/src/Core/Config/ConfigTest.php +++ b/tests/src/Core/Config/ConfigTest.php @@ -363,8 +363,6 @@ class ConfigTest extends MockedTest self::assertTrue($this->testedConfig->delete('test', 'it')); self::assertNull($this->testedConfig->get('test', 'it')); self::assertNull($this->testedConfig->getCache()->get('test', 'it')); - - self::assertEquals(['test' => null], $this->testedConfig->getCache()->getAll()); } /** @@ -532,4 +530,22 @@ class ConfigTest extends MockedTest self::assertEquals($assertion, $config->get($category)); } + + /** + * Tests, if an overwritten value of an existing key will reset to it's default after deletion + */ + public function testDeleteReturnsDefault() + { + vfsStream::newFile(ConfigFileManager::CONFIG_DATA_FILE) + ->at($this->root->getChild('config')) + ->setContent(ConfigFileTransformer::encode([ + 'config' => ['sitename' => 'overritten'], + ])); + + $config = $this->getInstance(); + self::assertEquals('overritten', $config->get('config', 'sitename')); + + $config->delete('config', 'sitename'); + self::assertEquals('Friendica Social Network', $config->get('config', 'sitename')); + } } From fef10e8a57984a90a164102583a42de351502df8 Mon Sep 17 00:00:00 2001 From: Philipp Date: Wed, 11 Jan 2023 22:00:29 +0100 Subject: [PATCH 3/3] Improve encapsulation --- .../Config/Capability/IManageConfigValues.php | 14 ----------- src/Core/Config/Model/Config.php | 25 +++++++++++++------ src/Core/Config/Model/ConfigTransaction.php | 4 +-- src/Core/Config/ValueObject/Cache.php | 2 +- 4 files changed, 21 insertions(+), 24 deletions(-) diff --git a/src/Core/Config/Capability/IManageConfigValues.php b/src/Core/Config/Capability/IManageConfigValues.php index 09e2cd9144..2c3d6da3c8 100644 --- a/src/Core/Config/Capability/IManageConfigValues.php +++ b/src/Core/Config/Capability/IManageConfigValues.php @@ -58,20 +58,6 @@ interface IManageConfigValues */ public function get(string $cat, string $key = null, $default_value = null); - /** - * Load all configuration values from a given cache and saves it back in the configuration node store - * @see ConfigFileManager::CONFIG_DATA_FILE - * - * All configuration values of the system are stored in the cache. - * - * @param Cache $cache a new cache - * - * @return void - * - * @throws ConfigPersistenceException In case the persistence layer throws errors - */ - public function load(Cache $cache); - /** * Sets a configuration value for system config * diff --git a/src/Core/Config/Model/Config.php b/src/Core/Config/Model/Config.php index 5258033a75..46d5643b34 100644 --- a/src/Core/Config/Model/Config.php +++ b/src/Core/Config/Model/Config.php @@ -49,6 +49,24 @@ class Config implements IManageConfigValues $this->configCache = $configCache; } + /** + * Load all configuration values from a given cache and saves it back in the configuration node store + * @see ConfigFileManager::CONFIG_DATA_FILE + * + * All configuration values of the system are stored in the cache. + * + * @param Cache $cache a new cache + * + * @return void + * + * @throws ConfigPersistenceException In case the persistence layer throws errors + */ + public function setCacheAndSave(Cache $cache) + { + $this->configCache = $cache; + $this->save(); + } + /** * {@inheritDoc} */ @@ -91,13 +109,6 @@ class Config implements IManageConfigValues $this->configCache = $configCache; } - /** {@inheritDoc} */ - public function load(Cache $cache) - { - $this->configCache = $cache; - $this->save(); - } - /** {@inheritDoc} */ public function get(string $cat, string $key = null, $default_value = null) { diff --git a/src/Core/Config/Model/ConfigTransaction.php b/src/Core/Config/Model/ConfigTransaction.php index ec0a2b9c8b..b9310301f3 100644 --- a/src/Core/Config/Model/ConfigTransaction.php +++ b/src/Core/Config/Model/ConfigTransaction.php @@ -57,7 +57,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally /** {@inheritDoc} */ public function delete(string $cat, string $key): ISetConfigValuesTransactionally { - $this->cache->delete($cat, $key, Cache::SOURCE_DATA); + $this->cache->delete($cat, $key); $this->changedConfig = true; return $this; @@ -72,7 +72,7 @@ class ConfigTransaction implements ISetConfigValuesTransactionally } try { - $this->config->load($this->cache); + $this->config->setCacheAndSave($this->cache); $this->cache = clone $this->config->getCache(); } catch (\Exception $e) { throw new ConfigPersistenceException('Cannot save config', $e); diff --git a/src/Core/Config/ValueObject/Cache.php b/src/Core/Config/ValueObject/Cache.php index be42ae1181..acfac3ab9a 100644 --- a/src/Core/Config/ValueObject/Cache.php +++ b/src/Core/Config/ValueObject/Cache.php @@ -56,7 +56,7 @@ class Cache private $source = []; /** - * @var array + * @var bool[][] */ private $delConfig = [];