From 1d3f4eb05a6075ebe7e56dd37ac5337320c7c6bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Joachim=20L=C3=B8vgaard?= Date: Thu, 11 Nov 2021 16:04:46 +0100 Subject: [PATCH] Simplifying --- composer.json | 1 + psalm.xml | 3 +- src/Batch/Batch.php | 24 +- src/Batch/BatchInterface.php | 2 + src/Batch/CollectionBatch.php | 29 --- src/Batch/CollectionBatchInterface.php | 10 - src/Batcher/Batcher.php | 213 ++++++++++-------- src/Batcher/Collection/CollectionBatcher.php | 20 -- .../Collection/CollectionBatcherInterface.php | 16 -- .../Collection/IdCollectionBatcher.php | 27 --- .../Collection/ObjectCollectionBatcher.php | 23 -- src/Exception/ExceptionInterface.php | 11 - ...rBoundIsGreaterThanUpperBoundException.php | 32 --- src/Exception/NoManagerException.php | 24 -- src/Factory/BatcherFactory.php | 39 ---- src/Factory/BatcherFactoryInterface.php | 15 -- src/Query/QueryRebuilder.php | 18 +- ...lectionBatcherTest.php => BatcherTest.php} | 24 +- .../Collection/IdCollectionBatcherTest.php | 75 ------ 19 files changed, 160 insertions(+), 446 deletions(-) delete mode 100644 src/Batch/CollectionBatch.php delete mode 100644 src/Batch/CollectionBatchInterface.php delete mode 100644 src/Batcher/Collection/CollectionBatcher.php delete mode 100644 src/Batcher/Collection/CollectionBatcherInterface.php delete mode 100644 src/Batcher/Collection/IdCollectionBatcher.php delete mode 100644 src/Batcher/Collection/ObjectCollectionBatcher.php delete mode 100644 src/Exception/ExceptionInterface.php delete mode 100644 src/Exception/LowerBoundIsGreaterThanUpperBoundException.php delete mode 100644 src/Exception/NoManagerException.php delete mode 100644 src/Factory/BatcherFactory.php delete mode 100644 src/Factory/BatcherFactoryInterface.php rename tests/Batcher/{Collection/ObjectCollectionBatcherTest.php => BatcherTest.php} (74%) delete mode 100644 tests/Batcher/Collection/IdCollectionBatcherTest.php diff --git a/composer.json b/composer.json index 921a413..8241f77 100644 --- a/composer.json +++ b/composer.json @@ -14,6 +14,7 @@ "doctrine/collections": "^1.6", "doctrine/orm": "^2.6", "doctrine/persistence": "^1.3 || ^2.1", + "setono/doctrine-object-manager-trait": "^1.0", "symfony/property-access": "^4.4 || ^5.2", "webmozart/assert": "^1.10" }, diff --git a/psalm.xml b/psalm.xml index 65e9d8c..ee10cca 100644 --- a/psalm.xml +++ b/psalm.xml @@ -1,7 +1,8 @@ diff --git a/src/Batch/Batch.php b/src/Batch/Batch.php index c77ef46..29863e6 100644 --- a/src/Batch/Batch.php +++ b/src/Batch/Batch.php @@ -4,29 +4,46 @@ namespace Setono\DoctrineORMBatcher\Batch; +use Doctrine\ORM\Query\Parameter; use Doctrine\ORM\QueryBuilder; use InvalidArgumentException; -abstract class Batch implements BatchInterface +class Batch implements BatchInterface { + private array $data; + + /** @var class-string */ protected string $class; protected string $dql; + /** + * @var array + */ protected array $parameters; - public function __construct(QueryBuilder $qb) + public function __construct(array $data, QueryBuilder $qb) { + /** @var array $rootEntities */ $rootEntities = $qb->getRootEntities(); if (0 === count($rootEntities)) { throw new InvalidArgumentException('The number of root entities on the query builder must be one or more'); } + $this->data = $data; $this->class = $rootEntities[0]; $this->dql = $qb->getDQL(); $this->parameters = $qb->getParameters()->toArray(); } + public function getData(): array + { + return $this->data; + } + + /** + * @return class-string + */ public function getClass(): string { return $this->class; @@ -37,6 +54,9 @@ public function getDql(): string return $this->dql; } + /** + * @return array + */ public function getParameters(): array { return $this->parameters; diff --git a/src/Batch/BatchInterface.php b/src/Batch/BatchInterface.php index 88b6850..543781b 100644 --- a/src/Batch/BatchInterface.php +++ b/src/Batch/BatchInterface.php @@ -6,6 +6,8 @@ interface BatchInterface { + public function getData(): array; + /** * This is the root entity that can be used to get the entity manager from the manager registry. */ diff --git a/src/Batch/CollectionBatch.php b/src/Batch/CollectionBatch.php deleted file mode 100644 index 7ee5f02..0000000 --- a/src/Batch/CollectionBatch.php +++ /dev/null @@ -1,29 +0,0 @@ -collection = $collection; - - $qb->setParameter(self::PARAMETER_COLLECTION, $this->getCollection()); - - /** @psalm-suppress ImpureMethodCall */ - parent::__construct($qb); - } - - public function getCollection(): array - { - return $this->collection; - } -} diff --git a/src/Batch/CollectionBatchInterface.php b/src/Batch/CollectionBatchInterface.php deleted file mode 100644 index de6f95d..0000000 --- a/src/Batch/CollectionBatchInterface.php +++ /dev/null @@ -1,10 +0,0 @@ -qb = clone $qb; - $this->identifier = $identifier; - $this->clearOnBatch = $clearOnBatch; - - $rootAliases = $this->qb->getRootAliases(); + $rootAliases = $qb->getRootAliases(); if (1 !== count($rootAliases)) { - throw new InvalidArgumentException('The query builder must have exactly one root alias'); // todo better exception + throw new InvalidArgumentException(sprintf( + 'The query builder must have exactly one root alias. Your query builder has %d root aliases', + count($rootAliases) + )); } - $this->alias = $rootAliases[0]; + $rootEntity = $qb->getRootEntities()[0]; + $rootEntityMetadata = $qb->getEntityManager()->getClassMetadata($rootEntity); - $this->propertyAccessor = PropertyAccess::createPropertyAccessorBuilder() - ->enableExceptionOnInvalidIndex() - ->enableExceptionOnInvalidPropertyPath() - ->getPropertyAccessor() - ; - } + if($rootEntityMetadata->isIdentifierComposite) { + throw new InvalidArgumentException('This library only support non composite primary keys for now. Sorry :('); + } - public function getBatchCount(int $batchSize = 100): int - { - return (int) ceil($this->getCount() / $batchSize); + // todo check for order by dql part since we override that + + $this->identifier = $rootEntityMetadata->getSingleIdentifierFieldName(); + $this->rootEntityMetadata = $rootEntityMetadata; + $this->rootAlias = $rootAliases[0]; + + // immediately we clone it so that changes to the query builder outside of the batcher doesn't affect it + $this->qb = clone $qb; } - /** - * Notice that the $select must include the identifier in some way. - * If the $select is null the original select statement will be used. - */ - protected function getResult(string $select = null, int $batchSize = 100): iterable + public function getBatches(int $batchSize = 100): iterable { $qb = $this->getQueryBuilder(); - if (null !== $select) { - $qb->select($select); - } - - $qb->orderBy(sprintf('%s.%s', $this->alias, $this->identifier), 'ASC') - ->andWhere(sprintf('%s.%s > :lastId', $this->alias, $this->identifier)) + $qb->orderBy(sprintf('%s.%s', $this->rootAlias, $this->identifier), 'ASC') ->setMaxResults($batchSize) ; - $lastId = 0; + $batch = []; + $lastIdentifier = null; - while (true) { - $this->clear(); + do { + if (null !== $lastIdentifier) { + $qb->setParameter(self::PARAMETER_LAST_ID, $lastIdentifier); + } - $qb->setParameter('lastId', $lastId); $result = $qb->getQuery()->getResult(); - - if (0 === count($result)) { + Assert::isArray($result, sprintf('The DQL query "%s" did not result in an array when executing', $qb->getDQL())); + if ([] === $result) { break; } - $lastRow = $result[count($result) - 1]; + foreach ($result as $item) { + Assert::true(is_array($item) || is_object($item)); - $propertyPath = is_array($lastRow) ? sprintf('[%s]', $this->identifier) : $this->identifier; - $lastId = $this->propertyAccessor->getValue($lastRow, $propertyPath); + $batch[] = $item; + } - yield $result; - } + if (null === $lastIdentifier) { + $qb->andWhere(sprintf('%s.%s > :%s', $this->rootAlias, $this->identifier, self::PARAMETER_LAST_ID)); + } + + $lastIdentifier = $this->getIdentifierValueFromItem($item); + + $this->flush(); + $this->clear(); + + yield new Batch($batch, $this->getBatchableQueryBuilder()); + } while (true); + $this->flush(); $this->clear(); } + public function getBatchCount(int $batchSize = 100): int + { + return (int) ceil($this->getCount() / $batchSize); + } + + public function flushOnBatch(): void + { + $this->flushOnBatch = true; + } + + public function doNotFlushOnBatch(): void + { + $this->flushOnBatch = false; + } + + public function clearOnBatch(): void + { + $this->clearOnBatch = true; + } + + public function doNotClearOnBatch(): void + { + $this->clearOnBatch = false; + } + + private function flush(): void + { + if (!$this->flushOnBatch) { + return; + } + + $this->qb->getEntityManager()->flush(); + } + private function clear(): void { if (!$this->clearOnBatch) { @@ -111,74 +148,58 @@ private function clear(): void /** * This is made to avoid side effects by passing around the query builder object. */ - protected function getQueryBuilder(): QueryBuilder + private function getQueryBuilder(): QueryBuilder { return clone $this->qb; } /** - * This will return a query builder where the constraints for the respective batcher are added. + * @param array|object $item + * @return string|int */ - abstract protected function getBatchableQueryBuilder(): QueryBuilder; - - protected function getMin(): int + private function getIdentifierValueFromItem($item) { - if (null === $this->min) { - $this->initMinMax(); + if(is_object($item)) { + $lastId = $this->rootEntityMetadata->getIdentifierValues($item)[$this->identifier]; + } else { + Assert::keyExists($item, $this->identifier, 'The identifier of your root entity needs to be part of your select statement'); + + /** @var mixed $lastId */ + $lastId = $item[$this->identifier]; } - Assert::notNull($this->min); + Assert::true(is_int($lastId) || is_string($lastId)); - return $this->min; + return $lastId; } - protected function getMax(): int + /** + * This will return a query builder where the constraints for the respective batcher are added. + */ + private function getBatchableQueryBuilder(): QueryBuilder { - if (null === $this->max) { - $this->initMinMax(); - } - - Assert::notNull($this->max); + $qb = $this->getQueryBuilder(); + $qb->andWhere(sprintf('%s.%s IN(:%s)', $this->rootAlias, $this->identifier, self::PARAMETER_DATA)); - return $this->max; + return $qb; } - protected function getCount(): int + private function getCount(): int { if (null === $this->count) { $this->initCount(); } - Assert::notNull($this->count); - return $this->count; } - private function initMinMax(): void - { - $qb = $this->getQueryBuilder(); - - $qb->select(sprintf('MIN(%s.%s) as min, MAX(%s.%s) as max', $this->alias, $this->identifier, $this->alias, $this->identifier)); - - $res = $qb->getQuery()->getScalarResult(); - if (count($res) < 1) { - throw new NoResultException(); - } - - $row = $res[0]; - - if (null === $row['min'] || null === $row['max']) { - throw new NoResultException(); - } - - $this->min = (int) $row['min']; - $this->max = (int) $row['max']; - } - + /** + * @psalm-assert int $this->count + */ private function initCount(): void { $qb = $this->getQueryBuilder(); - $qb->select(sprintf('COUNT(%s) as c', $this->alias)); + $qb->select(sprintf('COUNT(%s)', $this->rootAlias)); $this->count = (int) $qb->getQuery()->getSingleScalarResult(); } diff --git a/src/Batcher/Collection/CollectionBatcher.php b/src/Batcher/Collection/CollectionBatcher.php deleted file mode 100644 index cd6ca28..0000000 --- a/src/Batcher/Collection/CollectionBatcher.php +++ /dev/null @@ -1,20 +0,0 @@ -getQueryBuilder(); - $qb->andWhere(sprintf('%s.%s IN(:%s)', $this->alias, $this->identifier, CollectionBatch::PARAMETER_COLLECTION)); - - return $qb; - } -} diff --git a/src/Batcher/Collection/CollectionBatcherInterface.php b/src/Batcher/Collection/CollectionBatcherInterface.php deleted file mode 100644 index c883748..0000000 --- a/src/Batcher/Collection/CollectionBatcherInterface.php +++ /dev/null @@ -1,16 +0,0 @@ - - */ - public function getBatches(int $batchSize = 100): iterable; -} diff --git a/src/Batcher/Collection/IdCollectionBatcher.php b/src/Batcher/Collection/IdCollectionBatcher.php deleted file mode 100644 index 2bc5c23..0000000 --- a/src/Batcher/Collection/IdCollectionBatcher.php +++ /dev/null @@ -1,27 +0,0 @@ - - */ - public function getBatches(int $batchSize = 100): iterable - { - $result = $this->getResult(sprintf('%s.%s', $this->alias, $this->identifier), $batchSize); - - foreach ($result as $ids) { - $flattened = array_map(function ($elm) { - return $elm[$this->identifier]; - }, $ids); - - yield new CollectionBatch($flattened, $this->getBatchableQueryBuilder()); - } - } -} diff --git a/src/Batcher/Collection/ObjectCollectionBatcher.php b/src/Batcher/Collection/ObjectCollectionBatcher.php deleted file mode 100644 index 745dd0e..0000000 --- a/src/Batcher/Collection/ObjectCollectionBatcher.php +++ /dev/null @@ -1,23 +0,0 @@ - - */ - public function getBatches(int $batchSize = 100): iterable - { - $result = $this->getResult(null, $batchSize); - - foreach ($result as $objects) { - yield new CollectionBatch($objects, $this->getBatchableQueryBuilder()); - } - } -} diff --git a/src/Exception/ExceptionInterface.php b/src/Exception/ExceptionInterface.php deleted file mode 100644 index bdb76e8..0000000 --- a/src/Exception/ExceptionInterface.php +++ /dev/null @@ -1,11 +0,0 @@ -lowerBound = $lowerBound; - $this->upperBound = $upperBound; - } - - public function getLowerBound(): int - { - return $this->lowerBound; - } - - public function getUpperBound(): int - { - return $this->upperBound; - } -} diff --git a/src/Exception/NoManagerException.php b/src/Exception/NoManagerException.php deleted file mode 100644 index ab68cee..0000000 --- a/src/Exception/NoManagerException.php +++ /dev/null @@ -1,24 +0,0 @@ -class = $class; - } - - public function getClass(): string - { - return $this->class; - } -} diff --git a/src/Factory/BatcherFactory.php b/src/Factory/BatcherFactory.php deleted file mode 100644 index b147db3..0000000 --- a/src/Factory/BatcherFactory.php +++ /dev/null @@ -1,39 +0,0 @@ -objectCollectionBatcherClass = $objectCollectionBatcherClass; - $this->idCollectionBatcherClass = $idCollectionBatcherClass; - } - - public function createObjectCollectionBatcher( - QueryBuilder $qb, - string $identifier = 'id', - bool $clearOnBatch = true - ): CollectionBatcherInterface { - return new $this->objectCollectionBatcherClass($qb, $identifier, $clearOnBatch); - } - - public function createIdCollectionBatcher( - QueryBuilder $qb, - string $identifier = 'id', - bool $clearOnBatch = true - ): CollectionBatcherInterface { - return new $this->idCollectionBatcherClass($qb, $identifier, $clearOnBatch); - } -} diff --git a/src/Factory/BatcherFactoryInterface.php b/src/Factory/BatcherFactoryInterface.php deleted file mode 100644 index 1148b8e..0000000 --- a/src/Factory/BatcherFactoryInterface.php +++ /dev/null @@ -1,15 +0,0 @@ -createQuery($batch->getDql()); $q->setParameters(new ArrayCollection($batch->getParameters())); + $q->setParameter(Batcher::PARAMETER_DATA, $batch->getData()); return $q; } - - private function getManager(string $class): EntityManagerInterface - { - /** @var EntityManagerInterface|null $manager */ - $manager = $this->managerRegistry->getManagerForClass($class); - if (null === $manager) { - throw new NoManagerException($class); - } - - return $manager; - } } diff --git a/tests/Batcher/Collection/ObjectCollectionBatcherTest.php b/tests/Batcher/BatcherTest.php similarity index 74% rename from tests/Batcher/Collection/ObjectCollectionBatcherTest.php rename to tests/Batcher/BatcherTest.php index 864ecac..018904e 100644 --- a/tests/Batcher/Collection/ObjectCollectionBatcherTest.php +++ b/tests/Batcher/BatcherTest.php @@ -2,18 +2,21 @@ declare(strict_types=1); -namespace Tests\Setono\DoctrineORMBatcher\Batcher\Collection; +namespace Tests\Setono\DoctrineORMBatcher\Batcher; -use Setono\DoctrineORMBatcher\Batcher\Collection\ObjectCollectionBatcher; +use Setono\DoctrineORMBatcher\Batcher\Batcher; use Tests\Setono\DoctrineORMBatcher\Entity\Entity; use Tests\Setono\DoctrineORMBatcher\EntityManagerAwareTestCase; -final class ObjectCollectionBatcherTest extends EntityManagerAwareTestCase +/** + * @covers \Setono\DoctrineORMBatcher\Batcher\Batcher + */ +final class BatcherTest extends EntityManagerAwareTestCase { /** * @test */ - public function it_works(): void + public function it_batches(): void { $this->purger->purge(); @@ -52,17 +55,14 @@ public function it_works(): void ->from(Entity::class, 'o') ->andWhere('o.enabled = 1') ; + $batcher = new Batcher($qb); - $batcher = new ObjectCollectionBatcher($qb); - - $batches = $batcher->getBatches(10); - - foreach ($batches as $idx => $batch) { - /** @var Entity $entity */ - foreach ($batch->getCollection() as $entity) { + foreach ($batcher->getBatches() as $batch) { + /** @var Entity $item */ + foreach ($batch->getData() as $item) { $this->assertArrayHasKey($entity->getId(), $expectedIds); - $expectedIds[$entity->getId()] = true; + $expectedIds[$item->getId()] = true; } } diff --git a/tests/Batcher/Collection/IdCollectionBatcherTest.php b/tests/Batcher/Collection/IdCollectionBatcherTest.php deleted file mode 100644 index e032913..0000000 --- a/tests/Batcher/Collection/IdCollectionBatcherTest.php +++ /dev/null @@ -1,75 +0,0 @@ -purger->purge(); - - $expectedIds = []; - - for ($i = 10; $i <= 15; ++$i) { - $entity = new Entity($i); - $this->entityManager->persist($entity); - - $expectedIds[$i] = false; - } - - for ($i = 18; $i <= 28; ++$i) { - $entity = new Entity($i); - $this->entityManager->persist($entity); - - $expectedIds[$i] = false; - } - - for ($i = 35; $i <= 50; ++$i) { - $entity = new Entity($i, false); // these entities are not expected in the resulting batches - $this->entityManager->persist($entity); - } - - for ($i = 78; $i <= 100; ++$i) { - $entity = new Entity($i); - $this->entityManager->persist($entity); - - $expectedIds[$i] = false; - } - - $this->entityManager->flush(); - - $qb = $this->entityManager->createQueryBuilder(); - $qb->select('o') - ->from(Entity::class, 'o') - ->andWhere('o.enabled = 1') - ; - - $batcher = new IdCollectionBatcher($qb); - - $batches = $batcher->getBatches(10); - - foreach ($batches as $idx => $batch) { - foreach ($batch->getCollection() as $id) { - $this->assertArrayHasKey($id, $expectedIds); - - $expectedIds[$id] = true; - } - } - - // the call effectively removes all ids that are set to true (which means they were found) - $idsNotFound = array_filter($expectedIds, static function ($expectedId) { - return !$expectedId; - }); - - $this->assertCount(0, $idsNotFound); - } -}