diff --git a/changelog/_unreleased/2020-09-07-repository-iterator-prevent-predictable-last-empty-request.md b/changelog/_unreleased/2020-09-07-repository-iterator-prevent-predictable-last-empty-request.md new file mode 100644 index 00000000000..0a8ddbe8d13 --- /dev/null +++ b/changelog/_unreleased/2020-09-07-repository-iterator-prevent-predictable-last-empty-request.md @@ -0,0 +1,11 @@ +--- +title: Prevent predictable empty request in RepositoryIterator and improve developer experience +issue: NEXT-11081 +author: Joshua Behrens +author_email: code@joshua-behrens.de +author_github: @JoshuaBehrens +--- +# Core +* Added `\Shopware\Core\Framework\DataAbstractionLayer\Dbal\Common\RepositoryIterator::reset` to either remove `increment` filter if the iterated entity has an auto increment column or otherwise removes the offset. This helps to implement iterator resets without knowing how the iteration is implemented +* Added internal state into `\Shopware\Core\Framework\DataAbstractionLayer\Dbal\Common\RepositoryIterator`, whether the previous `fetch` or `fetchIds` result had less items than expected and therefore the next search will be empty anyways +* Added methods `iterateIds` and `iterateEntities` to class `\Shopware\Core\Framework\DataAbstractionLayer\Dbal\Common\RepositoryIterator` to automatically perform iteration on respectively `fetchIds` and `fetch` diff --git a/src/Core/Checkout/Cart/RuleLoader.php b/src/Core/Checkout/Cart/RuleLoader.php index 4b6cccf6ac0..c893f5a4959 100644 --- a/src/Core/Checkout/Cart/RuleLoader.php +++ b/src/Core/Checkout/Cart/RuleLoader.php @@ -50,9 +50,6 @@ public function load(Context $context): RuleCollection $rules->add($rule); } } - if ($result->count() < 500) { - break; - } } return $rules; diff --git a/src/Core/Framework/DataAbstractionLayer/Dbal/Common/RepositoryIterator.php b/src/Core/Framework/DataAbstractionLayer/Dbal/Common/RepositoryIterator.php index be5035ae804..cc0a10208fb 100644 --- a/src/Core/Framework/DataAbstractionLayer/Dbal/Common/RepositoryIterator.php +++ b/src/Core/Framework/DataAbstractionLayer/Dbal/Common/RepositoryIterator.php @@ -3,8 +3,10 @@ namespace Shopware\Core\Framework\DataAbstractionLayer\Dbal\Common; use Shopware\Core\Framework\Context; +use Shopware\Core\Framework\DataAbstractionLayer\Entity; use Shopware\Core\Framework\DataAbstractionLayer\EntityCollection; use Shopware\Core\Framework\DataAbstractionLayer\EntityRepository; +use Shopware\Core\Framework\DataAbstractionLayer\PartialEntity; use Shopware\Core\Framework\DataAbstractionLayer\Search\Criteria; use Shopware\Core\Framework\DataAbstractionLayer\Search\EntitySearchResult; use Shopware\Core\Framework\DataAbstractionLayer\Search\Filter\RangeFilter; @@ -28,6 +30,8 @@ class RepositoryIterator private bool $autoIncrement = false; + private bool $endReached = false; + /** * @param EntityRepository $repository */ @@ -56,6 +60,23 @@ public function __construct( $this->context = clone $context; } + public function reset(): void + { + if ($this->autoIncrement) { + $filters = $this->criteria->getFilters(); + $this->criteria->resetFilters(); + unset($filters['increment']); + + foreach ($filters as $filterKey => $filter) { + $this->criteria->setFilter($filterKey, $filter); + } + } else { + $this->criteria->setOffset(0); + } + + $this->endReached = false; + } + public function getTotal(): int { $criteria = clone $this->criteria; @@ -71,6 +92,10 @@ public function getTotal(): int */ public function fetchIds(): ?array { + if ($this->endReached) { + return null; + } + $this->criteria->setTotalCountMode(Criteria::TOTAL_COUNT_MODE_NONE); $ids = $this->repository->searchIds($this->criteria, $this->context); @@ -78,12 +103,18 @@ public function fetchIds(): ?array $values = $ids->getIds(); if (empty($values)) { + $this->endReached = true; + return null; } if (!$this->autoIncrement) { $this->criteria->setOffset($this->criteria->getOffset() + $this->criteria->getLimit()); + if (count($values) < $this->criteria->getLimit()) { + $this->endReached = true; + } + return $values; } @@ -95,6 +126,10 @@ public function fetchIds(): ?array $increment = $ids->getDataFieldOfId($last, 'autoIncrement') ?? 0; $this->criteria->setFilter('increment', new RangeFilter('autoIncrement', [RangeFilter::GT => $increment])); + if (count($values) < $this->criteria->getLimit()) { + $this->endReached = true; + } + return $values; } @@ -103,6 +138,10 @@ public function fetchIds(): ?array */ public function fetch(): ?EntitySearchResult { + if ($this->endReached) { + return null; + } + $this->criteria->setTotalCountMode(Criteria::TOTAL_COUNT_MODE_NONE); $result = $this->repository->search(clone $this->criteria, $this->context); @@ -111,9 +150,47 @@ public function fetch(): ?EntitySearchResult $this->criteria->setOffset($this->criteria->getOffset() + $this->criteria->getLimit()); if (empty($result->getIds())) { + $this->endReached = true; + return null; } + if ($result->count() < $this->criteria->getLimit()) { + $this->endReached = true; + } + return $result; } + + /** + * @return iterable + */ + public function iterateEntities(): iterable + { + try { + while (($entityResult = $this->fetch()) instanceof EntitySearchResult) { + // yield from is okay, as getElements keys by unique key + yield from $entityResult->getElements(); + } + } finally { + $this->reset(); + } + } + + /** + * @return iterable|iterable> + */ + public function iterateIds(): iterable + { + try { + while (\is_array($ids = $this->fetchIds())) { + // do not use yield from to ensure re-keying + foreach ($ids as $id) { + yield $id; + } + } + } finally { + $this->reset(); + } + } } diff --git a/src/Core/Framework/Test/DataAbstractionLayer/Dbal/RepositoryIteratorTest.php b/src/Core/Framework/Test/DataAbstractionLayer/Dbal/RepositoryIteratorTest.php index 63f2f657908..cfc6aa51420 100644 --- a/src/Core/Framework/Test/DataAbstractionLayer/Dbal/RepositoryIteratorTest.php +++ b/src/Core/Framework/Test/DataAbstractionLayer/Dbal/RepositoryIteratorTest.php @@ -6,15 +6,21 @@ use PHPUnit\Framework\TestCase; use Shopware\Core\Content\Product\ProductCollection; +use Shopware\Core\Content\Product\ProductEvents; use Shopware\Core\Content\Test\Product\ProductBuilder; use Shopware\Core\Framework\Context; use Shopware\Core\Framework\DataAbstractionLayer\Dbal\Common\RepositoryIterator; use Shopware\Core\Framework\DataAbstractionLayer\EntityRepository; +use Shopware\Core\Framework\DataAbstractionLayer\Event\EntitySearchedEvent; use Shopware\Core\Framework\DataAbstractionLayer\Search\Criteria; use Shopware\Core\Framework\DataAbstractionLayer\Search\Filter\ContainsFilter; +use Shopware\Core\Framework\DataAbstractionLayer\Search\Filter\EqualsFilter; use Shopware\Core\Framework\Test\IdsCollection; use Shopware\Core\Framework\Test\TestCaseBase\IntegrationTestBehaviour; +use Shopware\Core\System\Country\Aggregate\CountryState\CountryStateCollection; +use Shopware\Core\System\Country\Aggregate\CountryState\CountryStateEntity; use Shopware\Core\System\SystemConfig\SystemConfigCollection; +use Symfony\Component\EventDispatcher\EventDispatcher; /** * @internal @@ -72,19 +78,11 @@ public function testFetchIdAutoIncrement(): void $context = Context::createDefaultContext(); - $ids = new IdsCollection(); - - $builder = new ProductBuilder($ids, 'product1'); - $builder->price(1); - $productRepository->create([$builder->build()], $context); - - $builder = new ProductBuilder($ids, 'product2'); - $builder->price(2); - $productRepository->create([$builder->build()], $context); - - $builder = new ProductBuilder($ids, 'product3'); - $builder->price(3); - $productRepository->create([$builder->build()], $context); + $productRepository->create([ + (new ProductBuilder(new IdsCollection(), 'product1'))->price(1)->build(), + (new ProductBuilder(new IdsCollection(), 'product2'))->price(1)->build(), + (new ProductBuilder(new IdsCollection(), 'product3'))->price(1)->build(), + ], $context); $criteria = new Criteria(); $criteria->setLimit(1); @@ -96,4 +94,187 @@ public function testFetchIdAutoIncrement(): void } static::assertEquals($totalFetchedIds, 3); } + + public function testFetchNotObviousEmptyNextRequestAutoIncrement(): void + { + /** @var EntityRepository $productRepository */ + $productRepository = $this->getContainer()->get('product.repository'); + /** @var EventDispatcher $eventDispatcher */ + $eventDispatcher = $this->getContainer()->get('event_dispatcher'); + + $context = Context::createDefaultContext(); + + $productRepository->create([ + (new ProductBuilder(new IdsCollection(), 'product1'))->price(1)->build(), + (new ProductBuilder(new IdsCollection(), 'product2'))->price(1)->build(), + (new ProductBuilder(new IdsCollection(), 'product3'))->price(1)->build(), + ], $context); + + $criteria = new Criteria(); + $criteria->setTitle('test__product_iteration'); + // 3 products will be fetched in pairs of 2 + // The last response will obviously have 1 (less than 2) item and this already indicates end + // so this should not need an additional search + $criteria->setLimit(2); + + $searchesCount = 0; + $eventDispatcher->addListener(EntitySearchedEvent::class, function (EntitySearchedEvent $event) use (&$searchesCount): void { + if ($event->getCriteria()->getTitle() === 'test__product_iteration') { + ++$searchesCount; + } + }); + + $iterator = new RepositoryIterator($productRepository, $context, $criteria); + + while ($iterator->fetchIds() !== null) { + // fetch all ids and count searches + } + + static::assertSame(2, $searchesCount, '2 searches are enough to fetch 3 products by limit 2'); + + $searchesCount = 0; + $iterator->reset(); // removes increment filter + + while ($iterator->fetch() !== null) { + // fetch all entities and count searches + } + + static::assertSame(2, $searchesCount, '2 searches are enough to fetch 3 products by limit 2'); + } + + public function testFetchNotObviousEmptyNextRequestLimitOffset(): void + { + /** @var EntityRepository $countryStateRepository */ + $countryStateRepository = $this->getContainer()->get('country_state.repository'); + /** @var EventDispatcher $eventDispatcher */ + $eventDispatcher = $this->getContainer()->get('event_dispatcher'); + + $context = Context::createDefaultContext(); + + $criteria = new Criteria(); + $criteria->setTitle('test__country_state_iteration'); + // 16 German country states will be fetched in pairs of 5 + // The last response will obviously have 1 (less than 5) item and this already indicates end + // so this should not need an additional search + $criteria->setLimit(5); + $criteria->addFilter(new EqualsFilter('country.iso', 'DE')); + + $searchesCount = 0; + $eventDispatcher->addListener(EntitySearchedEvent::class, function (EntitySearchedEvent $event) use (&$searchesCount): void { + if ($event->getCriteria()->getTitle() === 'test__country_state_iteration') { + ++$searchesCount; + } + }); + + $iterator = new RepositoryIterator($countryStateRepository, $context, $criteria); + + $countFetchedIds = 0; + + while (($fetchedIds = $iterator->fetchIds()) !== null) { + // fetch all ids and count searches + $countFetchedIds += \count($fetchedIds); + } + + static::assertSame(4, $searchesCount, '4 searches are enough to fetch 16 German country states by limit 5'); + static::assertSame(16, $countFetchedIds); + + $searchesCount = 0; + $countFetchedEntities = 0; + $iterator->reset(); // removes offset + + while (($fetchedEntities = $iterator->fetch()) !== null) { + // fetch all entities and count searches + $countFetchedEntities += $fetchedEntities->count(); + } + + static::assertSame(4, $searchesCount, '4 searches are enough to fetch 16 German country states by limit 5'); + static::assertSame(16, $countFetchedEntities); + } + + public function testAutomaticIdIterationCanBeStartedWithoutManualReset(): void + { + /** @var EntityRepository $countryStateRepository */ + $countryStateRepository = $this->getContainer()->get('country_state.repository'); + + $context = Context::createDefaultContext(); + + $criteria = new Criteria(); + $criteria->setLimit(5); // 16 are expected therefore multiple requests are needed + $criteria->addFilter(new EqualsFilter('country.iso', 'DE')); + + $iterator = new RepositoryIterator($countryStateRepository, $context, $criteria); + $idsRun1 = [...$iterator->iterateIds()]; + $idsRun2 = [...$iterator->iterateIds()]; + + static::assertNotEmpty($idsRun1); + static::assertCount(16, $idsRun1, 'We expected 16 DE states but got less, this could be a array-key issue'); + static::assertEqualsCanonicalizing($idsRun1, $idsRun2); + } + + public function testAutomaticEntityIterationCanBeStartedWithoutManualReset(): void + { + /** @var EntityRepository $countryStateRepository */ + $countryStateRepository = $this->getContainer()->get('country_state.repository'); + + $context = Context::createDefaultContext(); + + $criteria = new Criteria(); + $criteria->setLimit(5); // 16 are expected therefore multiple requests are needed + $criteria->addFilter(new EqualsFilter('country.iso', 'DE')); + + $iterator = new RepositoryIterator($countryStateRepository, $context, $criteria); + $entitiesRun1 = [...$iterator->iterateEntities()]; + $entitiesRun2 = [...$iterator->iterateEntities()]; + + static::assertNotEmpty($entitiesRun1); + static::assertCount(16, $entitiesRun1, 'We expected 16 DE states but got less, this could be a array-key issue'); + + foreach ($entitiesRun1 as $state) { + static::assertInstanceOf(CountryStateEntity::class, $state); + static::assertArrayHasKey($state->getUniqueIdentifier(), $entitiesRun2); + } + } + + public function testAutomaticEntityIterationCanBeStartedAgainEvenAfterAnException(): void + { + /** @var EntityRepository $countryStateRepository */ + $countryStateRepository = $this->getContainer()->get('country_state.repository'); + /** @var EventDispatcher $eventDispatcher */ + $eventDispatcher = $this->getContainer()->get('event_dispatcher'); + + $context = Context::createDefaultContext(); + + $criteria = new Criteria(); + $criteria->setTitle('test__country_state_exception_iteration'); + $criteria->setLimit(5); // 16 are expected therefore multiple requests are needed + $criteria->addFilter(new EqualsFilter('country.iso', 'DE')); + + $searchesCount = 0; + $iterator = new RepositoryIterator($countryStateRepository, $context, $criteria); + + $eventDispatcher->addListener(EntitySearchedEvent::class, function (EntitySearchedEvent $event) use (&$searchesCount): void { + if ($event->getCriteria()->getTitle() === 'test__country_state_exception_iteration') { + ++$searchesCount; + } + + if ($searchesCount == 2) { + throw new \RuntimeException('This is expected', 1716055400); + } + }); + + try { + foreach ($iterator->iterateEntities() as $_) { + // fetch all entities and count searches + } + + static::fail('This should have been failing earlier'); + } catch (\RuntimeException $exception) { + static::assertSame(1716055400, $exception->getCode(), 'This is not the exception we expected'); + } + + // this should now run again normally + $entitiesRun1 = [...$iterator->iterateEntities()]; + static::assertNotEmpty($entitiesRun1); + static::assertCount(16, $entitiesRun1, 'We expected 16 DE states but got less, this could be a array-key issue'); + } } diff --git a/tests/unit/Core/Content/Media/UnusedMediaPurgerTest.php b/tests/unit/Core/Content/Media/UnusedMediaPurgerTest.php index c39d55b014f..e6af1694cc9 100644 --- a/tests/unit/Core/Content/Media/UnusedMediaPurgerTest.php +++ b/tests/unit/Core/Content/Media/UnusedMediaPurgerTest.php @@ -110,7 +110,7 @@ function (Criteria $criteria, Context $context) use ($id1, $id2) { self::assertCount(0, $filters); self::assertEquals(0, $criteria->getOffset()); - self::assertEquals(50, $criteria->getLimit()); + self::assertEquals(2, $criteria->getLimit()); return [$id1, $id2]; }, @@ -123,8 +123,8 @@ function (Criteria $criteria, Context $context) use ($id3, $id4) { $filters = $criteria->getFilters(); self::assertCount(0, $filters); - self::assertEquals(50, $criteria->getOffset()); - self::assertEquals(50, $criteria->getLimit()); + self::assertEquals(2, $criteria->getOffset()); + self::assertEquals(2, $criteria->getLimit()); return [$id3, $id4]; }, @@ -141,7 +141,7 @@ function () { ); $purger = new UnusedMediaPurger($repo, $this->createMock(Connection::class), new EventDispatcher()); - $media = array_merge([], ...iterator_to_array($purger->getNotUsedMedia())); + $media = array_merge([], ...iterator_to_array($purger->getNotUsedMedia(2))); static::assertEquals([$media1, $media2, $media3, $media4], $media); }