From 04389fc8b0f965fb833b1fc6109223d65d4021f8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 7 Aug 2019 17:01:09 +0200 Subject: [PATCH 1/3] Added support in RedisFactory to provide servers as a comma-separated string --- module/Common/src/Cache/RedisFactory.php | 11 ++++------- module/Common/test/Cache/RedisFactoryTest.php | 3 +++ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/module/Common/src/Cache/RedisFactory.php b/module/Common/src/Cache/RedisFactory.php index 6e7c864b..42118767 100644 --- a/module/Common/src/Cache/RedisFactory.php +++ b/module/Common/src/Cache/RedisFactory.php @@ -6,9 +6,8 @@ namespace Shlinkio\Shlink\Common\Cache; use Predis\Client as PredisClient; use Psr\Container\ContainerInterface; -use function array_shift; use function count; -use function is_array; +use function explode; use function is_string; class RedisFactory @@ -18,13 +17,11 @@ class RedisFactory public function __invoke(ContainerInterface $container): PredisClient { $redisConfig = $container->get('config')['redis'] ?? []; + $servers = $redisConfig['servers'] ?? []; + $servers = is_string($servers) ? explode(',', $servers) : $servers; + $options = count($servers) <= 1 ? null : ['cluster' => 'redis']; - if (is_array($servers) && count($servers) === 1) { - $servers = array_shift($servers); - } - - $options = is_string($servers) || count($servers) < 1 ? null : ['cluster' => 'redis']; return new PredisClient($servers, $options); } } diff --git a/module/Common/test/Cache/RedisFactoryTest.php b/module/Common/test/Cache/RedisFactoryTest.php index 765a3e8f..dbc1c9b8 100644 --- a/module/Common/test/Cache/RedisFactoryTest.php +++ b/module/Common/test/Cache/RedisFactoryTest.php @@ -54,5 +54,8 @@ class RedisFactoryTest extends TestCase yield 'empty cluster of servers' => [[ 'servers' => [], ], PredisCluster::class]; + yield 'cluster of servers as string' => [[ + 'servers' => 'tcp://1.1.1.1:6379,tcp://2.2.2.2:6379', + ], RedisCluster::class]; } } From 73fd348490513166ebb8dcda6fae4746d96d4235 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 7 Aug 2019 17:37:24 +0200 Subject: [PATCH 2/3] Ensured Redis lock store is wrapped into a retry adapter --- config/autoload/locks.global.php | 6 +++ .../Lock/RetryLockStoreDelegatorFactory.php | 18 ++++++++ .../RetryLockStoreDelegatorFactoryTest.php | 41 +++++++++++++++++++ 3 files changed, 65 insertions(+) create mode 100644 module/Common/src/Lock/RetryLockStoreDelegatorFactory.php create mode 100644 module/Common/test/Lock/RetryLockStoreDelegatorFactoryTest.php diff --git a/config/autoload/locks.global.php b/config/autoload/locks.global.php index f76c911e..cbf3a000 100644 --- a/config/autoload/locks.global.php +++ b/config/autoload/locks.global.php @@ -2,6 +2,7 @@ declare(strict_types=1); use Shlinkio\Shlink\Common\Cache\RedisFactory; +use Shlinkio\Shlink\Common\Lock\RetryLockStoreDelegatorFactory; use Symfony\Component\Lock; use Zend\ServiceManager\AbstractFactory\ConfigAbstractFactory; @@ -22,6 +23,11 @@ return [ 'lock_store' => Lock\Store\FlockStore::class, 'redis_lock_store' => Lock\Store\RedisStore::class, ], + 'delegators' => [ + Lock\Store\RedisStore::class => [ + RetryLockStoreDelegatorFactory::class, + ], + ], ], ConfigAbstractFactory::class => [ diff --git a/module/Common/src/Lock/RetryLockStoreDelegatorFactory.php b/module/Common/src/Lock/RetryLockStoreDelegatorFactory.php new file mode 100644 index 00000000..7828e24f --- /dev/null +++ b/module/Common/src/Lock/RetryLockStoreDelegatorFactory.php @@ -0,0 +1,18 @@ +originalStore = $this->prophesize(StoreInterface::class)->reveal(); + $this->delegator = new RetryLockStoreDelegatorFactory(); + } + + /** @test */ + public function originalStoreIsWrappedInRetryStore(): void + { + $callback = function () { + return $this->originalStore; + }; + + $result = ($this->delegator)(new ServiceManager(), '', $callback); + + $ref = new ReflectionObject($result); + $prop = $ref->getProperty('decorated'); + $prop->setAccessible(true); + + $this->assertSame($this->originalStore, $prop->getValue($result)); + } +} From 6b8ca3e611d195966b5cedc60c8374754bf6957e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 7 Aug 2019 18:45:28 +0200 Subject: [PATCH 3/3] Updated SimplifiedConfigParser so that it properly converts the redis_servers keys and aliases the store as a side effect --- config/config.php | 2 +- ...ocessor.php => SimplifiedConfigParser.php} | 25 +++++++++++++------ ...est.php => SimplifiedConfigParserTest.php} | 18 ++++++++++--- 3 files changed, 33 insertions(+), 12 deletions(-) rename module/Core/src/{ConfigPostProcessor.php => SimplifiedConfigParser.php} (64%) rename module/Core/test/{ConfigPostProcessorTest.php => SimplifiedConfigParserTest.php} (84%) diff --git a/config/config.php b/config/config.php index 03b949a9..2c29fe67 100644 --- a/config/config.php +++ b/config/config.php @@ -26,5 +26,5 @@ return (new ConfigAggregator\ConfigAggregator([ ? new ConfigAggregator\PhpFileProvider('config/test/*.global.php') : new ConfigAggregator\ZendConfigProvider('config/params/{generated_config.php,*.config.{php,json}}'), ], 'data/cache/app_config.php', [ - Core\ConfigPostProcessor::class, + Core\SimplifiedConfigParser::class, ]))->getMergedConfig(); diff --git a/module/Core/src/ConfigPostProcessor.php b/module/Core/src/SimplifiedConfigParser.php similarity index 64% rename from module/Core/src/ConfigPostProcessor.php rename to module/Core/src/SimplifiedConfigParser.php index 4a7dc53c..7cbb1b0a 100644 --- a/module/Core/src/ConfigPostProcessor.php +++ b/module/Core/src/SimplifiedConfigParser.php @@ -11,7 +11,7 @@ use function array_key_exists; use function Functional\contains; use function Functional\reduce_left; -class ConfigPostProcessor +class SimplifiedConfigParser { private const SIMPLIFIED_CONFIG_MAPPING = [ 'disable_track_param' => ['app_options', 'disable_track_param'], @@ -22,11 +22,21 @@ class ConfigPostProcessor 'db_config' => ['entity_manager', 'connection'], 'delete_short_url_threshold' => ['delete_short_urls', 'visits_threshold'], 'locale' => ['translator', 'locale'], - 'lock_store' => ['dependencies', 'aliases', 'lock_store'], + 'redis_servers' => ['redis', 'servers'], ]; - private const SIMPLIFIED_CONFIG_TOGGLES = [ - 'not_found_redirect_to' => ['url_shortener', 'not_found_short_url', 'enable_redirection'], - 'delete_short_url_threshold' => ['delete_short_urls', 'check_visits_threshold'], + private const SIMPLIFIED_CONFIG_SIDE_EFFECTS = [ + 'not_found_redirect_to' => [ + 'path' => ['url_shortener', 'not_found_short_url', 'enable_redirection'], + 'value' => true, + ], + 'delete_short_url_threshold' => [ + 'path' => ['delete_short_urls', 'check_visits_threshold'], + 'value' => true, + ], + 'redis_servers' => [ + 'path' => ['dependencies', 'aliases', 'lock_store'], + 'value' => 'redis_lock_store', + ], ]; private const SIMPLIFIED_MERGEABLE_CONFIG = ['db_config']; @@ -41,8 +51,9 @@ class ConfigPostProcessor } $collection->setValueInPath($value, $path); - if (array_key_exists($key, self::SIMPLIFIED_CONFIG_TOGGLES)) { - $collection->setValueInPath(true, self::SIMPLIFIED_CONFIG_TOGGLES[$key]); + if (array_key_exists($key, self::SIMPLIFIED_CONFIG_SIDE_EFFECTS)) { + ['path' => $sideEffectPath, 'value' => $sideEffectValue] = self::SIMPLIFIED_CONFIG_SIDE_EFFECTS[$key]; + $collection->setValueInPath($sideEffectValue, $sideEffectPath); } return $collection; diff --git a/module/Core/test/ConfigPostProcessorTest.php b/module/Core/test/SimplifiedConfigParserTest.php similarity index 84% rename from module/Core/test/ConfigPostProcessorTest.php rename to module/Core/test/SimplifiedConfigParserTest.php index 642ccdfc..fbef2567 100644 --- a/module/Core/test/ConfigPostProcessorTest.php +++ b/module/Core/test/SimplifiedConfigParserTest.php @@ -4,17 +4,17 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core; use PHPUnit\Framework\TestCase; -use Shlinkio\Shlink\Core\ConfigPostProcessor; +use Shlinkio\Shlink\Core\SimplifiedConfigParser; use function array_merge; -class ConfigPostProcessorTest extends TestCase +class SimplifiedConfigParserTest extends TestCase { private $postProcessor; public function setUp(): void { - $this->postProcessor = new ConfigPostProcessor(); + $this->postProcessor = new SimplifiedConfigParser(); } /** @test */ @@ -41,7 +41,10 @@ class ConfigPostProcessorTest extends TestCase 'delete_short_url_threshold' => 50, 'locale' => 'es', 'not_found_redirect_to' => 'foobar.com', - 'lock_store' => 'redis_lock_store', + 'redis_servers' => [ + 'tcp://1.1.1.1:1111', + 'tcp://1.2.2.2:2222', + ], 'db_config' => [ 'dbname' => 'shlink', 'user' => 'foo', @@ -91,6 +94,13 @@ class ConfigPostProcessorTest extends TestCase 'lock_store' => 'redis_lock_store', ], ], + + 'redis' => [ + 'servers' => [ + 'tcp://1.1.1.1:1111', + 'tcp://1.2.2.2:2222', + ], + ], ]; $result = ($this->postProcessor)(array_merge($config, $simplified));