Skip to content

Commit

Permalink
Fix visits counts not being deleted when deleting short URL or orphan…
Browse files Browse the repository at this point in the history
… visits
  • Loading branch information
acelaya committed Nov 15, 2024
2 parents 1fee745 + 42ff0d5 commit 178a99b
Showing 42 changed files with 473 additions and 489 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
6 changes: 5 additions & 1 deletion config/autoload/middleware-pipeline.global.php
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@
use Shlinkio\Shlink\Common\Middleware\AccessLogMiddleware;
use Shlinkio\Shlink\Common\Middleware\ContentLengthMiddleware;
use Shlinkio\Shlink\Common\Middleware\RequestIdMiddleware;
use Shlinkio\Shlink\Core\Geolocation\Middleware\IpGeolocationMiddleware;

return [

@@ -67,8 +68,11 @@
],
'not-found' => [
'middleware' => [
// This middleware is in front of tracking actions explicitly. Putting here for orphan visits tracking
// These two middlewares are in front of other tracking actions.
// Putting them here for orphan visits tracking
IpAddress::class,
IpGeolocationMiddleware::class,

Core\ErrorHandler\NotFoundTypeResolverMiddleware::class,
Core\ShortUrl\Middleware\ExtraPathRedirectMiddleware::class,
Core\ErrorHandler\NotFoundTrackerMiddleware::class,
3 changes: 3 additions & 0 deletions config/autoload/routes.config.php
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@
use RKA\Middleware\IpAddress;
use Shlinkio\Shlink\Core\Action as CoreAction;
use Shlinkio\Shlink\Core\Config\EnvVars;
use Shlinkio\Shlink\Core\Geolocation\Middleware\IpGeolocationMiddleware;
use Shlinkio\Shlink\Core\ShortUrl\Middleware\TrimTrailingSlashMiddleware;
use Shlinkio\Shlink\Rest\Action;
use Shlinkio\Shlink\Rest\ConfigProvider;
@@ -88,6 +89,7 @@
'path' => '/{shortCode}/track',
'middleware' => [
IpAddress::class,
IpGeolocationMiddleware::class,
CoreAction\PixelAction::class,
],
'allowed_methods' => [RequestMethodInterface::METHOD_GET],
@@ -105,6 +107,7 @@
'path' => sprintf('/{shortCode}%s', $shortUrlRouteSuffix),
'middleware' => [
IpAddress::class,
IpGeolocationMiddleware::class,
TrimTrailingSlashMiddleware::class,
CoreAction\RedirectAction::class,
],
2 changes: 1 addition & 1 deletion module/CLI/src/GeoLite/GeolocationDbUpdater.php
Original file line number Diff line number Diff line change
@@ -44,7 +44,7 @@ public function checkDbUpdate(
callable|null $beforeDownload = null,
callable|null $handleProgress = null,
): GeolocationResult {
if ($this->trackingOptions->disableTracking || $this->trackingOptions->disableIpTracking) {
if (! $this->trackingOptions->isGeolocationRelevant()) {
return GeolocationResult::CHECK_SKIPPED;
}

Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ protected function setUp(): void
public function outputIsProperlyGenerated(): void
{
$shortUrl = ShortUrl::createFake();
$visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate(
$visit = Visit::forValidShortUrl($shortUrl, Visitor::fromParams('bar', 'foo', ''))->locate(
VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')),
);
$domain = 's.test';
Original file line number Diff line number Diff line change
@@ -93,7 +93,7 @@ public function providingInvalidDatesPrintsWarning(): void
#[Test]
public function outputIsProperlyGenerated(): void
{
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('bar', 'foo', '', ''))->locate(
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams('bar', 'foo', ''))->locate(
VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')),
);
$shortCode = 'abc123';
2 changes: 1 addition & 1 deletion module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ protected function setUp(): void
public function outputIsProperlyGenerated(): void
{
$shortUrl = ShortUrl::createFake();
$visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate(
$visit = Visit::forValidShortUrl($shortUrl, Visitor::fromParams('bar', 'foo', ''))->locate(
VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')),
);
$tag = 'abc123';
Original file line number Diff line number Diff line change
@@ -40,7 +40,7 @@ protected function setUp(): void
public function outputIsProperlyGenerated(): void
{
$shortUrl = ShortUrl::createFake();
$visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate(
$visit = Visit::forValidShortUrl($shortUrl, Visitor::fromParams('bar', 'foo', ''))->locate(
VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')),
);
$this->visitsHelper->expects($this->once())->method('nonOrphanVisits')->withAnyParameters()->willReturn(
Original file line number Diff line number Diff line change
@@ -37,7 +37,7 @@ protected function setUp(): void
#[TestWith([['--type' => OrphanVisitType::BASE_URL->value], true])]
public function outputIsProperlyGenerated(array $args, bool $includesType): void
{
$visit = Visit::forBasePath(new Visitor('bar', 'foo', '', ''))->locate(
$visit = Visit::forBasePath(Visitor::fromParams('bar', 'foo', ''))->locate(
VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')),
);
$this->visitsHelper->expects($this->once())->method('orphanVisits')->with($this->callback(
6 changes: 3 additions & 3 deletions module/CLI/test/Command/Visit/LocateVisitsCommandTest.php
Original file line number Diff line number Diff line change
@@ -63,8 +63,8 @@ public function expectedSetOfVisitsIsProcessedBasedOnArgs(
bool $expectWarningPrint,
array $args,
): void {
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', ''));
$location = VisitLocation::fromGeolocation(Location::emptyInstance());
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams('', '', '1.2.3.4'));
$location = VisitLocation::fromGeolocation(Location::empty());
$mockMethodBehavior = $this->invokeHelperMethods($visit, $location);

$this->lock->method('acquire')->with($this->isFalse())->willReturn(true);
@@ -134,7 +134,7 @@ public static function provideIgnoredAddresses(): iterable
#[Test]
public function errorWhileLocatingIpIsDisplayed(): void
{
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', ''));
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4'));
$location = VisitLocation::fromGeolocation(Location::emptyInstance());

$this->lock->method('acquire')->with($this->isFalse())->willReturn(true);
10 changes: 10 additions & 0 deletions module/Core/config/dependencies.config.php
Original file line number Diff line number Diff line change
@@ -11,6 +11,7 @@
use Shlinkio\Shlink\Core\Config\Options\NotFoundRedirectOptions;
use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier;
use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface;
use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater;
use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;
use Symfony\Component\Lock;

@@ -102,6 +103,8 @@

EventDispatcher\PublishingUpdatesGenerator::class => ConfigAbstractFactory::class,

Geolocation\Middleware\IpGeolocationMiddleware::class => ConfigAbstractFactory::class,

Importer\ImportedLinksProcessor::class => ConfigAbstractFactory::class,

Crawling\CrawlingHelper::class => ConfigAbstractFactory::class,
@@ -237,6 +240,13 @@

EventDispatcher\PublishingUpdatesGenerator::class => [ShortUrl\Transformer\ShortUrlDataTransformer::class],

Geolocation\Middleware\IpGeolocationMiddleware::class => [
IpLocationResolverInterface::class,
DbUpdater::class,
'Logger_Shlink',
Config\Options\TrackingOptions::class,
],

Importer\ImportedLinksProcessor::class => [
'em',
ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class,
19 changes: 3 additions & 16 deletions module/Core/config/event_dispatcher.config.php
Original file line number Diff line number Diff line change
@@ -15,23 +15,18 @@
use Shlinkio\Shlink\Core\Visit\Geolocation\VisitLocator;
use Shlinkio\Shlink\Core\Visit\Geolocation\VisitToLocationHelper;
use Shlinkio\Shlink\EventDispatcher\Listener\EnabledListenerCheckerInterface;
use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater;
use Shlinkio\Shlink\IpGeolocation\GeoLite2\GeoLite2Options;
use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface;

use function Shlinkio\Shlink\Config\runningInRoadRunner;

return (static function (): array {
$regularEvents = [
EventDispatcher\Event\UrlVisited::class => [
EventDispatcher\LocateVisit::class,
],
EventDispatcher\Event\GeoLiteDbCreated::class => [
EventDispatcher\LocateUnlocatedVisits::class,
],
];
$asyncEvents = [
EventDispatcher\Event\VisitLocated::class => [
EventDispatcher\Event\UrlVisited::class => [
EventDispatcher\Mercure\NotifyVisitToMercure::class,
EventDispatcher\RabbitMq\NotifyVisitToRabbitMq::class,
EventDispatcher\RedisPubSub\NotifyVisitToRedis::class,
@@ -46,9 +41,9 @@

// Send visits to matomo asynchronously if the runtime allows it
if (runningInRoadRunner()) {
$asyncEvents[EventDispatcher\Event\VisitLocated::class][] = EventDispatcher\Matomo\SendVisitToMatomo::class;
$asyncEvents[EventDispatcher\Event\UrlVisited::class][] = EventDispatcher\Matomo\SendVisitToMatomo::class;
} else {
$regularEvents[EventDispatcher\Event\VisitLocated::class] = [EventDispatcher\Matomo\SendVisitToMatomo::class];
$regularEvents[EventDispatcher\Event\UrlVisited::class] = [EventDispatcher\Matomo\SendVisitToMatomo::class];
}

return [
@@ -60,7 +55,6 @@

'dependencies' => [
'factories' => [
EventDispatcher\LocateVisit::class => ConfigAbstractFactory::class,
EventDispatcher\Matomo\SendVisitToMatomo::class => ConfigAbstractFactory::class,
EventDispatcher\LocateUnlocatedVisits::class => ConfigAbstractFactory::class,
EventDispatcher\Mercure\NotifyVisitToMercure::class => ConfigAbstractFactory::class,
@@ -104,13 +98,6 @@
],

ConfigAbstractFactory::class => [
EventDispatcher\LocateVisit::class => [
IpLocationResolverInterface::class,
'em',
'Logger_Shlink',
DbUpdater::class,
EventDispatcherInterface::class,
],
EventDispatcher\LocateUnlocatedVisits::class => [VisitLocator::class, VisitToLocationHelper::class],
EventDispatcher\Mercure\NotifyVisitToMercure::class => [
MercureHubPublishingHelper::class,
11 changes: 11 additions & 0 deletions module/Core/functions/functions.php
Original file line number Diff line number Diff line change
@@ -18,6 +18,7 @@
use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory;
use Shlinkio\Shlink\Common\Util\DateRange;
use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlMode;
use Shlinkio\Shlink\IpGeolocation\Model\Location;

use function array_keys;
use function array_map;
@@ -289,3 +290,13 @@ function ipAddressFromRequest(ServerRequestInterface $request): string|null
{
return $request->getAttribute(IpAddressMiddlewareFactory::REQUEST_ATTR);
}

function geolocationFromRequest(ServerRequestInterface $request): Location|null
{
$geolocation = $request->getAttribute(Location::class);
if ($geolocation !== null && ! $geolocation instanceof Location) {
// TODO Throw exception
}

return $geolocation;
}
8 changes: 2 additions & 6 deletions module/Core/src/Action/AbstractTrackingAction.php
Original file line number Diff line number Diff line change
@@ -15,7 +15,6 @@
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 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface

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);
}
8 changes: 8 additions & 0 deletions module/Core/src/Config/Options/TrackingOptions.php
Original file line number Diff line number Diff line change
@@ -59,4 +59,12 @@ public function queryHasDisableTrackParam(array $query): bool
{
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;
}
}
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@
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 @@ public function __construct(
) {
}

public function __invoke(VisitLocated $visitLocated): void
public function __invoke(UrlVisited $visitLocated): void
{
if (! $this->isEnabled()) {
return;
27 changes: 0 additions & 27 deletions module/Core/src/EventDispatcher/Event/AbstractVisitEvent.php

This file was deleted.

4 changes: 2 additions & 2 deletions module/Core/src/EventDispatcher/Event/ShortUrlCreated.php
Original file line number Diff line number Diff line change
@@ -7,9 +7,9 @@
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)
{
}

20 changes: 19 additions & 1 deletion module/Core/src/EventDispatcher/Event/UrlVisited.php
Original file line number Diff line number Diff line change
@@ -4,6 +4,24 @@

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

Check warning on line 18 in module/Core/src/EventDispatcher/Event/UrlVisited.php

Codecov / codecov/patch

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

Added line #L18 was not covered by tests
{
return ['visitId' => $this->visitId, 'originalIpAddress' => $this->originalIpAddress];

Check warning on line 20 in module/Core/src/EventDispatcher/Event/UrlVisited.php

Codecov / codecov/patch

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

Added line #L20 was not covered by tests
}

public static function fromPayload(array $payload): self

Check warning on line 23 in module/Core/src/EventDispatcher/Event/UrlVisited.php

Codecov / codecov/patch

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

Added line #L23 was not covered by tests
{
return new self($payload['visitId'] ?? '', $payload['originalIpAddress'] ?? null);

Check warning on line 25 in module/Core/src/EventDispatcher/Event/UrlVisited.php

Codecov / codecov/patch

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

Added line #L25 was not covered by tests
}
}
9 changes: 0 additions & 9 deletions module/Core/src/EventDispatcher/Event/VisitLocated.php

This file was deleted.

Loading

0 comments on commit 178a99b

Please sign in to comment.