mirrors/shlink
1
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2024-11-27 16:26:37 +03:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #1345 from acelaya-forks/feature/extended-tags-ordering

Browse source 
Feature/extended tags ordering
This commit is contained in:
Alejandro Celaya 2022-01-23 11:08:02 +01:00 committed by GitHub
commit 1f90af3aec
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
17 changed files with 157 additions and 140 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
composer.json
View file

@ -74,7 +74,7 @@
"phpunit/phpunit": "^9.5",
"roave/security-advisories": "dev-master",
"shlinkio/php-coding-standard": "~2.2.0",
"shlinkio/shlink-test-utils": "^2.5",
"shlinkio/shlink-test-utils": "^3.0",
"symfony/var-dumper": "^6.0",
"veewee/composer-run-parallel": "^1.1"
},

2
docker-compose.yml
View file

@ -100,7 +100,7 @@ services:
shlink_db_maria:
container_name: shlink_db_maria
image: mariadb:10.5
image: mariadb:10.7
ports:
- "3308:3306"
volumes:

8
docs/swagger/paths/v2_tags_stats.json
View file

@ -45,13 +45,17 @@
{
"name": "orderBy",
"in": "query",
"description": "To determine how to order the results.",
"description": "To determine how to order the results.<br /><br />**Important!** Ordering by `shortUrlsCount` or `visitsCount` has a [known performance issue](https://github.com/shlinkio/shlink/issues/1346) which makes loading a subset of the list take as much as loading the whole list.<br />If you plan to order by any of these fields, it's worth loading the whole list with no pagination.",
"required": false,
"schema": {
"type": "string",
"enum": [
"tag-ASC",
"tag-DESC"
"tag-DESC",
"shortUrlsCount-ASC",
"shortUrlsCount-DESC",
"visitsCount-ASC",
"visitsCount-DESC"
]
}
}

3
module/CLI/src/Command/Tag/ListTagsCommand.php
View file

@ -46,8 +46,7 @@ class ListTagsCommand extends Command
return map(
$tags,
static fn (TagInfo $tagInfo) =>
[$tagInfo->tag()->__toString(), $tagInfo->shortUrlsCount(), $tagInfo->visitsCount()],
static fn (TagInfo $tagInfo) => [$tagInfo->tag(), $tagInfo->shortUrlsCount(), $tagInfo->visitsCount()],
);
}
}

5
module/CLI/test/Command/Tag/ListTagsCommandTest.php
View file

