OilfieldToolsNet/shlink
4
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2024-11-21 16:38:37 -06:00
Code Issues Packages Projects Releases Wiki Activity

Refactor Visitor model and allow a Location object to be passed to it

Browse Source
This commit is contained in:
Alejandro Celaya 2024-11-14 14:48:18 +01:00
parent 1fee745786
commit 4a0b7e3fc9
17 changed files with 131 additions and 87 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
module/CLI/src/GeoLite/GeolocationDbUpdater.php
View File

@ -44,7 +44,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface
callable|null $beforeDownload = null,
callable|null $handleProgress = null,
): GeolocationResult {
if ($this->trackingOptions->disableTracking || $this->trackingOptions->disableIpTracking) {
if (! $this->trackingOptions->isGeolocationRelevant()) {
return GeolocationResult::CHECK_SKIPPED;
}

2
module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php
View File

@ -40,7 +40,7 @@ class GetDomainVisitsCommandTest extends TestCase
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';

2
module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php
View File

@ -93,7 +93,7 @@ class GetShortUrlVisitsCommandTest extends TestCase
#[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
module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php
View File

@ -40,7 +40,7 @@ class GetTagVisitsCommandTest extends TestCase
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';

2
module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php
View File

@ -40,7 +40,7 @@ class GetNonOrphanVisitsCommandTest extends TestCase
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(

2
module/CLI/test/Command/Visit/GetOrphanVisitsCommandTest.php
View File

@ -37,7 +37,7 @@ class GetOrphanVisitsCommandTest extends TestCase
#[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
module/CLI/test/Command/Visit/LocateVisitsCommandTest.php
View File

@ -63,8 +63,8 @@ class LocateVisitsCommandTest extends TestCase
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 @@ class LocateVisitsCommandTest extends TestCase
#[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);

6
module/Core/functions/functions.php
View File

@ -18,6 +18,7 @@ use Psr\Http\Message\ServerRequestInterface;
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,8 @@ function ipAddressFromRequest(ServerRequestInterface $request): string|null
{
return $request->getAttribute(IpAddressMiddlewareFactory::REQUEST_ATTR);
}
function geolocationFromRequest(ServerRequestInterface $request): Location|null
{
return $request->getAttribute(Location::class);
}

8
module/Core/src/Config/Options/TrackingOptions.php
View File

@ -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;
}
}

2
module/Core/src/Visit/Entity/Visit.php
View File

@ -64,7 +64,7 @@ class Visit extends AbstractEntity implements JsonSerializable
type: $type,
userAgent: $visitor->userAgent,
referer: $visitor->referer,
potentialBot: $visitor->isPotentialBot(),
potentialBot: $visitor->potentialBot,
remoteAddr: self::processAddress($visitor->remoteAddress, $anonymize),
visitedUrl: $visitor->visitedUrl,
);

87
module/Core/src/Visit/Model/Visitor.php
View File

@ -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
$instance->potentialBot = $this->potentialBot;
return $instance;
potentialBot: $this->potentialBot,
geolocation: $this->geolocation,
);
}
}

30
module/Core/test/EventDispatcher/LocateVisitTest.php
View File

