Skip to content

PHPLIB-81: Implement CachingIterator #424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Oct 27, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
{ "name": "Derick Rethans", "email": "[email protected]" }
],
"require": {
"php": ">=5.4",
"php": ">=5.5",
"ext-hash": "*",
"ext-json": "*",
"ext-mongodb": "^1.3.0"
Expand Down
166 changes: 166 additions & 0 deletions src/Model/CachingIterator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
<?php
/*
* Copyright 2017 MongoDB, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

namespace MongoDB\Model;

use Countable;
use Generator;
use Iterator;
use Traversable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do this if if you're not aliasing them, or plucking them out of another namespace? And if you want the root namespace, shouldn't it be use \Countable ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use statements use fully qualified names, so the leading backslash isn't necessary. Out of habit, I list all used classes at the top of the file, rather than making a special case of \Countable for classes in the global namespace.


/**
* Iterator for wrapping a Traversable and caching its results.
*
* By caching results, this iterators allows a Traversable to be counted and
* rewound multiple times, even if the wrapped object does not natively support
* those operations (e.g. MongoDB\Driver\Cursor).
*
* @internal
*/
class CachingIterator implements Countable, Iterator
{
private $items = [];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helps if we count an empty Traversable, as the early return in storeCurrentItem() would have prevented this from ever being initialized from null to an array type.

private $iterator;
private $iteratorAdvanced = false;
private $iteratorExhausted = false;

/**
* Constructor.
*
* Initialize the iterator and stores the first item in the cache. This
* effectively rewinds the Traversable and the wrapping Generator, which
* will execute up to its first yield statement. Additionally, this mimics
* behavior of the SPL iterators and allows users to omit an explicit call
* to rewind() before using the other methods.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alcaeus: Strictly speaking, Iterators must call rewind() before iterating; however, most SPL iterators relax that restriction and rewind during construction. I kept that behavior, while adding $iteratorAdvanced to ensure that the initial call to rewind() does not exhaust all results. That may be why @bjori originally assumed you had implemented a BufferedIterator. Based on testPartialIterationDoesNotExhaust(), the original implementation was actually buffering up front!

*
* @param Traversable $traversable
*/
public function __construct(Traversable $traversable)
{
$this->iterator = $this->wrapTraversable($traversable);
$this->storeCurrentItem();
}

/**
* @see http://php.net/countable.count
* @return integer
*/
public function count()
{
$this->exhaustIterator();

return count($this->items);
}

/**
* @see http://php.net/iterator.current
* @return mixed
*/
public function current()
{
return current($this->items);
}

/**
* @see http://php.net/iterator.mixed
* @return mixed
*/
public function key()
{
return key($this->items);
}

/**
* @see http://php.net/iterator.next
* @return void
*/
public function next()
{
if ( ! $this->iteratorExhausted) {
$this->iterator->next();
$this->storeCurrentItem();
}

next($this->items);
}

/**
* @see http://php.net/iterator.rewind
* @return void
*/
public function rewind()
{
/* If the iterator has advanced, exhaust it now so that future iteration
* can rely on the cache.
*/
if ($this->iteratorAdvanced) {
$this->exhaustIterator();
}

reset($this->items);
}

/**
*
* @see http://php.net/iterator.valid
* @return boolean
*/
public function valid()
{
return $this->key() !== null;
}

/**
* Ensures that the inner iterator is fully consumed and cached.
*/
private function exhaustIterator()
{
while ( ! $this->iteratorExhausted) {
$this->next();
}
}

/**
* Stores the current item in the cache.
*/
private function storeCurrentItem()
{
$key = $this->iterator->key();

if ($key === null) {
return;
}

$this->items[$key] = $this->iterator->current();
}

/**
* Wraps the Traversable with a Generator.
*
* @param Traversable $traversable
* @return Generator
*/
private function wrapTraversable(Traversable $traversable)
{
foreach ($traversable as $key => $value) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to recap, each foreach starts with invoking the internal iterator's rewind handler, followed by a validity check. If valid, keys and values are fetched. On subsequent iterations, a call to the next handler takes place instead of rewind.

yield $key => $value;
$this->iteratorAdvanced = true;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding (and observation) is that the first call to wrapTraversable() from our constructor is going to immediately execute this function up to (and including?) the yield statement. The $iteratorAdvanced assignment will take place once we re-enter, immediately before foreach advances the Traversable (e.g. Iterator::next() if it happens to be an Iterator).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct - this could be tested by adding an assertion on the iteratorAdvanced property on the testConstructorRewinds test:

$this->assertAttributeSame(false, 'iteratorAdvanced', $iterator);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize those assertions existed :)

I think I'll do without them here, since we can get by testing with just the public API. Will keep it in mind for future use, though!

}

$this->iteratorExhausted = true;
}
}
5 changes: 3 additions & 2 deletions src/Operation/ListCollections.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use MongoDB\Driver\Server;
use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException;
use MongoDB\Exception\InvalidArgumentException;
use MongoDB\Model\CachingIterator;
use MongoDB\Model\CollectionInfoCommandIterator;
use MongoDB\Model\CollectionInfoIterator;
use MongoDB\Model\CollectionInfoLegacyIterator;
Expand Down Expand Up @@ -107,7 +108,7 @@ private function executeCommand(Server $server)
$cursor = $server->executeCommand($this->databaseName, new Command($cmd));
$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);

return new CollectionInfoCommandIterator($cursor);
return new CollectionInfoCommandIterator(new CachingIterator($cursor));
}

