From c27a362cb93733948c44d087a3c7a758d34ec330 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Fri, 16 Dec 2022 22:40:18 +0100 Subject: [PATCH 1/4] qa: optimize some psalm settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Allow `MixedAssignment` issue (by suppressing it). This issue will probably removed in future versions of psalm anyways and it mostly makes no real sense or is not avoidable. For further information, see: https://github.com/vimeo/psalm/pull/8776 Allow `RedundantCastGivenDocblockType` as at least the redis extension does not always provide proper typs. The `RedisException` as well as the `RedisClusterException` does not always provide an integer `code` and thus we have to cast it to ensure that this will always be an integer. Once the extension implemented native type-hints, a `RedundantCast` will be emitted and we are good to remove the cast when requiring the extension version which implemented native type-hints. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- psalm-baseline.xml | 59 ++----------------- psalm.xml | 22 +++++++ src/RedisCluster.php | 6 -- src/RedisClusterResourceManager.php | 12 ---- .../RedisClusterWithPhpIgbinaryTest.php | 1 - .../RedisClusterWithPhpSerializeTest.php | 1 - .../RedisClusterWithoutSerializerTest.php | 1 - .../CacheItemPool/RedisIntegrationTest.php | 1 - test/unit/RedisClusterOptionsTest.php | 1 - 9 files changed, 26 insertions(+), 78 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index ce12de0..4c693c3 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -1,5 +1,5 @@ - + $ttl @@ -39,14 +39,6 @@ $normalizedKeys - - $namespacedKeyValuePairs[$this->namespacePrefix . $normalizedKey] - $normalizedKey - $serializer - $serializer - $value - $value - RedisOptions int|float @@ -145,8 +137,7 @@ $resource['server'] $server - - $info['redis_version'] + $resource['database'] $resource['initialized'] $resource['initialized'] @@ -199,39 +190,6 @@ $libOptions[$constValue] - - $constValue - $info - $k - $key - $key - $libOptions[$constValue] - $redis - $redis - $resource - $resource - $resource - $resource - $resource - $resource - $resource - $resource - $resource - $resource - $resource - $resource - $resource['version'] - $resource['version'] - $result[$key] - $server - $success - $success - $success - $success - $v - $value - $value - RedisResource array @@ -242,17 +200,15 @@ string string|null - + auth connect connect connect getOption getOption - info pconnect select - setOption $resource['database'] @@ -270,18 +226,11 @@ (int) $server['port'] - + (int) $database - (int) $key (string) $id (string) $persistentId - - $info - - - $info - diff --git a/psalm.xml b/psalm.xml index d823d5c..ee3dd0a 100644 --- a/psalm.xml +++ b/psalm.xml @@ -6,6 +6,7 @@ xmlns="https://getpsalm.org/schema/config" xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd" errorBaseline="psalm-baseline.xml" + findUnusedPsalmSuppress="true" > @@ -26,6 +27,27 @@ + + + + + + + + + + + + diff --git a/src/RedisCluster.php b/src/RedisCluster.php index cdee934..a88c189 100644 --- a/src/RedisCluster.php +++ b/src/RedisCluster.php @@ -176,7 +176,6 @@ protected function internalGetItem(&$normalizedKey, &$success = null, &$casToken return null; } - /** @psalm-suppress MixedAssignment */ $value = $casToken = $values[$normalizedKey]; $success = true; return $value; @@ -202,7 +201,6 @@ protected function internalGetItems(array &$normalizedKeys): array } $result = []; - /** @psalm-suppress MixedAssignment */ foreach ($resultsByIndex as $keyIndex => $value) { $normalizedKey = $normalizedKeys[$keyIndex]; $namespacedKey = $namespacedKeys[$keyIndex]; @@ -210,7 +208,6 @@ protected function internalGetItems(array &$normalizedKeys): array continue; } - /** @psalm-suppress MixedAssignment */ $result[$normalizedKey] = $value; } @@ -299,7 +296,6 @@ protected function internalSetItems(array &$normalizedKeyValuePairs): array $ttl = (int) $this->getOptions()->getTtl(); $namespacedKeyValuePairs = []; - /** @psalm-suppress MixedAssignment */ foreach ($normalizedKeyValuePairs as $normalizedKey => $value) { $namespacedKeyValuePairs[$this->createNamespacedKey((string) $normalizedKey)] = $value; } @@ -307,7 +303,6 @@ protected function internalSetItems(array &$normalizedKeyValuePairs): array $successByKey = []; try { - /** @psalm-suppress MixedAssignment */ foreach ($namespacedKeyValuePairs as $key => $value) { if ($ttl) { /** @@ -443,7 +438,6 @@ private function clusterException( */ private function isFalseReturnValuePersisted(RedisClusterFromExtension $redis, string $key): bool { - /** @psalm-suppress MixedAssignment */ $serializer = $this->getLibOption(RedisClusterFromExtension::OPT_SERIALIZER); if ($serializer === RedisClusterFromExtension::SERIALIZER_NONE) { return false; diff --git a/src/RedisClusterResourceManager.php b/src/RedisClusterResourceManager.php index 7732a67..f5c2e2e 100644 --- a/src/RedisClusterResourceManager.php +++ b/src/RedisClusterResourceManager.php @@ -132,12 +132,10 @@ private function applyLibraryOptions( RedisClusterFromExtension $resource, array $options ): RedisClusterFromExtension { - /** @psalm-suppress MixedAssignment */ foreach ($options as $option => $value) { /** * @see https://github.com/phpredis/phpredis#setoption * - * @psalm-suppress InvalidArgument * @psalm-suppress MixedArgument */ $resource->setOption($option, $value); @@ -157,11 +155,6 @@ private function mergeLibraryOptionsFromCluster(array $options, RedisClusterFrom continue; } - /** - * @see https://github.com/phpredis/phpredis#getoption - * - * @psalm-suppress InvalidArgument - */ $options[$option] = $resource->getOption($option); } @@ -178,11 +171,6 @@ public function getLibOption(int $option) return $this->libraryOptions[$option]; } - /** - * @see https://github.com/phpredis/phpredis#getoption - * - * @psalm-suppress InvalidArgument - */ return $this->libraryOptions[$option] = $this->getResource()->getOption($option); } diff --git a/test/integration/Psr/CacheItemPool/RedisClusterWithPhpIgbinaryTest.php b/test/integration/Psr/CacheItemPool/RedisClusterWithPhpIgbinaryTest.php index 67a9ceb..fa18713 100644 --- a/test/integration/Psr/CacheItemPool/RedisClusterWithPhpIgbinaryTest.php +++ b/test/integration/Psr/CacheItemPool/RedisClusterWithPhpIgbinaryTest.php @@ -18,7 +18,6 @@ final class RedisClusterWithPhpIgbinaryTest extends AbstractCacheItemPoolIntegra protected function setUp(): void { parent::setUp(); - /** @psalm-suppress MixedArrayAssignment */ $this->skippedTests['testHasItemReturnsFalseWhenDeferredItemIsExpired'] = sprintf( '%s storage doesn\'t support driver deferred', RedisCluster::class diff --git a/test/integration/Psr/CacheItemPool/RedisClusterWithPhpSerializeTest.php b/test/integration/Psr/CacheItemPool/RedisClusterWithPhpSerializeTest.php index 184707c..a886af0 100644 --- a/test/integration/Psr/CacheItemPool/RedisClusterWithPhpSerializeTest.php +++ b/test/integration/Psr/CacheItemPool/RedisClusterWithPhpSerializeTest.php @@ -18,7 +18,6 @@ final class RedisClusterWithPhpSerializeTest extends AbstractCacheItemPoolIntegr protected function setUp(): void { parent::setUp(); - /** @psalm-suppress MixedArrayAssignment */ $this->skippedTests['testHasItemReturnsFalseWhenDeferredItemIsExpired'] = sprintf( '%s storage doesn\'t support driver deferred', RedisCluster::class diff --git a/test/integration/Psr/CacheItemPool/RedisClusterWithoutSerializerTest.php b/test/integration/Psr/CacheItemPool/RedisClusterWithoutSerializerTest.php index 151901b..06516d2 100644 --- a/test/integration/Psr/CacheItemPool/RedisClusterWithoutSerializerTest.php +++ b/test/integration/Psr/CacheItemPool/RedisClusterWithoutSerializerTest.php @@ -18,7 +18,6 @@ final class RedisClusterWithoutSerializerTest extends AbstractCacheItemPoolInteg protected function setUp(): void { parent::setUp(); - /** @psalm-suppress MixedArrayAssignment */ $this->skippedTests['testHasItemReturnsFalseWhenDeferredItemIsExpired'] = sprintf( '%s storage doesn\'t support driver deferred', RedisCluster::class diff --git a/test/integration/Psr/CacheItemPool/RedisIntegrationTest.php b/test/integration/Psr/CacheItemPool/RedisIntegrationTest.php index ea03f22..67c2e2f 100644 --- a/test/integration/Psr/CacheItemPool/RedisIntegrationTest.php +++ b/test/integration/Psr/CacheItemPool/RedisIntegrationTest.php @@ -15,7 +15,6 @@ class RedisIntegrationTest extends AbstractCacheItemPoolIntegrationTest protected function setUp(): void { - /** @psalm-suppress MixedArrayAssignment */ $this->skippedTests['testHasItemReturnsFalseWhenDeferredItemIsExpired'] = 'Cache decorator does not support deferred deletion'; diff --git a/test/unit/RedisClusterOptionsTest.php b/test/unit/RedisClusterOptionsTest.php index 307d7cf..70c0713 100644 --- a/test/unit/RedisClusterOptionsTest.php +++ b/test/unit/RedisClusterOptionsTest.php @@ -117,7 +117,6 @@ public function testOptionConstantsMatchingExtensionImplementation(string $const )); } - /** @psalm-suppress MixedAssignment */ $constantValueInOptions = constant($constantInOptions); self::assertIsInt($constantValueInOptions); self::assertEquals( From 7e9fa52f3ea4c0b948086e74ec9929afd3fb09e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Fri, 16 Dec 2022 23:20:20 +0100 Subject: [PATCH 2/4] qa: implement failing tests regarding #22 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In issue #22 the user `michaltyszczenko` reported that not every redis method which throws a `RedisException` is properly handled. With these unit tests we can emulate this problem and implement a proper fix. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- test/unit/RedisResourceManagerTest.php | 147 +++++++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/test/unit/RedisResourceManagerTest.php b/test/unit/RedisResourceManagerTest.php index 7b5cbf2..47187cc 100644 --- a/test/unit/RedisResourceManagerTest.php +++ b/test/unit/RedisResourceManagerTest.php @@ -4,8 +4,11 @@ namespace LaminasTest\Cache\Storage\Adapter; +use Laminas\Cache\Storage\Adapter\Exception\RedisRuntimeException; use Laminas\Cache\Storage\Adapter\RedisResourceManager; use PHPUnit\Framework\TestCase; +use Redis; +use RedisException; use function getenv; @@ -162,4 +165,148 @@ public function testGetMajorVersion(): void $this->assertGreaterThan(0, $this->resourceManager->getMajorVersion($resourceId)); } + + public function testWillCatchConnectExceptions(): void + { + $redis = $this->createMock(Redis::class); + $redis + ->expects(self::atLeastOnce()) + ->method('connect') + ->willThrowException(new RedisException('test')); + + $this->resourceManager->setResource('default', ['resource' => $redis, 'server' => 'localhost:6379']); + + $this->expectException(RedisRuntimeException::class); + $this->expectExceptionMessage('test'); + $this->resourceManager->getResource('default'); + } + + public function testWillCatchPConnectExceptions(): void + { + $redis = $this->createMock(Redis::class); + $redis + ->expects(self::atLeastOnce()) + ->method('pconnect') + ->willThrowException(new RedisException('test')); + + $this->expectException(RedisRuntimeException::class); + $this->expectExceptionMessage('test'); + $this->resourceManager->setResource( + 'default', + [ + 'resource' => $redis, + 'server' => 'localhost:6379', + 'persistent_id' => 'test', + ] + ); + $this->resourceManager->getResource('default'); + } + + public function testWillCatchAuthExceptions(): void + { + $redis = $this->createMock(Redis::class); + $redis + ->method('connect') + ->willReturn(true); + + $redis + ->method('info') + ->willReturn(['redis_version' => '1.2.3']); + + $redis + ->expects(self::atLeastOnce()) + ->method('auth') + ->with('foobar') + ->willThrowException(new RedisException('test')); + + $this->resourceManager->setResource( + 'default', + [ + 'resource' => $redis, + 'server' => 'whatever:6379', + 'password' => 'foobar', + ] + ); + $this->expectException(RedisRuntimeException::class); + $this->expectExceptionMessage('test'); + $this->resourceManager->getResource('default'); + } + + public function testWillCatchInfoExceptions(): void + { + $redis = $this->createMock(Redis::class); + $redis + ->method('connect') + ->willReturn(true); + + $redis + ->expects(self::atLeastOnce()) + ->method('info') + ->willThrowException(new RedisException('test')); + + $this->resourceManager->setResource( + 'default', + [ + 'resource' => $redis, + 'initialized' => true, + 'server' => 'somewhere:6379', + ] + ); + + $this->expectException(RedisRuntimeException::class); + $this->expectExceptionMessage('test'); + $this->resourceManager->getResource('default'); + } + + public function testWillCatchAuthDuringConnectException(): void + { + $redis = $this->createMock(Redis::class); + + $redis + ->method('connect') + ->willReturn(true); + + $redis + ->expects(self::atLeastOnce()) + ->method('auth') + ->with('secret') + ->willThrowException(new RedisException('test')); + + $this->resourceManager->setResource( + 'default', + [ + 'resource' => $redis, + 'initialized' => false, + 'server' => 'somewhere:6379', + 'password' => 'secret', + ] + ); + + $this->expectException(RedisRuntimeException::class); + $this->expectExceptionMessage('test'); + $this->resourceManager->getResource('default'); + } + + public function testWillCatchSelectDatabaseException(): void + { + $redis = $this->createMock(Redis::class); + + $redis + ->expects(self::atLeastOnce()) + ->method('select') + ->willThrowException(new RedisException('test')); + + $this->resourceManager->setResource( + 'default', + [ + 'resource' => $redis, + 'initialized' => true, + 'server' => 'somewhere:6379', + ] + ); + + $this->expectException(RedisRuntimeException::class); + $this->expectExceptionMessage('test'); + $this->resourceManager->setDatabase('default', 0); + } } From 1dde4700543d12affed2a05bab15684b17f4992c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Fri, 16 Dec 2022 23:20:41 +0100 Subject: [PATCH 3/4] bugfix: catch unhandled `RedisException` in `RedisResourceManager` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- src/Exception/RedisRuntimeException.php | 13 +++ src/RedisResourceManager.php | 117 ++++++++++++++++-------- 2 files changed, 93 insertions(+), 37 deletions(-) diff --git a/src/Exception/RedisRuntimeException.php b/src/Exception/RedisRuntimeException.php index 3f7301c..9471a23 100644 --- a/src/Exception/RedisRuntimeException.php +++ b/src/Exception/RedisRuntimeException.php @@ -5,8 +5,10 @@ namespace Laminas\Cache\Storage\Adapter\Exception; use Laminas\Cache\Exception\RuntimeException as LaminasCacheRuntimeException; +use Redis; use RedisCluster; use RedisClusterException; +use RedisException; use Throwable; final class RedisRuntimeException extends LaminasCacheRuntimeException @@ -26,4 +28,15 @@ public static function fromFailedConnection(Throwable $exception): self $exception ); } + + public static function fromRedisException(RedisException $exception, Redis $redis): self + { + try { + $message = $redis->getLastError() ?? $exception->getMessage(); + } catch (RedisException $exceptionThrownByGetLastErrorMethod) { + $message = $exception->getMessage(); + } + + return new self($message, (int) $exception->getCode(), $exception); + } } diff --git a/src/RedisResourceManager.php b/src/RedisResourceManager.php index e271dce..50fe8e5 100644 --- a/src/RedisResourceManager.php +++ b/src/RedisResourceManager.php @@ -5,12 +5,15 @@ namespace Laminas\Cache\Storage\Adapter; use Laminas\Cache\Exception; +use Laminas\Cache\Storage\Adapter\Exception\RedisRuntimeException; use Laminas\Stdlib\ArrayUtils; use Redis as RedisResource; +use RedisException as RedisResourceException; use ReflectionClass; use Traversable; use function array_merge; +use function assert; use function constant; use function defined; use function is_array; @@ -140,8 +143,11 @@ public function getResource($id) } if (! $resource['version']) { - $info = $resource['resource']->info(); + $redis = $resource['resource']; + assert($redis instanceof RedisResource); + $info = $this->getRedisInfo($redis); $resource['version'] = $info['redis_version']; + unset($info); } return $resource['resource']; @@ -158,8 +164,9 @@ public function getResource($id) $redis->setOption($k, $v); } - $info = $redis->info(); - $resource['version'] = $info['redis_version']; + $info = $this->getRedisInfo($redis); + $resource['version'] = $info['redis_version']; + unset($info); $this->resources[$id]['resource'] = $redis; return $redis; } @@ -283,32 +290,38 @@ protected function connect(array &$resource) { $server = $resource['server']; $redis = $resource['resource']; - if ($resource['persistent_id'] !== '') { - //connect or reuse persistent connection - $success = $redis->pconnect( - $server['host'], - $server['port'], - $server['timeout'], - $resource['persistent_id'] - ); - } elseif ($server['port']) { - $success = $redis->connect($server['host'], $server['port'], $server['timeout']); - } elseif ($server['timeout']) { - //connect through unix domain socket - $success = $redis->connect($server['host'], $server['timeout']); - } else { - $success = $redis->connect($server['host']); - } + assert($redis instanceof RedisResource); + + try { + if (($resource['persistent_id'] ?? '') !== '') { + //connect or reuse persistent connection + $success = $redis->pconnect( + $server['host'], + $server['port'], + $server['timeout'], + $resource['persistent_id'] + ); + } elseif ($server['port']) { + $success = $redis->connect($server['host'], $server['port'], $server['timeout']); + } elseif ($server['timeout']) { + //connect through unix domain socket + $success = $redis->connect($server['host'], $server['timeout']); + } else { + $success = $redis->connect($server['host']); + } - if (! $success) { - throw new Exception\RuntimeException('Could not establish connection with Redis instance'); - } + if (! $success) { + throw new Exception\RuntimeException('Could not establish connection with Redis instance'); + } - $resource['initialized'] = true; - if ($resource['password']) { - $redis->auth($resource['password']); + $resource['initialized'] = true; + if ($resource['password']) { + $redis->auth($resource['password']); + } + $redis->select($resource['database']); + } catch (RedisResourceException $exception) { + throw RedisRuntimeException::fromRedisException($exception, $redis); } - $redis->select($resource['database']); } /** @@ -462,6 +475,7 @@ public function setLibOptions($id, array $libOptions) $this->normalizeLibOptions($libOptions); $redis = &$resource['resource']; + assert($redis instanceof RedisResource); if (method_exists($redis, 'setOptions')) { $redis->setOptions($libOptions); @@ -545,6 +559,7 @@ public function getLibOption($id, $key) * * @param array|Traversable $libOptions * @throws Exception\InvalidArgumentException + * @param-out array $libOptions */ protected function normalizeLibOptions(&$libOptions) { @@ -568,19 +583,20 @@ protected function normalizeLibOptions(&$libOptions) * * @param string|int $key * @throws Exception\InvalidArgumentException + * @param-out int $key */ protected function normalizeLibOptionKey(&$key) { - // convert option name into it's constant value - if (is_string($key)) { - $const = 'Redis::OPT_' . str_replace([' ', '-'], '_', strtoupper($key)); - if (! defined($const)) { - throw new Exception\InvalidArgumentException("Unknown redis option '{$key}' ({$const})"); - } - $key = constant($const); - } else { - $key = (int) $key; + if (! is_string($key)) { + return; + } + + $const = 'Redis::OPT_' . str_replace([' ', '-'], '_', strtoupper($key)); + if (! defined($const)) { + throw new Exception\InvalidArgumentException("Unknown redis option '{$key}' ({$const})"); } + /** @var int $key */ + $key = constant($const); } /** @@ -662,12 +678,39 @@ public function setDatabase($id, $database) } $resource = &$this->resources[$id]; - if ($resource['resource'] instanceof RedisResource && $resource['initialized']) { - $resource['resource']->select($database); + $redis = $resource['resource']; + if ($redis instanceof RedisResource && $resource['initialized']) { + try { + $redis->select($database); + } catch (RedisResourceException $exception) { + throw RedisRuntimeException::fromRedisException($exception, $redis); + } } $resource['database'] = $database; return $this; } + + /** + * @return array{redis_version:string} + */ + private function getRedisInfo(RedisResource $redis): array + { + try { + $info = $redis->info(); + } catch (RedisResourceException $exception) { + throw RedisRuntimeException::fromRedisException($exception, $redis); + } + + if (! is_array($info)) { + return ['redis_version' => '0.0.0-unknown']; + } + + if (! isset($info['redis_version']) || ! is_string($info['redis_version'])) { + return ['redis_version' => '0.0.0-unknown']; + } + + return $info; + } } From b0e4eea1187c465840f2b3b08a9b76759e78858e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= <2189546+boesing@users.noreply.github.com> Date: Fri, 16 Dec 2022 23:39:14 +0100 Subject: [PATCH 4/4] qa: update baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This seems to add new array keys which were not reported before. Maybe that is related to the `psalm.xml` changes but these reported LoC were only re-aligned and therefore no code has changed. Signed-off-by: Maximilian Bösing <2189546+boesing@users.noreply.github.com> --- psalm-baseline.xml | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/psalm-baseline.xml b/psalm-baseline.xml index 4c693c3..eeba31a 100644 --- a/psalm-baseline.xml +++ b/psalm-baseline.xml @@ -76,10 +76,6 @@ Redis - - (string) $namespace - (string) $prefix - $info @@ -100,11 +96,6 @@ RedisOptions - - (string) $namespace - (string) $namespaceSeparator - (string) $resourceId - @@ -126,16 +117,26 @@ normalizePersistentId normalizeServer - + $k $key - $redis $resource $resource + $resource['database'] $resource['lib_options'] $resource['persistent_id'] + $resource['persistent_id'] $resource['server'] $server + $server['host'] + $server['host'] + $server['host'] + $server['host'] + $server['port'] + $server['port'] + $server['timeout'] + $server['timeout'] + $server['timeout'] $resource['database'] @@ -200,15 +201,9 @@ string string|null - - auth - connect - connect - connect + getOption getOption - pconnect - select $resource['database'] @@ -226,11 +221,6 @@ (int) $server['port'] - - (int) $database - (string) $id - (string) $persistentId -