From 327d35fe5752aa1d8b01df55880f646a33a6dcde Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 11:46:54 +0100 Subject: [PATCH 01/23] Created DTO used to transfer props needed to uniquely identify a short URL --- CHANGELOG.md | 3 +- .../Command/ShortUrl/ResolveUrlCommand.php | 5 +- .../ShortUrl/DeleteShortUrlCommandTest.php | 3 +- .../ShortUrl/ResolveUrlCommandTest.php | 13 ++++-- .../src/Action/AbstractTrackingAction.php | 5 +- module/Core/src/Action/QrCodeAction.php | 9 ++-- .../Exception/ShortUrlNotFoundException.php | 5 +- module/Core/src/Model/ShortUrlIdentifier.php | 46 +++++++++++++++++++ .../ShortUrl/DeleteShortUrlService.php | 3 +- .../Service/ShortUrl/FindShortCodeTrait.php | 13 +++--- .../src/Service/ShortUrl/ShortUrlResolver.php | 13 +++--- .../ShortUrl/ShortUrlResolverInterface.php | 5 +- module/Core/src/Service/ShortUrlService.php | 5 +- module/Core/src/Service/VisitsTracker.php | 3 +- .../src/Service/VisitsTrackerInterface.php | 4 +- module/Core/test/Action/PixelActionTest.php | 3 +- module/Core/test/Action/QrCodeActionTest.php | 16 ++++--- .../Core/test/Action/RedirectActionTest.php | 9 ++-- .../ShortUrlNotFoundExceptionTest.php | 3 +- .../ShortUrl/DeleteShortUrlServiceTest.php | 8 ++-- .../Service/ShortUrl/ShortUrlResolverTest.php | 9 ++-- .../Core/test/Service/ShortUrlServiceTest.php | 10 ++-- .../Action/ShortUrl/ResolveShortUrlAction.php | 5 +- .../ShortUrl/ResolveShortUrlActionTest.php | 3 +- 24 files changed, 134 insertions(+), 67 deletions(-) create mode 100644 module/Core/src/Model/ShortUrlIdentifier.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 9fe5ac60..9d13de92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Changed * [#577](https://github.com/shlinkio/shlink/issues/577) Wrapped params used to customize short URL lists into a DTO with implicit validation. -* [#620](https://github.com/shlinkio/shlink/issues/620) "Controlled" errors (like validation errors and such) will no longer be logged with error level, preventing logs to be polluted. #### Deprecated @@ -25,7 +24,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed -* *Nothing* +* [#620](https://github.com/shlinkio/shlink/issues/620) Ensured "controlled" errors (like validation errors and such) won't be logged with error level, preventing logs to be polluted. ## 2.0.3 - 2020-01-27 diff --git a/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php b/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php index 22f384db..e6fdef3d 100644 --- a/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\ShortUrl; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -54,11 +55,9 @@ class ResolveUrlCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): ?int { $io = new SymfonyStyle($input, $output); - $shortCode = $input->getArgument('shortCode'); - $domain = $input->getOption('domain'); try { - $url = $this->urlResolver->shortCodeToShortUrl($shortCode, $domain); + $url = $this->urlResolver->resolveShortUrl(ShortUrlIdentifier::fromCli($input)); $output->writeln(sprintf('Long URL: %s', $url->getLongUrl())); return ExitCodes::EXIT_SUCCESS; } catch (ShortUrlNotFoundException $e) { diff --git a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php index 7fd727ca..9d9a11c1 100644 --- a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php @@ -9,6 +9,7 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\ShortUrl\DeleteShortUrlCommand; use Shlinkio\Shlink\Core\Exception; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\DeleteShortUrlServiceInterface; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; @@ -56,7 +57,7 @@ class DeleteShortUrlCommandTest extends TestCase { $shortCode = 'abc123'; $deleteByShortCode = $this->service->deleteByShortCode($shortCode, false)->willThrow( - Exception\ShortUrlNotFoundException::fromNotFoundShortCode($shortCode), + Exception\ShortUrlNotFoundException::fromNotFound(new ShortUrlIdentifier($shortCode)), ); $this->commandTester->execute(['shortCode' => $shortCode]); diff --git a/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php index cb3e658d..7c307252 100644 --- a/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ResolveUrlCommandTest.php @@ -9,6 +9,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\ShortUrl\ResolveUrlCommand; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; @@ -38,8 +39,8 @@ class ResolveUrlCommandTest extends TestCase $shortCode = 'abc123'; $expectedUrl = 'http://domain.com/foo/bar'; $shortUrl = new ShortUrl($expectedUrl); - $this->urlResolver->shortCodeToShortUrl($shortCode, null)->willReturn($shortUrl) - ->shouldBeCalledOnce(); + $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode))->willReturn($shortUrl) + ->shouldBeCalledOnce(); $this->commandTester->execute(['shortCode' => $shortCode]); $output = $this->commandTester->getDisplay(); @@ -49,9 +50,11 @@ class ResolveUrlCommandTest extends TestCase /** @test */ public function incorrectShortCodeOutputsErrorMessage(): void { - $shortCode = 'abc123'; - $this->urlResolver->shortCodeToShortUrl($shortCode, null) - ->willThrow(ShortUrlNotFoundException::fromNotFoundShortCode($shortCode)) + $identifier = new ShortUrlIdentifier('abc123'); + $shortCode = $identifier->shortCode(); + + $this->urlResolver->resolveShortUrl($identifier) + ->willThrow(ShortUrlNotFoundException::fromNotFound($identifier)) ->shouldBeCalledOnce(); $this->commandTester->execute(['shortCode' => $shortCode]); diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 4e45d9cd..3655bd32 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -13,6 +13,7 @@ use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\AppOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; @@ -45,12 +46,12 @@ abstract class AbstractTrackingAction implements MiddlewareInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $shortCode = $request->getAttribute('shortCode', ''); - $domain = $request->getUri()->getAuthority(); + $identifier = ShortUrlIdentifier::fromRequest($request); $query = $request->getQueryParams(); $disableTrackParam = $this->appOptions->getDisableTrackParam(); try { - $url = $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, $domain); + $url = $this->urlResolver->resolveEnabledShortUrl($identifier); // Track visit to this short code if ($disableTrackParam === null || ! array_key_exists($disableTrackParam, $query)) { diff --git a/module/Core/src/Action/QrCodeAction.php b/module/Core/src/Action/QrCodeAction.php index c302a58d..df06edbd 100644 --- a/module/Core/src/Action/QrCodeAction.php +++ b/module/Core/src/Action/QrCodeAction.php @@ -14,6 +14,7 @@ use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Shlinkio\Shlink\Common\Response\QrCodeResponse; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; class QrCodeAction implements MiddlewareInterface @@ -38,18 +39,16 @@ class QrCodeAction implements MiddlewareInterface public function process(Request $request, RequestHandlerInterface $handler): Response { - // Make sure the short URL exists for this short code - $shortCode = $request->getAttribute('shortCode'); - $domain = $request->getUri()->getAuthority(); + $identifier = ShortUrlIdentifier::fromRequest($request); try { - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, $domain); + $this->urlResolver->resolveEnabledShortUrl($identifier); } catch (ShortUrlNotFoundException $e) { $this->logger->warning('An error occurred while creating QR code. {e}', ['e' => $e]); return $handler->handle($request); } - $path = $this->router->generateUri(RedirectAction::class, ['shortCode' => $shortCode]); + $path = $this->router->generateUri(RedirectAction::class, ['shortCode' => $identifier->shortCode()]); $size = $this->getSizeParam($request); $qrCode = new QrCode((string) $request->getUri()->withPath($path)->withQuery('')); diff --git a/module/Core/src/Exception/ShortUrlNotFoundException.php b/module/Core/src/Exception/ShortUrlNotFoundException.php index e68e55ed..0ae29da5 100644 --- a/module/Core/src/Exception/ShortUrlNotFoundException.php +++ b/module/Core/src/Exception/ShortUrlNotFoundException.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Exception; use Fig\Http\Message\StatusCodeInterface; use Mezzio\ProblemDetails\Exception\CommonProblemDetailsExceptionTrait; use Mezzio\ProblemDetails\Exception\ProblemDetailsExceptionInterface; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use function sprintf; @@ -17,8 +18,10 @@ class ShortUrlNotFoundException extends DomainException implements ProblemDetail private const TITLE = 'Short URL not found'; private const TYPE = 'INVALID_SHORTCODE'; - public static function fromNotFoundShortCode(string $shortCode, ?string $domain = null): self + public static function fromNotFound(ShortUrlIdentifier $identifier): self { + $shortCode = $identifier->shortCode(); + $domain = $identifier->domain(); $suffix = $domain === null ? '' : sprintf(' for domain "%s"', $domain); $e = new self(sprintf('No URL found with short code "%s"%s', $shortCode, $suffix)); diff --git a/module/Core/src/Model/ShortUrlIdentifier.php b/module/Core/src/Model/ShortUrlIdentifier.php new file mode 100644 index 00000000..dae3a15b --- /dev/null +++ b/module/Core/src/Model/ShortUrlIdentifier.php @@ -0,0 +1,46 @@ +shortCode = $shortCode; + $this->domain = $domain; + } + + public static function fromRequest(ServerRequestInterface $request): self + { + $shortCode = $request->getAttribute('shortCode', ''); + $domain = $request->getQueryParams()['domain'] ?? null; + + return new self($shortCode, $domain); + } + + public static function fromCli(InputInterface $input): self + { + $shortCode = $input->getArgument('shortCode'); + $domain = $input->getOption('domain'); + + return new self($shortCode, $domain); + } + + public function shortCode(): string + { + return $this->shortCode; + } + + public function domain(): ?string + { + return $this->domain; + } +} diff --git a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php index c9624bf3..2b4ad067 100644 --- a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php +++ b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options\DeleteShortUrlsOptions; class DeleteShortUrlService implements DeleteShortUrlServiceInterface @@ -28,7 +29,7 @@ class DeleteShortUrlService implements DeleteShortUrlServiceInterface */ public function deleteByShortCode(string $shortCode, bool $ignoreThreshold = false): void { - $shortUrl = $this->findByShortCode($this->em, $shortCode); + $shortUrl = $this->findByShortCode($this->em, new ShortUrlIdentifier($shortCode)); if (! $ignoreThreshold && $this->isThresholdReached($shortUrl)) { throw Exception\DeleteShortUrlException::fromVisitsThreshold( $this->deleteShortUrlsOptions->getVisitsThreshold(), diff --git a/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php b/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php index 95009704..654d587f 100644 --- a/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php +++ b/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php @@ -7,20 +7,21 @@ namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; trait FindShortCodeTrait { /** * @throws ShortUrlNotFoundException */ - private function findByShortCode(EntityManagerInterface $em, string $shortCode): ShortUrl + private function findByShortCode(EntityManagerInterface $em, ShortUrlIdentifier $identifier): ShortUrl { - /** @var ShortUrl|null $shortUrl */ - $shortUrl = $em->getRepository(ShortUrl::class)->findOneBy([ - 'shortCode' => $shortCode, - ]); + /** @var ShortUrlRepositoryInterface $repo */ + $repo = $em->getRepository(ShortUrl::class); + $shortUrl = $repo->findOneByShortCode($identifier->shortCode(), $identifier->domain()); if ($shortUrl === null) { - throw ShortUrlNotFoundException::fromNotFoundShortCode($shortCode); + throw ShortUrlNotFoundException::fromNotFound($identifier); } return $shortUrl; diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index 62f30c11..d7a71b1c 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; class ShortUrlResolver implements ShortUrlResolverInterface @@ -21,13 +22,13 @@ class ShortUrlResolver implements ShortUrlResolverInterface /** * @throws ShortUrlNotFoundException */ - public function shortCodeToShortUrl(string $shortCode, ?string $domain = null): ShortUrl + public function resolveShortUrl(ShortUrlIdentifier $identifier): ShortUrl { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOneByShortCode($shortCode, $domain); + $shortUrl = $shortUrlRepo->findOneByShortCode($identifier->shortCode(), $identifier->domain()); if ($shortUrl === null) { - throw ShortUrlNotFoundException::fromNotFoundShortCode($shortCode, $domain); + throw ShortUrlNotFoundException::fromNotFound($identifier); } return $shortUrl; @@ -36,11 +37,11 @@ class ShortUrlResolver implements ShortUrlResolverInterface /** * @throws ShortUrlNotFoundException */ - public function shortCodeToEnabledShortUrl(string $shortCode, ?string $domain = null): ShortUrl + public function resolveEnabledShortUrl(ShortUrlIdentifier $identifier): ShortUrl { - $shortUrl = $this->shortCodeToShortUrl($shortCode, $domain); + $shortUrl = $this->resolveShortUrl($identifier); if (! $shortUrl->isEnabled()) { - throw ShortUrlNotFoundException::fromNotFoundShortCode($shortCode, $domain); + throw ShortUrlNotFoundException::fromNotFound($identifier); } return $shortUrl; diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolverInterface.php b/module/Core/src/Service/ShortUrl/ShortUrlResolverInterface.php index b00beed5..a3a7c115 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolverInterface.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolverInterface.php @@ -6,16 +6,17 @@ namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; interface ShortUrlResolverInterface { /** * @throws ShortUrlNotFoundException */ - public function shortCodeToShortUrl(string $shortCode, ?string $domain = null): ShortUrl; + public function resolveShortUrl(ShortUrlIdentifier $identifier): ShortUrl; /** * @throws ShortUrlNotFoundException */ - public function shortCodeToEnabledShortUrl(string $shortCode, ?string $domain = null): ShortUrl; + public function resolveEnabledShortUrl(ShortUrlIdentifier $identifier): ShortUrl; } diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index dbfb1db6..a505c1fb 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -8,6 +8,7 @@ use Doctrine\ORM; use Laminas\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\ShortUrlRepositoryAdapter; @@ -47,7 +48,7 @@ class ShortUrlService implements ShortUrlServiceInterface */ public function setTagsByShortCode(string $shortCode, array $tags = []): ShortUrl { - $shortUrl = $this->findByShortCode($this->em, $shortCode); + $shortUrl = $this->findByShortCode($this->em, new ShortUrlIdentifier($shortCode)); $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); $this->em->flush(); @@ -59,7 +60,7 @@ class ShortUrlService implements ShortUrlServiceInterface */ public function updateMetadataByShortCode(string $shortCode, ShortUrlMeta $shortUrlMeta): ShortUrl { - $shortUrl = $this->findByShortCode($this->em, $shortCode); + $shortUrl = $this->findByShortCode($this->em, new ShortUrlIdentifier($shortCode)); $shortUrl->updateMeta($shortUrlMeta); $this->em->flush(); diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index 91c83a03..6d31243b 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; @@ -56,7 +57,7 @@ class VisitsTracker implements VisitsTrackerInterface /** @var ORM\EntityRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); if ($repo->count(['shortCode' => $shortCode]) < 1) { - throw ShortUrlNotFoundException::fromNotFoundShortCode($shortCode); + throw ShortUrlNotFoundException::fromNotFound(new ShortUrlIdentifier($shortCode)); // FIXME } /** @var VisitRepository $repo */ diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 5eddd96d..c8acc09f 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -15,7 +15,7 @@ interface VisitsTrackerInterface /** * Tracks a new visit to provided short code from provided visitor */ - public function track(string $shortCode, Visitor $visitor): void; + public function track(string $shortCode, Visitor $visitor): void; // FIXME /** * Returns the visits on certain short code @@ -23,5 +23,5 @@ interface VisitsTrackerInterface * @return Visit[]|Paginator * @throws ShortUrlNotFoundException */ - public function info(string $shortCode, VisitsParams $params): Paginator; + public function info(string $shortCode, VisitsParams $params): Paginator; // FIXME } diff --git a/module/Core/test/Action/PixelActionTest.php b/module/Core/test/Action/PixelActionTest.php index ed9d0774..f4aed872 100644 --- a/module/Core/test/Action/PixelActionTest.php +++ b/module/Core/test/Action/PixelActionTest.php @@ -12,6 +12,7 @@ use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Common\Response\PixelResponse; use Shlinkio\Shlink\Core\Action\PixelAction; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options\AppOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Service\VisitsTracker; @@ -38,7 +39,7 @@ class PixelActionTest extends TestCase public function imageIsReturned(): void { $shortCode = 'abc123'; - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, '')->willReturn( + $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn( new ShortUrl('http://domain.com/foo/bar'), )->shouldBeCalledOnce(); $this->visitTracker->track(Argument::cetera())->shouldBeCalledOnce(); diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index 63df94af..2a4d1a19 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Common\Response\QrCodeResponse; use Shlinkio\Shlink\Core\Action\QrCodeAction; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; class QrCodeActionTest extends TestCase @@ -36,8 +37,9 @@ class QrCodeActionTest extends TestCase public function aNotFoundShortCodeWillDelegateIntoNextMiddleware(): void { $shortCode = 'abc123'; - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, '')->willThrow(ShortUrlNotFoundException::class) - ->shouldBeCalledOnce(); + $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, '')) + ->willThrow(ShortUrlNotFoundException::class) + ->shouldBeCalledOnce(); $delegate = $this->prophesize(RequestHandlerInterface::class); $process = $delegate->handle(Argument::any())->willReturn(new Response()); @@ -50,8 +52,9 @@ class QrCodeActionTest extends TestCase public function anInvalidShortCodeWillReturnNotFoundResponse(): void { $shortCode = 'abc123'; - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, '')->willThrow(ShortUrlNotFoundException::class) - ->shouldBeCalledOnce(); + $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, '')) + ->willThrow(ShortUrlNotFoundException::class) + ->shouldBeCalledOnce(); $delegate = $this->prophesize(RequestHandlerInterface::class); $process = $delegate->handle(Argument::any())->willReturn(new Response()); @@ -64,8 +67,9 @@ class QrCodeActionTest extends TestCase public function aCorrectRequestReturnsTheQrCodeResponse(): void { $shortCode = 'abc123'; - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, '')->willReturn(new ShortUrl('')) - ->shouldBeCalledOnce(); + $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, '')) + ->willReturn(new ShortUrl('')) + ->shouldBeCalledOnce(); $delegate = $this->prophesize(RequestHandlerInterface::class); $resp = $this->action->process( diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 25e67f4b..8d28f21c 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -13,6 +13,7 @@ use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Action\RedirectAction; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; @@ -45,7 +46,8 @@ class RedirectActionTest extends TestCase { $shortCode = 'abc123'; $shortUrl = new ShortUrl('http://domain.com/foo/bar?some=thing'); - $shortCodeToUrl = $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, '')->willReturn($shortUrl); + $shortCodeToUrl = $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, '')) + ->willReturn($shortUrl); $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { }); @@ -74,8 +76,9 @@ class RedirectActionTest extends TestCase public function nextMiddlewareIsInvokedIfLongUrlIsNotFound(): void { $shortCode = 'abc123'; - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode, '')->willThrow(ShortUrlNotFoundException::class) - ->shouldBeCalledOnce(); + $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, '')) + ->willThrow(ShortUrlNotFoundException::class) + ->shouldBeCalledOnce(); $this->visitTracker->track(Argument::cetera())->shouldNotBeCalled(); $handler = $this->prophesize(RequestHandlerInterface::class); diff --git a/module/Core/test/Exception/ShortUrlNotFoundExceptionTest.php b/module/Core/test/Exception/ShortUrlNotFoundExceptionTest.php index be02a66c..d0d77fb8 100644 --- a/module/Core/test/Exception/ShortUrlNotFoundExceptionTest.php +++ b/module/Core/test/Exception/ShortUrlNotFoundExceptionTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Exception; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; class ShortUrlNotFoundExceptionTest extends TestCase { @@ -23,7 +24,7 @@ class ShortUrlNotFoundExceptionTest extends TestCase $expectedAdditional['domain'] = $domain; } - $e = ShortUrlNotFoundException::fromNotFoundShortCode($shortCode, $domain); + $e = ShortUrlNotFoundException::fromNotFound(new ShortUrlIdentifier($shortCode, $domain)); $this->assertEquals($expectedMessage, $e->getMessage()); $this->assertEquals($expectedMessage, $e->getDetail()); diff --git a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php index e7aefd9d..49d6f933 100644 --- a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php @@ -28,15 +28,15 @@ class DeleteShortUrlServiceTest extends TestCase public function setUp(): void { - $shortUrl = (new ShortUrl(''))->setVisits( - new ArrayCollection(map(range(0, 10), fn () => new Visit(new ShortUrl(''), Visitor::emptyInstance()))), - ); + $shortUrl = (new ShortUrl(''))->setVisits(new ArrayCollection( + map(range(0, 10), fn () => new Visit(new ShortUrl(''), Visitor::emptyInstance())), + )); $this->shortCode = $shortUrl->getShortCode(); $this->em = $this->prophesize(EntityManagerInterface::class); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $repo->findOneBy(Argument::type('array'))->willReturn($shortUrl); + $repo->findOneByShortCode(Argument::cetera())->willReturn($shortUrl); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); } diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index 58f79606..cde37599 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -12,6 +12,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; @@ -41,7 +42,7 @@ class ShortUrlResolverTest extends TestCase $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $result = $this->urlResolver->shortCodeToShortUrl($shortCode); + $result = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); $this->assertSame($shortUrl, $result); $findOneByShortCode->shouldHaveBeenCalledOnce(); @@ -61,7 +62,7 @@ class ShortUrlResolverTest extends TestCase $findOneByShortCode->shouldBeCalledOnce(); $getRepo->shouldBeCalledOnce(); - $this->urlResolver->shortCodeToShortUrl($shortCode); + $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); } /** @test */ @@ -74,7 +75,7 @@ class ShortUrlResolverTest extends TestCase $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $result = $this->urlResolver->shortCodeToEnabledShortUrl($shortCode); + $result = $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode)); $this->assertSame($shortUrl, $result); $findOneByShortCode->shouldHaveBeenCalledOnce(); @@ -97,7 +98,7 @@ class ShortUrlResolverTest extends TestCase $findOneByShortCode->shouldBeCalledOnce(); $getRepo->shouldBeCalledOnce(); - $this->urlResolver->shortCodeToEnabledShortUrl($shortCode); + $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode)); } public function provideDisabledShortUrls(): iterable diff --git a/module/Core/test/Service/ShortUrlServiceTest.php b/module/Core/test/Service/ShortUrlServiceTest.php index 69f0785d..2fb92f53 100644 --- a/module/Core/test/Service/ShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrlServiceTest.php @@ -57,8 +57,8 @@ class ShortUrlServiceTest extends TestCase { $shortCode = 'abc123'; $repo = $this->prophesize(ShortUrlRepository::class); - $repo->findOneBy(['shortCode' => $shortCode])->willReturn(null) - ->shouldBeCalledOnce(); + $repo->findOneByShortCode($shortCode, null)->willReturn(null) + ->shouldBeCalledOnce(); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class); @@ -72,8 +72,8 @@ class ShortUrlServiceTest extends TestCase $shortUrl->setTags(Argument::any())->shouldBeCalledOnce(); $shortCode = 'abc123'; $repo = $this->prophesize(ShortUrlRepository::class); - $repo->findOneBy(['shortCode' => $shortCode])->willReturn($shortUrl->reveal()) - ->shouldBeCalledOnce(); + $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl->reveal()) + ->shouldBeCalledOnce(); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $tagRepo = $this->prophesize(EntityRepository::class); @@ -90,7 +90,7 @@ class ShortUrlServiceTest extends TestCase $shortUrl = new ShortUrl(''); $repo = $this->prophesize(ShortUrlRepository::class); - $findShortUrl = $repo->findOneBy(['shortCode' => 'abc123'])->willReturn($shortUrl); + $findShortUrl = $repo->findOneByShortCode('abc123', null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $flush = $this->em->flush()->willReturn(null); diff --git a/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php b/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php index 03458a2c..bf31805d 100644 --- a/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php @@ -8,6 +8,7 @@ use Laminas\Diactoros\Response\JsonResponse; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Transformer\ShortUrlDataTransformer; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -32,11 +33,9 @@ class ResolveShortUrlAction extends AbstractRestAction public function handle(Request $request): Response { - $shortCode = $request->getAttribute('shortCode'); - $domain = $request->getQueryParams()['domain'] ?? null; $transformer = new ShortUrlDataTransformer($this->domainConfig); + $url = $this->urlResolver->resolveShortUrl(ShortUrlIdentifier::fromRequest($request)); - $url = $this->urlResolver->shortCodeToShortUrl($shortCode, $domain); return new JsonResponse($transformer->transform($url)); } } diff --git a/module/Rest/test/Action/ShortUrl/ResolveShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/ResolveShortUrlActionTest.php index 067b4e0e..a62b1f95 100644 --- a/module/Rest/test/Action/ShortUrl/ResolveShortUrlActionTest.php +++ b/module/Rest/test/Action/ShortUrl/ResolveShortUrlActionTest.php @@ -8,6 +8,7 @@ use Laminas\Diactoros\ServerRequest; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Rest\Action\ShortUrl\ResolveShortUrlAction; @@ -28,7 +29,7 @@ class ResolveShortUrlActionTest extends TestCase public function correctShortCodeReturnsSuccess(): void { $shortCode = 'abc123'; - $this->urlResolver->shortCodeToShortUrl($shortCode, null)->willReturn( + $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode))->willReturn( new ShortUrl('http://domain.com/foo/bar'), )->shouldBeCalledOnce(); From fd82de31c0cbf659a2157a81a3cd9a59a2973b26 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 12:54:10 +0100 Subject: [PATCH 02/23] Fixed the way ShortUrlIdentifier is created from requests, on different request scopes --- module/Core/src/Action/AbstractTrackingAction.php | 4 ++-- module/Core/src/Action/QrCodeAction.php | 2 +- module/Core/src/Model/ShortUrlIdentifier.php | 10 +++++++++- .../Rest/src/Action/ShortUrl/ResolveShortUrlAction.php | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 3655bd32..ae5e6fb6 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -45,8 +45,8 @@ abstract class AbstractTrackingAction implements MiddlewareInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - $shortCode = $request->getAttribute('shortCode', ''); - $identifier = ShortUrlIdentifier::fromRequest($request); + $identifier = ShortUrlIdentifier::fromRedirectRequest($request); + $shortCode = $identifier->shortCode(); $query = $request->getQueryParams(); $disableTrackParam = $this->appOptions->getDisableTrackParam(); diff --git a/module/Core/src/Action/QrCodeAction.php b/module/Core/src/Action/QrCodeAction.php index df06edbd..7a07f2a1 100644 --- a/module/Core/src/Action/QrCodeAction.php +++ b/module/Core/src/Action/QrCodeAction.php @@ -39,7 +39,7 @@ class QrCodeAction implements MiddlewareInterface public function process(Request $request, RequestHandlerInterface $handler): Response { - $identifier = ShortUrlIdentifier::fromRequest($request); + $identifier = ShortUrlIdentifier::fromRedirectRequest($request); try { $this->urlResolver->resolveEnabledShortUrl($identifier); diff --git a/module/Core/src/Model/ShortUrlIdentifier.php b/module/Core/src/Model/ShortUrlIdentifier.php index dae3a15b..820bcf03 100644 --- a/module/Core/src/Model/ShortUrlIdentifier.php +++ b/module/Core/src/Model/ShortUrlIdentifier.php @@ -18,7 +18,7 @@ final class ShortUrlIdentifier $this->domain = $domain; } - public static function fromRequest(ServerRequestInterface $request): self + public static function fromApiRequest(ServerRequestInterface $request): self { $shortCode = $request->getAttribute('shortCode', ''); $domain = $request->getQueryParams()['domain'] ?? null; @@ -26,6 +26,14 @@ final class ShortUrlIdentifier return new self($shortCode, $domain); } + public static function fromRedirectRequest(ServerRequestInterface $request): self + { + $shortCode = $request->getAttribute('shortCode', ''); + $domain = $request->getUri()->getAuthority(); + + return new self($shortCode, $domain); + } + public static function fromCli(InputInterface $input): self { $shortCode = $input->getArgument('shortCode'); diff --git a/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php b/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php index bf31805d..41cd2b2d 100644 --- a/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/ResolveShortUrlAction.php @@ -34,7 +34,7 @@ class ResolveShortUrlAction extends AbstractRestAction public function handle(Request $request): Response { $transformer = new ShortUrlDataTransformer($this->domainConfig); - $url = $this->urlResolver->resolveShortUrl(ShortUrlIdentifier::fromRequest($request)); + $url = $this->urlResolver->resolveShortUrl(ShortUrlIdentifier::fromApiRequest($request)); return new JsonResponse($transformer->transform($url)); } From 1b2a0d674f5904f11e3984b0d562d6749d874d35 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 13:03:48 +0100 Subject: [PATCH 03/23] Fixed correct short URL being tracked when domain exists --- module/Core/src/Action/AbstractTrackingAction.php | 3 +-- module/Core/src/Service/VisitsTracker.php | 7 +------ module/Core/src/Service/VisitsTrackerInterface.php | 3 ++- module/Core/test/Service/VisitsTrackerTest.php | 10 ++-------- 4 files changed, 6 insertions(+), 17 deletions(-) diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index ae5e6fb6..436810e6 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -46,7 +46,6 @@ abstract class AbstractTrackingAction implements MiddlewareInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $identifier = ShortUrlIdentifier::fromRedirectRequest($request); - $shortCode = $identifier->shortCode(); $query = $request->getQueryParams(); $disableTrackParam = $this->appOptions->getDisableTrackParam(); @@ -55,7 +54,7 @@ abstract class AbstractTrackingAction implements MiddlewareInterface // Track visit to this short code if ($disableTrackParam === null || ! array_key_exists($disableTrackParam, $query)) { - $this->visitTracker->track($shortCode, Visitor::fromRequest($request)); + $this->visitTracker->track($url, Visitor::fromRequest($request)); } return $this->createSuccessResp($this->buildUrlToRedirectTo($url, $query, $disableTrackParam)); diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index 6d31243b..ca1a65f8 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -31,13 +31,8 @@ class VisitsTracker implements VisitsTrackerInterface /** * Tracks a new visit to provided short code from provided visitor */ - public function track(string $shortCode, Visitor $visitor): void + public function track(ShortUrl $shortUrl, Visitor $visitor): void { - /** @var ShortUrl $shortUrl */ - $shortUrl = $this->em->getRepository(ShortUrl::class)->findOneBy([ - 'shortCode' => $shortCode, - ]); - $visit = new Visit($shortUrl, $visitor); $this->em->persist($visit); diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index c8acc09f..3f19b054 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Laminas\Paginator\Paginator; +use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\Visitor; @@ -15,7 +16,7 @@ interface VisitsTrackerInterface /** * Tracks a new visit to provided short code from provided visitor */ - public function track(string $shortCode, Visitor $visitor): void; // FIXME + public function track(ShortUrl $shortUrl, Visitor $visitor): void; // FIXME /** * Returns the visits on certain short code diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 5cf78b16..61f1e16d 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -39,14 +39,11 @@ class VisitsTrackerTest extends TestCase public function trackPersistsVisit(): void { $shortCode = '123ABC'; - $repo = $this->prophesize(EntityRepository::class); - $repo->findOneBy(['shortCode' => $shortCode])->willReturn(new ShortUrl('')); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $this->em->persist(Argument::that(fn (Visit $visit) => $visit->setId('1')))->shouldBeCalledOnce(); $this->em->flush()->shouldBeCalledOnce(); - $this->visitsTracker->track($shortCode, Visitor::emptyInstance()); + $this->visitsTracker->track(new ShortUrl($shortCode), Visitor::emptyInstance()); $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); } @@ -55,10 +52,7 @@ class VisitsTrackerTest extends TestCase public function trackedIpAddressGetsObfuscated(): void { $shortCode = '123ABC'; - $repo = $this->prophesize(EntityRepository::class); - $repo->findOneBy(['shortCode' => $shortCode])->willReturn(new ShortUrl('')); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); $this->em->persist(Argument::any())->will(function ($args) { /** @var Visit $visit */ $visit = $args[0]; @@ -68,7 +62,7 @@ class VisitsTrackerTest extends TestCase })->shouldBeCalledOnce(); $this->em->flush()->shouldBeCalledOnce(); - $this->visitsTracker->track($shortCode, new Visitor('', '', '4.3.2.1')); + $this->visitsTracker->track(new ShortUrl($shortCode), new Visitor('', '', '4.3.2.1')); $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); } From 279bd12a2da5f51e6fd998553a836a66319b8ed0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 17:34:16 +0100 Subject: [PATCH 04/23] Ensured domain can be passed when fetching visits for a short URL --- .../src/Command/ShortUrl/GetVisitsCommand.php | 5 +-- .../Command/ShortUrl/GetVisitsCommandTest.php | 16 +++++---- module/Core/src/Model/ShortUrlIdentifier.php | 4 +-- .../Adapter/VisitsPaginatorAdapter.php | 21 +++++++---- .../Core/src/Repository/VisitRepository.php | 23 +++++++++--- .../Repository/VisitRepositoryInterface.php | 7 +++- module/Core/src/Service/VisitsTracker.php | 11 +++--- .../src/Service/VisitsTrackerInterface.php | 3 +- .../Repository/VisitRepositoryTest.php | 12 +++---- .../Core/test/Service/VisitsTrackerTest.php | 36 +++++++++++++------ .../Rest/src/Action/Visit/GetVisitsAction.php | 5 +-- .../test/Action/Visit/GetVisitsActionTest.php | 5 +-- 12 files changed, 100 insertions(+), 48 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 363ff3aa..1fcee476 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -9,6 +9,7 @@ use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\CLI\Util\ShlinkTable; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Symfony\Component\Console\Input\InputArgument; @@ -65,11 +66,11 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand protected function execute(InputInterface $input, OutputInterface $output): ?int { - $shortCode = $input->getArgument('shortCode'); + $identifier = ShortUrlIdentifier::fromCli($input); $startDate = $this->getDateOption($input, $output, 'startDate'); $endDate = $this->getDateOption($input, $output, 'endDate'); - $paginator = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange($startDate, $endDate))); + $paginator = $this->visitsTracker->info($identifier, new VisitsParams(new DateRange($startDate, $endDate))); $rows = map($paginator->getCurrentItems(), function (Visit $visit) { $rowData = $visit->jsonSerialize(); diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index 53ba114e..a725240e 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; @@ -42,9 +43,12 @@ class GetVisitsCommandTest extends TestCase public function noDateFlagsTriesToListWithoutDateRange(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange(null, null)))->willReturn( - new Paginator(new ArrayAdapter([])), - )->shouldBeCalledOnce(); + $this->visitsTracker->info( + new ShortUrlIdentifier($shortCode), + new VisitsParams(new DateRange(null, null)), + ) + ->willReturn(new Paginator(new ArrayAdapter([]))) + ->shouldBeCalledOnce(); $this->commandTester->execute(['shortCode' => $shortCode]); } @@ -56,7 +60,7 @@ class GetVisitsCommandTest extends TestCase $startDate = '2016-01-01'; $endDate = '2016-02-01'; $this->visitsTracker->info( - $shortCode, + new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))), ) ->willReturn(new Paginator(new ArrayAdapter([]))) @@ -74,7 +78,7 @@ class GetVisitsCommandTest extends TestCase { $shortCode = 'abc123'; $startDate = 'foo'; - $info = $this->visitsTracker->info($shortCode, new VisitsParams(new DateRange())) + $info = $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange())) ->willReturn(new Paginator(new ArrayAdapter([]))); $this->commandTester->execute([ @@ -94,7 +98,7 @@ class GetVisitsCommandTest extends TestCase public function outputIsProperlyGenerated(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::any())->willReturn( + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), Argument::any())->willReturn( new Paginator(new ArrayAdapter([ (new Visit(new ShortUrl(''), new Visitor('bar', 'foo', '')))->locate( new VisitLocation(new Location('', 'Spain', '', '', 0, 0, '')), diff --git a/module/Core/src/Model/ShortUrlIdentifier.php b/module/Core/src/Model/ShortUrlIdentifier.php index 820bcf03..4a74ba07 100644 --- a/module/Core/src/Model/ShortUrlIdentifier.php +++ b/module/Core/src/Model/ShortUrlIdentifier.php @@ -36,8 +36,8 @@ final class ShortUrlIdentifier public static function fromCli(InputInterface $input): self { - $shortCode = $input->getArgument('shortCode'); - $domain = $input->getOption('domain'); + $shortCode = $input->getArguments()['shortCode'] ?? ''; + $domain = $input->getOptions()['domain'] ?? null; return new self($shortCode, $domain); } diff --git a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php index 19baff73..247ea93e 100644 --- a/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php +++ b/module/Core/src/Paginator/Adapter/VisitsPaginatorAdapter.php @@ -5,26 +5,31 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Paginator\Adapter; use Laminas\Paginator\Adapter\AdapterInterface; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface; class VisitsPaginatorAdapter implements AdapterInterface { private VisitRepositoryInterface $visitRepository; - private string $shortCode; + private ShortUrlIdentifier $identifier; private VisitsParams $params; - public function __construct(VisitRepositoryInterface $visitRepository, string $shortCode, VisitsParams $params) - { + public function __construct( + VisitRepositoryInterface $visitRepository, + ShortUrlIdentifier $identifier, + VisitsParams $params + ) { $this->visitRepository = $visitRepository; - $this->shortCode = $shortCode; $this->params = $params; + $this->identifier = $identifier; } public function getItems($offset, $itemCountPerPage): array // phpcs:ignore { return $this->visitRepository->findVisitsByShortCode( - $this->shortCode, + $this->identifier->shortCode(), + $this->identifier->domain(), $this->params->getDateRange(), $itemCountPerPage, $offset, @@ -33,6 +38,10 @@ class VisitsPaginatorAdapter implements AdapterInterface public function count(): int { - return $this->visitRepository->countVisitsByShortCode($this->shortCode, $this->params->getDateRange()); + return $this->visitRepository->countVisitsByShortCode( + $this->identifier->shortCode(), + $this->identifier->domain(), + $this->params->getDateRange(), + ); } } diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 0e48e0a1..454323ef 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -46,11 +46,12 @@ DQL; */ public function findVisitsByShortCode( string $shortCode, + ?string $domain = null, ?DateRange $dateRange = null, ?int $limit = null, ?int $offset = null ): array { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $dateRange); + $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); $qb->select('v') ->orderBy('v.date', 'DESC'); @@ -64,22 +65,34 @@ DQL; return $qb->getQuery()->getResult(); } - public function countVisitsByShortCode(string $shortCode, ?DateRange $dateRange = null): int + public function countVisitsByShortCode(string $shortCode, ?string $domain = null, ?DateRange $dateRange = null): int { - $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $dateRange); + $qb = $this->createVisitsByShortCodeQueryBuilder($shortCode, $domain, $dateRange); $qb->select('COUNT(DISTINCT v.id)'); return (int) $qb->getQuery()->getSingleScalarResult(); } - private function createVisitsByShortCodeQueryBuilder(string $shortCode, ?DateRange $dateRange = null): QueryBuilder - { + private function createVisitsByShortCodeQueryBuilder( + string $shortCode, + ?string $domain, + ?DateRange $dateRange + ): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(Visit::class, 'v') ->join('v.shortUrl', 'su') ->where($qb->expr()->eq('su.shortCode', ':shortCode')) ->setParameter('shortCode', $shortCode); + // Apply domain filtering + if ($domain !== null) { + $qb->join('su.domain', 'd') + ->andWhere($qb->expr()->eq('d.authority', ':domain')) + ->setParameter('domain', $domain); + } else { + $qb->andWhere($qb->expr()->isNull('su.domain')); + } + // Apply date range filtering if ($dateRange !== null && $dateRange->getStartDate() !== null) { $qb->andWhere($qb->expr()->gte('v.date', ':startDate')) diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index e70c989e..0d0b66d0 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -28,10 +28,15 @@ interface VisitRepositoryInterface extends ObjectRepository */ public function findVisitsByShortCode( string $shortCode, + ?string $domain = null, ?DateRange $dateRange = null, ?int $limit = null, ?int $offset = null ): array; - public function countVisitsByShortCode(string $shortCode, ?DateRange $dateRange = null): int; + public function countVisitsByShortCode( + string $shortCode, + ?string $domain = null, + ?DateRange $dateRange = null + ): int; } diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index ca1a65f8..54abe319 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -15,6 +15,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Repository\VisitRepository; class VisitsTracker implements VisitsTrackerInterface @@ -47,17 +48,17 @@ class VisitsTracker implements VisitsTrackerInterface * @return Visit[]|Paginator * @throws ShortUrlNotFoundException */ - public function info(string $shortCode, VisitsParams $params): Paginator + public function info(ShortUrlIdentifier $identifier, VisitsParams $params): Paginator { - /** @var ORM\EntityRepository $repo */ + /** @var ShortUrlRepositoryInterface $repo */ $repo = $this->em->getRepository(ShortUrl::class); - if ($repo->count(['shortCode' => $shortCode]) < 1) { - throw ShortUrlNotFoundException::fromNotFound(new ShortUrlIdentifier($shortCode)); // FIXME + if (! $repo->shortCodeIsInUse($identifier->shortCode(), $identifier->domain())) { + throw ShortUrlNotFoundException::fromNotFound($identifier); } /** @var VisitRepository $repo */ $repo = $this->em->getRepository(Visit::class); - $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $shortCode, $params)); + $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $identifier, $params)); $paginator->setItemCountPerPage($params->getItemsPerPage()) ->setCurrentPageNumber($params->getPage()); diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 3f19b054..862ef190 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -8,6 +8,7 @@ use Laminas\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; @@ -24,5 +25,5 @@ interface VisitsTrackerInterface * @return Visit[]|Paginator * @throws ShortUrlNotFoundException */ - public function info(string $shortCode, VisitsParams $params): Paginator; // FIXME + public function info(ShortUrlIdentifier $identifier, VisitsParams $params): Paginator; // FIXME } diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 80207d5a..d4a757e1 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -83,15 +83,15 @@ class VisitRepositoryTest extends DatabaseTestCase $this->assertCount(0, $this->repo->findVisitsByShortCode('invalid')); $this->assertCount(6, $this->repo->findVisitsByShortCode($shortUrl->getShortCode())); - $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03'), ))); - $this->assertCount(4, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertCount(4, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-03'), ))); - $this->assertCount(3, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, 3, 2)); - $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, 5, 4)); + $this->assertCount(3, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, null, 3, 2)); + $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, null, 5, 4)); } /** @test */ @@ -108,11 +108,11 @@ class VisitRepositoryTest extends DatabaseTestCase $this->assertEquals(0, $this->repo->countVisitsByShortCode('invalid')); $this->assertEquals(6, $this->repo->countVisitsByShortCode($shortUrl->getShortCode())); - $this->assertEquals(2, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertEquals(2, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03'), ))); - $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), new DateRange( + $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( Chronos::parse('2016-01-03'), ))); } diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 61f1e16d..9028d2c7 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Service; use Doctrine\ORM\EntityManager; -use Doctrine\ORM\EntityRepository; use Laminas\Stdlib\ArrayUtils; use PHPUnit\Framework\Assert; use PHPUnit\Framework\TestCase; @@ -16,11 +15,17 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\ShortUrlVisited; +use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; +use function Functional\map; +use function range; + class VisitsTrackerTest extends TestCase { private VisitsTracker $visitsTracker; @@ -71,22 +76,33 @@ class VisitsTrackerTest extends TestCase public function infoReturnsVisitsForCertainShortCode(): void { $shortCode = '123ABC'; - $repo = $this->prophesize(EntityRepository::class); - $count = $repo->count(['shortCode' => $shortCode])->willReturn(1); + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $count = $repo->shortCodeIsInUse($shortCode, null)->willReturn(true); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - $list = [ - new Visit(new ShortUrl(''), Visitor::emptyInstance()), - new Visit(new ShortUrl(''), Visitor::emptyInstance()), - ]; + $list = map(range(0, 1), fn () => new Visit(new ShortUrl(''), Visitor::emptyInstance())); $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByShortCode($shortCode, Argument::type(DateRange::class), 1, 0)->willReturn($list); - $repo2->countVisitsByShortCode($shortCode, Argument::type(DateRange::class))->willReturn(1); + $repo2->findVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), 1, 0)->willReturn($list); + $repo2->countVisitsByShortCode($shortCode, null, Argument::type(DateRange::class))->willReturn(1); $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - $paginator = $this->visitsTracker->info($shortCode, new VisitsParams()); + $paginator = $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams()); $this->assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentItems())); $count->shouldHaveBeenCalledOnce(); } + + /** @test */ + public function throwsExceptionWhenRequestingVisitsForInvalidShortCode(): void + { + $shortCode = '123ABC'; + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $count = $repo->shortCodeIsInUse($shortCode, null)->willReturn(false); + $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); + + $this->expectException(ShortUrlNotFoundException::class); + $count->shouldBeCalledOnce(); + + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams()); + } } diff --git a/module/Rest/src/Action/Visit/GetVisitsAction.php b/module/Rest/src/Action/Visit/GetVisitsAction.php index c1fa0095..bd6ae5a5 100644 --- a/module/Rest/src/Action/Visit/GetVisitsAction.php +++ b/module/Rest/src/Action/Visit/GetVisitsAction.php @@ -9,6 +9,7 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Paginator\Util\PaginatorUtilsTrait; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -30,8 +31,8 @@ class GetVisitsAction extends AbstractRestAction public function handle(Request $request): Response { - $shortCode = $request->getAttribute('shortCode'); - $visits = $this->visitsTracker->info($shortCode, VisitsParams::fromRawData($request->getQueryParams())); + $identifier = ShortUrlIdentifier::fromApiRequest($request); + $visits = $this->visitsTracker->info($identifier, VisitsParams::fromRawData($request->getQueryParams())); return new JsonResponse([ 'visits' => $this->serializePaginator($visits), diff --git a/module/Rest/test/Action/Visit/GetVisitsActionTest.php b/module/Rest/test/Action/Visit/GetVisitsActionTest.php index a445c3b2..a1f1681a 100644 --- a/module/Rest/test/Action/Visit/GetVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/GetVisitsActionTest.php @@ -12,6 +12,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTracker; use Shlinkio\Shlink\Rest\Action\Visit\GetVisitsAction; @@ -31,7 +32,7 @@ class GetVisitsActionTest extends TestCase public function providingCorrectShortCodeReturnsVisits(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, Argument::type(VisitsParams::class))->willReturn( + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), Argument::type(VisitsParams::class))->willReturn( new Paginator(new ArrayAdapter([])), )->shouldBeCalledOnce(); @@ -43,7 +44,7 @@ class GetVisitsActionTest extends TestCase public function paramsAreReadFromQuery(): void { $shortCode = 'abc123'; - $this->visitsTracker->info($shortCode, new VisitsParams( + $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams( new DateRange(null, Chronos::parse('2016-01-01 00:00:00')), 3, 10, From a3ff545d4354db0747d5cdf998ea4c1dcaee043b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 17:44:37 +0100 Subject: [PATCH 05/23] Improved VisitsRepositoryTest to cover fetching visits for URL with domain --- .../Repository/VisitRepositoryTest.php | 72 +++++++++++++------ 1 file changed, 50 insertions(+), 22 deletions(-) diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index d4a757e1..bd1a0f2d 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -6,9 +6,11 @@ namespace ShlinkioTest\Shlink\Core\Repository; use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; +use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -24,6 +26,7 @@ class VisitRepositoryTest extends DatabaseTestCase VisitLocation::class, Visit::class, ShortUrl::class, + Domain::class, ]; private VisitRepository $repo; @@ -72,48 +75,73 @@ class VisitRepositoryTest extends DatabaseTestCase /** @test */ public function findVisitsByShortCodeReturnsProperData(): void { - $shortUrl = new ShortUrl(''); - $this->getEntityManager()->persist($shortUrl); - - for ($i = 0; $i < 6; $i++) { - $visit = new Visit($shortUrl, Visitor::emptyInstance(), Chronos::parse(sprintf('2016-01-0%s', $i + 1))); - $this->getEntityManager()->persist($visit); - } - $this->getEntityManager()->flush(); + [$shortCode, $domain] = $this->createShortUrlsAndVisits(); $this->assertCount(0, $this->repo->findVisitsByShortCode('invalid')); - $this->assertCount(6, $this->repo->findVisitsByShortCode($shortUrl->getShortCode())); - $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( + $this->assertCount(6, $this->repo->findVisitsByShortCode($shortCode)); + $this->assertCount(3, $this->repo->findVisitsByShortCode($shortCode, $domain)); + $this->assertCount(2, $this->repo->findVisitsByShortCode($shortCode, null, new DateRange( Chronos::parse('2016-01-02'), Chronos::parse('2016-01-03'), ))); - $this->assertCount(4, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( + $this->assertCount(4, $this->repo->findVisitsByShortCode($shortCode, null, new DateRange( Chronos::parse('2016-01-03'), ))); - $this->assertCount(3, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, null, 3, 2)); - $this->assertCount(2, $this->repo->findVisitsByShortCode($shortUrl->getShortCode(), null, null, 5, 4)); + $this->assertCount(1, $this->repo->findVisitsByShortCode($shortCode, $domain, new DateRange( + Chronos::parse('2016-01-03'), + ))); + $this->assertCount(3, $this->repo->findVisitsByShortCode($shortCode, null, null, 3, 2)); + $this->assertCount(2, $this->repo->findVisitsByShortCode($shortCode, null, null, 5, 4)); + $this->assertCount(1, $this->repo->findVisitsByShortCode($shortCode, $domain, null, 3, 2)); } /** @test */ public function countVisitsByShortCodeReturnsProperData(): void + { + [$shortCode, $domain] = $this->createShortUrlsAndVisits(); + + $this->assertEquals(0, $this->repo->countVisitsByShortCode('invalid')); + $this->assertEquals(6, $this->repo->countVisitsByShortCode($shortCode)); + $this->assertEquals(3, $this->repo->countVisitsByShortCode($shortCode, $domain)); + $this->assertEquals(2, $this->repo->countVisitsByShortCode($shortCode, null, new DateRange( + Chronos::parse('2016-01-02'), + Chronos::parse('2016-01-03'), + ))); + $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortCode, null, new DateRange( + Chronos::parse('2016-01-03'), + ))); + $this->assertEquals(1, $this->repo->countVisitsByShortCode($shortCode, $domain, new DateRange( + Chronos::parse('2016-01-03'), + ))); + } + + private function createShortUrlsAndVisits(): array { $shortUrl = new ShortUrl(''); + $domain = 'example.com'; + $shortCode = $shortUrl->getShortCode(); + $shortUrlWithDomain = new ShortUrl('', ShortUrlMeta::fromRawData([ + 'customSlug' => $shortCode, + 'domain' => $domain, + ])); + $this->getEntityManager()->persist($shortUrl); + $this->getEntityManager()->persist($shortUrlWithDomain); for ($i = 0; $i < 6; $i++) { $visit = new Visit($shortUrl, Visitor::emptyInstance(), Chronos::parse(sprintf('2016-01-0%s', $i + 1))); $this->getEntityManager()->persist($visit); } + for ($i = 0; $i < 3; $i++) { + $visit = new Visit( + $shortUrlWithDomain, + Visitor::emptyInstance(), + Chronos::parse(sprintf('2016-01-0%s', $i + 1)), + ); + $this->getEntityManager()->persist($visit); + } $this->getEntityManager()->flush(); - $this->assertEquals(0, $this->repo->countVisitsByShortCode('invalid')); - $this->assertEquals(6, $this->repo->countVisitsByShortCode($shortUrl->getShortCode())); - $this->assertEquals(2, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( - Chronos::parse('2016-01-02'), - Chronos::parse('2016-01-03'), - ))); - $this->assertEquals(4, $this->repo->countVisitsByShortCode($shortUrl->getShortCode(), null, new DateRange( - Chronos::parse('2016-01-03'), - ))); + return [$shortCode, $domain]; } } From 5f00d8b73242f9050736340711c29b1cf066eba6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 17:56:43 +0100 Subject: [PATCH 06/23] Added domain flag to GetVisitsCommand --- module/CLI/src/Command/ShortUrl/GetVisitsCommand.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 1fcee476..43949993 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; @@ -37,7 +38,8 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand $this ->setName(self::NAME) ->setDescription('Returns the detailed visits information for provided short code') - ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code which visits we want to get'); + ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code which visits we want to get') + ->addOption('domain', 'd', InputOption::VALUE_REQUIRED, 'The domain for the short code'); } protected function getStartDateDesc(): string From 732bb06c628f948a7ebdd45f11d708fd9ac18cd6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 18:06:50 +0100 Subject: [PATCH 07/23] Updated short URL deletion so that it accepts the domain --- .../ShortUrl/DeleteShortUrlCommand.php | 23 ++++++++++++------- .../ShortUrl/DeleteShortUrlCommandTest.php | 16 ++++++++----- .../ShortUrl/DeleteShortUrlService.php | 4 ++-- .../DeleteShortUrlServiceInterface.php | 3 ++- .../ShortUrl/DeleteShortUrlServiceTest.php | 9 ++++---- .../Action/ShortUrl/DeleteShortUrlAction.php | 5 ++-- 6 files changed, 37 insertions(+), 23 deletions(-) diff --git a/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php index bee66c34..f57001b0 100644 --- a/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\CLI\Command\ShortUrl; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\Core\Exception; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\DeleteShortUrlServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -40,33 +41,39 @@ class DeleteShortUrlCommand extends Command InputOption::VALUE_NONE, 'Ignores the safety visits threshold check, which could make short URLs with many visits to be ' . 'accidentally deleted', + ) + ->addOption( + 'domain', + 'd', + InputOption::VALUE_REQUIRED, + 'The domain if the short code does not belong to the default one', ); } protected function execute(InputInterface $input, OutputInterface $output): ?int { $io = new SymfonyStyle($input, $output); - $shortCode = $input->getArgument('shortCode'); + $identifier = ShortUrlIdentifier::fromCli($input); $ignoreThreshold = $input->getOption('ignore-threshold'); try { - $this->runDelete($io, $shortCode, $ignoreThreshold); + $this->runDelete($io, $identifier, $ignoreThreshold); return ExitCodes::EXIT_SUCCESS; } catch (Exception\ShortUrlNotFoundException $e) { $io->error($e->getMessage()); return ExitCodes::EXIT_FAILURE; } catch (Exception\DeleteShortUrlException $e) { - return $this->retry($io, $shortCode, $e->getMessage()); + return $this->retry($io, $identifier, $e->getMessage()); } } - private function retry(SymfonyStyle $io, string $shortCode, string $warningMsg): int + private function retry(SymfonyStyle $io, ShortUrlIdentifier $identifier, string $warningMsg): int { $io->writeln(sprintf('%s', $warningMsg)); $forceDelete = $io->confirm('Do you want to delete it anyway?', false); if ($forceDelete) { - $this->runDelete($io, $shortCode, true); + $this->runDelete($io, $identifier, true); } else { $io->warning('Short URL was not deleted.'); } @@ -74,9 +81,9 @@ class DeleteShortUrlCommand extends Command return $forceDelete ? ExitCodes::EXIT_SUCCESS : ExitCodes::EXIT_WARNING; } - private function runDelete(SymfonyStyle $io, string $shortCode, bool $ignoreThreshold): void + private function runDelete(SymfonyStyle $io, ShortUrlIdentifier $identifier, bool $ignoreThreshold): void { - $this->deleteShortUrlService->deleteByShortCode($shortCode, $ignoreThreshold); - $io->success(sprintf('Short URL with short code "%s" successfully deleted.', $shortCode)); + $this->deleteShortUrlService->deleteByShortCode($identifier, $ignoreThreshold); + $io->success(sprintf('Short URL with short code "%s" successfully deleted.', $identifier->shortCode())); } } diff --git a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php index 9d9a11c1..2c3526f5 100644 --- a/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/DeleteShortUrlCommandTest.php @@ -39,8 +39,10 @@ class DeleteShortUrlCommandTest extends TestCase public function successMessageIsPrintedIfUrlIsProperlyDeleted(): void { $shortCode = 'abc123'; - $deleteByShortCode = $this->service->deleteByShortCode($shortCode, false)->will(function (): void { - }); + $deleteByShortCode = $this->service->deleteByShortCode(new ShortUrlIdentifier($shortCode), false)->will( + function (): void { + }, + ); $this->commandTester->execute(['shortCode' => $shortCode]); $output = $this->commandTester->getDisplay(); @@ -56,8 +58,9 @@ class DeleteShortUrlCommandTest extends TestCase public function invalidShortCodePrintsMessage(): void { $shortCode = 'abc123'; - $deleteByShortCode = $this->service->deleteByShortCode($shortCode, false)->willThrow( - Exception\ShortUrlNotFoundException::fromNotFound(new ShortUrlIdentifier($shortCode)), + $identifier = new ShortUrlIdentifier($shortCode); + $deleteByShortCode = $this->service->deleteByShortCode($identifier, false)->willThrow( + Exception\ShortUrlNotFoundException::fromNotFound($identifier), ); $this->commandTester->execute(['shortCode' => $shortCode]); @@ -77,7 +80,8 @@ class DeleteShortUrlCommandTest extends TestCase string $expectedMessage ): void { $shortCode = 'abc123'; - $deleteByShortCode = $this->service->deleteByShortCode($shortCode, Argument::type('bool'))->will( + $identifier = new ShortUrlIdentifier($shortCode); + $deleteByShortCode = $this->service->deleteByShortCode($identifier, Argument::type('bool'))->will( function (array $args) use ($shortCode): void { $ignoreThreshold = array_pop($args); @@ -110,7 +114,7 @@ class DeleteShortUrlCommandTest extends TestCase public function deleteIsNotRetriedWhenThresholdIsReachedAndQuestionIsDeclined(): void { $shortCode = 'abc123'; - $deleteByShortCode = $this->service->deleteByShortCode($shortCode, false)->willThrow( + $deleteByShortCode = $this->service->deleteByShortCode(new ShortUrlIdentifier($shortCode), false)->willThrow( Exception\DeleteShortUrlException::fromVisitsThreshold(10, $shortCode), ); $this->commandTester->setInputs(['no']); diff --git a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php index 2b4ad067..f1795d82 100644 --- a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php +++ b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php @@ -27,9 +27,9 @@ class DeleteShortUrlService implements DeleteShortUrlServiceInterface * @throws Exception\ShortUrlNotFoundException * @throws Exception\DeleteShortUrlException */ - public function deleteByShortCode(string $shortCode, bool $ignoreThreshold = false): void + public function deleteByShortCode(ShortUrlIdentifier $identifier, bool $ignoreThreshold = false): void { - $shortUrl = $this->findByShortCode($this->em, new ShortUrlIdentifier($shortCode)); + $shortUrl = $this->findByShortCode($this->em, $identifier); if (! $ignoreThreshold && $this->isThresholdReached($shortUrl)) { throw Exception\DeleteShortUrlException::fromVisitsThreshold( $this->deleteShortUrlsOptions->getVisitsThreshold(), diff --git a/module/Core/src/Service/ShortUrl/DeleteShortUrlServiceInterface.php b/module/Core/src/Service/ShortUrl/DeleteShortUrlServiceInterface.php index b196375d..4759bf24 100644 --- a/module/Core/src/Service/ShortUrl/DeleteShortUrlServiceInterface.php +++ b/module/Core/src/Service/ShortUrl/DeleteShortUrlServiceInterface.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service\ShortUrl; use Shlinkio\Shlink\Core\Exception; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; interface DeleteShortUrlServiceInterface { @@ -12,5 +13,5 @@ interface DeleteShortUrlServiceInterface * @throws Exception\ShortUrlNotFoundException * @throws Exception\DeleteShortUrlException */ - public function deleteByShortCode(string $shortCode, bool $ignoreThreshold = false): void; + public function deleteByShortCode(ShortUrlIdentifier $identifier, bool $ignoreThreshold = false): void; } diff --git a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php index 49d6f933..ed2914af 100644 --- a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php @@ -12,6 +12,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Exception\DeleteShortUrlException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\DeleteShortUrlsOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; @@ -51,7 +52,7 @@ class DeleteShortUrlServiceTest extends TestCase $this->shortCode, )); - $service->deleteByShortCode($this->shortCode); + $service->deleteByShortCode(new ShortUrlIdentifier($this->shortCode)); } /** @test */ @@ -62,7 +63,7 @@ class DeleteShortUrlServiceTest extends TestCase $remove = $this->em->remove(Argument::type(ShortUrl::class))->willReturn(null); $flush = $this->em->flush()->willReturn(null); - $service->deleteByShortCode($this->shortCode, true); + $service->deleteByShortCode(new ShortUrlIdentifier($this->shortCode), true); $remove->shouldHaveBeenCalledOnce(); $flush->shouldHaveBeenCalledOnce(); @@ -76,7 +77,7 @@ class DeleteShortUrlServiceTest extends TestCase $remove = $this->em->remove(Argument::type(ShortUrl::class))->willReturn(null); $flush = $this->em->flush()->willReturn(null); - $service->deleteByShortCode($this->shortCode); + $service->deleteByShortCode(new ShortUrlIdentifier($this->shortCode)); $remove->shouldHaveBeenCalledOnce(); $flush->shouldHaveBeenCalledOnce(); @@ -90,7 +91,7 @@ class DeleteShortUrlServiceTest extends TestCase $remove = $this->em->remove(Argument::type(ShortUrl::class))->willReturn(null); $flush = $this->em->flush()->willReturn(null); - $service->deleteByShortCode($this->shortCode); + $service->deleteByShortCode(new ShortUrlIdentifier($this->shortCode)); $remove->shouldHaveBeenCalledOnce(); $flush->shouldHaveBeenCalledOnce(); diff --git a/module/Rest/src/Action/ShortUrl/DeleteShortUrlAction.php b/module/Rest/src/Action/ShortUrl/DeleteShortUrlAction.php index e0b269f3..d86c60e9 100644 --- a/module/Rest/src/Action/ShortUrl/DeleteShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/DeleteShortUrlAction.php @@ -8,6 +8,7 @@ use Laminas\Diactoros\Response\EmptyResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrl\DeleteShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -26,8 +27,8 @@ class DeleteShortUrlAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { - $shortCode = $request->getAttribute('shortCode', ''); - $this->deleteShortUrlService->deleteByShortCode($shortCode); + $identifier = ShortUrlIdentifier::fromApiRequest($request); + $this->deleteShortUrlService->deleteByShortCode($identifier); return new EmptyResponse(); } } From 5d1d9dcac3e608fea6a41c53f5a83bfcef2e02d6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 22:54:02 +0100 Subject: [PATCH 08/23] Allowed domain to be provided when editing short URL meta --- module/Core/config/dependencies.config.php | 8 +++- .../ShortUrl/DeleteShortUrlService.php | 13 +++--- .../Service/ShortUrl/FindShortCodeTrait.php | 29 ------------ module/Core/src/Service/ShortUrlService.php | 14 +++--- .../src/Service/ShortUrlServiceInterface.php | 3 +- .../ShortUrl/DeleteShortUrlServiceTest.php | 10 ++--- .../Core/test/Service/ShortUrlServiceTest.php | 45 +++++++------------ .../Action/ShortUrl/EditShortUrlAction.php | 5 ++- 8 files changed, 49 insertions(+), 78 deletions(-) delete mode 100644 module/Core/src/Service/ShortUrl/FindShortCodeTrait.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index e1954329..9809c5dd 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -53,10 +53,14 @@ return [ Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Options\UrlShortenerOptions::class], Service\VisitsTracker::class => ['em', EventDispatcherInterface::class], - Service\ShortUrlService::class => ['em'], + Service\ShortUrlService::class => ['em', Service\ShortUrl\ShortUrlResolver::class], Service\VisitService::class => ['em'], Service\Tag\TagService::class => ['em'], - Service\ShortUrl\DeleteShortUrlService::class => ['em', Options\DeleteShortUrlsOptions::class], + Service\ShortUrl\DeleteShortUrlService::class => [ + 'em', + Options\DeleteShortUrlsOptions::class, + Service\ShortUrl\ShortUrlResolver::class, + ], Service\ShortUrl\ShortUrlResolver::class => ['em'], Util\UrlValidator::class => ['httpClient'], diff --git a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php index f1795d82..35a540da 100644 --- a/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php +++ b/module/Core/src/Service/ShortUrl/DeleteShortUrlService.php @@ -12,15 +12,18 @@ use Shlinkio\Shlink\Core\Options\DeleteShortUrlsOptions; class DeleteShortUrlService implements DeleteShortUrlServiceInterface { - use FindShortCodeTrait; - private EntityManagerInterface $em; private DeleteShortUrlsOptions $deleteShortUrlsOptions; + private ShortUrlResolverInterface $urlResolver; - public function __construct(EntityManagerInterface $em, DeleteShortUrlsOptions $deleteShortUrlsOptions) - { + public function __construct( + EntityManagerInterface $em, + DeleteShortUrlsOptions $deleteShortUrlsOptions, + ShortUrlResolverInterface $urlResolver + ) { $this->em = $em; $this->deleteShortUrlsOptions = $deleteShortUrlsOptions; + $this->urlResolver = $urlResolver; } /** @@ -29,7 +32,7 @@ class DeleteShortUrlService implements DeleteShortUrlServiceInterface */ public function deleteByShortCode(ShortUrlIdentifier $identifier, bool $ignoreThreshold = false): void { - $shortUrl = $this->findByShortCode($this->em, $identifier); + $shortUrl = $this->urlResolver->resolveShortUrl($identifier); if (! $ignoreThreshold && $this->isThresholdReached($shortUrl)) { throw Exception\DeleteShortUrlException::fromVisitsThreshold( $this->deleteShortUrlsOptions->getVisitsThreshold(), diff --git a/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php b/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php deleted file mode 100644 index 654d587f..00000000 --- a/module/Core/src/Service/ShortUrl/FindShortCodeTrait.php +++ /dev/null @@ -1,29 +0,0 @@ -getRepository(ShortUrl::class); - $shortUrl = $repo->findOneByShortCode($identifier->shortCode(), $identifier->domain()); - if ($shortUrl === null) { - throw ShortUrlNotFoundException::fromNotFound($identifier); - } - - return $shortUrl; - } -} diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index a505c1fb..8d20bf8d 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -13,19 +13,20 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; -use Shlinkio\Shlink\Core\Service\ShortUrl\FindShortCodeTrait; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Util\TagManagerTrait; class ShortUrlService implements ShortUrlServiceInterface { - use FindShortCodeTrait; use TagManagerTrait; private ORM\EntityManagerInterface $em; + private ShortUrlResolverInterface $urlResolver; - public function __construct(ORM\EntityManagerInterface $em) + public function __construct(ORM\EntityManagerInterface $em, ShortUrlResolverInterface $urlResolver) { $this->em = $em; + $this->urlResolver = $urlResolver; } /** @@ -48,8 +49,9 @@ class ShortUrlService implements ShortUrlServiceInterface */ public function setTagsByShortCode(string $shortCode, array $tags = []): ShortUrl { - $shortUrl = $this->findByShortCode($this->em, new ShortUrlIdentifier($shortCode)); + $shortUrl = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); + $this->em->flush(); return $shortUrl; @@ -58,9 +60,9 @@ class ShortUrlService implements ShortUrlServiceInterface /** * @throws ShortUrlNotFoundException */ - public function updateMetadataByShortCode(string $shortCode, ShortUrlMeta $shortUrlMeta): ShortUrl + public function updateMetadataByShortCode(ShortUrlIdentifier $identifier, ShortUrlMeta $shortUrlMeta): ShortUrl { - $shortUrl = $this->findByShortCode($this->em, new ShortUrlIdentifier($shortCode)); + $shortUrl = $this->urlResolver->resolveShortUrl($identifier); $shortUrl->updateMeta($shortUrlMeta); $this->em->flush(); diff --git a/module/Core/src/Service/ShortUrlServiceInterface.php b/module/Core/src/Service/ShortUrlServiceInterface.php index e431c51b..70dd3de5 100644 --- a/module/Core/src/Service/ShortUrlServiceInterface.php +++ b/module/Core/src/Service/ShortUrlServiceInterface.php @@ -7,6 +7,7 @@ namespace Shlinkio\Shlink\Core\Service; use Laminas\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; @@ -26,5 +27,5 @@ interface ShortUrlServiceInterface /** * @throws ShortUrlNotFoundException */ - public function updateMetadataByShortCode(string $shortCode, ShortUrlMeta $shortUrlMeta): ShortUrl; + public function updateMetadataByShortCode(ShortUrlIdentifier $identifier, ShortUrlMeta $shortUrlMeta): ShortUrl; } diff --git a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php index ed2914af..0911cb7b 100644 --- a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php @@ -15,8 +15,8 @@ use Shlinkio\Shlink\Core\Exception\DeleteShortUrlException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\DeleteShortUrlsOptions; -use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; use Shlinkio\Shlink\Core\Service\ShortUrl\DeleteShortUrlService; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use function Functional\map; use function range; @@ -25,6 +25,7 @@ use function sprintf; class DeleteShortUrlServiceTest extends TestCase { private ObjectProphecy $em; + private ObjectProphecy $urlResolver; private string $shortCode; public function setUp(): void @@ -36,9 +37,8 @@ class DeleteShortUrlServiceTest extends TestCase $this->em = $this->prophesize(EntityManagerInterface::class); - $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $repo->findOneByShortCode(Argument::cetera())->willReturn($shortUrl); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); + $this->urlResolver->resolveShortUrl(Argument::cetera())->willReturn($shortUrl); } /** @test */ @@ -102,6 +102,6 @@ class DeleteShortUrlServiceTest extends TestCase return new DeleteShortUrlService($this->em->reveal(), new DeleteShortUrlsOptions([ 'visitsThreshold' => $visitsThreshold, 'checkVisitsThreshold' => $checkVisitsThreshold, - ])); + ]), $this->urlResolver->reveal()); } } diff --git a/module/Core/test/Service/ShortUrlServiceTest.php b/module/Core/test/Service/ShortUrlServiceTest.php index 2fb92f53..d962c204 100644 --- a/module/Core/test/Service/ShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrlServiceTest.php @@ -12,10 +12,11 @@ use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; +use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Service\ShortUrlService; use function count; @@ -24,13 +25,17 @@ class ShortUrlServiceTest extends TestCase { private ShortUrlService $service; private ObjectProphecy $em; + private ObjectProphecy $urlResolver; public function setUp(): void { $this->em = $this->prophesize(EntityManagerInterface::class); $this->em->persist(Argument::any())->willReturn(null); $this->em->flush()->willReturn(null); - $this->service = new ShortUrlService($this->em->reveal()); + + $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); + + $this->service = new ShortUrlService($this->em->reveal(), $this->urlResolver->reveal()); } /** @test */ @@ -52,29 +57,14 @@ class ShortUrlServiceTest extends TestCase $this->assertEquals(4, $list->getCurrentItemCount()); } - /** @test */ - public function exceptionIsThrownWhenSettingTagsOnInvalidShortcode(): void - { - $shortCode = 'abc123'; - $repo = $this->prophesize(ShortUrlRepository::class); - $repo->findOneByShortCode($shortCode, null)->willReturn(null) - ->shouldBeCalledOnce(); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - - $this->expectException(ShortUrlNotFoundException::class); - $this->service->setTagsByShortCode($shortCode); - } - /** @test */ public function providedTagsAreGetFromRepoAndSetToTheShortUrl(): void { $shortUrl = $this->prophesize(ShortUrl::class); $shortUrl->setTags(Argument::any())->shouldBeCalledOnce(); $shortCode = 'abc123'; - $repo = $this->prophesize(ShortUrlRepository::class); - $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl->reveal()) - ->shouldBeCalledOnce(); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode))->willReturn($shortUrl->reveal()) + ->shouldBeCalledOnce(); $tagRepo = $this->prophesize(EntityRepository::class); $tagRepo->findOneBy(['name' => 'foo'])->willReturn(new Tag('foo'))->shouldBeCalledOnce(); @@ -89,23 +79,22 @@ class ShortUrlServiceTest extends TestCase { $shortUrl = new ShortUrl(''); - $repo = $this->prophesize(ShortUrlRepository::class); - $findShortUrl = $repo->findOneByShortCode('abc123', null)->willReturn($shortUrl); - $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); + $findShortUrl = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier('abc123'))->willReturn($shortUrl); $flush = $this->em->flush()->willReturn(null); - $result = $this->service->updateMetadataByShortCode('abc123', ShortUrlMeta::fromRawData([ - 'validSince' => Chronos::parse('2017-01-01 00:00:00')->toAtomString(), - 'validUntil' => Chronos::parse('2017-01-05 00:00:00')->toAtomString(), - 'maxVisits' => 5, - ])); + $result = $this->service->updateMetadataByShortCode(new ShortUrlIdentifier('abc123'), ShortUrlMeta::fromRawData( + [ + 'validSince' => Chronos::parse('2017-01-01 00:00:00')->toAtomString(), + 'validUntil' => Chronos::parse('2017-01-05 00:00:00')->toAtomString(), + 'maxVisits' => 5, + ], + )); $this->assertSame($shortUrl, $result); $this->assertEquals(Chronos::parse('2017-01-01 00:00:00'), $shortUrl->getValidSince()); $this->assertEquals(Chronos::parse('2017-01-05 00:00:00'), $shortUrl->getValidUntil()); $this->assertEquals(5, $shortUrl->getMaxVisits()); $findShortUrl->shouldHaveBeenCalled(); - $getRepo->shouldHaveBeenCalled(); $flush->shouldHaveBeenCalled(); } } diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php index a6bc5538..8b3e65ab 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php @@ -8,6 +8,7 @@ use Laminas\Diactoros\Response\EmptyResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -28,9 +29,9 @@ class EditShortUrlAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { $postData = (array) $request->getParsedBody(); - $shortCode = $request->getAttribute('shortCode', ''); + $identifier = ShortUrlIdentifier::fromApiRequest($request); - $this->shortUrlService->updateMetadataByShortCode($shortCode, ShortUrlMeta::fromRawData($postData)); + $this->shortUrlService->updateMetadataByShortCode($identifier, ShortUrlMeta::fromRawData($postData)); return new EmptyResponse(); } } From 6858dc47853a82b576d919c1d9245d76ca42e7d9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 22:59:21 +0100 Subject: [PATCH 09/23] Updated setting short URL tags so that it accepts providing the domain --- module/Core/src/Service/ShortUrlService.php | 4 ++-- module/Core/src/Service/ShortUrlServiceInterface.php | 2 +- module/Core/test/Service/ShortUrlServiceTest.php | 2 +- module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php | 7 ++++--- .../test/Action/ShortUrl/EditShortUrlTagsActionTest.php | 5 +++-- 5 files changed, 11 insertions(+), 9 deletions(-) diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index 8d20bf8d..e9aaf637 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -47,9 +47,9 @@ class ShortUrlService implements ShortUrlServiceInterface * @param string[] $tags * @throws ShortUrlNotFoundException */ - public function setTagsByShortCode(string $shortCode, array $tags = []): ShortUrl + public function setTagsByShortCode(ShortUrlIdentifier $identifier, array $tags = []): ShortUrl { - $shortUrl = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); + $shortUrl = $this->urlResolver->resolveShortUrl($identifier); $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); $this->em->flush(); diff --git a/module/Core/src/Service/ShortUrlServiceInterface.php b/module/Core/src/Service/ShortUrlServiceInterface.php index 70dd3de5..379abc55 100644 --- a/module/Core/src/Service/ShortUrlServiceInterface.php +++ b/module/Core/src/Service/ShortUrlServiceInterface.php @@ -22,7 +22,7 @@ interface ShortUrlServiceInterface * @param string[] $tags * @throws ShortUrlNotFoundException */ - public function setTagsByShortCode(string $shortCode, array $tags = []): ShortUrl; + public function setTagsByShortCode(ShortUrlIdentifier $identifier, array $tags = []): ShortUrl; /** * @throws ShortUrlNotFoundException diff --git a/module/Core/test/Service/ShortUrlServiceTest.php b/module/Core/test/Service/ShortUrlServiceTest.php index d962c204..842eac60 100644 --- a/module/Core/test/Service/ShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrlServiceTest.php @@ -71,7 +71,7 @@ class ShortUrlServiceTest extends TestCase $tagRepo->findOneBy(['name' => 'bar'])->willReturn(null)->shouldBeCalledOnce(); $this->em->getRepository(Tag::class)->willReturn($tagRepo->reveal()); - $this->service->setTagsByShortCode($shortCode, ['foo', 'bar']); + $this->service->setTagsByShortCode(new ShortUrlIdentifier($shortCode), ['foo', 'bar']); } /** @test */ diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php index e30710ed..0a48d986 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlTagsAction.php @@ -9,6 +9,7 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -27,7 +28,6 @@ class EditShortUrlTagsAction extends AbstractRestAction public function handle(Request $request): Response { - $shortCode = $request->getAttribute('shortCode'); $bodyParams = $request->getParsedBody(); if (! isset($bodyParams['tags'])) { @@ -35,9 +35,10 @@ class EditShortUrlTagsAction extends AbstractRestAction 'tags' => 'List of tags has to be provided', ]); } - $tags = $bodyParams['tags']; + ['tags' => $tags] = $bodyParams; + $identifier = ShortUrlIdentifier::fromApiRequest($request); - $shortUrl = $this->shortUrlService->setTagsByShortCode($shortCode, $tags); + $shortUrl = $this->shortUrlService->setTagsByShortCode($identifier, $tags); return new JsonResponse(['tags' => $shortUrl->getTags()->toArray()]); } } diff --git a/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php b/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php index 83db484c..d7a86844 100644 --- a/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php +++ b/module/Rest/test/Action/ShortUrl/EditShortUrlTagsActionTest.php @@ -9,6 +9,7 @@ use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Service\ShortUrlService; use Shlinkio\Shlink\Rest\Action\ShortUrl\EditShortUrlTagsAction; @@ -34,8 +35,8 @@ class EditShortUrlTagsActionTest extends TestCase public function tagsListIsReturnedIfCorrectShortCodeIsProvided(): void { $shortCode = 'abc123'; - $this->shortUrlService->setTagsByShortCode($shortCode, [])->willReturn(new ShortUrl('')) - ->shouldBeCalledOnce(); + $this->shortUrlService->setTagsByShortCode(new ShortUrlIdentifier($shortCode), [])->willReturn(new ShortUrl('')) + ->shouldBeCalledOnce(); $response = $this->action->handle( (new ServerRequest())->withAttribute('shortCode', 'abc123') From 1a8e4cdfd78a25de64b77c165c3be811d1f69470 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 08:57:04 +0100 Subject: [PATCH 10/23] Exposed domain on short URLs --- docs/swagger/definitions/ShortUrl.json | 4 ++++ docs/swagger/paths/v1_short-urls.json | 14 +++++++++----- docs/swagger/paths/v1_short-urls_shorten.json | 3 ++- docs/swagger/paths/v1_short-urls_{shortCode}.json | 3 ++- module/Core/src/Entity/Domain.php | 8 +++++++- module/Core/src/Entity/ShortUrl.php | 5 +++++ .../src/Transformer/ShortUrlDataTransformer.php | 5 ++--- module/Rest/test-api/Action/ListShortUrlsTest.php | 5 +++++ 8 files changed, 36 insertions(+), 11 deletions(-) diff --git a/docs/swagger/definitions/ShortUrl.json b/docs/swagger/definitions/ShortUrl.json index 8111bd6e..66d20115 100644 --- a/docs/swagger/definitions/ShortUrl.json +++ b/docs/swagger/definitions/ShortUrl.json @@ -31,6 +31,10 @@ }, "meta": { "$ref": "./ShortUrlMeta.json" + }, + "domain": { + "type": "string", + "description": "The domain in which the short URL was created. Null if it belongs to default domain." } } } diff --git a/docs/swagger/paths/v1_short-urls.json b/docs/swagger/paths/v1_short-urls.json index 0c6d0484..be274ab6 100644 --- a/docs/swagger/paths/v1_short-urls.json +++ b/docs/swagger/paths/v1_short-urls.json @@ -123,7 +123,8 @@ "validSince": "2017-01-21T00:00:00+02:00", "validUntil": null, "maxVisits": 100 - } + }, + "domain": null }, { "shortCode": "12Kb3", @@ -138,11 +139,12 @@ "validSince": null, "validUntil": null, "maxVisits": null - } + }, + "domain": null }, { "shortCode": "123bA", - "shortUrl": "https://doma.in/123bA", + "shortUrl": "https://example.com/123bA", "longUrl": "https://www.google.com", "dateCreated": "2015-10-01T20:34:16+02:00", "visitsCount": 25, @@ -151,7 +153,8 @@ "validSince": "2017-01-21T00:00:00+02:00", "validUntil": null, "maxVisits": null - } + }, + "domain": "example.com" } ], "pagination": { @@ -271,7 +274,8 @@ "validSince": "2017-01-21T00:00:00+02:00", "validUntil": null, "maxVisits": 500 - } + }, + "domain": null } } }, diff --git a/docs/swagger/paths/v1_short-urls_shorten.json b/docs/swagger/paths/v1_short-urls_shorten.json index c5ad9352..c31c0cd9 100644 --- a/docs/swagger/paths/v1_short-urls_shorten.json +++ b/docs/swagger/paths/v1_short-urls_shorten.json @@ -72,7 +72,8 @@ "validSince": "2017-01-21T00:00:00+02:00", "validUntil": null, "maxVisits": 100 - } + }, + "domain": null }, "text/plain": "https://doma.in/abc123" } diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}.json b/docs/swagger/paths/v1_short-urls_{shortCode}.json index d4537a83..4fbdeb8e 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}.json @@ -58,7 +58,8 @@ "validSince": "2017-01-21T00:00:00+02:00", "validUntil": null, "maxVisits": 100 - } + }, + "domain": null } } }, diff --git a/module/Core/src/Entity/Domain.php b/module/Core/src/Entity/Domain.php index 924b50e5..f836f7ed 100644 --- a/module/Core/src/Entity/Domain.php +++ b/module/Core/src/Entity/Domain.php @@ -4,9 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Entity; +use JsonSerializable; use Shlinkio\Shlink\Common\Entity\AbstractEntity; -class Domain extends AbstractEntity +class Domain extends AbstractEntity implements JsonSerializable { private string $authority; @@ -19,4 +20,9 @@ class Domain extends AbstractEntity { return $this->authority; } + + public function jsonSerialize(): string + { + return $this->getAuthority(); + } } diff --git a/module/Core/src/Entity/ShortUrl.php b/module/Core/src/Entity/ShortUrl.php index e260896d..98d6a146 100644 --- a/module/Core/src/Entity/ShortUrl.php +++ b/module/Core/src/Entity/ShortUrl.php @@ -69,6 +69,11 @@ class ShortUrl extends AbstractEntity return $this->dateCreated; } + public function getDomain(): ?Domain + { + return $this->domain; + } + /** * @return Collection|Tag[] */ diff --git a/module/Core/src/Transformer/ShortUrlDataTransformer.php b/module/Core/src/Transformer/ShortUrlDataTransformer.php index 532fa122..a6fb4c14 100644 --- a/module/Core/src/Transformer/ShortUrlDataTransformer.php +++ b/module/Core/src/Transformer/ShortUrlDataTransformer.php @@ -24,16 +24,15 @@ class ShortUrlDataTransformer implements DataTransformerInterface */ public function transform($shortUrl): array // phpcs:ignore { - $longUrl = $shortUrl->getLongUrl(); - return [ 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => $shortUrl->toString($this->domainConfig), - 'longUrl' => $longUrl, + 'longUrl' => $shortUrl->getLongUrl(), 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), 'visitsCount' => $shortUrl->getVisitsCount(), 'tags' => invoke($shortUrl->getTags(), '__toString'), 'meta' => $this->buildMeta($shortUrl), + 'domain' => $shortUrl->getDomain(), ]; } diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index 3fec6f7a..40c9fa9c 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -24,6 +24,7 @@ class ListShortUrlsTest extends ApiTestCase 'validUntil' => null, 'maxVisits' => null, ], + 'domain' => null, ]; private const SHORT_URL_CUSTOM_SLUG_AND_DOMAIN = [ 'shortCode' => 'custom-with-domain', @@ -37,6 +38,7 @@ class ListShortUrlsTest extends ApiTestCase 'validUntil' => null, 'maxVisits' => null, ], + 'domain' => 'some-domain.com', ]; private const SHORT_URL_META = [ 'shortCode' => 'def456', @@ -52,6 +54,7 @@ class ListShortUrlsTest extends ApiTestCase 'validUntil' => null, 'maxVisits' => null, ], + 'domain' => null, ]; private const SHORT_URL_CUSTOM_SLUG = [ 'shortCode' => 'custom', @@ -65,6 +68,7 @@ class ListShortUrlsTest extends ApiTestCase 'validUntil' => null, 'maxVisits' => 2, ], + 'domain' => null, ]; private const SHORT_URL_CUSTOM_DOMAIN = [ 'shortCode' => 'ghi789', @@ -80,6 +84,7 @@ class ListShortUrlsTest extends ApiTestCase 'validUntil' => null, 'maxVisits' => null, ], + 'domain' => 'example.com', ]; /** From 75cd9774b750ad104478c740ddbc19f04a16a9b5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 09:15:43 +0100 Subject: [PATCH 11/23] Added optional domain query param to documentation for all rest endpoints that need it --- docs/swagger/parameters/domain.json | 9 +++++++++ docs/swagger/paths/v1_short-urls_{shortCode}.json | 14 +++++++------- .../paths/v1_short-urls_{shortCode}_tags.json | 3 +++ .../paths/v1_short-urls_{shortCode}_visits.json | 3 +++ 4 files changed, 22 insertions(+), 7 deletions(-) create mode 100644 docs/swagger/parameters/domain.json diff --git a/docs/swagger/parameters/domain.json b/docs/swagger/parameters/domain.json new file mode 100644 index 00000000..9a9b41b9 --- /dev/null +++ b/docs/swagger/parameters/domain.json @@ -0,0 +1,9 @@ +{ + "name": "domain", + "description": "The domain in which the short code should be searched for.", + "in": "query", + "required": false, + "schema": { + "type": "string" + } +} diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}.json b/docs/swagger/paths/v1_short-urls_{shortCode}.json index 4fbdeb8e..b9baad92 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}.json @@ -20,13 +20,7 @@ } }, { - "name": "domain", - "in": "query", - "description": "The domain in which the short code should be searched for. Will fall back to default domain if not found.", - "required": false, - "schema": { - "type": "string" - } + "$ref": "../parameters/domain.json" } ], "security": [ @@ -105,6 +99,9 @@ "schema": { "type": "string" } + }, + { + "$ref": "../parameters/domain.json" } ], "requestBody": { @@ -215,6 +212,9 @@ "schema": { "type": "string" } + }, + { + "$ref": "../parameters/domain.json" } ], "security": [ diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}_tags.json b/docs/swagger/paths/v1_short-urls_{shortCode}_tags.json index 4065f718..fd497380 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}_tags.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}_tags.json @@ -18,6 +18,9 @@ "schema": { "type": "string" } + }, + { + "$ref": "../parameters/domain.json" } ], "requestBody": { diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json b/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json index 2369ba13..03d66a99 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}_visits.json @@ -19,6 +19,9 @@ "type": "string" } }, + { + "$ref": "../parameters/domain.json" + }, { "name": "startDate", "in": "query", From aa80c2bb826e0cc105ef8dd8d6abf70604ad6540 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 09:51:17 +0100 Subject: [PATCH 12/23] Updated API tests so that fixture short URLs are created with matching short codes and different domains --- .../src/Service/VisitsTrackerInterface.php | 4 +- .../Action/EditShortUrlActionTest.php | 43 ++++++++++++++++--- .../test-api/Action/ListShortUrlsTest.php | 18 ++++++++ .../test-api/Fixtures/ShortUrlsFixture.php | 10 ++++- 4 files changed, 66 insertions(+), 9 deletions(-) diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 862ef190..1ec4e110 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -17,7 +17,7 @@ interface VisitsTrackerInterface /** * Tracks a new visit to provided short code from provided visitor */ - public function track(ShortUrl $shortUrl, Visitor $visitor): void; // FIXME + public function track(ShortUrl $shortUrl, Visitor $visitor): void; /** * Returns the visits on certain short code @@ -25,5 +25,5 @@ interface VisitsTrackerInterface * @return Visit[]|Paginator * @throws ShortUrlNotFoundException */ - public function info(ShortUrlIdentifier $identifier, VisitsParams $params): Paginator; // FIXME + public function info(ShortUrlIdentifier $identifier, VisitsParams $params): Paginator; } diff --git a/module/Rest/test-api/Action/EditShortUrlActionTest.php b/module/Rest/test-api/Action/EditShortUrlActionTest.php index 482accfe..0e5039e5 100644 --- a/module/Rest/test-api/Action/EditShortUrlActionTest.php +++ b/module/Rest/test-api/Action/EditShortUrlActionTest.php @@ -7,9 +7,10 @@ namespace ShlinkioApiTest\Shlink\Rest\Action; use Cake\Chronos\Chronos; use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts; use GuzzleHttp\RequestOptions; +use Laminas\Diactoros\Uri; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; -use function Functional\first; +use function GuzzleHttp\Psr7\build_query; use function sprintf; class EditShortUrlActionTest extends ApiTestCase @@ -61,10 +62,9 @@ class EditShortUrlActionTest extends ApiTestCase private function findShortUrlMetaByShortCode(string $shortCode): ?array { - // FIXME Call GET /short-urls/{shortCode} once issue https://github.com/shlinkio/shlink/issues/628 is fixed - $allShortUrls = $this->getJsonResponsePayload($this->callApiWithKey(self::METHOD_GET, '/short-urls')); - $list = $allShortUrls['shortUrls']['data'] ?? []; - $matchingShortUrl = first($list, fn (array $shortUrl) => $shortUrl['shortCode'] ?? '' === $shortCode); + $matchingShortUrl = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/' . $shortCode), + ); return $matchingShortUrl['meta'] ?? null; } @@ -101,4 +101,37 @@ class EditShortUrlActionTest extends ApiTestCase $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Invalid data', $payload['title']); } + + /** + * @test + * @dataProvider provideDomains + */ + public function metadataIsEditedOnProperShortUrlBasedOnDomain(?string $domain, string $expectedUrl): void + { + $shortCode = 'ghi789'; + $url = new Uri(sprintf('/short-urls/%s', $shortCode)); + + if ($domain !== null) { + $url = $url->withQuery(build_query(['domain' => $domain])); + } + + $editResp = $this->callApiWithKey(self::METHOD_PATCH, (string) $url, [RequestOptions::JSON => [ + 'maxVisits' => 100, + ]]); + $editedShortUrl = $this->getJsonResponsePayload($this->callApiWithKey(self::METHOD_GET, (string) $url)); + + $this->assertEquals(self::STATUS_NO_CONTENT, $editResp->getStatusCode()); + $this->assertEquals($domain, $editedShortUrl['domain']); + $this->assertEquals($expectedUrl, $editedShortUrl['longUrl']); + $this->assertEquals(100, $editedShortUrl['meta']['maxVisits'] ?? null); + } + + public function provideDomains(): iterable + { + yield 'domain' => [null, 'https://shlink.io/documentation/']; + yield 'no domain' => [ + 'example.com', + 'https://blog.alejandrocelaya.com/2019/04/27/considerations-to-properly-use-open-source-software-projects/', + ]; + } } diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index 40c9fa9c..95729e2d 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -26,6 +26,20 @@ class ListShortUrlsTest extends ApiTestCase ], 'domain' => null, ]; + private const SHORT_URL_DOCS = [ + 'shortCode' => 'ghi789', + 'shortUrl' => 'http://doma.in/ghi789', + 'longUrl' => 'https://shlink.io/documentation/', + 'dateCreated' => '2018-05-01T00:00:00+00:00', + 'visitsCount' => 0, + 'tags' => [], + 'meta' => [ + 'validSince' => null, + 'validUntil' => null, + 'maxVisits' => null, + ], + 'domain' => null, + ]; private const SHORT_URL_CUSTOM_SLUG_AND_DOMAIN = [ 'shortCode' => 'custom-with-domain', 'shortUrl' => 'http://some-domain.com/custom-with-domain', @@ -109,6 +123,7 @@ class ListShortUrlsTest extends ApiTestCase { yield [[], [ self::SHORT_URL_SHLINK, + self::SHORT_URL_DOCS, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG, @@ -119,9 +134,11 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_CUSTOM_SLUG, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, self::SHORT_URL_META, + self::SHORT_URL_DOCS, self::SHORT_URL_CUSTOM_DOMAIN, ]]; yield [['orderBy' => ['shortCode' => 'DESC']], [ + self::SHORT_URL_DOCS, self::SHORT_URL_CUSTOM_DOMAIN, self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, @@ -135,6 +152,7 @@ class ListShortUrlsTest extends ApiTestCase ]]; yield [['endDate' => Chronos::parse('2018-12-01')->toAtomString()], [ self::SHORT_URL_SHLINK, + self::SHORT_URL_DOCS, self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, ]]; yield [['tags' => ['foo']], [ diff --git a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php index d7566063..deab73b9 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php @@ -37,11 +37,17 @@ class ShortUrlsFixture extends AbstractFixture ), '2019-01-01 00:00:20'); $manager->persist($customShortUrl); - $withDomainShortUrl = $this->setShortUrlDate(new ShortUrl( + $ghiShortUrl = $this->setShortUrlDate( + new ShortUrl('https://shlink.io/documentation/', ShortUrlMeta::fromRawData(['customSlug' => 'ghi789'])), + '2018-05-01', + ); + $manager->persist($ghiShortUrl); + + $withDomainDuplicatingShortCode = $this->setShortUrlDate(new ShortUrl( 'https://blog.alejandrocelaya.com/2019/04/27/considerations-to-properly-use-open-source-software-projects/', ShortUrlMeta::fromRawData(['domain' => 'example.com', 'customSlug' => 'ghi789']), ), '2019-01-01 00:00:30'); - $manager->persist($withDomainShortUrl); + $manager->persist($withDomainDuplicatingShortCode); $withDomainAndSlugShortUrl = $this->setShortUrlDate(new ShortUrl( 'https://google.com', From 881002634ad149729bf94911b5d2429be905b81e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 10:28:10 +0100 Subject: [PATCH 13/23] Added API tests for short URL deletion with domain --- .../Action/DeleteShortUrlActionTest.php | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php index 7bf01c51..d9e14113 100644 --- a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php +++ b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php @@ -42,4 +42,29 @@ class DeleteShortUrlActionTest extends ApiTestCase $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Cannot delete short URL', $payload['title']); } + + /** @test */ + public function properShortUrlIsDeletedWhenDomainIsProvided(): void + { + $fetchWithDomainBefore = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), + ); + $fetchWithoutDomainBefore = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), + ); + $deleteResp = $this->callApiWithKey(self::METHOD_DELETE, '/short-urls/ghi789?domain=example.com'); + $fetchWithDomainAfter = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), + ); + $fetchWithoutDomainAfter = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), + ); + + $this->assertEquals('example.com', $fetchWithDomainBefore['domain']); + $this->assertEquals(null, $fetchWithoutDomainBefore['domain']); + $this->assertEquals(self::STATUS_NO_CONTENT, $deleteResp->getStatusCode()); + // Falls back to the one without domain, since the other one has been deleted + $this->assertEquals(null, $fetchWithDomainAfter['domain']); + $this->assertEquals(null, $fetchWithoutDomainAfter['domain']); + } } From e58f2a384e25640b3253b1d28d833d2b9f05b7c5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 10:46:38 +0100 Subject: [PATCH 14/23] Added API test for visits with and without domain --- .../Action/EditShortUrlActionTest.php | 4 +-- .../test-api/Action/GetVisitsActionTest.php | 30 +++++++++++++++++++ .../test-api/Action/ListShortUrlsTest.php | 2 +- .../test-api/Fixtures/ShortUrlsFixture.php | 1 + .../Rest/test-api/Fixtures/VisitsFixture.php | 5 ++++ 5 files changed, 39 insertions(+), 3 deletions(-) diff --git a/module/Rest/test-api/Action/EditShortUrlActionTest.php b/module/Rest/test-api/Action/EditShortUrlActionTest.php index 0e5039e5..aeb1b990 100644 --- a/module/Rest/test-api/Action/EditShortUrlActionTest.php +++ b/module/Rest/test-api/Action/EditShortUrlActionTest.php @@ -128,10 +128,10 @@ class EditShortUrlActionTest extends ApiTestCase public function provideDomains(): iterable { - yield 'domain' => [null, 'https://shlink.io/documentation/']; - yield 'no domain' => [ + yield 'domain' => [ 'example.com', 'https://blog.alejandrocelaya.com/2019/04/27/considerations-to-properly-use-open-source-software-projects/', ]; + yield 'no domain' => [null, 'https://shlink.io/documentation/']; } } diff --git a/module/Rest/test-api/Action/GetVisitsActionTest.php b/module/Rest/test-api/Action/GetVisitsActionTest.php index f6167f78..df4ee6cc 100644 --- a/module/Rest/test-api/Action/GetVisitsActionTest.php +++ b/module/Rest/test-api/Action/GetVisitsActionTest.php @@ -4,8 +4,12 @@ declare(strict_types=1); namespace ShlinkioApiTest\Shlink\Rest\Action; +use Laminas\Diactoros\Uri; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; +use function GuzzleHttp\Psr7\build_query; +use function sprintf; + class GetVisitsActionTest extends ApiTestCase { /** @test */ @@ -23,4 +27,30 @@ class GetVisitsActionTest extends ApiTestCase $this->assertEquals('Short URL not found', $payload['title']); $this->assertEquals('invalid', $payload['shortCode']); } + + /** + * @test + * @dataProvider provideDomains + */ + public function properVisitsAreReturnedWhenDomainIsProvided(?string $domain, int $expectedAmountOfVisits): void + { + $shortCode = 'ghi789'; + $url = new Uri(sprintf('/short-urls/%s/visits', $shortCode)); + + if ($domain !== null) { + $url = $url->withQuery(build_query(['domain' => $domain])); + } + + $resp = $this->callApiWithKey(self::METHOD_GET, (string) $url); + $payload = $this->getJsonResponsePayload($resp); + + $this->assertEquals($expectedAmountOfVisits, $payload['visits']['pagination']['totalItems'] ?? -1); + $this->assertCount($expectedAmountOfVisits, $payload['visits']['data'] ?? []); + } + + public function provideDomains(): iterable + { + yield 'domain' => ['example.com', 0]; + yield 'no domain' => [null, 2]; + } } diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index 95729e2d..7af30948 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -31,7 +31,7 @@ class ListShortUrlsTest extends ApiTestCase 'shortUrl' => 'http://doma.in/ghi789', 'longUrl' => 'https://shlink.io/documentation/', 'dateCreated' => '2018-05-01T00:00:00+00:00', - 'visitsCount' => 0, + 'visitsCount' => 2, 'tags' => [], 'meta' => [ 'validSince' => null, diff --git a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php index deab73b9..0aa13a82 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlsFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlsFixture.php @@ -59,6 +59,7 @@ class ShortUrlsFixture extends AbstractFixture $this->addReference('abc123_short_url', $abcShortUrl); $this->addReference('def456_short_url', $defShortUrl); + $this->addReference('ghi789_short_url', $ghiShortUrl); } private function setShortUrlDate(ShortUrl $shortUrl, string $date): ShortUrl diff --git a/module/Rest/test-api/Fixtures/VisitsFixture.php b/module/Rest/test-api/Fixtures/VisitsFixture.php index 2c85c1a1..a07d95d1 100644 --- a/module/Rest/test-api/Fixtures/VisitsFixture.php +++ b/module/Rest/test-api/Fixtures/VisitsFixture.php @@ -31,6 +31,11 @@ class VisitsFixture extends AbstractFixture implements DependentFixtureInterface $manager->persist(new Visit($defShortUrl, new Visitor('shlink-tests-agent', '', '127.0.0.1'))); $manager->persist(new Visit($defShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', ''))); + /** @var ShortUrl $defShortUrl */ + $defShortUrl = $this->getReference('ghi789_short_url'); + $manager->persist(new Visit($defShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4'))); + $manager->persist(new Visit($defShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', ''))); + $manager->flush(); } } From e87d4d61bc7a15adb27df6e9df9bde284262106a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 10:53:49 +0100 Subject: [PATCH 15/23] Added API test for editing tags with and without domain --- .../Action/EditShortUrlTagsActionTest.php | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php b/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php index d48ffc5f..f120adf8 100644 --- a/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php +++ b/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php @@ -41,4 +41,25 @@ class EditShortUrlTagsActionTest extends ApiTestCase $this->assertEquals('Short URL not found', $payload['title']); $this->assertEquals('invalid', $payload['shortCode']); } + + /** @test */ + public function tagsAreSetOnProperShortUrlBasedOnProvidedDomain(): void + { + $urlWithoutDomain = '/short-urls/ghi789/tags'; + $urlWithDomain = $urlWithoutDomain . '?domain=example.com'; + + $setTagsWithDomain = $this->callApiWithKey(self::METHOD_PUT, $urlWithDomain, [RequestOptions::JSON => [ + 'tags' => ['foo', 'bar'], + ]]); + $fetchWithoutDomain = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), + ); + $fetchWithDomain = $this->getJsonResponsePayload( + $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), + ); + + $this->assertEquals(self::STATUS_OK, $setTagsWithDomain->getStatusCode()); + $this->assertEquals([], $fetchWithoutDomain['tags']); + $this->assertEquals(['bar', 'foo'], $fetchWithDomain['tags']); + } } From 10f79ec01d96f81989cf285fd90e54e01f55020f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 12:44:35 +0100 Subject: [PATCH 16/23] Created new repository method which will look for short URLs without doing domain fallback --- .../src/Repository/ShortUrlRepository.php | 31 +++++++++---- .../ShortUrlRepositoryInterface.php | 4 +- .../src/Service/ShortUrl/ShortUrlResolver.php | 2 +- .../Repository/ShortUrlRepositoryTest.php | 45 +++++++++++++++---- .../Service/ShortUrl/ShortUrlResolverTest.php | 8 ++-- 5 files changed, 67 insertions(+), 23 deletions(-) diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index cd96acfc..a9d21952 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -90,8 +90,8 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI ?DateRange $dateRange = null ): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->from(ShortUrl::class, 's'); - $qb->where('1=1'); + $qb->from(ShortUrl::class, 's') + ->where('1=1'); if ($dateRange !== null && $dateRange->getStartDate() !== null) { $qb->andWhere($qb->expr()->gte('s.dateCreated', ':startDate')); @@ -127,7 +127,7 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI return $qb; } - public function findOneByShortCode(string $shortCode, ?string $domain = null): ?ShortUrl + public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl { // When ordering DESC, Postgres puts nulls at the beginning while the rest of supported DB engines put them at // the bottom @@ -159,14 +159,30 @@ DQL; return $query->getOneOrNullResult(); } + public function findOne(string $shortCode, ?string $domain = null): ?ShortUrl + { + $qb = $this->createFindOneQueryBuilder($shortCode, $domain); + $qb->select('s'); + + return $qb->getQuery()->getOneOrNullResult(); + } + public function shortCodeIsInUse(string $slug, ?string $domain = null): bool + { + $qb = $this->createFindOneQueryBuilder($slug, $domain); + $qb->select('COUNT(DISTINCT s.id)'); + + return ((int) $qb->getQuery()->getSingleScalarResult()) > 0; + } + + private function createFindOneQueryBuilder(string $slug, ?string $domain = null): QueryBuilder { $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->select('COUNT(DISTINCT s.id)') - ->from(ShortUrl::class, 's') + $qb->from(ShortUrl::class, 's') ->where($qb->expr()->isNotNull('s.shortCode')) ->andWhere($qb->expr()->eq('s.shortCode', ':slug')) - ->setParameter('slug', $slug); + ->setParameter('slug', $slug) + ->setMaxResults(1); if ($domain !== null) { $qb->join('s.domain', 'd') @@ -176,7 +192,6 @@ DQL; $qb->andWhere($qb->expr()->isNull('s.domain')); } - $result = (int) $qb->getQuery()->getSingleScalarResult(); - return $result > 0; + return $qb; } } diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index d0acdf1a..065198b4 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -22,7 +22,9 @@ interface ShortUrlRepositoryInterface extends ObjectRepository public function countList(?string $searchTerm = null, array $tags = [], ?DateRange $dateRange = null): int; - public function findOneByShortCode(string $shortCode, ?string $domain = null): ?ShortUrl; + public function findOneWithDomainFallback(string $shortCode, ?string $domain = null): ?ShortUrl; + + public function findOne(string $shortCode, ?string $domain = null): ?ShortUrl; public function shortCodeIsInUse(string $slug, ?string $domain): bool; } diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index d7a71b1c..4f44f671 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -26,7 +26,7 @@ class ShortUrlResolver implements ShortUrlResolverInterface { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOneByShortCode($identifier->shortCode(), $identifier->domain()); + $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); if ($shortUrl === null) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 4af2fd61..4829fada 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -37,7 +37,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase } /** @test */ - public function findOneByShortCodeReturnsProperData(): void + public function findOneWithDomainFallbackReturnsProperData(): void { $regularOne = new ShortUrl('foo', ShortUrlMeta::fromRawData(['customSlug' => 'foo'])); $this->getEntityManager()->persist($regularOne); @@ -54,20 +54,25 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $this->assertSame($regularOne, $this->repo->findOneByShortCode($regularOne->getShortCode())); - $this->assertSame($regularOne, $this->repo->findOneByShortCode($withDomainDuplicatingRegular->getShortCode())); - $this->assertSame($withDomain, $this->repo->findOneByShortCode($withDomain->getShortCode(), 'example.com')); + $this->assertSame($regularOne, $this->repo->findOneWithDomainFallback($regularOne->getShortCode())); + $this->assertSame($regularOne, $this->repo->findOneWithDomainFallback( + $withDomainDuplicatingRegular->getShortCode(), + )); + $this->assertSame($withDomain, $this->repo->findOneWithDomainFallback( + $withDomain->getShortCode(), + 'example.com', + )); $this->assertSame( $withDomainDuplicatingRegular, - $this->repo->findOneByShortCode($withDomainDuplicatingRegular->getShortCode(), 'doma.in'), + $this->repo->findOneWithDomainFallback($withDomainDuplicatingRegular->getShortCode(), 'doma.in'), ); $this->assertSame( $regularOne, - $this->repo->findOneByShortCode($withDomainDuplicatingRegular->getShortCode(), 'other-domain.com'), + $this->repo->findOneWithDomainFallback($withDomainDuplicatingRegular->getShortCode(), 'other-domain.com'), ); - $this->assertNull($this->repo->findOneByShortCode('invalid')); - $this->assertNull($this->repo->findOneByShortCode($withDomain->getShortCode())); - $this->assertNull($this->repo->findOneByShortCode($withDomain->getShortCode(), 'other-domain.com')); + $this->assertNull($this->repo->findOneWithDomainFallback('invalid')); + $this->assertNull($this->repo->findOneWithDomainFallback($withDomain->getShortCode())); + $this->assertNull($this->repo->findOneWithDomainFallback($withDomain->getShortCode(), 'other-domain.com')); } /** @test */ @@ -183,4 +188,26 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->assertFalse($this->repo->shortCodeIsInUse('another-slug', 'example.com')); $this->assertTrue($this->repo->shortCodeIsInUse('another-slug', 'doma.in')); } + + /** @test */ + public function findOneLooksForShortUrlInProperSetOfTables(): void + { + $shortUrlWithoutDomain = new ShortUrl('foo', ShortUrlMeta::fromRawData(['customSlug' => 'my-cool-slug'])); + $this->getEntityManager()->persist($shortUrlWithoutDomain); + + $shortUrlWithDomain = new ShortUrl( + 'foo', + ShortUrlMeta::fromRawData(['domain' => 'doma.in', 'customSlug' => 'another-slug']), + ); + $this->getEntityManager()->persist($shortUrlWithDomain); + + $this->getEntityManager()->flush(); + + $this->assertNotNull($this->repo->findOne('my-cool-slug')); + $this->assertNull($this->repo->findOne('my-cool-slug', 'doma.in')); + $this->assertNull($this->repo->findOne('slug-not-in-use')); + $this->assertNull($this->repo->findOne('another-slug')); + $this->assertNull($this->repo->findOne('another-slug', 'example.com')); + $this->assertNotNull($this->repo->findOne('another-slug', 'doma.in')); + } } diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index cde37599..fb3b4e68 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -39,7 +39,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); @@ -55,7 +55,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = 'abc123'; $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn(null); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn(null); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class); @@ -72,7 +72,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode)); @@ -91,7 +91,7 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneByShortCode($shortCode, null)->willReturn($shortUrl); + $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class); From 297985cf0164c2d5879ff80e4b0f10e04f3d4d27 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 12:58:26 +0100 Subject: [PATCH 17/23] Ensured trying to fetch a short URL for any operation through the API results in 404 if it does not match with porovided domain --- .../src/Service/ShortUrl/ShortUrlResolver.php | 8 +++--- .../Service/ShortUrl/ShortUrlResolverTest.php | 8 +++--- .../Action/DeleteShortUrlActionTest.php | 25 ++++++------------- 3 files changed, 17 insertions(+), 24 deletions(-) diff --git a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php index 4f44f671..414a3446 100644 --- a/module/Core/src/Service/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/Service/ShortUrl/ShortUrlResolver.php @@ -26,7 +26,7 @@ class ShortUrlResolver implements ShortUrlResolverInterface { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); - $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); + $shortUrl = $shortUrlRepo->findOne($identifier->shortCode(), $identifier->domain()); if ($shortUrl === null) { throw ShortUrlNotFoundException::fromNotFound($identifier); } @@ -39,8 +39,10 @@ class ShortUrlResolver implements ShortUrlResolverInterface */ public function resolveEnabledShortUrl(ShortUrlIdentifier $identifier): ShortUrl { - $shortUrl = $this->resolveShortUrl($identifier); - if (! $shortUrl->isEnabled()) { + /** @var ShortUrlRepository $shortUrlRepo */ + $shortUrlRepo = $this->em->getRepository(ShortUrl::class); + $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier->shortCode(), $identifier->domain()); + if ($shortUrl === null || ! $shortUrl->isEnabled()) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index fb3b4e68..5b3e3e19 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -39,13 +39,13 @@ class ShortUrlResolverTest extends TestCase $shortCode = $shortUrl->getShortCode(); $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn($shortUrl); + $findOne = $repo->findOne($shortCode, null)->willReturn($shortUrl); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $result = $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); $this->assertSame($shortUrl, $result); - $findOneByShortCode->shouldHaveBeenCalledOnce(); + $findOne->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); } @@ -55,11 +55,11 @@ class ShortUrlResolverTest extends TestCase $shortCode = 'abc123'; $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $findOneByShortCode = $repo->findOneWithDomainFallback($shortCode, null)->willReturn(null); + $findOne = $repo->findOne($shortCode, null)->willReturn(null); $getRepo = $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); $this->expectException(ShortUrlNotFoundException::class); - $findOneByShortCode->shouldBeCalledOnce(); + $findOne->shouldBeCalledOnce(); $getRepo->shouldBeCalledOnce(); $this->urlResolver->resolveShortUrl(new ShortUrlIdentifier($shortCode)); diff --git a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php index d9e14113..539c6296 100644 --- a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php +++ b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php @@ -46,25 +46,16 @@ class DeleteShortUrlActionTest extends ApiTestCase /** @test */ public function properShortUrlIsDeletedWhenDomainIsProvided(): void { - $fetchWithDomainBefore = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), - ); - $fetchWithoutDomainBefore = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), - ); + $fetchWithDomainBefore = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'); + $fetchWithoutDomainBefore = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'); $deleteResp = $this->callApiWithKey(self::METHOD_DELETE, '/short-urls/ghi789?domain=example.com'); - $fetchWithDomainAfter = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'), - ); - $fetchWithoutDomainAfter = $this->getJsonResponsePayload( - $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'), - ); + $fetchWithDomainAfter = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789?domain=example.com'); + $fetchWithoutDomainAfter = $this->callApiWithKey(self::METHOD_GET, '/short-urls/ghi789'); - $this->assertEquals('example.com', $fetchWithDomainBefore['domain']); - $this->assertEquals(null, $fetchWithoutDomainBefore['domain']); + $this->assertEquals(self::STATUS_OK, $fetchWithDomainBefore->getStatusCode()); + $this->assertEquals(self::STATUS_OK, $fetchWithoutDomainBefore->getStatusCode()); $this->assertEquals(self::STATUS_NO_CONTENT, $deleteResp->getStatusCode()); - // Falls back to the one without domain, since the other one has been deleted - $this->assertEquals(null, $fetchWithDomainAfter['domain']); - $this->assertEquals(null, $fetchWithoutDomainAfter['domain']); + $this->assertEquals(self::STATUS_NOT_FOUND, $fetchWithDomainAfter->getStatusCode()); + $this->assertEquals(self::STATUS_OK, $fetchWithoutDomainAfter->getStatusCode()); } } From fe652c67f4e4499069cdde576f233c617d394ad7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 13:15:08 +0100 Subject: [PATCH 18/23] Covered with API tests getting invalid short URLs by short code and domain --- .../Action/DeleteShortUrlActionTest.php | 20 +++++++---- .../Action/EditShortUrlActionTest.php | 22 +++++++++---- .../Action/EditShortUrlTagsActionTest.php | 23 +++++++++---- .../test-api/Action/GetVisitsActionTest.php | 20 +++++++---- .../Action/ResolveShortUrlActionTest.php | 22 +++++++++---- .../Utils/NotFoundUrlHelpersTrait.php | 33 +++++++++++++++++++ 6 files changed, 107 insertions(+), 33 deletions(-) create mode 100644 module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php diff --git a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php index 539c6296..ef32190b 100644 --- a/module/Rest/test-api/Action/DeleteShortUrlActionTest.php +++ b/module/Rest/test-api/Action/DeleteShortUrlActionTest.php @@ -5,15 +5,22 @@ declare(strict_types=1); namespace ShlinkioApiTest\Shlink\Rest\Action; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; +use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait; class DeleteShortUrlActionTest extends ApiTestCase { - /** @test */ - public function notFoundErrorIsReturnWhenDeletingInvalidUrl(): void - { - $expectedDetail = 'No URL found with short code "invalid"'; + use NotFoundUrlHelpersTrait; - $resp = $this->callApiWithKey(self::METHOD_DELETE, '/short-urls/invalid'); + /** + * @test + * @dataProvider provideInvalidUrls + */ + public function notFoundErrorIsReturnWhenDeletingInvalidUrl( + string $shortCode, + ?string $domain, + string $expectedDetail + ): void { + $resp = $this->callApiWithKey(self::METHOD_DELETE, $this->buildShortUrlPath($shortCode, $domain)); $payload = $this->getJsonResponsePayload($resp); $this->assertEquals(self::STATUS_NOT_FOUND, $resp->getStatusCode()); @@ -21,7 +28,8 @@ class DeleteShortUrlActionTest extends ApiTestCase $this->assertEquals('INVALID_SHORTCODE', $payload['type']); $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Short URL not found', $payload['title']); - $this->assertEquals('invalid', $payload['shortCode']); + $this->assertEquals($shortCode, $payload['shortCode']); + $this->assertEquals($domain, $payload['domain'] ?? null); } /** @test */ diff --git a/module/Rest/test-api/Action/EditShortUrlActionTest.php b/module/Rest/test-api/Action/EditShortUrlActionTest.php index aeb1b990..d7d425dc 100644 --- a/module/Rest/test-api/Action/EditShortUrlActionTest.php +++ b/module/Rest/test-api/Action/EditShortUrlActionTest.php @@ -10,12 +10,14 @@ use GuzzleHttp\RequestOptions; use Laminas\Diactoros\Uri; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; +use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait; use function GuzzleHttp\Psr7\build_query; use function sprintf; class EditShortUrlActionTest extends ApiTestCase { use ArraySubsetAsserts; + use NotFoundUrlHelpersTrait; /** * @test @@ -69,12 +71,17 @@ class EditShortUrlActionTest extends ApiTestCase return $matchingShortUrl['meta'] ?? null; } - /** @test */ - public function tryingToEditInvalidUrlReturnsNotFoundError(): void - { - $expectedDetail = 'No URL found with short code "invalid"'; - - $resp = $this->callApiWithKey(self::METHOD_PATCH, '/short-urls/invalid', [RequestOptions::JSON => []]); + /** + * @test + * @dataProvider provideInvalidUrls + */ + public function tryingToEditInvalidUrlReturnsNotFoundError( + string $shortCode, + ?string $domain, + string $expectedDetail + ): void { + $url = $this->buildShortUrlPath($shortCode, $domain); + $resp = $this->callApiWithKey(self::METHOD_PATCH, $url, [RequestOptions::JSON => []]); $payload = $this->getJsonResponsePayload($resp); $this->assertEquals(self::STATUS_NOT_FOUND, $resp->getStatusCode()); @@ -82,7 +89,8 @@ class EditShortUrlActionTest extends ApiTestCase $this->assertEquals('INVALID_SHORTCODE', $payload['type']); $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Short URL not found', $payload['title']); - $this->assertEquals('invalid', $payload['shortCode']); + $this->assertEquals($shortCode, $payload['shortCode']); + $this->assertEquals($domain, $payload['domain'] ?? null); } /** @test */ diff --git a/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php b/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php index f120adf8..0433a388 100644 --- a/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php +++ b/module/Rest/test-api/Action/EditShortUrlTagsActionTest.php @@ -6,9 +6,12 @@ namespace ShlinkioApiTest\Shlink\Rest\Action; use GuzzleHttp\RequestOptions; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; +use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait; class EditShortUrlTagsActionTest extends ApiTestCase { + use NotFoundUrlHelpersTrait; + /** @test */ public function notProvidingTagsReturnsBadRequest(): void { @@ -24,12 +27,17 @@ class EditShortUrlTagsActionTest extends ApiTestCase $this->assertEquals('Invalid data', $payload['title']); } - /** @test */ - public function providingInvalidShortCodeReturnsBadRequest(): void - { - $expectedDetail = 'No URL found with short code "invalid"'; - - $resp = $this->callApiWithKey(self::METHOD_PUT, '/short-urls/invalid/tags', [RequestOptions::JSON => [ + /** + * @test + * @dataProvider provideInvalidUrls + */ + public function providingInvalidShortCodeReturnsBadRequest( + string $shortCode, + ?string $domain, + string $expectedDetail + ): void { + $url = $this->buildShortUrlPath($shortCode, $domain, '/tags'); + $resp = $this->callApiWithKey(self::METHOD_PUT, $url, [RequestOptions::JSON => [ 'tags' => ['foo', 'bar'], ]]); $payload = $this->getJsonResponsePayload($resp); @@ -39,7 +47,8 @@ class EditShortUrlTagsActionTest extends ApiTestCase $this->assertEquals('INVALID_SHORTCODE', $payload['type']); $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Short URL not found', $payload['title']); - $this->assertEquals('invalid', $payload['shortCode']); + $this->assertEquals($shortCode, $payload['shortCode']); + $this->assertEquals($domain, $payload['domain'] ?? null); } /** @test */ diff --git a/module/Rest/test-api/Action/GetVisitsActionTest.php b/module/Rest/test-api/Action/GetVisitsActionTest.php index df4ee6cc..cee466a3 100644 --- a/module/Rest/test-api/Action/GetVisitsActionTest.php +++ b/module/Rest/test-api/Action/GetVisitsActionTest.php @@ -6,18 +6,25 @@ namespace ShlinkioApiTest\Shlink\Rest\Action; use Laminas\Diactoros\Uri; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; +use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait; use function GuzzleHttp\Psr7\build_query; use function sprintf; class GetVisitsActionTest extends ApiTestCase { - /** @test */ - public function tryingToGetVisitsForInvalidUrlReturnsNotFoundError(): void - { - $expectedDetail = 'No URL found with short code "invalid"'; + use NotFoundUrlHelpersTrait; - $resp = $this->callApiWithKey(self::METHOD_GET, '/short-urls/invalid/visits'); + /** + * @test + * @dataProvider provideInvalidUrls + */ + public function tryingToGetVisitsForInvalidUrlReturnsNotFoundError( + string $shortCode, + ?string $domain, + string $expectedDetail + ): void { + $resp = $this->callApiWithKey(self::METHOD_GET, $this->buildShortUrlPath($shortCode, $domain, '/visits')); $payload = $this->getJsonResponsePayload($resp); $this->assertEquals(self::STATUS_NOT_FOUND, $resp->getStatusCode()); @@ -25,7 +32,8 @@ class GetVisitsActionTest extends ApiTestCase $this->assertEquals('INVALID_SHORTCODE', $payload['type']); $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Short URL not found', $payload['title']); - $this->assertEquals('invalid', $payload['shortCode']); + $this->assertEquals($shortCode, $payload['shortCode']); + $this->assertEquals($domain, $payload['domain'] ?? null); } /** diff --git a/module/Rest/test-api/Action/ResolveShortUrlActionTest.php b/module/Rest/test-api/Action/ResolveShortUrlActionTest.php index 27d9dd69..d76d7946 100644 --- a/module/Rest/test-api/Action/ResolveShortUrlActionTest.php +++ b/module/Rest/test-api/Action/ResolveShortUrlActionTest.php @@ -7,11 +7,14 @@ namespace ShlinkioApiTest\Shlink\Rest\Action; use Cake\Chronos\Chronos; use GuzzleHttp\RequestOptions; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; +use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait; use function sprintf; class ResolveShortUrlActionTest extends ApiTestCase { + use NotFoundUrlHelpersTrait; + /** * @test * @dataProvider provideDisabledMeta @@ -40,12 +43,16 @@ class ResolveShortUrlActionTest extends ApiTestCase yield 'maxVisits reached' => [['maxVisits' => 1]]; } - /** @test */ - public function tryingToResolveInvalidUrlReturnsNotFoundError(): void - { - $expectedDetail = 'No URL found with short code "invalid"'; - - $resp = $this->callApiWithKey(self::METHOD_GET, '/short-urls/invalid'); + /** + * @test + * @dataProvider provideInvalidUrls + */ + public function tryingToResolveInvalidUrlReturnsNotFoundError( + string $shortCode, + ?string $domain, + string $expectedDetail + ): void { + $resp = $this->callApiWithKey(self::METHOD_GET, $this->buildShortUrlPath($shortCode, $domain)); $payload = $this->getJsonResponsePayload($resp); $this->assertEquals(self::STATUS_NOT_FOUND, $resp->getStatusCode()); @@ -53,6 +60,7 @@ class ResolveShortUrlActionTest extends ApiTestCase $this->assertEquals('INVALID_SHORTCODE', $payload['type']); $this->assertEquals($expectedDetail, $payload['detail']); $this->assertEquals('Short URL not found', $payload['title']); - $this->assertEquals('invalid', $payload['shortCode']); + $this->assertEquals($shortCode, $payload['shortCode']); + $this->assertEquals($domain, $payload['domain'] ?? null); } } diff --git a/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php b/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php new file mode 100644 index 00000000..fab658f0 --- /dev/null +++ b/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php @@ -0,0 +1,33 @@ + ['invalid', null, 'No URL found with short code "invalid"']; + yield 'invalid shortcode + domain' => [ + 'abc123', + 'example.com', + 'No URL found with short code "abc123" for domain "example.com"', + ]; + } + + public function buildShortUrlPath(string $shortCode, ?string $domain, string $suffix = ''): string + { + $url = new Uri(sprintf('/short-urls/%s%s', $shortCode, $suffix)); + if ($domain !== null) { + $url = $url->withQuery(build_query(['domain' => $domain])); + } + + return (string) $url; + } +} From c07c37f7bd675444436720fb7b6c5aa41e3d3874 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 19:03:43 +0100 Subject: [PATCH 19/23] Created middleware to drop domain from query when it is the default one --- module/Rest/config/dependencies.config.php | 3 ++ module/Rest/config/routes.config.php | 21 ++++++------- .../DropDefaultDomainFromQueryMiddleware.php | 31 +++++++++++++++++++ 3 files changed, 44 insertions(+), 11 deletions(-) create mode 100644 module/Rest/src/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddleware.php diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 6938b6ba..0058b100 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -37,6 +37,7 @@ return [ Middleware\BodyParserMiddleware::class => InvokableFactory::class, Middleware\CrossDomainMiddleware::class => InvokableFactory::class, Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class => InvokableFactory::class, + Middleware\ShortUrl\DropDefaultDomainFromQueryMiddleware::class => ConfigAbstractFactory::class, ], ], @@ -72,6 +73,8 @@ return [ Action\Tag\DeleteTagsAction::class => [Service\Tag\TagService::class, LoggerInterface::class], Action\Tag\CreateTagsAction::class => [Service\Tag\TagService::class, LoggerInterface::class], Action\Tag\UpdateTagAction::class => [Service\Tag\TagService::class, LoggerInterface::class], + + Middleware\ShortUrl\DropDefaultDomainFromQueryMiddleware::class => ['config.url_shortener.domain.hostname'], ], ]; diff --git a/module/Rest/config/routes.config.php b/module/Rest/config/routes.config.php index d210f13b..301691aa 100644 --- a/module/Rest/config/routes.config.php +++ b/module/Rest/config/routes.config.php @@ -4,26 +4,25 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest; +$contentNegotiationMiddleware = [Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class]; +$dropDomainMiddleware = [Middleware\ShortUrl\DropDefaultDomainFromQueryMiddleware::class]; + return [ 'routes' => [ Action\HealthAction::getRouteDef(), // Short codes - Action\ShortUrl\CreateShortUrlAction::getRouteDef([ - Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class, - ]), - Action\ShortUrl\SingleStepCreateShortUrlAction::getRouteDef([ - Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class, - ]), - Action\ShortUrl\EditShortUrlAction::getRouteDef(), - Action\ShortUrl\DeleteShortUrlAction::getRouteDef(), - Action\ShortUrl\ResolveShortUrlAction::getRouteDef(), + Action\ShortUrl\CreateShortUrlAction::getRouteDef($contentNegotiationMiddleware), + Action\ShortUrl\SingleStepCreateShortUrlAction::getRouteDef($contentNegotiationMiddleware), + Action\ShortUrl\EditShortUrlAction::getRouteDef($dropDomainMiddleware), + Action\ShortUrl\DeleteShortUrlAction::getRouteDef($dropDomainMiddleware), + Action\ShortUrl\ResolveShortUrlAction::getRouteDef($dropDomainMiddleware), Action\ShortUrl\ListShortUrlsAction::getRouteDef(), - Action\ShortUrl\EditShortUrlTagsAction::getRouteDef(), + Action\ShortUrl\EditShortUrlTagsAction::getRouteDef($dropDomainMiddleware), // Visits - Action\Visit\GetVisitsAction::getRouteDef(), + Action\Visit\GetVisitsAction::getRouteDef($dropDomainMiddleware), // Tags Action\Tag\ListTagsAction::getRouteDef(), diff --git a/module/Rest/src/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddleware.php b/module/Rest/src/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddleware.php new file mode 100644 index 00000000..b894e40c --- /dev/null +++ b/module/Rest/src/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddleware.php @@ -0,0 +1,31 @@ +defaultDomain = $defaultDomain; + } + + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + $query = $request->getQueryParams(); + if (isset($query['domain']) && $query['domain'] === $this->defaultDomain) { + unset($query['domain']); + $request = $request->withQueryParams($query); + } + + return $handler->handle($request); + } +} From 0c1ecd3caa9f0e83a7c7be96e9100af9b2f43dc4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 19:13:32 +0100 Subject: [PATCH 20/23] Created DropDefaultDomainFromQueryMiddlewareTest --- ...opDefaultDomainFromQueryMiddlewareTest.php | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 module/Rest/test/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddlewareTest.php diff --git a/module/Rest/test/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddlewareTest.php b/module/Rest/test/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddlewareTest.php new file mode 100644 index 00000000..8f588304 --- /dev/null +++ b/module/Rest/test/Middleware/ShortUrl/DropDefaultDomainFromQueryMiddlewareTest.php @@ -0,0 +1,54 @@ +next = $this->prophesize(RequestHandlerInterface::class); + $this->middleware = new DropDefaultDomainFromQueryMiddleware('doma.in'); + } + + /** + * @test + * @dataProvider provideQueryParams + */ + public function domainIsDroppedWhenDefaultOneIsProvided(array $providedQuery, array $expectedQuery): void + { + $req = ServerRequestFactory::fromGlobals()->withQueryParams($providedQuery); + + $handle = $this->next->handle(Argument::that(function (ServerRequestInterface $request) use ($expectedQuery) { + Assert::assertEquals($expectedQuery, $request->getQueryParams()); + return $request; + }))->willReturn(new Response()); + + $this->middleware->process($req, $this->next->reveal()); + + $handle->shouldHaveBeenCalledOnce(); + } + + public function provideQueryParams(): iterable + { + yield [[], []]; + yield [['foo' => 'bar'], ['foo' => 'bar']]; + yield [['foo' => 'bar', 'domain' => 'doma.in'], ['foo' => 'bar']]; + yield [['foo' => 'bar', 'domain' => 'not_default'], ['foo' => 'bar', 'domain' => 'not_default']]; + yield [['domain' => 'doma.in'], []]; + } +} From 8a0ba11f79ca493f0fea9ec6606b7c3a4d019c25 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 19:15:14 +0100 Subject: [PATCH 21/23] Added one more test case for not found URLs on API tests --- module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php b/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php index fab658f0..3cf2ad30 100644 --- a/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php +++ b/module/Rest/test-api/Utils/NotFoundUrlHelpersTrait.php @@ -14,11 +14,16 @@ trait NotFoundUrlHelpersTrait public function provideInvalidUrls(): iterable { yield 'invalid shortcode' => ['invalid', null, 'No URL found with short code "invalid"']; - yield 'invalid shortcode + domain' => [ + yield 'invalid shortcode without domain' => [ 'abc123', 'example.com', 'No URL found with short code "abc123" for domain "example.com"', ]; + yield 'invalid shortcode + domain' => [ + 'custom-with-domain', + 'example.com', + 'No URL found with short code "custom-with-domain" for domain "example.com"', + ]; } public function buildShortUrlPath(string $shortCode, ?string $domain, string $suffix = ''): string From 907b8453c6d8ac956c31354faee2176d83e6b184 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 19:16:53 +0100 Subject: [PATCH 22/23] Updated changelog --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d13de92..7bdbbabf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this #### Fixed * [#620](https://github.com/shlinkio/shlink/issues/620) Ensured "controlled" errors (like validation errors and such) won't be logged with error level, preventing logs to be polluted. +* [#637](https://github.com/shlinkio/shlink/issues/637) Fixed several work flows in which short URLs with domain are handled form the API. +* [#644](https://github.com/shlinkio/shlink/issues/644) Fixed visits to short URL on non-default domain being linked to the URL on default domain with the same short code. ## 2.0.3 - 2020-01-27 From ce990c67e36d856e09de814cb350327d1c500b16 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 19:19:35 +0100 Subject: [PATCH 23/23] Fixed coding styles --- module/Rest/test-api/Action/EditShortUrlActionTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Rest/test-api/Action/EditShortUrlActionTest.php b/module/Rest/test-api/Action/EditShortUrlActionTest.php index d7d425dc..171a40cc 100644 --- a/module/Rest/test-api/Action/EditShortUrlActionTest.php +++ b/module/Rest/test-api/Action/EditShortUrlActionTest.php @@ -9,8 +9,8 @@ use DMS\PHPUnitExtensions\ArraySubset\ArraySubsetAsserts; use GuzzleHttp\RequestOptions; use Laminas\Diactoros\Uri; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; - use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait; + use function GuzzleHttp\Psr7\build_query; use function sprintf;