Fix visits counts not being deleted when deleting short URL or orphan visits

module/Core/src/Action/AbstractTrackingAction.php

@@ -15,7 +15,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl;
 use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier;
 use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface;
 use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface;
-use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface
 {
@@ -31,12 +30,9 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet
 
         try {
             $shortUrl = $this->urlResolver->resolveEnabledShortUrl($identifier);
-            $visit = $this->requestTracker->trackIfApplicable($shortUrl, $request);
+            $this->requestTracker->trackIfApplicable($shortUrl, $request);
 
-            return $this->createSuccessResp(
-                $shortUrl,
-                $request->withAttribute(Location::class, $visit?->getVisitLocation()),
-            );
+            return $this->createSuccessResp($shortUrl, $request);
         } catch (ShortUrlNotFoundException) {
             return $this->createErrorResp($request, $handler);
         }

module/Core/src/Config/Options/TrackingOptions.php

@@ -59,4 +59,12 @@ final readonly class TrackingOptions
     {
         return $this->disableTrackParam !== null && array_key_exists($this->disableTrackParam, $query);
     }
+
+    /**
+     * If IP address tracking is disabled, or tracking is disabled all together, then geolocation is not relevant
+     */
+    public function isGeolocationRelevant(): bool
+    {
+        return ! $this->disableTracking && ! $this->disableIpTracking;
+    }
 }

module/Core/src/EventDispatcher/Async/AbstractNotifyVisitListener.php

@@ -8,7 +8,7 @@ use Doctrine\ORM\EntityManagerInterface;
 use Psr\Log\LoggerInterface;
 use Shlinkio\Shlink\Common\UpdatePublishing\PublishingHelperInterface;
 use Shlinkio\Shlink\Common\UpdatePublishing\Update;
-use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated;
+use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited;
 use Shlinkio\Shlink\Core\EventDispatcher\PublishingUpdatesGeneratorInterface;
 use Shlinkio\Shlink\Core\Visit\Entity\Visit;
 use Throwable;
@@ -25,7 +25,7 @@ abstract class AbstractNotifyVisitListener extends AbstractAsyncListener
     ) {
     }
 
