From a1fb44f2a61d37863b02108c8255f47d6b28f1dd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Feb 2021 10:56:22 +0100 Subject: [PATCH 01/28] Added ADR for not-found visits tracking --- ...e-url-invalid-short-url-and-regular-404.md | 33 +++++++++++++++++++ docs/adr/README.md | 1 + 2 files changed, 34 insertions(+) create mode 100644 docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md diff --git a/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md b/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md new file mode 100644 index 00000000..032820ef --- /dev/null +++ b/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md @@ -0,0 +1,33 @@ +# Track visits to 'base_url', 'invalid_short_url' and 'regular_404' + +* Status: Accepted +* Date: 2021-02-07 + +## Context and problem statement + +Shlink has the mechanism to return either custom errors or custom redirects when visiting the instance's base URL, an invalid short URL, or any other kind of URL that would result in a "Not found" error. + +However, it does not track visits to any of those, just to valid short URLs. + +The intention is to change that, and allow users to track the cases mentioned above. + +## Considered option + +* Create a new table to track visits o this kind. +* Reuse the existing `visits` table, by making `short_url_id` nullable and adding a couple of other fields. + +## Decision outcome + +*TBD* + +## Pros and Cons of the Options + +### New table + +* Good because we don't touch existing models and tables, reducing the risk to introduce a backwards compatibility break. +* Bad because we will have to repeat data modeling and logic, or refactor some components to support both contexts. This in turn increases the options to introduce a BC break. + +### Reuse existing table + +* Good because all the mechanisms in place to handle visits will work out of the box, including locating visits and such. +* Bad because we will have more optional properties, which means more double checks in many places. diff --git a/docs/adr/README.md b/docs/adr/README.md index 56df328f..93d82cff 100644 --- a/docs/adr/README.md +++ b/docs/adr/README.md @@ -2,4 +2,5 @@ Here listed you will find the different architectural decisions taken in the project, including all the reasoning behind it, options considered, and final outcome. +* [2021-02-07 Track visits to 'base_url', 'invalid_short_url' and 'regular_404'](2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md) * [2021-01-17 Support restrictions and permissions in API keys](2021-01-17-support-restrictions-and-permissions-in-api-keys.md) From 23cffce861fbd418c9ecaae30c0b5b5afda54885 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Feb 2021 11:02:50 +0100 Subject: [PATCH 02/28] Updated Visit entity so that the short URL is nullable --- module/Core/src/Entity/Visit.php | 11 ++--------- .../src/EventDispatcher/NotifyVisitToWebHooks.php | 4 ++-- module/Core/src/Mercure/MercureUpdatesGenerator.php | 2 +- 3 files changed, 5 insertions(+), 12 deletions(-) diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 7e6ed060..c2b79a04 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -18,7 +18,7 @@ class Visit extends AbstractEntity implements JsonSerializable private Chronos $date; private ?string $remoteAddr = null; private string $userAgent; - private ShortUrl $shortUrl; + private ?ShortUrl $shortUrl; private ?VisitLocation $visitLocation = null; public function __construct(ShortUrl $shortUrl, Visitor $visitor, bool $anonymize = true, ?Chronos $date = null) @@ -54,7 +54,7 @@ class Visit extends AbstractEntity implements JsonSerializable return ! empty($this->remoteAddr); } - public function getShortUrl(): ShortUrl + public function getShortUrl(): ?ShortUrl { return $this->shortUrl; } @@ -75,13 +75,6 @@ class Visit extends AbstractEntity implements JsonSerializable return $this; } - /** - * Specify data which should be serialized to JSON - * @link http://php.net/manual/en/jsonserializable.jsonserialize.php - * @return array data which can be serialized by json_encode, - * which is a value of any type other than a resource. - * @since 5.4.0 - */ public function jsonSerialize(): array { return [ diff --git a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php index d3b27602..b236a1c1 100644 --- a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php +++ b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php @@ -10,6 +10,7 @@ use Fig\Http\Message\RequestMethodInterface; use GuzzleHttp\ClientInterface; use GuzzleHttp\Promise\Promise; use GuzzleHttp\Promise\PromiseInterface; +use GuzzleHttp\Promise\Utils; use GuzzleHttp\RequestOptions; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; @@ -20,7 +21,6 @@ use Throwable; use function Functional\map; use function Functional\partial_left; -use function GuzzleHttp\Promise\settle; class NotifyVisitToWebHooks { @@ -69,7 +69,7 @@ class NotifyVisitToWebHooks $requestPromises = $this->performRequests($requestOptions, $visitId); // Wait for all the promises to finish, ignoring rejections, as in those cases we only want to log the error. - settle($requestPromises)->wait(); + Utils::settle($requestPromises)->wait(); } private function buildRequestOptions(Visit $visit): array diff --git a/module/Core/src/Mercure/MercureUpdatesGenerator.php b/module/Core/src/Mercure/MercureUpdatesGenerator.php index bd00e836..9a0a28f3 100644 --- a/module/Core/src/Mercure/MercureUpdatesGenerator.php +++ b/module/Core/src/Mercure/MercureUpdatesGenerator.php @@ -38,7 +38,7 @@ final class MercureUpdatesGenerator implements MercureUpdatesGeneratorInterface $topic = sprintf('%s/%s', self::NEW_VISIT_TOPIC, $shortUrl->getShortCode()); return new Update($topic, $this->serialize([ - 'shortUrl' => $this->transformer->transform($visit->getShortUrl()), + 'shortUrl' => $this->transformer->transform($shortUrl), 'visit' => $visit, ])); } From f5666c945138d6275a29f5c70a178cd0d1ffec2a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Feb 2021 11:26:01 +0100 Subject: [PATCH 03/28] Added new columns for extra tracking in visits table --- data/migrations/Version20210207100807.php | 53 +++++++++++++++++++ .../Shlinkio.Shlink.Core.Entity.Visit.php | 13 ++++- module/Core/src/Entity/Visit.php | 8 +++ module/Core/src/Model/Visitor.php | 1 + 4 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 data/migrations/Version20210207100807.php diff --git a/data/migrations/Version20210207100807.php b/data/migrations/Version20210207100807.php new file mode 100644 index 00000000..a74c0b08 --- /dev/null +++ b/data/migrations/Version20210207100807.php @@ -0,0 +1,53 @@ +getTable('visits'); + $shortUrlId = $visits->getColumn('short_url_id'); + + $this->skipIf(! $shortUrlId->getNotnull()); + + $shortUrlId->setNotnull(false); + + $visits->addColumn('visited_url', Types::STRING, [ + 'length' => Visitor::VISITED_URL_MAX_LENGTH, + 'notnull' => false, + ]); + $visits->addColumn('type', Types::STRING, [ + 'length' => 255, + 'default' => Visit::TYPE_VALID_SHORT_URL, + ]); + } + + public function down(Schema $schema): void + { + $visits = $schema->getTable('visits'); + $shortUrlId = $visits->getColumn('short_url_id'); + + $this->skipIf($shortUrlId->getNotnull()); + + $shortUrlId->setNotnull(true); + $visits->dropColumn('visited_url'); + $visits->dropColumn('type'); + } + + /** + * @fixme Workaround for https://github.com/doctrine/migrations/issues/1104 + */ + public function isTransactional(): bool + { + return false; + } +} diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php index 5143389b..efcccb65 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Entity.Visit.php @@ -47,11 +47,22 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->build(); $builder->createManyToOne('shortUrl', Entity\ShortUrl::class) - ->addJoinColumn('short_url_id', 'id', false, false, 'CASCADE') + ->addJoinColumn('short_url_id', 'id', true, false, 'CASCADE') ->build(); $builder->createManyToOne('visitLocation', Entity\VisitLocation::class) ->addJoinColumn('visit_location_id', 'id', true, false, 'Set NULL') ->cascadePersist() ->build(); + + $builder->createField('visitedUrl', Types::STRING) + ->columnName('visited_url') + ->length(Visitor::VISITED_URL_MAX_LENGTH) + ->nullable() + ->build(); + + $builder->createField('type', Types::STRING) + ->columnName('type') + ->length(255) + ->build(); }; diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index c2b79a04..5b1b91d3 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -14,10 +14,17 @@ use Shlinkio\Shlink\Core\Visit\Model\VisitLocationInterface; class Visit extends AbstractEntity implements JsonSerializable { + public const TYPE_VALID_SHORT_URL = 'valid_short_url'; + public const TYPE_INVALID_SHORT_URL = 'invalid_short_url'; + public const TYPE_BASE_URL = 'base_url'; + public const TYPE_REGULAR_404 = 'regular_404'; + private string $referer; private Chronos $date; private ?string $remoteAddr = null; + private ?string $visitedUrl = null; private string $userAgent; + private string $type; private ?ShortUrl $shortUrl; private ?VisitLocation $visitLocation = null; @@ -28,6 +35,7 @@ class Visit extends AbstractEntity implements JsonSerializable $this->userAgent = $visitor->getUserAgent(); $this->referer = $visitor->getReferer(); $this->remoteAddr = $this->processAddress($anonymize, $visitor->getRemoteAddress()); + $this->type = self::TYPE_VALID_SHORT_URL; } private function processAddress(bool $anonymize, ?string $address): ?string diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php index 8c24ab26..f973be49 100644 --- a/module/Core/src/Model/Visitor.php +++ b/module/Core/src/Model/Visitor.php @@ -14,6 +14,7 @@ final 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; private string $userAgent; private string $referer; From 12b07bb0ac977023b562c6ef90cfb1e607a19f4d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 7 Feb 2021 21:31:12 +0100 Subject: [PATCH 04/28] Created named constructors for Visit entity and added tracking of the visited URL --- .../Command/ShortUrl/GetVisitsCommandTest.php | 2 +- .../Command/Visit/LocateVisitsCommandTest.php | 6 ++-- module/Core/src/Entity/Visit.php | 36 ++++++++++++++++--- module/Core/src/Model/Visitor.php | 12 +++++-- module/Core/src/Service/VisitsTracker.php | 2 +- .../Repository/ShortUrlRepositoryTest.php | 2 +- .../test-db/Repository/TagRepositoryTest.php | 8 ++--- .../Repository/VisitRepositoryTest.php | 2 +- module/Core/test/Entity/VisitTest.php | 24 +++++-------- .../LocateShortUrlVisitTest.php | 14 ++++---- .../NotifyVisitToMercureTest.php | 4 +-- .../NotifyVisitToWebHooksTest.php | 2 +- .../Mercure/MercureUpdatesGeneratorTest.php | 2 +- module/Core/test/Model/VisitorTest.php | 5 +-- .../ShortUrl/DeleteShortUrlServiceTest.php | 2 +- .../Service/ShortUrl/ShortUrlResolverTest.php | 4 +-- .../Core/test/Service/VisitsTrackerTest.php | 4 +-- module/Core/test/Visit/VisitLocatorTest.php | 5 +-- .../Rest/test-api/Fixtures/VisitsFixture.php | 25 +++++++++---- 19 files changed, 101 insertions(+), 60 deletions(-) diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index 3da492e3..4d5e5832 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -103,7 +103,7 @@ class GetVisitsCommandTest extends TestCase $shortCode = 'abc123'; $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), Argument::any())->willReturn( new Paginator(new ArrayAdapter([ - (new Visit(ShortUrl::createEmpty(), new Visitor('bar', 'foo', '')))->locate( + Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('bar', 'foo', '', ''))->locate( new VisitLocation(new Location('', 'Spain', '', '', 0, 0, '')), ), ])), diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index 5ba0778a..236fac50 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -77,7 +77,7 @@ class LocateVisitsCommandTest extends TestCase bool $expectWarningPrint, array $args ): void { - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4')); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')); $location = new VisitLocation(Location::emptyInstance()); $mockMethodBehavior = $this->invokeHelperMethods($visit, $location); @@ -121,7 +121,7 @@ class LocateVisitsCommandTest extends TestCase */ public function localhostAndEmptyAddressesAreIgnored(?string $address, string $message): void { - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', $address)); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $address, '')); $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( @@ -154,7 +154,7 @@ class LocateVisitsCommandTest extends TestCase /** @test */ public function errorWhileLocatingIpIsDisplayed(): void { - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4')); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')); $location = new VisitLocation(Location::emptyInstance()); $locateVisits = $this->visitService->locateUnlocatedVisits(Argument::cetera())->will( diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 5b1b91d3..d61e8af6 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -21,21 +21,27 @@ class Visit extends AbstractEntity implements JsonSerializable private string $referer; private Chronos $date; - private ?string $remoteAddr = null; - private ?string $visitedUrl = null; + private ?string $remoteAddr; + private ?string $visitedUrl; private string $userAgent; private string $type; private ?ShortUrl $shortUrl; private ?VisitLocation $visitLocation = null; - public function __construct(ShortUrl $shortUrl, Visitor $visitor, bool $anonymize = true, ?Chronos $date = null) - { + public function __construct( + ?ShortUrl $shortUrl, + Visitor $visitor, + bool $anonymize = true, + ?Chronos $date = null, + string $type = self::TYPE_VALID_SHORT_URL + ) { $this->shortUrl = $shortUrl; $this->date = $date ?? Chronos::now(); $this->userAgent = $visitor->getUserAgent(); $this->referer = $visitor->getReferer(); $this->remoteAddr = $this->processAddress($anonymize, $visitor->getRemoteAddress()); - $this->type = self::TYPE_VALID_SHORT_URL; + $this->visitedUrl = $visitor->getVisitedUrl(); + $this->type = $type; } private function processAddress(bool $anonymize, ?string $address): ?string @@ -52,6 +58,26 @@ class Visit extends AbstractEntity implements JsonSerializable } } + public static function forValidShortUrl(ShortUrl $shortUrl, Visitor $visitor, bool $anonymize = true): self + { + return new self($shortUrl, $visitor, $anonymize); + } + + public static function forBasePath(Visitor $visitor, bool $anonymize = true): self + { + return new self(null, $visitor, $anonymize, null, self::TYPE_BASE_URL); + } + + public static function forInvalidShortUrl(Visitor $visitor, bool $anonymize = true): self + { + return new self(null, $visitor, $anonymize, null, self::TYPE_INVALID_SHORT_URL); + } + + public static function forRegularNotFound(Visitor $visitor, bool $anonymize = true): self + { + return new self(null, $visitor, $anonymize, null, self::TYPE_REGULAR_404); + } + public function getRemoteAddr(): ?string { return $this->remoteAddr; diff --git a/module/Core/src/Model/Visitor.php b/module/Core/src/Model/Visitor.php index f973be49..7438bdce 100644 --- a/module/Core/src/Model/Visitor.php +++ b/module/Core/src/Model/Visitor.php @@ -18,12 +18,14 @@ final class Visitor private string $userAgent; private string $referer; + private string $visitedUrl; private ?string $remoteAddress; - public function __construct(string $userAgent, string $referer, ?string $remoteAddress) + public function __construct(string $userAgent, string $referer, ?string $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 = $this->cropToLength($remoteAddress, self::REMOTE_ADDRESS_MAX_LENGTH); } @@ -38,12 +40,13 @@ final class Visitor $request->getHeaderLine('User-Agent'), $request->getHeaderLine('Referer'), $request->getAttribute(IpAddressMiddlewareFactory::REQUEST_ATTR), + $request->getUri()->__toString(), ); } public static function emptyInstance(): self { - return new self('', '', null); + return new self('', '', null, ''); } public function getUserAgent(): string @@ -60,4 +63,9 @@ final class Visitor { return $this->remoteAddress; } + + public function getVisitedUrl(): string + { + return $this->visitedUrl; + } } diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index a8362f7c..bda33a01 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -41,7 +41,7 @@ class VisitsTracker implements VisitsTrackerInterface public function track(ShortUrl $shortUrl, Visitor $visitor): void { - $visit = new Visit($shortUrl, $visitor, $this->anonymizeRemoteAddr); + $visit = Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr); $this->em->persist($visit); $this->em->flush(); diff --git a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php index 29694867..48381857 100644 --- a/module/Core/test-db/Repository/ShortUrlRepositoryTest.php +++ b/module/Core/test-db/Repository/ShortUrlRepositoryTest.php @@ -95,7 +95,7 @@ class ShortUrlRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($foo); $bar = ShortUrl::withLongUrl('bar'); - $visit = new Visit($bar, Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl($bar, Visitor::emptyInstance()); $this->getEntityManager()->persist($visit); $bar->setVisits(new ArrayCollection([$visit])); $this->getEntityManager()->persist($bar); diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index 0a91775b..34a06a40 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -64,13 +64,13 @@ class TagRepositoryTest extends DatabaseTestCase $shortUrl = ShortUrl::fromMeta($metaWithTags($firstUrlTags), $this->relationResolver); $this->getEntityManager()->persist($shortUrl); - $this->getEntityManager()->persist(new Visit($shortUrl, Visitor::emptyInstance())); - $this->getEntityManager()->persist(new Visit($shortUrl, Visitor::emptyInstance())); - $this->getEntityManager()->persist(new Visit($shortUrl, Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance())); $shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags), $this->relationResolver); $this->getEntityManager()->persist($shortUrl2); - $this->getEntityManager()->persist(new Visit($shortUrl2, Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl2, Visitor::emptyInstance())); $this->getEntityManager()->flush(); $result = $this->repo->findTagsWithInfo(); diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 740edff5..5681ee26 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -52,7 +52,7 @@ class VisitRepositoryTest extends DatabaseTestCase }; for ($i = 0; $i < 6; $i++) { - $visit = new Visit($shortUrl, Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); if ($i >= 2) { $location = new VisitLocation(Location::emptyInstance()); diff --git a/module/Core/test/Entity/VisitTest.php b/module/Core/test/Entity/VisitTest.php index d583c799..7be3c3fc 100644 --- a/module/Core/test/Entity/VisitTest.php +++ b/module/Core/test/Entity/VisitTest.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Entity; -use Cake\Chronos\Chronos; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\ShortUrl; @@ -13,35 +12,30 @@ use Shlinkio\Shlink\Core\Model\Visitor; class VisitTest extends TestCase { - /** - * @test - * @dataProvider provideDates - */ - public function isProperlyJsonSerialized(?Chronos $date): void + /** @test */ + public function isProperlyJsonSerialized(): void { - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('Chrome', 'some site', '1.2.3.4'), true, $date); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('Chrome', 'some site', '1.2.3.4', '')); self::assertEquals([ 'referer' => 'some site', - 'date' => ($date ?? $visit->getDate())->toAtomString(), + 'date' => $visit->getDate()->toAtomString(), 'userAgent' => 'Chrome', 'visitLocation' => null, ], $visit->jsonSerialize()); } - public function provideDates(): iterable - { - yield 'null date' => [null]; - yield 'not null date' => [Chronos::now()->subDays(10)]; - } - /** * @test * @dataProvider provideAddresses */ public function addressIsAnonymizedWhenRequested(bool $anonymize, ?string $address, ?string $expectedAddress): void { - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('Chrome', 'some site', $address), $anonymize); + $visit = Visit::forValidShortUrl( + ShortUrl::createEmpty(), + new Visitor('Chrome', 'some site', $address, ''), + $anonymize, + ); self::assertEquals($expectedAddress, $visit->getRemoteAddr()); } diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php index 4d348528..03eef5f6 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php @@ -78,7 +78,7 @@ class LocateShortUrlVisitTest extends TestCase { $event = new ShortUrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn( - new Visit(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4')), + Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), ); $resolveLocation = $this->ipLocationResolver->resolveIpLocation(Argument::cetera())->willThrow( WrongIpException::class, @@ -127,9 +127,9 @@ class LocateShortUrlVisitTest extends TestCase { $shortUrl = ShortUrl::createEmpty(); - yield 'null IP' => [new Visit($shortUrl, new Visitor('', '', null))]; - yield 'empty IP' => [new Visit($shortUrl, new Visitor('', '', ''))]; - yield 'localhost' => [new Visit($shortUrl, new Visitor('', '', IpAddress::LOCALHOST))]; + 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, ''))]; } /** @@ -139,7 +139,7 @@ class LocateShortUrlVisitTest extends TestCase public function locatableVisitsResolveToLocation(string $anonymizedIpAddress, ?string $originalIpAddress): void { $ipAddr = $originalIpAddress ?? $anonymizedIpAddress; - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr)); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); $event = new ShortUrlVisited('123', $originalIpAddress); @@ -171,7 +171,7 @@ class LocateShortUrlVisitTest extends TestCase { $e = GeolocationDbUpdateFailedException::withOlderDb(); $ipAddr = '1.2.3.0'; - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr)); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); $event = new ShortUrlVisited('123'); @@ -202,7 +202,7 @@ class LocateShortUrlVisitTest extends TestCase { $e = GeolocationDbUpdateFailedException::withoutOlderDb(); $ipAddr = '1.2.3.0'; - $visit = new Visit(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr)); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); $event = new ShortUrlVisited('123'); diff --git a/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php b/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php index f5b525ea..1180d05c 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php @@ -77,7 +77,7 @@ class NotifyVisitToMercureTest extends TestCase public function notificationsAreSentWhenVisitIsFound(): void { $visitId = '123'; - $visit = new Visit(ShortUrl::createEmpty(), Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()); $update = new Update('', ''); $findVisit = $this->em->find(Visit::class, $visitId)->willReturn($visit); @@ -101,7 +101,7 @@ class NotifyVisitToMercureTest extends TestCase public function debugIsLoggedWhenExceptionIsThrown(): void { $visitId = '123'; - $visit = new Visit(ShortUrl::createEmpty(), Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()); $update = new Update('', ''); $e = new RuntimeException('Error'); diff --git a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php index 9599c2c8..fcd97d2d 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php @@ -82,7 +82,7 @@ class NotifyVisitToWebHooksTest extends TestCase $invalidWebhooks = ['invalid', 'baz']; $find = $this->em->find(Visit::class, '1')->willReturn( - new Visit(ShortUrl::createEmpty(), Visitor::emptyInstance()), + Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()), ); $requestAsync = $this->httpClient->requestAsync( RequestMethodInterface::METHOD_POST, diff --git a/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php b/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php index 435fb4d8..9e4b418e 100644 --- a/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php +++ b/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php @@ -35,7 +35,7 @@ class MercureUpdatesGeneratorTest extends TestCase 'longUrl' => '', 'title' => $title, ])); - $visit = new Visit($shortUrl, Visitor::emptyInstance()); + $visit = Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()); $update = $this->generator->{$method}($visit); diff --git a/module/Core/test/Model/VisitorTest.php b/module/Core/test/Model/VisitorTest.php index d52a6389..e1003056 100644 --- a/module/Core/test/Model/VisitorTest.php +++ b/module/Core/test/Model/VisitorTest.php @@ -31,7 +31,7 @@ class VisitorTest extends TestCase public function provideParams(): iterable { yield 'all values are bigger' => [ - [str_repeat('a', 1000), str_repeat('b', 2000), str_repeat('c', 500)], + [str_repeat('a', 1000), str_repeat('b', 2000), str_repeat('c', 500), ''], [ 'userAgent' => str_repeat('a', Visitor::USER_AGENT_MAX_LENGTH), 'referer' => str_repeat('b', Visitor::REFERER_MAX_LENGTH), @@ -39,7 +39,7 @@ class VisitorTest extends TestCase ], ]; yield 'some values are smaller' => [ - [str_repeat('a', 10), str_repeat('b', 2000), null], + [str_repeat('a', 10), str_repeat('b', 2000), null, ''], [ 'userAgent' => str_repeat('a', 10), 'referer' => str_repeat('b', Visitor::REFERER_MAX_LENGTH), @@ -51,6 +51,7 @@ class VisitorTest extends TestCase $userAgent = $this->generateRandomString(2000), $referer = $this->generateRandomString(50), null, + '', ], [ 'userAgent' => substr($userAgent, 0, Visitor::USER_AGENT_MAX_LENGTH), diff --git a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php index 9e2ca4ec..4c066848 100644 --- a/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php +++ b/module/Core/test/Service/ShortUrl/DeleteShortUrlServiceTest.php @@ -34,7 +34,7 @@ class DeleteShortUrlServiceTest extends TestCase public function setUp(): void { $shortUrl = ShortUrl::createEmpty()->setVisits(new ArrayCollection( - map(range(0, 10), fn () => new Visit(ShortUrl::createEmpty(), Visitor::emptyInstance())), + map(range(0, 10), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())), )); $this->shortCode = $shortUrl->getShortCode(); diff --git a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php index 038fe457..cf2330b3 100644 --- a/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/Service/ShortUrl/ShortUrlResolverTest.php @@ -121,7 +121,7 @@ class ShortUrlResolverTest extends TestCase $shortUrl = ShortUrl::fromMeta(ShortUrlMeta::fromRawData(['maxVisits' => 3, 'longUrl' => ''])); $shortUrl->setVisits(new ArrayCollection(map( range(0, 4), - fn () => new Visit($shortUrl, Visitor::emptyInstance()), + fn () => Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()), ))); return $shortUrl; @@ -140,7 +140,7 @@ class ShortUrlResolverTest extends TestCase ])); $shortUrl->setVisits(new ArrayCollection(map( range(0, 4), - fn () => new Visit($shortUrl, Visitor::emptyInstance()), + fn () => Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()), ))); return $shortUrl; diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 9b627a17..821db275 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -73,7 +73,7 @@ class VisitsTrackerTest extends TestCase $count = $repo->shortCodeIsInUse($shortCode, null, $spec)->willReturn(true); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - $list = map(range(0, 1), fn () => new Visit(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); $repo2 = $this->prophesize(VisitRepository::class); $repo2->findVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), 1, 0, $spec)->willReturn( $list, @@ -129,7 +129,7 @@ class VisitsTrackerTest extends TestCase $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); $spec = $apiKey === null ? null : $apiKey->spec(); - $list = map(range(0, 1), fn () => new Visit(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); $repo2 = $this->prophesize(VisitRepository::class); $repo2->findVisitsByTag($tag, Argument::type(DateRange::class), 1, 0, $spec)->willReturn($list); $repo2->countVisitsByTag($tag, Argument::type(DateRange::class), $spec)->willReturn(1); diff --git a/module/Core/test/Visit/VisitLocatorTest.php b/module/Core/test/Visit/VisitLocatorTest.php index 96caf968..c99d051b 100644 --- a/module/Core/test/Visit/VisitLocatorTest.php +++ b/module/Core/test/Visit/VisitLocatorTest.php @@ -57,7 +57,8 @@ class VisitLocatorTest extends TestCase ): void { $unlocatedVisits = map( range(1, 200), - fn (int $i) => new Visit(ShortUrl::withLongUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()), + fn (int $i) => + Visit::forValidShortUrl(ShortUrl::withLongUrl(sprintf('short_code_%s', $i)), Visitor::emptyInstance()), ); $findVisits = $this->mockRepoMethod($expectedRepoMethodName)->willReturn($unlocatedVisits); @@ -107,7 +108,7 @@ class VisitLocatorTest extends TestCase bool $isNonLocatableAddress ): void { $unlocatedVisits = [ - new Visit(ShortUrl::withLongUrl('foo'), Visitor::emptyInstance()), + Visit::forValidShortUrl(ShortUrl::withLongUrl('foo'), Visitor::emptyInstance()), ]; $findVisits = $this->mockRepoMethod($expectedRepoMethodName)->willReturn($unlocatedVisits); diff --git a/module/Rest/test-api/Fixtures/VisitsFixture.php b/module/Rest/test-api/Fixtures/VisitsFixture.php index 73601748..1e3b3a5c 100644 --- a/module/Rest/test-api/Fixtures/VisitsFixture.php +++ b/module/Rest/test-api/Fixtures/VisitsFixture.php @@ -22,19 +22,30 @@ class VisitsFixture extends AbstractFixture implements DependentFixtureInterface { /** @var ShortUrl $abcShortUrl */ $abcShortUrl = $this->getReference('abc123_short_url'); - $manager->persist(new Visit($abcShortUrl, new Visitor('shlink-tests-agent', '', '44.55.66.77'))); - $manager->persist(new Visit($abcShortUrl, new Visitor('shlink-tests-agent', 'https://google.com', '4.5.6.7'))); - $manager->persist(new Visit($abcShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4'))); + $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', ''), + )); + $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(new Visit($defShortUrl, new Visitor('shlink-tests-agent', '', '127.0.0.1'))); - $manager->persist(new Visit($defShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', ''))); + $manager->persist( + Visit::forValidShortUrl($defShortUrl, new Visitor('shlink-tests-agent', '', '127.0.0.1', '')), + ); + $manager->persist( + Visit::forValidShortUrl($defShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')), + ); /** @var ShortUrl $ghiShortUrl */ $ghiShortUrl = $this->getReference('ghi789_short_url'); - $manager->persist(new Visit($ghiShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4'))); - $manager->persist(new Visit($ghiShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', ''))); + $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->flush(); } From 1b4e62b823228ce473b4af87b3a4f1f2f9e385cd Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 19:46:51 +0100 Subject: [PATCH 05/28] Separated methods to track visits and list visits --- module/CLI/config/dependencies.config.php | 2 +- .../src/Command/ShortUrl/GetVisitsCommand.php | 13 ++- .../Command/ShortUrl/GetVisitsCommandTest.php | 20 ++-- module/Core/src/Service/VisitsTracker.php | 56 ----------- .../src/Service/VisitsTrackerInterface.php | 19 ---- module/Core/src/Visit/VisitsStatsHelper.php | 59 +++++++++++ .../src/Visit/VisitsStatsHelperInterface.php | 22 +++++ .../Core/test/Service/VisitsTrackerTest.php | 97 ------------------- .../Core/test/Visit/VisitsStatsHelperTest.php | 96 ++++++++++++++++++ module/Rest/config/dependencies.config.php | 4 +- .../src/Action/Visit/ShortUrlVisitsAction.php | 10 +- .../Rest/src/Action/Visit/TagVisitsAction.php | 10 +- .../Action/Visit/ShortUrlVisitsActionTest.php | 12 +-- .../test/Action/Visit/TagVisitsActionTest.php | 10 +- 14 files changed, 220 insertions(+), 210 deletions(-) diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 685dc9fd..7e224c33 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -74,7 +74,7 @@ return [ Service\ShortUrlService::class, ShortUrlDataTransformer::class, ], - Command\ShortUrl\GetVisitsCommand::class => [Service\VisitsTracker::class], + Command\ShortUrl\GetVisitsCommand::class => [Visit\VisitsStatsHelper::class], Command\ShortUrl\DeleteShortUrlCommand::class => [Service\ShortUrl\DeleteShortUrlService::class], Command\Visit\LocateVisitsCommand::class => [ diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index 0b7de663..7b020356 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -11,8 +11,8 @@ use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Core\Visit\Model\UnknownVisitLocation; +use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -27,11 +27,11 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand { public const NAME = 'short-url:visits'; - private VisitsTrackerInterface $visitsTracker; + private VisitsStatsHelperInterface $visitsHelper; - public function __construct(VisitsTrackerInterface $visitsTracker) + public function __construct(VisitsStatsHelperInterface $visitsHelper) { - $this->visitsTracker = $visitsTracker; + $this->visitsHelper = $visitsHelper; parent::__construct(); } @@ -74,7 +74,10 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand $startDate = $this->getStartDateOption($input, $output); $endDate = $this->getEndDateOption($input, $output); - $paginator = $this->visitsTracker->info($identifier, new VisitsParams(new DateRange($startDate, $endDate))); + $paginator = $this->visitsHelper->visitsForShortUrl( + $identifier, + new VisitsParams(new DateRange($startDate, $endDate)), + ); $rows = map($paginator->getCurrentPageResults(), function (Visit $visit) { $rowData = $visit->jsonSerialize(); diff --git a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php index 4d5e5832..d25d5763 100644 --- a/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetVisitsCommandTest.php @@ -19,7 +19,7 @@ use Shlinkio\Shlink\Core\Entity\VisitLocation; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Symfony\Component\Console\Application; use Symfony\Component\Console\Tester\CommandTester; @@ -31,12 +31,12 @@ class GetVisitsCommandTest extends TestCase use ProphecyTrait; private CommandTester $commandTester; - private ObjectProphecy $visitsTracker; + private ObjectProphecy $visitsHelper; public function setUp(): void { - $this->visitsTracker = $this->prophesize(VisitsTrackerInterface::class); - $command = new GetVisitsCommand($this->visitsTracker->reveal()); + $this->visitsHelper = $this->prophesize(VisitsStatsHelperInterface::class); + $command = new GetVisitsCommand($this->visitsHelper->reveal()); $app = new Application(); $app->add($command); $this->commandTester = new CommandTester($command); @@ -46,7 +46,7 @@ class GetVisitsCommandTest extends TestCase public function noDateFlagsTriesToListWithoutDateRange(): void { $shortCode = 'abc123'; - $this->visitsTracker->info( + $this->visitsHelper->visitsForShortUrl( new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange(null, null)), ) @@ -62,7 +62,7 @@ class GetVisitsCommandTest extends TestCase $shortCode = 'abc123'; $startDate = '2016-01-01'; $endDate = '2016-02-01'; - $this->visitsTracker->info( + $this->visitsHelper->visitsForShortUrl( new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange(Chronos::parse($startDate), Chronos::parse($endDate))), ) @@ -81,8 +81,10 @@ class GetVisitsCommandTest extends TestCase { $shortCode = 'abc123'; $startDate = 'foo'; - $info = $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams(new DateRange())) - ->willReturn(new Paginator(new ArrayAdapter([]))); + $info = $this->visitsHelper->visitsForShortUrl( + new ShortUrlIdentifier($shortCode), + new VisitsParams(new DateRange()), + )->willReturn(new Paginator(new ArrayAdapter([]))); $this->commandTester->execute([ 'shortCode' => $shortCode, @@ -101,7 +103,7 @@ class GetVisitsCommandTest extends TestCase public function outputIsProperlyGenerated(): void { $shortCode = 'abc123'; - $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), Argument::any())->willReturn( + $this->visitsHelper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), Argument::any())->willReturn( new Paginator(new ArrayAdapter([ Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('bar', 'foo', '', ''))->locate( new VisitLocation(new Location('', 'Spain', '', '', 0, 0, '')), diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Service/VisitsTracker.php index bda33a01..daf4aa24 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Service/VisitsTracker.php @@ -6,22 +6,10 @@ namespace Shlinkio\Shlink\Core\Service; use Doctrine\ORM; use Psr\EventDispatcher\EventDispatcherInterface; -use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; -use Shlinkio\Shlink\Core\Exception\TagNotFoundException; -use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsForTagPaginatorAdapter; -use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; -use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; -use Shlinkio\Shlink\Core\Repository\TagRepository; -use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface; -use Shlinkio\Shlink\Rest\Entity\ApiKey; class VisitsTracker implements VisitsTrackerInterface { @@ -48,48 +36,4 @@ class VisitsTracker implements VisitsTrackerInterface $this->eventDispatcher->dispatch(new ShortUrlVisited($visit->getId(), $visitor->getRemoteAddress())); } - - /** - * @return Visit[]|Paginator - * @throws ShortUrlNotFoundException - */ - public function info(ShortUrlIdentifier $identifier, VisitsParams $params, ?ApiKey $apiKey = null): Paginator - { - $spec = $apiKey !== null ? $apiKey->spec() : null; - - /** @var ShortUrlRepositoryInterface $repo */ - $repo = $this->em->getRepository(ShortUrl::class); - if (! $repo->shortCodeIsInUse($identifier->shortCode(), $identifier->domain(), $spec)) { - throw ShortUrlNotFoundException::fromNotFound($identifier); - } - - /** @var VisitRepositoryInterface $repo */ - $repo = $this->em->getRepository(Visit::class); - $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $identifier, $params, $spec)); - $paginator->setMaxPerPage($params->getItemsPerPage()) - ->setCurrentPage($params->getPage()); - - return $paginator; - } - - /** - * @return Visit[]|Paginator - * @throws TagNotFoundException - */ - public function visitsForTag(string $tag, VisitsParams $params, ?ApiKey $apiKey = null): Paginator - { - /** @var TagRepository $tagRepo */ - $tagRepo = $this->em->getRepository(Tag::class); - if (! $tagRepo->tagExists($tag, $apiKey)) { - throw TagNotFoundException::fromTag($tag); - } - - /** @var VisitRepositoryInterface $repo */ - $repo = $this->em->getRepository(Visit::class); - $paginator = new Paginator(new VisitsForTagPaginatorAdapter($repo, $tag, $params, $apiKey)); - $paginator->setMaxPerPage($params->getItemsPerPage()) - ->setCurrentPage($params->getPage()); - - return $paginator; - } } diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Service/VisitsTrackerInterface.php index 0814d986..c468cf0e 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Service/VisitsTrackerInterface.php @@ -4,29 +4,10 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Service; -use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Entity\Visit; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; -use Shlinkio\Shlink\Core\Exception\TagNotFoundException; -use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Rest\Entity\ApiKey; interface VisitsTrackerInterface { public function track(ShortUrl $shortUrl, Visitor $visitor): void; - - /** - * @return Visit[]|Paginator - * @throws ShortUrlNotFoundException - */ - public function info(ShortUrlIdentifier $identifier, VisitsParams $params, ?ApiKey $apiKey = null): Paginator; - - /** - * @return Visit[]|Paginator - * @throws TagNotFoundException - */ - public function visitsForTag(string $tag, VisitsParams $params, ?ApiKey $apiKey = null): Paginator; } diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index ab06079a..7c4efd23 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -5,8 +5,20 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Visit; use Doctrine\ORM\EntityManagerInterface; +use Shlinkio\Shlink\Common\Paginator\Paginator; +use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; +use Shlinkio\Shlink\Core\Model\VisitsParams; +use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsForTagPaginatorAdapter; +use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\Repository\VisitRepository; +use Shlinkio\Shlink\Core\Repository\VisitRepositoryInterface; use Shlinkio\Shlink\Core\Visit\Model\VisitsStats; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -30,4 +42,51 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface $visitsRepo = $this->em->getRepository(Visit::class); return $visitsRepo->countVisits($apiKey); } + + /** + * @return Visit[]|Paginator + * @throws ShortUrlNotFoundException + */ + public function visitsForShortUrl( + ShortUrlIdentifier $identifier, + VisitsParams $params, + ?ApiKey $apiKey = null + ): Paginator { + $spec = $apiKey !== null ? $apiKey->spec() : null; + + /** @var ShortUrlRepositoryInterface $repo */ + $repo = $this->em->getRepository(ShortUrl::class); + if (! $repo->shortCodeIsInUse($identifier->shortCode(), $identifier->domain(), $spec)) { + throw ShortUrlNotFoundException::fromNotFound($identifier); + } + + /** @var VisitRepositoryInterface $repo */ + $repo = $this->em->getRepository(Visit::class); + $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $identifier, $params, $spec)); + $paginator->setMaxPerPage($params->getItemsPerPage()) + ->setCurrentPage($params->getPage()); + + return $paginator; + } + + /** + * @return Visit[]|Paginator + * @throws TagNotFoundException + */ + public function visitsForTag(string $tag, VisitsParams $params, ?ApiKey $apiKey = null): Paginator + { + /** @var TagRepository $tagRepo */ + $tagRepo = $this->em->getRepository(Tag::class); + if (! $tagRepo->tagExists($tag, $apiKey)) { + throw TagNotFoundException::fromTag($tag); + } + + /** @var VisitRepositoryInterface $repo */ + $repo = $this->em->getRepository(Visit::class); + $paginator = new Paginator(new VisitsForTagPaginatorAdapter($repo, $tag, $params, $apiKey)); + $paginator->setMaxPerPage($params->getItemsPerPage()) + ->setCurrentPage($params->getPage()); + + return $paginator; + } } diff --git a/module/Core/src/Visit/VisitsStatsHelperInterface.php b/module/Core/src/Visit/VisitsStatsHelperInterface.php index ca044d4b..a67c8dcd 100644 --- a/module/Core/src/Visit/VisitsStatsHelperInterface.php +++ b/module/Core/src/Visit/VisitsStatsHelperInterface.php @@ -4,10 +4,32 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Visit; +use Shlinkio\Shlink\Common\Paginator\Paginator; +use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; +use Shlinkio\Shlink\Core\Model\VisitsParams; use Shlinkio\Shlink\Core\Visit\Model\VisitsStats; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface VisitsStatsHelperInterface { public function getVisitsStats(?ApiKey $apiKey = null): VisitsStats; + + /** + * @return Visit[]|Paginator + * @throws ShortUrlNotFoundException + */ + public function visitsForShortUrl( + ShortUrlIdentifier $identifier, + VisitsParams $params, + ?ApiKey $apiKey = null + ): Paginator; + + /** + * @return Visit[]|Paginator + * @throws TagNotFoundException + */ + public function visitsForTag(string $tag, VisitsParams $params, ?ApiKey $apiKey = null): Paginator; } diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Service/VisitsTrackerTest.php index 821db275..4807dc41 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Service/VisitsTrackerTest.php @@ -5,35 +5,19 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Service; use Doctrine\ORM\EntityManager; -use Laminas\Stdlib\ArrayUtils; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Psr\EventDispatcher\EventDispatcherInterface; -use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; -use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; -use Shlinkio\Shlink\Core\Exception\TagNotFoundException; -use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; -use Shlinkio\Shlink\Core\Repository\TagRepository; -use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Service\VisitsTracker; -use Shlinkio\Shlink\Rest\Entity\ApiKey; -use ShlinkioTest\Shlink\Core\Util\ApiKeyHelpersTrait; - -use function Functional\map; -use function range; class VisitsTrackerTest extends TestCase { - use ApiKeyHelpersTrait; use ProphecyTrait; private VisitsTracker $visitsTracker; @@ -60,85 +44,4 @@ class VisitsTrackerTest extends TestCase $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); } - - /** - * @test - * @dataProvider provideAdminApiKeys - */ - public function infoReturnsVisitsForCertainShortCode(?ApiKey $apiKey): void - { - $shortCode = '123ABC'; - $spec = $apiKey === null ? null : $apiKey->spec(); - $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse($shortCode, null, $spec)->willReturn(true); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - - $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); - $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), 1, 0, $spec)->willReturn( - $list, - ); - $repo2->countVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), $spec)->willReturn(1); - $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - - $paginator = $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams(), $apiKey); - - self::assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentPageResults())); - $count->shouldHaveBeenCalledOnce(); - } - - /** @test */ - public function throwsExceptionWhenRequestingVisitsForInvalidShortCode(): void - { - $shortCode = '123ABC'; - $repo = $this->prophesize(ShortUrlRepositoryInterface::class); - $count = $repo->shortCodeIsInUse($shortCode, null, null)->willReturn(false); - $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); - - $this->expectException(ShortUrlNotFoundException::class); - $count->shouldBeCalledOnce(); - - $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams()); - } - - /** @test */ - public function throwsExceptionWhenRequestingVisitsForInvalidTag(): void - { - $tag = 'foo'; - $apiKey = new ApiKey(); - $repo = $this->prophesize(TagRepository::class); - $tagExists = $repo->tagExists($tag, $apiKey)->willReturn(false); - $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); - - $this->expectException(TagNotFoundException::class); - $tagExists->shouldBeCalledOnce(); - $getRepo->shouldBeCalledOnce(); - - $this->visitsTracker->visitsForTag($tag, new VisitsParams(), $apiKey); - } - - /** - * @test - * @dataProvider provideAdminApiKeys - */ - public function visitsForTagAreReturnedAsExpected(?ApiKey $apiKey): void - { - $tag = 'foo'; - $repo = $this->prophesize(TagRepository::class); - $tagExists = $repo->tagExists($tag, $apiKey)->willReturn(true); - $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); - - $spec = $apiKey === null ? null : $apiKey->spec(); - $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); - $repo2 = $this->prophesize(VisitRepository::class); - $repo2->findVisitsByTag($tag, Argument::type(DateRange::class), 1, 0, $spec)->willReturn($list); - $repo2->countVisitsByTag($tag, Argument::type(DateRange::class), $spec)->willReturn(1); - $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); - - $paginator = $this->visitsTracker->visitsForTag($tag, new VisitsParams(), $apiKey); - - self::assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentPageResults())); - $tagExists->shouldHaveBeenCalledOnce(); - $getRepo->shouldHaveBeenCalledOnce(); - } } diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index cdc76bd4..20a39316 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -5,19 +5,34 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Visit; use Doctrine\ORM\EntityManagerInterface; +use Laminas\Stdlib\ArrayUtils; use PHPUnit\Framework\TestCase; +use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; +use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Entity\Visit; +use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; +use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; +use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Model\VisitsParams; +use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; +use Shlinkio\Shlink\Core\Repository\TagRepository; use Shlinkio\Shlink\Core\Repository\VisitRepository; use Shlinkio\Shlink\Core\Visit\Model\VisitsStats; use Shlinkio\Shlink\Core\Visit\VisitsStatsHelper; +use Shlinkio\Shlink\Rest\Entity\ApiKey; +use ShlinkioTest\Shlink\Core\Util\ApiKeyHelpersTrait; use function Functional\map; use function range; class VisitsStatsHelperTest extends TestCase { + use ApiKeyHelpersTrait; use ProphecyTrait; private VisitsStatsHelper $helper; @@ -50,4 +65,85 @@ class VisitsStatsHelperTest extends TestCase { return map(range(0, 50, 5), fn (int $value) => [$value]); } + + /** + * @test + * @dataProvider provideAdminApiKeys + */ + public function infoReturnsVisitsForCertainShortCode(?ApiKey $apiKey): void + { + $shortCode = '123ABC'; + $spec = $apiKey === null ? null : $apiKey->spec(); + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $count = $repo->shortCodeIsInUse($shortCode, null, $spec)->willReturn(true); + $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); + + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $repo2 = $this->prophesize(VisitRepository::class); + $repo2->findVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), 1, 0, $spec)->willReturn( + $list, + ); + $repo2->countVisitsByShortCode($shortCode, null, Argument::type(DateRange::class), $spec)->willReturn(1); + $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); + + $paginator = $this->helper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), new VisitsParams(), $apiKey); + + self::assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentPageResults())); + $count->shouldHaveBeenCalledOnce(); + } + + /** @test */ + public function throwsExceptionWhenRequestingVisitsForInvalidShortCode(): void + { + $shortCode = '123ABC'; + $repo = $this->prophesize(ShortUrlRepositoryInterface::class); + $count = $repo->shortCodeIsInUse($shortCode, null, null)->willReturn(false); + $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal())->shouldBeCalledOnce(); + + $this->expectException(ShortUrlNotFoundException::class); + $count->shouldBeCalledOnce(); + + $this->helper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), new VisitsParams()); + } + + /** @test */ + public function throwsExceptionWhenRequestingVisitsForInvalidTag(): void + { + $tag = 'foo'; + $apiKey = new ApiKey(); + $repo = $this->prophesize(TagRepository::class); + $tagExists = $repo->tagExists($tag, $apiKey)->willReturn(false); + $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); + + $this->expectException(TagNotFoundException::class); + $tagExists->shouldBeCalledOnce(); + $getRepo->shouldBeCalledOnce(); + + $this->helper->visitsForTag($tag, new VisitsParams(), $apiKey); + } + + /** + * @test + * @dataProvider provideAdminApiKeys + */ + public function visitsForTagAreReturnedAsExpected(?ApiKey $apiKey): void + { + $tag = 'foo'; + $repo = $this->prophesize(TagRepository::class); + $tagExists = $repo->tagExists($tag, $apiKey)->willReturn(true); + $getRepo = $this->em->getRepository(Tag::class)->willReturn($repo->reveal()); + + $spec = $apiKey === null ? null : $apiKey->spec(); + $list = map(range(0, 1), fn () => Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance())); + $repo2 = $this->prophesize(VisitRepository::class); + $repo2->findVisitsByTag($tag, Argument::type(DateRange::class), 1, 0, $spec)->willReturn($list); + $repo2->countVisitsByTag($tag, Argument::type(DateRange::class), $spec)->willReturn(1); + $this->em->getRepository(Visit::class)->willReturn($repo2->reveal())->shouldBeCalledOnce(); + + $paginator = $this->helper->visitsForTag($tag, new VisitsParams(), $apiKey); + + self::assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentPageResults())); + $tagExists->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + } } diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index cfb97320..5b68ada7 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -66,8 +66,8 @@ return [ Service\ShortUrl\ShortUrlResolver::class, ShortUrlDataTransformer::class, ], - Action\Visit\ShortUrlVisitsAction::class => [Service\VisitsTracker::class], - Action\Visit\TagVisitsAction::class => [Service\VisitsTracker::class], + Action\Visit\ShortUrlVisitsAction::class => [Visit\VisitsStatsHelper::class], + Action\Visit\TagVisitsAction::class => [Visit\VisitsStatsHelper::class], Action\Visit\GlobalVisitsAction::class => [Visit\VisitsStatsHelper::class], Action\ShortUrl\ListShortUrlsAction::class => [Service\ShortUrlService::class, ShortUrlDataTransformer::class], Action\ShortUrl\EditShortUrlTagsAction::class => [Service\ShortUrlService::class], diff --git a/module/Rest/src/Action/Visit/ShortUrlVisitsAction.php b/module/Rest/src/Action/Visit/ShortUrlVisitsAction.php index 7b7c1055..8175d1c7 100644 --- a/module/Rest/src/Action/Visit/ShortUrlVisitsAction.php +++ b/module/Rest/src/Action/Visit/ShortUrlVisitsAction.php @@ -10,7 +10,7 @@ use Psr\Http\Message\ServerRequestInterface as Request; use Shlinkio\Shlink\Common\Paginator\Util\PagerfantaUtilsTrait; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; @@ -21,11 +21,11 @@ class ShortUrlVisitsAction extends AbstractRestAction protected const ROUTE_PATH = '/short-urls/{shortCode}/visits'; protected const ROUTE_ALLOWED_METHODS = [self::METHOD_GET]; - private VisitsTrackerInterface $visitsTracker; + private VisitsStatsHelperInterface $visitsHelper; - public function __construct(VisitsTrackerInterface $visitsTracker) + public function __construct(VisitsStatsHelperInterface $visitsHelper) { - $this->visitsTracker = $visitsTracker; + $this->visitsHelper = $visitsHelper; } public function handle(Request $request): Response @@ -33,7 +33,7 @@ class ShortUrlVisitsAction extends AbstractRestAction $identifier = ShortUrlIdentifier::fromApiRequest($request); $params = VisitsParams::fromRawData($request->getQueryParams()); $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); - $visits = $this->visitsTracker->info($identifier, $params, $apiKey); + $visits = $this->visitsHelper->visitsForShortUrl($identifier, $params, $apiKey); return new JsonResponse([ 'visits' => $this->serializePaginator($visits), diff --git a/module/Rest/src/Action/Visit/TagVisitsAction.php b/module/Rest/src/Action/Visit/TagVisitsAction.php index aec42ebb..8d981c82 100644 --- a/module/Rest/src/Action/Visit/TagVisitsAction.php +++ b/module/Rest/src/Action/Visit/TagVisitsAction.php @@ -9,7 +9,7 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Shlinkio\Shlink\Common\Paginator\Util\PagerfantaUtilsTrait; use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; @@ -20,11 +20,11 @@ class TagVisitsAction extends AbstractRestAction protected const ROUTE_PATH = '/tags/{tag}/visits'; protected const ROUTE_ALLOWED_METHODS = [self::METHOD_GET]; - private VisitsTrackerInterface $visitsTracker; + private VisitsStatsHelperInterface $visitsHelper; - public function __construct(VisitsTrackerInterface $visitsTracker) + public function __construct(VisitsStatsHelperInterface $visitsHelper) { - $this->visitsTracker = $visitsTracker; + $this->visitsHelper = $visitsHelper; } public function handle(Request $request): Response @@ -32,7 +32,7 @@ class TagVisitsAction extends AbstractRestAction $tag = $request->getAttribute('tag', ''); $params = VisitsParams::fromRawData($request->getQueryParams()); $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); - $visits = $this->visitsTracker->visitsForTag($tag, $params, $apiKey); + $visits = $this->visitsHelper->visitsForTag($tag, $params, $apiKey); return new JsonResponse([ 'visits' => $this->serializePaginator($visits), diff --git a/module/Rest/test/Action/Visit/ShortUrlVisitsActionTest.php b/module/Rest/test/Action/Visit/ShortUrlVisitsActionTest.php index 9c751214..6b149877 100644 --- a/module/Rest/test/Action/Visit/ShortUrlVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/ShortUrlVisitsActionTest.php @@ -16,7 +16,7 @@ use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Service\VisitsTracker; +use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\Rest\Action\Visit\ShortUrlVisitsAction; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -25,19 +25,19 @@ class ShortUrlVisitsActionTest extends TestCase use ProphecyTrait; private ShortUrlVisitsAction $action; - private ObjectProphecy $visitsTracker; + private ObjectProphecy $visitsHelper; public function setUp(): void { - $this->visitsTracker = $this->prophesize(VisitsTracker::class); - $this->action = new ShortUrlVisitsAction($this->visitsTracker->reveal()); + $this->visitsHelper = $this->prophesize(VisitsStatsHelperInterface::class); + $this->action = new ShortUrlVisitsAction($this->visitsHelper->reveal()); } /** @test */ public function providingCorrectShortCodeReturnsVisits(): void { $shortCode = 'abc123'; - $this->visitsTracker->info( + $this->visitsHelper->visitsForShortUrl( new ShortUrlIdentifier($shortCode), Argument::type(VisitsParams::class), Argument::type(ApiKey::class), @@ -52,7 +52,7 @@ class ShortUrlVisitsActionTest extends TestCase public function paramsAreReadFromQuery(): void { $shortCode = 'abc123'; - $this->visitsTracker->info(new ShortUrlIdentifier($shortCode), new VisitsParams( + $this->visitsHelper->visitsForShortUrl(new ShortUrlIdentifier($shortCode), new VisitsParams( new DateRange(null, Chronos::parse('2016-01-01 00:00:00')), 3, 10, diff --git a/module/Rest/test/Action/Visit/TagVisitsActionTest.php b/module/Rest/test/Action/Visit/TagVisitsActionTest.php index c9097d07..da046f26 100644 --- a/module/Rest/test/Action/Visit/TagVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/TagVisitsActionTest.php @@ -12,7 +12,7 @@ use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Model\VisitsParams; -use Shlinkio\Shlink\Core\Service\VisitsTracker; +use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\Rest\Action\Visit\TagVisitsAction; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -21,12 +21,12 @@ class TagVisitsActionTest extends TestCase use ProphecyTrait; private TagVisitsAction $action; - private ObjectProphecy $visitsTracker; + private ObjectProphecy $visitsHelper; protected function setUp(): void { - $this->visitsTracker = $this->prophesize(VisitsTracker::class); - $this->action = new TagVisitsAction($this->visitsTracker->reveal()); + $this->visitsHelper = $this->prophesize(VisitsStatsHelperInterface::class); + $this->action = new TagVisitsAction($this->visitsHelper->reveal()); } /** @test */ @@ -34,7 +34,7 @@ class TagVisitsActionTest extends TestCase { $tag = 'foo'; $apiKey = new ApiKey(); - $getVisits = $this->visitsTracker->visitsForTag($tag, Argument::type(VisitsParams::class), $apiKey)->willReturn( + $getVisits = $this->visitsHelper->visitsForTag($tag, Argument::type(VisitsParams::class), $apiKey)->willReturn( new Paginator(new ArrayAdapter([])), ); From 36be44e7b5f2ce35f0177d17efe40100f8fd6530 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 19:50:17 +0100 Subject: [PATCH 06/28] Moved VisitsTracker service to Visit namespace --- module/Core/config/dependencies.config.php | 8 ++++---- module/Core/src/Action/AbstractTrackingAction.php | 2 +- module/Core/src/Action/RedirectAction.php | 2 +- module/Core/src/{Service => Visit}/VisitsTracker.php | 2 +- .../src/{Service => Visit}/VisitsTrackerInterface.php | 2 +- module/Core/test/Action/PixelActionTest.php | 2 +- module/Core/test/Action/RedirectActionTest.php | 2 +- module/Core/test/{Service => Visit}/VisitsTrackerTest.php | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-) rename module/Core/src/{Service => Visit}/VisitsTracker.php (96%) rename module/Core/src/{Service => Visit}/VisitsTrackerInterface.php (84%) rename module/Core/test/{Service => Visit}/VisitsTrackerTest.php (93%) diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index e742ad43..0f182b1b 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -24,7 +24,7 @@ return [ Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Service\UrlShortener::class => ConfigAbstractFactory::class, - Service\VisitsTracker::class => ConfigAbstractFactory::class, + Visit\VisitsTracker::class => ConfigAbstractFactory::class, Service\ShortUrlService::class => ConfigAbstractFactory::class, Visit\VisitLocator::class => ConfigAbstractFactory::class, Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class, @@ -75,7 +75,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, Service\ShortUrl\ShortCodeHelper::class, ], - Service\VisitsTracker::class => [ + Visit\VisitsTracker::class => [ 'em', EventDispatcherInterface::class, 'config.url_shortener.anonymize_remote_addr', @@ -104,14 +104,14 @@ return [ Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, - Service\VisitsTracker::class, + Visit\VisitsTracker::class, Options\AppOptions::class, Util\RedirectResponseHelper::class, 'Logger_Shlink', ], Action\PixelAction::class => [ Service\ShortUrl\ShortUrlResolver::class, - Service\VisitsTracker::class, + Visit\VisitsTracker::class, Options\AppOptions::class, 'Logger_Shlink', ], diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 86eb197b..b6a119b2 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -20,7 +20,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\AppOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; use function array_key_exists; use function array_merge; diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index 0fc6232d..d346456b 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -11,8 +11,8 @@ use Psr\Http\Server\RequestHandlerInterface; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; +use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; class RedirectAction extends AbstractTrackingAction implements StatusCodeInterface { diff --git a/module/Core/src/Service/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php similarity index 96% rename from module/Core/src/Service/VisitsTracker.php rename to module/Core/src/Visit/VisitsTracker.php index daf4aa24..e4a9f304 100644 --- a/module/Core/src/Service/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\Core\Service; +namespace Shlinkio\Shlink\Core\Visit; use Doctrine\ORM; use Psr\EventDispatcher\EventDispatcherInterface; diff --git a/module/Core/src/Service/VisitsTrackerInterface.php b/module/Core/src/Visit/VisitsTrackerInterface.php similarity index 84% rename from module/Core/src/Service/VisitsTrackerInterface.php rename to module/Core/src/Visit/VisitsTrackerInterface.php index c468cf0e..75f92434 100644 --- a/module/Core/src/Service/VisitsTrackerInterface.php +++ b/module/Core/src/Visit/VisitsTrackerInterface.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\Core\Service; +namespace Shlinkio\Shlink\Core\Visit; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Model\Visitor; diff --git a/module/Core/test/Action/PixelActionTest.php b/module/Core/test/Action/PixelActionTest.php index cae74926..065cc2c4 100644 --- a/module/Core/test/Action/PixelActionTest.php +++ b/module/Core/test/Action/PixelActionTest.php @@ -16,7 +16,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options\AppOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\Service\VisitsTracker; +use Shlinkio\Shlink\Core\Visit\VisitsTracker; class PixelActionTest extends TestCase { diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 411d9a50..f869e2c4 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -19,8 +19,8 @@ use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\Service\VisitsTrackerInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; +use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; use function array_key_exists; diff --git a/module/Core/test/Service/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php similarity index 93% rename from module/Core/test/Service/VisitsTrackerTest.php rename to module/Core/test/Visit/VisitsTrackerTest.php index 4807dc41..a7ca98c3 100644 --- a/module/Core/test/Service/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace ShlinkioTest\Shlink\Core\Service; +namespace ShlinkioTest\Shlink\Core\Visit; use Doctrine\ORM\EntityManager; use PHPUnit\Framework\TestCase; @@ -14,7 +14,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Service\VisitsTracker; +use Shlinkio\Shlink\Core\Visit\VisitsTracker; class VisitsTrackerTest extends TestCase { From 15061d3e0d66b35654383def411d5e9e370086ed Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 21:38:19 +0100 Subject: [PATCH 07/28] Created new middlewares to track not found visits --- .../autoload/middleware-pipeline.global.php | 2 + module/Core/config/dependencies.config.php | 5 +- .../src/ErrorHandler/Model/NotFoundType.php | 57 +++++++++++++++++++ .../ErrorHandler/NotFoundRedirectHandler.php | 35 +++--------- .../NotFoundTrackerMiddleware.php | 44 ++++++++++++++ .../NotFoundTypeResolverMiddleware.php | 27 +++++++++ .../Core/src/Repository/VisitRepository.php | 7 ++- module/Core/src/Visit/VisitsTracker.php | 23 +++++++- .../Core/src/Visit/VisitsTrackerInterface.php | 6 ++ .../NotFoundRedirectHandlerTest.php | 21 ++++--- 10 files changed, 187 insertions(+), 40 deletions(-) create mode 100644 module/Core/src/ErrorHandler/Model/NotFoundType.php create mode 100644 module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php create mode 100644 module/Core/src/ErrorHandler/NotFoundTypeResolverMiddleware.php diff --git a/config/autoload/middleware-pipeline.global.php b/config/autoload/middleware-pipeline.global.php index 9f8cc729..b83ee2e7 100644 --- a/config/autoload/middleware-pipeline.global.php +++ b/config/autoload/middleware-pipeline.global.php @@ -64,6 +64,8 @@ return [ ], 'not-found' => [ 'middleware' => [ + Core\ErrorHandler\NotFoundTypeResolverMiddleware::class, + Core\ErrorHandler\NotFoundTrackerMiddleware::class, Core\ErrorHandler\NotFoundRedirectHandler::class, Core\ErrorHandler\NotFoundTemplateHandler::class, ], diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 0f182b1b..50669f66 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -15,6 +15,8 @@ return [ 'dependencies' => [ 'factories' => [ + ErrorHandler\NotFoundTypeResolverMiddleware::class => ConfigAbstractFactory::class, + ErrorHandler\NotFoundTrackerMiddleware::class => ConfigAbstractFactory::class, ErrorHandler\NotFoundRedirectHandler::class => ConfigAbstractFactory::class, ErrorHandler\NotFoundTemplateHandler::class => InvokableFactory::class, @@ -58,10 +60,11 @@ return [ ], ConfigAbstractFactory::class => [ + ErrorHandler\NotFoundTypeResolverMiddleware::class => ['config.router.base_path'], + ErrorHandler\NotFoundTrackerMiddleware::class => [Visit\VisitsTracker::class], ErrorHandler\NotFoundRedirectHandler::class => [ NotFoundRedirectOptions::class, Util\RedirectResponseHelper::class, - 'config.router.base_path', ], Options\AppOptions::class => ['config.app_options'], diff --git a/module/Core/src/ErrorHandler/Model/NotFoundType.php b/module/Core/src/ErrorHandler/Model/NotFoundType.php new file mode 100644 index 00000000..7585a3ca --- /dev/null +++ b/module/Core/src/ErrorHandler/Model/NotFoundType.php @@ -0,0 +1,57 @@ +type = $type; + } + + public static function fromRequest(ServerRequestInterface $request, string $basePath): self + { + $isBaseUrl = rtrim($request->getUri()->getPath(), '/') === $basePath; + if ($isBaseUrl) { + return new self(Visit::TYPE_BASE_URL); + } + + /** @var RouteResult $routeResult */ + $routeResult = $request->getAttribute(RouteResult::class, RouteResult::fromRouteFailure(null)); + if ($routeResult->isFailure()) { + return new self(Visit::TYPE_REGULAR_404); + } + + if ($routeResult->getMatchedRouteName() === RedirectAction::class) { + return new self(Visit::TYPE_INVALID_SHORT_URL); + } + + return new self(self::class); + } + + public function isBaseUrl(): bool + { + return $this->type === Visit::TYPE_BASE_URL; + } + + public function isRegularNotFound(): bool + { + return $this->type === Visit::TYPE_REGULAR_404; + } + + public function isInvalidShortUrl(): bool + { + return $this->type === Visit::TYPE_INVALID_SHORT_URL; + } +} diff --git a/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php b/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php index a49db5bb..1f3b4fed 100644 --- a/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php +++ b/module/Core/src/ErrorHandler/NotFoundRedirectHandler.php @@ -4,67 +4,48 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\ErrorHandler; -use Mezzio\Router\RouteResult; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; -use Psr\Http\Message\UriInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; -use Shlinkio\Shlink\Core\Action\RedirectAction; +use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; -use function rtrim; - class NotFoundRedirectHandler implements MiddlewareInterface { private Options\NotFoundRedirectOptions $redirectOptions; private RedirectResponseHelperInterface $redirectResponseHelper; - private string $shlinkBasePath; public function __construct( Options\NotFoundRedirectOptions $redirectOptions, - RedirectResponseHelperInterface $redirectResponseHelper, - string $shlinkBasePath + RedirectResponseHelperInterface $redirectResponseHelper ) { $this->redirectOptions = $redirectOptions; - $this->shlinkBasePath = $shlinkBasePath; $this->redirectResponseHelper = $redirectResponseHelper; } public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - /** @var RouteResult $routeResult */ - $routeResult = $request->getAttribute(RouteResult::class, RouteResult::fromRouteFailure(null)); - $redirectResponse = $this->createRedirectResponse($routeResult, $request->getUri()); + /** @var NotFoundType $notFoundType */ + $notFoundType = $request->getAttribute(NotFoundType::class); - return $redirectResponse ?? $handler->handle($request); - } - - private function createRedirectResponse(RouteResult $routeResult, UriInterface $uri): ?ResponseInterface - { - $isBaseUrl = rtrim($uri->getPath(), '/') === $this->shlinkBasePath; - - if ($isBaseUrl && $this->redirectOptions->hasBaseUrlRedirect()) { + if ($notFoundType->isBaseUrl() && $this->redirectOptions->hasBaseUrlRedirect()) { return $this->redirectResponseHelper->buildRedirectResponse($this->redirectOptions->getBaseUrlRedirect()); } - if (!$isBaseUrl && $routeResult->isFailure() && $this->redirectOptions->hasRegular404Redirect()) { + if ($notFoundType->isRegularNotFound() && $this->redirectOptions->hasRegular404Redirect()) { return $this->redirectResponseHelper->buildRedirectResponse( $this->redirectOptions->getRegular404Redirect(), ); } - if ( - $routeResult->isSuccess() && - $routeResult->getMatchedRouteName() === RedirectAction::class && - $this->redirectOptions->hasInvalidShortUrlRedirect() - ) { + if ($notFoundType->isInvalidShortUrl() && $this->redirectOptions->hasInvalidShortUrlRedirect()) { return $this->redirectResponseHelper->buildRedirectResponse( $this->redirectOptions->getInvalidShortUrlRedirect(), ); } - return null; + return $handler->handle($request); } } diff --git a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php new file mode 100644 index 00000000..f792dd07 --- /dev/null +++ b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php @@ -0,0 +1,44 @@ +visitsTracker = $visitsTracker; + } + + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + /** @var NotFoundType $notFoundType */ + $notFoundType = $request->getAttribute(NotFoundType::class); + $visitor = Visitor::fromRequest($request); + + if ($notFoundType->isBaseUrl()) { + $this->visitsTracker->trackBaseUrlVisit($visitor); + } + + if ($notFoundType->isRegularNotFound()) { + $this->visitsTracker->trackRegularNotFoundVisit($visitor); + } + + if ($notFoundType->isInvalidShortUrl()) { + $this->visitsTracker->trackInvalidShortUrlVisit($visitor); + } + + return $handler->handle($request); + } +} diff --git a/module/Core/src/ErrorHandler/NotFoundTypeResolverMiddleware.php b/module/Core/src/ErrorHandler/NotFoundTypeResolverMiddleware.php new file mode 100644 index 00000000..6f13db73 --- /dev/null +++ b/module/Core/src/ErrorHandler/NotFoundTypeResolverMiddleware.php @@ -0,0 +1,27 @@ +shlinkBasePath = $shlinkBasePath; + } + + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + $notFoundType = NotFoundType::fromRequest($request, $this->shlinkBasePath); + return $handler->handle($request->withAttribute(NotFoundType::class, $notFoundType)); + } +} diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index a1df73a5..61bc50bb 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -211,8 +211,9 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo public function countVisits(?ApiKey $apiKey = null): int { - return (int) $this->matchSingleScalarResult( - Spec::countOf(new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrl')), - ); + return (int) $this->matchSingleScalarResult(Spec::countOf(Spec::andX( + Spec::isNotNull('shortUrl'), + new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrl'), + ))); } } diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index e4a9f304..af170cfc 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -29,11 +29,30 @@ class VisitsTracker implements VisitsTrackerInterface public function track(ShortUrl $shortUrl, Visitor $visitor): void { - $visit = Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr); + $visit = $this->trackVisit(Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr)); + $this->eventDispatcher->dispatch(new ShortUrlVisited($visit->getId(), $visitor->getRemoteAddress())); + } + public function trackInvalidShortUrlVisit(Visitor $visitor): void + { + $this->trackVisit(Visit::forInvalidShortUrl($visitor)); + } + + public function trackBaseUrlVisit(Visitor $visitor): void + { + $this->trackVisit(Visit::forBasePath($visitor)); + } + + public function trackRegularNotFoundVisit(Visitor $visitor): void + { + $this->trackVisit(Visit::forRegularNotFound($visitor)); + } + + private function trackVisit(Visit $visit): Visit + { $this->em->persist($visit); $this->em->flush(); - $this->eventDispatcher->dispatch(new ShortUrlVisited($visit->getId(), $visitor->getRemoteAddress())); + return $visit; } } diff --git a/module/Core/src/Visit/VisitsTrackerInterface.php b/module/Core/src/Visit/VisitsTrackerInterface.php index 75f92434..ae70d550 100644 --- a/module/Core/src/Visit/VisitsTrackerInterface.php +++ b/module/Core/src/Visit/VisitsTrackerInterface.php @@ -10,4 +10,10 @@ use Shlinkio\Shlink\Core\Model\Visitor; interface VisitsTrackerInterface { public function track(ShortUrl $shortUrl, Visitor $visitor): void; + + public function trackInvalidShortUrlVisit(Visitor $visitor): void; + + public function trackBaseUrlVisit(Visitor $visitor): void; + + public function trackRegularNotFoundVisit(Visitor $visitor): void; } diff --git a/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php b/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php index 83810d22..9df49879 100644 --- a/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php +++ b/module/Core/test/ErrorHandler/NotFoundRedirectHandlerTest.php @@ -17,6 +17,7 @@ use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Action\RedirectAction; +use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\ErrorHandler\NotFoundRedirectHandler; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; @@ -33,7 +34,7 @@ class NotFoundRedirectHandlerTest extends TestCase { $this->redirectOptions = new NotFoundRedirectOptions(); $this->helper = $this->prophesize(RedirectResponseHelperInterface::class); - $this->middleware = new NotFoundRedirectHandler($this->redirectOptions, $this->helper->reveal(), ''); + $this->middleware = new NotFoundRedirectHandler($this->redirectOptions, $this->helper->reveal()); } /** @@ -64,19 +65,19 @@ class NotFoundRedirectHandlerTest extends TestCase public function provideRedirects(): iterable { yield 'base URL with trailing slash' => [ - ServerRequestFactory::fromGlobals()->withUri(new Uri('/')), + $this->withNotFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri('/'))), 'baseUrl', ]; yield 'base URL without trailing slash' => [ - ServerRequestFactory::fromGlobals()->withUri(new Uri('')), + $this->withNotFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri(''))), 'baseUrl', ]; yield 'regular 404' => [ - ServerRequestFactory::fromGlobals()->withUri(new Uri('/foo/bar')), + $this->withNotFoundType(ServerRequestFactory::fromGlobals()->withUri(new Uri('/foo/bar'))), 'regular404', ]; yield 'invalid short URL' => [ - ServerRequestFactory::fromGlobals() + $this->withNotFoundType(ServerRequestFactory::fromGlobals() ->withAttribute( RouteResult::class, RouteResult::fromRoute( @@ -88,7 +89,7 @@ class NotFoundRedirectHandlerTest extends TestCase ), ), ) - ->withUri(new Uri('/abc123')), + ->withUri(new Uri('/abc123'))), 'invalidShortUrl', ]; } @@ -96,7 +97,7 @@ class NotFoundRedirectHandlerTest extends TestCase /** @test */ public function nextMiddlewareIsInvokedWhenNotRedirectNeedsToOccur(): void { - $req = ServerRequestFactory::fromGlobals(); + $req = $this->withNotFoundType(ServerRequestFactory::fromGlobals()); $resp = new Response(); $buildResp = $this->helper->buildRedirectResponse(Argument::cetera()); @@ -110,4 +111,10 @@ class NotFoundRedirectHandlerTest extends TestCase $buildResp->shouldNotHaveBeenCalled(); $handle->shouldHaveBeenCalledOnce(); } + + private function withNotFoundType(ServerRequestInterface $req): ServerRequestInterface + { + $type = NotFoundType::fromRequest($req, ''); + return $req->withAttribute(NotFoundType::class, $type); + } } From 55e7f7ccb069fd32319bf605580ffe9cb7f54fb3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 22:00:07 +0100 Subject: [PATCH 08/28] Improved VisitRepository tests --- .../Core/src/Repository/VisitRepository.php | 14 +++++---- .../Repository/VisitRepositoryInterface.php | 2 ++ .../src/Visit/Spec/CountOfOrphanVisits.php | 17 +++++++++++ .../src/Visit/Spec/CountOfShortUrlVisits.php | 30 +++++++++++++++++++ .../Repository/VisitRepositoryTest.php | 8 ++++- 5 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 module/Core/src/Visit/Spec/CountOfOrphanVisits.php create mode 100644 module/Core/src/Visit/Spec/CountOfShortUrlVisits.php diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 61bc50bb..082b17b8 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -7,13 +7,13 @@ namespace Shlinkio\Shlink\Core\Repository; use Doctrine\ORM\Query\ResultSetMappingBuilder; use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\EntitySpecificationRepository; -use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; -use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin; +use Shlinkio\Shlink\Core\Visit\Spec\CountOfOrphanVisits; +use Shlinkio\Shlink\Core\Visit\Spec\CountOfShortUrlVisits; use Shlinkio\Shlink\Rest\Entity\ApiKey; use const PHP_INT_MAX; @@ -211,9 +211,11 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo public function countVisits(?ApiKey $apiKey = null): int { - return (int) $this->matchSingleScalarResult(Spec::countOf(Spec::andX( - Spec::isNotNull('shortUrl'), - new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrl'), - ))); + return (int) $this->matchSingleScalarResult(new CountOfShortUrlVisits($apiKey)); + } + + public function countOrphanVisits(): int + { + return (int) $this->matchSingleScalarResult(new CountOfOrphanVisits()); } } diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 526645df..0d637f75 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -63,4 +63,6 @@ interface VisitRepositoryInterface extends ObjectRepository, EntitySpecification public function countVisitsByTag(string $tag, ?DateRange $dateRange = null, ?Specification $spec = null): int; public function countVisits(?ApiKey $apiKey = null): int; + + public function countOrphanVisits(): int; } diff --git a/module/Core/src/Visit/Spec/CountOfOrphanVisits.php b/module/Core/src/Visit/Spec/CountOfOrphanVisits.php new file mode 100644 index 00000000..7e15f330 --- /dev/null +++ b/module/Core/src/Visit/Spec/CountOfOrphanVisits.php @@ -0,0 +1,17 @@ +apiKey = $apiKey; + } + + protected function getSpec(): Specification + { + return Spec::countOf(Spec::andX( + Spec::isNotNull('shortUrl'), + new WithApiKeySpecsEnsuringJoin($this->apiKey, 'shortUrl'), + )); + } +} diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 5681ee26..00b558d4 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -168,7 +168,7 @@ class VisitRepositoryTest extends DatabaseTestCase } /** @test */ - public function countReturnsExpectedResultBasedOnApiKey(): void + public function countVisitsReturnsExpectedResultBasedOnApiKey(): void { $domain = new Domain('foo.com'); $this->getEntityManager()->persist($domain); @@ -200,12 +200,18 @@ class VisitRepositoryTest extends DatabaseTestCase $domainApiKey = ApiKey::withRoles(RoleDefinition::forDomain($domain)); $this->getEntityManager()->persist($domainApiKey); + // Visits not linked to any short URL + $this->getEntityManager()->persist(Visit::forBasePath(Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forInvalidShortUrl(Visitor::emptyInstance())); + $this->getEntityManager()->persist(Visit::forRegularNotFound(Visitor::emptyInstance())); + $this->getEntityManager()->flush(); self::assertEquals(4 + 5 + 7, $this->repo->countVisits()); self::assertEquals(4, $this->repo->countVisits($apiKey1)); self::assertEquals(5 + 7, $this->repo->countVisits($apiKey2)); self::assertEquals(4 + 7, $this->repo->countVisits($domainApiKey)); + self::assertEquals(3, $this->repo->countOrphanVisits()); } private function createShortUrlsAndVisits(bool $withDomain = true, array $tags = []): array From 0e165bc7e06577df1be648b7d4df904188600186 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 22:06:03 +0100 Subject: [PATCH 09/28] Created NotFoundTypeResolverMiddlewareTest --- .../NotFoundTypeResolverMiddlewareTest.php | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 module/Core/test/ErrorHandler/NotFoundTypeResolverMiddlewareTest.php diff --git a/module/Core/test/ErrorHandler/NotFoundTypeResolverMiddlewareTest.php b/module/Core/test/ErrorHandler/NotFoundTypeResolverMiddlewareTest.php new file mode 100644 index 00000000..c5d9be79 --- /dev/null +++ b/module/Core/test/ErrorHandler/NotFoundTypeResolverMiddlewareTest.php @@ -0,0 +1,47 @@ +middleware = new NotFoundTypeResolverMiddleware(''); + $this->handler = $this->prophesize(RequestHandlerInterface::class); + } + + /** @test */ + public function notFoundTypeIsAddedToRequest(): void + { + $request = ServerRequestFactory::fromGlobals(); + $handle = $this->handler->handle(Argument::that(function (ServerRequestInterface $req) { + Assert::assertArrayHasKey(NotFoundType::class, $req->getAttributes()); + + return true; + }))->willReturn(new Response()); + + $this->middleware->process($request, $this->handler->reveal()); + + self::assertArrayNotHasKey(NotFoundType::class, $request->getAttributes()); + $handle->shouldHaveBeenCalledOnce(); + } +} From d2e0413a48490d90da2c49e6a605816528ac8a5b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 22:16:15 +0100 Subject: [PATCH 10/28] Added NotFoundTrackerMiddlewareTest --- .../src/ErrorHandler/Model/NotFoundType.php | 2 +- .../NotFoundTrackerMiddleware.php | 8 +- .../NotFoundTrackerMiddlewareTest.php | 95 +++++++++++++++++++ 3 files changed, 98 insertions(+), 7 deletions(-) create mode 100644 module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php diff --git a/module/Core/src/ErrorHandler/Model/NotFoundType.php b/module/Core/src/ErrorHandler/Model/NotFoundType.php index 7585a3ca..57176e84 100644 --- a/module/Core/src/ErrorHandler/Model/NotFoundType.php +++ b/module/Core/src/ErrorHandler/Model/NotFoundType.php @@ -11,7 +11,7 @@ use Shlinkio\Shlink\Core\Entity\Visit; use function rtrim; -final class NotFoundType +class NotFoundType { private string $type; diff --git a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php index f792dd07..b81e55de 100644 --- a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php +++ b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php @@ -29,13 +29,9 @@ class NotFoundTrackerMiddleware implements MiddlewareInterface if ($notFoundType->isBaseUrl()) { $this->visitsTracker->trackBaseUrlVisit($visitor); - } - - if ($notFoundType->isRegularNotFound()) { + } elseif ($notFoundType->isRegularNotFound()) { $this->visitsTracker->trackRegularNotFoundVisit($visitor); - } - - if ($notFoundType->isInvalidShortUrl()) { + } elseif ($notFoundType->isInvalidShortUrl()) { $this->visitsTracker->trackInvalidShortUrlVisit($visitor); } diff --git a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php new file mode 100644 index 00000000..560a2468 --- /dev/null +++ b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php @@ -0,0 +1,95 @@ +notFoundType = $this->prophesize(NotFoundType::class); + $this->handler = $this->prophesize(RequestHandlerInterface::class); + $this->handler->handle(Argument::cetera())->willReturn(new Response()); + + $this->visitsTracker = $this->prophesize(VisitsTrackerInterface::class); + $this->middleware = new NotFoundTrackerMiddleware($this->visitsTracker->reveal()); + + $this->request = ServerRequestFactory::fromGlobals()->withAttribute( + NotFoundType::class, + $this->notFoundType->reveal(), + ); + } + + /** @test */ + public function baseUrlErrorIsTracked(): void + { + $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(true); + $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); + $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); + + $this->middleware->process($this->request, $this->handler->reveal()); + + $isBaseUrl->shouldHaveBeenCalledOnce(); + $isRegularNotFound->shouldNotHaveBeenCalled(); + $isInvalidShortUrl->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function regularNotFoundErrorIsTracked(): void + { + $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); + $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(true); + $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); + + $this->middleware->process($this->request, $this->handler->reveal()); + + $isBaseUrl->shouldHaveBeenCalledOnce(); + $isRegularNotFound->shouldHaveBeenCalledOnce(); + $isInvalidShortUrl->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function invalidShortUrlErrorIsTracked(): void + { + $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); + $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); + $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(true); + + $this->middleware->process($this->request, $this->handler->reveal()); + + $isBaseUrl->shouldHaveBeenCalledOnce(); + $isRegularNotFound->shouldHaveBeenCalledOnce(); + $isInvalidShortUrl->shouldHaveBeenCalledOnce(); + $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + } +} From f7215fc2c5e8aca565f353fbe63521f70ea72f17 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 22:20:18 +0100 Subject: [PATCH 11/28] Documented ADR decision outcome --- ...ck-visits-to-base-url-invalid-short-url-and-regular-404.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md b/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md index 032820ef..983410d1 100644 --- a/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md +++ b/docs/adr/2021-02-07-track-visits-to-base-url-invalid-short-url-and-regular-404.md @@ -18,7 +18,9 @@ The intention is to change that, and allow users to track the cases mentioned ab ## Decision outcome -*TBD* +The decision is to use the existing table, as making the short URL nullable can be handled seamlessly by using named constructors, and it has a lot of benefits on regards of reusing existing components. + +Also, the domain name this kind of visits will receive is "Orphan Visits", as they are detached from any existing short URL. ## Pros and Cons of the Options From 5278d7668cb4bb7b0d999ed61a93f958f8289a2b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 8 Feb 2021 22:44:58 +0100 Subject: [PATCH 12/28] Added orphan visits count to visits stats endpoint --- docs/swagger/definitions/VisitStats.json | 8 ++++++-- docs/swagger/paths/v2_visits.json | 3 ++- module/Core/src/Visit/Model/VisitsStats.php | 5 ++++- module/Core/src/Visit/VisitsStatsHelper.php | 8 ++------ module/Core/test/Visit/VisitsStatsHelperTest.php | 6 ++++-- module/Rest/test-api/Action/GlobalVisitsTest.php | 2 ++ module/Rest/test-api/Fixtures/VisitsFixture.php | 8 ++++++++ module/Rest/test/Action/Visit/GlobalVisitsActionTest.php | 2 +- 8 files changed, 29 insertions(+), 13 deletions(-) diff --git a/docs/swagger/definitions/VisitStats.json b/docs/swagger/definitions/VisitStats.json index 5f439c9b..2a97f597 100644 --- a/docs/swagger/definitions/VisitStats.json +++ b/docs/swagger/definitions/VisitStats.json @@ -1,10 +1,14 @@ { "type": "object", - "required": ["visitsCount"], + "required": ["visitsCount", "orphanVisitsCount"], "properties": { "visitsCount": { "type": "number", - "description": "The total amount of visits received." + "description": "The total amount of visits received on any short URL." + }, + "orphanVisitsCount": { + "type": "number", + "description": "The total amount of visits that could not be matched to a short URL (visits to the base URL, an invalid short URL or any other kind of 404)." } } } diff --git a/docs/swagger/paths/v2_visits.json b/docs/swagger/paths/v2_visits.json index 089223b3..3c712b1f 100644 --- a/docs/swagger/paths/v2_visits.json +++ b/docs/swagger/paths/v2_visits.json @@ -34,7 +34,8 @@ "examples": { "application/json": { "visits": { - "visitsCount": 1569874 + "visitsCount": 1569874, + "orphanVisitsCount": 71345 } } } diff --git a/module/Core/src/Visit/Model/VisitsStats.php b/module/Core/src/Visit/Model/VisitsStats.php index ac5083c7..982f03c4 100644 --- a/module/Core/src/Visit/Model/VisitsStats.php +++ b/module/Core/src/Visit/Model/VisitsStats.php @@ -9,16 +9,19 @@ use JsonSerializable; final class VisitsStats implements JsonSerializable { private int $visitsCount; + private int $orphanVisitsCount; - public function __construct(int $visitsCount) + public function __construct(int $visitsCount, int $orphanVisitsCount) { $this->visitsCount = $visitsCount; + $this->orphanVisitsCount = $orphanVisitsCount; } public function jsonSerialize(): array { return [ 'visitsCount' => $this->visitsCount, + 'orphanVisitsCount' => $this->orphanVisitsCount, ]; } } diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index 7c4efd23..0cb58897 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -32,15 +32,11 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface } public function getVisitsStats(?ApiKey $apiKey = null): VisitsStats - { - return new VisitsStats($this->getVisitsCount($apiKey)); - } - - private function getVisitsCount(?ApiKey $apiKey): int { /** @var VisitRepository $visitsRepo */ $visitsRepo = $this->em->getRepository(Visit::class); - return $visitsRepo->countVisits($apiKey); + + return new VisitsStats($visitsRepo->countVisits($apiKey), $visitsRepo->countOrphanVisits()); } /** diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index 20a39316..6fefd1d0 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -51,13 +51,15 @@ class VisitsStatsHelperTest extends TestCase public function returnsExpectedVisitsStats(int $expectedCount): void { $repo = $this->prophesize(VisitRepository::class); - $count = $repo->countVisits(null)->willReturn($expectedCount); + $count = $repo->countVisits(null)->willReturn($expectedCount * 3); + $countOrphan = $repo->countOrphanVisits()->willReturn($expectedCount); $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); $stats = $this->helper->getVisitsStats(); - self::assertEquals(new VisitsStats($expectedCount), $stats); + self::assertEquals(new VisitsStats($expectedCount * 3, $expectedCount), $stats); $count->shouldHaveBeenCalledOnce(); + $countOrphan->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); } diff --git a/module/Rest/test-api/Action/GlobalVisitsTest.php b/module/Rest/test-api/Action/GlobalVisitsTest.php index 99e05918..1b71f976 100644 --- a/module/Rest/test-api/Action/GlobalVisitsTest.php +++ b/module/Rest/test-api/Action/GlobalVisitsTest.php @@ -19,7 +19,9 @@ class GlobalVisitsTest extends ApiTestCase self::assertArrayHasKey('visits', $payload); self::assertArrayHasKey('visitsCount', $payload['visits']); + self::assertArrayHasKey('orphanVisitsCount', $payload['visits']); self::assertEquals($expectedVisits, $payload['visits']['visitsCount']); + self::assertEquals(3, $payload['visits']['orphanVisitsCount']); } public function provideApiKeys(): iterable diff --git a/module/Rest/test-api/Fixtures/VisitsFixture.php b/module/Rest/test-api/Fixtures/VisitsFixture.php index 1e3b3a5c..9fb53ac1 100644 --- a/module/Rest/test-api/Fixtures/VisitsFixture.php +++ b/module/Rest/test-api/Fixtures/VisitsFixture.php @@ -47,6 +47,14 @@ class VisitsFixture extends AbstractFixture implements DependentFixtureInterface Visit::forValidShortUrl($ghiShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')), ); + $manager->persist(Visit::forBasePath(new Visitor('shlink-tests-agent', 'https://doma.in', '1.2.3.4', ''))); + $manager->persist( + Visit::forRegularNotFound(new Visitor('shlink-tests-agent', 'https://doma.in/foo/bar', '1.2.3.4', '')), + ); + $manager->persist( + Visit::forInvalidShortUrl(new Visitor('shlink-tests-agent', 'https://doma.in/foo', '1.2.3.4', '')), + ); + $manager->flush(); } } diff --git a/module/Rest/test/Action/Visit/GlobalVisitsActionTest.php b/module/Rest/test/Action/Visit/GlobalVisitsActionTest.php index 6e3ab1e4..d53cb20d 100644 --- a/module/Rest/test/Action/Visit/GlobalVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/GlobalVisitsActionTest.php @@ -31,7 +31,7 @@ class GlobalVisitsActionTest extends TestCase public function statsAreReturnedFromHelper(): void { $apiKey = new ApiKey(); - $stats = new VisitsStats(5); + $stats = new VisitsStats(5, 3); $getStats = $this->helper->getVisitsStats($apiKey)->willReturn($stats); /** @var JsonResponse $resp */ From b01487ac91f67bd2bca3eb921a2ec3d98a53727f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 17:28:06 +0100 Subject: [PATCH 13/28] Ensured IP address is resolved when tracking orphan visits --- .../autoload/middleware-pipeline.global.php | 3 ++ .../ErrorHandler/NotFoundTemplateHandler.php | 8 ++--- .../NotFoundTemplateHandlerTest.php | 34 ++++++++++++++----- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/config/autoload/middleware-pipeline.global.php b/config/autoload/middleware-pipeline.global.php index b83ee2e7..1deff0fa 100644 --- a/config/autoload/middleware-pipeline.global.php +++ b/config/autoload/middleware-pipeline.global.php @@ -9,6 +9,7 @@ use Mezzio\Helper; use Mezzio\ProblemDetails; use Mezzio\Router; use PhpMiddleware\RequestId\RequestIdMiddleware; +use RKA\Middleware\IpAddress; return [ @@ -64,6 +65,8 @@ return [ ], 'not-found' => [ 'middleware' => [ + // This middleware is in front of tracking actions explicitly. Putting here for orphan visits tracking + IpAddress::class, Core\ErrorHandler\NotFoundTypeResolverMiddleware::class, Core\ErrorHandler\NotFoundTrackerMiddleware::class, Core\ErrorHandler\NotFoundRedirectHandler::class, diff --git a/module/Core/src/ErrorHandler/NotFoundTemplateHandler.php b/module/Core/src/ErrorHandler/NotFoundTemplateHandler.php index 62b78973..61d67403 100644 --- a/module/Core/src/ErrorHandler/NotFoundTemplateHandler.php +++ b/module/Core/src/ErrorHandler/NotFoundTemplateHandler.php @@ -7,10 +7,10 @@ namespace Shlinkio\Shlink\Core\ErrorHandler; use Closure; use Fig\Http\Message\StatusCodeInterface; use Laminas\Diactoros\Response; -use Mezzio\Router\RouteResult; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\RequestHandlerInterface; +use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use function file_get_contents; use function sprintf; @@ -29,11 +29,11 @@ class NotFoundTemplateHandler implements RequestHandlerInterface public function handle(ServerRequestInterface $request): ResponseInterface { - /** @var RouteResult $routeResult */ - $routeResult = $request->getAttribute(RouteResult::class) ?? RouteResult::fromRouteFailure(null); + /** @var NotFoundType $notFoundType */ + $notFoundType = $request->getAttribute(NotFoundType::class); $status = StatusCodeInterface::STATUS_NOT_FOUND; - $template = $routeResult->isFailure() ? self::NOT_FOUND_TEMPLATE : self::INVALID_SHORT_CODE_TEMPLATE; + $template = $notFoundType->isInvalidShortUrl() ? self::INVALID_SHORT_CODE_TEMPLATE : self::NOT_FOUND_TEMPLATE; $templateContent = ($this->readFile)(sprintf('%s/%s', self::TEMPLATES_BASE_DIR, $template)); return new Response\HtmlResponse($templateContent, $status); } diff --git a/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php b/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php index 6b9f9989..b5c80de4 100644 --- a/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php +++ b/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php @@ -7,27 +7,29 @@ namespace ShlinkioTest\Shlink\Core\ErrorHandler; use Closure; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequestFactory; +use Laminas\Diactoros\Uri; use Mezzio\Router\Route; use Mezzio\Router\RouteResult; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; +use Shlinkio\Shlink\Core\Action\RedirectAction; +use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\ErrorHandler\NotFoundTemplateHandler; class NotFoundTemplateHandlerTest extends TestCase { private NotFoundTemplateHandler $handler; - private Closure $readFile; private bool $readFileCalled; public function setUp(): void { $this->readFileCalled = false; - $this->readFile = function (string $fileName): string { + $readFile = function (string $fileName): string { $this->readFileCalled = true; return $fileName; }; - $this->handler = new NotFoundTemplateHandler($this->readFile); + $this->handler = new NotFoundTemplateHandler($readFile); } /** @@ -45,15 +47,29 @@ class NotFoundTemplateHandlerTest extends TestCase public function provideTemplates(): iterable { - $request = ServerRequestFactory::fromGlobals(); + $request = ServerRequestFactory::fromGlobals()->withUri(new Uri('/foo')); - yield [$request, NotFoundTemplateHandler::NOT_FOUND_TEMPLATE]; - yield [ - $request->withAttribute( + yield 'base url' => [$this->withNotFoundType($request, '/foo'), NotFoundTemplateHandler::NOT_FOUND_TEMPLATE]; + yield 'regular not found' => [$this->withNotFoundType($request), NotFoundTemplateHandler::NOT_FOUND_TEMPLATE]; + yield 'invalid short code' => [ + $this->withNotFoundType($request->withAttribute( RouteResult::class, - RouteResult::fromRoute(new Route('', $this->prophesize(MiddlewareInterface::class)->reveal())), - ), + RouteResult::fromRoute( + new Route( + '', + $this->prophesize(MiddlewareInterface::class)->reveal(), + ['GET'], + RedirectAction::class, + ), + ), + )), NotFoundTemplateHandler::INVALID_SHORT_CODE_TEMPLATE, ]; } + + private function withNotFoundType(ServerRequestInterface $req, string $baseUrl = ''): ServerRequestInterface + { + $type = NotFoundType::fromRequest($req, $baseUrl); + return $req->withAttribute(NotFoundType::class, $type); + } } From ab9042db2464031b8d0310bef5ac82e5a8928098 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 20:25:28 +0100 Subject: [PATCH 14/28] Ensured orphan visits are located ASAP when using swoole --- .../Core/config/event_dispatcher.config.php | 10 ++-- module/Core/src/Entity/Visit.php | 5 ++ .../{ShortUrlVisited.php => UrlVisited.php} | 2 +- ...ocateShortUrlVisit.php => LocateVisit.php} | 10 ++-- module/Core/src/Visit/VisitsTracker.php | 15 +++--- .../NotFoundTemplateHandlerTest.php | 1 - ...rtUrlVisitTest.php => LocateVisitTest.php} | 49 ++++++++++++------- module/Core/test/Visit/VisitsTrackerTest.php | 4 +- 8 files changed, 57 insertions(+), 39 deletions(-) rename module/Core/src/EventDispatcher/Event/{ShortUrlVisited.php => UrlVisited.php} (88%) rename module/Core/src/EventDispatcher/{LocateShortUrlVisit.php => LocateVisit.php} (93%) rename module/Core/test/EventDispatcher/{LocateShortUrlVisitTest.php => LocateVisitTest.php} (85%) diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 66a23637..5c2c88e0 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -20,28 +20,28 @@ return [ ], ], 'async' => [ - EventDispatcher\Event\ShortUrlVisited::class => [ - EventDispatcher\LocateShortUrlVisit::class, + EventDispatcher\Event\UrlVisited::class => [ + EventDispatcher\LocateVisit::class, ], ], ], 'dependencies' => [ 'factories' => [ - EventDispatcher\LocateShortUrlVisit::class => ConfigAbstractFactory::class, + EventDispatcher\LocateVisit::class => ConfigAbstractFactory::class, EventDispatcher\NotifyVisitToWebHooks::class => ConfigAbstractFactory::class, EventDispatcher\NotifyVisitToMercure::class => ConfigAbstractFactory::class, ], 'delegators' => [ - EventDispatcher\LocateShortUrlVisit::class => [ + EventDispatcher\LocateVisit::class => [ EventDispatcher\CloseDbConnectionEventListenerDelegator::class, ], ], ], ConfigAbstractFactory::class => [ - EventDispatcher\LocateShortUrlVisit::class => [ + EventDispatcher\LocateVisit::class => [ IpLocationResolverInterface::class, 'em', 'Logger_Shlink', diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index d61e8af6..3bcee8e1 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -109,6 +109,11 @@ class Visit extends AbstractEntity implements JsonSerializable return $this; } + public function isOrphan(): bool + { + return $this->shortUrl === null; + } + public function jsonSerialize(): array { return [ diff --git a/module/Core/src/EventDispatcher/Event/ShortUrlVisited.php b/module/Core/src/EventDispatcher/Event/UrlVisited.php similarity index 88% rename from module/Core/src/EventDispatcher/Event/ShortUrlVisited.php rename to module/Core/src/EventDispatcher/Event/UrlVisited.php index f177721f..87b9e4cb 100644 --- a/module/Core/src/EventDispatcher/Event/ShortUrlVisited.php +++ b/module/Core/src/EventDispatcher/Event/UrlVisited.php @@ -4,7 +4,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\EventDispatcher\Event; -final class ShortUrlVisited extends AbstractVisitEvent +final class UrlVisited extends AbstractVisitEvent { private ?string $originalIpAddress; diff --git a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php b/module/Core/src/EventDispatcher/LocateVisit.php similarity index 93% rename from module/Core/src/EventDispatcher/LocateShortUrlVisit.php rename to module/Core/src/EventDispatcher/LocateVisit.php index 8b193578..5e3baf74 100644 --- a/module/Core/src/EventDispatcher/LocateShortUrlVisit.php +++ b/module/Core/src/EventDispatcher/LocateVisit.php @@ -11,7 +11,7 @@ use Shlinkio\Shlink\CLI\Exception\GeolocationDbUpdateFailedException; use Shlinkio\Shlink\CLI\Util\GeolocationDbUpdaterInterface; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; @@ -19,7 +19,7 @@ use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use function sprintf; -class LocateShortUrlVisit +class LocateVisit { private IpLocationResolverInterface $ipLocationResolver; private EntityManagerInterface $em; @@ -41,7 +41,7 @@ class LocateShortUrlVisit $this->eventDispatcher = $eventDispatcher; } - public function __invoke(ShortUrlVisited $shortUrlVisited): void + public function __invoke(UrlVisited $shortUrlVisited): void { $visitId = $shortUrlVisited->visitId(); @@ -58,7 +58,9 @@ class LocateShortUrlVisit $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit); } - $this->eventDispatcher->dispatch(new VisitLocated($visitId)); + if (! $visit->isOrphan()) { + $this->eventDispatcher->dispatch(new VisitLocated($visitId)); + } } private function downloadOrUpdateGeoLiteDb(string $visitId): bool diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index af170cfc..48157e3b 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -8,7 +8,7 @@ use Doctrine\ORM; use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; class VisitsTracker implements VisitsTrackerInterface @@ -29,30 +29,29 @@ class VisitsTracker implements VisitsTrackerInterface public function track(ShortUrl $shortUrl, Visitor $visitor): void { - $visit = $this->trackVisit(Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr)); - $this->eventDispatcher->dispatch(new ShortUrlVisited($visit->getId(), $visitor->getRemoteAddress())); + $this->trackVisit(Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr), $visitor); } public function trackInvalidShortUrlVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forInvalidShortUrl($visitor)); + $this->trackVisit(Visit::forInvalidShortUrl($visitor, $this->anonymizeRemoteAddr), $visitor); } public function trackBaseUrlVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forBasePath($visitor)); + $this->trackVisit(Visit::forBasePath($visitor, $this->anonymizeRemoteAddr), $visitor); } public function trackRegularNotFoundVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forRegularNotFound($visitor)); + $this->trackVisit(Visit::forRegularNotFound($visitor, $this->anonymizeRemoteAddr), $visitor); } - private function trackVisit(Visit $visit): Visit + private function trackVisit(Visit $visit, Visitor $visitor): void { $this->em->persist($visit); $this->em->flush(); - return $visit; + $this->eventDispatcher->dispatch(new UrlVisited($visit->getId(), $visitor->getRemoteAddress())); } } diff --git a/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php b/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php index b5c80de4..dcf42b54 100644 --- a/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php +++ b/module/Core/test/ErrorHandler/NotFoundTemplateHandlerTest.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ErrorHandler; -use Closure; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequestFactory; use Laminas\Diactoros\Uri; diff --git a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php b/module/Core/test/EventDispatcher/LocateVisitTest.php similarity index 85% rename from module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php rename to module/Core/test/EventDispatcher/LocateVisitTest.php index 03eef5f6..af21e3dc 100644 --- a/module/Core/test/EventDispatcher/LocateShortUrlVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateVisitTest.php @@ -17,19 +17,19 @@ use Shlinkio\Shlink\Common\Util\IpAddress; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Entity\VisitLocation; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; -use Shlinkio\Shlink\Core\EventDispatcher\LocateShortUrlVisit; +use Shlinkio\Shlink\Core\EventDispatcher\LocateVisit; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\IpGeolocation\Exception\WrongIpException; use Shlinkio\Shlink\IpGeolocation\Model\Location; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; -class LocateShortUrlVisitTest extends TestCase +class LocateVisitTest extends TestCase { use ProphecyTrait; - private LocateShortUrlVisit $locateVisit; + private LocateVisit $locateVisit; private ObjectProphecy $ipLocationResolver; private ObjectProphecy $em; private ObjectProphecy $logger; @@ -44,7 +44,7 @@ class LocateShortUrlVisitTest extends TestCase $this->dbUpdater = $this->prophesize(GeolocationDbUpdaterInterface::class); $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); - $this->locateVisit = new LocateShortUrlVisit( + $this->locateVisit = new LocateVisit( $this->ipLocationResolver->reveal(), $this->em->reveal(), $this->logger->reveal(), @@ -56,7 +56,7 @@ class LocateShortUrlVisitTest extends TestCase /** @test */ public function invalidVisitLogsWarning(): void { - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn(null); $logWarning = $this->logger->warning('Tried to locate visit with id "{visitId}", but it does not exist.', [ 'visitId' => 123, @@ -76,7 +76,7 @@ class LocateShortUrlVisitTest extends TestCase /** @test */ public function invalidAddressLogsWarning(): void { - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn( Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), ); @@ -105,7 +105,7 @@ class LocateShortUrlVisitTest extends TestCase */ public function nonLocatableVisitsResolveToEmptyLocations(Visit $visit): void { - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { }); @@ -136,12 +136,14 @@ class LocateShortUrlVisitTest extends TestCase * @test * @dataProvider provideIpAddresses */ - public function locatableVisitsResolveToLocation(string $anonymizedIpAddress, ?string $originalIpAddress): void - { - $ipAddr = $originalIpAddress ?? $anonymizedIpAddress; - $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); + public function locatableVisitsResolveToLocation( + Visit $visit, + ?string $originalIpAddress, + int $expectedDispatchCalls + ): void { + $ipAddr = $originalIpAddress ?? $visit->getRemoteAddr(); $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new ShortUrlVisited('123', $originalIpAddress); + $event = new UrlVisited('123', $originalIpAddress); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { @@ -157,13 +159,24 @@ class LocateShortUrlVisitTest extends TestCase $flush->shouldHaveBeenCalledOnce(); $resolveIp->shouldHaveBeenCalledOnce(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); - $dispatch->shouldHaveBeenCalledOnce(); + $dispatch->shouldHaveBeenCalledTimes($expectedDispatchCalls); } public function provideIpAddresses(): iterable { - yield 'no original IP address' => ['1.2.3.0', null]; - yield 'original IP address' => ['1.2.3.0', '1.2.3.4']; + yield 'no original IP address' => [ + Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + null, + 1, + ]; + yield 'original IP address' => [ + Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), + '1.2.3.4', + 1, + ]; + yield 'base url' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; + yield 'invalid short url' => [Visit::forInvalidShortUrl(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; + yield 'regular not found' => [Visit::forRegularNotFound(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; } /** @test */ @@ -173,7 +186,7 @@ class LocateShortUrlVisitTest extends TestCase $ipAddr = '1.2.3.0'; $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { @@ -204,7 +217,7 @@ class LocateShortUrlVisitTest extends TestCase $ipAddr = '1.2.3.0'; $visit = Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', $ipAddr, '')); $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new ShortUrlVisited('123'); + $event = new UrlVisited('123'); $findVisit = $this->em->find(Visit::class, '123')->willReturn($visit); $flush = $this->em->flush()->will(function (): void { diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index a7ca98c3..2d8585d0 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -12,7 +12,7 @@ use Prophecy\Prophecy\ObjectProphecy; use Psr\EventDispatcher\EventDispatcherInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; -use Shlinkio\Shlink\Core\EventDispatcher\Event\ShortUrlVisited; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Visit\VisitsTracker; @@ -42,6 +42,6 @@ class VisitsTrackerTest extends TestCase $this->visitsTracker->track(ShortUrl::withLongUrl($shortCode), Visitor::emptyInstance()); - $this->eventDispatcher->dispatch(Argument::type(ShortUrlVisited::class))->shouldHaveBeenCalled(); + $this->eventDispatcher->dispatch(Argument::type(UrlVisited::class))->shouldHaveBeenCalled(); } } From 1fbcb44136bf6035fc0e169072cc3b18e24e8f19 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 20:34:12 +0100 Subject: [PATCH 15/28] Enhanced VisitsTrackerTest --- module/Core/test/Visit/VisitsTrackerTest.php | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index 2d8585d0..fd6e341f 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -32,16 +32,25 @@ class VisitsTrackerTest extends TestCase $this->visitsTracker = new VisitsTracker($this->em->reveal(), $this->eventDispatcher->reveal(), true); } - /** @test */ - public function trackPersistsVisit(): void + /** + * @test + * @dataProvider provideTrackingMethodNames + */ + public function trackPersistsVisitAndDispatchesEvent(string $method, array $args): void { - $shortCode = '123ABC'; - $this->em->persist(Argument::that(fn (Visit $visit) => $visit->setId('1')))->shouldBeCalledOnce(); $this->em->flush()->shouldBeCalledOnce(); - $this->visitsTracker->track(ShortUrl::withLongUrl($shortCode), Visitor::emptyInstance()); + $this->visitsTracker->{$method}(...$args); $this->eventDispatcher->dispatch(Argument::type(UrlVisited::class))->shouldHaveBeenCalled(); } + + public function provideTrackingMethodNames(): iterable + { + yield 'track' => ['track', [ShortUrl::createEmpty(), Visitor::emptyInstance()]]; + yield 'trackInvalidShortUrlVisit' => ['trackInvalidShortUrlVisit', [Visitor::emptyInstance()]]; + yield 'trackBaseUrlVisit' => ['trackBaseUrlVisit', [Visitor::emptyInstance()]]; + yield 'trackRegularNotFoundVisit' => ['trackRegularNotFoundVisit', [Visitor::emptyInstance()]]; + } } From 85dd023c0e37c90ca32f57d02258eb3e3f8a3081 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 21:22:36 +0100 Subject: [PATCH 16/28] Created methods to get orphan visits lists --- composer.json | 2 +- module/Core/src/Entity/Visit.php | 19 ++-- .../Core/src/Repository/VisitRepository.php | 33 ++++--- .../Repository/VisitRepositoryInterface.php | 9 +- module/Core/src/Spec/InDateRange.php | 38 ++++++++ .../src/Visit/Spec/CountOfOrphanVisits.php | 15 +++- .../Repository/VisitRepositoryTest.php | 87 ++++++++++++++++++- 7 files changed, 173 insertions(+), 30 deletions(-) create mode 100644 module/Core/src/Spec/InDateRange.php diff --git a/composer.json b/composer.json index 29068f06..ec9a028d 100644 --- a/composer.json +++ b/composer.json @@ -47,7 +47,7 @@ "predis/predis": "^1.1", "pugx/shortid-php": "^0.7", "ramsey/uuid": "^3.9", - "shlinkio/shlink-common": "dev-main#b889f5d as 3.5", + "shlinkio/shlink-common": "dev-main#62d4b84 as 3.5", "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^2.0", "shlinkio/shlink-importer": "^2.2", diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 3bcee8e1..efcf5d8f 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -28,15 +28,10 @@ class Visit extends AbstractEntity implements JsonSerializable private ?ShortUrl $shortUrl; private ?VisitLocation $visitLocation = null; - public function __construct( - ?ShortUrl $shortUrl, - Visitor $visitor, - bool $anonymize = true, - ?Chronos $date = null, - string $type = self::TYPE_VALID_SHORT_URL - ) { + private function __construct(?ShortUrl $shortUrl, Visitor $visitor, string $type, bool $anonymize = true) + { $this->shortUrl = $shortUrl; - $this->date = $date ?? Chronos::now(); + $this->date = Chronos::now(); $this->userAgent = $visitor->getUserAgent(); $this->referer = $visitor->getReferer(); $this->remoteAddr = $this->processAddress($anonymize, $visitor->getRemoteAddress()); @@ -60,22 +55,22 @@ class Visit extends AbstractEntity implements JsonSerializable public static function forValidShortUrl(ShortUrl $shortUrl, Visitor $visitor, bool $anonymize = true): self { - return new self($shortUrl, $visitor, $anonymize); + return new self($shortUrl, $visitor, self::TYPE_VALID_SHORT_URL, $anonymize); } public static function forBasePath(Visitor $visitor, bool $anonymize = true): self { - return new self(null, $visitor, $anonymize, null, self::TYPE_BASE_URL); + return new self(null, $visitor, self::TYPE_BASE_URL, $anonymize); } public static function forInvalidShortUrl(Visitor $visitor, bool $anonymize = true): self { - return new self(null, $visitor, $anonymize, null, self::TYPE_INVALID_SHORT_URL); + return new self(null, $visitor, self::TYPE_INVALID_SHORT_URL, $anonymize); } public static function forRegularNotFound(Visitor $visitor, bool $anonymize = true): self { - return new self(null, $visitor, $anonymize, null, self::TYPE_REGULAR_404); + return new self(null, $visitor, self::TYPE_REGULAR_404, $anonymize); } public function getRemoteAddr(): ?string diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 082b17b8..b869093e 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -168,6 +168,29 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo return $qb; } + public function findOrphanVisits(?DateRange $dateRange = null, ?int $limit = null, ?int $offset = null): array + { + // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later + // Since they are not strictly provided by the caller, it's reasonably safe + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->from(Visit::class, 'v') + ->where($qb->expr()->isNull('v.shortUrl')); + + $this->applyDatesInline($qb, $dateRange); + + return $this->resolveVisitsWithNativeQuery($qb, $limit, $offset); + } + + public function countOrphanVisits(?DateRange $dateRange = null): int + { + return (int) $this->matchSingleScalarResult(new CountOfOrphanVisits($dateRange)); + } + + public function countVisits(?ApiKey $apiKey = null): int + { + return (int) $this->matchSingleScalarResult(new CountOfShortUrlVisits($apiKey)); + } + private function applyDatesInline(QueryBuilder $qb, ?DateRange $dateRange): void { if ($dateRange !== null && $dateRange->getStartDate() !== null) { @@ -208,14 +231,4 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo return $query->getResult(); } - - public function countVisits(?ApiKey $apiKey = null): int - { - return (int) $this->matchSingleScalarResult(new CountOfShortUrlVisits($apiKey)); - } - - public function countOrphanVisits(): int - { - return (int) $this->matchSingleScalarResult(new CountOfOrphanVisits()); - } } diff --git a/module/Core/src/Repository/VisitRepositoryInterface.php b/module/Core/src/Repository/VisitRepositoryInterface.php index 0d637f75..3ecf0bca 100644 --- a/module/Core/src/Repository/VisitRepositoryInterface.php +++ b/module/Core/src/Repository/VisitRepositoryInterface.php @@ -62,7 +62,12 @@ interface VisitRepositoryInterface extends ObjectRepository, EntitySpecification public function countVisitsByTag(string $tag, ?DateRange $dateRange = null, ?Specification $spec = null): int; - public function countVisits(?ApiKey $apiKey = null): int; + /** + * @return Visit[] + */ + public function findOrphanVisits(?DateRange $dateRange = null, ?int $limit = null, ?int $offset = null): array; - public function countOrphanVisits(): int; + public function countOrphanVisits(?DateRange $dateRange = null): int; + + public function countVisits(?ApiKey $apiKey = null): int; } diff --git a/module/Core/src/Spec/InDateRange.php b/module/Core/src/Spec/InDateRange.php new file mode 100644 index 00000000..44944aed --- /dev/null +++ b/module/Core/src/Spec/InDateRange.php @@ -0,0 +1,38 @@ +dateRange = $dateRange; + $this->field = $field; + } + + protected function getSpec(): Specification + { + $criteria = []; + + if ($this->dateRange !== null && $this->dateRange->getStartDate() !== null) { + $criteria[] = Spec::gte($this->field, $this->dateRange->getStartDate()->toDateTimeString()); + } + + if ($this->dateRange !== null && $this->dateRange->getEndDate() !== null) { + $criteria[] = Spec::lte($this->field, $this->dateRange->getEndDate()->toDateTimeString()); + } + + return Spec::andX(...$criteria); + } +} diff --git a/module/Core/src/Visit/Spec/CountOfOrphanVisits.php b/module/Core/src/Visit/Spec/CountOfOrphanVisits.php index 7e15f330..fb8ee3bd 100644 --- a/module/Core/src/Visit/Spec/CountOfOrphanVisits.php +++ b/module/Core/src/Visit/Spec/CountOfOrphanVisits.php @@ -7,11 +7,24 @@ namespace Shlinkio\Shlink\Core\Visit\Spec; use Happyr\DoctrineSpecification\BaseSpecification; use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Specification\Specification; +use Shlinkio\Shlink\Common\Util\DateRange; +use Shlinkio\Shlink\Core\Spec\InDateRange; class CountOfOrphanVisits extends BaseSpecification { + private ?DateRange $dateRange; + + public function __construct(?DateRange $dateRange) + { + parent::__construct(); + $this->dateRange = $dateRange; + } + protected function getSpec(): Specification { - return Spec::countOf(Spec::isNull('shortUrl')); + return Spec::countOf(Spec::andX( + Spec::isNull('shortUrl'), + new InDateRange($this->dateRange), + )); } } diff --git a/module/Core/test-db/Repository/VisitRepositoryTest.php b/module/Core/test-db/Repository/VisitRepositoryTest.php index 00b558d4..b6c23699 100644 --- a/module/Core/test-db/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Repository/VisitRepositoryTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Repository; use Cake\Chronos\Chronos; +use ReflectionObject; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; @@ -214,6 +215,75 @@ class VisitRepositoryTest extends DatabaseTestCase self::assertEquals(3, $this->repo->countOrphanVisits()); } + /** @test */ + public function findOrphanVisitsReturnsExpectedResult(): void + { + $shortUrl = ShortUrl::fromMeta(ShortUrlMeta::fromRawData(['longUrl' => ''])); + $this->getEntityManager()->persist($shortUrl); + $this->createVisitsForShortUrl($shortUrl, 7); + + for ($i = 0; $i < 6; $i++) { + $this->getEntityManager()->persist($this->setDateOnVisit( + Visit::forBasePath(Visitor::emptyInstance()), + Chronos::parse(sprintf('2020-01-0%s', $i + 1)), + )); + $this->getEntityManager()->persist($this->setDateOnVisit( + Visit::forInvalidShortUrl(Visitor::emptyInstance()), + Chronos::parse(sprintf('2020-01-0%s', $i + 1)), + )); + $this->getEntityManager()->persist($this->setDateOnVisit( + Visit::forRegularNotFound(Visitor::emptyInstance()), + Chronos::parse(sprintf('2020-01-0%s', $i + 1)), + )); + } + + $this->getEntityManager()->flush(); + + self::assertCount(18, $this->repo->findOrphanVisits()); + self::assertCount(5, $this->repo->findOrphanVisits(null, 5)); + self::assertCount(10, $this->repo->findOrphanVisits(null, 15, 8)); + self::assertCount(9, $this->repo->findOrphanVisits(DateRange::withStartDate(Chronos::parse('2020-01-04')), 15)); + self::assertCount(2, $this->repo->findOrphanVisits( + DateRange::withStartAndEndDate(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), + 6, + 4, + )); + self::assertCount(3, $this->repo->findOrphanVisits(DateRange::withEndDate(Chronos::parse('2020-01-01')))); + } + + /** @test */ + public function countOrphanVisitsReturnsExpectedResult(): void + { + $shortUrl = ShortUrl::fromMeta(ShortUrlMeta::fromRawData(['longUrl' => ''])); + $this->getEntityManager()->persist($shortUrl); + $this->createVisitsForShortUrl($shortUrl, 7); + + for ($i = 0; $i < 6; $i++) { + $this->getEntityManager()->persist($this->setDateOnVisit( + Visit::forBasePath(Visitor::emptyInstance()), + Chronos::parse(sprintf('2020-01-0%s', $i + 1)), + )); + $this->getEntityManager()->persist($this->setDateOnVisit( + Visit::forInvalidShortUrl(Visitor::emptyInstance()), + Chronos::parse(sprintf('2020-01-0%s', $i + 1)), + )); + $this->getEntityManager()->persist($this->setDateOnVisit( + Visit::forRegularNotFound(Visitor::emptyInstance()), + Chronos::parse(sprintf('2020-01-0%s', $i + 1)), + )); + } + + $this->getEntityManager()->flush(); + + self::assertEquals(18, $this->repo->countOrphanVisits()); + self::assertEquals(18, $this->repo->countOrphanVisits(DateRange::emptyInstance())); + self::assertEquals(9, $this->repo->countOrphanVisits(DateRange::withStartDate(Chronos::parse('2020-01-04')))); + self::assertEquals(6, $this->repo->countOrphanVisits( + DateRange::withStartAndEndDate(Chronos::parse('2020-01-02'), Chronos::parse('2020-01-03')), + )); + self::assertEquals(3, $this->repo->countOrphanVisits(DateRange::withEndDate(Chronos::parse('2020-01-01')))); + } + private function createShortUrlsAndVisits(bool $withDomain = true, array $tags = []): array { $shortUrl = ShortUrl::fromMeta(ShortUrlMeta::fromRawData([ @@ -243,13 +313,22 @@ class VisitRepositoryTest extends DatabaseTestCase private function createVisitsForShortUrl(ShortUrl $shortUrl, int $amount = 6): void { for ($i = 0; $i < $amount; $i++) { - $visit = new Visit( - $shortUrl, - Visitor::emptyInstance(), - true, + $visit = $this->setDateOnVisit( + Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance()), Chronos::parse(sprintf('2016-01-0%s', $i + 1)), ); + $this->getEntityManager()->persist($visit); } } + + private function setDateOnVisit(Visit $visit, Chronos $date): Visit + { + $ref = new ReflectionObject($visit); + $dateProp = $ref->getProperty('date'); + $dateProp->setAccessible(true); + $dateProp->setValue($visit, $date); + + return $visit; + } } From dcf2526aad541a169f90e7cf990e6b33844ece48 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 22:03:03 +0100 Subject: [PATCH 17/28] Documented swagger for new orphan visits endpoint --- docs/swagger/definitions/OrphanVisit.json | 23 ++++ docs/swagger/definitions/Visit.json | 1 + docs/swagger/paths/v2_visits_orphan.json | 141 ++++++++++++++++++++++ docs/swagger/swagger.json | 3 + 4 files changed, 168 insertions(+) create mode 100644 docs/swagger/definitions/OrphanVisit.json create mode 100644 docs/swagger/paths/v2_visits_orphan.json diff --git a/docs/swagger/definitions/OrphanVisit.json b/docs/swagger/definitions/OrphanVisit.json new file mode 100644 index 00000000..04d8386d --- /dev/null +++ b/docs/swagger/definitions/OrphanVisit.json @@ -0,0 +1,23 @@ +{ + "type": "object", + "required": ["visitedUrl", "type"], + "allOf": [{ + "$ref": "./Visit.json" + }], + "properties": { + "visitedUrl": { + "type": "string", + "nullable": true, + "description": "The originally visited URL that triggered the tracking of this visit" + }, + "type": { + "type": "string", + "enum": [ + "invalid_short_url", + "base_url", + "regular_404" + ], + "description": "Tells the type of orphan visit" + } + } +} diff --git a/docs/swagger/definitions/Visit.json b/docs/swagger/definitions/Visit.json index 9e1eb5b5..e004e4fe 100644 --- a/docs/swagger/definitions/Visit.json +++ b/docs/swagger/definitions/Visit.json @@ -1,5 +1,6 @@ { "type": "object", + "required": ["referer", "date", "userAgent", "visitLocation"], "properties": { "referer": { "type": "string", diff --git a/docs/swagger/paths/v2_visits_orphan.json b/docs/swagger/paths/v2_visits_orphan.json new file mode 100644 index 00000000..683f40ec --- /dev/null +++ b/docs/swagger/paths/v2_visits_orphan.json @@ -0,0 +1,141 @@ +{ + "get": { + "operationId": "getOrphanVisits", + "tags": [ + "Visits" + ], + "summary": "List orphan visits", + "description": "Get the list of visits to invalid short URLs, the base URL or any other 404.", + "parameters": [ + { + "$ref": "../parameters/version.json" + }, + { + "name": "startDate", + "in": "query", + "description": "The date (in ISO-8601 format) from which we want to get visits.", + "required": false, + "schema": { + "type": "string" + } + }, + { + "name": "endDate", + "in": "query", + "description": "The date (in ISO-8601 format) until which we want to get visits.", + "required": false, + "schema": { + "type": "string" + } + }, + { + "name": "page", + "in": "query", + "description": "The page to display. Defaults to 1", + "required": false, + "schema": { + "type": "number" + } + }, + { + "name": "itemsPerPage", + "in": "query", + "description": "The amount of items to return on every page. Defaults to all the items", + "required": false, + "schema": { + "type": "number" + } + } + ], + "security": [ + { + "ApiKey": [] + } + ], + "responses": { + "200": { + "description": "List of visits.", + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "visits": { + "type": "object", + "properties": { + "data": { + "type": "array", + "items": { + "$ref": "../definitions/OrphanVisit.json" + } + }, + "pagination": { + "$ref": "../definitions/Pagination.json" + } + } + } + } + } + } + }, + "examples": { + "application/json": { + "visits": { + "data": [ + { + "referer": "https://twitter.com", + "date": "2015-08-20T05:05:03+04:00", + "userAgent": "Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0 Mozilla/5.0 (Macintosh; Intel Mac OS X x.y; rv:42.0) Gecko/20100101 Firefox/42.0", + "visitLocation": null, + "visitedUrl": "https://doma.in", + "type": "base_url" + }, + { + "referer": "https://t.co", + "date": "2015-08-20T05:05:03+04:00", + "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36", + "visitLocation": { + "cityName": "Cupertino", + "countryCode": "US", + "countryName": "United States", + "latitude": 37.3042, + "longitude": -122.0946, + "regionName": "California", + "timezone": "America/Los_Angeles" + }, + "visitedUrl": "https://doma.in/foo", + "type": "invalid_short_url" + }, + { + "referer": null, + "date": "2015-08-20T05:05:03+04:00", + "userAgent": "some_web_crawler/1.4", + "visitLocation": null, + "visitedUrl": "https://doma.in/foo/bar/baz", + "type": "regular_404" + } + ], + "pagination": { + "currentPage": 5, + "pagesCount": 12, + "itemsPerPage": 10, + "itemsInCurrentPage": 10, + "totalItems": 115 + } + } + } + } + }, + "500": { + "description": "Unexpected error.", + "content": { + "application/problem+json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + } + } + } +} diff --git a/docs/swagger/swagger.json b/docs/swagger/swagger.json index dc834905..21547f90 100644 --- a/docs/swagger/swagger.json +++ b/docs/swagger/swagger.json @@ -95,6 +95,9 @@ "/rest/v{version}/tags/{tag}/visits": { "$ref": "paths/v2_tags_{tag}_visits.json" }, + "/rest/v{version}/visits/orphan": { + "$ref": "paths/v2_visits_orphan.json" + }, "/rest/v{version}/domains": { "$ref": "paths/v2_domains.json" From 5d98316c4e95bde5d32944e1a70e5ea41bd0068a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 22:11:09 +0100 Subject: [PATCH 18/28] Created new REST API action to list orphan visits --- module/Core/config/dependencies.config.php | 12 ++++-- module/Core/functions/functions.php | 21 +++++++++ module/Core/src/Entity/Visit.php | 10 +++++ module/Core/src/Model/VisitsParams.php | 4 +- .../Adapter/OrphanVisitsPaginatorAdapter.php | 30 +++++++++++++ .../OrphanVisitDataTransformer.php | 24 +++++++++++ module/Core/src/Visit/VisitsStatsHelper.php | 28 +++++++++--- .../src/Visit/VisitsStatsHelperInterface.php | 5 +++ module/Rest/config/dependencies.config.php | 5 +++ module/Rest/config/routes.config.php | 1 + .../src/Action/Visit/OrphanVisitsAction.php | 43 +++++++++++++++++++ 11 files changed, 171 insertions(+), 12 deletions(-) create mode 100644 module/Core/src/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php create mode 100644 module/Core/src/Visit/Transformer/OrphanVisitDataTransformer.php create mode 100644 module/Rest/src/Action/Visit/OrphanVisitsAction.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 50669f66..43586e16 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -26,16 +26,20 @@ return [ Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Service\UrlShortener::class => ConfigAbstractFactory::class, - Visit\VisitsTracker::class => ConfigAbstractFactory::class, Service\ShortUrlService::class => ConfigAbstractFactory::class, - Visit\VisitLocator::class => ConfigAbstractFactory::class, - Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class, - Tag\TagService::class => ConfigAbstractFactory::class, Service\ShortUrl\DeleteShortUrlService::class => ConfigAbstractFactory::class, Service\ShortUrl\ShortUrlResolver::class => ConfigAbstractFactory::class, Service\ShortUrl\ShortCodeHelper::class => ConfigAbstractFactory::class, + + Tag\TagService::class => ConfigAbstractFactory::class, + Domain\DomainService::class => ConfigAbstractFactory::class, + Visit\VisitsTracker::class => ConfigAbstractFactory::class, + Visit\VisitLocator::class => ConfigAbstractFactory::class, + Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class, + Visit\Transformer\OrphanVisitDataTransformer::class => InvokableFactory::class, + Util\UrlValidator::class => ConfigAbstractFactory::class, Util\DoctrineBatchHelper::class => ConfigAbstractFactory::class, Util\RedirectResponseHelper::class => ConfigAbstractFactory::class, diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index f9a67e3d..00954049 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -9,6 +9,7 @@ use DateTimeInterface; use Fig\Http\Message\StatusCodeInterface; use Laminas\InputFilter\InputFilter; use PUGX\Shortid\Factory as ShortIdFactory; +use Shlinkio\Shlink\Common\Util\DateRange; use function Functional\reduce_left; use function is_array; @@ -44,6 +45,26 @@ function parseDateFromQuery(array $query, string $dateName): ?Chronos return ! isset($query[$dateName]) || empty($query[$dateName]) ? null : Chronos::parse($query[$dateName]); } +function parseDateRangeFromQuery(array $query, string $startDateName, string $endDateName): DateRange +{ + $startDate = parseDateFromQuery($query, $startDateName); + $endDate = parseDateFromQuery($query, $endDateName); + + if ($startDate === null && $endDate === null) { + return DateRange::emptyInstance(); + } + + if ($startDate !== null && $endDate !== null) { + return DateRange::withStartAndEndDate($startDate, $endDate); + } + + if ($startDate !== null) { + return DateRange::withStartDate($startDate); + } + + return DateRange::withEndDate($endDate); +} + /** * @param string|DateTimeInterface|Chronos|null $date */ diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index efcf5d8f..61739dec 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -109,6 +109,16 @@ class Visit extends AbstractEntity implements JsonSerializable return $this->shortUrl === null; } + public function visitedUrl(): ?string + { + return $this->visitedUrl; + } + + public function type(): string + { + return $this->type; + } + public function jsonSerialize(): array { return [ diff --git a/module/Core/src/Model/VisitsParams.php b/module/Core/src/Model/VisitsParams.php index 041aed9f..b579239b 100644 --- a/module/Core/src/Model/VisitsParams.php +++ b/module/Core/src/Model/VisitsParams.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\Model; use Shlinkio\Shlink\Common\Util\DateRange; -use function Shlinkio\Shlink\Core\parseDateFromQuery; +use function Shlinkio\Shlink\Core\parseDateRangeFromQuery; final class VisitsParams { @@ -36,7 +36,7 @@ final class VisitsParams public static function fromRawData(array $query): self { return new self( - new DateRange(parseDateFromQuery($query, 'startDate'), parseDateFromQuery($query, 'endDate')), + parseDateRangeFromQuery($query, 'startDate', 'endDate'), (int) ($query['page'] ?? 1), isset($query['itemsPerPage']) ? (int) $query['itemsPerPage'] : null, ); diff --git a/module/Core/src/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php b/module/Core/src/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php new file mode 100644 index 00000000..7167b9e7 --- /dev/null +++ b/module/Core/src/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php @@ -0,0 +1,30 @@ +repo = $repo; + $this->params = $params; + } + + protected function doCount(): int + { + return $this->repo->countOrphanVisits($this->params->getDateRange()); + } + + public function getSlice($offset, $length): iterable // phpcs:ignore + { + return $this->repo->findOrphanVisits($this->params->getDateRange(), $length, $offset); + } +} diff --git a/module/Core/src/Visit/Transformer/OrphanVisitDataTransformer.php b/module/Core/src/Visit/Transformer/OrphanVisitDataTransformer.php new file mode 100644 index 00000000..9f4842f5 --- /dev/null +++ b/module/Core/src/Visit/Transformer/OrphanVisitDataTransformer.php @@ -0,0 +1,24 @@ +jsonSerialize(); + $serializedVisit['visitedUrl'] = $visit->visitedUrl(); + $serializedVisit['type'] = $visit->type(); + + return $serializedVisit; + } +} diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index 0cb58897..61d879fd 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Visit; use Doctrine\ORM\EntityManagerInterface; +use Pagerfanta\Adapter\AdapterInterface; use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; @@ -13,6 +14,7 @@ use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\VisitsParams; +use Shlinkio\Shlink\Core\Paginator\Adapter\OrphanVisitsPaginatorAdapter; use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsForTagPaginatorAdapter; use Shlinkio\Shlink\Core\Paginator\Adapter\VisitsPaginatorAdapter; use Shlinkio\Shlink\Core\Repository\ShortUrlRepositoryInterface; @@ -58,11 +60,8 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface /** @var VisitRepositoryInterface $repo */ $repo = $this->em->getRepository(Visit::class); - $paginator = new Paginator(new VisitsPaginatorAdapter($repo, $identifier, $params, $spec)); - $paginator->setMaxPerPage($params->getItemsPerPage()) - ->setCurrentPage($params->getPage()); - return $paginator; + return $this->createPaginator(new VisitsPaginatorAdapter($repo, $identifier, $params, $spec), $params); } /** @@ -79,9 +78,26 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface /** @var VisitRepositoryInterface $repo */ $repo = $this->em->getRepository(Visit::class); - $paginator = new Paginator(new VisitsForTagPaginatorAdapter($repo, $tag, $params, $apiKey)); + + return $this->createPaginator(new VisitsForTagPaginatorAdapter($repo, $tag, $params, $apiKey), $params); + } + + /** + * @return Visit[]|Paginator + */ + public function orphanVisits(VisitsParams $params): Paginator + { + /** @var VisitRepositoryInterface $repo */ + $repo = $this->em->getRepository(Visit::class); + + return $this->createPaginator(new OrphanVisitsPaginatorAdapter($repo, $params), $params); + } + + private function createPaginator(AdapterInterface $adapter, VisitsParams $params): Paginator + { + $paginator = new Paginator($adapter); $paginator->setMaxPerPage($params->getItemsPerPage()) - ->setCurrentPage($params->getPage()); + ->setCurrentPage($params->getPage()); return $paginator; } diff --git a/module/Core/src/Visit/VisitsStatsHelperInterface.php b/module/Core/src/Visit/VisitsStatsHelperInterface.php index a67c8dcd..d2bf6032 100644 --- a/module/Core/src/Visit/VisitsStatsHelperInterface.php +++ b/module/Core/src/Visit/VisitsStatsHelperInterface.php @@ -32,4 +32,9 @@ interface VisitsStatsHelperInterface * @throws TagNotFoundException */ public function visitsForTag(string $tag, VisitsParams $params, ?ApiKey $apiKey = null): Paginator; + + /** + * @return Visit[]|Paginator + */ + public function orphanVisits(VisitsParams $params): Paginator; } diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 5b68ada7..e1a869df 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -34,6 +34,7 @@ return [ Action\Visit\ShortUrlVisitsAction::class => ConfigAbstractFactory::class, Action\Visit\TagVisitsAction::class => ConfigAbstractFactory::class, Action\Visit\GlobalVisitsAction::class => ConfigAbstractFactory::class, + Action\Visit\OrphanVisitsAction::class => ConfigAbstractFactory::class, Action\Tag\ListTagsAction::class => ConfigAbstractFactory::class, Action\Tag\DeleteTagsAction::class => ConfigAbstractFactory::class, Action\Tag\CreateTagsAction::class => ConfigAbstractFactory::class, @@ -69,6 +70,10 @@ return [ Action\Visit\ShortUrlVisitsAction::class => [Visit\VisitsStatsHelper::class], Action\Visit\TagVisitsAction::class => [Visit\VisitsStatsHelper::class], Action\Visit\GlobalVisitsAction::class => [Visit\VisitsStatsHelper::class], + Action\Visit\OrphanVisitsAction::class => [ + Visit\VisitsStatsHelper::class, + Visit\Transformer\OrphanVisitDataTransformer::class, + ], Action\ShortUrl\ListShortUrlsAction::class => [Service\ShortUrlService::class, ShortUrlDataTransformer::class], Action\ShortUrl\EditShortUrlTagsAction::class => [Service\ShortUrlService::class], Action\Tag\ListTagsAction::class => [TagService::class], diff --git a/module/Rest/config/routes.config.php b/module/Rest/config/routes.config.php index a5382c38..9b09a266 100644 --- a/module/Rest/config/routes.config.php +++ b/module/Rest/config/routes.config.php @@ -34,6 +34,7 @@ return [ Action\Visit\ShortUrlVisitsAction::getRouteDef([$dropDomainMiddleware]), Action\Visit\TagVisitsAction::getRouteDef(), Action\Visit\GlobalVisitsAction::getRouteDef(), + Action\Visit\OrphanVisitsAction::getRouteDef(), // Tags Action\Tag\ListTagsAction::getRouteDef(), diff --git a/module/Rest/src/Action/Visit/OrphanVisitsAction.php b/module/Rest/src/Action/Visit/OrphanVisitsAction.php new file mode 100644 index 00000000..7a65b920 --- /dev/null +++ b/module/Rest/src/Action/Visit/OrphanVisitsAction.php @@ -0,0 +1,43 @@ +visitsHelper = $visitsHelper; + $this->orphanVisitTransformer = $orphanVisitTransformer; + } + + public function handle(ServerRequestInterface $request): ResponseInterface + { + $params = VisitsParams::fromRawData($request->getQueryParams()); + $visits = $this->visitsHelper->orphanVisits($params); + + return new JsonResponse([ + 'visits' => $this->serializePaginator($visits, $this->orphanVisitTransformer), + ]); + } +} From bd9ec53e7b8b8aa7322bdcee5da25ef0b8e29ea0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 22:40:40 +0100 Subject: [PATCH 19/28] Added test for VisitsStatsHelper::orphanVisits --- .../Core/test/Visit/VisitsStatsHelperTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index 6fefd1d0..de2a3534 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -27,6 +27,7 @@ use Shlinkio\Shlink\Core\Visit\VisitsStatsHelper; use Shlinkio\Shlink\Rest\Entity\ApiKey; use ShlinkioTest\Shlink\Core\Util\ApiKeyHelpersTrait; +use function count; use function Functional\map; use function range; @@ -148,4 +149,21 @@ class VisitsStatsHelperTest extends TestCase $tagExists->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); } + + /** @test */ + public function orphanVisitsAreReturnedAsExpected(): void + { + $list = map(range(0, 3), fn () => Visit::forBasePath(Visitor::emptyInstance())); + $repo = $this->prophesize(VisitRepository::class); + $countVisits = $repo->countOrphanVisits(Argument::type(DateRange::class))->willReturn(count($list)); + $listVisits = $repo->findOrphanVisits(Argument::type(DateRange::class), Argument::cetera())->willReturn($list); + $getRepo = $this->em->getRepository(Visit::class)->willReturn($repo->reveal()); + + $paginator = $this->helper->orphanVisits(new VisitsParams()); + + self::assertEquals($list, ArrayUtils::iteratorToArray($paginator->getCurrentPageResults())); + $listVisits->shouldHaveBeenCalledOnce(); + $countVisits->shouldHaveBeenCalledOnce(); + $getRepo->shouldHaveBeenCalledOnce(); + } } From d5794a3dcb50d0a3d6fcd1e6caab0b92c54ae5e6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 22:52:44 +0100 Subject: [PATCH 20/28] Created OrphanVisitDataTransformerTest --- .../OrphanVisitDataTransformerTest.php | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 module/Core/test/Visit/Transformer/OrphanVisitDataTransformerTest.php diff --git a/module/Core/test/Visit/Transformer/OrphanVisitDataTransformerTest.php b/module/Core/test/Visit/Transformer/OrphanVisitDataTransformerTest.php new file mode 100644 index 00000000..cf36c052 --- /dev/null +++ b/module/Core/test/Visit/Transformer/OrphanVisitDataTransformerTest.php @@ -0,0 +1,82 @@ +transformer = new OrphanVisitDataTransformer(); + } + + /** + * @test + * @dataProvider provideVisits + */ + public function visitsAreParsedAsExpected(Visit $visit, array $expectedResult): void + { + $result = $this->transformer->transform($visit); + + self::assertEquals($expectedResult, $result); + } + + public function provideVisits(): iterable + { + yield 'base path visit' => [ + $visit = Visit::forBasePath(Visitor::emptyInstance()), + [ + 'referer' => '', + 'date' => $visit->getDate()->toAtomString(), + 'userAgent' => '', + 'visitLocation' => null, + 'visitedUrl' => '', + 'type' => Visit::TYPE_BASE_URL, + ], + ]; + yield 'invalid short url visit' => [ + $visit = Visit::forInvalidShortUrl(Visitor::fromRequest( + ServerRequestFactory::fromGlobals()->withHeader('User-Agent', 'foo') + ->withHeader('Referer', 'bar') + ->withUri(new Uri('https://example.com/foo')), + )), + [ + 'referer' => 'bar', + 'date' => $visit->getDate()->toAtomString(), + 'userAgent' => 'foo', + 'visitLocation' => null, + 'visitedUrl' => 'https://example.com/foo', + 'type' => Visit::TYPE_INVALID_SHORT_URL, + ], + ]; + yield 'regular 404 visit' => [ + $visit = Visit::forRegularNotFound( + Visitor::fromRequest( + ServerRequestFactory::fromGlobals()->withHeader('User-Agent', 'user-agent') + ->withHeader('Referer', 'referer') + ->withUri(new Uri('https://doma.in/foo/bar')), + ), + )->locate($location = new VisitLocation(Location::emptyInstance())), + [ + 'referer' => 'referer', + 'date' => $visit->getDate()->toAtomString(), + 'userAgent' => 'user-agent', + 'visitLocation' => $location, + 'visitedUrl' => 'https://doma.in/foo/bar', + 'type' => Visit::TYPE_REGULAR_404, + ], + ]; + } +} From 3497165ebd3d48f319c3ddb1cad7ad358bb29aa1 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 23:34:29 +0100 Subject: [PATCH 21/28] Created OrphanVisitsPaginatorAdapterTest --- .../OrphanVisitsPaginatorAdapterTest.php | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) create mode 100644 module/Core/test/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php diff --git a/module/Core/test/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php b/module/Core/test/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php new file mode 100644 index 00000000..6b28aa68 --- /dev/null +++ b/module/Core/test/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php @@ -0,0 +1,65 @@ +repo = $this->prophesize(VisitRepositoryInterface::class); + $this->params = VisitsParams::fromRawData([]); + $this->adapter = new OrphanVisitsPaginatorAdapter($this->repo->reveal(), $this->params); + } + + /** @test */ + public function countDelegatesToRepository(): void + { + $expectedCount = 5; + $repoCount = $this->repo->countOrphanVisits($this->params->getDateRange())->willReturn($expectedCount); + + $result = $this->adapter->getNbResults(); + + self::assertEquals($expectedCount, $result); + $repoCount->shouldHaveBeenCalledOnce(); + } + + /** + * @test + * @dataProvider provideLimitAndOffset + */ + public function getSliceDelegatesToRepository(int $limit, int $offset): void + { + $visitor = Visitor::emptyInstance(); + $list = [Visit::forRegularNotFound($visitor), Visit::forInvalidShortUrl($visitor)]; + $repoFind = $this->repo->findOrphanVisits($this->params->getDateRange(), $limit, $offset)->willReturn($list); + + $result = $this->adapter->getSlice($offset, $limit); + + self::assertEquals($list, $result); + $repoFind->shouldHaveBeenCalledOnce(); + } + + public function provideLimitAndOffset(): iterable + { + yield [1, 5]; + yield [10, 4]; + yield [30, 18]; + } +} From 82f4e22f691439a083cb0115af798f53bd153560 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 23:41:51 +0100 Subject: [PATCH 22/28] Created OrphanVisitsActionTest --- .../Action/Visit/OrphanVisitsActionTest.php | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 module/Rest/test/Action/Visit/OrphanVisitsActionTest.php diff --git a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php new file mode 100644 index 00000000..36273d09 --- /dev/null +++ b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php @@ -0,0 +1,58 @@ +visitsHelper = $this->prophesize(VisitsStatsHelperInterface::class); + $this->orphanVisitTransformer = $this->prophesize(DataTransformerInterface::class); + + $this->action = new OrphanVisitsAction($this->visitsHelper->reveal(), $this->orphanVisitTransformer->reveal()); + } + + /** @test */ + public function requestIsHandled(): void + { + $visitor = Visitor::emptyInstance(); + $visits = [Visit::forInvalidShortUrl($visitor), Visit::forRegularNotFound($visitor)]; + $orphanVisits = $this->visitsHelper->orphanVisits(Argument::type(VisitsParams::class))->willReturn( + new Paginator(new ArrayAdapter($visits)), + ); + $transform = $this->orphanVisitTransformer->transform(Argument::type(Visit::class))->willReturn([]); + + $response = $this->action->handle(ServerRequestFactory::fromGlobals()); + + self::assertInstanceOf(JsonResponse::class, $response); + self::assertEquals(200, $response->getStatusCode()); + $orphanVisits->shouldHaveBeenCalledOnce(); + $transform->shouldHaveBeenCalledTimes(count($visits)); + } +} From a18486cc2e3e65256c84e69ec96fc0a511282992 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 9 Feb 2021 23:56:46 +0100 Subject: [PATCH 23/28] Created OrphanVisits API test --- .../Rest/test-api/Action/OrphanVisitsTest.php | 59 +++++++++++++++++++ .../Rest/test-api/Fixtures/VisitsFixture.php | 29 +++++++-- .../Action/Visit/OrphanVisitsActionTest.php | 1 - 3 files changed, 82 insertions(+), 7 deletions(-) create mode 100644 module/Rest/test-api/Action/OrphanVisitsTest.php diff --git a/module/Rest/test-api/Action/OrphanVisitsTest.php b/module/Rest/test-api/Action/OrphanVisitsTest.php new file mode 100644 index 00000000..ea890f9f --- /dev/null +++ b/module/Rest/test-api/Action/OrphanVisitsTest.php @@ -0,0 +1,59 @@ + 'https://doma.in/foo', + 'date' => '2020-03-01T00:00:00+00:00', + 'userAgent' => 'shlink-tests-agent', + 'visitLocation' => null, + 'visitedUrl' => 'foo.com', + 'type' => 'invalid_short_url', + + ]; + private const REGULAR_NOT_FOUND = [ + 'referer' => 'https://doma.in/foo/bar', + 'date' => '2020-02-01T00:00:00+00:00', + 'userAgent' => 'shlink-tests-agent', + 'visitLocation' => null, + 'visitedUrl' => '', + 'type' => 'regular_404', + ]; + private const BASE_URL = [ + 'referer' => 'https://doma.in', + 'date' => '2020-01-01T00:00:00+00:00', + 'userAgent' => 'shlink-tests-agent', + 'visitLocation' => null, + 'visitedUrl' => '', + 'type' => 'base_url', + ]; + + /** + * @test + * @dataProvider provideQueries + */ + public function properVisitsAreReturnedBasedInQuery(array $query, int $expectedAmount, array $expectedVisits): void + { + $resp = $this->callApiWithKey(self::METHOD_GET, '/visits/orphan', [RequestOptions::QUERY => $query]); + $payload = $this->getJsonResponsePayload($resp); + $visits = $payload['visits']['data'] ?? []; + + self::assertEquals(3, $payload['visits']['pagination']['totalItems'] ?? -1); + self::assertCount($expectedAmount, $visits); + self::assertEquals($expectedVisits, $visits); + } + + public function provideQueries(): iterable + { + yield 'all data' => [[], 3, [self::INVALID_SHORT_URL, self::REGULAR_NOT_FOUND, self::BASE_URL]]; + yield 'limit items' => [['itemsPerPage' => 2], 2, [self::INVALID_SHORT_URL, self::REGULAR_NOT_FOUND]]; + yield 'limit items and page' => [['itemsPerPage' => 2, 'page' => 2], 1, [self::BASE_URL]]; + } +} diff --git a/module/Rest/test-api/Fixtures/VisitsFixture.php b/module/Rest/test-api/Fixtures/VisitsFixture.php index 9fb53ac1..412c79d5 100644 --- a/module/Rest/test-api/Fixtures/VisitsFixture.php +++ b/module/Rest/test-api/Fixtures/VisitsFixture.php @@ -4,9 +4,11 @@ declare(strict_types=1); namespace ShlinkioApiTest\Shlink\Rest\Fixtures; +use Cake\Chronos\Chronos; use Doctrine\Common\DataFixtures\AbstractFixture; use Doctrine\Common\DataFixtures\DependentFixtureInterface; use Doctrine\Persistence\ObjectManager; +use ReflectionObject; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\Model\Visitor; @@ -47,14 +49,29 @@ class VisitsFixture extends AbstractFixture implements DependentFixtureInterface Visit::forValidShortUrl($ghiShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')), ); - $manager->persist(Visit::forBasePath(new Visitor('shlink-tests-agent', 'https://doma.in', '1.2.3.4', ''))); - $manager->persist( + $manager->persist($this->setVisitDate( + Visit::forBasePath(new Visitor('shlink-tests-agent', 'https://doma.in', '1.2.3.4', '')), + '2020-01-01', + )); + $manager->persist($this->setVisitDate( Visit::forRegularNotFound(new Visitor('shlink-tests-agent', 'https://doma.in/foo/bar', '1.2.3.4', '')), - ); - $manager->persist( - Visit::forInvalidShortUrl(new Visitor('shlink-tests-agent', 'https://doma.in/foo', '1.2.3.4', '')), - ); + '2020-02-01', + )); + $manager->persist($this->setVisitDate( + Visit::forInvalidShortUrl(new Visitor('shlink-tests-agent', 'https://doma.in/foo', '1.2.3.4', 'foo.com')), + '2020-03-01', + )); $manager->flush(); } + + private function setVisitDate(Visit $visit, string $date): Visit + { + $ref = new ReflectionObject($visit); + $dateProp = $ref->getProperty('date'); + $dateProp->setAccessible(true); + $dateProp->setValue($visit, Chronos::parse($date)); + + return $visit; + } } diff --git a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php index 36273d09..9fec7e1f 100644 --- a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php @@ -11,7 +11,6 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; -use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\Entity\Visit; From 2fc6fb0a9a9e022ee85190f4b8fd73406b1452c4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 10 Feb 2021 20:09:25 +0100 Subject: [PATCH 24/28] Added option to disable orphan visitstracking --- config/autoload/url-shortener.global.php | 1 + docker/config/shlink_in_docker.local.php | 1 + module/Core/config/dependencies.config.php | 2 +- .../Core/src/Options/UrlShortenerOptions.php | 25 ++++++++++++++-- module/Core/src/Visit/VisitsTracker.php | 30 ++++++++++++++----- module/Core/test/Visit/VisitsTrackerTest.php | 27 ++++++++++++++++- 6 files changed, 75 insertions(+), 11 deletions(-) diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index 015d459e..5d31ae2d 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -20,6 +20,7 @@ return [ 'redirect_status_code' => DEFAULT_REDIRECT_STATUS_CODE, 'redirect_cache_lifetime' => DEFAULT_REDIRECT_CACHE_LIFETIME, 'auto_resolve_titles' => false, + 'track_orphan_visits' => true, ], ]; diff --git a/docker/config/shlink_in_docker.local.php b/docker/config/shlink_in_docker.local.php index 40173d69..4ddd52e5 100644 --- a/docker/config/shlink_in_docker.local.php +++ b/docker/config/shlink_in_docker.local.php @@ -126,6 +126,7 @@ return [ 'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE), 'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME), 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), + 'track_orphan_visits' => (bool) env('TRACK_ORPHAN_VISITS', true), ], 'not_found_redirects' => $helper->getNotFoundRedirectsConfig(), diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 43586e16..1b83ad7d 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -85,7 +85,7 @@ return [ Visit\VisitsTracker::class => [ 'em', EventDispatcherInterface::class, - 'config.url_shortener.anonymize_remote_addr', + Options\UrlShortenerOptions::class, ], Service\ShortUrlService::class => [ 'em', diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index ebedbf97..e1956203 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -19,6 +19,8 @@ class UrlShortenerOptions extends AbstractOptions private int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE; private int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME; private bool $autoResolveTitles = false; + private bool $anonymizeRemoteAddr = true; + private bool $trackOrphanVisits = true; public function isUrlValidationEnabled(): bool { @@ -62,9 +64,28 @@ class UrlShortenerOptions extends AbstractOptions return $this->autoResolveTitles; } - protected function setAutoResolveTitles(bool $autoResolveTitles): self + protected function setAutoResolveTitles(bool $autoResolveTitles): void { $this->autoResolveTitles = $autoResolveTitles; - return $this; + } + + public function anonymizeRemoteAddr(): bool + { + return $this->anonymizeRemoteAddr; + } + + protected function setAnonymizeRemoteAddr(bool $anonymizeRemoteAddr): void + { + $this->anonymizeRemoteAddr = $anonymizeRemoteAddr; + } + + public function trackOrphanVisits(): bool + { + return $this->trackOrphanVisits; + } + + protected function setTrackOrphanVisits(bool $trackOrphanVisits): void + { + $this->trackOrphanVisits = $trackOrphanVisits; } } diff --git a/module/Core/src/Visit/VisitsTracker.php b/module/Core/src/Visit/VisitsTracker.php index 48157e3b..306da7a9 100644 --- a/module/Core/src/Visit/VisitsTracker.php +++ b/module/Core/src/Visit/VisitsTracker.php @@ -10,41 +10,57 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; class VisitsTracker implements VisitsTrackerInterface { private ORM\EntityManagerInterface $em; private EventDispatcherInterface $eventDispatcher; - private bool $anonymizeRemoteAddr; + private UrlShortenerOptions $options; public function __construct( ORM\EntityManagerInterface $em, EventDispatcherInterface $eventDispatcher, - bool $anonymizeRemoteAddr + UrlShortenerOptions $options ) { $this->em = $em; $this->eventDispatcher = $eventDispatcher; - $this->anonymizeRemoteAddr = $anonymizeRemoteAddr; + $this->options = $options; } public function track(ShortUrl $shortUrl, Visitor $visitor): void { - $this->trackVisit(Visit::forValidShortUrl($shortUrl, $visitor, $this->anonymizeRemoteAddr), $visitor); + $this->trackVisit( + Visit::forValidShortUrl($shortUrl, $visitor, $this->options->anonymizeRemoteAddr()), + $visitor, + ); } public function trackInvalidShortUrlVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forInvalidShortUrl($visitor, $this->anonymizeRemoteAddr), $visitor); + if (! $this->options->trackOrphanVisits()) { + return; + } + + $this->trackVisit(Visit::forInvalidShortUrl($visitor, $this->options->anonymizeRemoteAddr()), $visitor); } public function trackBaseUrlVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forBasePath($visitor, $this->anonymizeRemoteAddr), $visitor); + if (! $this->options->trackOrphanVisits()) { + return; + } + + $this->trackVisit(Visit::forBasePath($visitor, $this->options->anonymizeRemoteAddr()), $visitor); } public function trackRegularNotFoundVisit(Visitor $visitor): void { - $this->trackVisit(Visit::forRegularNotFound($visitor, $this->anonymizeRemoteAddr), $visitor); + if (! $this->options->trackOrphanVisits()) { + return; + } + + $this->trackVisit(Visit::forRegularNotFound($visitor, $this->options->anonymizeRemoteAddr()), $visitor); } private function trackVisit(Visit $visit, Visitor $visitor): void diff --git a/module/Core/test/Visit/VisitsTrackerTest.php b/module/Core/test/Visit/VisitsTrackerTest.php index fd6e341f..118ebc06 100644 --- a/module/Core/test/Visit/VisitsTrackerTest.php +++ b/module/Core/test/Visit/VisitsTrackerTest.php @@ -14,6 +14,7 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Model\Visitor; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Visit\VisitsTracker; class VisitsTrackerTest extends TestCase @@ -23,13 +24,15 @@ class VisitsTrackerTest extends TestCase private VisitsTracker $visitsTracker; private ObjectProphecy $em; private ObjectProphecy $eventDispatcher; + private UrlShortenerOptions $options; public function setUp(): void { $this->em = $this->prophesize(EntityManager::class); $this->eventDispatcher = $this->prophesize(EventDispatcherInterface::class); + $this->options = new UrlShortenerOptions(); - $this->visitsTracker = new VisitsTracker($this->em->reveal(), $this->eventDispatcher->reveal(), true); + $this->visitsTracker = new VisitsTracker($this->em->reveal(), $this->eventDispatcher->reveal(), $this->options); } /** @@ -53,4 +56,26 @@ class VisitsTrackerTest extends TestCase yield 'trackBaseUrlVisit' => ['trackBaseUrlVisit', [Visitor::emptyInstance()]]; yield 'trackRegularNotFoundVisit' => ['trackRegularNotFoundVisit', [Visitor::emptyInstance()]]; } + + /** + * @test + * @dataProvider provideOrphanTrackingMethodNames + */ + public function orphanVisitsAreNotTrackedWhenDisabled(string $method): void + { + $this->options->trackOrphanVisits = false; + + $this->visitsTracker->{$method}(Visitor::emptyInstance()); + + $this->eventDispatcher->dispatch(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->em->flush()->shouldNotHaveBeenCalled(); + } + + public function provideOrphanTrackingMethodNames(): iterable + { + yield 'trackInvalidShortUrlVisit' => ['trackInvalidShortUrlVisit']; + yield 'trackBaseUrlVisit' => ['trackBaseUrlVisit']; + yield 'trackRegularNotFoundVisit' => ['trackRegularNotFoundVisit']; + } } From bd09b1571a34fbd45ffb603f1fcea89631e009bc Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 10 Feb 2021 20:39:37 +0100 Subject: [PATCH 25/28] Updated shlink-installer with support for orphan visits tracking option --- composer.json | 2 +- config/autoload/installer.global.php | 1 + config/autoload/url-shortener.global.php | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index ec9a028d..f4eedb4a 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,7 @@ "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^2.0", "shlinkio/shlink-importer": "^2.2", - "shlinkio/shlink-installer": "dev-develop#1ed5ac8 as 5.4", + "shlinkio/shlink-installer": "dev-develop#c489d3f as 5.4", "shlinkio/shlink-ip-geolocation": "^1.5", "symfony/console": "^5.1", "symfony/filesystem": "^5.1", diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index 7a355dbe..d18f31f4 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -41,6 +41,7 @@ return [ Option\UrlShortener\RedirectStatusCodeConfigOption::class, Option\UrlShortener\RedirectCacheLifeTimeConfigOption::class, Option\UrlShortener\AutoResolveTitlesConfigOption::class, + Option\UrlShortener\OrphanVisitsTrackingConfigOption::class, ], 'installation_commands' => [ diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index 5d31ae2d..3751b1e9 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -13,7 +13,7 @@ return [ 'schema' => 'https', 'hostname' => '', ], - 'validate_url' => false, + 'validate_url' => false, // Deprecated 'anonymize_remote_addr' => true, 'visits_webhooks' => [], 'default_short_codes_length' => DEFAULT_SHORT_CODES_LENGTH, From 7d6d8e3a68f85920937ab1208a19f9d89b1a907c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 11 Feb 2021 22:12:38 +0100 Subject: [PATCH 26/28] Added support to publish orphan visits in mercure --- docs/async-api/async-api.json | 57 +++++++++++++++++++ module/Core/config/dependencies.config.php | 5 +- .../Core/src/EventDispatcher/LocateVisit.php | 4 +- .../EventDispatcher/NotifyVisitToMercure.php | 21 ++++++- .../src/Mercure/MercureUpdatesGenerator.php | 24 ++++++-- .../MercureUpdatesGeneratorInterface.php | 2 + .../test/EventDispatcher/LocateVisitTest.php | 17 ++---- .../NotifyVisitToMercureTest.php | 49 ++++++++++++++-- .../Mercure/MercureUpdatesGeneratorTest.php | 36 +++++++++++- 9 files changed, 187 insertions(+), 28 deletions(-) diff --git a/docs/async-api/async-api.json b/docs/async-api/async-api.json index 5279ce91..df9bc6d6 100644 --- a/docs/async-api/async-api.json +++ b/docs/async-api/async-api.json @@ -58,6 +58,23 @@ } } } + }, + "http://shlink.io/new-orphan-visit": { + "subscribe": { + "summary": "Receive information about any new orphan visit.", + "operationId": "newOrphanVisit", + "message": { + "payload": { + "type": "object", + "additionalProperties": false, + "properties": { + "visit": { + "$ref": "#/components/schemas/OrphanVisit" + } + } + } + } + } } }, "components": { @@ -179,6 +196,46 @@ } } }, + "OrphanVisit": { + "allOf": [ + {"$ref": "#/components/schemas/Visit"}, + { + "type": "object", + "properties": { + "visitedUrl": { + "type": "string", + "nullable": true, + "description": "The originally visited URL that triggered the tracking of this visit" + }, + "type": { + "type": "string", + "enum": [ + "invalid_short_url", + "base_url", + "regular_404" + ], + "description": "Tells the type of orphan visit" + } + } + } + ], + "example": { + "referer": "https://t.co", + "date": "2015-08-20T05:05:03+04:00", + "userAgent": "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36", + "visitLocation": { + "cityName": "Cupertino", + "countryCode": "US", + "countryName": "United States", + "latitude": 37.3042, + "longitude": -122.0946, + "regionName": "California", + "timezone": "America/Los_Angeles" + }, + "visitedUrl": "https://doma.in", + "type": "base_url" + } + }, "VisitLocation": { "type": "object", "properties": { diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 1b83ad7d..479b497a 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -133,7 +133,10 @@ return [ ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], - Mercure\MercureUpdatesGenerator::class => [ShortUrl\Transformer\ShortUrlDataTransformer::class], + Mercure\MercureUpdatesGenerator::class => [ + ShortUrl\Transformer\ShortUrlDataTransformer::class, + Visit\Transformer\OrphanVisitDataTransformer::class, + ], Importer\ImportedLinksProcessor::class => [ 'em', diff --git a/module/Core/src/EventDispatcher/LocateVisit.php b/module/Core/src/EventDispatcher/LocateVisit.php index 5e3baf74..32da6060 100644 --- a/module/Core/src/EventDispatcher/LocateVisit.php +++ b/module/Core/src/EventDispatcher/LocateVisit.php @@ -58,9 +58,7 @@ class LocateVisit $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress(), $visit); } - if (! $visit->isOrphan()) { - $this->eventDispatcher->dispatch(new VisitLocated($visitId)); - } + $this->eventDispatcher->dispatch(new VisitLocated($visitId)); } private function downloadOrUpdateGeoLiteDb(string $visitId): bool diff --git a/module/Core/src/EventDispatcher/NotifyVisitToMercure.php b/module/Core/src/EventDispatcher/NotifyVisitToMercure.php index 33aab7af..0cf438ed 100644 --- a/module/Core/src/EventDispatcher/NotifyVisitToMercure.php +++ b/module/Core/src/EventDispatcher/NotifyVisitToMercure.php @@ -10,8 +10,11 @@ use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\Core\Mercure\MercureUpdatesGeneratorInterface; use Symfony\Component\Mercure\PublisherInterface; +use Symfony\Component\Mercure\Update; use Throwable; +use function Functional\each; + class NotifyVisitToMercure { private PublisherInterface $publisher; @@ -45,12 +48,26 @@ class NotifyVisitToMercure } try { - ($this->publisher)($this->updatesGenerator->newShortUrlVisitUpdate($visit)); - ($this->publisher)($this->updatesGenerator->newVisitUpdate($visit)); + each($this->determineUpdatesForVisit($visit), fn (Update $update) => ($this->publisher)($update)); } catch (Throwable $e) { $this->logger->debug('Error while trying to notify mercure hub with new visit. {e}', [ 'e' => $e, ]); } } + + /** + * @return Update[] + */ + private function determineUpdatesForVisit(Visit $visit): array + { + if ($visit->isOrphan()) { + return [$this->updatesGenerator->newOrphanVisitUpdate($visit)]; + } + + return [ + $this->updatesGenerator->newShortUrlVisitUpdate($visit), + $this->updatesGenerator->newVisitUpdate($visit), + ]; + } } diff --git a/module/Core/src/Mercure/MercureUpdatesGenerator.php b/module/Core/src/Mercure/MercureUpdatesGenerator.php index 9a0a28f3..23b3796c 100644 --- a/module/Core/src/Mercure/MercureUpdatesGenerator.php +++ b/module/Core/src/Mercure/MercureUpdatesGenerator.php @@ -16,29 +16,41 @@ use const JSON_THROW_ON_ERROR; final class MercureUpdatesGenerator implements MercureUpdatesGeneratorInterface { private const NEW_VISIT_TOPIC = 'https://shlink.io/new-visit'; + private const NEW_ORPHAN_VISIT_TOPIC = 'https://shlink.io/new-orphan-visit'; - private DataTransformerInterface $transformer; + private DataTransformerInterface $shortUrlTransformer; + private DataTransformerInterface $orphanVisitTransformer; - public function __construct(DataTransformerInterface $transformer) - { - $this->transformer = $transformer; + public function __construct( + DataTransformerInterface $shortUrlTransformer, + DataTransformerInterface $orphanVisitTransformer + ) { + $this->shortUrlTransformer = $shortUrlTransformer; + $this->orphanVisitTransformer = $orphanVisitTransformer; } public function newVisitUpdate(Visit $visit): Update { return new Update(self::NEW_VISIT_TOPIC, $this->serialize([ - 'shortUrl' => $this->transformer->transform($visit->getShortUrl()), + 'shortUrl' => $this->shortUrlTransformer->transform($visit->getShortUrl()), 'visit' => $visit, ])); } + public function newOrphanVisitUpdate(Visit $visit): Update + { + return new Update(self::NEW_ORPHAN_VISIT_TOPIC, $this->serialize([ + 'visit' => $this->orphanVisitTransformer->transform($visit), + ])); + } + public function newShortUrlVisitUpdate(Visit $visit): Update { $shortUrl = $visit->getShortUrl(); $topic = sprintf('%s/%s', self::NEW_VISIT_TOPIC, $shortUrl->getShortCode()); return new Update($topic, $this->serialize([ - 'shortUrl' => $this->transformer->transform($shortUrl), + 'shortUrl' => $this->shortUrlTransformer->transform($shortUrl), 'visit' => $visit, ])); } diff --git a/module/Core/src/Mercure/MercureUpdatesGeneratorInterface.php b/module/Core/src/Mercure/MercureUpdatesGeneratorInterface.php index d433d9ad..951e805c 100644 --- a/module/Core/src/Mercure/MercureUpdatesGeneratorInterface.php +++ b/module/Core/src/Mercure/MercureUpdatesGeneratorInterface.php @@ -11,5 +11,7 @@ interface MercureUpdatesGeneratorInterface { public function newVisitUpdate(Visit $visit): Update; + public function newOrphanVisitUpdate(Visit $visit): Update; + public function newShortUrlVisitUpdate(Visit $visit): Update; } diff --git a/module/Core/test/EventDispatcher/LocateVisitTest.php b/module/Core/test/EventDispatcher/LocateVisitTest.php index af21e3dc..081f0f86 100644 --- a/module/Core/test/EventDispatcher/LocateVisitTest.php +++ b/module/Core/test/EventDispatcher/LocateVisitTest.php @@ -136,11 +136,8 @@ class LocateVisitTest extends TestCase * @test * @dataProvider provideIpAddresses */ - public function locatableVisitsResolveToLocation( - Visit $visit, - ?string $originalIpAddress, - int $expectedDispatchCalls - ): void { + public function locatableVisitsResolveToLocation(Visit $visit, ?string $originalIpAddress): void + { $ipAddr = $originalIpAddress ?? $visit->getRemoteAddr(); $location = new Location('', '', '', '', 0.0, 0.0, ''); $event = new UrlVisited('123', $originalIpAddress); @@ -159,7 +156,7 @@ class LocateVisitTest extends TestCase $flush->shouldHaveBeenCalledOnce(); $resolveIp->shouldHaveBeenCalledOnce(); $this->logger->warning(Argument::cetera())->shouldNotHaveBeenCalled(); - $dispatch->shouldHaveBeenCalledTimes($expectedDispatchCalls); + $dispatch->shouldHaveBeenCalledOnce(); } public function provideIpAddresses(): iterable @@ -167,16 +164,14 @@ class LocateVisitTest extends TestCase yield 'no original IP address' => [ Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), null, - 1, ]; yield 'original IP address' => [ Visit::forValidShortUrl(ShortUrl::createEmpty(), new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', - 1, ]; - yield 'base url' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; - yield 'invalid short url' => [Visit::forInvalidShortUrl(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; - yield 'regular not found' => [Visit::forRegularNotFound(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4', 0]; + 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']; } /** @test */ diff --git a/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php b/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php index 1180d05c..f323a155 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToMercureTest.php @@ -57,10 +57,9 @@ class NotifyVisitToMercureTest extends TestCase $logDebug = $this->logger->debug(Argument::cetera()); $buildNewShortUrlVisitUpdate = $this->updatesGenerator->newShortUrlVisitUpdate( Argument::type(Visit::class), - )->willReturn(new Update('', '')); - $buildNewVisitUpdate = $this->updatesGenerator->newVisitUpdate(Argument::type(Visit::class))->willReturn( - new Update('', ''), ); + $buildNewOrphanVisitUpdate = $this->updatesGenerator->newOrphanVisitUpdate(Argument::type(Visit::class)); + $buildNewVisitUpdate = $this->updatesGenerator->newVisitUpdate(Argument::type(Visit::class)); $publish = $this->publisher->__invoke(Argument::type(Update::class)); ($this->listener)(new VisitLocated($visitId)); @@ -70,6 +69,7 @@ class NotifyVisitToMercureTest extends TestCase $logDebug->shouldNotHaveBeenCalled(); $buildNewShortUrlVisitUpdate->shouldNotHaveBeenCalled(); $buildNewVisitUpdate->shouldNotHaveBeenCalled(); + $buildNewOrphanVisitUpdate->shouldNotHaveBeenCalled(); $publish->shouldNotHaveBeenCalled(); } @@ -84,6 +84,7 @@ class NotifyVisitToMercureTest extends TestCase $logWarning = $this->logger->warning(Argument::cetera()); $logDebug = $this->logger->debug(Argument::cetera()); $buildNewShortUrlVisitUpdate = $this->updatesGenerator->newShortUrlVisitUpdate($visit)->willReturn($update); + $buildNewOrphanVisitUpdate = $this->updatesGenerator->newOrphanVisitUpdate($visit)->willReturn($update); $buildNewVisitUpdate = $this->updatesGenerator->newVisitUpdate($visit)->willReturn($update); $publish = $this->publisher->__invoke($update); @@ -94,6 +95,7 @@ class NotifyVisitToMercureTest extends TestCase $logDebug->shouldNotHaveBeenCalled(); $buildNewShortUrlVisitUpdate->shouldHaveBeenCalledOnce(); $buildNewVisitUpdate->shouldHaveBeenCalledOnce(); + $buildNewOrphanVisitUpdate->shouldNotHaveBeenCalled(); $publish->shouldHaveBeenCalledTimes(2); } @@ -111,6 +113,7 @@ class NotifyVisitToMercureTest extends TestCase 'e' => $e, ]); $buildNewShortUrlVisitUpdate = $this->updatesGenerator->newShortUrlVisitUpdate($visit)->willReturn($update); + $buildNewOrphanVisitUpdate = $this->updatesGenerator->newOrphanVisitUpdate($visit)->willReturn($update); $buildNewVisitUpdate = $this->updatesGenerator->newVisitUpdate($visit)->willReturn($update); $publish = $this->publisher->__invoke($update)->willThrow($e); @@ -120,7 +123,45 @@ class NotifyVisitToMercureTest extends TestCase $logWarning->shouldNotHaveBeenCalled(); $logDebug->shouldHaveBeenCalledOnce(); $buildNewShortUrlVisitUpdate->shouldHaveBeenCalledOnce(); - $buildNewVisitUpdate->shouldNotHaveBeenCalled(); + $buildNewVisitUpdate->shouldHaveBeenCalledOnce(); + $buildNewOrphanVisitUpdate->shouldNotHaveBeenCalled(); $publish->shouldHaveBeenCalledOnce(); } + + /** + * @test + * @dataProvider provideOrphanVisits + */ + public function notificationsAreSentForOrphanVisits(Visit $visit): void + { + $visitId = '123'; + $update = new Update('', ''); + + $findVisit = $this->em->find(Visit::class, $visitId)->willReturn($visit); + $logWarning = $this->logger->warning(Argument::cetera()); + $logDebug = $this->logger->debug(Argument::cetera()); + $buildNewShortUrlVisitUpdate = $this->updatesGenerator->newShortUrlVisitUpdate($visit)->willReturn($update); + $buildNewOrphanVisitUpdate = $this->updatesGenerator->newOrphanVisitUpdate($visit)->willReturn($update); + $buildNewVisitUpdate = $this->updatesGenerator->newVisitUpdate($visit)->willReturn($update); + $publish = $this->publisher->__invoke($update); + + ($this->listener)(new VisitLocated($visitId)); + + $findVisit->shouldHaveBeenCalledOnce(); + $logWarning->shouldNotHaveBeenCalled(); + $logDebug->shouldNotHaveBeenCalled(); + $buildNewShortUrlVisitUpdate->shouldNotHaveBeenCalled(); + $buildNewVisitUpdate->shouldNotHaveBeenCalled(); + $buildNewOrphanVisitUpdate->shouldHaveBeenCalledOnce(); + $publish->shouldHaveBeenCalledOnce(); + } + + public function provideOrphanVisits(): iterable + { + $visitor = Visitor::emptyInstance(); + + yield Visit::TYPE_REGULAR_404 => [Visit::forRegularNotFound($visitor)]; + yield Visit::TYPE_INVALID_SHORT_URL => [Visit::forInvalidShortUrl($visitor)]; + yield Visit::TYPE_BASE_URL => [Visit::forBasePath($visitor)]; + } } diff --git a/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php b/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php index 9e4b418e..b4361ca5 100644 --- a/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php +++ b/module/Core/test/Mercure/MercureUpdatesGeneratorTest.php @@ -12,6 +12,7 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; +use Shlinkio\Shlink\Core\Visit\Transformer\OrphanVisitDataTransformer; use function Shlinkio\Shlink\Common\json_decode; @@ -21,7 +22,10 @@ class MercureUpdatesGeneratorTest extends TestCase public function setUp(): void { - $this->generator = new MercureUpdatesGenerator(new ShortUrlDataTransformer(new ShortUrlStringifier([]))); + $this->generator = new MercureUpdatesGenerator( + new ShortUrlDataTransformer(new ShortUrlStringifier([])), + new OrphanVisitDataTransformer(), + ); } /** @@ -70,4 +74,34 @@ class MercureUpdatesGeneratorTest extends TestCase yield 'newVisitUpdate' => ['newVisitUpdate', 'https://shlink.io/new-visit', 'the cool title']; yield 'newShortUrlVisitUpdate' => ['newShortUrlVisitUpdate', 'https://shlink.io/new-visit/foo', null]; } + + /** + * @test + * @dataProvider provideOrphanVisits + */ + public function orphanVisitIsProperlySerializedIntoUpdate(Visit $orphanVisit): void + { + $update = $this->generator->newOrphanVisitUpdate($orphanVisit); + + self::assertEquals(['https://shlink.io/new-orphan-visit'], $update->getTopics()); + self::assertEquals([ + 'visit' => [ + 'referer' => '', + 'userAgent' => '', + 'visitLocation' => null, + 'date' => $orphanVisit->getDate()->toAtomString(), + 'visitedUrl' => $orphanVisit->visitedUrl(), + 'type' => $orphanVisit->type(), + ], + ], json_decode($update->getData())); + } + + public function provideOrphanVisits(): iterable + { + $visitor = Visitor::emptyInstance(); + + yield Visit::TYPE_REGULAR_404 => [Visit::forRegularNotFound($visitor)]; + yield Visit::TYPE_INVALID_SHORT_URL => [Visit::forInvalidShortUrl($visitor)]; + yield Visit::TYPE_BASE_URL => [Visit::forBasePath($visitor)]; + } } From a0d8d237d7c03ba9a42829f169d63905f0b927c6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 11 Feb 2021 22:23:30 +0100 Subject: [PATCH 27/28] Gitignored helper file --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 8cfea409..03b2790e 100644 --- a/.gitignore +++ b/.gitignore @@ -9,5 +9,6 @@ data/shlink-tests.db data/GeoLite2-City.mmdb data/GeoLite2-City.mmdb.* docs/swagger-ui* +docs/mercure.html docker-compose.override.yml .phpunit.result.cache From cc68cb944f7cbfde54383e08156e5ba7866578e3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 11 Feb 2021 22:43:23 +0100 Subject: [PATCH 28/28] Updated changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e11c74cc..9f00b680 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this The file requires the `Long URL` and `Short code` columns, and it also accepts the optional `title`, `domain` and `tags` columns. * [#1000](https://github.com/shlinkio/shlink/issues/1000) Added support to provide a `margin` query param when generating some URL's QR code. +* [#675](https://github.com/shlinkio/shlink/issues/1000) Added ability to track visits to the base URL, invalid short URLs or any other "not found" URL, as known as orphan visits. + + This behavior is enabled by default, but you can opt out via env vars or config options. + + This new orphan visits can be consumed in these ways: + + * The `https://shlink.io/new-orphan-visit` mercure topic, which gets notified when an orphan visit occurs. + * The `GET /visits/orphan` REST endpoint, which behaves like the short URL visits and tags visits endpoints, but returns only orphan visits. ### Changed * [#977](https://github.com/shlinkio/shlink/issues/977) Migrated from `laminas/laminas-paginator` to `pagerfanta/core` to handle pagination.