From 146fa76d12c27b8c3543fe2000910b0810fa25dd Mon Sep 17 00:00:00 2001 From: Michael Crumm Date: Mon, 3 Apr 2017 11:04:35 -0700 Subject: [PATCH] Make ObjectIterator compatible with Doctrine Collections Ensure Collection::get() hydrates a document Handle conversion in ObjectIterator::first() and ObjectIterator::last() Handle conversion for ObjectIterator methods that return values Add tests for ObjectCallbackIterator CS fixes for ObjectIterator --- Collection/Collection.php | 104 +---------- Resources/doc/mapping.md | 8 +- Result/Converter.php | 2 +- Result/ObjectCallbackIterator.php | 57 +++++++ Result/ObjectIterator.php | 161 ++++++++++++++++-- .../Functional/Result/ObjectIteratorTest.php | 105 ++++++++++++ Tests/Unit/Collection/CollectionTest.php | 8 +- .../Result/ObjectCallbackIteratorTest.php | 83 +++++++++ .../fixture/TestBundle/Document/Product.php | 4 +- .../app/fixture/TestBundle/Entity/Product.php | 4 +- composer.json | 1 + 11 files changed, 409 insertions(+), 128 deletions(-) create mode 100644 Result/ObjectCallbackIterator.php create mode 100644 Tests/Unit/Result/ObjectCallbackIteratorTest.php diff --git a/Collection/Collection.php b/Collection/Collection.php index 9415859e..7d102803 100644 --- a/Collection/Collection.php +++ b/Collection/Collection.php @@ -11,107 +11,13 @@ namespace ONGR\ElasticsearchBundle\Collection; +use Doctrine\Common\Collections\ArrayCollection; + /** * This class is a holder for collection of objects. + * + * @deprecated Use Doctrine\Common\Collections\ArrayCollection instead. */ -class Collection implements \Countable, \Iterator, \ArrayAccess +class Collection extends ArrayCollection { - /** - * @var array - */ - private $elements = []; - - /** - * Constructor. - * - * @param array $elements - */ - public function __construct(array $elements = []) - { - $this->elements = $elements; - } - - /** - * {@inheritdoc} - */ - public function current() - { - return current($this->elements); - } - - /** - * {@inheritdoc} - */ - public function next() - { - next($this->elements); - } - - /** - * {@inheritdoc} - */ - public function key() - { - return key($this->elements); - } - - /** - * {@inheritdoc} - */ - public function valid() - { - return $this->offsetExists($this->key()); - } - - /** - * {@inheritdoc} - */ - public function rewind() - { - reset($this->elements); - } - - /** - * {@inheritdoc} - */ - public function offsetExists($offset) - { - return array_key_exists($offset, $this->elements); - } - - /** - * {@inheritdoc} - */ - public function offsetGet($offset) - { - return $this->elements[$offset]; - } - - /** - * {@inheritdoc} - */ - public function offsetSet($offset, $value) - { - if ($offset === null) { - $this->elements[] = $value; - } else { - $this->elements[$offset] = $value; - } - } - - /** - * {@inheritdoc} - */ - public function offsetUnset($offset) - { - unset($this->elements[$offset]); - } - - /** - * {@inheritdoc} - */ - public function count() - { - return count($this->elements); - } } diff --git a/Resources/doc/mapping.md b/Resources/doc/mapping.md index 3d8545d5..3c50995d 100644 --- a/Resources/doc/mapping.md +++ b/Resources/doc/mapping.md @@ -337,7 +337,7 @@ To insert a document with mapping from example above you have to create 2 object As shown in the example above, by ElasticsearchBundle default, only a single object will be saved in the document. Meanwhile, Elasticsearch database doesn't care if in an object is stored as a single value or as an array. If it is necessary to store multiple objects (array), you have to add `multiple=true` to the annotation. While -initiating a document with multiple items you need to initialize property with the new instance of `Collection()`. +initiating a document with multiple items you need to initialize property with the new instance of `ArrayCollection()`. Here's an example: @@ -346,8 +346,8 @@ Here's an example: namespace AppBundle\Document; +use Doctrine\Common\Collections\ArrayCollection; use ONGR\ElasticsearchBundle\Annotation as ES; -use ONGR\ElasticsearchBundle\Collection\Collection; /** * @ES\Document() @@ -368,7 +368,7 @@ class Product public function __construct() { - $this->variants = new Collection(); + $this->variants = new ArrayCollection(); } /** @@ -379,7 +379,7 @@ class Product */ public function addVariant(VariantObject $variant) { - $this->variants[] => $variant; + $this->variants[] = $variant; return $this; } diff --git a/Result/Converter.php b/Result/Converter.php index b678134c..13cbfdc9 100644 --- a/Result/Converter.php +++ b/Result/Converter.php @@ -11,9 +11,9 @@ namespace ONGR\ElasticsearchBundle\Result; +use Doctrine\Common\Collections\Collection; use ONGR\ElasticsearchBundle\Annotation\Nested; use ONGR\ElasticsearchBundle\Annotation\Object; -use ONGR\ElasticsearchBundle\Collection\Collection; use ONGR\ElasticsearchBundle\Mapping\MetadataCollector; use ONGR\ElasticsearchBundle\Service\Manager; diff --git a/Result/ObjectCallbackIterator.php b/Result/ObjectCallbackIterator.php new file mode 100644 index 00000000..1927da0d --- /dev/null +++ b/Result/ObjectCallbackIterator.php @@ -0,0 +1,57 @@ +callback = $callback; + + parent::__construct($array); + } + + /** + * {@inheritdoc} + */ + public function current() + { + $value = parent::current(); + + // Generate objects on demand + if ($value === null && $this->valid()) { + $key = $this->key(); + $callback = $this->callback; + return $callback($key); + } + + return $value; + } + + /** + * {@inheritdoc} + */ + public function offsetGet($offset) + { + $value = parent::offsetGet($offset); + + // Generate objects on demand + if ($value === null && $this->valid()) { + $callback = $this->callback; + return $callback($offset); + } + + return $value; + } +} diff --git a/Result/ObjectIterator.php b/Result/ObjectIterator.php index 608865f5..59f5fb6c 100644 --- a/Result/ObjectIterator.php +++ b/Result/ObjectIterator.php @@ -11,12 +11,13 @@ namespace ONGR\ElasticsearchBundle\Result; -use ONGR\ElasticsearchBundle\Collection\Collection; +use Closure; +use Doctrine\Common\Collections\ArrayCollection; /** * ObjectIterator class. */ -class ObjectIterator extends Collection +class ObjectIterator extends ArrayCollection { /** * @var Converter @@ -33,6 +34,11 @@ class ObjectIterator extends Collection */ private $rawObjects; + /** + * @var \Closure + */ + private $convertCallback; + /** * Using part of abstract iterator functionality only. * @@ -46,6 +52,14 @@ public function __construct($converter, $objects, $alias) $this->rawObjects = $objects; $this->alias = $alias; + // On-demand conversion callback for ArrayAccess/IteratorAggregate + $this->convertCallback = function ($key) { + $value = $this->convertDocument($this->rawObjects[$key]); + $this->rawObjects[$key] = null; + $this->offsetSet($key, $value); + return $value; + }; + $callback = function ($v) { return null; }; @@ -54,6 +68,23 @@ public function __construct($converter, $objects, $alias) parent::__construct(array_map($callback, $objects)); } + /** + * Converts all existing array values into their document equivalents. + * + * @return array + */ + public function toArray() + { + $all = parent::toArray(); + $callback = $this->convertCallback; + array_walk($all, function (&$value, $key) use ($callback) { + if ($value === null) { + $value = $callback($key); + } + }); + return $all; + } + /** * {@inheritdoc} */ @@ -66,38 +97,136 @@ protected function convertDocument(array $document) ); } + /** + * Converts a raw object when the key for the object must be determined first. + * + * @param array $rawObject + * + * @return bool|object + */ + protected function convertFromValue(array $rawObject) + { + if (false === $rawObject) { + return false; + } + $callback = $this->convertCallback; + $key = key($this->rawObjects); + return $callback($key); + } + + /** + * {@inheritdoc} + */ + public function getIterator() + { + $callback = $this->convertCallback; + return new ObjectCallbackIterator($callback, $this->toArray()); + } + + /** + * {@inheritdoc} + */ + public function first() + { + $first = parent::first(); + if ($first === null) { + $first = reset($this->rawObjects); + return $this->convertFromValue($first); + } + + return $first; + } + + /** + * {@inheritdoc} + */ + public function last() + { + $last = parent::last(); + + if ($last === null) { + $last = end($this->rawObjects); + return $this->convertFromValue($last); + } + + return $last; + } + + /** + * {@inheritdoc} + */ + public function next() + { + $next = parent::next(); + + if ($next === null) { + $next = next($this->rawObjects); + return $this->convertFromValue($next); + } + + return $next; + } + /** * {@inheritdoc} */ public function current() { - $value = parent::current(); + $current = parent::current(); - // Generate objects on demand - if ($value === null && $this->valid()) { - $key = $this->key(); - $value = $this->convertDocument($this->rawObjects[$key]); - $this->rawObjects[$key] = null; - $this->offsetSet($key, $value); + if ($current === null) { + $current = current($this->rawObjects); + return $this->convertFromValue($current); } - return $value; + return $current; } /** * {@inheritdoc} */ - public function offsetGet($offset) + public function get($offset) { - $value = parent::offsetGet($offset); + $value = parent::get($offset); // Generate objects on demand - if ($value === null && $this->valid()) { - $value = $this->convertDocument($this->rawObjects[$offset]); - $this->rawObjects[$offset] = null; - $this->offsetSet($offset, $value); + if ($value === null && $this->containsKey($this->key())) { + $callback = $this->convertCallback; + return $callback($offset); } return $value; } + + /** + * {@inheritdoc} + */ + public function getValues() + { + return array_values($this->toArray()); + } + + /** + * {@inheritdoc} + */ + public function map(Closure $func) + { + return new ArrayCollection(array_map($func, $this->toArray())); + } + + /** + * {@inheritdoc} + */ + public function filter(Closure $p) + { + return new ArrayCollection(array_filter($this->toArray(), $p)); + } + + /** + * {@inheritdoc} + */ + protected function createFrom(array $elements) + { + return new static($this->converter, $elements, $this->alias); + } } diff --git a/Tests/Functional/Result/ObjectIteratorTest.php b/Tests/Functional/Result/ObjectIteratorTest.php index 10a58da0..77f756cb 100644 --- a/Tests/Functional/Result/ObjectIteratorTest.php +++ b/Tests/Functional/Result/ObjectIteratorTest.php @@ -11,6 +11,9 @@ namespace ONGR\ElasticsearchBundle\Tests\Functional\Result; +use Doctrine\Common\Collections\Collection; +use ONGR\ElasticsearchBundle\Tests\app\fixture\TestBundle\Document\CategoryObject; +use ONGR\ElasticsearchBundle\Tests\app\fixture\TestBundle\Document\Product; use ONGR\ElasticsearchDSL\Query\MatchAllQuery; use ONGR\ElasticsearchBundle\Service\Repository; use ONGR\ElasticsearchBundle\Test\AbstractElasticsearchTestCase; @@ -52,6 +55,22 @@ protected function getDataArray() ]; } + /** + * @param string $id + * + * @return Collection + */ + protected function getCategoriesForProduct($id) + { + /** @var Repository $repo */ + $repo = $this->getManager()->getRepository('TestBundle:Product'); + + /** @var Product $document */ + $document = $repo->find($id); + + return $document->getRelatedCategories(); + } + /** * Iteration test. */ @@ -77,4 +96,90 @@ public function testIteration() $this->assertNotNull($categories[0]); } } + + public function testGetItem() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $this->assertNotNull($categories->get(1)); + } + + public function testGetFirstItem() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $this->assertNotNull($categories->first()); + } + + public function testGetLastItem() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $this->assertNotNull($categories->last()); + } + + public function testGetNextItem() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $this->assertNotNull($categories->next()); + } + + public function testGetCurrentItem() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $this->assertNotNull($categories->current()); + } + + public function testCollectionGetValues() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $values = $categories->getValues(); + + foreach ($values as $value) { + $this->assertInstanceOf( + 'ONGR\ElasticsearchBundle\Tests\app\fixture\TestBundle\Document\CategoryObject', + $value + ); + } + } + + public function testCollectionMap() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $mapped = $categories->map(function (CategoryObject $category) { + $category->setTitle($category->getTitle() . '!'); + return $category; + }); + + $this->assertEquals('Acme!', $mapped[0]->getTitle()); + } + + public function testCollectionFilter() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $filtered = $categories->filter(function (CategoryObject $category) { + return $category->getTitle() === 'Acme'; + }); + + $this->assertCount(1, $filtered); + } + + public function testCollectionToArray() + { + $categories = $this->getCategoriesForProduct('doc2'); + + $values = $categories->toArray(); + + foreach ($values as $value) { + $this->assertInstanceOf( + 'ONGR\ElasticsearchBundle\Tests\app\fixture\TestBundle\Document\CategoryObject', + $value + ); + } + } } diff --git a/Tests/Unit/Collection/CollectionTest.php b/Tests/Unit/Collection/CollectionTest.php index 0423d94c..48d2fcfb 100644 --- a/Tests/Unit/Collection/CollectionTest.php +++ b/Tests/Unit/Collection/CollectionTest.php @@ -11,7 +11,7 @@ namespace ONGR\ElasticsearchBundle\Tests\Unit\Collection; -use ONGR\ElasticsearchBundle\Collection\Collection; +use Doctrine\Common\Collections\ArrayCollection; class CollectionTest extends \PHPUnit_Framework_TestCase { @@ -28,7 +28,7 @@ class CollectionTest extends \PHPUnit_Framework_TestCase */ public function testCountable() { - $this->assertCount(count($this->data), new Collection($this->data)); + $this->assertCount(count($this->data), new ArrayCollection($this->data)); } /** @@ -36,7 +36,7 @@ public function testCountable() */ public function testIterator() { - $this->assertEquals($this->data, iterator_to_array(new Collection($this->data))); + $this->assertEquals($this->data, iterator_to_array(new ArrayCollection($this->data))); } /** @@ -44,7 +44,7 @@ public function testIterator() */ public function testArrayAccess() { - $collection = new Collection($this->data); + $collection = new ArrayCollection($this->data); $this->assertArrayHasKey('foo', $collection); $this->assertEquals($this->data['foo'], $collection['foo']); diff --git a/Tests/Unit/Result/ObjectCallbackIteratorTest.php b/Tests/Unit/Result/ObjectCallbackIteratorTest.php new file mode 100644 index 00000000..1a33437b --- /dev/null +++ b/Tests/Unit/Result/ObjectCallbackIteratorTest.php @@ -0,0 +1,83 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace ONGR\ElasticsearchBundle\Tests\Unit\Result; + +use ONGR\ElasticsearchBundle\Result\ObjectCallbackIterator; + +class ObjectCallbackIteratorTest extends \PHPUnit_Framework_TestCase +{ + /** + * Tests the current method of the iterator uses the callback. + */ + public function testCallbackIteratorCurrent() + { + $rawData = [null, null, null]; + + $callback = function ($key) { + return $key; + }; + + $iterator = new ObjectCallbackIterator($callback, $rawData); + + $this->assertEquals(0, $iterator->current()); + } + + /** + * Tests the offsetGet method of the iterator uses the callback. + */ + public function testCallbackIteratorOffsetGet() + { + $rawData = [null, null, null]; + + $callback = function ($key) { + return $key; + }; + + $iterator = new ObjectCallbackIterator($callback, $rawData); + + $this->assertEquals(1, $iterator->offsetGet(1)); + } + + /** + * Tests iteration of the iterator uses the callback. + */ + public function testCallbackIteratorIteration() + { + $rawData = [null, null, null]; + + $callback = function ($key) { + return 'foo'; + }; + + $iterator = new ObjectCallbackIterator($callback, $rawData); + + foreach ($iterator as $item) { + $this->assertEquals('foo', $item); + } + } + + /** + * Tests iterator skips the callback for non-null values. + */ + public function testCallbackIteratorUsesValue() + { + $rawData = [null, 'bar', null]; + + $callback = function ($key) { + return 'foo'; + }; + + $iterator = new ObjectCallbackIterator($callback, $rawData); + + $this->assertEquals('bar', $iterator[1]); + } +} diff --git a/Tests/app/fixture/TestBundle/Document/Product.php b/Tests/app/fixture/TestBundle/Document/Product.php index abe947e4..c575707e 100644 --- a/Tests/app/fixture/TestBundle/Document/Product.php +++ b/Tests/app/fixture/TestBundle/Document/Product.php @@ -11,8 +11,8 @@ namespace ONGR\ElasticsearchBundle\Tests\app\fixture\TestBundle\Document; +use Doctrine\Common\Collections\ArrayCollection; use ONGR\ElasticsearchBundle\Annotation as ES; -use ONGR\ElasticsearchBundle\Collection\Collection; /** * Product document for testing. @@ -117,7 +117,7 @@ class Product */ public function __construct() { - $this->relatedCategories = new Collection(); + $this->relatedCategories = new ArrayCollection(); } /** diff --git a/Tests/app/fixture/TestBundle/Entity/Product.php b/Tests/app/fixture/TestBundle/Entity/Product.php index 88a80e9e..ca68741f 100644 --- a/Tests/app/fixture/TestBundle/Entity/Product.php +++ b/Tests/app/fixture/TestBundle/Entity/Product.php @@ -11,8 +11,8 @@ namespace ONGR\ElasticsearchBundle\Tests\app\fixture\TestBundle\Entity; +use Doctrine\Common\Collections\ArrayCollection; use ONGR\ElasticsearchBundle\Annotation as ES; -use ONGR\ElasticsearchBundle\Collection\Collection; /** * Product document for testing. @@ -43,6 +43,6 @@ class Product public function __construct() { - $this->categories = new Collection(); + $this->categories = new ArrayCollection(); } } diff --git a/composer.json b/composer.json index c0134ec6..44f6f34e 100644 --- a/composer.json +++ b/composer.json @@ -16,6 +16,7 @@ "doctrine/annotations": "~1.2", "doctrine/inflector": "~1.0", "doctrine/cache": "~1.4", + "doctrine/collections": "~1.4", "monolog/monolog": "~1.10", "ongr/elasticsearch-dsl": "~5.0" },