From 240d2588f9aee75571f053588b8161a627d82d3f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 09:41:48 +0100 Subject: [PATCH 01/35] Extracted some private functions ase helper global functions --- module/Core/functions/functions.php | 23 +++++++++++++++ module/Core/src/Model/ShortUrlMeta.php | 28 ++++--------------- module/Core/src/Model/VisitsParams.php | 13 ++------- .../Validation/ShortUrlMetaInputFilter.php | 6 ++-- .../Action/ShortUrl/ListShortUrlsAction.php | 8 ------ 5 files changed, 33 insertions(+), 45 deletions(-) diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 92d40fe1..1384549f 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -4,6 +4,8 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core; +use Cake\Chronos\Chronos; +use DateTimeInterface; use PUGX\Shortid\Factory as ShortIdFactory; function generateRandomShortCode(int $length = 5): string @@ -16,3 +18,24 @@ function generateRandomShortCode(int $length = 5): string $alphabet = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'; return $shortIdFactory->generate($length, $alphabet)->serialize(); } + +function parseDateFromQuery(array $query, string $dateName): ?Chronos +{ + return ! isset($query[$dateName]) || empty($query[$dateName]) ? null : Chronos::parse($query[$dateName]); +} + +/** + * @param string|DateTimeInterface|Chronos|null $date + */ +function parseDateField($date): ?Chronos +{ + if ($date === null || $date instanceof Chronos) { + return $date; + } + + if ($date instanceof DateTimeInterface) { + return Chronos::instance($date); + } + + return Chronos::parse($date); +} diff --git a/module/Core/src/Model/ShortUrlMeta.php b/module/Core/src/Model/ShortUrlMeta.php index f0c487b7..27c8e624 100644 --- a/module/Core/src/Model/ShortUrlMeta.php +++ b/module/Core/src/Model/ShortUrlMeta.php @@ -5,11 +5,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; use Cake\Chronos\Chronos; -use DateTimeInterface; use Shlinkio\Shlink\Core\Exception\ValidationException; use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter; use function array_key_exists; +use function Shlinkio\Shlink\Core\parseDateField; final class ShortUrlMeta { @@ -34,30 +34,28 @@ final class ShortUrlMeta } /** - * @param array $data * @throws ValidationException */ public static function fromRawData(array $data): self { $instance = new self(); - $instance->validate($data); + $instance->validateAndInit($data); return $instance; } /** - * @param array $data * @throws ValidationException */ - private function validate(array $data): void + private function validateAndInit(array $data): void { $inputFilter = new ShortUrlMetaInputFilter($data); if (! $inputFilter->isValid()) { throw ValidationException::fromInputFilter($inputFilter); } - $this->validSince = $this->parseDateField($inputFilter->getValue(ShortUrlMetaInputFilter::VALID_SINCE)); + $this->validSince = parseDateField($inputFilter->getValue(ShortUrlMetaInputFilter::VALID_SINCE)); $this->validSincePropWasProvided = array_key_exists(ShortUrlMetaInputFilter::VALID_SINCE, $data); - $this->validUntil = $this->parseDateField($inputFilter->getValue(ShortUrlMetaInputFilter::VALID_UNTIL)); + $this->validUntil = parseDateField($inputFilter->getValue(ShortUrlMetaInputFilter::VALID_UNTIL)); $this->validUntilPropWasProvided = array_key_exists(ShortUrlMetaInputFilter::VALID_UNTIL, $data); $this->customSlug = $inputFilter->getValue(ShortUrlMetaInputFilter::CUSTOM_SLUG); $maxVisits = $inputFilter->getValue(ShortUrlMetaInputFilter::MAX_VISITS); @@ -67,22 +65,6 @@ final class ShortUrlMeta $this->domain = $inputFilter->getValue(ShortUrlMetaInputFilter::DOMAIN); } - /** - * @param string|DateTimeInterface|Chronos|null $date - */ - private function parseDateField($date): ?Chronos - { - if ($date === null || $date instanceof Chronos) { - return $date; - } - - if ($date instanceof DateTimeInterface) { - return Chronos::instance($date); - } - - return Chronos::parse($date); - } - public function getValidSince(): ?Chronos { return $this->validSince; diff --git a/module/Core/src/Model/VisitsParams.php b/module/Core/src/Model/VisitsParams.php index 98fcbe82..041aed9f 100644 --- a/module/Core/src/Model/VisitsParams.php +++ b/module/Core/src/Model/VisitsParams.php @@ -4,9 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Model; -use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Util\DateRange; +use function Shlinkio\Shlink\Core\parseDateFromQuery; + final class VisitsParams { private const FIRST_PAGE = 1; @@ -34,21 +35,13 @@ final class VisitsParams public static function fromRawData(array $query): self { - $startDate = self::getDateQueryParam($query, 'startDate'); - $endDate = self::getDateQueryParam($query, 'endDate'); - return new self( - new DateRange($startDate, $endDate), + new DateRange(parseDateFromQuery($query, 'startDate'), parseDateFromQuery($query, 'endDate')), (int) ($query['page'] ?? 1), isset($query['itemsPerPage']) ? (int) $query['itemsPerPage'] : null, ); } - private static function getDateQueryParam(array $query, string $key): ?Chronos - { - return ! isset($query[$key]) || empty($query[$key]) ? null : Chronos::parse($query[$key]); - } - public function getDateRange(): DateRange { return $this->dateRange; diff --git a/module/Core/src/Validation/ShortUrlMetaInputFilter.php b/module/Core/src/Validation/ShortUrlMetaInputFilter.php index eca97a13..187ec66f 100644 --- a/module/Core/src/Validation/ShortUrlMetaInputFilter.php +++ b/module/Core/src/Validation/ShortUrlMetaInputFilter.php @@ -20,12 +20,10 @@ class ShortUrlMetaInputFilter extends InputFilter public const FIND_IF_EXISTS = 'findIfExists'; public const DOMAIN = 'domain'; - public function __construct(?array $data = null) + public function __construct(array $data) { $this->initialize(); - if ($data !== null) { - $this->setData($data); - } + $this->setData($data); } private function initialize(): void diff --git a/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php b/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php index a50493db..11a09a76 100644 --- a/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php +++ b/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Action\ShortUrl; use Cake\Chronos\Chronos; -use InvalidArgumentException; use Laminas\Diactoros\Response\JsonResponse; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; @@ -36,9 +35,6 @@ class ListShortUrlsAction extends AbstractRestAction $this->domainConfig = $domainConfig; } - /** - * @throws InvalidArgumentException - */ public function handle(Request $request): Response { $params = $this->queryToListParams($request->getQueryParams()); @@ -48,10 +44,6 @@ class ListShortUrlsAction extends AbstractRestAction ))]); } - /** - * @param array $query - * @return array - */ private function queryToListParams(array $query): array { return [ From 452bfea088a8275fcf2a7efbc3b4f1ac4d4b3039 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 10:49:55 +0100 Subject: [PATCH 02/35] Created DTOs with implicit validation to wrap short URLs lists params --- .../Command/ShortUrl/ListShortUrlsCommand.php | 37 +++----- .../ShortUrl/ListShortUrlsCommandTest.php | 40 +++++---- module/Core/src/Model/ShortUrlsOrdering.php | 63 ++++++++++++++ module/Core/src/Model/ShortUrlsParams.php | 85 +++++++++++++++++++ .../Adapter/ShortUrlRepositoryAdapter.php | 42 +++------ .../src/Repository/ShortUrlRepository.php | 16 ++-- .../ShortUrlRepositoryInterface.php | 6 +- module/Core/src/Service/ShortUrlService.php | 18 ++-- .../src/Service/ShortUrlServiceInterface.php | 13 +-- .../Validation/ShortUrlsParamsInputFilter.php | 55 ++++++++++++ .../Repository/ShortUrlRepositoryTest.php | 9 +- .../Adapter/ShortUrlRepositoryAdapterTest.php | 45 +++++++--- .../Core/test/Service/ShortUrlServiceTest.php | 3 +- .../Action/ShortUrl/ListShortUrlsAction.php | 25 +----- .../ShortUrl/ListShortUrlsActionTest.php | 44 +++++----- 15 files changed, 335 insertions(+), 166 deletions(-) create mode 100644 module/Core/src/Model/ShortUrlsOrdering.php create mode 100644 module/Core/src/Model/ShortUrlsParams.php create mode 100644 module/Core/src/Validation/ShortUrlsParamsInputFilter.php diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index c4251ed9..98f006d4 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -4,16 +4,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; -use Cake\Chronos\Chronos; use Laminas\Paginator\Paginator; use Shlinkio\Shlink\CLI\Command\Util\AbstractWithDateRangeCommand; use Shlinkio\Shlink\CLI\Util\ExitCodes; use Shlinkio\Shlink\CLI\Util\ShlinkTable; use Shlinkio\Shlink\Common\Paginator\Util\PaginatorUtilsTrait; -use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Core\Transformer\ShortUrlDataTransformer; +use Shlinkio\Shlink\Core\Validation\ShortUrlsParamsInputFilter; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; @@ -108,7 +109,14 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand $orderBy = $this->processOrderBy($input); do { - $result = $this->renderPage($output, $page, $searchTerm, $tags, $showTags, $startDate, $endDate, $orderBy); + $result = $this->renderPage($output, $showTags, ShortUrlsParams::fromRawData([ + ShortUrlsParamsInputFilter::PAGE => $page, + ShortUrlsParamsInputFilter::SEARCH_TERM => $searchTerm, + ShortUrlsParamsInputFilter::TAGS => $tags, + ShortUrlsOrdering::ORDER_BY => $orderBy, + ShortUrlsParamsInputFilter::START_DATE => $startDate !== null ? $startDate->toAtomString() : null, + ShortUrlsParamsInputFilter::END_DATE => $endDate !== null ? $endDate->toAtomString() : null, + ])); $page++; $continue = $this->isLastPage($result) @@ -122,26 +130,9 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand return ExitCodes::EXIT_SUCCESS; } - /** - * @param string|array|null $orderBy - */ - private function renderPage( - OutputInterface $output, - int $page, - ?string $searchTerm, - array $tags, - bool $showTags, - ?Chronos $startDate, - ?Chronos $endDate, - $orderBy - ): Paginator { - $result = $this->shortUrlService->listShortUrls( - $page, - $searchTerm, - $tags, - $orderBy, - new DateRange($startDate, $endDate), - ); + private function renderPage(OutputInterface $output, bool $showTags, ShortUrlsParams $params): Paginator + { + $result = $this->shortUrlService->listShortUrls($params); $headers = ['Short code', 'Short URL', 'Long URL', 'Date created', 'Visits count']; if ($showTags) { diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index d4182e27..2e315581 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -11,8 +11,8 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\ShortUrl\ListShortUrlsCommand; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; @@ -64,7 +64,7 @@ class ListShortUrlsCommandTest extends TestCase $data[] = new ShortUrl('url_' . $i); } - $this->shortUrlService->listShortUrls(1, null, [], null, new DateRange()) + $this->shortUrlService->listShortUrls(ShortUrlsParams::emptyInstance()) ->willReturn(new Paginator(new ArrayAdapter($data))) ->shouldBeCalledOnce(); @@ -85,7 +85,7 @@ class ListShortUrlsCommandTest extends TestCase public function passingPageWillMakeListStartOnThatPage(): void { $page = 5; - $this->shortUrlService->listShortUrls($page, null, [], null, new DateRange()) + $this->shortUrlService->listShortUrls(ShortUrlsParams::fromRawData(['page' => $page])) ->willReturn(new Paginator(new ArrayAdapter())) ->shouldBeCalledOnce(); @@ -96,7 +96,7 @@ class ListShortUrlsCommandTest extends TestCase /** @test */ public function ifTagsFlagIsProvidedTagsColumnIsIncluded(): void { - $this->shortUrlService->listShortUrls(1, null, [], null, new DateRange()) + $this->shortUrlService->listShortUrls(ShortUrlsParams::emptyInstance()) ->willReturn(new Paginator(new ArrayAdapter())) ->shouldBeCalledOnce(); @@ -115,10 +115,16 @@ class ListShortUrlsCommandTest extends TestCase ?int $page, ?string $searchTerm, array $tags, - ?DateRange $dateRange + ?string $startDate = null, + ?string $endDate = null ): void { - $listShortUrls = $this->shortUrlService->listShortUrls($page, $searchTerm, $tags, null, $dateRange) - ->willReturn(new Paginator(new ArrayAdapter())); + $listShortUrls = $this->shortUrlService->listShortUrls(ShortUrlsParams::fromRawData([ + 'page' => $page, + 'searchTerm' => $searchTerm, + 'tags' => $tags, + 'startDate' => $startDate !== null ? Chronos::parse($startDate)->toAtomString() : null, + 'endDate' => $endDate !== null ? Chronos::parse($endDate)->toAtomString() : null, + ]))->willReturn(new Paginator(new ArrayAdapter())); $this->commandTester->setInputs(['n']); $this->commandTester->execute($commandArgs); @@ -128,36 +134,37 @@ class ListShortUrlsCommandTest extends TestCase public function provideArgs(): iterable { - yield [[], 1, null, [], new DateRange()]; - yield [['--page' => $page = 3], $page, null, [], new DateRange()]; - yield [['--searchTerm' => $searchTerm = 'search this'], 1, $searchTerm, [], new DateRange()]; + yield [[], 1, null, []]; + yield [['--page' => $page = 3], $page, null, []]; + yield [['--searchTerm' => $searchTerm = 'search this'], 1, $searchTerm, []]; yield [ ['--page' => $page = 3, '--searchTerm' => $searchTerm = 'search this', '--tags' => $tags = 'foo,bar'], $page, $searchTerm, explode(',', $tags), - new DateRange(), ]; yield [ ['--startDate' => $startDate = '2019-01-01'], 1, null, [], - new DateRange(Chronos::parse($startDate)), + $startDate, ]; yield [ ['--endDate' => $endDate = '2020-05-23'], 1, null, [], - new DateRange(null, Chronos::parse($endDate)), + null, + $endDate, ]; yield [ ['--startDate' => $startDate = '2019-01-01', '--endDate' => $endDate = '2020-05-23'], 1, null, [], - new DateRange(Chronos::parse($startDate), Chronos::parse($endDate)), + $startDate, + $endDate, ]; } @@ -168,8 +175,9 @@ class ListShortUrlsCommandTest extends TestCase */ public function orderByIsProperlyComputed(array $commandArgs, $expectedOrderBy): void { - $listShortUrls = $this->shortUrlService->listShortUrls(1, null, [], $expectedOrderBy, new DateRange()) - ->willReturn(new Paginator(new ArrayAdapter())); + $listShortUrls = $this->shortUrlService->listShortUrls(ShortUrlsParams::fromRawData([ + 'orderBy' => $expectedOrderBy, + ]))->willReturn(new Paginator(new ArrayAdapter())); $this->commandTester->setInputs(['n']); $this->commandTester->execute($commandArgs); diff --git a/module/Core/src/Model/ShortUrlsOrdering.php b/module/Core/src/Model/ShortUrlsOrdering.php new file mode 100644 index 00000000..a98ad99a --- /dev/null +++ b/module/Core/src/Model/ShortUrlsOrdering.php @@ -0,0 +1,63 @@ +validateAndInit($query); + + return $instance; + } + + /** + * @throws ValidationException + */ + private function validateAndInit(array $data): void + { + /** @var string|array|null $orderBy */ + $orderBy = $data[self::ORDER_BY] ?? null; + if ($orderBy === null) { + return; + } + + $isArray = is_array($orderBy); + if (! $isArray && $orderBy !== null && ! is_string($orderBy)) { + throw ValidationException::fromArray([ + 'orderBy' => '"Order by" must be an array, string or null', + ]); + } + + $this->orderField = $isArray ? key($orderBy) : $orderBy; + $this->orderDirection = $isArray ? $orderBy[$this->orderField] : self::DEFAULT_ORDER_DIRECTION; + } + + public function orderField(): ?string + { + return $this->orderField; + } + + public function orderDirection(): string + { + return $this->orderDirection; + } +} diff --git a/module/Core/src/Model/ShortUrlsParams.php b/module/Core/src/Model/ShortUrlsParams.php new file mode 100644 index 00000000..cbd74bec --- /dev/null +++ b/module/Core/src/Model/ShortUrlsParams.php @@ -0,0 +1,85 @@ +validateAndInit($query); + + return $instance; + } + + /** + * @throws ValidationException + */ + private function validateAndInit(array $query): void + { + $inputFilter = new ShortUrlsParamsInputFilter($query); + if (! $inputFilter->isValid()) { + throw ValidationException::fromInputFilter($inputFilter); + } + + $this->page = (int) ($inputFilter->getValue(ShortUrlsParamsInputFilter::PAGE) ?? 1); + $this->searchTerm = $inputFilter->getValue(ShortUrlsParamsInputFilter::SEARCH_TERM); + $this->tags = (array) $inputFilter->getValue(ShortUrlsParamsInputFilter::TAGS); + $this->dateRange = new DateRange( + parseDateField($inputFilter->getValue(ShortUrlsParamsInputFilter::START_DATE)), + parseDateField($inputFilter->getValue(ShortUrlsParamsInputFilter::END_DATE)), + ); + $this->orderBy = ShortUrlsOrdering::fromRawData($query); + } + + public function page(): int + { + return $this->page; + } + + public function searchTerm(): ?string + { + return $this->searchTerm; + } + + public function tags(): array + { + return $this->tags; + } + + public function orderBy(): ShortUrlsOrdering + { + return $this->orderBy; + } + + public function dateRange(): ?DateRange + { + return $this->dateRange; + } +} diff --git a/module/Core/src/Paginator/Adapter/ShortUrlRepositoryAdapter.php b/module/Core/src/Paginator/Adapter/ShortUrlRepositoryAdapter.php index d15d925c..a4cf3190 100644 --- a/module/Core/src/Paginator/Adapter/ShortUrlRepositoryAdapter.php +++ b/module/Core/src/Paginator/Adapter/ShortUrlRepositoryAdapter.php @@ -5,38 +5,20 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Paginator\Adapter; use Laminas\Paginator\Adapter\AdapterInterface; -use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; -use function strip_tags; -use function trim; - class ShortUrlRepositoryAdapter implements AdapterInterface { public const ITEMS_PER_PAGE = 10; private ShortUrlRepositoryInterface $repository; - private ?string $searchTerm; - /** @var null|array|string */ - private $orderBy; - private array $tags; - private ?DateRange $dateRange; + private ShortUrlsParams $params; - /** - * @param string|array|null $orderBy - */ - public function __construct( - ShortUrlRepositoryInterface $repository, - ?string $searchTerm = null, - array $tags = [], - $orderBy = null, - ?DateRange $dateRange = null - ) { + public function __construct(ShortUrlRepositoryInterface $repository, ShortUrlsParams $params) + { $this->repository = $repository; - $this->searchTerm = $searchTerm !== null ? trim(strip_tags($searchTerm)) : null; - $this->orderBy = $orderBy; - $this->tags = $tags; - $this->dateRange = $dateRange; + $this->params = $params; } /** @@ -50,10 +32,10 @@ class ShortUrlRepositoryAdapter implements AdapterInterface return $this->repository->findList( $itemCountPerPage, $offset, - $this->searchTerm, - $this->tags, - $this->orderBy, - $this->dateRange, + $this->params->searchTerm(), + $this->params->tags(), + $this->params->orderBy(), + $this->params->dateRange(), ); } @@ -68,6 +50,10 @@ class ShortUrlRepositoryAdapter implements AdapterInterface */ public function count(): int { - return $this->repository->countList($this->searchTerm, $this->tags, $this->dateRange); + return $this->repository->countList( + $this->params->searchTerm(), + $this->params->tags(), + $this->params->dateRange(), + ); } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index a334a838..a41f99a8 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -8,18 +8,16 @@ use Doctrine\ORM\EntityRepository; use Doctrine\ORM\QueryBuilder; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use function array_column; use function array_key_exists; use function Functional\contains; -use function is_array; -use function key; class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryInterface { /** * @param string[] $tags - * @param string|array|null $orderBy * @return ShortUrl[] */ public function findList( @@ -27,7 +25,7 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI ?int $offset = null, ?string $searchTerm = null, array $tags = [], - $orderBy = null, + ?ShortUrlsOrdering $orderBy = null, ?DateRange $dateRange = null ): array { $qb = $this->createListQueryBuilder($searchTerm, $tags, $dateRange); @@ -51,14 +49,10 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI return $qb->getQuery()->getResult(); } - /** - * @param string|array|null $orderBy - */ - private function processOrderByForList(QueryBuilder $qb, $orderBy): array + private function processOrderByForList(QueryBuilder $qb, ShortUrlsOrdering $orderBy): array { - $isArray = is_array($orderBy); - $fieldName = $isArray ? key($orderBy) : $orderBy; - $order = $isArray ? $orderBy[$fieldName] : 'ASC'; + $fieldName = $orderBy->orderField(); + $order = $orderBy->orderDirection(); if (contains(['visits', 'visitsCount', 'visitCount'], $fieldName)) { $qb->addSelect('COUNT(DISTINCT v) AS totalVisits') diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index 8695021a..d0acdf1a 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -7,18 +7,16 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\Persistence\ObjectRepository; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; interface ShortUrlRepositoryInterface extends ObjectRepository { - /** - * @param string|array|null $orderBy - */ public function findList( ?int $limit = null, ?int $offset = null, ?string $searchTerm = null, array $tags = [], - $orderBy = null, + ?ShortUrlsOrdering $orderBy = null, ?DateRange $dateRange = null ): array; diff --git a/module/Core/src/Service/ShortUrlService.php b/module/Core/src/Service/ShortUrlService.php index 8733cddb..dbfb1db6 100644 --- a/module/Core/src/Service/ShortUrlService.php +++ b/module/Core/src/Service/ShortUrlService.php @@ -6,10 +6,10 @@ namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM; use Laminas\Paginator\Paginator; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; 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; @@ -28,23 +28,15 @@ class ShortUrlService implements ShortUrlServiceInterface } /** - * @param string[] $tags - * @param array|string|null $orderBy - * * @return ShortUrl[]|Paginator */ - public function listShortUrls( - int $page = 1, - ?string $searchQuery = null, - array $tags = [], - $orderBy = null, - ?DateRange $dateRange = null - ) { + public function listShortUrls(ShortUrlsParams $params): Paginator + { /** @var ShortUrlRepository $repo */ $repo = $this->em->getRepository(ShortUrl::class); - $paginator = new Paginator(new ShortUrlRepositoryAdapter($repo, $searchQuery, $tags, $orderBy, $dateRange)); + $paginator = new Paginator(new ShortUrlRepositoryAdapter($repo, $params)); $paginator->setItemCountPerPage(ShortUrlRepositoryAdapter::ITEMS_PER_PAGE) - ->setCurrentPageNumber($page); + ->setCurrentPageNumber($params->page()); return $paginator; } diff --git a/module/Core/src/Service/ShortUrlServiceInterface.php b/module/Core/src/Service/ShortUrlServiceInterface.php index 310ba6b3..e431c51b 100644 --- a/module/Core/src/Service/ShortUrlServiceInterface.php +++ b/module/Core/src/Service/ShortUrlServiceInterface.php @@ -5,26 +5,17 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; use Laminas\Paginator\Paginator; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; interface ShortUrlServiceInterface { /** - * @param string[] $tags - * @param array|string|null $orderBy - * * @return ShortUrl[]|Paginator */ - public function listShortUrls( - int $page = 1, - ?string $searchQuery = null, - array $tags = [], - $orderBy = null, - ?DateRange $dateRange = null - ); + public function listShortUrls(ShortUrlsParams $params): Paginator; /** * @param string[] $tags diff --git a/module/Core/src/Validation/ShortUrlsParamsInputFilter.php b/module/Core/src/Validation/ShortUrlsParamsInputFilter.php new file mode 100644 index 00000000..376eb531 --- /dev/null +++ b/module/Core/src/Validation/ShortUrlsParamsInputFilter.php @@ -0,0 +1,55 @@ +initialize(); + $this->setData($data); + } + + private function initialize(): void + { + $startDate = $this->createInput(self::START_DATE, false); + $startDate->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); + $this->add($startDate); + + $endDate = $this->createInput(self::END_DATE, false); + $endDate->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); + $this->add($endDate); + + $this->add($this->createInput(self::SEARCH_TERM, false)); + + $page = $this->createInput(self::PAGE, false); + $page->getValidatorChain()->attach(new Validator\Digits()) + ->attach(new Validator\GreaterThan(['min' => 1, 'inclusive' => true])); + $this->add($page); + + $tags = new ArrayInput(self::TAGS); + $tags->setRequired(false) + ->getFilterChain()->attach(new Filter\StripTags()) + ->attach(new Filter\StringTrim()) + ->attach(new Filter\StringToLower()) + ->attach(new Validation\SluggerFilter()); + $this->add($tags); + } +} diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index f742c6eb..4af2fd61 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; +use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; @@ -122,7 +123,9 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->assertCount(1, $this->repo->findList(2, 2)); - $result = $this->repo->findList(null, null, null, [], ['visits' => 'DESC']); + $result = $this->repo->findList(null, null, null, [], ShortUrlsOrdering::fromRawData([ + 'orderBy' => ['visits' => 'DESC'], + ])); $this->assertCount(3, $result); $this->assertSame($bar, $result[0]); @@ -148,7 +151,9 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $result = $this->repo->findList(null, null, null, [], ['longUrl' => 'ASC']); + $result = $this->repo->findList(null, null, null, [], ShortUrlsOrdering::fromRawData([ + 'orderBy' => ['longUrl' => 'ASC'], + ])); $this->assertCount(count($urls), $result); $this->assertEquals('a', $result[0]->getLongUrl()); diff --git a/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php b/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php index dd2fa24a..eb094c25 100644 --- a/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php +++ b/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php @@ -7,7 +7,7 @@ namespace ShlinkioTest\Shlink\Core\Paginator\Adapter; use Cake\Chronos\Chronos; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; -use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; @@ -21,17 +21,26 @@ class ShortUrlRepositoryAdapterTest extends TestCase } /** - * @param string|array|null $orderBy * @test * @dataProvider provideFilteringArgs */ public function getItemsFallsBackToFindList( ?string $searchTerm = null, array $tags = [], - ?DateRange $dateRange = null, - $orderBy = null + ?string $startDate = null, + ?string $endDate = null, + ?string $orderBy = null ): void { - $adapter = new ShortUrlRepositoryAdapter($this->repo->reveal(), $searchTerm, $tags, $orderBy, $dateRange); + $params = ShortUrlsParams::fromRawData([ + 'searchTerm' => $searchTerm, + 'tags' => $tags, + 'startDate' => $startDate, + 'endDate' => $endDate, + 'orderBy' => $orderBy, + ]); + $adapter = new ShortUrlRepositoryAdapter($this->repo->reveal(), $params); + $orderBy = $params->orderBy(); + $dateRange = $params->dateRange(); $this->repo->findList(10, 5, $searchTerm, $tags, $orderBy, $dateRange)->shouldBeCalledOnce(); $adapter->getItems(5, 10); @@ -44,9 +53,17 @@ class ShortUrlRepositoryAdapterTest extends TestCase public function countFallsBackToCountList( ?string $searchTerm = null, array $tags = [], - ?DateRange $dateRange = null + ?string $startDate = null, + ?string $endDate = null ): void { - $adapter = new ShortUrlRepositoryAdapter($this->repo->reveal(), $searchTerm, $tags, null, $dateRange); + $params = ShortUrlsParams::fromRawData([ + 'searchTerm' => $searchTerm, + 'tags' => $tags, + 'startDate' => $startDate, + 'endDate' => $endDate, + ]); + $adapter = new ShortUrlRepositoryAdapter($this->repo->reveal(), $params); + $dateRange = $params->dateRange(); $this->repo->countList($searchTerm, $tags, $dateRange)->shouldBeCalledOnce(); $adapter->count(); @@ -58,12 +75,12 @@ class ShortUrlRepositoryAdapterTest extends TestCase yield ['search']; yield ['search', []]; yield ['search', ['foo', 'bar']]; - yield ['search', ['foo', 'bar'], null, 'order']; - yield ['search', ['foo', 'bar'], new DateRange(), 'order']; - yield ['search', ['foo', 'bar'], new DateRange(Chronos::now()), 'order']; - yield ['search', ['foo', 'bar'], new DateRange(null, Chronos::now()), 'order']; - yield ['search', ['foo', 'bar'], new DateRange(Chronos::now(), Chronos::now()), 'order']; - yield ['search', ['foo', 'bar'], new DateRange(Chronos::now())]; - yield [null, ['foo', 'bar'], new DateRange(Chronos::now(), Chronos::now())]; + yield ['search', ['foo', 'bar'], null, null, 'order']; + yield ['search', ['foo', 'bar'], Chronos::now()->toAtomString(), null, 'order']; + yield ['search', ['foo', 'bar'], null, Chronos::now()->toAtomString(), 'order']; + yield ['search', ['foo', 'bar'], Chronos::now()->toAtomString(), Chronos::now()->toAtomString(), 'order']; + yield [null, ['foo', 'bar'], Chronos::now()->toAtomString(), null, 'order']; + yield [null, ['foo', 'bar'], Chronos::now()->toAtomString()]; + yield [null, ['foo', 'bar'], Chronos::now()->toAtomString(), Chronos::now()->toAtomString()]; } } diff --git a/module/Core/test/Service/ShortUrlServiceTest.php b/module/Core/test/Service/ShortUrlServiceTest.php index 719c69d5..69f0785d 100644 --- a/module/Core/test/Service/ShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrlServiceTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\ShortUrlService; @@ -47,7 +48,7 @@ class ShortUrlServiceTest extends TestCase $repo->countList(Argument::cetera())->willReturn(count($list))->shouldBeCalledOnce(); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $list = $this->service->listShortUrls(); + $list = $this->service->listShortUrls(ShortUrlsParams::emptyInstance()); $this->assertEquals(4, $list->getCurrentItemCount()); } diff --git a/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php b/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php index 11a09a76..5801eeec 100644 --- a/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php +++ b/module/Rest/src/Action/ShortUrl/ListShortUrlsAction.php @@ -4,13 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Action\ShortUrl; -use Cake\Chronos\Chronos; 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\Common\Paginator\Util\PaginatorUtilsTrait; -use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Core\Transformer\ShortUrlDataTransformer; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; @@ -37,29 +36,9 @@ class ListShortUrlsAction extends AbstractRestAction public function handle(Request $request): Response { - $params = $this->queryToListParams($request->getQueryParams()); - $shortUrls = $this->shortUrlService->listShortUrls(...$params); + $shortUrls = $this->shortUrlService->listShortUrls(ShortUrlsParams::fromRawData($request->getQueryParams())); return new JsonResponse(['shortUrls' => $this->serializePaginator($shortUrls, new ShortUrlDataTransformer( $this->domainConfig, ))]); } - - private function queryToListParams(array $query): array - { - return [ - (int) ($query['page'] ?? 1), - $query['searchTerm'] ?? null, - $query['tags'] ?? [], - $query['orderBy'] ?? null, - $this->determineDateRangeFromQuery($query), - ]; - } - - private function determineDateRangeFromQuery(array $query): DateRange - { - return new DateRange( - isset($query['startDate']) ? Chronos::parse($query['startDate']) : null, - isset($query['endDate']) ? Chronos::parse($query['endDate']) : null, - ); - } } diff --git a/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php b/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php index 52cc62de..3d98c2fe 100644 --- a/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php +++ b/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php @@ -12,7 +12,7 @@ use Laminas\Paginator\Paginator; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Service\ShortUrlService; use Shlinkio\Shlink\Rest\Action\ShortUrl\ListShortUrlsAction; @@ -43,15 +43,17 @@ class ListShortUrlsActionTest extends TestCase ?string $expectedSearchTerm, array $expectedTags, ?string $expectedOrderBy, - DateRange $expectedDateRange + ?string $startDate = null, + ?string $endDate = null ): void { - $listShortUrls = $this->service->listShortUrls( - $expectedPage, - $expectedSearchTerm, - $expectedTags, - $expectedOrderBy, - $expectedDateRange, - )->willReturn(new Paginator(new ArrayAdapter())); + $listShortUrls = $this->service->listShortUrls(ShortUrlsParams::fromRawData([ + 'page' => $expectedPage, + 'searchTerm' => $expectedSearchTerm, + 'tags' => $expectedTags, + 'orderBy' => $expectedOrderBy, + 'startDate' => $startDate, + 'endDate' => $endDate, + ]))->willReturn(new Paginator(new ArrayAdapter())); /** @var JsonResponse $response */ $response = $this->action->handle((new ServerRequest())->withQueryParams($query)); @@ -66,25 +68,25 @@ class ListShortUrlsActionTest extends TestCase public function provideFilteringData(): iterable { - yield [[], 1, null, [], null, new DateRange()]; - yield [['page' => 10], 10, null, [], null, new DateRange()]; - yield [['page' => null], 1, null, [], null, new DateRange()]; - yield [['page' => '8'], 8, null, [], null, new DateRange()]; - yield [['searchTerm' => $searchTerm = 'foo'], 1, $searchTerm, [], null, new DateRange()]; - yield [['tags' => $tags = ['foo','bar']], 1, null, $tags, null, new DateRange()]; - yield [['orderBy' => $orderBy = 'something'], 1, null, [], $orderBy, new DateRange()]; + yield [[], 1, null, [], null]; + yield [['page' => 10], 10, null, [], null]; + yield [['page' => null], 1, null, [], null]; + yield [['page' => '8'], 8, null, [], null]; + yield [['searchTerm' => $searchTerm = 'foo'], 1, $searchTerm, [], null]; + yield [['tags' => $tags = ['foo','bar']], 1, null, $tags, null]; + yield [['orderBy' => $orderBy = 'something'], 1, null, [], $orderBy]; yield [[ 'page' => '2', 'orderBy' => $orderBy = 'something', 'tags' => $tags = ['one', 'two'], - ], 2, null, $tags, $orderBy, new DateRange()]; + ], 2, null, $tags, $orderBy]; yield [ ['startDate' => $date = Chronos::now()->toAtomString()], 1, null, [], null, - new DateRange(Chronos::parse($date)), + $date, ]; yield [ ['endDate' => $date = Chronos::now()->toAtomString()], @@ -92,7 +94,8 @@ class ListShortUrlsActionTest extends TestCase null, [], null, - new DateRange(null, Chronos::parse($date)), + null, + $date, ]; yield [ [ @@ -103,7 +106,8 @@ class ListShortUrlsActionTest extends TestCase null, [], null, - new DateRange(Chronos::parse($startDate), Chronos::parse($endDate)), + $startDate, + $endDate, ]; } } From fccd92497a91979f74d2094dbb2ce9a8a6d4aa9e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 11:17:54 +0100 Subject: [PATCH 03/35] Added last check on ShortUrlsOrdering which makes sure everything keeps behaving as it used to --- module/Core/src/Model/ShortUrlsOrdering.php | 5 +++++ module/Core/src/Repository/ShortUrlRepository.php | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/module/Core/src/Model/ShortUrlsOrdering.php b/module/Core/src/Model/ShortUrlsOrdering.php index a98ad99a..00c30a54 100644 --- a/module/Core/src/Model/ShortUrlsOrdering.php +++ b/module/Core/src/Model/ShortUrlsOrdering.php @@ -60,4 +60,9 @@ final class ShortUrlsOrdering { return $this->orderDirection; } + + public function hasOrderField(): bool + { + return $this->orderField !== null; + } } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index a41f99a8..cd96acfc 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -40,7 +40,7 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI } // In case the ordering has been specified, the query could be more complex. Process it - if ($orderBy !== null) { + if ($orderBy !== null && $orderBy->hasOrderField()) { return $this->processOrderByForList($qb, $orderBy); } From 6ff5a532ea162f6add728b764a5d3ef3824f0844 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 11:20:48 +0100 Subject: [PATCH 04/35] Added extra API test covering complex order by for short URL lists --- module/Rest/test-api/Action/ListShortUrlsTest.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index f8d8c542..3fec6f7a 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -116,6 +116,13 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_META, self::SHORT_URL_CUSTOM_DOMAIN, ]]; + yield [['orderBy' => ['shortCode' => 'DESC']], [ + self::SHORT_URL_CUSTOM_DOMAIN, + self::SHORT_URL_META, + self::SHORT_URL_CUSTOM_SLUG_AND_DOMAIN, + self::SHORT_URL_CUSTOM_SLUG, + self::SHORT_URL_SHLINK, + ]]; yield [['startDate' => Chronos::parse('2018-12-01')->toAtomString()], [ self::SHORT_URL_META, self::SHORT_URL_CUSTOM_SLUG, From 51ebe57ac8ce8a070ab9659584c422e72e31f05b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 12:12:50 +0100 Subject: [PATCH 05/35] Updated changelog --- CHANGELOG.md | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74ee51ab..93cb803a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,29 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] + +#### Added + +* *Nothing* + +#### Changed + +* [#577](https://github.com/shlinkio/shlink/issues/577) Wrapped params used to customize short URL lists into a DTO with implicit validation. + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* *Nothing* + + ## 2.0.3 - 2020-01-27 #### Added From 7add41d560a15d9f04eceeefae8c99058a274026 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 12:57:21 +0100 Subject: [PATCH 06/35] Ensured BC on dates for short urls params --- composer.json | 2 +- .../Validation/ShortUrlsParamsInputFilter.php | 18 ++++-------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/composer.json b/composer.json index 19b3f557..0f259095 100644 --- a/composer.json +++ b/composer.json @@ -47,7 +47,7 @@ "phly/phly-event-dispatcher": "^1.0", "predis/predis": "^1.1", "pugx/shortid-php": "^0.5", - "shlinkio/shlink-common": "^2.5", + "shlinkio/shlink-common": "dev-master#e6658205370260df72f295df15364ac569ff702c as 2.6.0", "shlinkio/shlink-event-dispatcher": "^1.3", "shlinkio/shlink-installer": "^4.0.1", "shlinkio/shlink-ip-geolocation": "^1.3.1", diff --git a/module/Core/src/Validation/ShortUrlsParamsInputFilter.php b/module/Core/src/Validation/ShortUrlsParamsInputFilter.php index 376eb531..b3e6db2d 100644 --- a/module/Core/src/Validation/ShortUrlsParamsInputFilter.php +++ b/module/Core/src/Validation/ShortUrlsParamsInputFilter.php @@ -4,9 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Validation; -use DateTime; use Laminas\Filter; -use Laminas\InputFilter\ArrayInput; use Laminas\InputFilter\InputFilter; use Laminas\Validator; use Shlinkio\Shlink\Common\Validation; @@ -29,13 +27,8 @@ class ShortUrlsParamsInputFilter extends InputFilter private function initialize(): void { - $startDate = $this->createInput(self::START_DATE, false); - $startDate->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); - $this->add($startDate); - - $endDate = $this->createInput(self::END_DATE, false); - $endDate->getValidatorChain()->attach(new Validator\Date(['format' => DateTime::ATOM])); - $this->add($endDate); + $this->add($this->createDateInput(self::START_DATE, false)); + $this->add($this->createDateInput(self::END_DATE, false)); $this->add($this->createInput(self::SEARCH_TERM, false)); @@ -44,11 +37,8 @@ class ShortUrlsParamsInputFilter extends InputFilter ->attach(new Validator\GreaterThan(['min' => 1, 'inclusive' => true])); $this->add($page); - $tags = new ArrayInput(self::TAGS); - $tags->setRequired(false) - ->getFilterChain()->attach(new Filter\StripTags()) - ->attach(new Filter\StringTrim()) - ->attach(new Filter\StringToLower()) + $tags = $this->createArrayInput(self::TAGS, false); + $tags->getFilterChain()->attach(new Filter\StringToLower()) ->attach(new Validation\SluggerFilter()); $this->add($tags); } From 9b2ccaeb7be21890536241cb1fc9effe69a2ae4f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 28 Jan 2020 18:11:39 +0100 Subject: [PATCH 07/35] Updated to shlink-common 2.6 --- CHANGELOG.md | 1 + composer.json | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93cb803a..9fe5ac60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ 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 diff --git a/composer.json b/composer.json index 0f259095..b4189a29 100644 --- a/composer.json +++ b/composer.json @@ -47,7 +47,7 @@ "phly/phly-event-dispatcher": "^1.0", "predis/predis": "^1.1", "pugx/shortid-php": "^0.5", - "shlinkio/shlink-common": "dev-master#e6658205370260df72f295df15364ac569ff702c as 2.6.0", + "shlinkio/shlink-common": "^2.6.0", "shlinkio/shlink-event-dispatcher": "^1.3", "shlinkio/shlink-installer": "^4.0.1", "shlinkio/shlink-ip-geolocation": "^1.3.1", From 96350c8b8f5737aadd1b77b6421d64a603f50734 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 29 Jan 2020 10:06:42 +0100 Subject: [PATCH 08/35] Updated entities mapping config so that they return a function --- composer.json | 2 +- config/autoload/entity-manager.global.php | 1 + .../Shlinkio.Shlink.Core.Entity.Domain.php | 27 +++--- .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 97 ++++++++++--------- .../Shlinkio.Shlink.Core.Entity.Tag.php | 29 +++--- .../Shlinkio.Shlink.Core.Entity.Visit.php | 71 +++++++------- ...inkio.Shlink.Core.Entity.VisitLocation.php | 65 +++++++------ .../Shlinkio.Shlink.Rest.Entity.ApiKey.php | 39 ++++---- 8 files changed, 169 insertions(+), 162 deletions(-) diff --git a/composer.json b/composer.json index b4189a29..908b9af3 100644 --- a/composer.json +++ b/composer.json @@ -47,7 +47,7 @@ "phly/phly-event-dispatcher": "^1.0", "predis/predis": "^1.1", "pugx/shortid-php": "^0.5", - "shlinkio/shlink-common": "^2.6.0", + "shlinkio/shlink-common": "^2.7.0", "shlinkio/shlink-event-dispatcher": "^1.3", "shlinkio/shlink-installer": "^4.0.1", "shlinkio/shlink-ip-geolocation": "^1.3.1", diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index 561579f1..c08f66f2 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -9,6 +9,7 @@ return [ 'entity_manager' => [ 'orm' => [ 'proxies_dir' => 'data/proxies', + 'load_mappings_using_functional_style' => true, ], 'connection' => [ 'user' => '', diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php index de7252ee..1c751d17 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php @@ -6,20 +6,21 @@ namespace Shlinkio\Shlink\Core; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; -use Doctrine\ORM\Mapping\ClassMetadata; // @codingStandardsIgnoreLine +use Doctrine\ORM\Mapping\ClassMetadata; -/** @var $metadata ClassMetadata */ // @codingStandardsIgnoreLine -$builder = new ClassMetadataBuilder($metadata); +return static function (ClassMetadata $metadata): void { + $builder = new ClassMetadataBuilder($metadata); -$builder->setTable('domains'); + $builder->setTable('domains'); -$builder->createField('id', Types::BIGINT) - ->columnName('id') - ->makePrimaryKey() - ->generatedValue('IDENTITY') - ->option('unsigned', true) - ->build(); + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); -$builder->createField('authority', Types::STRING) - ->unique() - ->build(); + $builder->createField('authority', Types::STRING) + ->unique() + ->build(); +}; diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index 0d24d555..a8bcf7ea 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -6,65 +6,66 @@ namespace Shlinkio\Shlink\Core; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; -use Doctrine\ORM\Mapping\ClassMetadata; // @codingStandardsIgnoreLine +use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; -/** @var $metadata ClassMetadata */ // @codingStandardsIgnoreLine -$builder = new ClassMetadataBuilder($metadata); +return static function (ClassMetadata $metadata): void { + $builder = new ClassMetadataBuilder($metadata); -$builder->setTable('short_urls') - ->setCustomRepositoryClass(Repository\ShortUrlRepository::class); + $builder->setTable('short_urls') + ->setCustomRepositoryClass(Repository\ShortUrlRepository::class); -$builder->createField('id', Types::BIGINT) - ->columnName('id') - ->makePrimaryKey() - ->generatedValue('IDENTITY') - ->option('unsigned', true) - ->build(); + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); -$builder->createField('longUrl', Types::STRING) - ->columnName('original_url') - ->length(2048) - ->build(); + $builder->createField('longUrl', Types::STRING) + ->columnName('original_url') + ->length(2048) + ->build(); -$builder->createField('shortCode', Types::STRING) - ->columnName('short_code') - ->length(255) - ->build(); + $builder->createField('shortCode', Types::STRING) + ->columnName('short_code') + ->length(255) + ->build(); -$builder->createField('dateCreated', ChronosDateTimeType::CHRONOS_DATETIME) - ->columnName('date_created') - ->build(); + $builder->createField('dateCreated', ChronosDateTimeType::CHRONOS_DATETIME) + ->columnName('date_created') + ->build(); -$builder->createField('validSince', ChronosDateTimeType::CHRONOS_DATETIME) - ->columnName('valid_since') - ->nullable() - ->build(); + $builder->createField('validSince', ChronosDateTimeType::CHRONOS_DATETIME) + ->columnName('valid_since') + ->nullable() + ->build(); -$builder->createField('validUntil', ChronosDateTimeType::CHRONOS_DATETIME) - ->columnName('valid_until') - ->nullable() - ->build(); + $builder->createField('validUntil', ChronosDateTimeType::CHRONOS_DATETIME) + ->columnName('valid_until') + ->nullable() + ->build(); -$builder->createField('maxVisits', Types::INTEGER) - ->columnName('max_visits') - ->nullable() - ->build(); + $builder->createField('maxVisits', Types::INTEGER) + ->columnName('max_visits') + ->nullable() + ->build(); -$builder->createOneToMany('visits', Entity\Visit::class) - ->mappedBy('shortUrl') - ->fetchExtraLazy() - ->build(); + $builder->createOneToMany('visits', Entity\Visit::class) + ->mappedBy('shortUrl') + ->fetchExtraLazy() + ->build(); -$builder->createManyToMany('tags', Entity\Tag::class) - ->setJoinTable('short_urls_in_tags') - ->addInverseJoinColumn('tag_id', 'id', true, false, 'CASCADE') - ->addJoinColumn('short_url_id', 'id', true, false, 'CASCADE') - ->build(); + $builder->createManyToMany('tags', Entity\Tag::class) + ->setJoinTable('short_urls_in_tags') + ->addInverseJoinColumn('tag_id', 'id', true, false, 'CASCADE') + ->addJoinColumn('short_url_id', 'id', true, false, 'CASCADE') + ->build(); -$builder->createManyToOne('domain', Entity\Domain::class) - ->addJoinColumn('domain_id', 'id', true, false, 'RESTRICT') - ->cascadePersist() - ->build(); + $builder->createManyToOne('domain', Entity\Domain::class) + ->addJoinColumn('domain_id', 'id', true, false, 'RESTRICT') + ->cascadePersist() + ->build(); -$builder->addUniqueConstraint(['short_code', 'domain_id'], 'unique_short_code_plus_domain'); + $builder->addUniqueConstraint(['short_code', 'domain_id'], 'unique_short_code_plus_domain'); +}; diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php index 09a98151..7d310eb8 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php @@ -6,21 +6,22 @@ namespace Shlinkio\Shlink\Core; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; -use Doctrine\ORM\Mapping\ClassMetadata; // @codingStandardsIgnoreLine +use Doctrine\ORM\Mapping\ClassMetadata; -/** @var $metadata ClassMetadata */ // @codingStandardsIgnoreLine -$builder = new ClassMetadataBuilder($metadata); +return static function (ClassMetadata $metadata): void { + $builder = new ClassMetadataBuilder($metadata); -$builder->setTable('tags') - ->setCustomRepositoryClass(Repository\TagRepository::class); + $builder->setTable('tags') + ->setCustomRepositoryClass(Repository\TagRepository::class); -$builder->createField('id', Types::BIGINT) - ->columnName('id') - ->makePrimaryKey() - ->generatedValue('IDENTITY') - ->option('unsigned', true) - ->build(); + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); -$builder->createField('name', Types::STRING) - ->unique() - ->build(); + $builder->createField('name', Types::STRING) + ->unique() + ->build(); +}; diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php index 6770f9d3..5d8f6ff4 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php @@ -6,49 +6,50 @@ namespace Shlinkio\Shlink\Core; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; -use Doctrine\ORM\Mapping\ClassMetadata; // @codingStandardsIgnoreLine +use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Core\Model\Visitor; -/** @var $metadata ClassMetadata */ // @codingStandardsIgnoreLine -$builder = new ClassMetadataBuilder($metadata); +return static function (ClassMetadata $metadata): void { + $builder = new ClassMetadataBuilder($metadata); -$builder->setTable('visits') - ->setCustomRepositoryClass(Repository\VisitRepository::class); + $builder->setTable('visits') + ->setCustomRepositoryClass(Repository\VisitRepository::class); -$builder->createField('id', Types::BIGINT) - ->columnName('id') - ->makePrimaryKey() - ->generatedValue('IDENTITY') - ->option('unsigned', true) - ->build(); + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); -$builder->createField('referer', Types::STRING) - ->nullable() - ->length(Visitor::REFERER_MAX_LENGTH) - ->build(); + $builder->createField('referer', Types::STRING) + ->nullable() + ->length(Visitor::REFERER_MAX_LENGTH) + ->build(); -$builder->createField('date', ChronosDateTimeType::CHRONOS_DATETIME) - ->columnName('`date`') - ->build(); + $builder->createField('date', ChronosDateTimeType::CHRONOS_DATETIME) + ->columnName('`date`') + ->build(); -$builder->createField('remoteAddr', Types::STRING) - ->columnName('remote_addr') - ->length(Visitor::REMOTE_ADDRESS_MAX_LENGTH) - ->nullable() - ->build(); + $builder->createField('remoteAddr', Types::STRING) + ->columnName('remote_addr') + ->length(Visitor::REMOTE_ADDRESS_MAX_LENGTH) + ->nullable() + ->build(); -$builder->createField('userAgent', Types::STRING) - ->columnName('user_agent') - ->length(Visitor::USER_AGENT_MAX_LENGTH) - ->nullable() - ->build(); + $builder->createField('userAgent', Types::STRING) + ->columnName('user_agent') + ->length(Visitor::USER_AGENT_MAX_LENGTH) + ->nullable() + ->build(); -$builder->createManyToOne('shortUrl', Entity\ShortUrl::class) - ->addJoinColumn('short_url_id', 'id', false, false, 'CASCADE') - ->build(); + $builder->createManyToOne('shortUrl', Entity\ShortUrl::class) + ->addJoinColumn('short_url_id', 'id', false, false, 'CASCADE') + ->build(); -$builder->createManyToOne('visitLocation', Entity\VisitLocation::class) - ->addJoinColumn('visit_location_id', 'id', true, false, 'Set NULL') - ->cascadePersist() - ->build(); + $builder->createManyToOne('visitLocation', Entity\VisitLocation::class) + ->addJoinColumn('visit_location_id', 'id', true, false, 'Set NULL') + ->cascadePersist() + ->build(); +}; diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php index 117c2acc..b9652a08 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php @@ -6,41 +6,42 @@ namespace Shlinkio\Shlink\Core; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; -use Doctrine\ORM\Mapping\ClassMetadata; // @codingStandardsIgnoreLine +use Doctrine\ORM\Mapping\ClassMetadata; -/** @var $metadata ClassMetadata */ // @codingStandardsIgnoreLine -$builder = new ClassMetadataBuilder($metadata); +return static function (ClassMetadata $metadata): void { + $builder = new ClassMetadataBuilder($metadata); -$builder->setTable('visit_locations'); + $builder->setTable('visit_locations'); -$builder->createField('id', Types::BIGINT) - ->columnName('id') - ->makePrimaryKey() - ->generatedValue('IDENTITY') - ->option('unsigned', true) - ->build(); - -$columns = [ - 'country_code' => 'countryCode', - 'country_name' => 'countryName', - 'region_name' => 'regionName', - 'city_name' => 'cityName', - 'timezone' => 'timezone', -]; - -foreach ($columns as $columnName => $fieldName) { - $builder->createField($fieldName, Types::STRING) - ->columnName($columnName) - ->nullable() + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) ->build(); -} -$builder->createField('latitude', Types::FLOAT) - ->columnName('lat') - ->nullable(false) - ->build(); + $columns = [ + 'country_code' => 'countryCode', + 'country_name' => 'countryName', + 'region_name' => 'regionName', + 'city_name' => 'cityName', + 'timezone' => 'timezone', + ]; -$builder->createField('longitude', Types::FLOAT) - ->columnName('lon') - ->nullable(false) - ->build(); + foreach ($columns as $columnName => $fieldName) { + $builder->createField($fieldName, Types::STRING) + ->columnName($columnName) + ->nullable() + ->build(); + } + + $builder->createField('latitude', Types::FLOAT) + ->columnName('lat') + ->nullable(false) + ->build(); + + $builder->createField('longitude', Types::FLOAT) + ->columnName('lon') + ->nullable(false) + ->build(); +}; diff --git a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php index 5bc5aec9..a79bd642 100644 --- a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php +++ b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php @@ -6,29 +6,30 @@ namespace Shlinkio\Shlink\Rest; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; -use Doctrine\ORM\Mapping\ClassMetadata; // @codingStandardsIgnoreLine +use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; -/** @var $metadata ClassMetadata */ // @codingStandardsIgnoreLine -$builder = new ClassMetadataBuilder($metadata); +return static function (ClassMetadata $metadata): void { + $builder = new ClassMetadataBuilder($metadata); -$builder->setTable('api_keys'); + $builder->setTable('api_keys'); -$builder->createField('id', Types::BIGINT) - ->makePrimaryKey() - ->generatedValue('IDENTITY') - ->option('unsigned', true) - ->build(); + $builder->createField('id', Types::BIGINT) + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); -$builder->createField('key', Types::STRING) - ->columnName('`key`') - ->unique() - ->build(); + $builder->createField('key', Types::STRING) + ->columnName('`key`') + ->unique() + ->build(); -$builder->createField('expirationDate', ChronosDateTimeType::CHRONOS_DATETIME) - ->columnName('expiration_date') - ->nullable() - ->build(); + $builder->createField('expirationDate', ChronosDateTimeType::CHRONOS_DATETIME) + ->columnName('expiration_date') + ->nullable() + ->build(); -$builder->createField('enabled', Types::BOOLEAN) - ->build(); + $builder->createField('enabled', Types::BOOLEAN) + ->build(); +}; From bd2f488e2c0b632dbf370eb1202701f04f89c95e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 29 Jan 2020 10:53:06 +0100 Subject: [PATCH 09/35] Updated entity mappings so that schema an table prefixes can be eventually provided --- .../Shlinkio.Shlink.Core.Entity.Domain.php | 4 ++-- .../Shlinkio.Shlink.Core.Entity.ShortUrl.php | 6 +++--- .../Shlinkio.Shlink.Core.Entity.Tag.php | 4 ++-- .../Shlinkio.Shlink.Core.Entity.Visit.php | 4 ++-- .../Shlinkio.Shlink.Core.Entity.VisitLocation.php | 4 ++-- module/Core/functions/functions.php | 14 ++++++++++++++ .../Shlinkio.Shlink.Rest.Entity.ApiKey.php | 8 +++++--- phpstan.neon | 1 - 8 files changed, 30 insertions(+), 15 deletions(-) diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php index 1c751d17..c6349b74 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Domain.php @@ -8,10 +8,10 @@ use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; -return static function (ClassMetadata $metadata): void { +return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable('domains'); + $builder->setTable(determineTableName('domains', $emConfig)); $builder->createField('id', Types::BIGINT) ->columnName('id') diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php index a8bcf7ea..a4aef29f 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.ShortUrl.php @@ -9,10 +9,10 @@ use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; -return static function (ClassMetadata $metadata): void { +return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable('short_urls') + $builder->setTable(determineTableName('short_urls', $emConfig)) ->setCustomRepositoryClass(Repository\ShortUrlRepository::class); $builder->createField('id', Types::BIGINT) @@ -57,7 +57,7 @@ return static function (ClassMetadata $metadata): void { ->build(); $builder->createManyToMany('tags', Entity\Tag::class) - ->setJoinTable('short_urls_in_tags') + ->setJoinTable(determineTableName('short_urls_in_tags', $emConfig)) ->addInverseJoinColumn('tag_id', 'id', true, false, 'CASCADE') ->addJoinColumn('short_url_id', 'id', true, false, 'CASCADE') ->build(); diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php index 7d310eb8..214396bd 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Tag.php @@ -8,10 +8,10 @@ use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; -return static function (ClassMetadata $metadata): void { +return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable('tags') + $builder->setTable(determineTableName('tags', $emConfig)) ->setCustomRepositoryClass(Repository\TagRepository::class); $builder->createField('id', Types::BIGINT) diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php index 5d8f6ff4..803b9790 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php @@ -10,10 +10,10 @@ use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Core\Model\Visitor; -return static function (ClassMetadata $metadata): void { +return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable('visits') + $builder->setTable(determineTableName('visits', $emConfig)) ->setCustomRepositoryClass(Repository\VisitRepository::class); $builder->createField('id', Types::BIGINT) diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php index b9652a08..fde00abc 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.VisitLocation.php @@ -8,10 +8,10 @@ use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; -return static function (ClassMetadata $metadata): void { +return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable('visit_locations'); + $builder->setTable(determineTableName('visit_locations', $emConfig)); $builder->createField('id', Types::BIGINT) ->columnName('id') diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index 1384549f..7ab5ebbb 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -8,6 +8,8 @@ use Cake\Chronos\Chronos; use DateTimeInterface; use PUGX\Shortid\Factory as ShortIdFactory; +use function sprintf; + function generateRandomShortCode(int $length = 5): string { static $shortIdFactory; @@ -39,3 +41,15 @@ function parseDateField($date): ?Chronos return Chronos::parse($date); } + +function determineTableName(string $tableName, array $emConfig = []): string +{ + $schema = $emConfig['connection']['schema'] ?? null; +// $tablePrefix = $emConfig['connection']['table_prefix'] ?? null; // TODO + + if ($schema === null) { + return $tableName; + } + + return sprintf('%s.%s', $schema, $tableName); +} diff --git a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php index a79bd642..a5084cee 100644 --- a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php +++ b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php @@ -9,10 +9,12 @@ use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; use Doctrine\ORM\Mapping\ClassMetadata; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; -return static function (ClassMetadata $metadata): void { +use function Shlinkio\Shlink\Core\determineTableName; + +return static function (ClassMetadata $metadata, array $emConfig): void { $builder = new ClassMetadataBuilder($metadata); - $builder->setTable('api_keys'); + $builder->setTable(determineTableName('api_keys', $emConfig)); $builder->createField('id', Types::BIGINT) ->makePrimaryKey() @@ -31,5 +33,5 @@ return static function (ClassMetadata $metadata): void { ->build(); $builder->createField('enabled', Types::BOOLEAN) - ->build(); + ->build(); }; diff --git a/phpstan.neon b/phpstan.neon index b6c65f7f..d983a985 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -2,6 +2,5 @@ parameters: checkMissingIterableValueType: false checkGenericClassInNonGenericObjectType: false ignoreErrors: - - '#Undefined variable: \$metadata#' - '#AbstractQuery::setParameters()#' - '#mustRun()#' From 327d35fe5752aa1d8b01df55880f646a33a6dcde Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 1 Feb 2020 11:46:54 +0100 Subject: [PATCH 10/35] 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 11/35] 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 12/35] 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 13/35] 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 14/35] 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 15/35] 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 16/35] 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 17/35] 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 18/35] 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 19/35] 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 20/35] 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 21/35] 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 22/35] 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 23/35] 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 24/35] 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 25/35] 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 26/35] 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 27/35] 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 28/35] 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 29/35] 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 30/35] 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 31/35] 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 32/35] 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; From 8ff913aaf239bcacc5f8b0a8b7e0f6f062d4e26e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 20:07:19 +0100 Subject: [PATCH 33/35] Ensured search terms are applied to the domain too --- module/Core/src/Repository/ShortUrlRepository.php | 14 ++++++++------ module/Rest/test-api/Action/ListShortUrlsTest.php | 3 +++ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index a9d21952..31fe1385 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -110,12 +110,14 @@ class ShortUrlRepository extends EntityRepository implements ShortUrlRepositoryI } // Apply search conditions - $qb->andWhere($qb->expr()->orX( - $qb->expr()->like('s.longUrl', ':searchPattern'), - $qb->expr()->like('s.shortCode', ':searchPattern'), - $qb->expr()->like('t.name', ':searchPattern'), - )); - $qb->setParameter('searchPattern', '%' . $searchTerm . '%'); + $qb->leftJoin('s.domain', 'd') + ->andWhere($qb->expr()->orX( + $qb->expr()->like('s.longUrl', ':searchPattern'), + $qb->expr()->like('s.shortCode', ':searchPattern'), + $qb->expr()->like('t.name', ':searchPattern'), + $qb->expr()->like('d.authority', ':searchPattern'), + )) + ->setParameter('searchPattern', '%' . $searchTerm . '%'); } // Filter by tags if provided diff --git a/module/Rest/test-api/Action/ListShortUrlsTest.php b/module/Rest/test-api/Action/ListShortUrlsTest.php index 7af30948..7d4e51a7 100644 --- a/module/Rest/test-api/Action/ListShortUrlsTest.php +++ b/module/Rest/test-api/Action/ListShortUrlsTest.php @@ -169,6 +169,9 @@ class ListShortUrlsTest extends ApiTestCase self::SHORT_URL_META, self::SHORT_URL_CUSTOM_DOMAIN, ]]; + yield [['searchTerm' => 'example.com'], [ + self::SHORT_URL_CUSTOM_DOMAIN, + ]]; } private function buildPagination(int $itemsCount): array From 8d8a0f2484d08a21b1c7e152dd8f41767e93df7f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 20:08:22 +0100 Subject: [PATCH 34/35] Updated changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7bdbbabf..31fc0890 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## [Unreleased] +## 2.0.3 - 2020-02-02 #### Added @@ -27,6 +27,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#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. +* [#643](https://github.com/shlinkio/shlink/issues/643) Fixed searching on short URL lists not taking into consideration the domain name. ## 2.0.3 - 2020-01-27 From 0c0349fa3933326f138f57cfedef4b8ec8f00e13 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 2 Feb 2020 20:09:30 +0100 Subject: [PATCH 35/35] Fixed version on changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31fc0890..947806a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). -## 2.0.3 - 2020-02-02 +## 2.0.4 - 2020-02-02 #### Added