From 73c8b538825f077d6c2ca2b4173c8aca1c11178f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 14 Dec 2022 12:28:23 +0100 Subject: [PATCH 1/3] Split some logic from VisitRepository into its own injectable repository --- CHANGELOG.md | 1 + composer.json | 4 +- module/Core/config/dependencies.config.php | 6 +- .../src/Visit/Geolocation/VisitLocator.php | 13 ++-- .../Repository/VisitLocationRepository.php | 74 +++++++++++++++++++ .../VisitLocationRepositoryInterface.php | 27 +++++++ .../src/Visit/Repository/VisitRepository.php | 59 --------------- .../Repository/VisitRepositoryInterface.php | 18 ----- .../VisitLocationRepositoryTest.php | 63 ++++++++++++++++ .../Visit/Repository/VisitRepositoryTest.php | 50 ------------- .../Visit/Geolocation/VisitLocatorTest.php | 11 ++- 11 files changed, 182 insertions(+), 144 deletions(-) create mode 100644 module/Core/src/Visit/Repository/VisitLocationRepository.php create mode 100644 module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php create mode 100644 module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index ce5b82043..94fae122e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Changed * [#1563](https://github.com/shlinkio/shlink/issues/1563) Moved logic to reuse command options to option classes instead of base abstract command classes. * [#1569](https://github.com/shlinkio/shlink/issues/1569) Migrated test doubles from phpspec/prophecy to PHPUnit mocks. +* [#1329](https://github.com/shlinkio/shlink/issues/1329) Split some logic from `VisitRepository` and `ShortUrlRepository` into separated repository classes. ### Deprecated * *Nothing* diff --git a/composer.json b/composer.json index 033920ca7..6d169d8a9 100644 --- a/composer.json +++ b/composer.json @@ -45,7 +45,7 @@ "php-middleware/request-id": "^4.1", "pugx/shortid-php": "^1.1", "ramsey/uuid": "^4.5", - "shlinkio/shlink-common": "dev-main#e2a5bb7 as 5.2", + "shlinkio/shlink-common": "dev-main#107b753 as 5.2", "shlinkio/shlink-config": "dev-main#96c81fb as 2.3", "shlinkio/shlink-event-dispatcher": "^2.6", "shlinkio/shlink-importer": "dev-main#c97662b as 5.0", @@ -135,7 +135,7 @@ "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=80", "infect:ci:db": "@infect:ci:base --coverage=build/coverage-db --min-msi=95 --configuration=infection-db.json5", "infect:ci:api": "@infect:ci:base --coverage=build/coverage-api --min-msi=80 --configuration=infection-api.json5", - "infect:ci:cli": "@infect:ci:base --coverage=build/coverage-cli --min-msi=80 --configuration=infection-cli.json5", + "infect:ci:cli": "@infect:ci:base --coverage=build/coverage-cli --min-msi=90 --configuration=infection-cli.json5", "infect:ci": "@parallel infect:ci:unit infect:ci:db infect:ci:api infect:ci:cli", "infect:test": [ "@parallel test:unit:ci test:db:sqlite:ci test:api:ci", diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 708bb8a32..0a501566f 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -61,6 +61,10 @@ Visit\Geolocation\VisitToLocationHelper::class => ConfigAbstractFactory::class, Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class, Visit\Transformer\OrphanVisitDataTransformer::class => InvokableFactory::class, + Visit\Repository\VisitLocationRepository::class => [ + EntityRepositoryFactory::class, + Visit\Entity\Visit::class, + ], Util\UrlValidator::class => ConfigAbstractFactory::class, Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class, @@ -119,7 +123,7 @@ ShortUrl\Repository\ShortUrlListRepository::class, Options\UrlShortenerOptions::class, ], - Visit\Geolocation\VisitLocator::class => ['em'], + Visit\Geolocation\VisitLocator::class => ['em', Visit\Repository\VisitLocationRepository::class], Visit\Geolocation\VisitToLocationHelper::class => [IpLocationResolverInterface::class], Visit\VisitsStatsHelper::class => ['em'], Tag\TagService::class => ['em'], diff --git a/module/Core/src/Visit/Geolocation/VisitLocator.php b/module/Core/src/Visit/Geolocation/VisitLocator.php index 129002603..4b3b8e22b 100644 --- a/module/Core/src/Visit/Geolocation/VisitLocator.php +++ b/module/Core/src/Visit/Geolocation/VisitLocator.php @@ -8,18 +8,15 @@ use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation; -use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; +use Shlinkio\Shlink\Core\Visit\Repository\VisitLocationRepositoryInterface; use Shlinkio\Shlink\IpGeolocation\Model\Location; class VisitLocator implements VisitLocatorInterface { - private VisitRepositoryInterface $repo; - - public function __construct(private EntityManagerInterface $em) - { - /** @var VisitRepositoryInterface $repo */ - $repo = $em->getRepository(Visit::class); - $this->repo = $repo; + public function __construct( + private readonly EntityManagerInterface $em, + private readonly VisitLocationRepositoryInterface $repo, + ) { } public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void diff --git a/module/Core/src/Visit/Repository/VisitLocationRepository.php b/module/Core/src/Visit/Repository/VisitLocationRepository.php new file mode 100644 index 000000000..6db1a4f88 --- /dev/null +++ b/module/Core/src/Visit/Repository/VisitLocationRepository.php @@ -0,0 +1,74 @@ + + */ + public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('v') + ->from(Visit::class, 'v') + ->where($qb->expr()->isNull('v.visitLocation')); + + return $this->visitsIterableForQuery($qb, $blockSize); + } + + /** + * @return iterable + */ + public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('v') + ->from(Visit::class, 'v') + ->join('v.visitLocation', 'vl') + ->where($qb->expr()->isNotNull('v.visitLocation')) + ->andWhere($qb->expr()->eq('vl.isEmpty', ':isEmpty')) + ->setParameter('isEmpty', true); + + return $this->visitsIterableForQuery($qb, $blockSize); + } + + /** + * @return iterable + */ + public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable + { + $qb = $this->createQueryBuilder('v'); + return $this->visitsIterableForQuery($qb, $blockSize); + } + + private function visitsIterableForQuery(QueryBuilder $qb, int $blockSize): iterable + { + $originalQueryBuilder = $qb->setMaxResults($blockSize) + ->orderBy('v.id', 'ASC'); + $lastId = '0'; + + do { + $qb = (clone $originalQueryBuilder)->andWhere($qb->expr()->gt('v.id', $lastId)); + $iterator = $qb->getQuery()->toIterable(); + $resultsFound = false; + /** @var Visit|null $lastProcessedVisit */ + $lastProcessedVisit = null; + + foreach ($iterator as $key => $visit) { + $resultsFound = true; + $lastProcessedVisit = $visit; + yield $key => $visit; + } + + // As the query is ordered by ID, we can take the last one every time in order to exclude the whole list + $lastId = $lastProcessedVisit?->getId() ?? $lastId; + } while ($resultsFound); + } +} diff --git a/module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php b/module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php new file mode 100644 index 000000000..083d61f22 --- /dev/null +++ b/module/Core/src/Visit/Repository/VisitLocationRepositoryInterface.php @@ -0,0 +1,27 @@ + + */ + public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; + + /** + * @return iterable + */ + public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; + + /** + * @return iterable + */ + public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; +} diff --git a/module/Core/src/Visit/Repository/VisitRepository.php b/module/Core/src/Visit/Repository/VisitRepository.php index af1647c7f..7021e70b5 100644 --- a/module/Core/src/Visit/Repository/VisitRepository.php +++ b/module/Core/src/Visit/Repository/VisitRepository.php @@ -22,65 +22,6 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepositoryInterface { - /** - * @return iterable|Visit[] - */ - public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable - { - $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->select('v') - ->from(Visit::class, 'v') - ->where($qb->expr()->isNull('v.visitLocation')); - - return $this->visitsIterableForQuery($qb, $blockSize); - } - - /** - * @return iterable|Visit[] - */ - public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable - { - $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->select('v') - ->from(Visit::class, 'v') - ->join('v.visitLocation', 'vl') - ->where($qb->expr()->isNotNull('v.visitLocation')) - ->andWhere($qb->expr()->eq('vl.isEmpty', ':isEmpty')) - ->setParameter('isEmpty', true); - - return $this->visitsIterableForQuery($qb, $blockSize); - } - - public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable - { - $qb = $this->createQueryBuilder('v'); - return $this->visitsIterableForQuery($qb, $blockSize); - } - - private function visitsIterableForQuery(QueryBuilder $qb, int $blockSize): iterable - { - $originalQueryBuilder = $qb->setMaxResults($blockSize) - ->orderBy('v.id', 'ASC'); - $lastId = '0'; - - do { - $qb = (clone $originalQueryBuilder)->andWhere($qb->expr()->gt('v.id', $lastId)); - $iterator = $qb->getQuery()->toIterable(); - $resultsFound = false; - /** @var Visit|null $lastProcessedVisit */ - $lastProcessedVisit = null; - - foreach ($iterator as $key => $visit) { - $resultsFound = true; - $lastProcessedVisit = $visit; - yield $key => $visit; - } - - // As the query is ordered by ID, we can take the last one every time in order to exclude the whole list - $lastId = $lastProcessedVisit?->getId() ?? $lastId; - } while ($resultsFound); - } - /** * @return Visit[] */ diff --git a/module/Core/src/Visit/Repository/VisitRepositoryInterface.php b/module/Core/src/Visit/Repository/VisitRepositoryInterface.php index ebc4f4fe1..4e53db2b3 100644 --- a/module/Core/src/Visit/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Visit/Repository/VisitRepositoryInterface.php @@ -11,26 +11,8 @@ use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; -// TODO Split into VisitsListsRepository and VisitsLocationRepository interface VisitRepositoryInterface extends ObjectRepository, EntitySpecificationRepositoryInterface { - public const DEFAULT_BLOCK_SIZE = 10000; - - /** - * @return iterable|Visit[] - */ - public function findUnlocatedVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; - - /** - * @return iterable|Visit[] - */ - public function findVisitsWithEmptyLocation(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; - - /** - * @return iterable|Visit[] - */ - public function findAllVisits(int $blockSize = self::DEFAULT_BLOCK_SIZE): iterable; - /** * @return Visit[] */ diff --git a/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php new file mode 100644 index 000000000..77f4c1e6e --- /dev/null +++ b/module/Core/test-db/Visit/Repository/VisitLocationRepositoryTest.php @@ -0,0 +1,63 @@ +getEntityManager(); + $this->repo = new VisitLocationRepository($em, $em->getClassMetadata(Visit::class)); + } + + /** + * @test + * @dataProvider provideBlockSize + */ + public function findVisitsReturnsProperVisits(int $blockSize): void + { + $shortUrl = ShortUrl::createEmpty(); + $this->getEntityManager()->persist($shortUrl); + + for ($i = 0; $i < 6; $i++) { + $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); + + if ($i >= 2) { + $location = VisitLocation::fromGeolocation(Location::emptyInstance()); + $this->getEntityManager()->persist($location); + $visit->locate($location); + } + + $this->getEntityManager()->persist($visit); + } + $this->getEntityManager()->flush(); + + $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation($blockSize); + $unlocated = $this->repo->findUnlocatedVisits($blockSize); + $all = $this->repo->findAllVisits($blockSize); + + self::assertCount(2, [...$unlocated]); + self::assertCount(4, [...$withEmptyLocation]); + self::assertCount(6, [...$all]); + } + + public function provideBlockSize(): iterable + { + return map(range(1, 10), fn (int $value) => [$value]); + } +} diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index da4758323..eb806208c 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -14,20 +14,16 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; use Shlinkio\Shlink\Core\Visit\Entity\Visit; -use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation; use Shlinkio\Shlink\Core\Visit\Model\Visitor; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; -use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; -use function Functional\map; use function is_string; -use function range; use function sprintf; use function str_pad; @@ -44,52 +40,6 @@ protected function setUp(): void $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager()); } - /** - * @test - * @dataProvider provideBlockSize - */ - public function findVisitsReturnsProperVisits(int $blockSize): void - { - $shortUrl = ShortUrl::createEmpty(); - $this->getEntityManager()->persist($shortUrl); - $countIterable = static function (iterable $results): int { - $resultsCount = 0; - foreach ($results as $value) { - $resultsCount++; - } - - return $resultsCount; - }; - - for ($i = 0; $i < 6; $i++) { - $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); - - if ($i >= 2) { - $location = VisitLocation::fromGeolocation(Location::emptyInstance()); - $this->getEntityManager()->persist($location); - $visit->locate($location); - } - - $this->getEntityManager()->persist($visit); - } - $this->getEntityManager()->flush(); - - $withEmptyLocation = $this->repo->findVisitsWithEmptyLocation($blockSize); - $unlocated = $this->repo->findUnlocatedVisits($blockSize); - $all = $this->repo->findAllVisits($blockSize); - - // Important! assertCount will not work here, as this iterable object loads data dynamically and the count - // is 0 if not iterated - self::assertEquals(2, $countIterable($unlocated)); - self::assertEquals(4, $countIterable($withEmptyLocation)); - self::assertEquals(6, $countIterable($all)); - } - - public function provideBlockSize(): iterable - { - return map(range(1, 10), fn (int $value) => [$value]); - } - /** @test */ public function findVisitsByShortCodeReturnsProperData(): void { diff --git a/module/Core/test/Visit/Geolocation/VisitLocatorTest.php b/module/Core/test/Visit/Geolocation/VisitLocatorTest.php index a39940ae5..ba0d70c43 100644 --- a/module/Core/test/Visit/Geolocation/VisitLocatorTest.php +++ b/module/Core/test/Visit/Geolocation/VisitLocatorTest.php @@ -15,7 +15,7 @@ use Shlinkio\Shlink\Core\Visit\Geolocation\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\Geolocation\VisitLocator; use Shlinkio\Shlink\Core\Visit\Model\Visitor; -use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; +use Shlinkio\Shlink\Core\Visit\Repository\VisitLocationRepositoryInterface; use Shlinkio\Shlink\IpGeolocation\Model\Location; use function count; @@ -28,15 +28,14 @@ class VisitLocatorTest extends TestCase { private VisitLocator $visitService; private MockObject & EntityManager $em; - private MockObject & VisitRepositoryInterface $repo; + private MockObject & VisitLocationRepositoryInterface $repo; protected function setUp(): void { $this->em = $this->createMock(EntityManager::class); - $this->repo = $this->createMock(VisitRepositoryInterface::class); - $this->em->method('getRepository')->with(Visit::class)->willReturn($this->repo); + $this->repo = $this->createMock(VisitLocationRepositoryInterface::class); - $this->visitService = new VisitLocator($this->em); + $this->visitService = new VisitLocator($this->em, $this->repo); } /** @@ -103,7 +102,7 @@ public function visitsWhichCannotBeLocatedAreIgnoredOrLocatedAsEmpty( $this->visitService->{$serviceMethodName}( new class ($isNonLocatableAddress) implements VisitGeolocationHelperInterface { - public function __construct(private bool $isNonLocatableAddress) + public function __construct(private readonly bool $isNonLocatableAddress) { } From 60ef98b836145fb87a689104120f582082e04ef2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 14 Dec 2022 14:38:22 +0100 Subject: [PATCH 2/3] Extracted method to find crawlable short codes to its own query object --- module/Core/config/dependencies.config.php | 4 ++ module/Core/src/Crawling/CrawlingHelper.php | 10 ++-- .../Repository/CrawlableShortCodesQuery.php | 38 ++++++++++++++ .../CrawlableShortCodesQueryInterface.php | 13 +++++ .../Repository/ShortUrlRepository.php | 24 --------- .../ShortUrlRepositoryInterface.php | 2 - .../CrawlableShortCodesQueryTest.php | 50 +++++++++++++++++++ .../Repository/ShortUrlRepositoryTest.php | 33 ------------ .../Core/test/Crawling/CrawlingHelperTest.php | 20 +++----- 9 files changed, 114 insertions(+), 80 deletions(-) create mode 100644 module/Core/src/ShortUrl/Repository/CrawlableShortCodesQuery.php create mode 100644 module/Core/src/ShortUrl/Repository/CrawlableShortCodesQueryInterface.php create mode 100644 module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 0a501566f..48fe3cb2d 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -50,6 +50,10 @@ EntityRepositoryFactory::class, ShortUrl\Entity\ShortUrl::class, ], + ShortUrl\Repository\CrawlableShortCodesQuery::class => [ + EntityRepositoryFactory::class, + ShortUrl\Entity\ShortUrl::class, + ], Tag\TagService::class => ConfigAbstractFactory::class, diff --git a/module/Core/src/Crawling/CrawlingHelper.php b/module/Core/src/Crawling/CrawlingHelper.php index 2c38fabde..958cb96e5 100644 --- a/module/Core/src/Crawling/CrawlingHelper.php +++ b/module/Core/src/Crawling/CrawlingHelper.php @@ -4,20 +4,16 @@ namespace Shlinkio\Shlink\Core\Crawling; -use Doctrine\ORM\EntityManagerInterface; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; -use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\ShortUrl\Repository\CrawlableShortCodesQueryInterface; class CrawlingHelper implements CrawlingHelperInterface { - public function __construct(private EntityManagerInterface $em) + public function __construct(private readonly CrawlableShortCodesQueryInterface $query) { } public function listCrawlableShortCodes(): iterable { - /** @var ShortUrlRepositoryInterface $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - yield from $repo->findCrawlableShortCodes(); + yield from ($this->query)(); } } diff --git a/module/Core/src/ShortUrl/Repository/CrawlableShortCodesQuery.php b/module/Core/src/ShortUrl/Repository/CrawlableShortCodesQuery.php new file mode 100644 index 000000000..7b3821d8a --- /dev/null +++ b/module/Core/src/ShortUrl/Repository/CrawlableShortCodesQuery.php @@ -0,0 +1,38 @@ + + */ + public function __invoke(): iterable + { + $blockSize = 1000; + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('DISTINCT s.shortCode') + ->from(ShortUrl::class, 's') + ->where($qb->expr()->eq('s.crawlable', ':crawlable')) + ->setParameter('crawlable', true) + ->setMaxResults($blockSize); + + $page = 0; + do { + $qbClone = (clone $qb)->setFirstResult($blockSize * $page); + $iterator = $qbClone->getQuery()->toIterable(); + $resultsFound = false; + $page++; + + foreach ($iterator as ['shortCode' => $shortCode]) { + $resultsFound = true; + yield $shortCode; + } + } while ($resultsFound); + } +} diff --git a/module/Core/src/ShortUrl/Repository/CrawlableShortCodesQueryInterface.php b/module/Core/src/ShortUrl/Repository/CrawlableShortCodesQueryInterface.php new file mode 100644 index 000000000..9e8211e54 --- /dev/null +++ b/module/Core/src/ShortUrl/Repository/CrawlableShortCodesQueryInterface.php @@ -0,0 +1,13 @@ + + */ + public function __invoke(): iterable; +} diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php index 216c4579f..5e95f7772 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlRepository.php @@ -190,28 +190,4 @@ private function whereDomainIs(QueryBuilder $qb, ?string $domain): void $qb->andWhere($qb->expr()->isNull('s.domain')); } } - - public function findCrawlableShortCodes(): iterable - { - $blockSize = 1000; - $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->select('DISTINCT s.shortCode') - ->from(ShortUrl::class, 's') - ->where($qb->expr()->eq('s.crawlable', ':crawlable')) - ->setParameter('crawlable', true) - ->setMaxResults($blockSize); - - $page = 0; - do { - $qbClone = (clone $qb)->setFirstResult($blockSize * $page); - $iterator = $qbClone->getQuery()->toIterable(); - $resultsFound = false; - $page++; - - foreach ($iterator as ['shortCode' => $shortCode]) { - $resultsFound = true; - yield $shortCode; - } - } while ($resultsFound); - } } diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php index ad5e3a5dd..cc574ac57 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlRepositoryInterface.php @@ -25,6 +25,4 @@ public function shortCodeIsInUseWithLock(ShortUrlIdentifier $identifier, ?Specif public function findOneMatching(ShortUrlCreation $meta): ?ShortUrl; public function findOneByImportedUrl(ImportedShlinkUrl $url): ?ShortUrl; - - public function findCrawlableShortCodes(): iterable; } diff --git a/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php b/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php new file mode 100644 index 000000000..04c670fa1 --- /dev/null +++ b/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php @@ -0,0 +1,50 @@ +getEntityManager(); + $this->query = new CrawlableShortCodesQuery($em, $em->getClassMetadata(ShortUrl::class)); + } + + /** @test */ + public function invokingQueryReturnsExpectedResult(): void + { + $createShortUrl = fn (bool $crawlable) => ShortUrl::create( + ShortUrlCreation::fromRawData(['crawlable' => $crawlable, 'longUrl' => 'foo.com']), + ); + + $shortUrl1 = $createShortUrl(true); + $this->getEntityManager()->persist($shortUrl1); + $shortUrl2 = $createShortUrl(false); + $this->getEntityManager()->persist($shortUrl2); + $shortUrl3 = $createShortUrl(true); + $this->getEntityManager()->persist($shortUrl3); + $shortUrl4 = $createShortUrl(true); + $this->getEntityManager()->persist($shortUrl4); + $shortUrl5 = $createShortUrl(false); + $this->getEntityManager()->persist($shortUrl5); + $this->getEntityManager()->flush(); + + $results = [...($this->query)()]; + + self::assertCount(3, $results); + self::assertContains($shortUrl1->getShortCode(), $results); + self::assertContains($shortUrl3->getShortCode(), $results); + self::assertContains($shortUrl4->getShortCode(), $results); + self::assertNotContains($shortUrl2->getShortCode(), $results); + self::assertNotContains($shortUrl5->getShortCode(), $results); + } +} diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php index a477eff84..c842bcb4b 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlRepositoryTest.php @@ -391,37 +391,4 @@ public function importedShortUrlsAreFoundWhenExpected(): void self::assertNull($this->repo->findOneByImportedUrl($buildImported('my-cool-slug', 'doma.in'))); self::assertNull($this->repo->findOneByImportedUrl($buildImported('another-slug'))); } - - /** @test */ - public function findCrawlableShortCodesReturnsExpectedResult(): void - { - $createShortUrl = fn (bool $crawlable) => ShortUrl::create( - ShortUrlCreation::fromRawData(['crawlable' => $crawlable, 'longUrl' => 'foo.com']), - ); - - $shortUrl1 = $createShortUrl(true); - $this->getEntityManager()->persist($shortUrl1); - $shortUrl2 = $createShortUrl(false); - $this->getEntityManager()->persist($shortUrl2); - $shortUrl3 = $createShortUrl(true); - $this->getEntityManager()->persist($shortUrl3); - $shortUrl4 = $createShortUrl(true); - $this->getEntityManager()->persist($shortUrl4); - $shortUrl5 = $createShortUrl(false); - $this->getEntityManager()->persist($shortUrl5); - $this->getEntityManager()->flush(); - - $iterable = $this->repo->findCrawlableShortCodes(); - $results = []; - foreach ($iterable as $shortCode) { - $results[] = $shortCode; - } - - self::assertCount(3, $results); - self::assertContains($shortUrl1->getShortCode(), $results); - self::assertContains($shortUrl3->getShortCode(), $results); - self::assertContains($shortUrl4->getShortCode(), $results); - self::assertNotContains($shortUrl2->getShortCode(), $results); - self::assertNotContains($shortUrl5->getShortCode(), $results); - } } diff --git a/module/Core/test/Crawling/CrawlingHelperTest.php b/module/Core/test/Crawling/CrawlingHelperTest.php index 1843d35c2..295b7ec33 100644 --- a/module/Core/test/Crawling/CrawlingHelperTest.php +++ b/module/Core/test/Crawling/CrawlingHelperTest.php @@ -4,34 +4,26 @@ namespace ShlinkioTest\Shlink\Core\Crawling; -use Doctrine\ORM\EntityManagerInterface; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Crawling\CrawlingHelper; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; -use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\ShortUrl\Repository\CrawlableShortCodesQueryInterface; class CrawlingHelperTest extends TestCase { private CrawlingHelper $helper; - private MockObject & EntityManagerInterface $em; + private MockObject & CrawlableShortCodesQueryInterface $query; protected function setUp(): void { - $this->em = $this->createMock(EntityManagerInterface::class); - $this->helper = new CrawlingHelper($this->em); + $this->query = $this->createMock(CrawlableShortCodesQueryInterface::class); + $this->helper = new CrawlingHelper($this->query); } /** @test */ public function listCrawlableShortCodesDelegatesIntoRepository(): void { - $repo = $this->createMock(ShortUrlRepositoryInterface::class); - $repo->expects($this->once())->method('findCrawlableShortCodes')->willReturn([]); - $this->em->expects($this->once())->method('getRepository')->with(ShortUrl::class)->willReturn($repo); - - $result = $this->helper->listCrawlableShortCodes(); - foreach ($result as $shortCode) { - // $result is a generator and therefore, it needs to be iterated - } + $this->query->expects($this->once())->method('__invoke')->willReturn([]); + [...$this->helper->listCrawlableShortCodes()]; } } From 30edfdbdc555191aa4d727cd13157e2f64c56738 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 14 Dec 2022 15:01:00 +0100 Subject: [PATCH 3/3] Updated docker images to PHP 8.2 --- CHANGELOG.md | 2 +- Dockerfile | 4 ++-- data/infra/php.Dockerfile | 6 ++++-- data/infra/roadrunner.Dockerfile | 6 ++++-- data/infra/swoole.Dockerfile | 6 ++++-- 5 files changed, 15 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 94fae122e..6031a7729 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,7 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1599](https://github.com/shlinkio/shlink/issues/1599) Added support for credentials on redis DSNs, either only password, or both username and password. * [#1616](https://github.com/shlinkio/shlink/issues/1616) Added support to import orphan visits when importing short URLs from another Shlink instance. * [#1519](https://github.com/shlinkio/shlink/issues/1519) Allowing to search short URLs by default domain. -* [#1555](https://github.com/shlinkio/shlink/issues/1555) Added full support for PHP 8.2, pdating the dockr image to this version. +* [#1555](https://github.com/shlinkio/shlink/issues/1555) and [#1625](https://github.com/shlinkio/shlink/issues/1625) Added full support for PHP 8.2, updating the docker image to this version. ### Changed * [#1563](https://github.com/shlinkio/shlink/issues/1563) Moved logic to reuse command options to option classes instead of base abstract command classes. diff --git a/Dockerfile b/Dockerfile index 90c2ba6b0..8c38653b1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,4 @@ -FROM php:8.1.13-alpine3.17 as base +FROM php:8.2-alpine3.17 as base ARG SHLINK_VERSION=latest ENV SHLINK_VERSION ${SHLINK_VERSION} @@ -15,7 +15,7 @@ WORKDIR /etc/shlink # Install required PHP extensions RUN \ # Temp install dev dependencies needed to compile the extensions - apk add --no-cache --virtual .dev-deps sqlite-dev postgresql-dev icu-dev libzip-dev zlib-dev libpng-dev && \ + apk add --no-cache --virtual .dev-deps sqlite-dev postgresql-dev icu-dev libzip-dev zlib-dev libpng-dev linux-headers && \ docker-php-ext-install -j"$(nproc)" pdo_mysql pdo_pgsql intl calendar sockets bcmath zip gd && \ apk add --no-cache sqlite-libs && \ docker-php-ext-install -j"$(nproc)" pdo_sqlite && \ diff --git a/data/infra/php.Dockerfile b/data/infra/php.Dockerfile index c43b21cbd..90ccab23d 100644 --- a/data/infra/php.Dockerfile +++ b/data/infra/php.Dockerfile @@ -1,4 +1,4 @@ -FROM php:8.1.13-fpm-alpine3.17 +FROM php:8.2-fpm-alpine3.17 MAINTAINER Alejandro Celaya ENV APCU_VERSION 5.1.21 @@ -31,7 +31,9 @@ RUN docker-php-ext-install gd RUN apk add --no-cache postgresql-dev RUN docker-php-ext-install pdo_pgsql -RUN docker-php-ext-install sockets +RUN apk add --no-cache --virtual .phpize-deps $PHPIZE_DEPS linux-headers && \ + docker-php-ext-install sockets && \ + apk del .phpize-deps RUN docker-php-ext-install bcmath # Install APCu extension diff --git a/data/infra/roadrunner.Dockerfile b/data/infra/roadrunner.Dockerfile index f019d9694..383099e4a 100644 --- a/data/infra/roadrunner.Dockerfile +++ b/data/infra/roadrunner.Dockerfile @@ -1,4 +1,4 @@ -FROM php:8.1.13-alpine3.17 +FROM php:8.2-alpine3.17 MAINTAINER Alejandro Celaya ENV APCU_VERSION 5.1.21 @@ -31,7 +31,9 @@ RUN docker-php-ext-install gd RUN apk add --no-cache postgresql-dev RUN docker-php-ext-install pdo_pgsql -RUN docker-php-ext-install sockets +RUN apk add --no-cache --virtual .phpize-deps $PHPIZE_DEPS linux-headers && \ + docker-php-ext-install sockets && \ + apk del .phpize-deps RUN docker-php-ext-install bcmath # Install APCu extension diff --git a/data/infra/swoole.Dockerfile b/data/infra/swoole.Dockerfile index 68fa0db24..21e7d95f0 100644 --- a/data/infra/swoole.Dockerfile +++ b/data/infra/swoole.Dockerfile @@ -1,4 +1,4 @@ -FROM php:8.1.13-alpine3.17 +FROM php:8.2-alpine3.17 MAINTAINER Alejandro Celaya ENV APCU_VERSION 5.1.21 @@ -33,7 +33,9 @@ RUN docker-php-ext-install gd RUN apk add --no-cache postgresql-dev RUN docker-php-ext-install pdo_pgsql -RUN docker-php-ext-install sockets +RUN apk add --no-cache --virtual .phpize-deps $PHPIZE_DEPS linux-headers && \ + docker-php-ext-install sockets && \ + apk del .phpize-deps RUN docker-php-ext-install bcmath # Install APCu extension