From 8274525f754bac418db9419e1e504c79da42ee14 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2024 12:53:49 +0100 Subject: [PATCH 1/6] Add redirect_url field to track where a visitor is redirected for a visit --- docs/async-api/async-api.json | 5 +++ docs/swagger/definitions/Visit.json | 4 ++ ...hlinkio.Shlink.Core.Visit.Entity.Visit.php | 6 +++ .../Core/migrations/Version20241124112257.php | 39 +++++++++++++++++++ module/Core/src/Visit/Entity/Visit.php | 3 ++ module/Core/src/Visit/Model/Visitor.php | 7 ++++ .../PublishingUpdatesGeneratorTest.php | 2 + module/Core/test/Visit/Entity/VisitTest.php | 4 ++ .../Rest/test-api/Action/OrphanVisitsTest.php | 3 ++ 9 files changed, 73 insertions(+) create mode 100644 module/Core/migrations/Version20241124112257.php diff --git a/docs/async-api/async-api.json b/docs/async-api/async-api.json index b2da154b..09817a99 100644 --- a/docs/async-api/async-api.json +++ b/docs/async-api/async-api.json @@ -247,6 +247,11 @@ "type": "string", "nullable": true, "description": "The originally visited URL that triggered the tracking of this visit" + }, + "redirectUrl": { + "type": "string", + "nullable": true, + "description": "The URL to which the visitor was redirected" } }, "example": { diff --git a/docs/swagger/definitions/Visit.json b/docs/swagger/definitions/Visit.json index c4589bb1..826ad1ac 100644 --- a/docs/swagger/definitions/Visit.json +++ b/docs/swagger/definitions/Visit.json @@ -25,6 +25,10 @@ "visitedUrl": { "type": ["string", "null"], "description": "The originally visited URL that triggered the tracking of this visit" + }, + "redirectUrl": { + "type": ["string", "null"], + "description": "The URL to which the visitor was redirected" } } } diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.Visit.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.Visit.php index 7d402384..34d98572 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.Visit.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.Visit.Entity.Visit.php @@ -75,4 +75,10 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->columnName('potential_bot') ->option('default', false) ->build(); + + fieldWithUtf8Charset($builder->createField('redirectUrl', Types::STRING), $emConfig) + ->columnName('redirect_url') + ->length(Visitor::REDIRECT_URL_MAX_LENGTH) + ->nullable() + ->build(); }; diff --git a/module/Core/migrations/Version20241124112257.php b/module/Core/migrations/Version20241124112257.php new file mode 100644 index 00000000..49c5eb05 --- /dev/null +++ b/module/Core/migrations/Version20241124112257.php @@ -0,0 +1,39 @@ +getTable('visits'); + $this->skipIf($visits->hasColumn(self::COLUMN_NAME)); + + $visits->addColumn('redirected_url', Types::STRING, [ + 'length' => 2048, + 'notnull' => false, + 'default' => null, + ]); + } + + public function down(Schema $schema): void + { + $visits = $schema->getTable('visits'); + $this->skipIf(! $visits->hasColumn(self::COLUMN_NAME)); + $visits->dropColumn(self::COLUMN_NAME); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/src/Visit/Entity/Visit.php b/module/Core/src/Visit/Entity/Visit.php index 9e8540bc..033d451b 100644 --- a/module/Core/src/Visit/Entity/Visit.php +++ b/module/Core/src/Visit/Entity/Visit.php @@ -28,6 +28,7 @@ class Visit extends AbstractEntity implements JsonSerializable public readonly bool $potentialBot, public readonly string|null $remoteAddr = null, public readonly string|null $visitedUrl = null, + public readonly string|null $redirectUrl = null, private VisitLocation|null $visitLocation = null, public readonly Chronos $date = new Chronos(), ) { @@ -68,6 +69,7 @@ class Visit extends AbstractEntity implements JsonSerializable potentialBot: $visitor->potentialBot, remoteAddr: self::processAddress($visitor->remoteAddress, $anonymize), visitedUrl: $visitor->visitedUrl, + redirectUrl: null, // TODO visitLocation: $geolocation !== null ? VisitLocation::fromGeolocation($geolocation) : null, ); } @@ -156,6 +158,7 @@ class Visit extends AbstractEntity implements JsonSerializable 'visitLocation' => $this->visitLocation, 'potentialBot' => $this->potentialBot, 'visitedUrl' => $this->visitedUrl, + 'redirectUrl' => $this->redirectUrl, ]; if (! $this->isOrphan()) { return $base; diff --git a/module/Core/src/Visit/Model/Visitor.php b/module/Core/src/Visit/Model/Visitor.php index e13712e1..b33d10a1 100644 --- a/module/Core/src/Visit/Model/Visitor.php +++ b/module/Core/src/Visit/Model/Visitor.php @@ -19,6 +19,7 @@ final readonly class Visitor public const REFERER_MAX_LENGTH = 1024; public const REMOTE_ADDRESS_MAX_LENGTH = 256; public const VISITED_URL_MAX_LENGTH = 2048; + public const REDIRECT_URL_MAX_LENGTH = 2048; private function __construct( public string $userAgent, @@ -27,6 +28,7 @@ final readonly class Visitor public string $visitedUrl, public bool $potentialBot, public Location|null $geolocation, + public string $redirectUrl, ) { } @@ -36,6 +38,7 @@ final readonly class Visitor string|null $remoteAddress = null, string $visitedUrl = '', Location|null $geolocation = null, + string $redirectUrl = '', ): self { return new self( userAgent: self::cropToLength($userAgent, self::USER_AGENT_MAX_LENGTH), @@ -46,6 +49,7 @@ final readonly class Visitor visitedUrl: self::cropToLength($visitedUrl, self::VISITED_URL_MAX_LENGTH), potentialBot: isCrawler($userAgent), geolocation: $geolocation, + redirectUrl: self::cropToLength($redirectUrl, self::REDIRECT_URL_MAX_LENGTH), ); } @@ -62,6 +66,8 @@ final readonly class Visitor remoteAddress: ipAddressFromRequest($request), visitedUrl: $request->getUri()->__toString(), geolocation: geolocationFromRequest($request), + // TODO + redirectUrl: '', ); } @@ -85,6 +91,7 @@ final readonly class Visitor // Keep the fact that the visit was a potential bot, even if we no longer save the user agent potentialBot: $this->potentialBot, geolocation: $this->geolocation, + redirectUrl: $this->redirectUrl, ); } } diff --git a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php index 2e232038..310c8b3f 100644 --- a/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php +++ b/module/Core/test/EventDispatcher/PublishingUpdatesGeneratorTest.php @@ -80,6 +80,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'date' => $visit->date->toAtomString(), 'potentialBot' => false, 'visitedUrl' => '', + 'redirectUrl' => null, ], ], $update->payload); } @@ -105,6 +106,7 @@ class PublishingUpdatesGeneratorTest extends TestCase 'potentialBot' => false, 'visitedUrl' => $orphanVisit->visitedUrl, 'type' => $orphanVisit->type->value, + 'redirectUrl' => null, ], ], $update->payload); } diff --git a/module/Core/test/Visit/Entity/VisitTest.php b/module/Core/test/Visit/Entity/VisitTest.php index db23af97..438ca55f 100644 --- a/module/Core/test/Visit/Entity/VisitTest.php +++ b/module/Core/test/Visit/Entity/VisitTest.php @@ -34,6 +34,7 @@ class VisitTest extends TestCase 'visitLocation' => null, 'potentialBot' => $expectedToBePotentialBot, 'visitedUrl' => $visit->visitedUrl, + 'redirectUrl' => $visit->redirectUrl, ], $visit->jsonSerialize()); } @@ -67,6 +68,7 @@ class VisitTest extends TestCase 'potentialBot' => false, 'visitedUrl' => '', 'type' => VisitType::BASE_URL->value, + 'redirectUrl' => null, ], ]; yield 'invalid short url visit' => [ @@ -83,6 +85,7 @@ class VisitTest extends TestCase 'potentialBot' => false, 'visitedUrl' => 'https://example.com/foo', 'type' => VisitType::INVALID_SHORT_URL->value, + 'redirectUrl' => null, ], ]; yield 'regular 404 visit' => [ @@ -101,6 +104,7 @@ class VisitTest extends TestCase 'potentialBot' => false, 'visitedUrl' => 'https://s.test/foo/bar', 'type' => VisitType::REGULAR_404->value, + 'redirectUrl' => null, ], ]; } diff --git a/module/Rest/test-api/Action/OrphanVisitsTest.php b/module/Rest/test-api/Action/OrphanVisitsTest.php index cf7cee0f..3761e113 100644 --- a/module/Rest/test-api/Action/OrphanVisitsTest.php +++ b/module/Rest/test-api/Action/OrphanVisitsTest.php @@ -21,6 +21,7 @@ class OrphanVisitsTest extends ApiTestCase 'potentialBot' => true, 'visitedUrl' => 'foo.com', 'type' => 'invalid_short_url', + 'redirectUrl' => null, ]; private const REGULAR_NOT_FOUND = [ 'referer' => 'https://s.test/foo/bar', @@ -30,6 +31,7 @@ class OrphanVisitsTest extends ApiTestCase 'potentialBot' => false, 'visitedUrl' => '', 'type' => 'regular_404', + 'redirectUrl' => null, ]; private const BASE_URL = [ 'referer' => 'https://s.test', @@ -39,6 +41,7 @@ class OrphanVisitsTest extends ApiTestCase 'potentialBot' => false, 'visitedUrl' => '', 'type' => 'base_url', + 'redirectUrl' => null, ]; #[Test, DataProvider('provideQueries')] From 89f70114e4304021597a7fa32da9b7a93e7e09c4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2024 13:18:32 +0100 Subject: [PATCH 2/6] Fix typo in migration --- module/Core/migrations/Version20241124112257.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Core/migrations/Version20241124112257.php b/module/Core/migrations/Version20241124112257.php index 49c5eb05..c11cbe2b 100644 --- a/module/Core/migrations/Version20241124112257.php +++ b/module/Core/migrations/Version20241124112257.php @@ -18,7 +18,7 @@ final class Version20241124112257 extends AbstractMigration $visits = $schema->getTable('visits'); $this->skipIf($visits->hasColumn(self::COLUMN_NAME)); - $visits->addColumn('redirected_url', Types::STRING, [ + $visits->addColumn(self::COLUMN_NAME, Types::STRING, [ 'length' => 2048, 'notnull' => false, 'default' => null, From 86cc2b717c32ab0b8355030e9aa3cea96038b496 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2024 13:21:48 +0100 Subject: [PATCH 3/6] Save where a visitor is redirected for any kind of tracked visit --- config/constants.php | 1 + module/Core/src/Action/AbstractTrackingAction.php | 10 ++++++++-- .../src/ErrorHandler/NotFoundTrackerMiddleware.php | 13 ++++++++++--- .../Middleware/ExtraPathRedirectMiddleware.php | 9 +++++++-- module/Core/src/Visit/Entity/Visit.php | 2 +- module/Core/src/Visit/Model/Visitor.php | 10 +++++----- 6 files changed, 32 insertions(+), 13 deletions(-) diff --git a/config/constants.php b/config/constants.php index d6bb9621..09df0e60 100644 --- a/config/constants.php +++ b/config/constants.php @@ -22,3 +22,4 @@ const DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS = true; const DEFAULT_QR_CODE_COLOR = '#000000'; // Black const DEFAULT_QR_CODE_BG_COLOR = '#ffffff'; // White const IP_ADDRESS_REQUEST_ATTRIBUTE = 'remote_address'; +const REDIRECT_URL_REQUEST_ATTRIBUTE = 'redirect_url'; diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 78eebc05..ff35828f 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -16,6 +16,8 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface { public function __construct( @@ -30,9 +32,13 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet try { $shortUrl = $this->urlResolver->resolveEnabledShortUrl($identifier); - $this->requestTracker->trackIfApplicable($shortUrl, $request); + $response = $this->createSuccessResp($shortUrl, $request); + $this->requestTracker->trackIfApplicable($shortUrl, $request->withAttribute( + REDIRECT_URL_REQUEST_ATTRIBUTE, + $response->hasHeader('Location') ? $response->getHeaderLine('Location') : null, + )); - return $this->createSuccessResp($shortUrl, $request); + return $response; } catch (ShortUrlNotFoundException) { return $this->createErrorResp($request, $handler); } diff --git a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php index f3342c5a..633d83db 100644 --- a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php +++ b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php @@ -10,7 +10,9 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; -class NotFoundTrackerMiddleware implements MiddlewareInterface +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + +readonly class NotFoundTrackerMiddleware implements MiddlewareInterface { public function __construct(private RequestTrackerInterface $requestTracker) { @@ -18,7 +20,12 @@ class NotFoundTrackerMiddleware implements MiddlewareInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - $this->requestTracker->trackNotFoundIfApplicable($request); - return $handler->handle($request); + $response = $handler->handle($request); + $this->requestTracker->trackNotFoundIfApplicable($request->withAttribute( + REDIRECT_URL_REQUEST_ATTRIBUTE, + $response->hasHeader('Location') ? $response->getHeaderLine('Location') : null, + )); + + return $response; } } diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php index 4a02f6e9..4b013b33 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -25,6 +25,8 @@ use function implode; use function sprintf; use function trim; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + readonly class ExtraPathRedirectMiddleware implements MiddlewareInterface { public function __construct( @@ -73,9 +75,12 @@ readonly class ExtraPathRedirectMiddleware implements MiddlewareInterface try { $shortUrl = $this->resolver->resolveEnabledShortUrl($identifier); - $this->requestTracker->trackIfApplicable($shortUrl, $request); - $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath); + $this->requestTracker->trackIfApplicable( + $shortUrl, + $request->withAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE, $longUrl), + ); + return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } catch (ShortUrlNotFoundException) { if ($extraPath === null || ! $this->urlShortenerOptions->multiSegmentSlugsEnabled) { diff --git a/module/Core/src/Visit/Entity/Visit.php b/module/Core/src/Visit/Entity/Visit.php index 033d451b..70733593 100644 --- a/module/Core/src/Visit/Entity/Visit.php +++ b/module/Core/src/Visit/Entity/Visit.php @@ -69,7 +69,7 @@ class Visit extends AbstractEntity implements JsonSerializable potentialBot: $visitor->potentialBot, remoteAddr: self::processAddress($visitor->remoteAddress, $anonymize), visitedUrl: $visitor->visitedUrl, - redirectUrl: null, // TODO + redirectUrl: $visitor->redirectUrl, visitLocation: $geolocation !== null ? VisitLocation::fromGeolocation($geolocation) : null, ); } diff --git a/module/Core/src/Visit/Model/Visitor.php b/module/Core/src/Visit/Model/Visitor.php index b33d10a1..53504d75 100644 --- a/module/Core/src/Visit/Model/Visitor.php +++ b/module/Core/src/Visit/Model/Visitor.php @@ -12,6 +12,7 @@ use function Shlinkio\Shlink\Core\geolocationFromRequest; use function Shlinkio\Shlink\Core\ipAddressFromRequest; use function Shlinkio\Shlink\Core\isCrawler; use function substr; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; final readonly class Visitor { @@ -28,7 +29,7 @@ final readonly class Visitor public string $visitedUrl, public bool $potentialBot, public Location|null $geolocation, - public string $redirectUrl, + public string|null $redirectUrl, ) { } @@ -38,7 +39,7 @@ final readonly class Visitor string|null $remoteAddress = null, string $visitedUrl = '', Location|null $geolocation = null, - string $redirectUrl = '', + string|null $redirectUrl = null, ): self { return new self( userAgent: self::cropToLength($userAgent, self::USER_AGENT_MAX_LENGTH), @@ -49,7 +50,7 @@ final readonly class Visitor visitedUrl: self::cropToLength($visitedUrl, self::VISITED_URL_MAX_LENGTH), potentialBot: isCrawler($userAgent), geolocation: $geolocation, - redirectUrl: self::cropToLength($redirectUrl, self::REDIRECT_URL_MAX_LENGTH), + redirectUrl: $redirectUrl === null ? null : self::cropToLength($redirectUrl, self::REDIRECT_URL_MAX_LENGTH), ); } @@ -66,8 +67,7 @@ final readonly class Visitor remoteAddress: ipAddressFromRequest($request), visitedUrl: $request->getUri()->__toString(), geolocation: geolocationFromRequest($request), - // TODO - redirectUrl: '', + redirectUrl: $request->getAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE), ); } From 85065c9330d3ceab399af7a44d8e6ce8a2c77d64 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2024 14:05:33 +0100 Subject: [PATCH 4/6] Test behavior to track redirect URL --- module/Core/src/Visit/Model/Visitor.php | 1 + module/Core/test/Action/PixelActionTest.php | 13 +++++++--- .../Core/test/Action/RedirectActionTest.php | 12 +++++++--- .../NotFoundTrackerMiddlewareTest.php | 24 +++++++++++++++---- .../ExtraPathRedirectMiddlewareTest.php | 7 +++++- 5 files changed, 45 insertions(+), 12 deletions(-) diff --git a/module/Core/src/Visit/Model/Visitor.php b/module/Core/src/Visit/Model/Visitor.php index 53504d75..cab834e6 100644 --- a/module/Core/src/Visit/Model/Visitor.php +++ b/module/Core/src/Visit/Model/Visitor.php @@ -12,6 +12,7 @@ use function Shlinkio\Shlink\Core\geolocationFromRequest; use function Shlinkio\Shlink\Core\ipAddressFromRequest; use function Shlinkio\Shlink\Core\isCrawler; use function substr; + use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; final readonly class Visitor diff --git a/module/Core/test/Action/PixelActionTest.php b/module/Core/test/Action/PixelActionTest.php index d6f2566a..e78df177 100644 --- a/module/Core/test/Action/PixelActionTest.php +++ b/module/Core/test/Action/PixelActionTest.php @@ -16,6 +16,8 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + class PixelActionTest extends TestCase { private PixelAction $action; @@ -34,12 +36,17 @@ class PixelActionTest extends TestCase public function imageIsReturned(): void { $shortCode = 'abc123'; + $shortUrl = ShortUrl::withLongUrl('http://domain.com/foo/bar'); + $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); + $this->urlResolver->expects($this->once())->method('resolveEnabledShortUrl')->with( ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, ''), - )->willReturn(ShortUrl::withLongUrl('http://domain.com/foo/bar')); - $this->requestTracker->expects($this->once())->method('trackIfApplicable')->withAnyParameters(); + )->willReturn($shortUrl); + $this->requestTracker->expects($this->once())->method('trackIfApplicable')->with( + $shortUrl, + $request->withAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE, null), + ); - $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); $response = $this->action->process($request, $this->createMock(RequestHandlerInterface::class)); self::assertInstanceOf(PixelResponse::class, $response); diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 2364371c..fa4a561d 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -19,6 +19,8 @@ use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + class RedirectActionTest extends TestCase { private const LONG_URL = 'https://domain.com/foo/bar?some=thing'; @@ -50,16 +52,20 @@ class RedirectActionTest extends TestCase { $shortCode = 'abc123'; $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); + $expectedResp = new Response\RedirectResponse(self::LONG_URL); + $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); + $this->urlResolver->expects($this->once())->method('resolveEnabledShortUrl')->with( ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, ''), )->willReturn($shortUrl); - $this->requestTracker->expects($this->once())->method('trackIfApplicable'); - $expectedResp = new Response\RedirectResponse(self::LONG_URL); + $this->requestTracker->expects($this->once())->method('trackIfApplicable')->with( + $shortUrl, + $request->withAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE, self::LONG_URL), + ); $this->redirectRespHelper->expects($this->once())->method('buildRedirectResponse')->with( self::LONG_URL, )->willReturn($expectedResp); - $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); $response = $this->action->process($request, $this->createMock(RequestHandlerInterface::class)); self::assertSame($expectedResp, $response); diff --git a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php index 4558197b..9df12a6d 100644 --- a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php +++ b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php @@ -4,7 +4,9 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\ErrorHandler; +use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequestFactory; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; @@ -14,6 +16,8 @@ use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\ErrorHandler\NotFoundTrackerMiddleware; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + class NotFoundTrackerMiddlewareTest extends TestCase { private NotFoundTrackerMiddleware $middleware; @@ -33,12 +37,22 @@ class NotFoundTrackerMiddlewareTest extends TestCase ); } - #[Test] - public function delegatesIntoRequestTracker(): void + #[Test, DataProvider('provideResponses')] + public function delegatesIntoRequestTracker(Response $resp, string|null $expectedRedirectUrl): void { - $this->handler->expects($this->once())->method('handle')->with($this->request); - $this->requestTracker->expects($this->once())->method('trackNotFoundIfApplicable')->with($this->request); + $this->handler->expects($this->once())->method('handle')->with($this->request)->willReturn($resp); + $this->requestTracker->expects($this->once())->method('trackNotFoundIfApplicable')->with( + $this->request->withAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE, $expectedRedirectUrl), + ); - $this->middleware->process($this->request, $this->handler); + $result = $this->middleware->process($this->request, $this->handler); + + self::assertSame($resp, $result); + } + + public static function provideResponses(): iterable + { + yield 'no location response' => [new Response(), null]; + yield 'location response' => [new Response\RedirectResponse('the_location'), 'the_location']; } } diff --git a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php index 85168020..84ceb790 100644 --- a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php +++ b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php @@ -30,6 +30,8 @@ use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; use function Laminas\Stratigility\middleware; use function str_starts_with; +use const Shlinkio\Shlink\REDIRECT_URL_REQUEST_ATTRIBUTE; + class ExtraPathRedirectMiddlewareTest extends TestCase { private MockObject & ShortUrlResolverInterface $resolver; @@ -159,7 +161,10 @@ class ExtraPathRedirectMiddlewareTest extends TestCase $this->redirectResponseHelper->expects($this->once())->method('buildRedirectResponse')->with( 'the_built_long_url', )->willReturn(new RedirectResponse('')); - $this->requestTracker->expects($this->once())->method('trackIfApplicable')->with($shortUrl, $request); + $this->requestTracker->expects($this->once())->method('trackIfApplicable')->with( + $shortUrl, + $request->withAttribute(REDIRECT_URL_REQUEST_ATTRIBUTE, 'the_built_long_url'), + ); $this->middleware($options)->process($request, $this->handler); } From d5544554efc7645761f390560ca4ba61ad79df53 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2024 14:08:23 +0100 Subject: [PATCH 5/6] Improve API docs description for redirectUrl fields --- docs/async-api/async-api.json | 2 +- docs/swagger/definitions/Visit.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/async-api/async-api.json b/docs/async-api/async-api.json index 09817a99..2d69084b 100644 --- a/docs/async-api/async-api.json +++ b/docs/async-api/async-api.json @@ -251,7 +251,7 @@ "redirectUrl": { "type": "string", "nullable": true, - "description": "The URL to which the visitor was redirected" + "description": "The URL to which the visitor was redirected, or null if a redirect did not occur, like for 404 requests or pixel tracking" } }, "example": { diff --git a/docs/swagger/definitions/Visit.json b/docs/swagger/definitions/Visit.json index 826ad1ac..2ccdfd23 100644 --- a/docs/swagger/definitions/Visit.json +++ b/docs/swagger/definitions/Visit.json @@ -28,7 +28,7 @@ }, "redirectUrl": { "type": ["string", "null"], - "description": "The URL to which the visitor was redirected" + "description": "The URL to which the visitor was redirected, or null if a redirect did not occur, like for 404 requests or pixel tracking" } } } From 571a4643ab139f03c9e35e188da3ee66650eafae Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Nov 2024 14:11:44 +0100 Subject: [PATCH 6/6] Update changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3281630..fee93cf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * `geolocation-country-code`: Allows to perform redirections based on the ISO 3166-1 alpha-2 two-letter country code resolved while geolocating the visitor. * `geolocation-city-name`: Allows to perform redirections based on the city name resolved while geolocating the visitor. +* [#2032](https://github.com/shlinkio/shlink/issues/2032) Save the URL to which a visitor is redirected when a visit is tracked. + + The value is exposed in the API as a new `redirectUrl` field for visit objects. + + This is useful to know where a visitor was redirected for a short URL with dynamic redirect rules, for special redirects, or simply in case the long URL was changed over time, and you still want to know where visitors were redirected originally. + + Some visits may not have a redirect URL if a redirect didn't happen, like for orphan visits when no special redirects are configured, or when a visit is tracked as part of the pixel action. + ### Changed * [#2193](https://github.com/shlinkio/shlink/issues/2193) API keys are now hashed using SHA256, instead of being saved in plain text.