Use same strategy for both client-side sharding and redis-cluster.

This change is possible because, after a few changes in redis-cluster,
our default cluster strategy used for client side sharding and the one
used for redis-cluster turned out to be exactly the same, except for
the hashing function used to calculate distribution.

Actually some checks used to enforce correctness are redundant in the
context of redis-cluster (e.g. the one used to make sure that keys in
requests performing cross-keys operations will hash to the same slot,
which is performed by the server) so we could also add a more dumb and
permissive cluster strategy that relies on checks performed by Redis.

Differently to v0.8, the strategy for client-side sharding now uses
the same rules for extracting hash tags from keys especially when
empty tags are found in the string.

src/Cluster/PredisStrategy.php

@@ -24,8 +24,8 @@ use Predis\Command\ScriptCommand;
  */
 class PredisStrategy implements StrategyInterface
 {
-    private $commands;
-    private $hashGenerator;
+    protected $commands;
+    protected $hashGenerator;
 
     /**
      * @param HashGeneratorInterface $hashGenerator Hash generator instance.

src/Cluster/RedisStrategy.php

@@ -21,305 +21,13 @@ use Predis\Command\ScriptCommand;
  *
  * @author Daniele Alessandri <suppakilla@gmail.com>
  */
-class RedisStrategy implements StrategyInterface
+class RedisStrategy extends PredisStrategy
 {
-    private $commands;
-    private $hashGenerator;
-
-    /**
-     *
-     */
-    public function __construct()
-    {
-        $this->commands = $this->getDefaultCommands();
-        $this->hashGenerator = new Hash\CRC16();
-    }
-
-    /**
-     * Returns the default map of supported commands with their handlers.
-     *
-     * @return array
-     */
-    protected function getDefaultCommands()
-    {
-        $getKeyFromFirstArgument = array($this, 'getKeyFromFirstArgument');
-        $getKeyFromAllArguments = array($this, 'getKeyFromAllArguments');
-
-        return array(
-            /* commands operating on the key space */
-            'EXISTS'                => $getKeyFromFirstArgument,
-            'DEL'                   => $getKeyFromAllArguments,
-            'TYPE'                  => $getKeyFromFirstArgument,
-            'EXPIRE'                => $getKeyFromFirstArgument,
-            'EXPIREAT'              => $getKeyFromFirstArgument,
-            'PERSIST'               => $getKeyFromFirstArgument,
-            'PEXPIRE'               => $getKeyFromFirstArgument,
-            'PEXPIREAT'             => $getKeyFromFirstArgument,
-            'TTL'                   => $getKeyFromFirstArgument,
-            'PTTL'                  => $getKeyFromFirstArgument,
-            'SORT'                  => $getKeyFromFirstArgument, // TODO
-
-            /* commands operating on string values */
-            'APPEND'                => $getKeyFromFirstArgument,
-            'DECR'                  => $getKeyFromFirstArgument,
-            'DECRBY'                => $getKeyFromFirstArgument,
-            'GET'                   => $getKeyFromFirstArgument,
-            'GETBIT'                => $getKeyFromFirstArgument,
-            'MGET'                  => $getKeyFromAllArguments,
-            'SET'                   => $getKeyFromFirstArgument,
-            'GETRANGE'              => $getKeyFromFirstArgument,
-            'GETSET'                => $getKeyFromFirstArgument,
-            'INCR'                  => $getKeyFromFirstArgument,
-            'INCRBY'                => $getKeyFromFirstArgument,
-            'INCRBYFLOAT'           => $getKeyFromFirstArgument,
-            'SETBIT'                => $getKeyFromFirstArgument,
-            'SETEX'                 => $getKeyFromFirstArgument,
-            'MSET'                  => array($this, 'getKeyFromInterleavedArguments'),
-            'MSETNX'                => array($this, 'getKeyFromInterleavedArguments'),
-            'SETNX'                 => $getKeyFromFirstArgument,
-            'SETRANGE'              => $getKeyFromFirstArgument,
-            'STRLEN'                => $getKeyFromFirstArgument,
-            'SUBSTR'                => $getKeyFromFirstArgument,
-            'BITCOUNT'              => $getKeyFromFirstArgument,
-
-            /* commands operating on lists */
-            'LINSERT'               => $getKeyFromFirstArgument,
-            'LINDEX'                => $getKeyFromFirstArgument,
-            'LLEN'                  => $getKeyFromFirstArgument,
-            'LPOP'                  => $getKeyFromFirstArgument,
-            'RPOP'                  => $getKeyFromFirstArgument,
-            'BLPOP'                 => array($this, 'getKeyFromBlockingListCommands'),
-            'BRPOP'                 => array($this, 'getKeyFromBlockingListCommands'),
-            'LPUSH'                 => $getKeyFromFirstArgument,
-            'LPUSHX'                => $getKeyFromFirstArgument,
-            'RPUSH'                 => $getKeyFromFirstArgument,
-            'RPUSHX'                => $getKeyFromFirstArgument,
-            'LRANGE'                => $getKeyFromFirstArgument,
-            'LREM'                  => $getKeyFromFirstArgument,
-            'LSET'                  => $getKeyFromFirstArgument,
-            'LTRIM'                 => $getKeyFromFirstArgument,
-
-            /* commands operating on sets */
-            'SADD'                  => $getKeyFromFirstArgument,
-            'SCARD'                 => $getKeyFromFirstArgument,
-            'SISMEMBER'             => $getKeyFromFirstArgument,
-            'SMEMBERS'              => $getKeyFromFirstArgument,
-            'SSCAN'                 => $getKeyFromFirstArgument,
-            'SPOP'                  => $getKeyFromFirstArgument,
-            'SRANDMEMBER'           => $getKeyFromFirstArgument,
-            'SREM'                  => $getKeyFromFirstArgument,
-
-            /* commands operating on sorted sets */
-            'ZADD'                  => $getKeyFromFirstArgument,
-            'ZCARD'                 => $getKeyFromFirstArgument,
-            'ZCOUNT'                => $getKeyFromFirstArgument,
-            'ZINCRBY'               => $getKeyFromFirstArgument,
-            'ZRANGE'                => $getKeyFromFirstArgument,
-            'ZRANGEBYSCORE'         => $getKeyFromFirstArgument,
-            'ZRANK'                 => $getKeyFromFirstArgument,
-            'ZREM'                  => $getKeyFromFirstArgument,
-            'ZREMRANGEBYRANK'       => $getKeyFromFirstArgument,
-            'ZREMRANGEBYSCORE'      => $getKeyFromFirstArgument,
-            'ZREVRANGE'             => $getKeyFromFirstArgument,
-            'ZREVRANGEBYSCORE'      => $getKeyFromFirstArgument,
-            'ZREVRANK'              => $getKeyFromFirstArgument,
-            'ZSCORE'                => $getKeyFromFirstArgument,
-            'ZSCAN'                 => $getKeyFromFirstArgument,
-            'ZLEXCOUNT'             => $getKeyFromFirstArgument,
-            'ZRANGEBYLEX'           => $getKeyFromFirstArgument,
-            'ZREMRANGEBYLEX'        => $getKeyFromFirstArgument,
-
-            /* commands operating on hashes */
-            'HDEL'                  => $getKeyFromFirstArgument,
-            'HEXISTS'               => $getKeyFromFirstArgument,
-            'HGET'                  => $getKeyFromFirstArgument,
-            'HGETALL'               => $getKeyFromFirstArgument,
-            'HMGET'                 => $getKeyFromFirstArgument,
-            'HMSET'                 => $getKeyFromFirstArgument,
-            'HINCRBY'               => $getKeyFromFirstArgument,
-            'HINCRBYFLOAT'          => $getKeyFromFirstArgument,
-            'HKEYS'                 => $getKeyFromFirstArgument,
-            'HLEN'                  => $getKeyFromFirstArgument,
-            'HSET'                  => $getKeyFromFirstArgument,
-            'HSETNX'                => $getKeyFromFirstArgument,
-            'HVALS'                 => $getKeyFromFirstArgument,
-            'HSCAN'                 => $getKeyFromFirstArgument,
-
-            /* commands operating on HyperLogLog */
-            'PFADD'                 => $getKeyFromFirstArgument,
-            'PFCOUNT'               => $getKeyFromAllArguments,
-            'PFMERGE'               => $getKeyFromAllArguments,
-
-            /* scripting */
-            'EVAL'                  => array($this, 'getKeyFromScriptingCommands'),
-            'EVALSHA'               => array($this, 'getKeyFromScriptingCommands'),
-        );
-    }
-
-    /**
-     * Returns the list of IDs for the supported commands.
-     *
-     * @return array
-     */
-    public function getSupportedCommands()
-    {
-        return array_keys($this->commands);
-    }
-
-    /**
-     * Sets an handler for the specified command ID.
-     *
-     * The signature of the callback must have a single parameter of type
-     * Predis\Command\CommandInterface.
-     *
-     * When the callback argument is omitted or NULL, the previously associated
-     * handler for the specified command ID is removed.
-     *
-     * @param string $commandID Command ID.
-     * @param mixed  $callback  A valid callable object, or NULL to unset the handler.
-     */
-    public function setCommandHandler($commandID, $callback = null)
-    {
-        $commandID = strtoupper($commandID);
-
-        if (!isset($callback)) {
-            unset($this->commands[$commandID]);
-
-            return;
-        }
-
-        if (!is_callable($callback)) {
-            throw new InvalidArgumentException(
-                "The argument must be a callable object or NULL."
-            );
-        }
-
-        $this->commands[$commandID] = $callback;
-    }
-
-    /**
-     * Extracts the key from the first argument of a command instance.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromFirstArgument(CommandInterface $command)
-    {
-        return $command->getArgument(0);
-    }
-
-    /**
-     * Extracts the key from a command that can accept multiple keys ensuring
-     * that only one key is actually specified to comply with redis-cluster.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromAllArguments(CommandInterface $command)
-    {
-        $arguments = $command->getArguments();
-
-        if (count($arguments) === 1) {
-            return $arguments[0];
-        }
-    }
-
-    /**
-     * Extracts the key from a command that can accept multiple keys ensuring
-     * that only one key is actually specified to comply with redis-cluster.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromInterleavedArguments(CommandInterface $command)
-    {
-        $arguments = $command->getArguments();
-
-        if (count($arguments) === 2) {
-            return $arguments[0];
-        }
-    }
-
-    /**
-     * Extracts the key from BLPOP and BRPOP commands ensuring that only one key
-     * is actually specified to comply with redis-cluster.
-     *
-     * @param  CommandInterface $command Command instance.
-     * @return string
-     */
-    protected function getKeyFromBlockingListCommands(CommandInterface $command)
-    {
-        $arguments = $command->getArguments();
-
-        if (count($arguments) === 2) {
-            return $arguments[0];
-        }
-    }
-
     /**
-     * Extracts the key from EVAL and EVALSHA commands.
      *
-     * @param  CommandInterface $command Command instance.
-     * @return string
      */
-    protected function getKeyFromScriptingCommands(CommandInterface $command)
+    public function __construct(HashGeneratorInterface $hashGenerator = null)
     {
-        if ($command instanceof ScriptCommand) {
-            $keys = $command->getKeys();
-        } else {
-            $keys = array_slice($args = $command->getArguments(), 2, $args[1]);
-        }
-
-        if (count($keys) === 1) {
-            return $keys[0];
-        }
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getHash(CommandInterface $command)
-    {
-        $hash = $command->getHash();
-
-        if (!isset($hash) && isset($this->commands[$cmdID = $command->getId()])) {
-            $key = call_user_func($this->commands[$cmdID], $command);
-
-            if (isset($key)) {
-                $hash = $this->hashGenerator->hash($key);
-                $command->setHash($hash);
-            }
-        }
-
-        return $hash;
-    }
-
-    /**
-     * {@inheritdoc}
-     */
-    public function getKeyHash($key)
-    {
-        $key = $this->extractKeyTag($key);
-        $hash = $this->hashGenerator->hash($key);
-
-        return $hash;
-    }
-
-    /**
-     * Returns only the hashable part of a key (delimited by "{...}"), or the
-     * whole key if a key tag is not found in the string.
-     *
-     * @param  string $key A key.
-     * @return string
-     */
-    protected function extractKeyTag($key)
-    {
-        if (false !== $start = strpos($key, '{')) {
-            if (false !== ($end = strpos($key, '}', $start)) && $end !== ++$start) {
-                $key = substr($key, $start, $end - $start);
-            }
-        }
-
-        return $key;
+        parent::__construct($hashGenerator ?: new Hash\CRC16());
     }
 }

tests/Predis/Cluster/RedisStrategyTest.php

@@ -275,6 +275,8 @@ class RedisStrategyTest extends PredisTestCase
             'TTL'                   => 'keys-first',
             'PTTL'                  => 'keys-first',
             'SORT'                  => 'keys-first', // TODO
+            'DUMP'                  => 'keys-first',
+            'RESTORE'               => 'keys-first',
 
             /* commands operating on string values */
             'APPEND'                => 'keys-first',
@@ -297,6 +299,7 @@ class RedisStrategyTest extends PredisTestCase
             'SETRANGE'              => 'keys-first',
             'STRLEN'                => 'keys-first',
             'SUBSTR'                => 'keys-first',
+            'BITOP'                 => 'keys-bitop',
             'BITCOUNT'              => 'keys-first',
 
             /* commands operating on lists */
@@ -305,8 +308,10 @@ class RedisStrategyTest extends PredisTestCase
             'LLEN'                  => 'keys-first',
             'LPOP'                  => 'keys-first',
             'RPOP'                  => 'keys-first',
+            'RPOPLPUSH'             => 'keys-all',
             'BLPOP'                 => 'keys-blockinglist',
             'BRPOP'                 => 'keys-blockinglist',
+            'BRPOPLPUSH'            => 'keys-blockinglist',
             'LPUSH'                 => 'keys-first',
             'LPUSHX'                => 'keys-first',
             'RPUSH'                 => 'keys-first',
@@ -319,6 +324,12 @@ class RedisStrategyTest extends PredisTestCase
             /* commands operating on sets */
             'SADD'                  => 'keys-first',
             'SCARD'                 => 'keys-first',
+            'SDIFF'                 => 'keys-all',
+            'SDIFFSTORE'            => 'keys-all',
+            'SINTER'                => 'keys-all',
+            'SINTERSTORE'           => 'keys-all',
+            'SUNION'                => 'keys-all',
+            'SUNIONSTORE'           => 'keys-all',
             'SISMEMBER'             => 'keys-first',
             'SMEMBERS'              => 'keys-first',
             'SSCAN'                 => 'keys-first',
@@ -331,6 +342,7 @@ class RedisStrategyTest extends PredisTestCase
             'ZCARD'                 => 'keys-first',
             'ZCOUNT'                => 'keys-first',
             'ZINCRBY'               => 'keys-first',
+            'ZINTERSTORE'           => 'keys-zaggregated',
             'ZRANGE'                => 'keys-first',
             'ZRANGEBYSCORE'         => 'keys-first',
             'ZRANK'                 => 'keys-first',
@@ -341,6 +353,7 @@ class RedisStrategyTest extends PredisTestCase
             'ZREVRANGEBYSCORE'      => 'keys-first',
             'ZREVRANK'              => 'keys-first',
             'ZSCORE'                => 'keys-first',
+            'ZUNIONSTORE'           => 'keys-zaggregated',
             'ZSCAN'                 => 'keys-first',
             'ZLEXCOUNT'             => 'keys-first',
             'ZRANGEBYLEX'           => 'keys-first',

tests/Predis/Connection/Aggregate/PredisClusterTest.php

@@ -269,6 +269,31 @@ class PredisClusterTest extends PredisTestCase
         $cluster->getConnection($ping);
     }
 
+    /**
+     * @group disconnected
+     */
+    public function testSupportsKeyHashTags()
+    {
+        $profile = Profile\Factory::getDefault();
+
+        $connection1 = $this->getMockConnection('tcp://127.0.0.1:6379');
+        $connection2 = $this->getMockConnection('tcp://127.0.0.1:6380');
+
+        $cluster = new RedisCluster();
+        $cluster->add($connection1);
+        $cluster->add($connection2);
+
+        $set = $profile->createCommand('set', array('{node:1001}:foo', 'foobar'));
+        $get = $profile->createCommand('get', array('{node:1001}:foo'));
+        $this->assertSame($connection1, $cluster->getConnection($set));
+        $this->assertSame($connection1, $cluster->getConnection($get));
+
+        $set = $profile->createCommand('set', array('{node:1001}:bar', 'foobar'));
+        $get = $profile->createCommand('get', array('{node:1001}:bar'));
+        $this->assertSame($connection1, $cluster->getConnection($set));
+        $this->assertSame($connection1, $cluster->getConnection($get));
+    }
+
     /**
      * @group disconnected
      */

tests/Predis/Connection/Aggregate/RedisClusterTest.php

@@ -428,7 +428,7 @@ class RedisClusterTest extends PredisTestCase
     /**
      * @group disconnected
      */
-    public function testDoesNotSupportKeyTags()
+    public function testSupportsKeyHashTags()
     {
         $profile = Profile\Factory::getDefault();
 
@@ -446,8 +446,8 @@ class RedisClusterTest extends PredisTestCase
 
         $set = $profile->createCommand('set', array('{node:1001}:bar', 'foobar'));
         $get = $profile->createCommand('get', array('{node:1001}:bar'));
-        $this->assertSame($connection2, $cluster->getConnection($set));
-        $this->assertSame($connection2, $cluster->getConnection($get));
+        $this->assertSame($connection1, $cluster->getConnection($set));
+        $this->assertSame($connection1, $cluster->getConnection($get));
     }
 
     /**