@ -10,7 +10,6 @@ use Prophecy\Argument;
use Prophecy\Prophecy\ObjectProphecy;
use Shlinkio\Shlink\CLI\Command\Tag\ListTagsCommand;
use Shlinkio\Shlink\Common\Paginator\Paginator;
use Shlinkio\Shlink\Core\Entity\Tag;
use Shlinkio\Shlink\Core\Tag\Model\TagInfo;
use Shlinkio\Shlink\Core\Tag\TagServiceInterface;
use ShlinkioTest\Shlink\CLI\CliTestUtilsTrait;
@ -45,8 +44,8 @@ class ListTagsCommandTest extends TestCase
public function listOfTagsIsPrinted(): void
{
$tagsInfo = $this->tagService->tagsInfo(Argument::any())->willReturn(new Paginator(new ArrayAdapter([
new TagInfo(new Tag('foo'), 10, 2),
new TagInfo(new Tag('bar'), 7, 32),
new TagInfo('foo', 10, 2),
new TagInfo('bar', 7, 32),
])));
$this->commandTester->execute([]);

3
module/Core/src/Model/Ordering.php
View file

@ -12,6 +12,9 @@ final class Ordering
{
}
/**
* @param array{string|null, string|null} $props
*/
public static function fromTuple(array $props): self
{
[$field, $dir] = $props;

40
module/Core/src/Repository/TagRepository.php
View file

@ -16,6 +16,7 @@ use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin;
use Shlinkio\Shlink\Rest\ApiKey\Spec\WithInlinedApiKeySpecsEnsuringJoin;
use Shlinkio\Shlink\Rest\Entity\ApiKey;
use function Functional\contains;
use function Functional\map;
use const PHP_INT_MAX;
@ -40,12 +41,19 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
*/
public function findTagsWithInfo(?TagsListFiltering $filtering = null): array
{
$orderField = $filtering?->orderBy()?->orderField();
$orderDir = $filtering?->orderBy()?->orderDirection();
$orderMainQuery = contains(['shortUrlsCount', 'visitsCount'], $orderField);
$conn = $this->getEntityManager()->getConnection();
$subQb = $this->createQueryBuilder('t');
$subQb->select('t.id', 't.name')
->orderBy('t.name', $filtering?->orderBy()?->orderDirection() ?? 'ASC') // TODO Make filed dynamic
->setMaxResults($filtering?->limit() ?? PHP_INT_MAX)
->setFirstResult($filtering?->offset() ?? 0);
$subQb->select('t.id', 't.name');
if (! $orderMainQuery) {
$subQb->orderBy('t.name', $orderDir ?? 'ASC')
->setMaxResults($filtering?->limit() ?? PHP_INT_MAX)
->setFirstResult($filtering?->offset() ?? 0);
}
$searchTerm = $filtering?->searchTerm();
if ($searchTerm !== null) {
@ -53,7 +61,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
}
$apiKey = $filtering?->apiKey();
$this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey, 'shortUrls'), 't');
$this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey), 't');
$subQuery = $subQb->getQuery();
$subQuerySql = $subQuery->getSQL();
@ -73,11 +81,10 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
->leftJoin('t', 'short_urls_in_tags', 'st', $nativeQb->expr()->eq('t.id_0', 'st.tag_id'))
->leftJoin('st', 'short_urls', 's', $nativeQb->expr()->eq('s.id', 'st.short_url_id'))
->leftJoin('st', 'visits', 'v', $nativeQb->expr()->eq('s.id', 'v.short_url_id'))
->groupBy('t.id_0', 't.name_1')
->orderBy('t.name_1', $filtering?->orderBy()?->orderDirection() ?? 'ASC'); // TODO Make field dynamic
->groupBy('t.id_0', 't.name_1');
// Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates
$apiKey?->mapRoles(fn (string $roleName, array $meta) => match ($roleName) {
$apiKey?->mapRoles(static fn (string $roleName, array $meta) => match ($roleName) {
Role::DOMAIN_SPECIFIC => $nativeQb->andWhere(
$nativeQb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))),
),
@ -87,14 +94,27 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito
default => $nativeQb,
});
if ($orderMainQuery) {
$nativeQb
->orderBy(
$orderField === 'shortUrlsCount' ? 'short_urls_count' : 'visits_count',
$orderDir ?? 'ASC',
)
->setMaxResults($filtering?->limit() ?? PHP_INT_MAX)
->setFirstResult($filtering?->offset() ?? 0);
}
// Add ordering by tag name, as a fallback in case of same amount, or as default ordering
$nativeQb->addOrderBy('t.name_1', $orderMainQuery || $orderDir === null ? 'ASC' : $orderDir);
$rsm = new ResultSetMappingBuilder($this->getEntityManager());
$rsm->addRootEntityFromClassMetadata(Tag::class, 't');
$rsm->addScalarResult('name', 'tag');
$rsm->addScalarResult('short_urls_count', 'shortUrlsCount');
$rsm->addScalarResult('visits_count', 'visitsCount');
return map(
$this->getEntityManager()->createNativeQuery($nativeQb->getSQL(), $rsm)->getResult(),
static fn (array $row) => new TagInfo($row[0], (int) $row['shortUrlsCount'], (int) $row['visitsCount']),
static fn (array $row) => new TagInfo($row['tag'], (int) $row['shortUrlsCount'], (int) $row['visitsCount']),
);
}

5
module/Core/src/Tag/Model/TagInfo.php
View file

