From 445f2f93f652f6cf1a0b827a0bde362d10ed69f3 Mon Sep 17 00:00:00 2001 From: Gnucki Date: Mon, 13 Oct 2014 15:40:31 +0200 Subject: [PATCH 1/3] Add slice fetching abstraction --- Doctrine/AbstractProvider.php | 28 +++++++++++++++--- Doctrine/MongoDB/Provider.php | 14 ++------- Doctrine/MongoDB/SliceFetcher.php | 44 ++++++++++++++++++++++++++++ Doctrine/ORM/Provider.php | 2 +- Doctrine/ORM/SliceFetcher.php | 47 ++++++++++++++++++++++++++++++ Doctrine/SliceFetcherInterface.php | 23 +++++++++++++++ Resources/config/mongodb.xml | 8 +++-- Resources/config/orm.xml | 21 ++++++++----- 8 files changed, 160 insertions(+), 27 deletions(-) create mode 100644 Doctrine/MongoDB/SliceFetcher.php create mode 100644 Doctrine/ORM/SliceFetcher.php create mode 100644 Doctrine/SliceFetcherInterface.php diff --git a/Doctrine/AbstractProvider.php b/Doctrine/AbstractProvider.php index d897c96..a66e476 100644 --- a/Doctrine/AbstractProvider.php +++ b/Doctrine/AbstractProvider.php @@ -11,6 +11,7 @@ use FOS\ElasticaBundle\Provider\IndexableInterface; abstract class AbstractProvider extends BaseAbstractProvider { protected $managerRegistry; + protected $sliceFetcher; /** * Constructor. @@ -20,13 +21,15 @@ abstract class AbstractProvider extends BaseAbstractProvider * @param string $objectClass * @param array $options * @param ManagerRegistry $managerRegistry + * @param SliceFetcherInterface $sliceFetcher */ public function __construct( ObjectPersisterInterface $objectPersister, IndexableInterface $indexable, $objectClass, array $options, - ManagerRegistry $managerRegistry + ManagerRegistry $managerRegistry, + SliceFetcherInterface $sliceFetcher = null ) { parent::__construct($objectPersister, $indexable, $objectClass, array_merge(array( 'clear_object_manager' => true, @@ -36,6 +39,7 @@ abstract class AbstractProvider extends BaseAbstractProvider ), $options)); $this->managerRegistry = $managerRegistry; + $this->sliceFetcher = $sliceFetcher; } /** @@ -60,7 +64,24 @@ abstract class AbstractProvider extends BaseAbstractProvider if ($loggerClosure) { $stepStartTime = microtime(true); } - $objects = $this->fetchSlice($queryBuilder, $batchSize, $offset, $objects); + + if ($this->sliceFetcher) { + $identifierFieldNames = $this->managerRegistry + ->getManagerForClass($this->objectClass) + ->getClassMetadata($this->objectClass) + ->getIdentifierFieldNames(); + + $objects = $this->sliceFetcher->fetch( + $queryBuilder, + $batchSize, + $offset, + $objects, + $identifierFieldNames + ); + } else { + $objects = $this->fetchSlice($queryBuilder, $batchSize, $offset); + } + if ($loggerClosure) { $stepNbObjects = count($objects); } @@ -134,10 +155,9 @@ abstract class AbstractProvider extends BaseAbstractProvider * @param object $queryBuilder * @param integer $limit * @param integer $offset - * @param array $previousSlice * @return array */ - protected abstract function fetchSlice($queryBuilder, $limit, $offset, array $previousSlice); + protected abstract function fetchSlice($queryBuilder, $limit, $offset); /** * Creates the query builder, which will be used to fetch objects to index. diff --git a/Doctrine/MongoDB/Provider.php b/Doctrine/MongoDB/Provider.php index ae711ec..6a9ab19 100644 --- a/Doctrine/MongoDB/Provider.php +++ b/Doctrine/MongoDB/Provider.php @@ -59,25 +59,15 @@ class Provider extends AbstractProvider /** * @see FOS\ElasticaBundle\Doctrine\AbstractProvider::fetchSlice() */ - protected function fetchSlice($queryBuilder, $limit, $offset, array $previousSlice) + protected function fetchSlice($queryBuilder, $limit, $offset) { if (!$queryBuilder instanceof Builder) { throw new InvalidArgumentTypeException($queryBuilder, 'Doctrine\ODM\MongoDB\Query\Builder'); } - $lastObject = array_pop($previousSlice); - - if ($lastObject) { - $queryBuilder - ->field('_id')->gt($lastObject->getId()) - ->skip(0); - } else { - $queryBuilder->skip($offset); - } - return $queryBuilder + ->skip($offset) ->limit($limit) - ->sort(array('_id' => 'asc')) ->getQuery() ->execute() ->toArray(); diff --git a/Doctrine/MongoDB/SliceFetcher.php b/Doctrine/MongoDB/SliceFetcher.php new file mode 100644 index 0000000..f90783e --- /dev/null +++ b/Doctrine/MongoDB/SliceFetcher.php @@ -0,0 +1,44 @@ + + */ +class SliceFetcher implements SliceFetcherInterface +{ + /** + * {@inheritdoc} + */ + public function fetch($queryBuilder, $limit, $offset, array $previousSlice, array $identifierFieldNames) + { + if (!$queryBuilder instanceof Builder) { + throw new InvalidArgumentTypeException($queryBuilder, 'Doctrine\ODM\MongoDB\Query\Builder'); + } + + $lastObject = array_pop($previousSlice); + + if ($lastObject) { + $queryBuilder + ->field('_id')->gt($lastObject->getId()) + ->skip(0) + ; + } else { + $queryBuilder->skip($offset); + } + + return $queryBuilder + ->limit($limit) + ->sort(array('_id' => 'asc')) + ->getQuery() + ->execute() + ->toArray() + ; + } +} diff --git a/Doctrine/ORM/Provider.php b/Doctrine/ORM/Provider.php index 701c6c5..7e2ac12 100644 --- a/Doctrine/ORM/Provider.php +++ b/Doctrine/ORM/Provider.php @@ -70,7 +70,7 @@ class Provider extends AbstractProvider /** * @see FOS\ElasticaBundle\Doctrine\AbstractProvider::fetchSlice() */ - protected function fetchSlice($queryBuilder, $limit, $offset, array $previousSlice) + protected function fetchSlice($queryBuilder, $limit, $offset) { if (!$queryBuilder instanceof QueryBuilder) { throw new InvalidArgumentTypeException($queryBuilder, 'Doctrine\ORM\QueryBuilder'); diff --git a/Doctrine/ORM/SliceFetcher.php b/Doctrine/ORM/SliceFetcher.php new file mode 100644 index 0000000..d7f81e1 --- /dev/null +++ b/Doctrine/ORM/SliceFetcher.php @@ -0,0 +1,47 @@ + + */ +class SliceFetcher implements SliceFetcherInterface +{ + /** + * {@inheritdoc} + */ + public function fetch($queryBuilder, $limit, $offset, array $previousSlice, array $identifierFieldNames) + { + if (!$queryBuilder instanceof QueryBuilder) { + throw new InvalidArgumentTypeException($queryBuilder, 'Doctrine\ORM\QueryBuilder'); + } + + /** + * An orderBy DQL part is required to avoid feching the same row twice. + * @see http://stackoverflow.com/questions/6314879/does-limit-offset-length-require-order-by-for-pagination + * @see http://www.postgresql.org/docs/current/static/queries-limit.html + * @see http://www.sqlite.org/lang_select.html#orderby + */ + $orderBy = $queryBuilder->getDQLPart('orderBy'); + if (empty($orderBy)) { + $rootAliases = $queryBuilder->getRootAliases(); + + foreach ($identifierFieldNames as $fieldName) { + $queryBuilder->addOrderBy($rootAliases[0].'.'.$fieldName); + } + } + + return $queryBuilder + ->setFirstResult($offset) + ->setMaxResults($limit) + ->getQuery() + ->getResult() + ; + } +} diff --git a/Doctrine/SliceFetcherInterface.php b/Doctrine/SliceFetcherInterface.php new file mode 100644 index 0000000..9df7152 --- /dev/null +++ b/Doctrine/SliceFetcherInterface.php @@ -0,0 +1,23 @@ + + */ +interface SliceFetcherInterface +{ + /** + * Fetches a slice of objects using the query builder. + * + * @param object $queryBuilder + * @param integer $limit + * @param integer $offset + * @param array $previousSlice + * @param array $identifierFieldNames + * @return array + */ + function fetch($queryBuilder, $limit, $offset, array $previousSlice, array $identifierFieldNames); +} diff --git a/Resources/config/mongodb.xml b/Resources/config/mongodb.xml index 8e15533..84baf3e 100644 --- a/Resources/config/mongodb.xml +++ b/Resources/config/mongodb.xml @@ -5,20 +5,24 @@ xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> + FOS\ElasticaBundle\Doctrine\MongoDB\SliceFetcher FOS\ElasticaBundle\Doctrine\MongoDB\Provider FOS\ElasticaBundle\Doctrine\Listener FOS\ElasticaBundle\Doctrine\MongoDB\ElasticaToModelTransformer FOS\ElasticaBundle\Doctrine\RepositoryManager - + + + - + + diff --git a/Resources/config/orm.xml b/Resources/config/orm.xml index 94f21d4..ea95245 100644 --- a/Resources/config/orm.xml +++ b/Resources/config/orm.xml @@ -4,20 +4,25 @@ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> - - FOS\ElasticaBundle\Doctrine\ORM\Provider - FOS\ElasticaBundle\Doctrine\Listener - FOS\ElasticaBundle\Doctrine\ORM\ElasticaToModelTransformer - FOS\ElasticaBundle\Doctrine\RepositoryManager - - + + FOS\ElasticaBundle\Doctrine\ORM\SliceFetcher + FOS\ElasticaBundle\Doctrine\ORM\Provider + FOS\ElasticaBundle\Doctrine\Listener + FOS\ElasticaBundle\Doctrine\ORM\ElasticaToModelTransformer + FOS\ElasticaBundle\Doctrine\RepositoryManager + + + + + - + + From 901ea49a3248cb00aee4eccd1869d9b9af8bb3b4 Mon Sep 17 00:00:00 2001 From: Gnucki Date: Sun, 2 Nov 2014 14:22:26 +0100 Subject: [PATCH 2/3] Factorize manager call --- Doctrine/AbstractProvider.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Doctrine/AbstractProvider.php b/Doctrine/AbstractProvider.php index a66e476..b4799fd 100644 --- a/Doctrine/AbstractProvider.php +++ b/Doctrine/AbstractProvider.php @@ -66,8 +66,7 @@ abstract class AbstractProvider extends BaseAbstractProvider } if ($this->sliceFetcher) { - $identifierFieldNames = $this->managerRegistry - ->getManagerForClass($this->objectClass) + $identifierFieldNames = $manager ->getClassMetadata($this->objectClass) ->getIdentifierFieldNames(); From 1369a01dd73ce0b620e1d7e51df40b3c25130725 Mon Sep 17 00:00:00 2001 From: Gnucki Date: Sun, 2 Nov 2014 14:24:28 +0100 Subject: [PATCH 3/3] Add slice fetching abstraction --- Tests/Doctrine/AbstractProviderTest.php | 95 ++++++++++++++++++++++--- 1 file changed, 85 insertions(+), 10 deletions(-) diff --git a/Tests/Doctrine/AbstractProviderTest.php b/Tests/Doctrine/AbstractProviderTest.php index 99ed2de..0c3ac17 100644 --- a/Tests/Doctrine/AbstractProviderTest.php +++ b/Tests/Doctrine/AbstractProviderTest.php @@ -13,6 +13,7 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase private $options; private $managerRegistry; private $indexable; + private $sliceFetcher; public function setUp() { @@ -32,6 +33,8 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase ->method('getManagerForClass') ->with($this->objectClass) ->will($this->returnValue($this->objectManager)); + + $this->sliceFetcher = $this->getMockSliceFetcher(); } /** @@ -45,6 +48,54 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase $queryBuilder = new \stdClass(); + $provider->expects($this->once()) + ->method('createQueryBuilder') + ->will($this->returnValue($queryBuilder)); + + $provider->expects($this->once()) + ->method('countObjects') + ->with($queryBuilder) + ->will($this->returnValue($nbObjects)); + + $this->indexable->expects($this->any()) + ->method('isObjectIndexable') + ->with('index', 'type', $this->anything()) + ->will($this->returnValue(true)); + + $providerInvocationOffset = 2; + $previousSlice = array(); + + foreach ($objectsByIteration as $i => $objects) { + $offset = $objects[0] - 1; + + $this->sliceFetcher->expects($this->at($i)) + ->method('fetch') + ->with($queryBuilder, $batchSize, $offset, $previousSlice, array('id')) + ->will($this->returnValue($objects)); + + $this->objectManager->expects($this->at($i)) + ->method('clear'); + + $previousSlice = $objects; + } + + $this->objectPersister->expects($this->exactly(count($objectsByIteration))) + ->method('insertMany'); + + $provider->populate(); + } + + /** + * @dataProvider providePopulateIterations + */ + public function testPopulateIterationsWithoutSliceFetcher($nbObjects, $objectsByIteration, $batchSize) + { + $this->options['batch_size'] = $batchSize; + + $provider = $this->getMockAbstractProvider(false); + + $queryBuilder = new \stdClass(); + $provider->expects($this->once()) ->method('createQueryBuilder') ->will($this->returnValue($queryBuilder)); @@ -107,8 +158,8 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase ->method('countObjects') ->will($this->returnValue($nbObjects)); - $provider->expects($this->any()) - ->method('fetchSlice') + $this->sliceFetcher->expects($this->any()) + ->method('fetch') ->will($this->returnValue($objects)); $this->indexable->expects($this->any()) @@ -133,8 +184,8 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase ->method('countObjects') ->will($this->returnValue($nbObjects)); - $provider->expects($this->any()) - ->method('fetchSlice') + $this->sliceFetcher->expects($this->any()) + ->method('fetch') ->will($this->returnValue($objects)); $this->indexable->expects($this->any()) @@ -165,8 +216,8 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase ->method('countObjects') ->will($this->returnValue($nbObjects)); - $provider->expects($this->any()) - ->method('fetchSlice') + $this->sliceFetcher->expects($this->any()) + ->method('fetch') ->will($this->returnValue($objects)); $this->indexable->expects($this->any()) @@ -192,8 +243,9 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase $provider->expects($this->any()) ->method('countObjects') ->will($this->returnValue($nbObjects)); - $provider->expects($this->any()) - ->method('fetchSlice') + + $this->sliceFetcher->expects($this->any()) + ->method('fetch') ->will($this->returnValue($objects)); $this->indexable->expects($this->at(0)) @@ -214,9 +266,11 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase } /** + * @param boolean $setSliceFetcher Whether or not to set the slice fetcher. + * * @return \FOS\ElasticaBundle\Doctrine\AbstractProvider|\PHPUnit_Framework_MockObject_MockObject */ - private function getMockAbstractProvider() + private function getMockAbstractProvider($setSliceFetcher = true) { return $this->getMockForAbstractClass('FOS\ElasticaBundle\Doctrine\AbstractProvider', array( $this->objectPersister, @@ -224,6 +278,7 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase $this->objectClass, $this->options, $this->managerRegistry, + $setSliceFetcher ? $this->sliceFetcher : null )); } @@ -250,7 +305,17 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase */ private function getMockObjectManager() { - return $this->getMock(__NAMESPACE__ . '\ObjectManager'); + $mock = $this->getMock(__NAMESPACE__ . '\ObjectManager'); + + $mock->expects($this->any()) + ->method('getClassMetadata') + ->will($this->returnSelf()); + + $mock->expects($this->any()) + ->method('getIdentifierFieldNames') + ->will($this->returnValue(array('id'))); + + return $mock; } /** @@ -268,6 +333,14 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase { return $this->getMock('FOS\ElasticaBundle\Provider\IndexableInterface'); } + + /** + * @return \FOS\ElasticaBundle\Doctrine\SliceFetcherInterface|\PHPUnit_Framework_MockObject_MockObject + */ + private function getMockSliceFetcher() + { + return $this->getMock('FOS\ElasticaBundle\Doctrine\SliceFetcherInterface'); + } } /** @@ -277,4 +350,6 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase interface ObjectManager { function clear(); + function getClassMetadata(); + function getIdentifierFieldNames(); }