@ -72,7 +72,7 @@ class LocateVisitTest extends TestCase
{
$event = new UrlVisited('123');
$this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn(
Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')),
Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')),
);
$this->em->expects($this->never())->method('flush');
$this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(false);
@ -91,7 +91,7 @@ class LocateVisitTest extends TestCase
{
$event = new UrlVisited('123');
$this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn(
Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')),
Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')),
);
$this->em->expects($this->never())->method('flush');
$this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true);
@ -112,7 +112,7 @@ class LocateVisitTest extends TestCase
{
$event = new UrlVisited('123');
$this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn(
Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')),
Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')),
);
$this->em->expects($this->never())->method('flush');
$this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true);
@ -149,9 +149,11 @@ class LocateVisitTest extends TestCase
{
$shortUrl = ShortUrl::createFake();
yield 'null IP' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', null, ''))];
yield 'empty IP' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', '', ''))];
yield 'localhost' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', IpAddress::LOCALHOST, ''))];
yield 'null IP' => [Visit::forValidShortUrl($shortUrl, Visitor::empty())];
yield 'empty IP' => [Visit::forValidShortUrl($shortUrl, Visitor::fromParams(remoteAddress: ''))];
yield 'localhost' => [
Visit::forValidShortUrl($shortUrl, Visitor::fromParams(remoteAddress: IpAddress::LOCALHOST)),
];
}
#[Test, DataProvider('provideIpAddresses')]
@ -181,15 +183,21 @@ class LocateVisitTest extends TestCase
public static function provideIpAddresses(): iterable
{
yield 'no original IP address' => [
Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')),
Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')),
null,
];
yield 'original IP address' => [
Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')),
Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')),
'1.2.3.4',
];
yield 'base url' => [Visit::forBasePath(Visitor::fromParams(remoteAddress: '1.2.3.4')), '1.2.3.4'];
yield 'invalid short url' => [
Visit::forInvalidShortUrl(Visitor::fromParams(remoteAddress: '1.2.3.4')),
'1.2.3.4',
];
yield 'regular not found' => [
Visit::forRegularNotFound(Visitor::fromParams(remoteAddress: '1.2.3.4')),
'1.2.3.4',
];
yield 'base url' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4'];
yield 'invalid short url' => [Visit::forInvalidShortUrl(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4'];
yield 'regular not found' => [Visit::forRegularNotFound(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4'];
}
}

4
module/Core/test/Matomo/MatomoVisitSenderTest.php
View File

@ -92,7 +92,7 @@ class MatomoVisitSenderTest extends TestCase
'1.2.3.4',
['setCity', 'setCountry', 'setLatitude', 'setLongitude', 'setIp'],
];
yield 'fallback IP' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), null, ['setIp']];
yield 'fallback IP' => [Visit::forBasePath(Visitor::fromParams(remoteAddress: '1.2.3.4')), null, ['setIp']];
}
#[Test, DataProvider('provideUrlsToTrack')]
@ -117,7 +117,7 @@ class MatomoVisitSenderTest extends TestCase
{
yield 'orphan visit without visited URL' => [Visit::forBasePath(Visitor::empty()), ''];
yield 'orphan visit with visited URL' => [
Visit::forBasePath(new Visitor('', '', null, 'https://s.test/foo')),
Visit::forBasePath(Visitor::fromParams(visitedUrl: 'https://s.test/foo')),
'https://s.test/foo',
];
yield 'non-orphan visit' => [

7
module/Core/test/Visit/Entity/VisitTest.php
View File

@ -22,7 +22,10 @@ class VisitTest extends TestCase
#[Test, DataProvider('provideUserAgents')]
public function isProperlyJsonSerialized(string $userAgent, bool $expectedToBePotentialBot): void
{
$visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor($userAgent, 'some site', '1.2.3.4', ''));
$visit = Visit::forValidShortUrl(
ShortUrl::createFake(),
Visitor::fromParams($userAgent, 'some site', '1.2.3.4'),
);
self::assertEquals([
'referer' => 'some site',
@ -110,7 +113,7 @@ class VisitTest extends TestCase
): void {
$visit = Visit::forValidShortUrl(
ShortUrl::createFake(),
new Visitor('Chrome', 'some site', $address, ''),
Visitor::fromParams('Chrome', 'some site', $address),
$anonymize,
);

4
module/Core/test/Visit/Geolocation/VisitToLocationHelperTest.php
View File

@ -42,7 +42,7 @@ class VisitToLocationHelperTest extends TestCase
{
yield [Visit::forBasePath(Visitor::empty()), IpCannotBeLocatedException::forEmptyAddress()];
yield [
Visit::forBasePath(new Visitor('foo', 'bar', IpAddress::LOCALHOST, '')),
Visit::forBasePath(Visitor::fromParams('foo', 'bar', IpAddress::LOCALHOST)),
IpCannotBeLocatedException::forLocalhost(),
];
}
@ -55,6 +55,6 @@ class VisitToLocationHelperTest extends TestCase
$this->expectExceptionObject(IpCannotBeLocatedException::forError($e));
$this->ipLocationResolver->expects($this->once())->method('resolveIpLocation')->willThrowException($e);
$this->helper->resolveVisitLocation(Visit::forBasePath(new Visitor('foo', 'bar', '1.2.3.4', '')));
$this->helper->resolveVisitLocation(Visit::forBasePath(Visitor::fromParams('foo', 'bar', '1.2.3.4')));
}
}

4
module/Core/test/Visit/Model/VisitorTest.php
View File

@ -20,7 +20,7 @@ class VisitorTest extends TestCase
#[Test, DataProvider('provideParams')]
public function providedFieldsValuesAreCropped(array $params, array $expected): void
{
$visitor = new Visitor(...$params);
$visitor = Visitor::fromParams(...$params);
['userAgent' => $userAgent, 'referer' => $referer, 'remoteAddress' => $remoteAddress] = $expected;
self::assertEquals($userAgent, $visitor->userAgent);
@ -75,7 +75,7 @@ class VisitorTest extends TestCase
#[Test]
public function newNormalizedInstanceIsCreatedFromTrackingOptions(): void
{
$visitor = new Visitor(
$visitor = Visitor::fromParams(
self::generateRandomString(2000),
self::generateRandomString(2000),
self::generateRandomString(2000),

48
module/Rest/test-api/Fixtures/VisitsFixture.php
View File

@ -23,43 +23,55 @@ class VisitsFixture extends AbstractFixture implements DependentFixtureInterface
{
/** @var ShortUrl $abcShortUrl */
$abcShortUrl = $this->getReference('abc123_short_url');
$manager->persist(
Visit::forValidShortUrl($abcShortUrl, new Visitor('shlink-tests-agent', '', '44.55.66.77', '')),
);
$manager->persist(Visit::forValidShortUrl(
$abcShortUrl,
new Visitor('shlink-tests-agent', 'https://google.com', '4.5.6.7', ''),
Visitor::fromParams(userAgent: 'shlink-tests-agent', remoteAddress: '44.55.66.77'),
));
$manager->persist(Visit::forValidShortUrl(
$abcShortUrl,
Visitor::fromParams('shlink-tests-agent', 'https://google.com', '4.5.6.7'),
));
$manager->persist(Visit::forValidShortUrl(
$abcShortUrl,
Visitor::fromParams(userAgent: 'shlink-tests-agent', remoteAddress: '1.2.3.4'),
));
$manager->persist(Visit::forValidShortUrl($abcShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4', '')));
/** @var ShortUrl $defShortUrl */
$defShortUrl = $this->getReference('def456_short_url');
$manager->persist(
Visit::forValidShortUrl($defShortUrl, new Visitor('cf-facebook', '', '127.0.0.1', '')),
);
$manager->persist(
Visit::forValidShortUrl($defShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')),
);
$manager->persist(Visit::forValidShortUrl(
$defShortUrl,
Visitor::fromParams(userAgent: 'cf-facebook', remoteAddress: '127.0.0.1'),
));
$manager->persist(Visit::forValidShortUrl(
$defShortUrl,
Visitor::fromParams('shlink-tests-agent', 'https://app.shlink.io', ''),
));
/** @var ShortUrl $ghiShortUrl */
$ghiShortUrl = $this->getReference('ghi789_short_url');
$manager->persist(Visit::forValidShortUrl($ghiShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4', '')));
$manager->persist(
Visit::forValidShortUrl($ghiShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')),
);
$manager->persist(Visit::forValidShortUrl(
$ghiShortUrl,
Visitor::fromParams(userAgent: 'shlink-tests-agent', remoteAddress: '1.2.3.4'),
));
$manager->persist(Visit::forValidShortUrl(
$ghiShortUrl,
Visitor::fromParams('shlink-tests-agent', 'https://app.shlink.io', ''),
));
$manager->persist($this->setVisitDate(
fn () => Visit::forBasePath(new Visitor('shlink-tests-agent', 'https://s.test', '1.2.3.4', '')),
fn () => Visit::forBasePath(Visitor::fromParams('shlink-tests-agent', 'https://s.test', '1.2.3.4')),
'2020-01-01',
));
$manager->persist($this->setVisitDate(
fn () => Visit::forRegularNotFound(
new Visitor('shlink-tests-agent', 'https://s.test/foo/bar', '1.2.3.4', ''),
Visitor::fromParams('shlink-tests-agent', 'https://s.test/foo/bar', '1.2.3.4'),
),
'2020-02-01',
));
$manager->persist($this->setVisitDate(
fn () => Visit::forInvalidShortUrl(new Visitor('cf-facebook', 'https://s.test/foo', '1.2.3.4', 'foo.com')),
fn () => Visit::forInvalidShortUrl(
Visitor::fromParams('cf-facebook', 'https://s.test/foo', '1.2.3.4', 'foo.com'),
),
'2020-03-01',
));