@ -5,15 +5,14 @@ declare(strict_types=1);
namespace Shlinkio\Shlink\Core\Tag\Model;
use JsonSerializable;
use Shlinkio\Shlink\Core\Entity\Tag;
final class TagInfo implements JsonSerializable
{
public function __construct(private Tag $tag, private int $shortUrlsCount, private int $visitsCount)
public function __construct(private string $tag, private int $shortUrlsCount, private int $visitsCount)
{
}
public function tag(): Tag
public function tag(): string
{
return $this->tag;
}

2
module/Core/test-db/Domain/Repository/DomainRepositoryTest.php
View file

@ -21,7 +21,7 @@ class DomainRepositoryTest extends DatabaseTestCase
{
private DomainRepository $repo;
protected function beforeEach(): void
protected function setUp(): void
{
$this->repo = $this->getEntityManager()->getRepository(Domain::class);
}

2
module/Core/test-db/Repository/ShortUrlRepositoryTest.php
View file

@ -33,7 +33,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase
private ShortUrlRepository $repo;
private PersistenceShortUrlRelationResolver $relationResolver;
public function beforeEach(): void
protected function setUp(): void
{
$this->repo = $this->getEntityManager()->getRepository(ShortUrl::class);
$this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager());

208
module/Core/test-db/Repository/TagRepositoryTest.php
View file

@ -13,7 +13,6 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
use Shlinkio\Shlink\Core\Model\Visitor;
use Shlinkio\Shlink\Core\Repository\TagRepository;
use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver;
use Shlinkio\Shlink\Core\Tag\Model\TagInfo;
use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering;
use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition;
@ -21,13 +20,14 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase;
use function array_chunk;
use function count;
class TagRepositoryTest extends DatabaseTestCase
{
private TagRepository $repo;
private PersistenceShortUrlRelationResolver $relationResolver;
protected function beforeEach(): void
protected function setUp(): void
{
$this->repo = $this->getEntityManager()->getRepository(Tag::class);
$this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager());
@ -57,7 +57,7 @@ class TagRepositoryTest extends DatabaseTestCase
* @test
* @dataProvider provideFilters
*/
public function properTagsInfoIsReturned(?TagsListFiltering $filtering, callable $asserts): void
public function properTagsInfoIsReturned(?TagsListFiltering $filtering, array $expectedList): void
{
$names = ['foo', 'bar', 'baz', 'another'];
foreach ($names as $name) {
@ -86,126 +86,120 @@ class TagRepositoryTest extends DatabaseTestCase
$shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags, null), $this->relationResolver);
$this->getEntityManager()->persist($shortUrl2);
$this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl2, Visitor::emptyInstance()));
// One of the tags has two extra short URLs, but with no visits
$this->getEntityManager()->persist(
ShortUrl::fromMeta($metaWithTags(['bar'], null), $this->relationResolver),
);
$this->getEntityManager()->persist(
ShortUrl::fromMeta($metaWithTags(['bar'], $apiKey), $this->relationResolver),
);
$this->getEntityManager()->flush();
$result = $this->repo->findTagsWithInfo($filtering);
$asserts($result, $names);
self::assertCount(count($expectedList), $result);
foreach ($expectedList as $index => [$tag, $shortUrlsCount, $visitsCount]) {
self::assertEquals($shortUrlsCount, $result[$index]->shortUrlsCount());
self::assertEquals($visitsCount, $result[$index]->visitsCount());
self::assertEquals($tag, $result[$index]->tag());
}
}
public function provideFilters(): iterable
{
$defaultAsserts = static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(4, $result);
self::assertEquals(0, $result[0]->shortUrlsCount());
self::assertEquals(0, $result[0]->visitsCount());
self::assertEquals($tagNames[3], $result[0]->tag()->__toString());
self::assertEquals(1, $result[1]->shortUrlsCount());
self::assertEquals(3, $result[1]->visitsCount());
self::assertEquals($tagNames[1], $result[1]->tag()->__toString());
self::assertEquals(1, $result[2]->shortUrlsCount());
self::assertEquals(3, $result[2]->visitsCount());
self::assertEquals($tagNames[2], $result[2]->tag()->__toString());
self::assertEquals(2, $result[3]->shortUrlsCount());
self::assertEquals(4, $result[3]->visitsCount());
self::assertEquals($tagNames[0], $result[3]->tag()->__toString());
};
yield 'no filter' => [null, $defaultAsserts];
yield 'empty filter' => [new TagsListFiltering(), $defaultAsserts];
yield 'limit' => [new TagsListFiltering(2), static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(2, $result);
self::assertEquals(0, $result[0]->shortUrlsCount());
self::assertEquals(0, $result[0]->visitsCount());
self::assertEquals($tagNames[3], $result[0]->tag()->__toString());
self::assertEquals(1, $result[1]->shortUrlsCount());
self::assertEquals(3, $result[1]->visitsCount());
self::assertEquals($tagNames[1], $result[1]->tag()->__toString());
}];
yield 'offset' => [new TagsListFiltering(null, 3), static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(1, $result);
self::assertEquals(2, $result[0]->shortUrlsCount());
self::assertEquals(4, $result[0]->visitsCount());
self::assertEquals($tagNames[0], $result[0]->tag()->__toString());
}];
yield 'limit and offset' => [
new TagsListFiltering(2, 1),
static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(2, $result);
self::assertEquals(1, $result[0]->shortUrlsCount());
self::assertEquals(3, $result[0]->visitsCount());
self::assertEquals($tagNames[1], $result[0]->tag()->__toString());
self::assertEquals(1, $result[1]->shortUrlsCount());
self::assertEquals(3, $result[1]->visitsCount());
self::assertEquals($tagNames[2], $result[1]->tag()->__toString());
},
$defaultList = [
['another', 0, 0],
['bar', 3, 3],
['baz', 1, 3],
['foo', 2, 4],
];
yield 'search term' => [
new TagsListFiltering(null, null, 'ba'),
static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(2, $result);
self::assertEquals(1, $result[0]->shortUrlsCount());
self::assertEquals(3, $result[0]->visitsCount());
self::assertEquals($tagNames[1], $result[0]->tag()->__toString());
self::assertEquals(1, $result[1]->shortUrlsCount());
self::assertEquals(3, $result[1]->visitsCount());
self::assertEquals($tagNames[2], $result[1]->tag()->__toString());
},
];
yield 'no filter' => [null, $defaultList];
yield 'empty filter' => [new TagsListFiltering(), $defaultList];
yield 'limit' => [new TagsListFiltering(2), [
['another', 0, 0],
['bar', 3, 3],
]];
yield 'offset' => [new TagsListFiltering(null, 3), [
['foo', 2, 4],
]];
yield 'limit and offset' => [new TagsListFiltering(2, 1), [
['bar', 3, 3],
['baz', 1, 3],
]];
yield 'search term' => [new TagsListFiltering(null, null, 'ba'), [
['bar', 3, 3],
['baz', 1, 3],
]];
yield 'ASC ordering' => [
new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'ASC'])),
$defaultAsserts,
$defaultList,
];
yield 'DESC ordering' => [
new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'DESC'])),
static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(4, $result);
self::assertEquals(0, $result[3]->shortUrlsCount());
self::assertEquals(0, $result[3]->visitsCount());
self::assertEquals($tagNames[3], $result[3]->tag()->__toString());
self::assertEquals(1, $result[2]->shortUrlsCount());
self::assertEquals(3, $result[2]->visitsCount());
self::assertEquals($tagNames[1], $result[2]->tag()->__toString());
self::assertEquals(1, $result[1]->shortUrlsCount());
self::assertEquals(3, $result[1]->visitsCount());
self::assertEquals($tagNames[2], $result[1]->tag()->__toString());
self::assertEquals(2, $result[0]->shortUrlsCount());
self::assertEquals(4, $result[0]->visitsCount());
self::assertEquals($tagNames[0], $result[0]->tag()->__toString());
},
yield 'DESC ordering' => [new TagsListFiltering(null, null, null, Ordering::fromTuple(['tag', 'DESC'])), [
['foo', 2, 4],
['baz', 1, 3],
['bar', 3, 3],
['another', 0, 0],
]];
yield 'short URLs count ASC ordering' => [
new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'ASC'])),
[
['another', 0, 0],
['baz', 1, 3],
['foo', 2, 4],
['bar', 3, 3],
],
];
yield 'short URLs count DESC ordering' => [
new TagsListFiltering(null, null, null, Ordering::fromTuple(['shortUrlsCount', 'DESC'])),
[
['bar', 3, 3],
['foo', 2, 4],
['baz', 1, 3],
['another', 0, 0],
],
];
yield 'visits count ASC ordering' => [
new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'ASC'])),
[
['another', 0, 0],
['bar', 3, 3],
['baz', 1, 3],
['foo', 2, 4],
],
];
yield 'visits count DESC ordering' => [
new TagsListFiltering(null, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])),
[
['foo', 2, 4],
['bar', 3, 3],
['baz', 1, 3],
['another', 0, 0],
],
];
yield 'visits count DESC ordering and limit' => [
new TagsListFiltering(2, null, null, Ordering::fromTuple(['visitsCount', 'DESC'])),
[
['foo', 2, 4],
['bar', 3, 3],
],
];
yield 'api key' => [new TagsListFiltering(null, null, null, null, ApiKey::fromMeta(
ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()),
)), static function (array $result, array $tagNames): void {
/** @var TagInfo[] $result */
self::assertCount(3, $result);
self::assertEquals(1, $result[0]->shortUrlsCount());
self::assertEquals(3, $result[0]->visitsCount());
self::assertEquals($tagNames[1], $result[0]->tag()->__toString());
self::assertEquals(1, $result[1]->shortUrlsCount());
self::assertEquals(3, $result[1]->visitsCount());
self::assertEquals($tagNames[2], $result[1]->tag()->__toString());
self::assertEquals(1, $result[2]->shortUrlsCount());
self::assertEquals(3, $result[2]->visitsCount());
self::assertEquals($tagNames[0], $result[2]->tag()->__toString());
}];
)), [
['bar', 2, 3],
['baz', 1, 3],
['foo', 1, 3],
]];
yield 'combined' => [new TagsListFiltering(1, null, null, Ordering::fromTuple(
['shortUrls', 'DESC'],
), ApiKey::fromMeta(
ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()),
)), [
['foo', 1, 3],
]];
}
/** @test */

