Make Relay work with Redis cluster (#1397)

.coveralls.yml

@@ -0,0 +1 @@
+coverage_clover: build/logs/clover-*.xml

.gitattributes

@@ -5,6 +5,7 @@
 /examples export-ignore
 /tests export-ignore
 /.codespellrc export-ignore linguist-language=INI
+/.coveralls.yml export-ignore
 /.editorconfig export-ignore
 /.gitattributes export-ignore
 /.gitignore export-ignore

.github/workflows/linters.yml

@@ -137,7 +137,7 @@ jobs:
           EXPECTED="LICENSE,README.md,autoload.php,composer.json"
           CURRENT="$(
               git archive HEAD \
-                  | tar --list --exclude="src" --exclude="src/*" --exclude="bin" --exclude="bin/*" --exclude="docker" --exclude="docker/*" \
+                  | tar --list --exclude="src" --exclude="src/*" --exclude="bin" --exclude="bin/*" \
                   | paste --serial --delimiters=","
           )"
           echo "CURRENT =${CURRENT}"

.github/workflows/tests.yml

@@ -66,7 +66,7 @@ jobs:
 
       - name: Run tests with coverage
         if: ${{ matrix.php == '8.1' && matrix.redis == '7' }}
-        run: vendor/bin/phpunit --coverage-php build/cov/coverage-predis.cov --coverage-filter ./src
+        run: vendor/bin/phpunit --coverage-clover build/logs/clover-default.xml --coverage-filter ./src
 
       - name: Run tests using Relay
         if: ${{ matrix.redis >= '6' && (matrix.php != '8.1' || matrix.redis != '7')}}
@@ -74,11 +74,7 @@ jobs:
 
       - name: Run tests using Relay with coverage
         if: ${{ matrix.php == '8.1' && matrix.redis == '7' }}
-        run: vendor/bin/phpunit -c phpunit.relay.xml --coverage-php build/cov/coverage-relay.cov --coverage-filter ./src
-
-      - name: Merge coverage reports
-        if: ${{ matrix.php == '8.1' && matrix.redis == '7' }}
-        run: php vendor/bin/phpcov merge --clover build/logs/clover.xml build/cov
+        run: vendor/bin/phpunit -c phpunit.relay.xml --coverage-clover build/logs/clover-relay.xml --coverage-filter ./src
 
       - name: Send coverage to Coveralls
         uses: coverallsapp/github-action@v2
@@ -107,12 +103,12 @@ jobs:
       - name: Checkout repository
         uses: actions/checkout@v3
 
-      - name: Run redis cluster
+      - name: Run Redis cluster
         uses: isbang/compose-action@v1.4.1
         with:
-          compose-file: "./docker/unstable_cluster/docker-compose.yml"
+          compose-file: .github/workflows/cluster/docker-compose.yml
 
-      - name: Setup PHP with Composer and extensions
+      - name: Setup PHP
         uses: shivammathur/setup-php@v2
         with:
           php-version: ${{ matrix.php }}
@@ -127,8 +123,13 @@ jobs:
 
       - name: Run tests against cluster
         run: |
-          sleep 5 # Timeout to make sure that docker image is setup
-          vendor/bin/phpunit --group cluster,gears-cluster
+          sleep 5 # make sure that docker image is setup
+          vendor/bin/phpunit --group cluster
+
+      - name: Run tests against cluster using Relay
+        run: |
+          sleep 5 # make sure nodes are stable and fully joined
+          vendor/bin/phpunit -c phpunit.relay.xml --group cluster
 
   predis-stack:
 

src/Connection/Cluster/RedisCluster.php

@@ -489,6 +489,14 @@ class RedisCluster extends AbstractAggregateConnection implements ClusterInterfa
     {
         [$slot, $connectionID] = explode(' ', $details, 2);
 
+        // Handle connection ID in the form of "IP:port (details about exception)"
+        // by trimming everything after first space (including the space)
+        $startPositionOfExtraDetails = strpos($connectionID, ' ');
+
+        if ($startPositionOfExtraDetails !== false) {
+            $connectionID = substr($connectionID, 0, $startPositionOfExtraDetails);
+        }
+
         if (!$connection = $this->getConnectionById($connectionID)) {
             $connection = $this->createConnection($connectionID);
         }

src/Connection/RelayConnection.php

@@ -158,6 +158,18 @@ class RelayConnection extends AbstractConnection
         return $this->client;
     }
 
