From 2abcaf02e22a4ede2e4d2d3abebf1d904f5dfa00 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 9 Jan 2022 11:23:27 +0100 Subject: [PATCH] Standardized ordering field handling and added validation for short URLs list --- composer.json | 2 +- .../Command/ShortUrl/ListShortUrlsCommand.php | 3 +- .../ShortUrl/ListShortUrlsCommandTest.php | 8 +-- module/Core/src/Model/Ordering.php | 35 +++++++++++ module/Core/src/Model/ShortUrlsOrdering.php | 60 ------------------- module/Core/src/Model/ShortUrlsParams.php | 7 ++- .../src/Repository/ShortUrlRepository.php | 9 +-- .../ShortUrlRepositoryInterface.php | 4 +- .../Validation/ShortUrlsParamsInputFilter.php | 3 + .../Repository/ShortUrlRepositoryTest.php | 10 +--- .../Adapter/ShortUrlRepositoryAdapterTest.php | 10 ++-- .../ShortUrl/ListShortUrlsActionTest.php | 9 +-- 12 files changed, 68 insertions(+), 92 deletions(-) create mode 100644 module/Core/src/Model/Ordering.php delete mode 100644 module/Core/src/Model/ShortUrlsOrdering.php diff --git a/composer.json b/composer.json index a5ca2bd1..2522f158 100644 --- a/composer.json +++ b/composer.json @@ -48,7 +48,7 @@ "predis/predis": "^1.1", "pugx/shortid-php": "^1.0", "ramsey/uuid": "^4.2", - "shlinkio/shlink-common": "^4.3", + "shlinkio/shlink-common": "dev-main#cbcff58 as 4.4", "shlinkio/shlink-config": "^1.5", "shlinkio/shlink-event-dispatcher": "^2.3", "shlinkio/shlink-importer": "^2.5", diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index 0c0daf33..751006bf 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -11,7 +11,6 @@ use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Common\Paginator\Util\PagerfantaUtilsTrait; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Service\ShortUrlServiceInterface; use Shlinkio\Shlink\Core\Validation\ShortUrlsParamsInputFilter; @@ -135,7 +134,7 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand ShortUrlsParamsInputFilter::SEARCH_TERM => $searchTerm, ShortUrlsParamsInputFilter::TAGS => $tags, ShortUrlsParamsInputFilter::TAGS_MODE => $tagsMode, - ShortUrlsOrdering::ORDER_BY => $orderBy, + ShortUrlsParamsInputFilter::ORDER_BY => $orderBy, ShortUrlsParamsInputFilter::START_DATE => $startDate?->toAtomString(), ShortUrlsParamsInputFilter::END_DATE => $endDate?->toAtomString(), ]; diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index 97ced44c..38d3bcd3 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -263,10 +263,10 @@ class ListShortUrlsCommandTest extends TestCase public function provideOrderBy(): iterable { yield [[], null]; - yield [['--order-by' => 'foo'], 'foo']; - yield [['--order-by' => 'foo,ASC'], 'foo-ASC']; - yield [['--order-by' => 'bar,DESC'], 'bar-DESC']; - yield [['--order-by' => 'baz-DESC'], 'baz-DESC']; + yield [['--order-by' => 'visits'], 'visits']; + yield [['--order-by' => 'longUrl,ASC'], 'longUrl-ASC']; + yield [['--order-by' => 'shortCode,DESC'], 'shortCode-DESC']; + yield [['--order-by' => 'title-DESC'], 'title-DESC']; } /** @test */ diff --git a/module/Core/src/Model/Ordering.php b/module/Core/src/Model/Ordering.php new file mode 100644 index 00000000..95fb6a14 --- /dev/null +++ b/module/Core/src/Model/Ordering.php @@ -0,0 +1,35 @@ +field; + } + + public function orderDirection(): string + { + return $this->dir; + } + + public function hasOrderField(): bool + { + return $this->field !== null; + } +} diff --git a/module/Core/src/Model/ShortUrlsOrdering.php b/module/Core/src/Model/ShortUrlsOrdering.php deleted file mode 100644 index 2466b571..00000000 --- a/module/Core/src/Model/ShortUrlsOrdering.php +++ /dev/null @@ -1,60 +0,0 @@ -validateAndInit($query); - - return $instance; - } - - /** - * @throws ValidationException - */ - private function validateAndInit(array $data): void - { - $orderBy = $data[self::ORDER_BY] ?? null; - if ($orderBy === null) { - return; - } - - [$field, $dir] = array_pad(explode('-', $orderBy), 2, null); - $this->orderField = $field; - $this->orderDirection = $dir ?? self::DEFAULT_ORDER_DIRECTION; - } - - public function orderField(): ?string - { - return $this->orderField; - } - - public function orderDirection(): string - { - return $this->orderDirection; - } - - public function hasOrderField(): bool - { - return $this->orderField !== null; - } -} diff --git a/module/Core/src/Model/ShortUrlsParams.php b/module/Core/src/Model/ShortUrlsParams.php index ac78b807..9abfd10f 100644 --- a/module/Core/src/Model/ShortUrlsParams.php +++ b/module/Core/src/Model/ShortUrlsParams.php @@ -13,6 +13,7 @@ use function Shlinkio\Shlink\Core\parseDateField; final class ShortUrlsParams { + public const ORDERABLE_FIELDS = ['longUrl', 'shortCode', 'dateCreated', 'title', 'visits']; public const DEFAULT_ITEMS_PER_PAGE = 10; public const TAGS_MODE_ANY = 'any'; public const TAGS_MODE_ALL = 'all'; @@ -23,7 +24,7 @@ final class ShortUrlsParams private array $tags; /** @var self::TAGS_MODE_ANY|self::TAGS_MODE_ALL */ private string $tagsMode = self::TAGS_MODE_ANY; - private ShortUrlsOrdering $orderBy; + private Ordering $orderBy; private ?DateRange $dateRange; private function __construct() @@ -63,7 +64,7 @@ final class ShortUrlsParams parseDateField($inputFilter->getValue(ShortUrlsParamsInputFilter::START_DATE)), parseDateField($inputFilter->getValue(ShortUrlsParamsInputFilter::END_DATE)), ); - $this->orderBy = ShortUrlsOrdering::fromRawData($query); + $this->orderBy = Ordering::fromTuple($inputFilter->getValue(ShortUrlsParamsInputFilter::ORDER_BY)); $this->itemsPerPage = (int) ( $inputFilter->getValue(ShortUrlsParamsInputFilter::ITEMS_PER_PAGE) ?? self::DEFAULT_ITEMS_PER_PAGE ); @@ -90,7 +91,7 @@ final class ShortUrlsParams return $this->tags; } - public function orderBy(): ShortUrlsOrdering + public function orderBy(): Ordering { return $this->orderBy; } diff --git a/module/Core/src/Repository/ShortUrlRepository.php b/module/Core/src/Repository/ShortUrlRepository.php index 3218f2ac..57aa480c 100644 --- a/module/Core/src/Repository/ShortUrlRepository.php +++ b/module/Core/src/Repository/ShortUrlRepository.php @@ -13,9 +13,9 @@ use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\Ordering; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; -use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; @@ -35,7 +35,7 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU ?string $searchTerm = null, array $tags = [], ?string $tagsMode = null, - ?ShortUrlsOrdering $orderBy = null, + ?Ordering $orderBy = null, ?DateRange $dateRange = null, ?Specification $spec = null, ): array { @@ -53,13 +53,14 @@ class ShortUrlRepository extends EntitySpecificationRepository implements ShortU return $qb->orderBy('s.dateCreated', 'DESC')->getQuery()->getResult(); } - private function processOrderByForList(QueryBuilder $qb, ShortUrlsOrdering $orderBy): array + private function processOrderByForList(QueryBuilder $qb, Ordering $orderBy): array { $fieldName = $orderBy->orderField(); $order = $orderBy->orderDirection(); if ($fieldName === 'visits') { - // FIXME This query is inefficient. Debug it. + // FIXME This query is inefficient. + // Diagnostic: It might need to use a sub-query, as done with the tags list query. $qb->addSelect('COUNT(DISTINCT v) AS totalVisits') ->leftJoin('s.visits', 'v') ->groupBy('s') diff --git a/module/Core/src/Repository/ShortUrlRepositoryInterface.php b/module/Core/src/Repository/ShortUrlRepositoryInterface.php index c3bd8d7f..c6f8df60 100644 --- a/module/Core/src/Repository/ShortUrlRepositoryInterface.php +++ b/module/Core/src/Repository/ShortUrlRepositoryInterface.php @@ -9,9 +9,9 @@ use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterfa use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Model\Ordering; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; -use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Importer\Model\ImportedShlinkUrl; interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificationRepositoryInterface @@ -22,7 +22,7 @@ interface ShortUrlRepositoryInterface extends ObjectRepository, EntitySpecificat ?string $searchTerm = null, array $tags = [], ?string $tagsMode = null, - ?ShortUrlsOrdering $orderBy = null, + ?Ordering $orderBy = null, ?DateRange $dateRange = null, ?Specification $spec = null, ): array; diff --git a/module/Core/src/Validation/ShortUrlsParamsInputFilter.php b/module/Core/src/Validation/ShortUrlsParamsInputFilter.php index 9223037f..6c0443aa 100644 --- a/module/Core/src/Validation/ShortUrlsParamsInputFilter.php +++ b/module/Core/src/Validation/ShortUrlsParamsInputFilter.php @@ -21,6 +21,7 @@ class ShortUrlsParamsInputFilter extends InputFilter public const END_DATE = 'endDate'; public const ITEMS_PER_PAGE = 'itemsPerPage'; public const TAGS_MODE = 'tagsMode'; + public const ORDER_BY = 'orderBy'; public function __construct(array $data) { @@ -46,5 +47,7 @@ class ShortUrlsParamsInputFilter extends InputFilter 'strict' => InArray::COMPARE_STRICT, ])); $this->add($tagsMode); + + $this->add($this->createOrderByInput(self::ORDER_BY, ShortUrlsParams::ORDERABLE_FIELDS)); } } diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 0001889b..5bf34437 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -11,9 +11,9 @@ 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\Model\Ordering; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; -use Shlinkio\Shlink\Core\Model\ShortUrlsOrdering; use Shlinkio\Shlink\Core\Model\ShortUrlsParams; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; @@ -128,9 +128,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase self::assertCount(1, $this->repo->findList(2, 2)); - $result = $this->repo->findList(null, null, null, [], null, ShortUrlsOrdering::fromRawData([ - 'orderBy' => 'visits-DESC', - ])); + $result = $this->repo->findList(null, null, null, [], null, Ordering::fromTuple(['visits', 'DESC'])); self::assertCount(3, $result); self::assertSame($bar, $result[0]); @@ -164,9 +162,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $result = $this->repo->findList(null, null, null, [], null, ShortUrlsOrdering::fromRawData([ - 'orderBy' => 'longUrl-ASC', - ])); + $result = $this->repo->findList(null, null, null, [], null, Ordering::fromTuple(['longUrl', 'ASC'])); self::assertCount(count($urls), $result); self::assertEquals('a', $result[0]->getLongUrl()); diff --git a/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php b/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php index 99195818..3d4b01ae 100644 --- a/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php +++ b/module/Core/test/Paginator/Adapter/ShortUrlRepositoryAdapterTest.php @@ -82,11 +82,11 @@ class ShortUrlRepositoryAdapterTest extends TestCase yield ['search']; yield ['search', []]; yield ['search', ['foo', 'bar']]; - 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 ['search', ['foo', 'bar'], null, null, 'longUrl']; + yield ['search', ['foo', 'bar'], Chronos::now()->toAtomString(), null, 'longUrl']; + yield ['search', ['foo', 'bar'], null, Chronos::now()->toAtomString(), 'longUrl']; + yield ['search', ['foo', 'bar'], Chronos::now()->toAtomString(), Chronos::now()->toAtomString(), 'longUrl']; + yield [null, ['foo', 'bar'], Chronos::now()->toAtomString(), null, 'longUrl']; yield [null, ['foo', 'bar'], Chronos::now()->toAtomString()]; yield [null, ['foo', 'bar'], Chronos::now()->toAtomString(), Chronos::now()->toAtomString()]; } diff --git a/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php b/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php index 170ccc09..59876b55 100644 --- a/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php +++ b/module/Rest/test/Action/ShortUrl/ListShortUrlsActionTest.php @@ -6,7 +6,7 @@ namespace ShlinkioTest\Shlink\Rest\Action\ShortUrl; use Cake\Chronos\Chronos; use Laminas\Diactoros\Response\JsonResponse; -use Laminas\Diactoros\ServerRequest; +use Laminas\Diactoros\ServerRequestFactory; use Pagerfanta\Adapter\ArrayAdapter; use PHPUnit\Framework\TestCase; use Prophecy\PhpUnit\ProphecyTrait; @@ -52,7 +52,8 @@ class ListShortUrlsActionTest extends TestCase ?string $endDate = null, ): void { $apiKey = ApiKey::create(); - $request = (new ServerRequest())->withQueryParams($query)->withAttribute(ApiKey::class, $apiKey); + $request = ServerRequestFactory::fromGlobals()->withQueryParams($query) + ->withAttribute(ApiKey::class, $apiKey); $listShortUrls = $this->service->listShortUrls(ShortUrlsParams::fromRawData([ 'page' => $expectedPage, 'searchTerm' => $expectedSearchTerm, @@ -81,10 +82,10 @@ class ListShortUrlsActionTest extends TestCase 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 [['orderBy' => $orderBy = 'longUrl'], 1, null, [], $orderBy]; yield [[ 'page' => '2', - 'orderBy' => $orderBy = 'something', + 'orderBy' => $orderBy = 'visits', 'tags' => $tags = ['one', 'two'], ], 2, null, $tags, $orderBy]; yield [