2
module/Core/test-db/Repository/VisitRepositoryTest.php
View file

@ -38,7 +38,7 @@ class VisitRepositoryTest extends DatabaseTestCase
private VisitRepository $repo;
private PersistenceShortUrlRelationResolver $relationResolver;
protected function beforeEach(): void
protected function setUp(): void
{
$this->repo = $this->getEntityManager()->getRepository(Visit::class);
$this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager());

2
module/Core/test-db/Tag/Paginator/Adapter/TagsPaginatorAdapterTest.php
View file

@ -16,7 +16,7 @@ class TagsPaginatorAdapterTest extends DatabaseTestCase
{
private TagRepository $repo;
protected function beforeEach(): void
protected function setUp(): void
{
$this->repo = $this->getEntityManager()->getRepository(Tag::class);
}

2
module/Core/test/Tag/TagServiceTest.php
View file

@ -67,7 +67,7 @@ class TagServiceTest extends TestCase
TagsListFiltering $expectedFiltering,
int $countCalls,
): void {
$expected = [new TagInfo(new Tag('foo'), 1, 1), new TagInfo(new Tag('bar'), 3, 10)];
$expected = [new TagInfo('foo', 1, 1), new TagInfo('bar', 3, 10)];
$find = $this->repo->findTagsWithInfo($expectedFiltering)->willReturn($expected);
$count = $this->repo->matchSingleScalarResult(Argument::cetera())->willReturn(2);

2
module/Rest/src/Action/Tag/ListTagsAction.php
View file

@ -41,7 +41,7 @@ class ListTagsAction extends AbstractRestAction
// This part is deprecated. To get tags with stats, the /tags/stats endpoint should be used instead
$tagsInfo = $this->tagService->tagsInfo($params, $apiKey);
$rawTags = $this->serializePaginator($tagsInfo, null, 'stats');
$rawTags['data'] = map($tagsInfo, static fn (TagInfo $info) => $info->tag()->__toString());
$rawTags['data'] = map($tagsInfo, static fn (TagInfo $info) => $info->tag());
return new JsonResponse(['tags' => $rawTags]);
}

