From b50547d868004e3bedc9ad4bd87deffe8b331782 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 31 Mar 2024 13:18:44 +0200 Subject: [PATCH 1/2] Create new orphan_visits_counts table --- .../Core/migrations/Version20240331111103.php | 56 +++++++++++++++++++ .../Core/migrations/Version20240331111447.php | 45 +++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 module/Core/migrations/Version20240331111103.php create mode 100644 module/Core/migrations/Version20240331111447.php diff --git a/module/Core/migrations/Version20240331111103.php b/module/Core/migrations/Version20240331111103.php new file mode 100644 index 000000000..ec0a50877 --- /dev/null +++ b/module/Core/migrations/Version20240331111103.php @@ -0,0 +1,56 @@ +skipIf($schema->hasTable('orphan_visits_counts')); + + $table = $schema->createTable('orphan_visits_counts'); + $table->addColumn('id', Types::BIGINT, [ + 'unsigned' => true, + 'autoincrement' => true, + 'notnull' => true, + ]); + $table->setPrimaryKey(['id']); + + $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(['potential_bot', 'slot_id'], 'UQ_slot'); + } + + public function down(Schema $schema): void + { + $this->skipIf(! $schema->hasTable('orphan_visits_counts')); + $schema->dropTable('orphan_visits_counts'); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/migrations/Version20240331111447.php b/module/Core/migrations/Version20240331111447.php new file mode 100644 index 000000000..333656c47 --- /dev/null +++ b/module/Core/migrations/Version20240331111447.php @@ -0,0 +1,45 @@ +connection->createQueryBuilder(); + $visitsQb->select('COUNT(id)') + ->from('visits') + ->where($visitsQb->expr()->isNull('short_url_id')) + ->andWhere($visitsQb->expr()->eq('potential_bot', ':potential_bot')); + + $botsCount = $visitsQb->setParameter('potential_bot', '1')->executeQuery()->fetchOne(); + $nonBotsCount = $visitsQb->setParameter('potential_bot', '0')->executeQuery()->fetchOne(); + + if ($botsCount > 0) { + $this->insertCount($botsCount, potentialBot: true); + } + if ($nonBotsCount > 0) { + $this->insertCount($nonBotsCount, potentialBot: false); + } + } + + private function insertCount(string|int $count, bool $potentialBot): void + { + $this->connection->createQueryBuilder() + ->insert('orphan_visits_counts') + ->values([ + 'count' => ':count', + 'potential_bot' => ':potential_bot', + ]) + ->setParameters([ + 'count' => $count, + 'potential_bot' => $potentialBot ? '1' : '0', + ]) + ->executeStatement(); + } +} From d090260b175601fc194a4b0c30775b34c179326e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 1 Apr 2024 10:22:51 +0200 Subject: [PATCH 2/2] Track orphan visits counts --- config/autoload/entity-manager.global.php | 5 +- module/Core/config/dependencies.config.php | 1 + ...nk.Core.Visit.Entity.OrphanVisitsCount.php | 41 +++++ .../src/Visit/Entity/OrphanVisitsCount.php | 17 ++ .../Listener/OrphanVisitsCountTracker.php | 145 ++++++++++++++++++ .../OrphanVisitsCountRepository.php | 31 ++++ .../OrphanVisitsCountRepositoryInterface.php | 12 ++ module/Core/src/Visit/VisitsStatsHelper.php | 13 +- .../Listener/OrphanVisitsCountTrackerTest.php | 69 +++++++++ .../Visit/Repository/VisitRepositoryTest.php | 16 +- .../Core/test/Visit/VisitsStatsHelperTest.php | 8 +- phpunit-db.xml | 2 +- phpunit.xml.dist | 2 +- 13 files changed, 349 insertions(+), 13 deletions(-) create mode 100644 module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.OrphanVisitsCount.php create mode 100644 module/Core/src/Visit/Entity/OrphanVisitsCount.php create mode 100644 module/Core/src/Visit/Listener/OrphanVisitsCountTracker.php create mode 100644 module/Core/src/Visit/Repository/OrphanVisitsCountRepository.php create mode 100644 module/Core/src/Visit/Repository/OrphanVisitsCountRepositoryInterface.php create mode 100644 module/Core/test-db/Visit/Listener/OrphanVisitsCountTrackerTest.php diff --git a/config/autoload/entity-manager.global.php b/config/autoload/entity-manager.global.php index 842339157..c3aa66327 100644 --- a/config/autoload/entity-manager.global.php +++ b/config/autoload/entity-manager.global.php @@ -5,6 +5,7 @@ use Doctrine\ORM\Events; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Core\Config\EnvVars; +use Shlinkio\Shlink\Core\Visit\Listener\OrphanVisitsCountTracker; use Shlinkio\Shlink\Core\Visit\Listener\ShortUrlVisitsCountTracker; use function Shlinkio\Shlink\Core\ArrayUtils\contains; @@ -63,8 +64,8 @@ 'load_mappings_using_functional_style' => true, 'default_repository_classname' => EntitySpecificationRepository::class, 'listeners' => [ - Events::onFlush => [ShortUrlVisitsCountTracker::class], - Events::postFlush => [ShortUrlVisitsCountTracker::class], + Events::onFlush => [ShortUrlVisitsCountTracker::class, OrphanVisitsCountTracker::class], + Events::postFlush => [ShortUrlVisitsCountTracker::class, OrphanVisitsCountTracker::class], ], ], 'connection' => $resolveConnection(), diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 951c7b527..5437555b3 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -77,6 +77,7 @@ Visit\Entity\Visit::class, ], Visit\Listener\ShortUrlVisitsCountTracker::class => InvokableFactory::class, + Visit\Listener\OrphanVisitsCountTracker::class => InvokableFactory::class, Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class, Util\RedirectResponseHelper::class => ConfigAbstractFactory::class, diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.OrphanVisitsCount.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.OrphanVisitsCount.php new file mode 100644 index 000000000..dbdf9e9ab --- /dev/null +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.OrphanVisitsCount.php @@ -0,0 +1,41 @@ +setTable(determineTableName('orphan_visits_counts', $emConfig)) + ->setCustomRepositoryClass(Visit\Repository\OrphanVisitsCountRepository::class); + + $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->addUniqueConstraint(['potential_bot', 'slot_id'], 'UQ_slot'); +}; diff --git a/module/Core/src/Visit/Entity/OrphanVisitsCount.php b/module/Core/src/Visit/Entity/OrphanVisitsCount.php new file mode 100644 index 000000000..8199b1744 --- /dev/null +++ b/module/Core/src/Visit/Entity/OrphanVisitsCount.php @@ -0,0 +1,17 @@ +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 an orphan visit + if (! $entity instanceof Visit || ! $entity->isOrphan()) { + return; + } + $visit = $entity; + + $isBot = $visit->potentialBot; + $conn = $em->getConnection(); + $platformClass = $conn->getDatabasePlatform(); + + match ($platformClass::class) { + PostgreSQLPlatform::class => $this->incrementForPostgres($conn, $isBot), + SQLitePlatform::class, SQLServerPlatform::class => $this->incrementForOthers($conn, $isBot), + default => $this->incrementForMySQL($conn, $isBot), + }; + } + + /** + * @throws Exception + */ + private function incrementForMySQL(Connection $conn, bool $potentialBot): void + { + $this->incrementWithPreparedStatement($conn, $potentialBot, <<incrementWithPreparedStatement($conn, $potentialBot, <<prepare($query); + $statement->bindValue('potential_bot', $potentialBot ? 1 : 0); + $statement->executeStatement(); + } + + /** + * @throws Exception + */ + private function incrementForOthers(Connection $conn, 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('orphan_visits_counts') + ->where($qb->expr()->and( + $qb->expr()->eq('potential_bot', ':potential_bot'), + $qb->expr()->eq('slot_id', ':slot_id'), + )) + ->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('orphan_visits_counts') + ->values([ + 'potential_bot' => ':potential_bot', + 'slot_id' => ':slot_id', + ]) + ->setParameter('potential_bot', $potentialBot ? '1' : '0') + ->setParameter('slot_id', $slotId) + : $conn->createQueryBuilder() + ->update('orphan_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/Repository/OrphanVisitsCountRepository.php b/module/Core/src/Visit/Repository/OrphanVisitsCountRepository.php new file mode 100644 index 000000000..d5e7670cf --- /dev/null +++ b/module/Core/src/Visit/Repository/OrphanVisitsCountRepository.php @@ -0,0 +1,31 @@ +apiKey?->hasRole(Role::NO_ORPHAN_VISITS)) { + return 0; + } + + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('COALESCE(SUM(vc.count), 0)') + ->from(OrphanVisitsCount::class, 'vc'); + + if ($filtering->excludeBots) { + $qb->andWhere($qb->expr()->eq('vc.potentialBot', ':potentialBot')) + ->setParameter('potentialBot', false); + } + + return (int) $qb->getQuery()->getSingleScalarResult(); + } +} diff --git a/module/Core/src/Visit/Repository/OrphanVisitsCountRepositoryInterface.php b/module/Core/src/Visit/Repository/OrphanVisitsCountRepositoryInterface.php new file mode 100644 index 000000000..adbbe0764 --- /dev/null +++ b/module/Core/src/Visit/Repository/OrphanVisitsCountRepositoryInterface.php @@ -0,0 +1,12 @@ +em->getRepository(Visit::class); + /** @var OrphanVisitsCountRepository $orphanVisitsCountRepo */ + $orphanVisitsCountRepo = $this->em->getRepository(OrphanVisitsCount::class); /** @var ShortUrlVisitsCountRepository $visitsCountRepo */ $visitsCountRepo = $this->em->getRepository(ShortUrlVisitsCount::class); return new VisitsStats( nonOrphanVisitsTotal: $visitsCountRepo->countNonOrphanVisits(new VisitsCountFiltering(apiKey: $apiKey)), - orphanVisitsTotal: $visitsRepo->countOrphanVisits(new OrphanVisitsCountFiltering(apiKey: $apiKey)), + orphanVisitsTotal: $orphanVisitsCountRepo->countOrphanVisits( + new OrphanVisitsCountFiltering(apiKey: $apiKey), + ), nonOrphanVisitsNonBots: $visitsCountRepo->countNonOrphanVisits( new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey), ), - orphanVisitsNonBots: $visitsRepo->countOrphanVisits( + orphanVisitsNonBots: $orphanVisitsCountRepo->countOrphanVisits( new OrphanVisitsCountFiltering(excludeBots: true, apiKey: $apiKey), ), ); diff --git a/module/Core/test-db/Visit/Listener/OrphanVisitsCountTrackerTest.php b/module/Core/test-db/Visit/Listener/OrphanVisitsCountTrackerTest.php new file mode 100644 index 000000000..20302c755 --- /dev/null +++ b/module/Core/test-db/Visit/Listener/OrphanVisitsCountTrackerTest.php @@ -0,0 +1,69 @@ +repo = $this->getEntityManager()->getRepository(OrphanVisitsCount::class); + } + + #[Test] + public function createsNewEntriesWhenNoneExist(): void + { + $visit = Visit::forBasePath(Visitor::emptyInstance()); + $this->getEntityManager()->persist($visit); + $this->getEntityManager()->flush(); + + /** @var OrphanVisitsCount[] $result */ + $result = $this->repo->findAll(); + + 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 + { + for ($i = 0; $i <= 100; $i++) { + $this->getEntityManager()->persist(new OrphanVisitsCount(slotId: $i)); + } + $this->getEntityManager()->flush(); + + $visit = Visit::forRegularNotFound(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 OrphanVisitsCount[] $result */ + $result = $this->repo->findAll(); + $itemsWithCountBiggerThanOnce = array_values(array_filter( + $result, + static fn (OrphanVisitsCount $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-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index d14e70789..1506dd1a6 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Core\ShortUrl\Resolver\PersistenceShortUrlRelationResolver; +use Shlinkio\Shlink\Core\Visit\Entity\OrphanVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\OrphanVisitType; @@ -22,6 +23,7 @@ use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; +use Shlinkio\Shlink\Core\Visit\Repository\OrphanVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\ShortUrlVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; @@ -39,14 +41,18 @@ class VisitRepositoryTest extends DatabaseTestCase { private VisitRepository $repo; private ShortUrlVisitsCountRepository $countRepo; + private OrphanVisitsCountRepository $orphanCountRepo; private PersistenceShortUrlRelationResolver $relationResolver; protected function setUp(): void { $this->repo = $this->getEntityManager()->getRepository(Visit::class); - // Testing the ShortUrlVisitsCountRepository in this very same test, helps checking the fact that results should + + // Testing the visits count repositories in this very same test, helps checking the fact that results should // match what VisitRepository returns $this->countRepo = $this->getEntityManager()->getRepository(ShortUrlVisitsCount::class); + $this->orphanCountRepo = $this->getEntityManager()->getRepository(OrphanVisitsCount::class); + $this->relationResolver = new PersistenceShortUrlRelationResolver($this->getEntityManager()); } @@ -326,6 +332,9 @@ public function countVisitsReturnsExpectedResultBasedOnApiKey(): void self::assertEquals(0, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering( apiKey: $noOrphanVisitsApiKey, ))); + self::assertEquals(0, $this->orphanCountRepo->countOrphanVisits(new OrphanVisitsCountFiltering( + apiKey: $noOrphanVisitsApiKey, + ))); self::assertEquals(4, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::since( Chronos::parse('2016-01-05')->startOfDay(), )))); @@ -342,7 +351,11 @@ public function countVisitsReturnsExpectedResultBasedOnApiKey(): void new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey2), )); self::assertEquals(4, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering())); + self::assertEquals(4, $this->orphanCountRepo->countOrphanVisits(new OrphanVisitsCountFiltering())); self::assertEquals(3, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering(excludeBots: true))); + self::assertEquals(3, $this->orphanCountRepo->countOrphanVisits( + new OrphanVisitsCountFiltering(excludeBots: true), + )); } #[Test] @@ -432,6 +445,7 @@ public function countOrphanVisitsReturnsExpectedResult(): void $this->getEntityManager()->flush(); self::assertEquals(18, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering())); + self::assertEquals(18, $this->orphanCountRepo->countOrphanVisits(new OrphanVisitsCountFiltering())); self::assertEquals(18, $this->repo->countOrphanVisits(new OrphanVisitsCountFiltering(DateRange::allTime()))); self::assertEquals(9, $this->repo->countOrphanVisits( new OrphanVisitsCountFiltering(DateRange::since(Chronos::parse('2020-01-04'))), diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index e109852ab..c1aa07475 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -22,6 +22,7 @@ use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Repository\TagRepository; +use Shlinkio\Shlink\Core\Visit\Entity\OrphanVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\OrphanVisitsParams; @@ -32,6 +33,7 @@ use Shlinkio\Shlink\Core\Visit\Persistence\OrphanVisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; +use Shlinkio\Shlink\Core\Visit\Repository\OrphanVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\ShortUrlVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepository; use Shlinkio\Shlink\Core\Visit\VisitsStatsHelper; @@ -68,13 +70,13 @@ function (VisitsCountFiltering $options) use ($expectedCount, $apiKey, &$callCou }, ); - $visitsRepo = $this->createMock(VisitRepository::class); - $visitsRepo->expects($this->exactly(2))->method('countOrphanVisits')->with( + $orphanVisitsCountRepo = $this->createMock(OrphanVisitsCountRepository::class); + $orphanVisitsCountRepo->expects($this->exactly(2))->method('countOrphanVisits')->with( $this->isInstanceOf(VisitsCountFiltering::class), )->willReturn($expectedCount); $this->em->expects($this->exactly(2))->method('getRepository')->willReturnMap([ - [Visit::class, $visitsRepo], + [OrphanVisitsCount::class, $orphanVisitsCountRepo], [ShortUrlVisitsCount::class, $visitsCountRepo], ]); diff --git a/phpunit-db.xml b/phpunit-db.xml index 3c5ffb64c..17e748b87 100644 --- a/phpunit-db.xml +++ b/phpunit-db.xml @@ -20,7 +20,7 @@ ./module/*/src/Spec ./module/*/src/**/Spec ./module/*/src/**/**/Spec - ./module/Core/src/Visit/Listener/ShortUrlVisitsCountTracker.php + ./module/Core/src/Visit/Listener/*.php diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 4364c82cd..30f2286db 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -30,7 +30,7 @@ ./module/Core/src/Spec ./module/Core/src/**/Spec ./module/Core/src/**/**/Spec - ./module/Core/src/Visit/Listener/ShortUrlVisitsCountTracker.php + ./module/Core/src/Visit/Listener/*.php