From 4bfd90d324ca6b743b04db5963bcb3aea44617e8 Mon Sep 17 00:00:00 2001 From: Dmytro Poperechnyy Date: Wed, 21 Jun 2017 16:07:57 +0300 Subject: [PATCH] MAGETWO-69894: Configuration values return null when redis reaches max_memory --- .../Magento/Config/App/Config/Type/System.php | 235 ++++++++++-------- .../Config/App/Config/Type/System/Reader.php | 71 ++++++ .../App/Config/Type/System/ReaderTest.php | 93 +++++++ .../Test/Unit/App/Config/Type/SystemTest.php | 97 ++++---- app/code/Magento/Config/etc/di.xml | 8 + .../Store/Model/Config/Placeholder.php | 4 +- .../Config/App/Config/Type/SystemTest.php | 50 ++++ 7 files changed, 412 insertions(+), 146 deletions(-) create mode 100644 app/code/Magento/Config/App/Config/Type/System/Reader.php create mode 100644 app/code/Magento/Config/Test/Unit/App/Config/Type/System/ReaderTest.php create mode 100644 dev/tests/integration/testsuite/Magento/Config/App/Config/Type/SystemTest.php diff --git a/app/code/Magento/Config/App/Config/Type/System.php b/app/code/Magento/Config/App/Config/Type/System.php index a10462d87b205..9208d2bf2f816 100644 --- a/app/code/Magento/Config/App/Config/Type/System.php +++ b/app/code/Magento/Config/App/Config/Type/System.php @@ -6,10 +6,11 @@ namespace Magento\Config\App\Config\Type; use Magento\Framework\App\Config\ConfigTypeInterface; -use Magento\Framework\DataObject; +use Magento\Framework\App\ObjectManager; +use Magento\Config\App\Config\Type\System\Reader; /** - * Class process source, cache them and retrieve value by path + * System configuration type */ class System implements ConfigTypeInterface { @@ -23,9 +24,9 @@ class System implements ConfigTypeInterface private $source; /** - * @var DataObject + * @var array */ - private $data; + private $data = []; /** * @var \Magento\Framework\App\Config\Spi\PostProcessorInterface @@ -65,14 +66,18 @@ class System implements ConfigTypeInterface private $configType; /** - * Key name for flag which displays whether configuration is cached or not. + * @var Reader + */ + private $reader; + + /** + * List of scopes that were retrieved from configuration storage * - * Once configuration is cached additional flag pushed to cache storage - * to be able check cache existence without data load. + * Is used to make sure that we don't try to load non-existing configuration scopes. * - * @var string + * @var array */ - private $cacheExistenceKey; + private $availableDataScopes = null; /** * @param \Magento\Framework\App\Config\ConfigSourceInterface $source @@ -83,6 +88,7 @@ class System implements ConfigTypeInterface * @param \Magento\Framework\App\Config\Spi\PreProcessorInterface $preProcessor * @param int $cachingNestedLevel * @param string $configType + * @param Reader $reader */ public function __construct( \Magento\Framework\App\Config\ConfigSourceInterface $source, @@ -92,7 +98,8 @@ public function __construct( \Magento\Framework\Serialize\SerializerInterface $serializer, \Magento\Framework\App\Config\Spi\PreProcessorInterface $preProcessor, $cachingNestedLevel = 1, - $configType = self::CONFIG_TYPE + $configType = self::CONFIG_TYPE, + Reader $reader = null ) { $this->source = $source; $this->postProcessor = $postProcessor; @@ -102,150 +109,174 @@ public function __construct( $this->fallback = $fallback; $this->serializer = $serializer; $this->configType = $configType; - $this->cacheExistenceKey = $this->configType . '_CACHE_EXISTS'; + $this->reader = $reader ?: ObjectManager::getInstance()->get(Reader::class); } /** + * System configuration is separated by scopes (default, websites, stores). Configuration of a scope is inherited + * from its parent scope (store inherits website). + * + * Because there can be many scopes on single instance of application, the configuration data can be pretty large, + * so it does not make sense to load all of it on every application request. That is why we cache configuration + * data by scope and only load configuration scope when a value from that scope is requested. + * + * Possible path values: + * '' - will return whole system configuration (default scope + all other scopes) + * 'default' - will return all default scope configuration values + * '{scopeType}' - will return data from all scopes of a specified {scopeType} (websites, stores) + * '{scopeType}/{scopeCode}' - will return data for all values of the scope specified by {scopeCode} and scope type + * '{scopeType}/{scopeCode}/some/config/variable' - will return value of the config variable in the specified scope + * * @inheritdoc */ public function get($path = '') { - if ($path === null) { - $path = ''; + if ($path === '') { + $this->data = array_replace_recursive($this->loadAllData(), $this->data); + return $this->data; } - if ($this->isConfigRead($path)) { - return $this->data->getData($path); + $pathParts = explode('/', $path); + if (count($pathParts) === 1 && $pathParts[0] !== 'default') { + if (!isset($this->data[$pathParts[0]])) { + $data = $this->reader->read(); + $this->data = array_replace_recursive($data, $this->data); + } + return $this->data[$pathParts[0]]; } - - if (!empty($path) && $this->isCacheExists()) { - return $this->readFromCache($path); + $scopeType = array_shift($pathParts); + if ($scopeType === 'default') { + if (!isset($this->data[$scopeType])) { + $this->data = array_replace_recursive($this->loadDefaultScopeData($scopeType), $this->data); + } + return $this->getDataByPathParts($this->data[$scopeType], $pathParts); } - - $config = $this->loadConfig(); - $this->cacheConfig($config); - $this->data = new DataObject($config); - return $this->data->getData($path); - } - - /** - * Check whether configuration is cached - * - * In case configuration cache exists method 'load' returns - * value equal to $this->cacheExistenceKey - * - * @return bool - */ - private function isCacheExists() - { - return $this->cache->load($this->cacheExistenceKey) !== false; + $scopeId = array_shift($pathParts); + if (!isset($this->data[$scopeType][$scopeId])) { + $this->data = array_replace_recursive($this->loadScopeData($scopeType, $scopeId), $this->data); + } + return isset($this->data[$scopeType][$scopeId]) + ? $this->getDataByPathParts($this->data[$scopeType][$scopeId], $pathParts) + : null; } /** - * Explode path by '/'(forward slash) separator - * - * In case $path string contains forward slash symbol(/) the $path is exploded and parts array is returned - * In other case empty array is returned + * Load configuration data for all scopes * - * @param string $path * @return array */ - private function getPathParts($path) + private function loadAllData() { - $pathParts = []; - if (strpos($path, '/') !== false) { - $pathParts = explode('/', $path); + $cachedData = $this->cache->load($this->configType); + if ($cachedData === false) { + $data = $this->reader->read(); + } else { + $data = $this->serializer->unserialize($cachedData); } - return $pathParts; + return $data; } /** - * Check whether requested configuration data is read to memory - * - * Because of configuration is cached partially each part can be loaded separately - * Method performs check if corresponding system configuration part is already loaded to memory - * and value can be retrieved directly without cache look up + * Load configuration data for default scope * - * - * @param string $path - * @return bool + * @param string $scopeType + * @return array */ - private function isConfigRead($path) + private function loadDefaultScopeData($scopeType) { - $pathParts = $this->getPathParts($path); - return !empty($pathParts) && isset($this->data[$pathParts[0]][$pathParts[1]]); + $cachedData = $this->cache->load($this->configType . '_' . $scopeType); + if ($cachedData === false) { + $data = $this->reader->read(); + $this->cacheData($data); + } else { + $data = [$scopeType => $this->serializer->unserialize($cachedData)]; + } + return $data; } /** - * Load configuration from all the sources - * - * System configuration is loaded in 3 steps performing consecutive calls to - * Pre Processor, Fallback Processor, Post Processor accordingly + * Load configuration data for a specified scope * + * @param string $scopeType + * @param string $scopeId * @return array */ - private function loadConfig() + private function loadScopeData($scopeType, $scopeId) { - $data = $this->preProcessor->process($this->source->get()); - $this->data = new DataObject($data); - $data = $this->fallback->process($data); - $this->data = new DataObject($data); - - return $this->postProcessor->process($data); + $cachedData = $this->cache->load($this->configType . '_' . $scopeType . '_' . $scopeId); + if ($cachedData === false) { + if ($this->availableDataScopes === null) { + $cachedScopeData = $this->cache->load($this->configType . '_scopes'); + if ($cachedScopeData !== false) { + $this->availableDataScopes = $this->serializer->unserialize($cachedScopeData); + } + } + if (is_array($this->availableDataScopes) && !isset($this->availableDataScopes[$scopeType][$scopeId])) { + return [$scopeType => [$scopeId => []]]; + } + $data = $this->reader->read(); + $this->cacheData($data); + } else { + $data = [$scopeType => [$scopeId => $this->serializer->unserialize($cachedData)]]; + } + return $data; } /** - * - * Load configuration and caching it by parts. - * - * To be cached configuration is loaded first. - * Then it is cached by parts to minimize memory usage on load. - * Additional flag cached as well to give possibility check cache existence without data load. + * Cache configuration data. + * Caches data per scope to avoid reading data for all scopes on every request * * @param array $data * @return void */ - private function cacheConfig($data) + private function cacheData(array $data) { - foreach ($data as $scope => $scopeData) { - foreach ($scopeData as $key => $config) { + $this->cache->save( + $this->serializer->serialize($data), + $this->configType, + [self::CACHE_TAG] + ); + $this->cache->save( + $this->serializer->serialize($data['default']), + $this->configType . '_default', + [self::CACHE_TAG] + ); + $scopes = []; + foreach (['websites', 'stores'] as $curScopeType) { + foreach ($data[$curScopeType] as $curScopeId => $curScopeData) { + $scopes[$curScopeType][$curScopeId] = 1; $this->cache->save( - $this->serializer->serialize($config), - $this->configType . '_' . $scope . $key, + $this->serializer->serialize($curScopeData), + $this->configType . '_' . $curScopeType . '_' . $curScopeId, [self::CACHE_TAG] ); } } - $this->cache->save($this->cacheExistenceKey, $this->cacheExistenceKey, [self::CACHE_TAG]); + $this->cache->save( + $this->serializer->serialize($scopes), + $this->configType . "_scopes", + [self::CACHE_TAG] + ); } /** - * Read cached configuration + * Walk nested hash map by keys from $pathParts * - * Read section of system configuration corresponding to requested $path from cache - * Configuration stored to internal property right after load to prevent additional - * requests to cache storage - * - * @param string $path + * @param array $data to walk in + * @param array $pathParts keys path * @return mixed */ - private function readFromCache($path) + private function getDataByPathParts($data, $pathParts) { - if ($this->data === null) { - $this->data = new DataObject(); - } - - $result = null; - $pathParts = $this->getPathParts($path); - if (!empty($pathParts)) { - $result = $this->cache->load($this->configType . '_' . $pathParts[0] . $pathParts[1]); - if ($result !== false) { - $readData = $this->data->getData(); - $readData[$pathParts[0]][$pathParts[1]] = $this->serializer->unserialize($result); - $this->data->setData($readData); + foreach ($pathParts as $key) { + if ((array)$data === $data && isset($data[$key])) { + $data = $data[$key]; + } elseif ($data instanceof \Magento\Framework\DataObject) { + $data = $data->getDataByKey($key); + } else { + return null; } } - - return $this->data->getData($path); + return $data; } /** @@ -259,7 +290,7 @@ private function readFromCache($path) */ public function clean() { - $this->data = null; + $this->data = []; $this->cache->clean(\Zend_Cache::CLEANING_MODE_MATCHING_TAG, [self::CACHE_TAG]); } } diff --git a/app/code/Magento/Config/App/Config/Type/System/Reader.php b/app/code/Magento/Config/App/Config/Type/System/Reader.php new file mode 100644 index 0000000000000..d5a0f09cdd631 --- /dev/null +++ b/app/code/Magento/Config/App/Config/Type/System/Reader.php @@ -0,0 +1,71 @@ +source = $source; + $this->fallback = $fallback; + $this->preProcessor = $preProcessor; + $this->postProcessor = $postProcessor; + } + + /** + * Retrieve and process system configuration data + * + * Processing includes configuration fallback (default, website, store) and placeholder replacement + * + * @return array + */ + public function read() + { + return $this->postProcessor->process( + $this->fallback->process( + $this->preProcessor->process( + $this->source->get() + ) + ) + ); + } +} diff --git a/app/code/Magento/Config/Test/Unit/App/Config/Type/System/ReaderTest.php b/app/code/Magento/Config/Test/Unit/App/Config/Type/System/ReaderTest.php new file mode 100644 index 0000000000000..3f02e22465aa0 --- /dev/null +++ b/app/code/Magento/Config/Test/Unit/App/Config/Type/System/ReaderTest.php @@ -0,0 +1,93 @@ +source = $this->getMockBuilder(ConfigSourceInterface::class) + ->disableOriginalConstructor() + ->getMockForAbstractClass(); + $this->fallback = $this->getMockBuilder(Fallback::class) + ->disableOriginalConstructor() + ->getMock(); + $this->preProcessor = $this->getMockBuilder(PreProcessorInterface::class) + ->disableOriginalConstructor() + ->getMockForAbstractClass(); + $this->postProcessor = $this->getMockBuilder(PostProcessorInterface::class) + ->disableOriginalConstructor() + ->getMockForAbstractClass(); + + $this->model = $helper->getObject( + Reader::class, + [ + 'source' => $this->source, + 'fallback' => $this->fallback, + 'preProcessor' => $this->preProcessor, + 'postProcessor' => $this->postProcessor + ] + ); + } + + public function testGetCachedWithLoadDefaultScopeData() + { + $data = [ + 'default' => [], + 'websites' => [], + 'stores' => [] + ]; + $this->source->expects($this->once()) + ->method('get') + ->willReturn($data); + $this->preProcessor->expects($this->once()) + ->method('process') + ->with($data) + ->willReturn($data); + $this->fallback->expects($this->once()) + ->method('process') + ->with($data) + ->willReturn($data); + $this->postProcessor->expects($this->once()) + ->method('process') + ->with($data) + ->willReturn($data); + $this->assertEquals($data, $this->model->read()); + } +} diff --git a/app/code/Magento/Config/Test/Unit/App/Config/Type/SystemTest.php b/app/code/Magento/Config/Test/Unit/App/Config/Type/SystemTest.php index 985ef34fe0238..cbc89df802890 100644 --- a/app/code/Magento/Config/Test/Unit/App/Config/Type/SystemTest.php +++ b/app/code/Magento/Config/Test/Unit/App/Config/Type/SystemTest.php @@ -13,6 +13,7 @@ use Magento\Framework\Cache\FrontendInterface; use Magento\Framework\Serialize\SerializerInterface; use Magento\Store\Model\Config\Processor\Fallback; +use Magento\Config\App\Config\Type\System\Reader; /** * Test how Class process source, cache them and retrieve value by path @@ -55,6 +56,11 @@ class SystemTest extends \PHPUnit_Framework_TestCase */ private $serializer; + /** + * @var Reader|\PHPUnit_Framework_MockObject_MockObject + */ + private $reader; + public function setUp() { $this->source = $this->getMockBuilder(ConfigSourceInterface::class) @@ -70,6 +76,9 @@ public function setUp() ->getMockForAbstractClass(); $this->serializer = $this->getMockBuilder(SerializerInterface::class) ->getMock(); + $this->reader = $this->getMockBuilder(Reader::class) + ->disableOriginalConstructor() + ->getMock(); $this->configType = new System( $this->source, @@ -77,23 +86,27 @@ public function setUp() $this->fallback, $this->cache, $this->serializer, - $this->preProcessor + $this->preProcessor, + 1, + 'system', + $this->reader ); } - public function testGetCached() + public function testGetCachedWithLoadDefaultScopeData() { $path = 'default/dev/unsecure/url'; $url = 'http://magento.test/'; $data = [ - 'unsecure' => [ - 'url' => $url + 'dev' => [ + 'unsecure' => [ + 'url' => $url + ] ] ]; $this->cache->expects($this->any()) ->method('load') - ->withConsecutive(['system_CACHE_EXISTS'], ['system_defaultdev']) ->willReturnOnConsecutiveCalls('1', serialize($data)); $this->serializer->expects($this->once()) ->method('unserialize') @@ -101,62 +114,62 @@ public function testGetCached() $this->assertEquals($url, $this->configType->get($path)); } - public function testGetNotCached() + public function testGetCachedWithLoadAllData() { - $path = 'default/dev/unsecure/url'; $url = 'http://magento.test/'; $data = [ - 'default' => [ - 'dev' => [ - 'unsecure' => [ - 'url' => $url - ] + 'dev' => [ + 'unsecure' => [ + 'url' => $url ] ] ]; + $this->cache->expects($this->any()) + ->method('load') + ->willReturnOnConsecutiveCalls('1', serialize($data)); + $this->serializer->expects($this->once()) + ->method('unserialize') + ->willReturn($data); + $this->assertEquals($data, $this->configType->get('')); + } + + public function testGetNotCached() + { + $path = 'stores/default/dev/unsecure/url'; + $url = 'http://magento.test/'; + $dataToCache = [ 'unsecure' => [ 'url' => $url ] ]; + $data = [ + 'default' => [], + 'websites' => [], + 'stores' => [ + 'default' => [ + 'dev' => [ + 'unsecure' => [ + 'url' => $url + ] + ] + ] + ] + ]; $this->cache->expects($this->any()) ->method('load') - ->withConsecutive(['system_CACHE_EXISTS'], ['system_defaultdev']) ->willReturnOnConsecutiveCalls(false, false); - $this->serializer->expects($this->once()) + $this->serializer->expects($this->atLeastOnce()) ->method('serialize') ->willReturn(serialize($dataToCache)); - $this->source->expects($this->once()) - ->method('get') - ->willReturn($data); - $this->fallback->expects($this->once()) - ->method('process') - ->with($data) - ->willReturnArgument(0); - $this->preProcessor->expects($this->once()) - ->method('process') - ->with($data) - ->willReturnArgument(0); - $this->postProcessor->expects($this->once()) - ->method('process') - ->with($data) - ->willReturnArgument(0); - $this->cache->expects($this->any()) + $this->cache->expects($this->atLeastOnce()) ->method('save') - ->withConsecutive( - [ - serialize($dataToCache), - 'system_defaultdev', - [System::CACHE_TAG] - ], - [ - 'system_CACHE_EXISTS', - 'system_CACHE_EXISTS', - [System::CACHE_TAG] - ] - ); + ->willReturnSelf(); + $this->reader->expects($this->once()) + ->method('read') + ->willReturn($data); $this->assertEquals($url, $this->configType->get($path)); } diff --git a/app/code/Magento/Config/etc/di.xml b/app/code/Magento/Config/etc/di.xml index 1abc5e35f578f..7711d2fd1fdde 100644 --- a/app/code/Magento/Config/etc/di.xml +++ b/app/code/Magento/Config/etc/di.xml @@ -84,6 +84,14 @@ Magento\Framework\App\Cache\Type\Config systemConfigPreProcessorComposite Magento\Framework\Serialize\Serializer\Serialize + Magento\Config\App\Config\Type\System\Reader\Proxy + + + + + systemConfigSourceAggregated + systemConfigPostProcessorComposite + systemConfigPreProcessorComposite diff --git a/app/code/Magento/Store/Model/Config/Placeholder.php b/app/code/Magento/Store/Model/Config/Placeholder.php index 3636390cf375f..b08e58c59c1a7 100644 --- a/app/code/Magento/Store/Model/Config/Placeholder.php +++ b/app/code/Magento/Store/Model/Config/Placeholder.php @@ -91,8 +91,8 @@ protected function _processPlaceholders($value, $data) if ($url) { $value = str_replace('{{' . $placeholder . '}}', $url, $value); } elseif (strpos($value, $this->urlPlaceholder) !== false) { - $distroBaseUrl = $this->request->getDistroBaseUrl(); - $value = str_replace($this->urlPlaceholder, $distroBaseUrl, $value); + // localhost is replaced for cli requests, for http requests method getDistroBaseUrl is used + $value = str_replace($this->urlPlaceholder, 'http://localhost/', $value); } if (null !== $this->_getPlaceholder($value)) { diff --git a/dev/tests/integration/testsuite/Magento/Config/App/Config/Type/SystemTest.php b/dev/tests/integration/testsuite/Magento/Config/App/Config/Type/SystemTest.php new file mode 100644 index 0000000000000..9ab436ad077ca --- /dev/null +++ b/dev/tests/integration/testsuite/Magento/Config/App/Config/Type/SystemTest.php @@ -0,0 +1,50 @@ +objectManager = Bootstrap::getObjectManager(); + $this->system = $this->objectManager->create(System::class); + } + + public function testGetValueDefaultScope() + { + $this->assertEquals( + 'value1.db.default.test', + $this->system->get('default/web/test/test_value_1') + ); + + $this->assertEquals( + 'value1.db.website_base.test', + $this->system->get('websites/base/web/test/test_value_1') + ); + + $this->assertEquals( + 'value1.db.store_default.test', + $this->system->get('stores/default/web/test/test_value_1') + ); + } +}