From 9df9dbdedcf3076cb3da3bbdb7e89819ffac54a5 Mon Sep 17 00:00:00 2001 From: Daniele Alessandri Date: Tue, 21 Aug 2012 16:21:07 +0200 Subject: [PATCH] Promote multibulk response iterators as Predis response objects. Multibulk response iterators will not be passed anymore to the response parser method of the command that generated the response. Pipeline and transaction abstractions still consume interators returned as response items. --- CHANGELOG.md | 5 +++ lib/Predis/Command/HashGetAll.php | 6 ---- lib/Predis/Command/ServerConfig.php | 6 ---- lib/Predis/Command/ServerSlowlog.php | 16 ++-------- lib/Predis/Command/ServerTime.php | 8 ----- lib/Predis/Command/ZSetRange.php | 6 ---- lib/Predis/Iterator/MultiBulkResponse.php | 4 ++- .../Iterator/MultiBulkResponseSimple.php | 10 ++++++ .../Iterator/MultiBulkResponseTuple.php | 18 +++++++++++ .../Iterator/MultiBulkResponseSimpleTest.php | 12 +++++++ .../Iterator/MultiBulkResponseTupleTest.php | 31 +++++++++++++++++-- 11 files changed, 79 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 085c8f8a..395d6e59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,11 @@ v0.8.0 (201x-xx-xx) `-NOSCRIPT` error. Automatic fallback to `EVAL` does not work with pipelines, inside a MULTI / EXEC context or with plain `EVALSHA` commands. +- Iterators for multi-bulk replies now skip the response parsing method of the + command that generated the response and are passed directly to user code. + Pipeline and transaction objects still consume automatically the iterators + that are present in response elements. + - Cluster and replication connections now extend a new common interface, `Predis\Connection\AggregatedConnectionInterface`. diff --git a/lib/Predis/Command/HashGetAll.php b/lib/Predis/Command/HashGetAll.php index 9fa7f7a1..51a745f7 100644 --- a/lib/Predis/Command/HashGetAll.php +++ b/lib/Predis/Command/HashGetAll.php @@ -11,8 +11,6 @@ namespace Predis\Command; -use Predis\Iterator\MultiBulkResponseTuple; - /** * @link http://redis.io/commands/hgetall * @author Daniele Alessandri @@ -32,10 +30,6 @@ class HashGetAll extends PrefixableCommand */ public function parseResponse($data) { - if ($data instanceof \Iterator) { - return new MultiBulkResponseTuple($data); - } - $result = array(); for ($i = 0; $i < count($data); $i++) { diff --git a/lib/Predis/Command/ServerConfig.php b/lib/Predis/Command/ServerConfig.php index 77a20a37..251efdd8 100644 --- a/lib/Predis/Command/ServerConfig.php +++ b/lib/Predis/Command/ServerConfig.php @@ -11,8 +11,6 @@ namespace Predis\Command; -use Predis\Iterator\MultiBulkResponseTuple; - /** * @link http://redis.io/commands/config-set * @link http://redis.io/commands/config-get @@ -34,10 +32,6 @@ class ServerConfig extends AbstractCommand */ public function parseResponse($data) { - if ($data instanceof \Iterator) { - return new MultiBulkResponseTuple($data); - } - if (is_array($data)) { $result = array(); diff --git a/lib/Predis/Command/ServerSlowlog.php b/lib/Predis/Command/ServerSlowlog.php index 2520cc92..ad4155f2 100644 --- a/lib/Predis/Command/ServerSlowlog.php +++ b/lib/Predis/Command/ServerSlowlog.php @@ -11,8 +11,6 @@ namespace Predis\Command; -use Predis\Iterator\MultiBulkResponse; - /** * @link http://redis.io/commands/slowlog * @author Daniele Alessandri @@ -32,28 +30,18 @@ class ServerSlowlog extends AbstractCommand */ public function parseResponse($data) { - if (($iterable = $data instanceof \Iterator) || is_array($data)) { - // NOTE: we consume iterable multibulk replies inplace since it is not - // possible to do anything fancy on sub-elements. + if (is_array($data)) { $log = array(); foreach ($data as $index => $entry) { - if ($iterable) { - $entry = iterator_to_array($entry); - } - $log[$index] = array( 'id' => $entry[0], 'timestamp' => $entry[1], 'duration' => $entry[2], - 'command' => $iterable ? iterator_to_array($entry[3]) : $entry[3], + 'command' => $entry[3], ); } - if ($iterable === true) { - unset($data); - } - return $log; } diff --git a/lib/Predis/Command/ServerTime.php b/lib/Predis/Command/ServerTime.php index 924f91ba..542179a0 100644 --- a/lib/Predis/Command/ServerTime.php +++ b/lib/Predis/Command/ServerTime.php @@ -32,12 +32,4 @@ class ServerTime extends AbstractCommand { return false; } - - /** - * {@inheritdoc} - */ - public function parseResponse($data) - { - return $data instanceof \Iterator ? iterator_to_array($data) : $data; - } } diff --git a/lib/Predis/Command/ZSetRange.php b/lib/Predis/Command/ZSetRange.php index d2a4f79f..61a7bfcc 100644 --- a/lib/Predis/Command/ZSetRange.php +++ b/lib/Predis/Command/ZSetRange.php @@ -11,8 +11,6 @@ namespace Predis\Command; -use Predis\Iterator\MultiBulkResponseTuple; - /** * @link http://redis.io/commands/zrange * @author Daniele Alessandri @@ -90,10 +88,6 @@ class ZSetRange extends PrefixableCommand public function parseResponse($data) { if ($this->withScores()) { - if ($data instanceof \Iterator) { - return new MultiBulkResponseTuple($data); - } - $result = array(); for ($i = 0; $i < count($data); $i++) { diff --git a/lib/Predis/Iterator/MultiBulkResponse.php b/lib/Predis/Iterator/MultiBulkResponse.php index 138794d4..a7bbad81 100644 --- a/lib/Predis/Iterator/MultiBulkResponse.php +++ b/lib/Predis/Iterator/MultiBulkResponse.php @@ -11,13 +11,15 @@ namespace Predis\Iterator; +use Predis\ResponseObjectInterface; + /** * Iterator that abstracts the access to multibulk replies and allows * them to be consumed by user's code in a streaming fashion. * * @author Daniele Alessandri */ -abstract class MultiBulkResponse implements \Iterator, \Countable +abstract class MultiBulkResponse implements \Iterator, \Countable, ResponseObjectInterface { protected $position; protected $current; diff --git a/lib/Predis/Iterator/MultiBulkResponseSimple.php b/lib/Predis/Iterator/MultiBulkResponseSimple.php index b3448376..68864b18 100644 --- a/lib/Predis/Iterator/MultiBulkResponseSimple.php +++ b/lib/Predis/Iterator/MultiBulkResponseSimple.php @@ -75,4 +75,14 @@ class MultiBulkResponseSimple extends MultiBulkResponse { return $this->connection->read(); } + + /** + * Returns an iterator that reads the multi-bulk response as a tuple. + * + * @return MultiBulkResponseTuple + */ + public function asTuple() + { + return new MultiBulkResponseTuple($this); + } } diff --git a/lib/Predis/Iterator/MultiBulkResponseTuple.php b/lib/Predis/Iterator/MultiBulkResponseTuple.php index 0c014846..39d7828f 100644 --- a/lib/Predis/Iterator/MultiBulkResponseTuple.php +++ b/lib/Predis/Iterator/MultiBulkResponseTuple.php @@ -26,6 +26,8 @@ class MultiBulkResponseTuple extends MultiBulkResponse implements \OuterIterator */ public function __construct(MultiBulkResponseSimple $iterator) { + $this->checkPreconditions($iterator); + $virtualSize = count($iterator) / 2; $this->iterator = $iterator; $this->position = $iterator->getPosition(); @@ -33,6 +35,22 @@ class MultiBulkResponseTuple extends MultiBulkResponse implements \OuterIterator $this->replySize = $virtualSize; } + /** + * Checks for valid preconditions. + * + * @param MultiBulkResponseSimple $iterator Multibulk reply iterator. + */ + protected function checkPreconditions(MultiBulkResponseSimple $iterator) + { + if ($iterator->getPosition() !== 0) { + throw new \RuntimeException('Cannot initialize a tuple iterator with an already initiated iterator'); + } + + if (($size = count($iterator)) % 2 !== 0) { + throw new \UnexpectedValueException("Invalid reply size for a tuple iterator [$size]"); + } + } + /** * {@inheritdoc} */ diff --git a/tests/Predis/Iterator/MultiBulkResponseSimpleTest.php b/tests/Predis/Iterator/MultiBulkResponseSimpleTest.php index 9c4a5fa9..3701fe80 100644 --- a/tests/Predis/Iterator/MultiBulkResponseSimpleTest.php +++ b/tests/Predis/Iterator/MultiBulkResponseSimpleTest.php @@ -48,6 +48,18 @@ class MultiBulkResponseSimpleTest extends StandardTestCase $this->assertTrue($client->ping()); } + /** + * @group connected + */ + public function testIterableMultibulkCanBeWrappedAsTupleIterator() + { + $client = $this->getClient(); + $client->mset('foo', 'bar', 'hoge', 'piyo'); + + $this->assertInstanceOf('Predis\Iterator\MultiBulkResponseSimple', $iterator = $client->mget('foo', 'bar')); + $this->assertInstanceOf('Predis\Iterator\MultiBulkResponseTuple', $iterator->asTuple()); + } + /** * @group connected */ diff --git a/tests/Predis/Iterator/MultiBulkResponseTupleTest.php b/tests/Predis/Iterator/MultiBulkResponseTupleTest.php index 96659542..cb15392c 100644 --- a/tests/Predis/Iterator/MultiBulkResponseTupleTest.php +++ b/tests/Predis/Iterator/MultiBulkResponseTupleTest.php @@ -20,6 +20,33 @@ use Predis\Client; */ class MultiBulkResponseTupleTest extends StandardTestCase { + /** + * @group disconnected + * @expectedException RuntimeException + * @expectedExceptionMessage Cannot initialize a tuple iterator with an already initiated iterator + */ + public function testInitiatedMultiBulkIteratorsAreNotValid() + { + $connection = $this->getMock('Predis\Connection\SingleConnectionInterface'); + $iterator = new MultiBulkResponseSimple($connection, 2); + $iterator->next(); + + new MultiBulkResponseTuple($iterator); + } + + /** + * @group disconnected + * @expectedException UnexpectedValueException + * @expectedExceptionMessage Invalid reply size for a tuple iterator [3] + */ + public function testMultiBulkWithOddSizesAreInvalid() + { + $connection = $this->getMock('Predis\Connection\SingleConnectionInterface'); + $iterator = new MultiBulkResponseSimple($connection, 3); + + new MultiBulkResponseTuple($iterator); + } + /** * @group connected */ @@ -28,7 +55,7 @@ class MultiBulkResponseTupleTest extends StandardTestCase $client = $this->getClient(); $client->zadd('metavars', 1, 'foo', 2, 'hoge', 3, 'lol'); - $this->assertInstanceOf('OuterIterator', $iterator = $client->zrange('metavars', 0, -1, 'withscores')); + $this->assertInstanceOf('OuterIterator', $iterator = $client->zrange('metavars', 0, -1, 'withscores')->asTuple()); $this->assertInstanceOf('Predis\Iterator\MultiBulkResponseTuple', $iterator); $this->assertInstanceOf('Predis\Iterator\MultiBulkResponseSimple', $iterator->getInnerIterator()); $this->assertTrue($iterator->valid()); @@ -57,7 +84,7 @@ class MultiBulkResponseTupleTest extends StandardTestCase $client = $this->getClient(); $client->zadd('metavars', 1, 'foo', 2, 'hoge', 3, 'lol'); - $iterator = $client->zrange('metavars', 0, -1, 'withscores'); + $iterator = $client->zrange('metavars', 0, -1, 'withscores')->asTuple(); unset($iterator);