-    public function __invoke(VisitLocated $visitLocated): void
+    public function __invoke(UrlVisited $visitLocated): void
     {
         if (! $this->isEnabled()) {
             return;

module/Core/src/EventDispatcher/Event/AbstractVisitEvent.php

@@ -1,27 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Shlinkio\Shlink\Core\EventDispatcher\Event;
-
-use JsonSerializable;
-use Shlinkio\Shlink\EventDispatcher\Util\JsonUnserializable;
-
-abstract class AbstractVisitEvent implements JsonSerializable, JsonUnserializable
-{
-    final public function __construct(
-        public readonly string $visitId,
-        public readonly string|null $originalIpAddress = null,
-    ) {
-    }
-
-    public function jsonSerialize(): array
-    {
-        return ['visitId' => $this->visitId, 'originalIpAddress' => $this->originalIpAddress];
-    }
-
-    public static function fromPayload(array $payload): self
-    {
-        return new static($payload['visitId'] ?? '', $payload['originalIpAddress'] ?? null);
-    }
-}

module/Core/src/EventDispatcher/Event/ShortUrlCreated.php

@@ -7,9 +7,9 @@ namespace Shlinkio\Shlink\Core\EventDispatcher\Event;
 use JsonSerializable;
 use Shlinkio\Shlink\EventDispatcher\Util\JsonUnserializable;
 
-final class ShortUrlCreated implements JsonSerializable, JsonUnserializable
+final readonly class ShortUrlCreated implements JsonSerializable, JsonUnserializable
 {
-    public function __construct(public readonly string $shortUrlId)
+    public function __construct(public string $shortUrlId)
     {
     }
 

module/Core/src/EventDispatcher/Event/UrlVisited.php

@@ -4,6 +4,24 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\EventDispatcher\Event;
 
-final class UrlVisited extends AbstractVisitEvent
+use JsonSerializable;
+use Shlinkio\Shlink\EventDispatcher\Util\JsonUnserializable;
+
+final readonly class UrlVisited implements JsonSerializable, JsonUnserializable
 {
+    final public function __construct(
+        public string $visitId,
+        public string|null $originalIpAddress = null,
+    ) {
+    }
+
+    public function jsonSerialize(): array
+    {
+        return ['visitId' => $this->visitId, 'originalIpAddress' => $this->originalIpAddress];
+    }
+
+    public static function fromPayload(array $payload): self
+    {
+        return new self($payload['visitId'] ?? '', $payload['originalIpAddress'] ?? null);
+    }
 }

module/Core/src/EventDispatcher/Event/VisitLocated.php

@@ -1,9 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Shlinkio\Shlink\Core\EventDispatcher\Event;
-
-final class VisitLocated extends AbstractVisitEvent
-{
-}

module/Core/src/EventDispatcher/LocateVisit.php

@@ -1,77 +0,0 @@
-<?php
-
-declare(strict_types=1);
-
-namespace Shlinkio\Shlink\Core\EventDispatcher;
-
-use Doctrine\ORM\EntityManagerInterface;
-use Psr\EventDispatcher\EventDispatcherInterface;
-use Psr\Log\LoggerInterface;
-use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited;
-use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated;
-use Shlinkio\Shlink\Core\Visit\Entity\Visit;
-use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation;
-use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
-use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface;
-use Shlinkio\Shlink\IpGeolocation\Model\Location;
-use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
-use Throwable;
-
-readonly class LocateVisit
-{
-    public function __construct(
-        private IpLocationResolverInterface $ipLocationResolver,
-        private EntityManagerInterface $em,
-        private LoggerInterface $logger,
-        private DbUpdaterInterface $dbUpdater,
-        private EventDispatcherInterface $eventDispatcher,
-    ) {
-    }
-
-    public function __invoke(UrlVisited $shortUrlVisited): void
-    {
-        $visitId = $shortUrlVisited->visitId;
-
-        /** @var Visit|null $visit */
-        $visit = $this->em->find(Visit::class, $visitId);
-        if ($visit === null) {
-            $this->logger->warning('Tried to locate visit with id "{visitId}", but it does not exist.', [
-                'visitId' => $visitId,
-            ]);
-            return;
-        }
-
-        $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress, $visit);
-        $this->eventDispatcher->dispatch(new VisitLocated($visitId, $shortUrlVisited->originalIpAddress));
-    }
-
-    private function locateVisit(string $visitId, string|null $originalIpAddress, Visit $visit): void
-    {
-        if (! $this->dbUpdater->databaseFileExists()) {
-            $this->logger->warning('Tried to locate visit with id "{visitId}", but a GeoLite2 db was not found.', [
-                'visitId' => $visitId,
-            ]);
-            return;
-        }
-
-        $isLocatable = $originalIpAddress !== null || $visit->isLocatable();
-        $addr = $originalIpAddress ?? $visit->remoteAddr ?? '';
-
-        try {
-            $location = $isLocatable ? $this->ipLocationResolver->resolveIpLocation($addr) : Location::emptyInstance();
-
-            $visit->locate(VisitLocation::fromGeolocation($location));
-            $this->em->flush();
-        } catch (WrongIpException $e) {
-            $this->logger->warning(
-                'Tried to locate visit with id "{visitId}", but its address seems to be wrong. {e}',
-                ['e' => $e, 'visitId' => $visitId],
-            );
-        } catch (Throwable $e) {
-            $this->logger->error(
-                'An unexpected error occurred while trying to locate visit with id "{visitId}". {e}',
-                ['e' => $e, 'visitId' => $visitId],
-            );
-        }
-    }
-}

module/Core/src/EventDispatcher/Matomo/SendVisitToMatomo.php

@@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\EventDispatcher\Matomo;
 
 use Doctrine\ORM\EntityManagerInterface;
 use Psr\Log\LoggerInterface;
-use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated;
+use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited;
 use Shlinkio\Shlink\Core\Matomo\MatomoOptions;
 use Shlinkio\Shlink\Core\Matomo\MatomoVisitSenderInterface;
 use Shlinkio\Shlink\Core\Visit\Entity\Visit;
@@ -22,7 +22,7 @@ readonly class SendVisitToMatomo
     ) {
     }
 
-    public function __invoke(VisitLocated $visitLocated): void
+    public function __invoke(UrlVisited $visitLocated): void
     {
         if (! $this->matomoOptions->enabled) {
             return;

module/Core/src/Geolocation/Middleware/IpGeolocationMiddleware.php

@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Geolocation\Middleware;
+
+use Psr\Http\Message\ResponseInterface;
+use Psr\Http\Message\ServerRequestInterface;
+use Psr\Http\Server\MiddlewareInterface;
+use Psr\Http\Server\RequestHandlerInterface;
+use Psr\Log\LoggerInterface;
+use Shlinkio\Shlink\Core\Config\Options\TrackingOptions;
+use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException;
+use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdaterInterface;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
+use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
+use Throwable;
+
+use function Shlinkio\Shlink\Core\ipAddressFromRequest;
+
+readonly class IpGeolocationMiddleware implements MiddlewareInterface
+{
+    public function __construct(
+        private IpLocationResolverInterface $ipLocationResolver,
+        private DbUpdaterInterface $dbUpdater,
+        private LoggerInterface $logger,
+        private TrackingOptions $trackingOptions,
+    ) {
+    }
+
+    public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
+    {
+        if (! $this->trackingOptions->isGeolocationRelevant()) {
+            return $handler->handle($request);
+        }
+
+        if (! $this->dbUpdater->databaseFileExists()) {
+            $this->logger->warning('Tried to geolocate IP address, but a GeoLite2 db was not found.');
+            return $handler->handle($request);
+        }
+
+        $location = $this->geolocateIpAddress(ipAddressFromRequest($request));
+        return $handler->handle($request->withAttribute(Location::class, $location));
+    }
+
+    private function geolocateIpAddress(string|null $ipAddress): Location
+    {
+        try {
+            return $ipAddress === null ? Location::empty() : $this->ipLocationResolver->resolveIpLocation($ipAddress);
+        } catch (WrongIpException $e) {
+            $this->logger->warning('Tried to locate IP address, but it seems to be wrong. {e}', ['e' => $e]);
+            return Location::empty();
+        } catch (Throwable $e) {
+            $this->logger->error('An unexpected error occurred while trying to locate IP address. {e}', ['e' => $e]);
+            return Location::empty();
+        }
+    }
+}

module/Core/src/RedirectRule/Entity/RedirectCondition.php

@@ -9,11 +9,10 @@ use Shlinkio\Shlink\Core\Model\DeviceType;
 use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType;
 use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter;
 use Shlinkio\Shlink\Core\Util\IpAddressUtils;
-use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation;
-use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 use function Shlinkio\Shlink\Core\acceptLanguageToLocales;
 use function Shlinkio\Shlink\Core\ArrayUtils\some;
+use function Shlinkio\Shlink\Core\geolocationFromRequest;
 use function Shlinkio\Shlink\Core\ipAddressFromRequest;
 use function Shlinkio\Shlink\Core\normalizeLocale;
 use function Shlinkio\Shlink\Core\splitLocale;
@@ -134,9 +133,8 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable
 
     private function matchesGeolocationCountryCode(ServerRequestInterface $request): bool
     {
-        $geolocation = $request->getAttribute(Location::class);
-        // TODO We should eventually rely on `Location` type only
-        if (! ($geolocation instanceof Location) && ! ($geolocation instanceof VisitLocation)) {
+        $geolocation = geolocationFromRequest($request);
+        if ($geolocation === null) {
             return false;
         }
 
@@ -145,14 +143,12 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable
 
     private function matchesGeolocationCityName(ServerRequestInterface $request): bool
     {
-        $geolocation = $request->getAttribute(Location::class);
-        // TODO We should eventually rely on `Location` type only
-        if (! ($geolocation instanceof Location) && ! ($geolocation instanceof VisitLocation)) {
+        $geolocation = geolocationFromRequest($request);
+        if ($geolocation === null) {
             return false;
         }
 
-        $cityName = $geolocation instanceof Location ? $geolocation->city : $geolocation->cityName;
-        return strcasecmp($cityName, $this->matchValue) === 0;
+        return strcasecmp($geolocation->city, $this->matchValue) === 0;
     }
 
     public function jsonSerialize(): array

module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php

@@ -17,7 +17,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier;
 use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface;
 use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface;
 use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface;
-use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
 use function array_slice;
 use function count;
@@ -74,13 +73,9 @@ readonly class ExtraPathRedirectMiddleware implements MiddlewareInterface
 
         try {
             $shortUrl = $this->resolver->resolveEnabledShortUrl($identifier);
-            $visit = $this->requestTracker->trackIfApplicable($shortUrl, $request);
+            $this->requestTracker->trackIfApplicable($shortUrl, $request);
 
-            $longUrl = $this->redirectionBuilder->buildShortUrlRedirect(
-                $shortUrl,
-                $request->withAttribute(Location::class, $visit?->getVisitLocation()),
-                $extraPath,
-            );
+            $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath);
             return $this->redirectResponseHelper->buildRedirectResponse($longUrl);
         } catch (ShortUrlNotFoundException) {
             if ($extraPath === null || ! $this->urlShortenerOptions->multiSegmentSlugsEnabled) {

module/Core/src/Visit/Entity/Visit.php

@@ -59,14 +59,16 @@ class Visit extends AbstractEntity implements JsonSerializable
         Visitor $visitor,
         bool $anonymize,
     ): self {
+        $geolocation = $visitor->geolocation;
         return new self(
             shortUrl: $shortUrl,
             type: $type,
             userAgent: $visitor->userAgent,
             referer: $visitor->referer,
-            potentialBot: $visitor->isPotentialBot(),
+            potentialBot: $visitor->potentialBot,
             remoteAddr: self::processAddress($visitor->remoteAddress, $anonymize),
             visitedUrl: $visitor->visitedUrl,
+            visitLocation: $geolocation !== null ? VisitLocation::fromGeolocation($geolocation) : null,
         );
     }
 

module/Core/src/Visit/Model/Visitor.php

@@ -6,78 +6,85 @@ namespace Shlinkio\Shlink\Core\Visit\Model;
 
 use Psr\Http\Message\ServerRequestInterface;
 use Shlinkio\Shlink\Core\Config\Options\TrackingOptions;
+use Shlinkio\Shlink\IpGeolocation\Model\Location;
 
+use function Shlinkio\Shlink\Core\geolocationFromRequest;
 use function Shlinkio\Shlink\Core\ipAddressFromRequest;
 use function Shlinkio\Shlink\Core\isCrawler;
 use function substr;
 
-final class Visitor
+final readonly class Visitor
 {
     public const USER_AGENT_MAX_LENGTH = 512;
     public const REFERER_MAX_LENGTH = 1024;
     public const REMOTE_ADDRESS_MAX_LENGTH = 256;
     public const VISITED_URL_MAX_LENGTH = 2048;
 
-    public readonly string $userAgent;
-    public readonly string $referer;
-    public readonly string $visitedUrl;
-    public readonly string|null $remoteAddress;
-    private bool $potentialBot;
-
-    public function __construct(string $userAgent, string $referer, string|null $remoteAddress, string $visitedUrl)
-    {
-        $this->userAgent = $this->cropToLength($userAgent, self::USER_AGENT_MAX_LENGTH);
-        $this->referer = $this->cropToLength($referer, self::REFERER_MAX_LENGTH);
-        $this->visitedUrl = $this->cropToLength($visitedUrl, self::VISITED_URL_MAX_LENGTH);
-        $this->remoteAddress = $remoteAddress === null ? null : $this->cropToLength(
-            $remoteAddress,
-            self::REMOTE_ADDRESS_MAX_LENGTH,
-        );
-        $this->potentialBot = isCrawler($userAgent);
+    private function __construct(
+        public string $userAgent,
+        public string $referer,
+        public string|null $remoteAddress,
+        public string $visitedUrl,
+        public bool $potentialBot,
+        public Location|null $geolocation,
+    ) {
     }
 
-    private function cropToLength(string $value, int $length): string
+    public static function fromParams(
+        string $userAgent = '',
+        string $referer = '',
+        string|null $remoteAddress = null,
+        string $visitedUrl = '',
+        Location|null $geolocation = null,
+    ): self {
+        return new self(
+            userAgent: self::cropToLength($userAgent, self::USER_AGENT_MAX_LENGTH),
+            referer: self::cropToLength($referer, self::REFERER_MAX_LENGTH),
+            remoteAddress: $remoteAddress === null
+                ? null
+                : self::cropToLength($remoteAddress, self::REMOTE_ADDRESS_MAX_LENGTH),
+            visitedUrl: self::cropToLength($visitedUrl, self::VISITED_URL_MAX_LENGTH),
+            potentialBot: isCrawler($userAgent),
+            geolocation: $geolocation,
+        );
+    }
+
+    private static function cropToLength(string $value, int $length): string
     {
         return substr($value, 0, $length);
     }
 
     public static function fromRequest(ServerRequestInterface $request): self
     {
-        return new self(
-            $request->getHeaderLine('User-Agent'),
-            $request->getHeaderLine('Referer'),
-            ipAddressFromRequest($request),
-            $request->getUri()->__toString(),
+        return self::fromParams(
+            userAgent: $request->getHeaderLine('User-Agent'),
+            referer: $request->getHeaderLine('Referer'),
+            remoteAddress: ipAddressFromRequest($request),
+            visitedUrl: $request->getUri()->__toString(),
+            geolocation: geolocationFromRequest($request),
         );
     }
 
     public static function empty(): self
     {
-        return new self('', '', null, '');
+        return self::fromParams();
     }
 
     public static function botInstance(): self
     {
-        return new self('cf-facebook', '', null, '');
-    }
-
-    public function isPotentialBot(): bool
-    {
-        return $this->potentialBot;
+        return self::fromParams(userAgent: 'cf-facebook');
     }
 
     public function normalizeForTrackingOptions(TrackingOptions $options): self
     {
-        $instance = new self(
-            $options->disableUaTracking ? '' : $this->userAgent,
-            $options->disableReferrerTracking ? '' : $this->referer,
-            $options->disableIpTracking ? null : $this->remoteAddress,
-            $this->visitedUrl,
+        return new self(
+            userAgent: $options->disableUaTracking ? '' : $this->userAgent,
+            referer: $options->disableReferrerTracking ? '' : $this->referer,
+            remoteAddress: $options->disableIpTracking ? null : $this->remoteAddress,
+            visitedUrl: $this->visitedUrl,
+            // Keep the fact that the visit was a potential bot, even if we no longer save the user agent
+            potentialBot: $this->potentialBot,
+            geolocation: $this->geolocation,
         );
-
-        // Keep the fact that the visit was a potential bot, even if we no longer save the user agent
-        $instance->potentialBot = $this->potentialBot;
-
-        return $instance;
     }
 }

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<Visit> */
 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<Visit | ShortUrlVisitsCount> $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();
+        });
     }
 }