diff --git a/CHANGELOG.md b/CHANGELOG.md index 8439c1c9..d0a75254 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Added * [#1148](https://github.com/shlinkio/shlink/issues/1148) Add support to delete short URL visits. - This can be done via `DELETE /short-urls/{shortCode}/visits` REST endpoint or via `short-url:delete-visits` console command. + This can be done via `DELETE /short-urls/{shortCode}/visits` REST endpoint or via `short-url:visits-delete` console command. + + The CLI command includes a warning and requires the user to confirm before proceeding. + +* [#1681](https://github.com/shlinkio/shlink/issues/1681) Add support to delete orphan visits. + + This can be done via `DELETE /visits/orphan` REST endpoint or via `visit:orphan-delete` console command. The CLI command includes a warning and requires the user to confirm before proceeding. diff --git a/composer.json b/composer.json index b47d0dfc..c9d9757d 100644 --- a/composer.json +++ b/composer.json @@ -65,7 +65,7 @@ "require-dev": { "cebe/php-openapi": "^1.7", "devster/ubench": "^2.1", - "infection/infection": "^0.26.19", + "infection/infection": "^0.27", "openswoole/ide-helper": "~22.0.0", "phpstan/phpstan": "^1.9", "phpstan/phpstan-doctrine": "^1.3", diff --git a/config/autoload/routes.config.php b/config/autoload/routes.config.php index 93464519..ea305d86 100644 --- a/config/autoload/routes.config.php +++ b/config/autoload/routes.config.php @@ -38,6 +38,7 @@ return (static function (): array { Action\Visit\DomainVisitsAction::getRouteDef(), Action\Visit\GlobalVisitsAction::getRouteDef(), Action\Visit\OrphanVisitsAction::getRouteDef(), + Action\Visit\DeleteOrphanVisitsAction::getRouteDef(), Action\Visit\NonOrphanVisitsAction::getRouteDef(), // Short URLs diff --git a/docs/swagger/paths/v2_visits_orphan.json b/docs/swagger/paths/v2_visits_orphan.json index b10ac37f..fe799934 100644 --- a/docs/swagger/paths/v2_visits_orphan.json +++ b/docs/swagger/paths/v2_visits_orphan.json @@ -148,5 +148,55 @@ } } } + }, + + "delete": { + "operationId": "deleteOrphanVisits", + "tags": [ + "Visits" + ], + "summary": "Delete orphan visits", + "description": "Delete all visits to invalid short URLs, the base URL or any other 404.", + "parameters": [ + { + "$ref": "../parameters/version.json" + } + ], + "security": [ + { + "ApiKey": [] + } + ], + "responses": { + "200": { + "description": "Deleted visits", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "deletedVisits": { + "description": "Amount of affected visits", + "type": "number" + } + } + }, + "example": { + "deletedVisits": 536 + } + } + } + }, + "default": { + "description": "Unexpected error.", + "content": { + "application/problem+json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + } + } } } diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index 012c6800..9feeee7b 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -18,6 +18,7 @@ return [ Command\Visit\LocateVisitsCommand::NAME => Command\Visit\LocateVisitsCommand::class, Command\Visit\DownloadGeoLiteDbCommand::NAME => Command\Visit\DownloadGeoLiteDbCommand::class, Command\Visit\GetOrphanVisitsCommand::NAME => Command\Visit\GetOrphanVisitsCommand::class, + Command\Visit\DeleteOrphanVisitsCommand::NAME => Command\Visit\DeleteOrphanVisitsCommand::class, Command\Visit\GetNonOrphanVisitsCommand::NAME => Command\Visit\GetNonOrphanVisitsCommand::class, Command\Api\GenerateKeyCommand::NAME => Command\Api\GenerateKeyCommand::class, diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 384df91d..6b7fc552 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -47,6 +47,7 @@ return [ Command\Visit\DownloadGeoLiteDbCommand::class => ConfigAbstractFactory::class, Command\Visit\LocateVisitsCommand::class => ConfigAbstractFactory::class, Command\Visit\GetOrphanVisitsCommand::class => ConfigAbstractFactory::class, + Command\Visit\DeleteOrphanVisitsCommand::class => ConfigAbstractFactory::class, Command\Visit\GetNonOrphanVisitsCommand::class => ConfigAbstractFactory::class, Command\Api\GenerateKeyCommand::class => ConfigAbstractFactory::class, @@ -98,6 +99,7 @@ return [ LockFactory::class, ], Command\Visit\GetOrphanVisitsCommand::class => [Visit\VisitsStatsHelper::class], + Command\Visit\DeleteOrphanVisitsCommand::class => [Visit\VisitsDeleter::class], Command\Visit\GetNonOrphanVisitsCommand::class => [Visit\VisitsStatsHelper::class, ShortUrlStringifier::class], Command\Api\GenerateKeyCommand::class => [ApiKeyService::class, ApiKey\RoleResolver::class], diff --git a/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php b/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php index fa7a8ee6..6cd04bfe 100644 --- a/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php @@ -4,22 +4,21 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; +use Shlinkio\Shlink\CLI\Command\Visit\AbstractDeleteVisitsCommand; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlVisitsDeleterInterface; -use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; -use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; use function sprintf; -class DeleteShortUrlVisitsCommand extends Command +class DeleteShortUrlVisitsCommand extends AbstractDeleteVisitsCommand { - public const NAME = 'short-url:delete-visits'; + public const NAME = 'short-url:visits-delete'; public function __construct(private readonly ShortUrlVisitsDeleterInterface $deleter) { @@ -44,15 +43,9 @@ class DeleteShortUrlVisitsCommand extends Command ); } - protected function execute(InputInterface $input, OutputInterface $output): ?int + protected function doExecute(InputInterface $input, SymfonyStyle $io): ?int { $identifier = ShortUrlIdentifier::fromCli($input); - $io = new SymfonyStyle($input, $output); - if (! $this->confirm($io)) { - $io->info('Operation aborted'); - return ExitCode::EXIT_SUCCESS; - } - try { $result = $this->deleter->deleteShortUrlVisits($identifier); $io->success(sprintf('Successfully deleted %s visits', $result->affectedItems)); @@ -64,9 +57,8 @@ class DeleteShortUrlVisitsCommand extends Command } } - private function confirm(SymfonyStyle $io): bool + protected function getWarningMessage(): string { - $io->warning('You are about to delete all visits for a short URL. This operation cannot be undone.'); - return $io->confirm('Continue deleting visits?', false); + return 'You are about to delete all visits for a short URL. This operation cannot be undone.'; } } diff --git a/module/CLI/src/Command/Visit/AbstractDeleteVisitsCommand.php b/module/CLI/src/Command/Visit/AbstractDeleteVisitsCommand.php new file mode 100644 index 00000000..f171d59a --- /dev/null +++ b/module/CLI/src/Command/Visit/AbstractDeleteVisitsCommand.php @@ -0,0 +1,35 @@ +confirm($io)) { + $io->info('Operation aborted'); + return ExitCode::EXIT_SUCCESS; + } + + return $this->doExecute($input, $io); + } + + private function confirm(SymfonyStyle $io): bool + { + $io->warning($this->getWarningMessage()); + return $io->confirm('Continue deleting visits?', false); + } + + abstract protected function doExecute(InputInterface $input, SymfonyStyle $io): ?int; + + abstract protected function getWarningMessage(): string; +} diff --git a/module/CLI/src/Command/Visit/DeleteOrphanVisitsCommand.php b/module/CLI/src/Command/Visit/DeleteOrphanVisitsCommand.php new file mode 100644 index 00000000..af1b7c66 --- /dev/null +++ b/module/CLI/src/Command/Visit/DeleteOrphanVisitsCommand.php @@ -0,0 +1,42 @@ +setName(self::NAME) + ->setDescription('Deletes all orphan visits'); + } + + protected function doExecute(InputInterface $input, SymfonyStyle $io): ?int + { + $result = $this->deleter->deleteOrphanVisits(); + $io->success(sprintf('Successfully deleted %s visits', $result->affectedItems)); + + return ExitCode::EXIT_SUCCESS; + } + + protected function getWarningMessage(): string + { + return 'You are about to delete all orphan visits. This operation cannot be undone.'; + } +} diff --git a/module/CLI/test/Command/Visit/DeleteOrphanVisitsCommandTest.php b/module/CLI/test/Command/Visit/DeleteOrphanVisitsCommandTest.php new file mode 100644 index 00000000..c18fe7f4 --- /dev/null +++ b/module/CLI/test/Command/Visit/DeleteOrphanVisitsCommandTest.php @@ -0,0 +1,43 @@ +deleter = $this->createMock(VisitsDeleterInterface::class); + $this->commandTester = $this->testerForCommand(new DeleteOrphanVisitsCommand($this->deleter)); + } + + #[Test] + public function successMessageIsPrintedAfterDeletion(): void + { + $this->deleter->expects($this->once())->method('deleteOrphanVisits')->willReturn(new BulkDeleteResult(5)); + $this->commandTester->setInputs(['yes']); + + $exitCode = $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); + self::assertStringContainsString('You are about to delete all orphan visits.', $output); + self::assertStringContainsString('Successfully deleted 5 visits', $output); + } +} diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 8e3e9a52..8b6acc3e 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -62,6 +62,7 @@ return [ Visit\VisitsTracker::class => ConfigAbstractFactory::class, Visit\RequestTracker::class => ConfigAbstractFactory::class, + Visit\VisitsDeleter::class => ConfigAbstractFactory::class, Visit\Geolocation\VisitLocator::class => ConfigAbstractFactory::class, Visit\Geolocation\VisitToLocationHelper::class => ConfigAbstractFactory::class, Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class, @@ -122,6 +123,7 @@ return [ Options\TrackingOptions::class, ], Visit\RequestTracker::class => [Visit\VisitsTracker::class, Options\TrackingOptions::class], + Visit\VisitsDeleter::class => [Visit\Repository\VisitDeleterRepository::class], ShortUrl\ShortUrlService::class => [ 'em', ShortUrl\ShortUrlResolver::class, diff --git a/module/Core/src/Visit/Repository/VisitDeleterRepository.php b/module/Core/src/Visit/Repository/VisitDeleterRepository.php index 602ba576..19f79dc7 100644 --- a/module/Core/src/Visit/Repository/VisitDeleterRepository.php +++ b/module/Core/src/Visit/Repository/VisitDeleterRepository.php @@ -19,4 +19,13 @@ class VisitDeleterRepository extends EntitySpecificationRepository implements Vi return $qb->getQuery()->execute(); } + + public function deleteOrphanVisits(): int + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->delete(Visit::class, 'v') + ->where($qb->expr()->isNull('v.shortUrl')); + + return $qb->getQuery()->execute(); + } } diff --git a/module/Core/src/Visit/Repository/VisitDeleterRepositoryInterface.php b/module/Core/src/Visit/Repository/VisitDeleterRepositoryInterface.php index 61a8af9b..a1516225 100644 --- a/module/Core/src/Visit/Repository/VisitDeleterRepositoryInterface.php +++ b/module/Core/src/Visit/Repository/VisitDeleterRepositoryInterface.php @@ -9,4 +9,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; interface VisitDeleterRepositoryInterface { public function deleteShortUrlVisits(ShortUrl $shortUrl): int; + + public function deleteOrphanVisits(): int; } diff --git a/module/Core/src/Visit/VisitsDeleter.php b/module/Core/src/Visit/VisitsDeleter.php new file mode 100644 index 00000000..5a8adca5 --- /dev/null +++ b/module/Core/src/Visit/VisitsDeleter.php @@ -0,0 +1,22 @@ +repository->deleteOrphanVisits()); + } +} diff --git a/module/Core/src/Visit/VisitsDeleterInterface.php b/module/Core/src/Visit/VisitsDeleterInterface.php new file mode 100644 index 00000000..3a75a0d3 --- /dev/null +++ b/module/Core/src/Visit/VisitsDeleterInterface.php @@ -0,0 +1,13 @@ +getEntityManager()->persist($shortUrl1); @@ -59,4 +59,21 @@ class VisitDeleterRepositoryTest extends DatabaseTestCase self::assertEquals(1, $this->repo->deleteShortUrlVisits($shortUrl3)); self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl3)); } + + #[Test] + public function deletesExpectedOrphanVisits(): void + { + $visitor = Visitor::emptyInstance(); + $this->getEntityManager()->persist(Visit::forBasePath($visitor)); + $this->getEntityManager()->persist(Visit::forInvalidShortUrl($visitor)); + $this->getEntityManager()->persist(Visit::forRegularNotFound($visitor)); + $this->getEntityManager()->persist(Visit::forBasePath($visitor)); + $this->getEntityManager()->persist(Visit::forInvalidShortUrl($visitor)); + $this->getEntityManager()->persist(Visit::forRegularNotFound($visitor)); + + $this->getEntityManager()->flush(); + + self::assertEquals(6, $this->repo->deleteOrphanVisits()); + self::assertEquals(0, $this->repo->deleteOrphanVisits()); + } } diff --git a/module/Core/test/Visit/VisitsDeleterTest.php b/module/Core/test/Visit/VisitsDeleterTest.php new file mode 100644 index 00000000..155d0725 --- /dev/null +++ b/module/Core/test/Visit/VisitsDeleterTest.php @@ -0,0 +1,41 @@ +repo = $this->createMock(VisitDeleterRepositoryInterface::class); + $this->visitsDeleter = new VisitsDeleter($this->repo); + } + + #[Test, DataProvider('provideVisitsCounts')] + public function returnsDeletedVisitsFromRepo(int $visitsCount): void + { + $this->repo->expects($this->once())->method('deleteOrphanVisits')->willReturn($visitsCount); + + $result = $this->visitsDeleter->deleteOrphanVisits(); + + self::assertEquals($visitsCount, $result->affectedItems); + } + + public static function provideVisitsCounts(): iterable + { + yield '45' => [45]; + yield '5000' => [5000]; + yield '0' => [0]; + } +} diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 43625c41..acca571d 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -38,6 +38,7 @@ return [ Action\Visit\DomainVisitsAction::class => ConfigAbstractFactory::class, Action\Visit\GlobalVisitsAction::class => ConfigAbstractFactory::class, Action\Visit\OrphanVisitsAction::class => ConfigAbstractFactory::class, + Action\Visit\DeleteOrphanVisitsAction::class => ConfigAbstractFactory::class, Action\Visit\NonOrphanVisitsAction::class => ConfigAbstractFactory::class, Action\Tag\ListTagsAction::class => ConfigAbstractFactory::class, Action\Tag\TagsStatsAction::class => ConfigAbstractFactory::class, @@ -90,6 +91,7 @@ return [ Visit\VisitsStatsHelper::class, Visit\Transformer\OrphanVisitDataTransformer::class, ], + Action\Visit\DeleteOrphanVisitsAction::class => [Visit\VisitsDeleter::class], Action\Visit\NonOrphanVisitsAction::class => [Visit\VisitsStatsHelper::class], Action\ShortUrl\ListShortUrlsAction::class => [ ShortUrl\ShortUrlListService::class, diff --git a/module/Rest/src/Action/Visit/DeleteOrphanVisitsAction.php b/module/Rest/src/Action/Visit/DeleteOrphanVisitsAction.php new file mode 100644 index 00000000..d1d2bc84 --- /dev/null +++ b/module/Rest/src/Action/Visit/DeleteOrphanVisitsAction.php @@ -0,0 +1,33 @@ +visitsDeleter->deleteOrphanVisits($apiKey); + + return new JsonResponse($result->toArray('deletedVisits')); + } +} diff --git a/module/Rest/src/Action/Visit/OrphanVisitsAction.php b/module/Rest/src/Action/Visit/OrphanVisitsAction.php index e53c2a6f..af5292a2 100644 --- a/module/Rest/src/Action/Visit/OrphanVisitsAction.php +++ b/module/Rest/src/Action/Visit/OrphanVisitsAction.php @@ -21,8 +21,8 @@ class OrphanVisitsAction extends AbstractRestAction protected const ROUTE_ALLOWED_METHODS = [self::METHOD_GET]; public function __construct( - private VisitsStatsHelperInterface $visitsHelper, - private DataTransformerInterface $orphanVisitTransformer, + private readonly VisitsStatsHelperInterface $visitsHelper, + private readonly DataTransformerInterface $orphanVisitTransformer, ) { } diff --git a/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php b/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php new file mode 100644 index 00000000..b7cf59b9 --- /dev/null +++ b/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php @@ -0,0 +1,42 @@ +getTotalVisits()); + self::assertEquals(3, $this->getOrphanVisits()); + + $resp = $this->callApiWithKey(self::METHOD_DELETE, '/visits/orphan'); + $payload = $this->getJsonResponsePayload($resp); + + self::assertEquals(200, $resp->getStatusCode()); + self::assertEquals(3, $payload['deletedVisits']); + self::assertEquals(7, $this->getTotalVisits()); // This verifies that regular visits have not been affected + self::assertEquals(0, $this->getOrphanVisits()); + } + + private function getTotalVisits(): int + { + $resp = $this->callApiWithKey(self::METHOD_GET, '/visits/non-orphan'); + $payload = $this->getJsonResponsePayload($resp); + + return $payload['visits']['pagination']['totalItems']; + } + + private function getOrphanVisits(): int + { + $resp = $this->callApiWithKey(self::METHOD_GET, '/visits/orphan'); + $payload = $this->getJsonResponsePayload($resp); + + return $payload['visits']['pagination']['totalItems']; + } +} diff --git a/module/Rest/test-api/Action/DeleteShortUrlVisitsTest.php b/module/Rest/test-api/Action/DeleteShortUrlVisitsTest.php index 045f2c9a..7b5f306d 100644 --- a/module/Rest/test-api/Action/DeleteShortUrlVisitsTest.php +++ b/module/Rest/test-api/Action/DeleteShortUrlVisitsTest.php @@ -22,8 +22,8 @@ class DeleteShortUrlVisitsTest extends ApiTestCase self::assertEquals(200, $resp->getStatusCode()); self::assertEquals(3, $payload['deletedVisits']); - self::assertEquals(4, $this->getTotalVisits()); - self::assertEquals(3, $this->getOrphanVisits()); + self::assertEquals(4, $this->getTotalVisits()); // This verifies that other visits have not been affected + self::assertEquals(3, $this->getOrphanVisits()); // This verifies that orphan visits have not been affected } private function getTotalVisits(): int diff --git a/module/Rest/test/Action/Visit/DeleteOrphanVisitsActionTest.php b/module/Rest/test/Action/Visit/DeleteOrphanVisitsActionTest.php new file mode 100644 index 00000000..b7f6031e --- /dev/null +++ b/module/Rest/test/Action/Visit/DeleteOrphanVisitsActionTest.php @@ -0,0 +1,53 @@ +deleter = $this->createMock(VisitsDeleterInterface::class); + $this->action = new DeleteOrphanVisitsAction($this->deleter); + } + + #[Test, DataProvider('provideVisitsCounts')] + public function orphanVisitsAreDeleted(int $visitsCount): void + { + $apiKey = ApiKey::create(); + $request = ServerRequestFactory::fromGlobals()->withAttribute(ApiKey::class, $apiKey); + + $this->deleter->expects($this->once())->method('deleteOrphanVisits')->with($apiKey)->willReturn( + new BulkDeleteResult($visitsCount), + ); + + /** @var JsonResponse $resp */ + $resp = $this->action->handle($request); + $payload = $resp->getPayload(); + + self::assertEquals(['deletedVisits' => $visitsCount], $payload); + } + + public static function provideVisitsCounts(): iterable + { + yield '1' => [1]; + yield '0' => [0]; + yield '300' => [300]; + yield '1234' => [1234]; + } +}