From 09e81b00c559a5615eff2d2108dd9d5f8321aa5d Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 24 Feb 2024 23:10:08 +0100 Subject: [PATCH 1/9] Create component to resolve the long URL to redirect to for a short URL --- module/Core/config/dependencies.config.php | 8 ++- .../ShortUrlRedirectionResolver.php | 23 +++++++ .../ShortUrlRedirectionResolverInterface.php | 11 ++++ .../Helper/ShortUrlRedirectionBuilder.php | 13 ++-- .../ShortUrlRedirectionResolverTest.php | 60 +++++++++++++++++++ .../Helper/ShortUrlRedirectionBuilderTest.php | 37 ++++-------- 6 files changed, 121 insertions(+), 31 deletions(-) create mode 100644 module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php create mode 100644 module/Core/src/RedirectRule/ShortUrlRedirectionResolverInterface.php create mode 100644 module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 5f9ae565..6246b307 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -32,6 +32,8 @@ return [ Options\QrCodeOptions::class => [ValinorConfigFactory::class, 'config.qr_codes'], Options\RabbitMqOptions::class => [ValinorConfigFactory::class, 'config.rabbitmq'], + RedirectRule\ShortUrlRedirectionResolver::class => ConfigAbstractFactory::class, + ShortUrl\UrlShortener::class => ConfigAbstractFactory::class, ShortUrl\ShortUrlService::class => ConfigAbstractFactory::class, ShortUrl\ShortUrlListService::class => ConfigAbstractFactory::class, @@ -156,6 +158,7 @@ return [ Util\RedirectResponseHelper::class => [Options\RedirectOptions::class], Config\NotFoundRedirectResolver::class => [Util\RedirectResponseHelper::class, 'Logger_Shlink'], + RedirectRule\ShortUrlRedirectionResolver::class => ['em'], Action\RedirectAction::class => [ ShortUrl\ShortUrlResolver::class, @@ -179,7 +182,10 @@ return [ ], ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'], ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => ['httpClient', Options\UrlShortenerOptions::class], - ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [Options\TrackingOptions::class], + ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [ + Options\TrackingOptions::class, + RedirectRule\ShortUrlRedirectionResolver::class, + ], ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], ShortUrl\Middleware\ExtraPathRedirectMiddleware::class => [ ShortUrl\ShortUrlResolver::class, diff --git a/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php b/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php new file mode 100644 index 00000000..dc2ae131 --- /dev/null +++ b/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php @@ -0,0 +1,23 @@ +getHeaderLine('User-Agent')); + return $shortUrl->longUrlForDevice($device); + } +} diff --git a/module/Core/src/RedirectRule/ShortUrlRedirectionResolverInterface.php b/module/Core/src/RedirectRule/ShortUrlRedirectionResolverInterface.php new file mode 100644 index 00000000..a1dd92a2 --- /dev/null +++ b/module/Core/src/RedirectRule/ShortUrlRedirectionResolverInterface.php @@ -0,0 +1,11 @@ +redirectionResolver->resolveLongUrl($shortUrl, $request)); $currentQuery = $request->getQueryParams(); - $device = DeviceType::matchFromUserAgent($request->getHeaderLine('User-Agent')); - $uri = new Uri($shortUrl->longUrlForDevice($device)); $shouldForwardQuery = $shortUrl->forwardQuery(); return $uri diff --git a/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php b/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php new file mode 100644 index 00000000..03a5ce6c --- /dev/null +++ b/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php @@ -0,0 +1,60 @@ +em = $this->createMock(EntityManagerInterface::class); + $this->resolver = new ShortUrlRedirectionResolver($this->em); + } + + #[Test, DataProvider('provideData')] + public function resolveLongUrlReturnsExpectedValue(ServerRequestInterface $request, string $expectedUrl): void + { + $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ + 'longUrl' => 'https://example.com/foo/bar', + 'deviceLongUrls' => [ + DeviceType::ANDROID->value => 'https://example.com/android', + DeviceType::IOS->value => 'https://example.com/ios', + ], + ])); + + $result = $this->resolver->resolveLongUrl($shortUrl, $request); + + self::assertEquals($expectedUrl, $result); + } + + public static function provideData(): iterable + { + $request = static fn (string $userAgent = '') => ServerRequestFactory::fromGlobals()->withHeader( + 'User-Agent', + $userAgent, + ); + + yield 'unknown user agent' => [$request('Unknown'), 'https://example.com/foo/bar']; + yield 'desktop user agent' => [$request(DESKTOP_USER_AGENT), 'https://example.com/foo/bar']; + yield 'android user agent' => [$request(ANDROID_USER_AGENT), 'https://example.com/android']; + yield 'ios user agent' => [$request(IOS_USER_AGENT), 'https://example.com/ios']; + } +} diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php index cf88db35..53a86322 100644 --- a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -7,26 +7,26 @@ namespace ShlinkioTest\Shlink\Core\ShortUrl\Helper; use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; -use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\Options\TrackingOptions; +use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectionResolverInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilder; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; -use const ShlinkioTest\Shlink\ANDROID_USER_AGENT; -use const ShlinkioTest\Shlink\DESKTOP_USER_AGENT; -use const ShlinkioTest\Shlink\IOS_USER_AGENT; - class ShortUrlRedirectionBuilderTest extends TestCase { private ShortUrlRedirectionBuilder $redirectionBuilder; + private ShortUrlRedirectionResolverInterface & MockObject $redirectionResolver; protected function setUp(): void { $trackingOptions = new TrackingOptions(disableTrackParam: 'foobar'); - $this->redirectionBuilder = new ShortUrlRedirectionBuilder($trackingOptions); + $this->redirectionResolver = $this->createMock(ShortUrlRedirectionResolverInterface::class); + + $this->redirectionBuilder = new ShortUrlRedirectionBuilder($trackingOptions, $this->redirectionResolver); } #[Test, DataProvider('provideData')] @@ -39,11 +39,12 @@ class ShortUrlRedirectionBuilderTest extends TestCase $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'longUrl' => 'https://domain.com/foo/bar?some=thing', 'forwardQuery' => $forwardQuery, - 'deviceLongUrls' => [ - DeviceType::ANDROID->value => 'https://domain.com/android', - DeviceType::IOS->value => 'https://domain.com/ios', - ], ])); + $this->redirectionResolver->expects($this->once())->method('resolveLongUrl')->with( + $shortUrl, + $request, + )->willReturn($shortUrl->getLongUrl()); + $result = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath); self::assertEquals($expectedUrl, $result); @@ -72,7 +73,7 @@ class ShortUrlRedirectionBuilderTest extends TestCase ]; yield [ 'https://domain.com/foo/bar?some=overwritten', - $request(['foobar' => 'notrack', 'some' => 'overwritten'])->withHeader('User-Agent', 'Unknown'), + $request(['foobar' => 'notrack', 'some' => 'overwritten']), null, true, ]; @@ -91,7 +92,7 @@ class ShortUrlRedirectionBuilderTest extends TestCase yield ['https://domain.com/foo/bar/something/else-baz?some=thing', $request(), '/something/else-baz', true]; yield [ 'https://domain.com/foo/bar/something/else-baz?some=thing&hello=world', - $request(['hello' => 'world'])->withHeader('User-Agent', DESKTOP_USER_AGENT), + $request(['hello' => 'world']), '/something/else-baz', true, ]; @@ -107,17 +108,5 @@ class ShortUrlRedirectionBuilderTest extends TestCase '/something/else-baz', false, ]; - yield [ - 'https://domain.com/android/something', - $request(['foo' => 'bar'])->withHeader('User-Agent', ANDROID_USER_AGENT), - '/something', - false, - ]; - yield [ - 'https://domain.com/ios?foo=bar', - $request(['foo' => 'bar'])->withHeader('User-Agent', IOS_USER_AGENT), - null, - null, - ]; } } From 7f83d37b3c35d2f397b1e25587f7cdc0adaebcbc Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 24 Feb 2024 23:33:16 +0100 Subject: [PATCH 2/9] Add logic to match redirect conditions based on query params or language --- module/Core/functions/functions.php | 7 +++++ .../RedirectRule/Entity/RedirectCondition.php | 29 +++++++++++++++++++ .../Model/RedirectConditionType.php | 6 ++-- 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index f26cb84f..586195da 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -26,7 +26,9 @@ use function print_r; use function Shlinkio\Shlink\Common\buildDateRange; use function sprintf; use function str_repeat; +use function str_replace; use function strtolower; +use function trim; use function ucfirst; function generateRandomShortCode(int $length, ShortUrlMode $mode = ShortUrlMode::STRICT): string @@ -74,6 +76,11 @@ function normalizeDate(string|DateTimeInterface|Chronos $date): Chronos return normalizeOptionalDate($date); } +function normalizeLocale(string $locale): string +{ + return trim(strtolower(str_replace('_', '-', $locale))); +} + function getOptionalIntFromInputFilter(InputFilter $inputFilter, string $fieldName): ?int { $value = $inputFilter->getValue($fieldName); diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 367d96d9..72960d82 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -2,9 +2,14 @@ namespace Shlinkio\Shlink\Core\RedirectRule\Entity; +use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; +use function explode; +use function Shlinkio\Shlink\Core\ArrayUtils\some; +use function Shlinkio\Shlink\Core\normalizeLocale; + class RedirectCondition extends AbstractEntity { public function __construct( @@ -14,4 +19,28 @@ class RedirectCondition extends AbstractEntity public readonly ?string $matchKey = null, ) { } + + /** + * Tells if this condition matches provided request + */ + public function matchesRequest(ServerRequestInterface $request): bool + { + if ($this->type === RedirectConditionType::QUERY_PARAM && $this->matchKey !== null) { + $query = $request->getQueryParams(); + $queryValue = $query[$this->matchKey] ?? null; + return $queryValue === $this->matchValue; + } + + if ($this->type === RedirectConditionType::LANGUAGE && $request->hasHeader('Accept-Language')) { + $acceptedLanguages = explode(',', $request->getHeaderLine('Accept-Language')); + $normalizedLanguage = normalizeLocale($this->matchValue); + + return some( + $acceptedLanguages, + static fn (string $lang) => normalizeLocale($lang) === $normalizedLanguage, + ); + } + + return false; + } } diff --git a/module/Core/src/RedirectRule/Model/RedirectConditionType.php b/module/Core/src/RedirectRule/Model/RedirectConditionType.php index 6e6b8113..764c5a2b 100644 --- a/module/Core/src/RedirectRule/Model/RedirectConditionType.php +++ b/module/Core/src/RedirectRule/Model/RedirectConditionType.php @@ -4,7 +4,7 @@ namespace Shlinkio\Shlink\Core\RedirectRule\Model; enum RedirectConditionType: string { - case DEVICE = 'device'; -// case LANGUAGE = 'language'; -// case QUERY_PARAM = 'query'; +// case DEVICE = 'device'; + case LANGUAGE = 'language'; + case QUERY_PARAM = 'query'; } From 4e87affb0bfd7a8dbe576a80ab0f01db7ff9b6f1 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Feb 2024 12:25:55 +0100 Subject: [PATCH 3/9] Take redirect rules into consideration when resolving the long URL for a short URL --- .../RedirectRule/Entity/ShortUrlRedirectRule.php | 16 +++++++++++++++- .../RedirectRule/ShortUrlRedirectionResolver.php | 12 +++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php index 65b39901..7fa1a671 100644 --- a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php +++ b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php @@ -4,9 +4,12 @@ namespace Shlinkio\Shlink\Core\RedirectRule\Entity; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; +use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; +use function Shlinkio\Shlink\Core\ArrayUtils\every; + class ShortUrlRedirectRule extends AbstractEntity { /** @@ -14,9 +17,20 @@ class ShortUrlRedirectRule extends AbstractEntity */ public function __construct( private readonly ShortUrl $shortUrl, // No need to read this field. It's used by doctrine - public readonly int $priority, + private readonly int $priority, public readonly string $longUrl, public readonly Collection $conditions = new ArrayCollection(), ) { } + + /** + * Tells if this condition matches provided request + */ + public function matchesRequest(ServerRequestInterface $request): bool + { + return every( + $this->conditions, + static fn (RedirectCondition $condition) => $condition->matchesRequest($request), + ); + } } diff --git a/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php b/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php index dc2ae131..b916d78b 100644 --- a/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php +++ b/module/Core/src/RedirectRule/ShortUrlRedirectionResolver.php @@ -5,6 +5,7 @@ namespace Shlinkio\Shlink\Core\RedirectRule; use Doctrine\ORM\EntityManagerInterface; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Model\DeviceType; +use Shlinkio\Shlink\Core\RedirectRule\Entity\ShortUrlRedirectRule; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; readonly class ShortUrlRedirectionResolver implements ShortUrlRedirectionResolverInterface @@ -15,7 +16,16 @@ readonly class ShortUrlRedirectionResolver implements ShortUrlRedirectionResolve public function resolveLongUrl(ShortUrl $shortUrl, ServerRequestInterface $request): string { - // TODO Resolve rules and check if any of them matches + $rules = $this->em->getRepository(ShortUrlRedirectRule::class)->findBy( + criteria: ['shortUrl' => $shortUrl], + orderBy: ['priority' => 'ASC'], + ); + foreach ($rules as $rule) { + // Return the long URL for the first rule found that matches + if ($rule->matchesRequest($request)) { + return $rule->longUrl; + } + } $device = DeviceType::matchFromUserAgent($request->getHeaderLine('User-Agent')); return $shortUrl->longUrlForDevice($device); From 202d0b86b3fe0ca480737a8ac6a0e585ee307ac5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Feb 2024 17:13:54 +0100 Subject: [PATCH 4/9] Extract logic to match every type of redirect condition to its own private method --- .../RedirectRule/Entity/RedirectCondition.php | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 72960d82..608acd06 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -25,22 +25,38 @@ class RedirectCondition extends AbstractEntity */ public function matchesRequest(ServerRequestInterface $request): bool { - if ($this->type === RedirectConditionType::QUERY_PARAM && $this->matchKey !== null) { - $query = $request->getQueryParams(); - $queryValue = $query[$this->matchKey] ?? null; - return $queryValue === $this->matchValue; + return match ($this->type) { + RedirectConditionType::QUERY_PARAM => $this->matchesQueryParam($request), + RedirectConditionType::LANGUAGE => $this->matchesLanguage($request), + default => false, + }; + } + + public function matchesQueryParam(ServerRequestInterface $request): bool + { + if ($this->matchKey !== null) { + return false; } - if ($this->type === RedirectConditionType::LANGUAGE && $request->hasHeader('Accept-Language')) { - $acceptedLanguages = explode(',', $request->getHeaderLine('Accept-Language')); - $normalizedLanguage = normalizeLocale($this->matchValue); + $query = $request->getQueryParams(); + $queryValue = $query[$this->matchKey] ?? null; - return some( - $acceptedLanguages, - static fn (string $lang) => normalizeLocale($lang) === $normalizedLanguage, - ); + return $queryValue === $this->matchValue; + } + + public function matchesLanguage(ServerRequestInterface $request): bool + { + $acceptLanguage = $request->getHeaderLine('Accept-Language'); + if ($acceptLanguage === '' || $acceptLanguage === '*') { + return false; } - return false; + $acceptedLanguages = explode(',', $acceptLanguage); + $normalizedLanguage = normalizeLocale($this->matchValue); + + return some( + $acceptedLanguages, + static fn (string $lang) => normalizeLocale($lang) === $normalizedLanguage, + ); } } From 3f1b253c3183c92d37b81d51ad74ba285ed072f8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Feb 2024 19:21:39 +0100 Subject: [PATCH 5/9] Add test for RedirectCondition request matching --- .../RedirectRule/Entity/RedirectCondition.php | 30 +++++++--- .../Entity/RedirectConditionTest.php | 59 +++++++++++++++++++ 2 files changed, 80 insertions(+), 9 deletions(-) create mode 100644 module/Core/test/RedirectRule/Entity/RedirectConditionTest.php diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 608acd06..5f3de073 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -9,17 +9,34 @@ use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; use function explode; use function Shlinkio\Shlink\Core\ArrayUtils\some; use function Shlinkio\Shlink\Core\normalizeLocale; +use function sprintf; class RedirectCondition extends AbstractEntity { - public function __construct( + private function __construct( public readonly string $name, - public readonly RedirectConditionType $type, + private readonly RedirectConditionType $type, public readonly string $matchValue, public readonly ?string $matchKey = null, ) { } + public static function forQueryParam(string $param, string $value): self + { + $type = RedirectConditionType::QUERY_PARAM; + $name = sprintf('%s-%s-%s', $type->value, $param, $value); + + return new self($name, $type, $value, $param); + } + + public static function forLanguage(string $language): self + { + $type = RedirectConditionType::LANGUAGE; + $name = sprintf('%s-%s', $type->value, $language); + + return new self($name, $type, $language); + } + /** * Tells if this condition matches provided request */ @@ -28,23 +45,18 @@ class RedirectCondition extends AbstractEntity return match ($this->type) { RedirectConditionType::QUERY_PARAM => $this->matchesQueryParam($request), RedirectConditionType::LANGUAGE => $this->matchesLanguage($request), - default => false, }; } - public function matchesQueryParam(ServerRequestInterface $request): bool + private function matchesQueryParam(ServerRequestInterface $request): bool { - if ($this->matchKey !== null) { - return false; - } - $query = $request->getQueryParams(); $queryValue = $query[$this->matchKey] ?? null; return $queryValue === $this->matchValue; } - public function matchesLanguage(ServerRequestInterface $request): bool + private function matchesLanguage(ServerRequestInterface $request): bool { $acceptLanguage = $request->getHeaderLine('Accept-Language'); if ($acceptLanguage === '' || $acceptLanguage === '*') { diff --git a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php new file mode 100644 index 00000000..c52988dc --- /dev/null +++ b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php @@ -0,0 +1,59 @@ +withQueryParams(['foo' => 'bar']); + $result = RedirectCondition::forQueryParam($param, $value)->matchesRequest($request); + + self::assertEquals($expectedResult, $result); + } + + #[Test] + #[TestWith([null, '', false])] // no accept language + #[TestWith(['', '', false])] // empty accept language + #[TestWith(['*', '', false])] // wildcard accept language + #[TestWith(['en', 'en', true])] // single language match + #[TestWith(['es, en,fr', 'en', true])] // multiple languages match + #[TestWith(['es_ES', 'es-ES', true])] // single locale match + #[TestWith(['en-UK', 'en-uk', true])] // different casing match + public function matchesLanguage(?string $acceptLanguage, string $value, bool $expected): void + { + $request = ServerRequestFactory::fromGlobals(); + if ($acceptLanguage !== null) { + $request = $request->withHeader('Accept-Language', $acceptLanguage); + } + + $result = RedirectCondition::forLanguage($value)->matchesRequest($request); + + self::assertEquals($expected, $result); + } + + #[Test, DataProvider('provideNames')] + public function generatesExpectedName(RedirectCondition $condition, string $expectedName): void + { + self::assertEquals($expectedName, $condition->name); + } + + public static function provideNames(): iterable + { + yield [RedirectCondition::forLanguage('es-ES'), 'language-es-ES']; + yield [RedirectCondition::forLanguage('en_UK'), 'language-en_UK']; + yield [RedirectCondition::forQueryParam('foo', 'bar'), 'query-foo-bar']; + yield [RedirectCondition::forQueryParam('baz', 'foo'), 'query-baz-foo']; + } +} From 175712d4a951342f105b40ae64bc40d3c1405c94 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Feb 2024 19:38:54 +0100 Subject: [PATCH 6/9] Add test for ShortUrlRedirectRule request matching --- .../Entity/ShortUrlRedirectRule.php | 2 +- .../Entity/ShortUrlRedirectRuleTest.php | 49 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php diff --git a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php index 7fa1a671..74a87930 100644 --- a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php +++ b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php @@ -28,7 +28,7 @@ class ShortUrlRedirectRule extends AbstractEntity */ public function matchesRequest(ServerRequestInterface $request): bool { - return every( + return $this->conditions->count() > 0 && every( $this->conditions, static fn (RedirectCondition $condition) => $condition->matchesRequest($request), ); diff --git a/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php b/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php new file mode 100644 index 00000000..244e4f2f --- /dev/null +++ b/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php @@ -0,0 +1,49 @@ +withHeader('Accept-Language', 'en-UK') + ->withQueryParams(['foo' => 'bar']); + + $result = $this->createRule($conditions)->matchesRequest($request); + + self::assertEquals($expectedResult, $result); + } + + public static function provideConditions(): iterable + { + yield 'no conditions' => [[], false]; + yield 'not all conditions match' => [ + [RedirectCondition::forLanguage('en-UK'), RedirectCondition::forQueryParam('foo', 'foo')], + false, + ]; + yield 'all conditions match' => [ + [RedirectCondition::forLanguage('en-UK'), RedirectCondition::forQueryParam('foo', 'bar')], + true, + ]; + } + + /** + * @param RedirectCondition[] $conditions + */ + private function createRule(array $conditions): ShortUrlRedirectRule + { + $shortUrl = ShortUrl::withLongUrl('https://s.test'); + return new ShortUrlRedirectRule($shortUrl, 1, '', new ArrayCollection($conditions)); + } +} From 07ae92943d66b02aa3f04baf7a004bb106c7e254 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Feb 2024 23:09:16 +0100 Subject: [PATCH 7/9] Add test for ShortUrlRedirectResolver rule matching --- .../ShortUrlRedirectionResolverTest.php | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php b/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php index 03a5ce6c..2a71dfe2 100644 --- a/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php +++ b/module/Core/test/RedirectRule/ShortUrlRedirectionResolverTest.php @@ -2,7 +2,9 @@ namespace RedirectRule; +use Doctrine\Common\Collections\ArrayCollection; use Doctrine\ORM\EntityManagerInterface; +use Doctrine\ORM\EntityRepository; use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; @@ -10,6 +12,8 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Model\DeviceType; +use Shlinkio\Shlink\Core\RedirectRule\Entity\RedirectCondition; +use Shlinkio\Shlink\Core\RedirectRule\Entity\ShortUrlRedirectRule; use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectionResolver; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlCreation; @@ -30,8 +34,11 @@ class ShortUrlRedirectionResolverTest extends TestCase } #[Test, DataProvider('provideData')] - public function resolveLongUrlReturnsExpectedValue(ServerRequestInterface $request, string $expectedUrl): void - { + public function resolveLongUrlReturnsExpectedValue( + ServerRequestInterface $request, + ?RedirectCondition $condition, + string $expectedUrl, + ): void { $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ 'longUrl' => 'https://example.com/foo/bar', 'deviceLongUrls' => [ @@ -40,6 +47,16 @@ class ShortUrlRedirectionResolverTest extends TestCase ], ])); + $repo = $this->createMock(EntityRepository::class); + $repo->expects($this->once())->method('findBy')->willReturn($condition !== null ? [ + new ShortUrlRedirectRule($shortUrl, 1, 'https://example.com/from-rule', new ArrayCollection([ + $condition, + ])), + ] : []); + $this->em->expects($this->once())->method('getRepository')->with(ShortUrlRedirectRule::class)->willReturn( + $repo, + ); + $result = $this->resolver->resolveLongUrl($shortUrl, $request); self::assertEquals($expectedUrl, $result); @@ -52,9 +69,27 @@ class ShortUrlRedirectionResolverTest extends TestCase $userAgent, ); - yield 'unknown user agent' => [$request('Unknown'), 'https://example.com/foo/bar']; - yield 'desktop user agent' => [$request(DESKTOP_USER_AGENT), 'https://example.com/foo/bar']; - yield 'android user agent' => [$request(ANDROID_USER_AGENT), 'https://example.com/android']; - yield 'ios user agent' => [$request(IOS_USER_AGENT), 'https://example.com/ios']; + yield 'unknown user agent' => [ + $request('Unknown'), // This user agent won't match any device + RedirectCondition::forLanguage('es-ES'), // This condition won't match + 'https://example.com/foo/bar', + ]; + yield 'desktop user agent' => [$request(DESKTOP_USER_AGENT), null, 'https://example.com/foo/bar']; + yield 'android user agent' => [ + $request(ANDROID_USER_AGENT), + RedirectCondition::forQueryParam('foo', 'bar'), // This condition won't match + 'https://example.com/android', + ]; + yield 'ios user agent' => [$request(IOS_USER_AGENT), null, 'https://example.com/ios']; + yield 'matching language' => [ + $request()->withHeader('Accept-Language', 'es-ES'), + RedirectCondition::forLanguage('es-ES'), + 'https://example.com/from-rule', + ]; + yield 'matching query params' => [ + $request()->withQueryParams(['foo' => 'bar']), + RedirectCondition::forQueryParam('foo', 'bar'), + 'https://example.com/from-rule', + ]; } } From df5ad554c13256cb31be4e74853368669118e670 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 26 Feb 2024 19:05:39 +0100 Subject: [PATCH 8/9] Add E2E tests for dynamic rule-based redirects --- composer.json | 2 +- .../RedirectRule/Entity/RedirectCondition.php | 4 +- .../Entity/ShortUrlRedirectRule.php | 2 +- module/Core/test-api/Action/RedirectTest.php | 46 ++++++++++++-- .../Fixtures/ShortUrlRedirectRulesFixture.php | 62 +++++++++++++++++++ 5 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php diff --git a/composer.json b/composer.json index fdb6459f..a09eafa0 100644 --- a/composer.json +++ b/composer.json @@ -71,7 +71,7 @@ "phpunit/phpunit": "^10.4", "roave/security-advisories": "dev-master", "shlinkio/php-coding-standard": "~2.3.0", - "shlinkio/shlink-test-utils": "^4.0", + "shlinkio/shlink-test-utils": "^4.1", "symfony/var-dumper": "^7.0", "veewee/composer-run-parallel": "^1.3" }, diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 5f3de073..da235f9e 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -16,8 +16,8 @@ class RedirectCondition extends AbstractEntity private function __construct( public readonly string $name, private readonly RedirectConditionType $type, - public readonly string $matchValue, - public readonly ?string $matchKey = null, + private readonly string $matchValue, + private readonly ?string $matchKey = null, ) { } diff --git a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php index 74a87930..9e84e4fb 100644 --- a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php +++ b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php @@ -19,7 +19,7 @@ class ShortUrlRedirectRule extends AbstractEntity private readonly ShortUrl $shortUrl, // No need to read this field. It's used by doctrine private readonly int $priority, public readonly string $longUrl, - public readonly Collection $conditions = new ArrayCollection(), + private Collection $conditions = new ArrayCollection(), ) { } diff --git a/module/Core/test-api/Action/RedirectTest.php b/module/Core/test-api/Action/RedirectTest.php index bbcc6fec..64c5fd95 100644 --- a/module/Core/test-api/Action/RedirectTest.php +++ b/module/Core/test-api/Action/RedirectTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioApiTest\Shlink\Core\Action; +use GuzzleHttp\RequestOptions; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; @@ -15,9 +16,9 @@ use const ShlinkioTest\Shlink\IOS_USER_AGENT; class RedirectTest extends ApiTestCase { #[Test, DataProvider('provideUserAgents')] - public function properRedirectHappensBasedOnUserAgent(?string $userAgent, string $expectedRedirect): void + public function properRedirectHappensBasedOnUserAgent(array $options, string $expectedRedirect): void { - $response = $this->callShortUrl('def456', $userAgent); + $response = $this->callShortUrl('def456', $options); self::assertEquals(302, $response->getStatusCode()); self::assertEquals($expectedRedirect, $response->getHeaderLine('Location')); @@ -25,15 +26,48 @@ class RedirectTest extends ApiTestCase public static function provideUserAgents(): iterable { - yield 'android' => [ANDROID_USER_AGENT, 'https://blog.alejandrocelaya.com/android']; - yield 'ios' => [IOS_USER_AGENT, 'https://blog.alejandrocelaya.com/ios']; + yield 'android' => [ + [ + RequestOptions::HEADERS => ['User-Agent' => ANDROID_USER_AGENT], + ], + 'https://blog.alejandrocelaya.com/android', + ]; + yield 'ios' => [ + [ + RequestOptions::HEADERS => ['User-Agent' => IOS_USER_AGENT], + ], + 'https://blog.alejandrocelaya.com/ios', + ]; yield 'desktop' => [ - DESKTOP_USER_AGENT, + [ + RequestOptions::HEADERS => ['User-Agent' => DESKTOP_USER_AGENT], + ], 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', ]; yield 'unknown' => [ - null, + [], 'https://blog.alejandrocelaya.com/2017/12/09/acmailer-7-0-the-most-important-release-in-a-long-time/', ]; + yield 'rule: english and foo' => [ + [ + RequestOptions::HEADERS => [ + 'Accept-Language' => 'en-UK', + ], + RequestOptions::QUERY => ['foo' => 'bar'], + ], + 'https://example.com/english-and-foo-query?foo=bar', + ]; + yield 'rule: multiple query params' => [ + [ + RequestOptions::QUERY => ['foo' => 'bar', 'hello' => 'world'], + ], + 'https://example.com/multiple-query-params?foo=bar&hello=world', + ]; + yield 'rule: english' => [ + [ + RequestOptions::HEADERS => ['Accept-Language' => 'en-UK'], + ], + 'https://example.com/only-english', + ]; } } diff --git a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php new file mode 100644 index 00000000..8f40af7d --- /dev/null +++ b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php @@ -0,0 +1,62 @@ +getReference('def456_short_url'); + + $englishCondition = RedirectCondition::forLanguage('en-UK'); + $manager->persist($englishCondition); + + $fooQueryCondition = RedirectCondition::forQueryParam('foo', 'bar'); + $manager->persist($fooQueryCondition); + + $helloQueryCondition = RedirectCondition::forQueryParam('hello', 'world'); + $manager->persist($helloQueryCondition); + + $englishAndFooQueryRule = new ShortUrlRedirectRule( + $defShortUrl, + 1, + 'https://example.com/english-and-foo-query', + new ArrayCollection([$englishCondition, $fooQueryCondition]), + ); + $manager->persist($englishAndFooQueryRule); + + $multipleQueryParamsRule = new ShortUrlRedirectRule( + $defShortUrl, + 2, + 'https://example.com/multiple-query-params', + new ArrayCollection([$helloQueryCondition, $fooQueryCondition]), + ); + $manager->persist($multipleQueryParamsRule); + + $onlyEnglishRule = new ShortUrlRedirectRule( + $defShortUrl, + 3, + 'https://example.com/only-english', + new ArrayCollection([$englishCondition]), + ); + $manager->persist($onlyEnglishRule); + + $manager->flush(); + } +} From 3284cea6f243f4637246857573b992f01ae2a298 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 26 Feb 2024 19:08:21 +0100 Subject: [PATCH 9/9] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f1cf0b8d..522f28c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ## [Unreleased] ### Added +* [#1902](https://github.com/shlinkio/shlink/issues/1902) Add dynamic redirects based on query parameters. + + This is implemented on top of the new [rule-based redirects](https://github.com/shlinkio/shlink/discussions/1912). + * [#1868](https://github.com/shlinkio/shlink/issues/1868) Add support for [docker compose secrets](https://docs.docker.com/compose/use-secrets/) to the docker image. * [#1979](https://github.com/shlinkio/shlink/issues/1979) Allow orphan visits lists to be filtered by type.