diff --git a/module/CLI/src/Command/Visit/LocateVisitsCommand.php b/module/CLI/src/Command/Visit/LocateVisitsCommand.php index 935b9eee..89a8aa71 100644 --- a/module/CLI/src/Command/Visit/LocateVisitsCommand.php +++ b/module/CLI/src/Command/Visit/LocateVisitsCommand.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; +use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocatorInterface; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -27,7 +28,7 @@ use Throwable; use function sprintf; -class LocateVisitsCommand extends AbstractLockedCommand +class LocateVisitsCommand extends AbstractLockedCommand implements VisitGeolocationHelperInterface { public const NAME = 'visit:locate'; @@ -73,20 +74,13 @@ class LocateVisitsCommand extends AbstractLockedCommand { $this->io = new SymfonyStyle($input, $output); $retry = $input->getOption('retry'); - $geolocateVisit = [$this, 'getGeolocationDataForVisit']; - $notifyVisitWithLocation = static function (VisitLocation $location) use ($output): void { - $message = ! $location->isEmpty() - ? sprintf(' [Address located in "%s"]', $location->getCountryName()) - : ' [Address not found]'; - $output->writeln($message); - }; try { $this->checkDbUpdate(); - $this->visitLocator->locateUnlocatedVisits($geolocateVisit, $notifyVisitWithLocation); + $this->visitLocator->locateUnlocatedVisits($this); if ($retry) { - $this->visitLocator->locateVisitsWithEmptyLocation($geolocateVisit, $notifyVisitWithLocation); + $this->visitLocator->locateVisitsWithEmptyLocation($this); } $this->io->success('Finished processing all IPs'); @@ -101,7 +95,10 @@ class LocateVisitsCommand extends AbstractLockedCommand } } - public function getGeolocationDataForVisit(Visit $visit): Location + /** + * @throws IpCannotBeLocatedException + */ + public function geolocateVisit(Visit $visit): Location { if (! $visit->hasRemoteAddr()) { $this->io->writeln( @@ -130,6 +127,14 @@ class LocateVisitsCommand extends AbstractLockedCommand } } + public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void + { + $message = ! $visitLocation->isEmpty() + ? sprintf(' [Address located in "%s"]', $visitLocation->getCountryName()) + : ' [Address not found]'; + $this->io->writeln($message); + } + private function checkDbUpdate(): void { try { diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 94cd0bd1..30be9846 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocator; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -24,7 +25,6 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Tester\CommandTester; use Symfony\Component\Lock; -use function array_shift; use function sprintf; class LocateVisitsCommandTest extends TestCase @@ -68,13 +68,7 @@ class LocateVisitsCommandTest extends TestCase $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $this->invokeHelperMethods($visit, $location), ); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( Location::emptyInstance(), @@ -98,13 +92,7 @@ class LocateVisitsCommandTest extends TestCase $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $this->invokeHelperMethods($visit, $location), ); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willReturn( Location::emptyInstance(), @@ -137,13 +125,7 @@ class LocateVisitsCommandTest extends TestCase $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( - function (array $args) use ($visit, $location): void { - $firstCallback = array_shift($args); - $firstCallback($visit); - - $secondCallback = array_shift($args); - $secondCallback($location, $visit); - }, + $this->invokeHelperMethods($visit, $location), ); $resolveIpLocation = $this->ipResolver->resolveIpLocation(Argument::any())->willThrow(WrongIpException::class); @@ -156,6 +138,17 @@ class LocateVisitsCommandTest extends TestCase $resolveIpLocation->shouldHaveBeenCalledOnce(); } + private function invokeHelperMethods(Visit $visit, VisitLocation $location): callable + { + return function (array $args) use ($visit, $location): void { + /** @var VisitGeolocationHelperInterface $helper */ + [$helper] = $args; + + $helper->geolocateVisit($visit); + $helper->onVisitLocated($location, $visit); + }; + } + /** @test */ public function noActionIsPerformedIfLockIsAcquired(): void { diff --git a/module/Core/src/Visit/VisitGeolocationHelperInterface.php b/module/Core/src/Visit/VisitGeolocationHelperInterface.php new file mode 100644 index 00000000..95cca4a7 --- /dev/null +++ b/module/Core/src/Visit/VisitGeolocationHelperInterface.php @@ -0,0 +1,20 @@ +em = $em; + + /** @var VisitRepositoryInterface $repo */ + $repo = $em->getRepository(Visit::class); + $this->repo = $repo; } - public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void + public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void { - /** @var VisitRepository $repo */ - $repo = $this->em->getRepository(Visit::class); - $this->locateVisits($repo->findUnlocatedVisits(), $geolocateVisit, $notifyVisitWithLocation); + $this->locateVisits($this->repo->findUnlocatedVisits(), $helper); } - public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void + public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void { - /** @var VisitRepository $repo */ - $repo = $this->em->getRepository(Visit::class); - $this->locateVisits($repo->findVisitsWithEmptyLocation(), $geolocateVisit, $notifyVisitWithLocation); + $this->locateVisits($this->repo->findVisitsWithEmptyLocation(), $helper); } /** * @param iterable|Visit[] $results */ - private function locateVisits(iterable $results, callable $geolocateVisit, callable $notifyVisitWithLocation): void + private function locateVisits(iterable $results, VisitGeolocationHelperInterface $helper): void { $count = 0; $persistBlock = 200; @@ -46,8 +47,7 @@ class VisitLocator implements VisitLocatorInterface $count++; try { - /** @var Location $location */ - $location = $geolocateVisit($visit); + $location = $helper->geolocateVisit($visit); } catch (IpCannotBeLocatedException $e) { if (! $e->isNonLocatableAddress()) { // Skip if the visit's IP could not be located because of an error @@ -59,7 +59,7 @@ class VisitLocator implements VisitLocatorInterface } $location = new VisitLocation($location); - $this->locateVisit($visit, $location, $notifyVisitWithLocation); + $this->locateVisit($visit, $location, $helper); // Flush and clear after X iterations if ($count % $persistBlock === 0) { @@ -72,11 +72,11 @@ class VisitLocator implements VisitLocatorInterface $this->em->clear(); } - private function locateVisit(Visit $visit, VisitLocation $location, callable $notifyVisitWithLocation): void + private function locateVisit(Visit $visit, VisitLocation $location, VisitGeolocationHelperInterface $helper): void { $visit->locate($location); $this->em->persist($visit); - $notifyVisitWithLocation($location, $visit); + $helper->onVisitLocated($location, $visit); } } diff --git a/module/Core/src/Visit/VisitLocatorInterface.php b/module/Core/src/Visit/VisitLocatorInterface.php index ea06c1b1..6e06d296 100644 --- a/module/Core/src/Visit/VisitLocatorInterface.php +++ b/module/Core/src/Visit/VisitLocatorInterface.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\Visit; interface VisitLocatorInterface { - public function locateUnlocatedVisits(callable $geolocateVisit, callable $notifyVisitWithLocation): void; + public function locateUnlocatedVisits(VisitGeolocationHelperInterface $helper): void; - public function locateVisitsWithEmptyLocation(callable $geolocateVisit, callable $notifyVisitWithLocation): void; + public function locateVisitsWithEmptyLocation(VisitGeolocationHelperInterface $helper): void; } diff --git a/module/Core/test/Visit/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php index 9e223e15..1972860b 100644 --- a/module/Core/test/Visit/VisitLocatorTest.php +++ b/module/Core/test/Visit/VisitLocatorTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Visit; use Doctrine\ORM\EntityManager; use Exception; +use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; @@ -14,7 +15,8 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Exception\IpCannotBeLocatedException; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Repository\VisitRepository; +use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface; +use Shlinkio\Shlink\Core\Visit\VisitGeolocationHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitLocator; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -30,10 +32,14 @@ class VisitLocatorTest extends TestCase { private VisitLocator $visitService; private ObjectProphecy $em; + private ObjectProphecy $repo; public function setUp(): void { $this->em = $this->prophesize(EntityManager::class); + $this->repo = $this->prophesize(VisitRepositoryInterface::class); + $this->em->getRepository(Visit::class)->willReturn($this->repo->reveal()); + $this->visitService = new VisitLocator($this->em->reveal()); } @@ -45,9 +51,7 @@ class VisitLocatorTest extends TestCase fn (int $i) => new Visit(new ShortUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()), ); - $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); - $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { }); @@ -56,15 +60,22 @@ class VisitLocatorTest extends TestCase $clear = $this->em->clear()->will(function (): void { }); - $this->visitService->locateUnlocatedVisits(fn () => Location::emptyInstance(), function (): void { - $args = func_get_args(); + $this->visitService->locateUnlocatedVisits(new class implements VisitGeolocationHelperInterface { + public function geolocateVisit(Visit $visit): Location + { + return Location::emptyInstance(); + } - $this->assertInstanceOf(VisitLocation::class, array_shift($args)); - $this->assertInstanceOf(Visit::class, array_shift($args)); + public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void + { + $args = func_get_args(); + + Assert::assertInstanceOf(VisitLocation::class, array_shift($args)); + Assert::assertInstanceOf(Visit::class, array_shift($args)); + } }); $findUnlocatedVisits->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledTimes(count($unlocatedVisits)); $flush->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); $clear->shouldHaveBeenCalledTimes(floor(count($unlocatedVisits) / 200) + 1); @@ -80,9 +91,7 @@ class VisitLocatorTest extends TestCase new Visit(new ShortUrl('foo'), Visitor::emptyInstance()), ]; - $repo = $this->prophesize(VisitRepository::class); - $findUnlocatedVisits = $repo->findUnlocatedVisits()->willReturn($unlocatedVisits); - $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + $findUnlocatedVisits = $this->repo->findUnlocatedVisits()->willReturn($unlocatedVisits); $persist = $this->em->persist(Argument::type(Visit::class))->will(function (): void { }); @@ -91,15 +100,29 @@ class VisitLocatorTest extends TestCase $clear = $this->em->clear()->will(function (): void { }); - $this->visitService->locateUnlocatedVisits(function () use ($isNonLocatableAddress): void { - throw $isNonLocatableAddress - ? new IpCannotBeLocatedException('Cannot be located') - : IpCannotBeLocatedException::forError(new Exception('')); - }, static function (): void { - }); + $this->visitService->locateUnlocatedVisits( + new class ($isNonLocatableAddress) implements VisitGeolocationHelperInterface { + private bool $isNonLocatableAddress; + + public function __construct(bool $isNonLocatableAddress) + { + $this->isNonLocatableAddress = $isNonLocatableAddress; + } + + public function geolocateVisit(Visit $visit): Location + { + throw $this->isNonLocatableAddress + ? new IpCannotBeLocatedException('Cannot be located') + : IpCannotBeLocatedException::forError(new Exception('')); + } + + public function onVisitLocated(VisitLocation $visitLocation, Visit $visit): void + { + } + }, + ); $findUnlocatedVisits->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledTimes($isNonLocatableAddress ? 1 : 0); $flush->shouldHaveBeenCalledOnce(); $clear->shouldHaveBeenCalledOnce();