diff --git a/CHANGELOG.md b/CHANGELOG.md index 8157c47f..7b5f5cff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Changed * [#2034](https://github.com/shlinkio/shlink/issues/2034) Modernize entities, using constructor property promotion and readonly wherever possible. +* [#2036](https://github.com/shlinkio/shlink/issues/2036) Deep performance improvement when listing short URLs ordered by visits counts. + + This has been achieved by introducing a new table which tracks slotted visits counts. We can then `SUM` all counts for certain visit, avoiding `COUNT(visits)` aggregates which are less performant when there are a lot of visits. ### Deprecated * *Nothing* diff --git a/composer.json b/composer.json index a35dd727..8d8f33f9 100644 --- a/composer.json +++ b/composer.json @@ -46,7 +46,7 @@ "shlinkio/shlink-common": "^6.0", "shlinkio/shlink-config": "^3.0", "shlinkio/shlink-event-dispatcher": "^4.0", - "shlinkio/shlink-importer": "^5.3", + "shlinkio/shlink-importer": "^5.3.1", "shlinkio/shlink-installer": "^9.0", "shlinkio/shlink-ip-geolocation": "^4.0", "shlinkio/shlink-json": "^1.1", @@ -129,6 +129,10 @@ "test:db:postgres": "DB_DRIVER=postgres composer test:db:sqlite", "test:db:ms": "DB_DRIVER=mssql composer test:db:sqlite", "test:api": "bin/test/run-api-tests.sh", + "test:api:sqlite": "DB_DRIVER=sqlite composer test:api", + "test:api:mysql": "DB_DRIVER=mysql composer test:api", + "test:api:maria": "DB_DRIVER=maria composer test:api", + "test:api:mssql": "DB_DRIVER=mssql composer test:api", "test:api:ci": "GENERATE_COVERAGE=yes composer test:api && vendor/bin/phpcov merge build/coverage-api --php build/coverage-api.cov && rm build/coverage-api/*.cov", "test:api:pretty": "GENERATE_COVERAGE=yes composer test:api && vendor/bin/phpcov merge build/coverage-api --html build/coverage-api/coverage-html && rm build/coverage-api/*.cov", "test:cli": "APP_ENV=test DB_DRIVER=maria TEST_ENV=cli php vendor/bin/phpunit --order-by=random --colors=always --testdox -c phpunit-cli.xml", diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index 3eb43edf..84233915 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -2,8 +2,10 @@ declare(strict_types=1); +use Doctrine\ORM\Events; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Core\Config\EnvVars; +use Shlinkio\Shlink\Core\Visit\Listener\ShortUrlVisitsCountTracker; use function Shlinkio\Shlink\Core\ArrayUtils\contains; @@ -60,6 +62,10 @@ return (static function (): array { 'proxies_dir' => 'data/proxies', 'load_mappings_using_functional_style' => true, 'default_repository_classname' => EntitySpecificationRepository::class, + 'listeners' => [ + Events::onFlush => [ShortUrlVisitsCountTracker::class], + Events::postFlush => [ShortUrlVisitsCountTracker::class], + ], ], 'connection' => $resolveConnection(), ], diff --git a/config/autoload/rabbit.local.php.dist b/config/autoload/rabbit.local.php.dist index b758528e..d19f82c6 100644 --- a/config/autoload/rabbit.local.php.dist +++ b/config/autoload/rabbit.local.php.dist @@ -7,7 +7,7 @@ return [ 'rabbitmq' => [ 'enabled' => true, 'host' => 'shlink_rabbitmq', - 'port' => '5673', + 'port' => '5672', 'user' => 'rabbit', 'password' => 'rabbit', ], diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index c4346f14..4e3a7706 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Common\Paginator\Util\PagerfantaUtilsTrait; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlsParams; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlsParamsInputFilter; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlListServiceInterface; @@ -23,10 +24,10 @@ use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; use function array_keys; -use function array_map; use function array_pad; use function explode; use function implode; +use function Shlinkio\Shlink\Core\ArrayUtils\map; use function sprintf; class ListShortUrlsCommand extends Command @@ -176,6 +177,9 @@ class ListShortUrlsCommand extends Command return ExitCode::EXIT_SUCCESS; } + /** + * @param array $columnsMap + */ private function renderPage( OutputInterface $output, array $columnsMap, @@ -184,10 +188,10 @@ class ListShortUrlsCommand extends Command ): Paginator { $shortUrls = $this->shortUrlService->listShortUrls($params); - $rows = array_map(function (ShortUrl $shortUrl) use ($columnsMap) { - $rawShortUrl = $this->transformer->transform($shortUrl); - return array_map(fn (callable $call) => $call($rawShortUrl, $shortUrl), $columnsMap); - }, [...$shortUrls]); + $rows = map([...$shortUrls], function (ShortUrlWithVisitsSummary $shortUrl) use ($columnsMap) { + $serializedShortUrl = $this->transformer->transform($shortUrl); + return map($columnsMap, fn (callable $call) => $call($serializedShortUrl, $shortUrl->shortUrl)); + }); ShlinkTable::default($output)->render( array_keys($columnsMap), @@ -209,6 +213,9 @@ class ListShortUrlsCommand extends Command return $dir === null ? $field : sprintf('%s-%s', $field, $dir); } + /** + * @return array + */ private function resolveColumnsMap(InputInterface $input): array { $pickProp = static fn (string $prop): callable => static fn (array $shortUrl) => $shortUrl[$prop]; diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index 6859ec13..6016da1c 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -16,6 +16,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlsParams; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlListServiceInterface; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; @@ -47,7 +48,7 @@ class ListShortUrlsCommandTest extends TestCase // The paginator will return more than one page $data = []; for ($i = 0; $i < 50; $i++) { - $data[] = ShortUrl::withLongUrl('https://url_' . $i); + $data[] = ShortUrlWithVisitsSummary::fromShortUrl(ShortUrl::withLongUrl('https://url_' . $i)); } $this->shortUrlService->expects($this->exactly(3))->method('listShortUrls')->withAnyParameters() @@ -69,7 +70,7 @@ class ListShortUrlsCommandTest extends TestCase // The paginator will return more than one page $data = []; for ($i = 0; $i < 30; $i++) { - $data[] = ShortUrl::withLongUrl('https://url_' . $i); + $data[] = ShortUrlWithVisitsSummary::fromShortUrl(ShortUrl::withLongUrl('https://url_' . $i)); } $this->shortUrlService->expects($this->once())->method('listShortUrls')->with( @@ -111,11 +112,13 @@ class ListShortUrlsCommandTest extends TestCase $this->shortUrlService->expects($this->once())->method('listShortUrls')->with( ShortUrlsParams::emptyInstance(), )->willReturn(new Paginator(new ArrayAdapter([ - ShortUrl::create(ShortUrlCreation::fromRawData([ - 'longUrl' => 'https://foo.com', - 'tags' => ['foo', 'bar', 'baz'], - 'apiKey' => $apiKey, - ])), + ShortUrlWithVisitsSummary::fromShortUrl( + ShortUrl::create(ShortUrlCreation::fromRawData([ + 'longUrl' => 'https://foo.com', + 'tags' => ['foo', 'bar', 'baz'], + 'apiKey' => $apiKey, + ])), + ), ]))); $this->commandTester->setInputs(['y']); diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index d75c6bb8..951c7b52 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -76,6 +76,7 @@ return [ EntityRepositoryFactory::class, Visit\Entity\Visit::class, ], + Visit\Listener\ShortUrlVisitsCountTracker::class => InvokableFactory::class, Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class, Util\RedirectResponseHelper::class => ConfigAbstractFactory::class, diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php index 358ee6bd..b159da13 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.ShortUrl.Entity.ShortUrl.php @@ -67,6 +67,11 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->fetchExtraLazy() ->build(); + $builder->createOneToMany('visitsCounts', Visit\Entity\ShortUrlVisitsCount::class) + ->mappedBy('shortUrl') + ->fetchExtraLazy() // TODO Check if this makes sense + ->build(); + $builder->createManyToMany('tags', Tag\Entity\Tag::class) ->setJoinTable(determineTableName('short_urls_in_tags', $emConfig)) ->addInverseJoinColumn('tag_id', 'id', onDelete: 'CASCADE') diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php new file mode 100644 index 00000000..d4a8546b --- /dev/null +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.ShortUrlVisitsCount.php @@ -0,0 +1,44 @@ +setTable(determineTableName('short_url_visits_counts', $emConfig)); + + $builder->createField('id', Types::BIGINT) + ->columnName('id') + ->makePrimaryKey() + ->generatedValue('IDENTITY') + ->option('unsigned', true) + ->build(); + + $builder->createField('potentialBot', Types::BOOLEAN) + ->columnName('potential_bot') + ->option('default', false) + ->build(); + + $builder->createField('count', Types::BIGINT) + ->columnName('count') + ->option('unsigned', true) + ->option('default', 1) + ->build(); + + $builder->createField('slotId', Types::INTEGER) + ->columnName('slot_id') + ->option('unsigned', true) + ->build(); + + $builder->createManyToOne('shortUrl', ShortUrl\Entity\ShortUrl::class) + ->addJoinColumn('short_url_id', 'id', onDelete: 'CASCADE') + ->build(); + + $builder->addUniqueConstraint(['short_url_id', 'potential_bot', 'slot_id'], 'UQ_slot_per_short_url'); +}; diff --git a/module/Core/migrations/Version20240306132518.php b/module/Core/migrations/Version20240306132518.php new file mode 100644 index 00000000..a2960e9b --- /dev/null +++ b/module/Core/migrations/Version20240306132518.php @@ -0,0 +1,65 @@ +skipIf($schema->hasTable('short_url_visits_counts')); + + $table = $schema->createTable('short_url_visits_counts'); + $table->addColumn('id', Types::BIGINT, [ + 'unsigned' => true, + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->setPrimaryKey(['id']); + + $table->addColumn('short_url_id', Types::BIGINT, [ + 'unsigned' => true, + 'notnull' => true, + ]); + $table->addForeignKeyConstraint('short_urls', ['short_url_id'], ['id'], [ + 'onDelete' => 'CASCADE', + 'onUpdate' => 'RESTRICT', + ]); + + $table->addColumn('potential_bot', Types::BOOLEAN, ['default' => false]); + + $table->addColumn('slot_id', Types::INTEGER, [ + 'unsigned' => true, + 'notnull' => true, + 'default' => 1, + ]); + + $table->addColumn('count', Types::BIGINT, [ + 'unsigned' => true, + 'notnull' => true, + 'default' => 1, + ]); + + $table->addUniqueIndex(['short_url_id', 'potential_bot', 'slot_id'], 'UQ_slot_per_short_url'); + } + + public function down(Schema $schema): void + { + $this->skipIf(! $schema->hasTable('short_url_visits_counts')); + $schema->dropTable('short_url_visits_counts'); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/migrations/Version20240318084804.php b/module/Core/migrations/Version20240318084804.php new file mode 100644 index 00000000..6ff25fac --- /dev/null +++ b/module/Core/migrations/Version20240318084804.php @@ -0,0 +1,58 @@ +connection->createQueryBuilder(); + $result = $qb->select('id') + ->from('short_urls') + ->executeQuery(); + + while ($shortUrlId = $result->fetchOne()) { + $visitsQb = $this->connection->createQueryBuilder(); + $visitsQb->select('COUNT(id)') + ->from('visits') + ->where($visitsQb->expr()->eq('short_url_id', ':short_url_id')) + ->andWhere($visitsQb->expr()->eq('potential_bot', ':potential_bot')) + ->setParameter('short_url_id', $shortUrlId); + + $botsCount = $visitsQb->setParameter('potential_bot', '1')->executeQuery()->fetchOne(); + $nonBotsCount = $visitsQb->setParameter('potential_bot', '0')->executeQuery()->fetchOne(); + + if ($botsCount > 0) { + $this->insertCount($shortUrlId, $botsCount, potentialBot: true); + } + if ($nonBotsCount > 0) { + $this->insertCount($shortUrlId, $nonBotsCount, potentialBot: false); + } + } + } + + private function insertCount(string|int $shortUrlId, string|int $count, bool $potentialBot): void + { + $this->connection->createQueryBuilder() + ->insert('short_url_visits_counts') + ->values([ + 'short_url_id' => ':short_url_id', + 'count' => ':count', + 'potential_bot' => ':potential_bot', + ]) + ->setParameters([ + 'short_url_id' => $shortUrlId, + 'count' => $count, + 'potential_bot' => $potentialBot ? '1' : '0', + ]) + ->executeStatement(); + } +} diff --git a/module/Core/src/EventDispatcher/LocateVisit.php b/module/Core/src/EventDispatcher/LocateVisit.php index aa6afed8..6f7fb7e8 100644 --- a/module/Core/src/EventDispatcher/LocateVisit.php +++ b/module/Core/src/EventDispatcher/LocateVisit.php @@ -17,7 +17,7 @@ use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use Throwable; -class LocateVisit +readonly class LocateVisit { public function __construct( private IpLocationResolverInterface $ipLocationResolver, diff --git a/module/Core/src/Model/Ordering.php b/module/Core/src/Model/Ordering.php index 69a1429a..e1b91510 100644 --- a/module/Core/src/Model/Ordering.php +++ b/module/Core/src/Model/Ordering.php @@ -10,10 +10,15 @@ final readonly class Ordering private const ASC_DIR = 'ASC'; private const DEFAULT_DIR = self::ASC_DIR; - private function __construct(public ?string $field, public string $direction) + public function __construct(public ?string $field = null, public string $direction = self::DEFAULT_DIR) { } + public static function none(): self + { + return new self(); + } + /** * @param array{string|null, string|null} $props */ @@ -23,11 +28,6 @@ final readonly class Ordering return new self($field, $dir ?? self::DEFAULT_DIR); } - public static function none(): self - { - return new self(null, self::DEFAULT_DIR); - } - public static function fromFieldAsc(string $field): self { return new self($field, self::ASC_DIR); diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 474d5afc..ac21b34b 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -19,6 +19,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Core\ShortUrl\Resolver\ShortUrlRelationResolverInterface; use Shlinkio\Shlink\Core\ShortUrl\Resolver\SimpleShortUrlRelationResolver; use Shlinkio\Shlink\Core\Tag\Entity\Tag; +use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\VisitsSummary; use Shlinkio\Shlink\Core\Visit\Model\VisitType; @@ -37,6 +38,7 @@ class ShortUrl extends AbstractEntity /** * @param Collection $tags * @param Collection & Selectable $visits + * @param Collection & Selectable $visitsCounts */ private function __construct( private string $longUrl, @@ -44,6 +46,7 @@ class ShortUrl extends AbstractEntity private Chronos $dateCreated = new Chronos(), private Collection $tags = new ArrayCollection(), private Collection & Selectable $visits = new ArrayCollection(), + private Collection & Selectable $visitsCounts = new ArrayCollection(), private ?Chronos $validSince = null, private ?Chronos $validUntil = null, private ?int $maxVisits = null, @@ -179,16 +182,16 @@ class ShortUrl extends AbstractEntity return $this->shortCode; } - public function getDateCreated(): Chronos - { - return $this->dateCreated; - } - public function getDomain(): ?Domain { return $this->domain; } + public function forwardQuery(): bool + { + return $this->forwardQuery; + } + public function reachedVisits(int $visitsAmount): bool { return count($this->visits) >= $visitsAmount; @@ -214,11 +217,6 @@ class ShortUrl extends AbstractEntity return $this; } - public function forwardQuery(): bool - { - return $this->forwardQuery; - } - /** * @throws ShortCodeCannotBeRegeneratedException */ @@ -258,7 +256,7 @@ class ShortUrl extends AbstractEntity return true; } - public function toArray(): array + public function toArray(?VisitsSummary $precalculatedSummary = null): array { return [ 'shortCode' => $this->shortCode, @@ -274,7 +272,7 @@ class ShortUrl extends AbstractEntity 'title' => $this->title, 'crawlable' => $this->crawlable, 'forwardQuery' => $this->forwardQuery, - 'visitsSummary' => VisitsSummary::fromTotalAndNonBots( + 'visitsSummary' => $precalculatedSummary ?? VisitsSummary::fromTotalAndNonBots( count($this->visits), count($this->visits->matching( Criteria::create()->where(Criteria::expr()->eq('potentialBot', false)), diff --git a/module/Core/src/ShortUrl/Model/OrderableField.php b/module/Core/src/ShortUrl/Model/OrderableField.php index 685f6f12..a4053742 100644 --- a/module/Core/src/ShortUrl/Model/OrderableField.php +++ b/module/Core/src/ShortUrl/Model/OrderableField.php @@ -2,8 +2,6 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model; -use function Shlinkio\Shlink\Core\ArrayUtils\contains; - enum OrderableField: string { case LONG_URL = 'longUrl'; @@ -12,17 +10,4 @@ enum OrderableField: string case TITLE = 'title'; case VISITS = 'visits'; case NON_BOT_VISITS = 'nonBotVisits'; - - public static function isBasicField(string $value): bool - { - return contains( - $value, - [self::LONG_URL->value, self::SHORT_CODE->value, self::DATE_CREATED->value, self::TITLE->value], - ); - } - - public static function isVisitsField(string $value): bool - { - return $value === self::VISITS->value || $value === self::NON_BOT_VISITS->value; - } } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php b/module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php new file mode 100644 index 00000000..50efaaee --- /dev/null +++ b/module/Core/src/ShortUrl/Model/ShortUrlWithVisitsSummary.php @@ -0,0 +1,36 @@ +shortUrl->toArray($this->visitsSummary); + } +} diff --git a/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php b/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php index 83ce8bd9..56f8e5a5 100644 --- a/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php +++ b/module/Core/src/ShortUrl/Paginator/Adapter/ShortUrlRepositoryAdapter.php @@ -11,13 +11,13 @@ use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlListRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; -class ShortUrlRepositoryAdapter implements AdapterInterface +readonly class ShortUrlRepositoryAdapter implements AdapterInterface { public function __construct( - private readonly ShortUrlListRepositoryInterface $repository, - private readonly ShortUrlsParams $params, - private readonly ?ApiKey $apiKey, - private readonly string $defaultDomain, + private ShortUrlListRepositoryInterface $repository, + private ShortUrlsParams $params, + private ?ApiKey $apiKey, + private string $defaultDomain, ) { } diff --git a/module/Core/src/ShortUrl/Persistence/ShortUrlsListFiltering.php b/module/Core/src/ShortUrl/Persistence/ShortUrlsListFiltering.php index db8b9a70..589947dd 100644 --- a/module/Core/src/ShortUrl/Persistence/ShortUrlsListFiltering.php +++ b/module/Core/src/ShortUrl/Persistence/ShortUrlsListFiltering.php @@ -13,9 +13,9 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class ShortUrlsListFiltering extends ShortUrlsCountFiltering { public function __construct( - public readonly ?int $limit, - public readonly ?int $offset, - public readonly Ordering $orderBy, + public readonly ?int $limit = null, + public readonly ?int $offset = null, + public readonly Ordering $orderBy = new Ordering(), ?string $searchTerm = null, array $tags = [], ?TagsMode $tagsMode = null, diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php b/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php index d6f7e421..790a3dbb 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlListRepository.php @@ -11,62 +11,66 @@ use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Common\Doctrine\Type\ChronosDateTimeType; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\OrderableField; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsCountFiltering; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; +use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; -use function array_column; +use function Shlinkio\Shlink\Core\ArrayUtils\map; use function sprintf; class ShortUrlListRepository extends EntitySpecificationRepository implements ShortUrlListRepositoryInterface { /** - * @return ShortUrl[] + * @return ShortUrlWithVisitsSummary[] */ public function findList(ShortUrlsListFiltering $filtering): array { + $buildVisitsSubQuery = function (string $alias, bool $excludingBots): string { + $vqb = $this->getEntityManager()->createQueryBuilder(); + $vqb->select('COALESCE(SUM(' . $alias . '.count), 0)') + ->from(ShortUrlVisitsCount::class, $alias) + ->where($vqb->expr()->eq($alias . '.shortUrl', 's')); + + if ($excludingBots) { + $vqb->andWhere($vqb->expr()->eq($alias . '.potentialBot', ':potentialBot')); + } + + return $vqb->getDQL(); + }; + $qb = $this->createListQueryBuilder($filtering); - $qb->select('DISTINCT s') + $qb->select( + 'DISTINCT s AS shortUrl', + '(' . $buildVisitsSubQuery('v', excludingBots: false) . ') AS ' . OrderableField::VISITS->value, + '(' . $buildVisitsSubQuery('v2', excludingBots: true) . ') AS ' . OrderableField::NON_BOT_VISITS->value, + ) ->setMaxResults($filtering->limit) - ->setFirstResult($filtering->offset); + ->setFirstResult($filtering->offset) + // This param is used in one of the sub-queries, but needs to set in the parent query + ->setParameter('potentialBot', false); $this->processOrderByForList($qb, $filtering); + /** @var array{shortUrl: ShortUrl, visits: string, nonBotVisits: string}[] $result */ $result = $qb->getQuery()->getResult(); - if (OrderableField::isVisitsField($filtering->orderBy->field ?? '')) { - return array_column($result, 0); - } - - return $result; + return map($result, static fn (array $s) => ShortUrlWithVisitsSummary::fromArray($s)); } private function processOrderByForList(QueryBuilder $qb, ShortUrlsListFiltering $filtering): void { - // With no explicit order by, fallback to dateCreated-DESC $fieldName = $filtering->orderBy->field; - if ($fieldName === null) { - $qb->orderBy('s.dateCreated', 'DESC'); - return; - } - $order = $filtering->orderBy->direction; - if (OrderableField::isBasicField($fieldName)) { - $qb->orderBy('s.' . $fieldName, $order); - } elseif (OrderableField::isVisitsField($fieldName)) { - $leftJoinConditions = [$qb->expr()->eq('v.shortUrl', 's')]; - if ($fieldName === OrderableField::NON_BOT_VISITS->value) { - $leftJoinConditions[] = $qb->expr()->eq('v.potentialBot', 'false'); - } - - // 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)') - ->leftJoin('s.visits', 'v', Join::WITH, $qb->expr()->andX(...$leftJoinConditions)) - ->groupBy('s') - ->orderBy('COUNT(DISTINCT v)', $order); - } + match (true) { + // With no explicit order by, fallback to dateCreated-DESC + $fieldName === null => $qb->orderBy('s.dateCreated', 'DESC'), + $fieldName === OrderableField::VISITS->value, + $fieldName === OrderableField::NON_BOT_VISITS->value => $qb->orderBy($fieldName, $order), + default => $qb->orderBy('s.' . $fieldName, $order), + }; } public function countList(ShortUrlsCountFiltering $filtering): int diff --git a/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php b/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php index 130e0db7..db3f8017 100644 --- a/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php +++ b/module/Core/src/ShortUrl/Repository/ShortUrlListRepositoryInterface.php @@ -4,14 +4,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl\Repository; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsCountFiltering; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; interface ShortUrlListRepositoryInterface { /** - * @return ShortUrl[] + * @return ShortUrlWithVisitsSummary[] */ public function findList(ShortUrlsListFiltering $filtering): array; diff --git a/module/Core/src/ShortUrl/ShortUrlListService.php b/module/Core/src/ShortUrl/ShortUrlListService.php index 60f56554..d86c4988 100644 --- a/module/Core/src/ShortUrl/ShortUrlListService.php +++ b/module/Core/src/ShortUrl/ShortUrlListService.php @@ -6,22 +6,22 @@ namespace Shlinkio\Shlink\Core\ShortUrl; use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlsParams; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Paginator\Adapter\ShortUrlRepositoryAdapter; use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlListRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; -class ShortUrlListService implements ShortUrlListServiceInterface +readonly class ShortUrlListService implements ShortUrlListServiceInterface { public function __construct( - private readonly ShortUrlListRepositoryInterface $repo, - private readonly UrlShortenerOptions $urlShortenerOptions, + private ShortUrlListRepositoryInterface $repo, + private UrlShortenerOptions $urlShortenerOptions, ) { } /** - * @return ShortUrl[]|Paginator + * @return ShortUrlWithVisitsSummary[]|Paginator */ public function listShortUrls(ShortUrlsParams $params, ?ApiKey $apiKey = null): Paginator { diff --git a/module/Core/src/ShortUrl/ShortUrlListServiceInterface.php b/module/Core/src/ShortUrl/ShortUrlListServiceInterface.php index ef7b31c2..ffbb1374 100644 --- a/module/Core/src/ShortUrl/ShortUrlListServiceInterface.php +++ b/module/Core/src/ShortUrl/ShortUrlListServiceInterface.php @@ -5,14 +5,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ShortUrl; use Shlinkio\Shlink\Common\Paginator\Paginator; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlsParams; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface ShortUrlListServiceInterface { /** - * @return ShortUrl[]|Paginator + * @return ShortUrlWithVisitsSummary[]|Paginator */ public function listShortUrls(ShortUrlsParams $params, ?ApiKey $apiKey = null): Paginator; } diff --git a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php index 09b9436b..413f5a69 100644 --- a/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php +++ b/module/Core/src/ShortUrl/Transformer/ShortUrlDataTransformer.php @@ -7,7 +7,11 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Transformer; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifierInterface; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; +/** + * @fixme Do not implement DataTransformerInterface, but a separate interface + */ readonly class ShortUrlDataTransformer implements DataTransformerInterface { public function __construct(private ShortUrlStringifierInterface $stringifier) @@ -15,13 +19,14 @@ readonly class ShortUrlDataTransformer implements DataTransformerInterface } /** - * @param ShortUrl $shortUrl + * @param ShortUrlWithVisitsSummary|ShortUrl $data */ - public function transform($shortUrl): array // phpcs:ignore + public function transform($data): array // phpcs:ignore { + $shortUrl = $data instanceof ShortUrlWithVisitsSummary ? $data->shortUrl : $data; return [ 'shortUrl' => $this->stringifier->stringify($shortUrl), - ...$shortUrl->toArray(), + ...$data->toArray(), ]; } } diff --git a/module/Core/src/Visit/Entity/ShortUrlVisitsCount.php b/module/Core/src/Visit/Entity/ShortUrlVisitsCount.php new file mode 100644 index 00000000..d513cfe8 --- /dev/null +++ b/module/Core/src/Visit/Entity/ShortUrlVisitsCount.php @@ -0,0 +1,19 @@ +entitiesToBeCreated = $args->getObjectManager()->getUnitOfWork()->getScheduledEntityInsertions(); + } + + /** + * @throws Exception + */ + public function postFlush(PostFlushEventArgs $args): void + { + $em = $args->getObjectManager(); + $entitiesToBeCreated = $this->entitiesToBeCreated; + + // Reset tracked entities until next flush operation + $this->entitiesToBeCreated = []; + + foreach ($entitiesToBeCreated as $entity) { + $this->trackVisitCount($em, $entity); + } + } + + /** + * @throws Exception + */ + private function trackVisitCount(EntityManagerInterface $em, object $entity): void + { + // This is not a visit + if (!$entity instanceof Visit) { + return; + } + $visit = $entity; + + // The short URL is not persisted yet or this is an orphan visit + $shortUrlId = $visit->shortUrl?->getId(); + if ($shortUrlId === null || $shortUrlId === '') { + return; + } + + $isBot = $visit->potentialBot; + $conn = $em->getConnection(); + $platformClass = $conn->getDatabasePlatform(); + + match ($platformClass::class) { + PostgreSQLPlatform::class => $this->incrementForPostgres($conn, $shortUrlId, $isBot), + SQLitePlatform::class, SQLServerPlatform::class => $this->incrementForOthers($conn, $shortUrlId, $isBot), + default => $this->incrementForMySQL($conn, $shortUrlId, $isBot), + }; + } + + /** + * @throws Exception + */ + private function incrementForMySQL(Connection $conn, string $shortUrlId, bool $potentialBot): void + { + $this->incrementWithPreparedStatement($conn, $shortUrlId, $potentialBot, <<incrementWithPreparedStatement($conn, $shortUrlId, $potentialBot, <<prepare($query); + $statement->bindValue('short_url_id', $shortUrlId); + $statement->bindValue('potential_bot', $potentialBot ? 1 : 0); + $statement->executeStatement(); + } + + /** + * @throws Exception + */ + private function incrementForOthers(Connection $conn, string $shortUrlId, bool $potentialBot): void + { + $slotId = rand(1, 100); + + // For engines without a specific UPSERT syntax, do a regular locked select followed by an insert or update + $qb = $conn->createQueryBuilder(); + $qb->select('id') + ->from('short_url_visits_counts') + ->where($qb->expr()->and( + $qb->expr()->eq('short_url_id', ':short_url_id'), + $qb->expr()->eq('potential_bot', ':potential_bot'), + $qb->expr()->eq('slot_id', ':slot_id'), + )) + ->setParameter('short_url_id', $shortUrlId) + ->setParameter('potential_bot', $potentialBot ? '1' : '0') + ->setParameter('slot_id', $slotId) + ->setMaxResults(1); + + if ($conn->getDatabasePlatform()::class === SQLServerPlatform::class) { + $qb->forUpdate(); + } + + $visitsCountId = $qb->executeQuery()->fetchOne(); + + $writeQb = ! $visitsCountId + ? $conn->createQueryBuilder() + ->insert('short_url_visits_counts') + ->values([ + 'short_url_id' => ':short_url_id', + 'potential_bot' => ':potential_bot', + 'slot_id' => ':slot_id', + ]) + ->setParameter('short_url_id', $shortUrlId) + ->setParameter('potential_bot', $potentialBot ? '1' : '0') + ->setParameter('slot_id', $slotId) + : $conn->createQueryBuilder() + ->update('short_url_visits_counts') + ->set('count', 'count + 1') + ->where($qb->expr()->eq('id', ':visits_count_id')) + ->setParameter('visits_count_id', $visitsCountId); + + $writeQb->executeStatement(); + } +} diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index 9e4b88df..f2e26493 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -12,12 +12,12 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\Visitor; -class VisitsTracker implements VisitsTrackerInterface +readonly class VisitsTracker implements VisitsTrackerInterface { public function __construct( - private readonly ORM\EntityManagerInterface $em, - private readonly EventDispatcherInterface $eventDispatcher, - private readonly TrackingOptions $options, + private ORM\EntityManagerInterface $em, + private EventDispatcherInterface $eventDispatcher, + private TrackingOptions $options, ) { } @@ -72,8 +72,13 @@ class VisitsTracker implements VisitsTrackerInterface } $visit = $createVisit($visitor->normalizeForTrackingOptions($this->options)); - $this->em->persist($visit); - $this->em->flush(); + + // Wrap persisting and flushing the visit in a transaction, so that the ShortUrlVisitsCountTracker performs + // changes inside that very same transaction atomically + $this->em->wrapInTransaction(function () use ($visit): void { + $this->em->persist($visit); + $this->em->flush(); + }); $this->eventDispatcher->dispatch(new UrlVisited($visit->getId(), $visitor->remoteAddress)); } diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php index 315491d8..95924956 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Core\Model\Ordering; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\OrderableField; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; +use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlWithVisitsSummary; use Shlinkio\Shlink\Core\ShortUrl\Model\TagsMode; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsCountFiltering; use Shlinkio\Shlink\Core\ShortUrl\Persistence\ShortUrlsListFiltering; @@ -25,6 +26,7 @@ use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; use function array_map; use function count; use function range; +use function Shlinkio\Shlink\Core\ArrayUtils\map; class ShortUrlListRepositoryTest extends DatabaseTestCase { @@ -85,60 +87,54 @@ class ShortUrlListRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $result = $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::none(), 'foo', ['bar']), - ); + $result = $this->repo->findList(new ShortUrlsListFiltering(searchTerm: 'foo', tags: ['bar'])); self::assertCount(1, $result); self::assertEquals(1, $this->repo->countList(new ShortUrlsCountFiltering('foo', ['bar']))); - self::assertSame($foo, $result[0]); + self::assertSame($foo, $result[0]->shortUrl); // Assert searched text also applies to tags - $result = $this->repo->findList(new ShortUrlsListFiltering(null, null, Ordering::none(), 'bar')); + $result = $this->repo->findList(new ShortUrlsListFiltering(searchTerm: 'bar')); self::assertCount(2, $result); self::assertEquals(2, $this->repo->countList(new ShortUrlsCountFiltering('bar'))); - self::assertContains($foo, $result); + self::assertContains($foo, map($result, fn (ShortUrlWithVisitsSummary $s) => $s->shortUrl)); - $result = $this->repo->findList(new ShortUrlsListFiltering(null, null, Ordering::none())); + $result = $this->repo->findList(new ShortUrlsListFiltering()); self::assertCount(3, $result); - $result = $this->repo->findList(new ShortUrlsListFiltering(2, null, Ordering::none())); + $result = $this->repo->findList(new ShortUrlsListFiltering(limit: 2)); self::assertCount(2, $result); - $result = $this->repo->findList(new ShortUrlsListFiltering(2, 1, Ordering::none())); + $result = $this->repo->findList(new ShortUrlsListFiltering(limit: 2, offset: 1)); self::assertCount(2, $result); - self::assertCount(1, $this->repo->findList(new ShortUrlsListFiltering(2, 2, Ordering::none()))); + self::assertCount(1, $this->repo->findList(new ShortUrlsListFiltering(limit: 2, offset: 2))); - $result = $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::fromFieldDesc(OrderableField::VISITS->value)), - ); - self::assertCount(3, $result); - self::assertSame($bar, $result[0]); - - $result = $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::fromFieldDesc(OrderableField::NON_BOT_VISITS->value)), - ); - self::assertCount(3, $result); - self::assertSame($foo2, $result[0]); - - $result = $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::none(), null, [], null, DateRange::until( - Chronos::now()->subDays(2), - )), - ); - self::assertCount(1, $result); - self::assertEquals(1, $this->repo->countList(new ShortUrlsCountFiltering(null, [], null, DateRange::until( - Chronos::now()->subDays(2), - )))); - self::assertSame($foo2, $result[0]); - - self::assertCount(2, $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::none(), null, [], null, DateRange::since( - Chronos::now()->subDays(2), - )), + $result = $this->repo->findList(new ShortUrlsListFiltering( + orderBy: Ordering::fromFieldDesc(OrderableField::VISITS->value), )); + self::assertCount(3, $result); + self::assertSame($bar, $result[0]->shortUrl); + + $result = $this->repo->findList(new ShortUrlsListFiltering( + orderBy: Ordering::fromFieldDesc(OrderableField::NON_BOT_VISITS->value), + )); + self::assertCount(3, $result); + self::assertSame($foo2, $result[0]->shortUrl); + + $result = $this->repo->findList(new ShortUrlsListFiltering( + dateRange: DateRange::until(Chronos::now()->subDays(2)), + )); + self::assertCount(1, $result); + self::assertEquals(1, $this->repo->countList(new ShortUrlsCountFiltering( + dateRange: DateRange::until(Chronos::now()->subDays(2)), + ))); + self::assertSame($foo2, $result[0]->shortUrl); + + self::assertCount(2, $this->repo->findList(new ShortUrlsListFiltering( + dateRange: DateRange::since(Chronos::now()->subDays(2)), + ))); self::assertEquals(2, $this->repo->countList( - new ShortUrlsCountFiltering(null, [], null, DateRange::since(Chronos::now()->subDays(2))), + new ShortUrlsCountFiltering(dateRange: DateRange::since(Chronos::now()->subDays(2))), )); } @@ -152,15 +148,13 @@ class ShortUrlListRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - $result = $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::fromFieldAsc('longUrl')), - ); + $result = $this->repo->findList(new ShortUrlsListFiltering(orderBy: Ordering::fromFieldAsc('longUrl'))); self::assertCount(count($urls), $result); - self::assertEquals('https://a', $result[0]->getLongUrl()); - self::assertEquals('https://b', $result[1]->getLongUrl()); - self::assertEquals('https://c', $result[2]->getLongUrl()); - self::assertEquals('https://z', $result[3]->getLongUrl()); + self::assertEquals('https://a', $result[0]->shortUrl->getLongUrl()); + self::assertEquals('https://b', $result[1]->shortUrl->getLongUrl()); + self::assertEquals('https://c', $result[2]->shortUrl->getLongUrl()); + self::assertEquals('https://z', $result[3]->shortUrl->getLongUrl()); } #[Test] @@ -194,81 +188,55 @@ class ShortUrlListRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertCount(5, $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::none(), null, ['foo', 'bar']), - )); + self::assertCount(5, $this->repo->findList(new ShortUrlsListFiltering(tags: ['foo', 'bar']))); self::assertCount(5, $this->repo->findList(new ShortUrlsListFiltering( - null, - null, - Ordering::none(), - null, - ['foo', 'bar'], - TagsMode::ANY, + tags: ['foo', 'bar'], + tagsMode: TagsMode::ANY, ))); self::assertCount(1, $this->repo->findList(new ShortUrlsListFiltering( - null, - null, - Ordering::none(), - null, - ['foo', 'bar'], - TagsMode::ALL, + tags: ['foo', 'bar'], + tagsMode: TagsMode::ALL, ))); - self::assertEquals(5, $this->repo->countList(new ShortUrlsCountFiltering(null, ['foo', 'bar']))); - self::assertEquals(5, $this->repo->countList(new ShortUrlsCountFiltering(null, ['foo', 'bar'], TagsMode::ANY))); - self::assertEquals(1, $this->repo->countList(new ShortUrlsCountFiltering(null, ['foo', 'bar'], TagsMode::ALL))); - - self::assertCount(4, $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::none(), null, ['bar', 'baz']), + self::assertEquals(5, $this->repo->countList(new ShortUrlsCountFiltering(tags: ['foo', 'bar']))); + self::assertEquals(5, $this->repo->countList( + new ShortUrlsCountFiltering(tags: ['foo', 'bar'], tagsMode: TagsMode::ANY), )); + self::assertEquals(1, $this->repo->countList( + new ShortUrlsCountFiltering(tags: ['foo', 'bar'], tagsMode: TagsMode::ALL), + )); + + self::assertCount(4, $this->repo->findList(new ShortUrlsListFiltering(tags: ['bar', 'baz']))); self::assertCount(4, $this->repo->findList(new ShortUrlsListFiltering( - null, - null, - Ordering::none(), - null, - ['bar', 'baz'], - TagsMode::ANY, + tags: ['bar', 'baz'], + tagsMode: TagsMode::ANY, ))); self::assertCount(2, $this->repo->findList(new ShortUrlsListFiltering( - null, - null, - Ordering::none(), - null, - ['bar', 'baz'], - TagsMode::ALL, + tags: ['bar', 'baz'], + tagsMode: TagsMode::ALL, ))); - self::assertEquals(4, $this->repo->countList(new ShortUrlsCountFiltering(null, ['bar', 'baz']))); + self::assertEquals(4, $this->repo->countList(new ShortUrlsCountFiltering(tags: ['bar', 'baz']))); self::assertEquals(4, $this->repo->countList( - new ShortUrlsCountFiltering(null, ['bar', 'baz'], TagsMode::ANY), + new ShortUrlsCountFiltering(tags: ['bar', 'baz'], tagsMode: TagsMode::ANY), )); self::assertEquals(2, $this->repo->countList( - new ShortUrlsCountFiltering(null, ['bar', 'baz'], TagsMode::ALL), + new ShortUrlsCountFiltering(tags: ['bar', 'baz'], tagsMode: TagsMode::ALL), )); - self::assertCount(5, $this->repo->findList( - new ShortUrlsListFiltering(null, null, Ordering::none(), null, ['foo', 'bar', 'baz']), - )); + self::assertCount(5, $this->repo->findList(new ShortUrlsListFiltering(tags: ['foo', 'bar', 'baz']))); self::assertCount(5, $this->repo->findList(new ShortUrlsListFiltering( - null, - null, - Ordering::none(), - null, - ['foo', 'bar', 'baz'], - TagsMode::ANY, + tags: ['foo', 'bar', 'baz'], + tagsMode: TagsMode::ANY, ))); self::assertCount(0, $this->repo->findList(new ShortUrlsListFiltering( - null, - null, - Ordering::none(), - null, - ['foo', 'bar', 'baz'], - TagsMode::ALL, + tags: ['foo', 'bar', 'baz'], + tagsMode: TagsMode::ALL, ))); - self::assertEquals(5, $this->repo->countList(new ShortUrlsCountFiltering(null, ['foo', 'bar', 'baz']))); + self::assertEquals(5, $this->repo->countList(new ShortUrlsCountFiltering(tags: ['foo', 'bar', 'baz']))); self::assertEquals(5, $this->repo->countList( - new ShortUrlsCountFiltering(null, ['foo', 'bar', 'baz'], TagsMode::ANY), + new ShortUrlsCountFiltering(tags: ['foo', 'bar', 'baz'], tagsMode: TagsMode::ANY), )); self::assertEquals(0, $this->repo->countList( - new ShortUrlsCountFiltering(null, ['foo', 'bar', 'baz'], TagsMode::ALL), + new ShortUrlsCountFiltering(tags: ['foo', 'bar', 'baz'], tagsMode: TagsMode::ALL), )); } @@ -294,9 +262,6 @@ class ShortUrlListRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); $buildFiltering = static fn (string $searchTerm) => new ShortUrlsListFiltering( - null, - null, - Ordering::none(), searchTerm: $searchTerm, defaultDomain: 'deFaulT-domain.com', ); @@ -339,9 +304,6 @@ class ShortUrlListRepositoryTest extends DatabaseTestCase $filtering = static fn (bool $excludeMaxVisitsReached, bool $excludePastValidUntil) => new ShortUrlsListFiltering( - null, - null, - Ordering::none(), excludeMaxVisitsReached: $excludeMaxVisitsReached, excludePastValidUntil: $excludePastValidUntil, ); diff --git a/module/Core/test-db/Visit/Listener/ShortUrlVisitsCountTrackerTest.php b/module/Core/test-db/Visit/Listener/ShortUrlVisitsCountTrackerTest.php new file mode 100644 index 00000000..29f5d5d1 --- /dev/null +++ b/module/Core/test-db/Visit/Listener/ShortUrlVisitsCountTrackerTest.php @@ -0,0 +1,76 @@ +repo = $this->getEntityManager()->getRepository(ShortUrlVisitsCount::class); + } + + #[Test] + public function createsNewEntriesWhenNoneExist(): void + { + $shortUrl = ShortUrl::createFake(); + $this->getEntityManager()->persist($shortUrl); + + $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); + $this->getEntityManager()->persist($visit); + $this->getEntityManager()->flush(); + + /** @var ShortUrlVisitsCount[] $result */ + $result = $this->repo->findBy(['shortUrl' => $shortUrl]); + + self::assertCount(1, $result); + self::assertEquals('1', $result[0]->count); + self::assertGreaterThanOrEqual(0, $result[0]->slotId); + self::assertLessThanOrEqual(100, $result[0]->slotId); + } + + #[Test] + public function editsExistingEntriesWhenAlreadyExist(): void + { + $shortUrl = ShortUrl::createFake(); + $this->getEntityManager()->persist($shortUrl); + + for ($i = 0; $i <= 100; $i++) { + $this->getEntityManager()->persist(new ShortUrlVisitsCount($shortUrl, slotId: $i)); + } + $this->getEntityManager()->flush(); + + $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); + $this->getEntityManager()->persist($visit); + $this->getEntityManager()->flush(); + + // Clear entity manager to force it to get fresh data from the database + // This is needed because the tracker inserts natively, bypassing the entity manager + $this->getEntityManager()->clear(); + + /** @var ShortUrlVisitsCount[] $result */ + $result = $this->repo->findBy(['shortUrl' => $shortUrl]); + $itemsWithCountBiggerThanOnce = array_values(array_filter( + $result, + static fn (ShortUrlVisitsCount $item) => ((int) $item->count) > 1, + )); + + self::assertCount(101, $result); + self::assertCount(1, $itemsWithCountBiggerThanOnce); + self::assertEquals('2', $itemsWithCountBiggerThanOnce[0]->count); + } +} diff --git a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php index 032c3263..2dd1c0c7 100644 --- a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php +++ b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\EventDispatcher; +use Cake\Chronos\Chronos; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; @@ -22,14 +23,23 @@ use Shlinkio\Shlink\Core\Visit\Model\VisitType; class PublishingUpdatesGeneratorTest extends TestCase { private PublishingUpdatesGenerator $generator; + private Chronos $now; protected function setUp(): void { + $this->now = Chronos::now(); + Chronos::setTestNow($this->now); + $this->generator = new PublishingUpdatesGenerator( new ShortUrlDataTransformer(new ShortUrlStringifier([])), ); } + protected function tearDown(): void + { + Chronos::setTestNow(); + } + #[Test, DataProvider('provideMethod')] public function visitIsProperlySerializedIntoUpdate(string $method, string $expectedTopic, ?string $title): void { @@ -49,7 +59,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => 'http:/' . $shortUrl->getShortCode(), 'longUrl' => 'https://longUrl', - 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), + 'dateCreated' => $this->now->toAtomString(), 'tags' => [], 'meta' => [ 'validSince' => null, @@ -123,7 +133,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'shortCode' => $shortUrl->getShortCode(), 'shortUrl' => 'http:/' . $shortUrl->getShortCode(), 'longUrl' => 'https://longUrl', - 'dateCreated' => $shortUrl->getDateCreated()->toAtomString(), + 'dateCreated' => $this->now->toAtomString(), 'tags' => [], 'meta' => [ 'validSince' => null, diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index 32cd10a8..d6cbf4bf 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -25,6 +25,8 @@ class VisitsTrackerTest extends TestCase protected function setUp(): void { $this->em = $this->createMock(EntityManager::class); + $this->em->method('wrapInTransaction')->willReturnCallback(fn (callable $callback) => $callback()); + $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class); } diff --git a/phpunit-db.xml b/phpunit-db.xml index b883d8ca..3c5ffb64 100644 --- a/phpunit-db.xml +++ b/phpunit-db.xml @@ -20,6 +20,7 @@ ./module/*/src/Spec ./module/*/src/**/Spec ./module/*/src/**/**/Spec + ./module/Core/src/Visit/Listener/ShortUrlVisitsCountTracker.php diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 9c85d2c4..4364c82c 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -30,6 +30,7 @@ ./module/Core/src/Spec ./module/Core/src/**/Spec ./module/Core/src/**/**/Spec + ./module/Core/src/Visit/Listener/ShortUrlVisitsCountTracker.php