4
module/Rest/test/Action/Tag/ListTagsActionTest.php
View file

@ -76,8 +76,8 @@ class ListTagsActionTest extends TestCase
public function returnsStatsWhenRequested(): void
{
$stats = [
new TagInfo(new Tag('foo'), 1, 1),
new TagInfo(new Tag('bar'), 3, 10),
new TagInfo('foo', 1, 1),
new TagInfo('bar', 3, 10),
];
$itemsCount = count($stats);
$tagsInfo = $this->tagService->tagsInfo(Argument::any(), Argument::type(ApiKey::class))->willReturn(

5
module/Rest/test/Action/Tag/TagsStatsActionTest.php
View file

@ -13,7 +13,6 @@ use Prophecy\PhpUnit\ProphecyTrait;
use Prophecy\Prophecy\ObjectProphecy;
use Psr\Http\Message\ServerRequestInterface;
use Shlinkio\Shlink\Common\Paginator\Paginator;
use Shlinkio\Shlink\Core\Entity\Tag;
use Shlinkio\Shlink\Core\Tag\Model\TagInfo;
use Shlinkio\Shlink\Core\Tag\TagServiceInterface;
use Shlinkio\Shlink\Rest\Action\Tag\TagsStatsAction;
@ -38,8 +37,8 @@ class TagsStatsActionTest extends TestCase
public function returnsTagsStatsWhenRequested(): void
{
$stats = [
new TagInfo(new Tag('foo'), 1, 1),
new TagInfo(new Tag('bar'), 3, 10),
new TagInfo('foo', 1, 1),
new TagInfo('bar', 3, 10),
];
$itemsCount = count($stats);
$tagsInfo = $this->tagService->tagsInfo(Argument::any(), Argument::type(ApiKey::class))->willReturn(