diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e9555e5..a810fdc1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,7 +38,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * *Nothing* ### Fixed -* *Nothing* +* [#2264](https://github.com/shlinkio/shlink/issues/2264) Fix visits counts not being deleted when deleting short URL or orphan visits. ## [4.2.5] - 2024-11-03 diff --git a/module/Core/src/Visit/Repository/VisitDeleterRepository.php b/module/Core/src/Visit/Repository/VisitDeleterRepository.php index 94c57091..425dd88a 100644 --- a/module/Core/src/Visit/Repository/VisitDeleterRepository.php +++ b/module/Core/src/Visit/Repository/VisitDeleterRepository.php @@ -6,15 +6,28 @@ namespace Shlinkio\Shlink\Core\Visit\Repository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Visit\Entity\OrphanVisitsCount; +use Shlinkio\Shlink\Core\Visit\Entity\ShortUrlVisitsCount; use Shlinkio\Shlink\Core\Visit\Entity\Visit; /** @extends EntitySpecificationRepository */ class VisitDeleterRepository extends EntitySpecificationRepository implements VisitDeleterRepositoryInterface { public function deleteShortUrlVisits(ShortUrl $shortUrl): int + { + return $this->getEntityManager()->wrapInTransaction(function () use ($shortUrl): int { + $this->deleteByShortUrl(ShortUrlVisitsCount::class, $shortUrl); + return $this->deleteByShortUrl(Visit::class, $shortUrl); + }); + } + + /** + * @param class-string $entityName + */ + private function deleteByShortUrl(string $entityName, ShortUrl $shortUrl): int { $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->delete(Visit::class, 'v') + $qb->delete($entityName, 'v') ->where($qb->expr()->eq('v.shortUrl', ':shortUrl')) ->setParameter('shortUrl', $shortUrl); @@ -23,10 +36,15 @@ class VisitDeleterRepository extends EntitySpecificationRepository implements Vi public function deleteOrphanVisits(): int { - $qb = $this->getEntityManager()->createQueryBuilder(); - $qb->delete(Visit::class, 'v') - ->where($qb->expr()->isNull('v.shortUrl')); + $em = $this->getEntityManager(); + return $em->wrapInTransaction(function () use ($em): int { + $em->createQueryBuilder()->delete(OrphanVisitsCount::class, 'v')->getQuery()->execute(); - return $qb->getQuery()->execute(); + $qb = $em->createQueryBuilder(); + $qb->delete(Visit::class, 'v') + ->where($qb->expr()->isNull('v.shortUrl')); + + return $qb->getQuery()->execute(); + }); } } diff --git a/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php index eedd2897..58d52713 100644 --- a/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php @@ -9,18 +9,26 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; 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\Visitor; +use Shlinkio\Shlink\Core\Visit\Repository\OrphanVisitsCountRepository; +use Shlinkio\Shlink\Core\Visit\Repository\ShortUrlVisitsCountRepository; use Shlinkio\Shlink\Core\Visit\Repository\VisitDeleterRepository; use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; class VisitDeleterRepositoryTest extends DatabaseTestCase { private VisitDeleterRepository $repo; + private ShortUrlVisitsCountRepository $visitsCountRepo; + private OrphanVisitsCountRepository $orphanVisitsCountRepo; protected function setUp(): void { $this->repo = $this->createRepository(Visit::class, VisitDeleterRepository::class); + $this->visitsCountRepo = $this->getEntityManager()->getRepository(ShortUrlVisitsCount::class); + $this->orphanVisitsCountRepo = $this->getEntityManager()->getRepository(OrphanVisitsCount::class); } #[Test] @@ -51,12 +59,20 @@ class VisitDeleterRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); - self::assertEquals(2, $this->repo->deleteShortUrlVisits($shortUrl1)); - self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl1)); - self::assertEquals(4, $this->repo->deleteShortUrlVisits($shortUrl2)); - self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl2)); - self::assertEquals(1, $this->repo->deleteShortUrlVisits($shortUrl3)); - self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl3)); + $this->assertVisitsDeletionForShortUrl($shortUrl1, 2); + $this->assertVisitsDeletionForShortUrl($shortUrl2, 4); + $this->assertVisitsDeletionForShortUrl($shortUrl3, 1); + } + + private function assertVisitsDeletionForShortUrl(ShortUrl $shortUrl, int $expectedDeleteCount): void + { + // There should be at least one visit count before deletion + self::assertGreaterThan(0, $this->visitsCountRepo->count(['shortUrl' => $shortUrl])); + self::assertEquals($expectedDeleteCount, $this->repo->deleteShortUrlVisits($shortUrl)); + + // Visits counts are also deleted, and trying to delete again results in no visits deleted + self::assertEquals(0, $this->visitsCountRepo->count(['shortUrl' => $shortUrl])); + self::assertEquals(0, $this->repo->deleteShortUrlVisits($shortUrl)); } #[Test] @@ -72,7 +88,9 @@ class VisitDeleterRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); + self::assertGreaterThan(0, $this->orphanVisitsCountRepo->count()); self::assertEquals(6, $this->repo->deleteOrphanVisits()); self::assertEquals(0, $this->repo->deleteOrphanVisits()); + self::assertEquals(0, $this->orphanVisitsCountRepo->count()); } }