+    /**
+     * {@inheritdoc}
+     */
+    public function getIdentifier()
+    {
+        try {
+            return $this->client->endpointId();
+        } catch (RelayException $ex) {
+            return parent::getIdentifier();
+        }
+    }
+
     /**
      * {@inheritdoc}
      */
@@ -177,7 +189,13 @@ class RelayConnection extends AbstractConnection
                 ? $this->client->{$name}(...$command->getArguments())
                 : $this->client->rawCommand($name, ...$command->getArguments());
         } catch (RelayException $ex) {
-            throw $this->onCommandError($ex, $command);
+            $exception = $this->onCommandError($ex, $command);
+
+            if ($exception instanceof ErrorResponseInterface) {
+                return $exception;
+            }
+
+            throw $exception;
         }
     }
 
@@ -189,15 +207,15 @@ class RelayConnection extends AbstractConnection
         $code = $exception->getCode();
         $message = $exception->getMessage();
 
-        if (strpos($message, 'RELAY_ERR_IO')) {
+        if (strpos($message, 'RELAY_ERR_IO') !== false) {
             return new ConnectionException($this, $message, $code, $exception);
         }
 
-        if (strpos($message, 'RELAY_ERR_REDIS')) {
+        if (strpos($message, 'RELAY_ERR_REDIS') !== false) {
             return new ServerException($message, $code, $exception);
         }
 
-        if (strpos($message, 'RELAY_ERR_WRONGTYPE') && strpos($message, "Got reply-type 'status'")) {
+        if (strpos($message, 'RELAY_ERR_WRONGTYPE') !== false && strpos($message, "Got reply-type 'status'") !== false) {
             $message = 'Operation against a key holding the wrong kind of value';
         }
 

tests/Predis/Command/Redis/GET_Test.php

@@ -98,6 +98,16 @@ class GET_Test extends PredisCommandTestCase
         $this->assertEquals('bar', $redis->get('foo'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testReturnsStringValueUsingCluster(): void
+    {
+        $this->testReturnsStringValue();
+    }
+
     /**
      * @group connected
      */
@@ -111,6 +121,16 @@ class GET_Test extends PredisCommandTestCase
         $this->assertSame('', $redis->get('foo'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testReturnsEmptyStringOnEmptyStringsUsingCluster(): void
+    {
+        $this->testReturnsEmptyStringOnEmptyStrings();
+    }
+
     /**
      * @group connected
      */
@@ -122,6 +142,16 @@ class GET_Test extends PredisCommandTestCase
         $this->assertNull($redis->get('foo'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testReturnsNullOnNonExistingKeysUsingCluster(): void
+    {
+        $this->testReturnsNullOnNonExistingKeys();
+    }
+
     /**
      * @group connected
      */
@@ -135,4 +165,14 @@ class GET_Test extends PredisCommandTestCase
         $redis->rpush('metavars', 'foo');
         $redis->get('metavars');
     }
+
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testThrowsExceptionOnWrongTypeUsingCluster(): void
+    {
+        $this->testThrowsExceptionOnWrongType();
+    }
 }

tests/Predis/Command/Redis/HGETALL_Test.php

@@ -108,6 +108,20 @@ class HGETALL_Test extends PredisCommandTestCase
         $this->assertSame([], $redis->hgetall('unknown'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testReturnsAllTheFieldsAndTheirValuesUsingCluster(): void
+    {
+        $redis = $this->getClient();
+
+        $redis->del('metavars');
+
+        $this->testReturnsAllTheFieldsAndTheirValues();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.0.0
@@ -115,11 +129,21 @@ class HGETALL_Test extends PredisCommandTestCase
     public function testThrowsExceptionOnWrongType(): void
     {
         $this->expectException('Predis\Response\ServerException');
-        $this->expectExceptionMessage('Operation against a key holding the wrong kind of value');
+        $this->expectExceptionMessageMatches('/.*Operation against a key holding the wrong kind of value.*/');
 
         $redis = $this->getClient();
 
         $redis->set('foo', 'bar');
         $redis->hgetall('foo');
     }
+
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testThrowsExceptionOnWrongTypeUsingCluster(): void
+    {
+        $this->testThrowsExceptionOnWrongType();
+    }
 }

tests/Predis/Command/Redis/HGET_Test.php

@@ -105,6 +105,20 @@ class HGET_Test extends PredisCommandTestCase
         $this->assertNull($redis->hget('unknown', 'foo'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testReturnsValueOfSpecifiedFieldUsingCluster(): void
+    {
+        $redis = $this->getClient();
+
+        $redis->del('metavars');
+
+        $this->testReturnsValueOfSpecifiedField();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.0.0
@@ -112,11 +126,21 @@ class HGET_Test extends PredisCommandTestCase
     public function testThrowsExceptionOnWrongType(): void
     {
         $this->expectException('Predis\Response\ServerException');
-        $this->expectExceptionMessage('Operation against a key holding the wrong kind of value');
+        $this->expectExceptionMessageMatches('/.*Operation against a key holding the wrong kind of value.*/');
 
         $redis = $this->getClient();
 
         $redis->set('foo', 'bar');
         $redis->hget('foo', 'bar');
     }
+
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testThrowsExceptionOnWrongTypeUsingCluster(): void
+    {
+        $this->testThrowsExceptionOnWrongType();
+    }
 }

tests/Predis/Command/Redis/HSET_Test.php

@@ -106,6 +106,20 @@ class HSET_Test extends PredisCommandTestCase
         $this->assertSame(['bar', 'piyo'], $redis->hmget('metavars', 'foo', 'hoge'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testSetsValueOfSpecifiedFieldUsingCluster(): void
+    {
+        $redis = $this->getClient();
+
+        $redis->del('metavars');
+
+        $this->testSetsValueOfSpecifiedField();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.0.0
@@ -113,11 +127,21 @@ class HSET_Test extends PredisCommandTestCase
     public function testThrowsExceptionOnWrongType(): void
     {
         $this->expectException('Predis\Response\ServerException');
-        $this->expectExceptionMessage('Operation against a key holding the wrong kind of value');
+        $this->expectExceptionMessageMatches('/.*Operation against a key holding the wrong kind of value.*/');
 
         $redis = $this->getClient();
 
         $redis->set('metavars', 'foo');
         $redis->hset('metavars', 'foo', 'bar');
     }
+
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testThrowsExceptionOnWrongTypeUsingCluster(): void
+    {
+        $this->testThrowsExceptionOnWrongType();
+    }
 }

tests/Predis/Command/Redis/SET_Test.php

@@ -114,6 +114,16 @@ class SET_Test extends PredisCommandTestCase
         $this->assertSame('bar', $redis->get('foo'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testSetStringValueUsingCluster(): void
+    {
+        $this->testSetStringValue();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.6.12
@@ -126,6 +136,16 @@ class SET_Test extends PredisCommandTestCase
         $this->assertSame(1, $redis->ttl('foo'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testSetStringValueWithModifierEXUsingCluster(): void
+    {
+        $this->testSetStringValueWithModifierEX();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.6.12
@@ -141,6 +161,16 @@ class SET_Test extends PredisCommandTestCase
         $this->assertLessThanOrEqual(1000, $pttl);
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testSetStringValueWithModifierPXUsingCluster(): void
+    {
+        $this->testSetStringValueWithModifierPX();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.6.12
@@ -153,6 +183,16 @@ class SET_Test extends PredisCommandTestCase
         $this->assertNull($redis->set('foo', 'bar', 'NX'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testSetStringValueWithModifierNXUsingCluster(): void
+    {
+        $this->testSetStringValueWithModifierNX();
+    }
+
     /**
      * @group connected
      * @requiresRedisVersion >= 2.6.12
@@ -167,11 +207,20 @@ class SET_Test extends PredisCommandTestCase
         $this->assertNull($redis->set('foofoo', 'barbar', 'XX'));
     }
 
+    /**
+     * @group connected
+     * @group cluster
+     * @requiresRedisVersion >= 6.0.0
+     */
+    public function testSetStringValueWithModifierXXUsingCluster(): void
+    {
+        $this->testSetStringValueWithModifierXX();
+    }
+
     /**
      * @group connected
      * @group cluster
      * @requiresRedisVersion >= 3.0.0
-     * @return void
      */
     public function testSetStringValueInClusterMode(): void
     {

tests/Predis/Connection/Cluster/RedisClusterTest.php

@@ -12,6 +12,7 @@
 
 namespace Predis\Connection\Cluster;
 
+use Iterator;
 use PHPUnit\Framework\MockObject\MockObject;
 use Predis\Cluster;
 use Predis\Command;
@@ -1208,11 +1209,12 @@ class RedisClusterTest extends PredisTestCase
 
     /**
      * @group disconnected
+     * @dataProvider onMovedResponsesDataProvider
      */
-    public function testAskSlotMapToRedisClusterOnMovedResponseByDefault(): void
+    public function testAskSlotMapToRedisClusterOnMovedResponseByDefault(string $movedErrorMessage): void
     {
         $cmdGET = Command\RawCommand::create('GET', 'node:1001');
-        $rspMOVED = new Response\Error('MOVED 1970 127.0.0.1:6380');
+        $rspMOVED = new Response\Error($movedErrorMessage);
         $rspSlotsArray = [
             [0,  8191, ['127.0.0.1', 6379]],
             [8192, 16383, ['127.0.0.1', 6380]],
@@ -1257,6 +1259,20 @@ class RedisClusterTest extends PredisTestCase
         $this->assertCount(2, $cluster);
     }
 
+    /**
+     * @return Iterator<string, array{movedErrorMessage: string}>
+     */
+    public function onMovedResponsesDataProvider(): Iterator
+    {
+        yield 'MOVED 1970 127.0.0.1:6380' => [
+            'movedErrorMessage' => 'MOVED 1970 127.0.0.1:6380',
+        ];
+
+        yield 'MOVED 1970 127.0.0.1:6380 (relay exception details)' => [
+            'movedErrorMessage' => 'MOVED 1970 127.0.0.1:6380 (relay exception details)',
+        ];
+    }
+
     /**
      * @group disconnected
      */

tests/Predis/Connection/RelayConnectionTest.php

@@ -12,11 +12,16 @@
 
 namespace Predis\Connection;
 
+use PHPUnit\Framework\MockObject\MockObject;
+use Predis\ClientException;
 use Predis\Command\RawCommand;
 use Predis\NotSupportedException;
 use Predis\Response\Error as ErrorResponse;
 use PredisTestCase;
 use Relay\Relay;
+use Predis\Response\ErrorInterface as ErrorResponseInterface;
+use ReflectionClass;
+use Relay\Exception as RelayException;
 
 /**
  * @group ext-relay
@@ -281,6 +286,170 @@ class RelayConnectionTest extends PredisTestCase
         return RelayConnection::class;
     }
 
+    /**
+     * @group disconnected
+     */
+    public function testThrowsExceptionOnInitializationCommandFailure(): void
+    {
+        $this->expectException('Predis\Connection\ConnectionException');
+        $this->expectExceptionMessage('`SELECT` failed: ERR invalid DB index [tcp://127.0.0.1:6379]');
+
+        $cmdSelect = RawCommand::create('SELECT', '1000');
+
+        /** @var NodeConnectionInterface|MockObject */
+        $connection = $this
+            ->getMockBuilder($this->getConnectionClass())
+            ->onlyMethods(['executeCommand', 'createResource'])
+            ->setConstructorArgs([new Parameters()])
+            ->getMock();
+        $connection
+            ->method('executeCommand')
+            ->with($cmdSelect)
+            ->willReturn(
+                new ErrorResponse('ERR invalid DB index')
+            );
+
+        $connection->method('createResource');
+
+        $connection->addConnectCommand($cmdSelect);
+        $connection->connect();
+    }
+
+    /**
+     * @group connected
+     */
+    public function testGetIdentifierUsesParentGetIdentifier(): void
+    {
+        $relayMock = $this
+            ->getMockBuilder(Relay::class)
+            ->onlyMethods(['endpointId'])
+            ->getMock();
+
+        $relayMock->method('endpointId')
+            ->willThrowException(
+                new RelayException('Not Connected')
+            );
+
+        /** @var RelayConnection&MockObject $connection */
+        $connection = $this
+            ->getMockBuilder($this->getConnectionClass())
+            ->onlyMethods(['createResource'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $reflection = new ReflectionClass($connection);
+        $propertyClient = $reflection->getProperty('client');
+        $propertyClient->setAccessible(true);
+        $propertyClient->setValue($connection, $relayMock);
+        $propertyParameters = $reflection->getProperty('parameters');
+        $propertyParameters->setAccessible(true);
+        $propertyParameters->setValue($connection, new Parameters([
+            'host' => '127.0.0.1',
+            'port' => 6379,
+        ]));
+
+        $this->assertEquals('127.0.0.1:6379', $connection->getIdentifier());
+    }
+
+    /**
+     * @group connected
+     */
+    public function testGetIdentifierUsesClientEndpointId(): void
+    {
+        $relayMock = $this
+            ->getMockBuilder(Relay::class)
+            ->onlyMethods(['endpointId'])
+            ->getMock();
+
+        $relayMock->method('endpointId')
+            ->willReturn('127.0.0.1:6379');
+
+        /** @var RelayConnection&MockObject $connection */
+        $connection = $this
+            ->getMockBuilder($this->getConnectionClass())
+            ->onlyMethods(['createResource'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $reflection = new ReflectionClass($connection);
+        $propertyClient = $reflection->getProperty('client');
+        $propertyClient->setAccessible(true);
+        $propertyClient->setValue($connection, $relayMock);
+
+        $this->assertEquals('127.0.0.1:6379', $connection->getIdentifier());
+    }
+
+    /**
+     * @group connected
+     */
+    public function testExecuteCommandReturnsErrorResponseWhenItIsThrownByRelay(): void
+    {
+        $cmdSelect = RawCommand::create('GET', '1');
+
+        $relayMock = $this
+            ->getMockBuilder(Relay::class)
+            ->onlyMethods(['rawCommand'])
+            ->getMock();
+
+        $relayMock->method('rawCommand')
+            ->willThrowException(
+                new RelayException('RELAY_ERR_REDIS')
+            );
+
+        /** @var RelayConnection&MockObject $connection */
+        $connection = $this
+            ->getMockBuilder($this->getConnectionClass())
+            ->onlyMethods(['createResource', 'createClient'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $reflection = new ReflectionClass($connection);
+        $property = $reflection->getProperty('client');
+        $property->setAccessible(true);
+        $property->setValue($connection, $relayMock);
+
+        $connection->method('createResource');
+
+        $response = $connection->executeCommand($cmdSelect);
+
+        $this->assertInstanceOf(ErrorResponseInterface::class, $response);
+    }
+
+    /**
+     * @group connected
+     */
+    public function testExecuteCommandThrowsExceptionWhenThrownByRelayAndItIsNotErrorResponse(): void
+    {
+        $this->expectException('Predis\ClientException');
+        $cmdSelect = RawCommand::create('GET', '1');
+
+        $relayMock = $this
+            ->getMockBuilder(Relay::class)
+            ->onlyMethods(['rawCommand'])
+            ->getMock();
+
+        $relayMock->method('rawCommand')
+            ->willThrowException(
+                new ClientException('RELAY_ERR_REDIS')
+            );
+
+        /** @var RelayConnection&MockObject $connection */
+        $connection = $this
+            ->getMockBuilder($this->getConnectionClass())
+            ->onlyMethods(['createResource', 'createClient'])
+            ->disableOriginalConstructor()
+            ->getMock();
+
+        $reflection = new ReflectionClass($connection);
+        $property = $reflection->getProperty('client');
+        $property->setAccessible(true);
+        $property->setValue($connection, $relayMock);
+
+        $connection->method('createResource');
+
+        $connection->executeCommand($cmdSelect);
+    }
+
     // ******************************************************************** //
     // ---- INTEGRATION TESTS --------------------------------------------- //
     // ******************************************************************** //