diff --git a/CHANGELOG-3.1.md b/CHANGELOG-3.1.md index 57fb645..9f79d88 100644 --- a/CHANGELOG-3.1.md +++ b/CHANGELOG-3.1.md @@ -35,3 +35,6 @@ https://github.com/FriendsOfSymfony/FOSElasticaBundle/compare/v3.0.4...v3.1.0 for indexing. #794 * Fixed a case where ProgressCommand would always ignore errors regardless of --ignore-errors being passed or not. + * Added a `SliceFetcher` abstraction for Doctrine providers that get more + information about the previous slice allowing for optimising queries during + population. #725 diff --git a/Doctrine/AbstractProvider.php b/Doctrine/AbstractProvider.php index 2d5d264..fd5aa89 100644 --- a/Doctrine/AbstractProvider.php +++ b/Doctrine/AbstractProvider.php @@ -10,6 +10,14 @@ use FOS\ElasticaBundle\Provider\IndexableInterface; abstract class AbstractProvider extends BaseAbstractProvider { + /** + * @var SliceFetcherInterface + */ + private $sliceFetcher; + + /** + * @var ManagerRegistry + */ protected $managerRegistry; /** @@ -20,13 +28,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 +46,7 @@ abstract class AbstractProvider extends BaseAbstractProvider ), $options)); $this->managerRegistry = $managerRegistry; + $this->sliceFetcher = $sliceFetcher; } /** @@ -55,8 +66,9 @@ abstract class AbstractProvider extends BaseAbstractProvider $ignoreErrors = isset($options['ignore-errors']) ? $options['ignore-errors'] : $this->options['ignore_errors']; $manager = $this->managerRegistry->getManagerForClass($this->objectClass); + $objects = array(); for (; $offset < $nbObjects; $offset += $batchSize) { - $objects = $this->fetchSlice($queryBuilder, $batchSize, $offset); + $objects = $this->getSlice($queryBuilder, $batchSize, $offset, $objects); $objects = array_filter($objects, array($this, 'isObjectIndexable')); if ($objects) { @@ -89,6 +101,36 @@ abstract class AbstractProvider extends BaseAbstractProvider } } + /** + * If this Provider has a SliceFetcher defined, we use it instead of falling back to + * the fetchSlice methods defined in the ORM/MongoDB subclasses. + * + * @param $queryBuilder + * @param int $limit + * @param int $offset + * @param array $lastSlice + * @return array + */ + protected function getSlice($queryBuilder, $limit, $offset, $lastSlice) + { + if (!$this->sliceFetcher) { + return $this->fetchSlice($queryBuilder, $limit, $offset); + } + + $manager = $this->managerRegistry->getManagerForClass($this->objectClass); + $identifierFieldNames = $manager + ->getClassMetadata($this->objectClass) + ->getIdentifierFieldNames(); + + return $this->sliceFetcher->fetch( + $queryBuilder, + $limit, + $offset, + $lastSlice, + $identifierFieldNames + ); + } + /** * Counts objects that would be indexed using the query builder. * diff --git a/Doctrine/MongoDB/Provider.php b/Doctrine/MongoDB/Provider.php index 9e1c5dd..6a9ab19 100644 --- a/Doctrine/MongoDB/Provider.php +++ b/Doctrine/MongoDB/Provider.php @@ -66,8 +66,8 @@ class Provider extends AbstractProvider } return $queryBuilder - ->limit($limit) ->skip($offset) + ->limit($limit) ->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/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 703c1d2..07161e9 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 2d033ee..19a2bf5 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 + + + + + - + + diff --git a/Tests/Doctrine/AbstractProviderTest.php b/Tests/Doctrine/AbstractProviderTest.php index a129a6c..623ab19 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()) @@ -127,14 +178,14 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase $nbObjects = 1; $objects = array(1); - $provider = $this->getMockAbstractProvider(); + $provider = $this->getMockAbstractProvider(true); $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->any()) @@ -159,8 +210,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()) @@ -191,8 +242,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()) @@ -218,8 +269,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)) @@ -240,9 +292,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, @@ -250,6 +304,7 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase $this->objectClass, $this->options, $this->managerRegistry, + $setSliceFetcher ? $this->sliceFetcher : null )); } @@ -276,7 +331,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; } /** @@ -294,6 +359,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'); + } } /** @@ -303,4 +376,6 @@ class AbstractProviderTest extends \PHPUnit_Framework_TestCase interface ObjectManager { function clear(); + function getClassMetadata(); + function getIdentifierFieldNames(); }