/**
Expand Down Expand Up @@ -138,6 +139,6 @@ private function executeLegacy(Server $server)
$cursor = $server->executeQuery($this->databaseName . '.system.namespaces', new Query($filter, $options));
$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);

return new CollectionInfoLegacyIterator($cursor);
return new CollectionInfoLegacyIterator(new CachingIterator($cursor));
}
}
5 changes: 3 additions & 2 deletions src/Operation/ListIndexes.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use MongoDB\Driver\Server;
use MongoDB\Driver\Exception\RuntimeException as DriverRuntimeException;
use MongoDB\Exception\InvalidArgumentException;
use MongoDB\Model\CachingIterator;
use MongoDB\Model\IndexInfoIterator;
use MongoDB\Model\IndexInfoIteratorIterator;
use EmptyIterator;
Expand Down Expand Up @@ -114,7 +115,7 @@ private function executeCommand(Server $server)

$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);

return new IndexInfoIteratorIterator($cursor);
return new IndexInfoIteratorIterator(new CachingIterator($cursor));
}

/**
Expand All @@ -136,6 +137,6 @@ private function executeLegacy(Server $server)
$cursor = $server->executeQuery($this->databaseName . '.system.indexes', new Query($filter, $options));
$cursor->setTypeMap(['root' => 'array', 'document' => 'array']);

return new IndexInfoIteratorIterator($cursor);
return new IndexInfoIteratorIterator(new CachingIterator($cursor));
}
}
130 changes: 130 additions & 0 deletions tests/Model/CachingIteratorTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
<?php

namespace MongoDB\Tests\Model;

use MongoDB\Model\CachingIterator;
use Exception;

class CachingIteratorTest extends \PHPUnit_Framework_TestCase
{
/**
* Sanity check for all following tests.
*
* @expectedException \Exception
* @expectedExceptionMessage Cannot traverse an already closed generator
*/
public function testTraversingGeneratorConsumesIt()
{
$iterator = $this->getTraversable([1, 2, 3]);
$this->assertSame([1, 2, 3], iterator_to_array($iterator));
$this->assertSame([1, 2, 3], iterator_to_array($iterator));
}

public function testConstructorRewinds()
{
$iterator = new CachingIterator($this->getTraversable([1, 2, 3]));

$this->assertTrue($iterator->valid());
$this->assertSame(0, $iterator->key());
$this->assertSame(1, $iterator->current());
}

public function testIteration()
{
$iterator = new CachingIterator($this->getTraversable([1, 2, 3]));

$expectedKey = 0;
$expectedItem = 1;

foreach ($iterator as $key => $item) {
$this->assertSame($expectedKey++, $key);
$this->assertSame($expectedItem++, $item);
}

$this->assertFalse($iterator->valid());
}

public function testIterationWithEmptySet()
{
$iterator = new CachingIterator($this->getTraversable([]));

$iterator->rewind();
$this->assertFalse($iterator->valid());
}

public function testPartialIterationDoesNotExhaust()
{
$traversable = $this->getTraversableThatThrows([1, 2, new Exception]);
$iterator = new CachingIterator($traversable);

$expectedKey = 0;
$expectedItem = 1;

foreach ($iterator as $key => $item) {
$this->assertSame($expectedKey++, $key);
$this->assertSame($expectedItem++, $item);

if ($key === 1) {
break;
}
}

$this->assertTrue($iterator->valid());
}

public function testRewindAfterPartialIteration()
{
$iterator = new CachingIterator($this->getTraversable([1, 2, 3]));

$iterator->rewind();
$this->assertTrue($iterator->valid());
$this->assertSame(0, $iterator->key());
$this->assertSame(1, $iterator->current());

$iterator->next();
$this->assertSame([1, 2, 3], iterator_to_array($iterator));
}

public function testCount()
{
$iterator = new CachingIterator($this->getTraversable([1, 2, 3]));
$this->assertCount(3, $iterator);
}

public function testCountAfterPartialIteration()
{
$iterator = new CachingIterator($this->getTraversable([1, 2, 3]));

$iterator->rewind();
$this->assertTrue($iterator->valid());
$this->assertSame(0, $iterator->key());
$this->assertSame(1, $iterator->current());

$iterator->next();
$this->assertCount(3, $iterator);
}

public function testCountWithEmptySet()
{
$iterator = new CachingIterator($this->getTraversable([]));
$this->assertCount(0, $iterator);
}

private function getTraversable($items)
{
foreach ($items as $item) {
yield $item;
}
}

private function getTraversableThatThrows($items)
{
foreach ($items as $item) {
if ($item instanceof Exception) {
throw $item;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stack trace of the exception only reports when it was constructed (it's not generated/updated when the exception is thrown). I was surprised to learn that, but it's not too important as we only ever assert that nothing is thrown (testing that initial rewind no longer exhausts the internal traversable).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I tried fixing that in Xdebug, but adding the previous exception to a special property.

} else {
yield $item;
}
}
}
}
5 changes: 3 additions & 2 deletions tests/Operation/ListCollectionsFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,9 @@ public function testListCollectionsForNewlyCreatedDatabase()
$this->assertEquals(1, $writeResult->getInsertedCount());

$operation = new ListCollections($this->getDatabaseName(), ['filter' => ['name' => $this->getCollectionName()]]);
// Convert the CollectionInfoIterator to an array since we cannot rewind its cursor
$collections = iterator_to_array($operation->execute($server));
$collections = $operation->execute($server);

$this->assertInstanceOf('MongoDB\Model\CollectionInfoIterator', $collections);

$this->assertCount(1, $collections);

Expand Down
3 changes: 0 additions & 3 deletions tests/Operation/ListIndexesFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,6 @@ public function testListIndexesForNewlyCreatedCollection()

$this->assertInstanceOf('MongoDB\Model\IndexInfoIterator', $indexes);

// Convert the CursorInfoIterator to an array since we cannot rewind its cursor
$indexes = iterator_to_array($indexes);

$this->assertCount(1, $indexes);

foreach ($indexes as $index) {
Expand Down