From 5b5d0aae4991d074d0a126f0be5b79ee6b8ea8eb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 22 Jul 2025 08:20:21 +0200 Subject: [PATCH] Make RedirectCondition->matchValue nullable --- .../definitions/SetShortUrlRedirectRule.json | 2 +- ....RedirectRule.Entity.RedirectCondition.php | 1 + .../Core/migrations/Version20250722060208.php | 26 +++++++++++++++++++ .../RedirectRule/Entity/RedirectCondition.php | 22 +++++++++------- .../Model/RedirectConditionType.php | 6 +---- .../Validation/RedirectRulesInputFilter.php | 2 +- .../Model/RedirectConditionTypeTest.php | 4 +-- .../test-api/Action/ListRedirectRulesTest.php | 17 +++++++----- .../test-api/Action/SetRedirectRulesTest.php | 17 +++++++----- .../Fixtures/ShortUrlRedirectRulesFixture.php | 2 +- 10 files changed, 66 insertions(+), 33 deletions(-) create mode 100644 module/Core/migrations/Version20250722060208.php diff --git a/docs/swagger/definitions/SetShortUrlRedirectRule.json b/docs/swagger/definitions/SetShortUrlRedirectRule.json index b6c66f5f..15380faa 100644 --- a/docs/swagger/definitions/SetShortUrlRedirectRule.json +++ b/docs/swagger/definitions/SetShortUrlRedirectRule.json @@ -31,7 +31,7 @@ "type": ["string", "null"] }, "matchValue": { - "type": "string" + "type": ["string", "null"] } } } diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.RedirectCondition.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.RedirectCondition.php index 513089fa..516b0640 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.RedirectCondition.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.RedirectCondition.php @@ -39,5 +39,6 @@ return static function (ClassMetadata $metadata, array $emConfig): void { fieldWithUtf8Charset($builder->createField('matchValue', Types::STRING), $emConfig) ->columnName('match_value') ->length(512) + ->nullable() ->build(); }; diff --git a/module/Core/migrations/Version20250722060208.php b/module/Core/migrations/Version20250722060208.php new file mode 100644 index 00000000..552b3dcc --- /dev/null +++ b/module/Core/migrations/Version20250722060208.php @@ -0,0 +1,26 @@ +getTable('redirect_conditions')->getColumn('match_value')->setNotnull(false); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 45e29839..2413400d 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -26,7 +26,7 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable { private function __construct( public readonly RedirectConditionType $type, - private readonly string $matchValue, + private readonly string|null $matchValue = null, private readonly string|null $matchKey = null, ) { } @@ -38,12 +38,12 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable public static function forAnyValueQueryParam(string $param): self { - return new self(RedirectConditionType::ANY_VALUE_QUERY_PARAM, $param); + return new self(RedirectConditionType::ANY_VALUE_QUERY_PARAM, matchKey: $param); } public static function forValuelessQueryParam(string $param): self { - return new self(RedirectConditionType::VALUELESS_QUERY_PARAM, $param); + return new self(RedirectConditionType::VALUELESS_QUERY_PARAM, matchKey: $param); } public static function forLanguage(string $language): self @@ -131,19 +131,19 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesValuelessQueryParam(ServerRequestInterface $request): bool { $query = $request->getQueryParams(); - return array_key_exists($this->matchValue, $query) && empty($query[$this->matchValue]); + return $this->matchKey !== null && array_key_exists($this->matchKey, $query) && empty($query[$this->matchKey]); } private function matchesAnyValueQueryParam(ServerRequestInterface $request): bool { $query = $request->getQueryParams(); - return array_key_exists($this->matchValue, $query); + return $this->matchKey !== null && array_key_exists($this->matchKey, $query); } private function matchesLanguage(ServerRequestInterface $request): bool { $acceptLanguage = trim($request->getHeaderLine('Accept-Language')); - if ($acceptLanguage === '' || $acceptLanguage === '*') { + if ($acceptLanguage === '' || $acceptLanguage === '*' || $this->matchValue === null) { return false; } @@ -173,13 +173,17 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesRemoteIpAddress(ServerRequestInterface $request): bool { $remoteAddress = ipAddressFromRequest($request); - return $remoteAddress !== null && IpAddressUtils::ipAddressMatchesGroups($remoteAddress, [$this->matchValue]); + return ( + $this->matchValue !== null + && $remoteAddress !== null + && IpAddressUtils::ipAddressMatchesGroups($remoteAddress, [$this->matchValue]) + ); } private function matchesGeolocationCountryCode(ServerRequestInterface $request): bool { $geolocation = geolocationFromRequest($request); - if ($geolocation === null) { + if ($geolocation === null || $this->matchValue === null) { return false; } @@ -189,7 +193,7 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesGeolocationCityName(ServerRequestInterface $request): bool { $geolocation = geolocationFromRequest($request); - if ($geolocation === null) { + if ($geolocation === null || $this->matchValue === null) { return false; } diff --git a/module/Core/src/RedirectRule/Model/RedirectConditionType.php b/module/Core/src/RedirectRule/Model/RedirectConditionType.php index 8f7657e3..0461d968 100644 --- a/module/Core/src/RedirectRule/Model/RedirectConditionType.php +++ b/module/Core/src/RedirectRule/Model/RedirectConditionType.php @@ -31,11 +31,7 @@ enum RedirectConditionType: string // RedirectConditionType::LANGUAGE => TODO Validate at least format, RedirectConditionType::IP_ADDRESS => IpAddressUtils::isStaticIpCidrOrWildcard($value), RedirectConditionType::GEOLOCATION_COUNTRY_CODE => contains($value, ISO_COUNTRY_CODES), - RedirectConditionType::QUERY_PARAM, - RedirectConditionType::ANY_VALUE_QUERY_PARAM, - RedirectConditionType::VALUELESS_QUERY_PARAM => $value !== '', - // FIXME We should at least validate the value is not empty - // default => $value !== '', + RedirectConditionType::QUERY_PARAM => $value !== '', default => true, }; } diff --git a/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php b/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php index c802d75f..c1fb7fc7 100644 --- a/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php +++ b/module/Core/src/RedirectRule/Model/Validation/RedirectRulesInputFilter.php @@ -75,7 +75,7 @@ class RedirectRulesInputFilter extends InputFilter ])); $redirectConditionInputFilter->add($type); - $value = InputFactory::basic(self::CONDITION_MATCH_VALUE, required: true); + $value = InputFactory::basic(self::CONDITION_MATCH_VALUE, required: true)->setAllowEmpty(true); $value->getValidatorChain()->attach(new Callback( function (string $value, array $context): bool { $conditionType = RedirectConditionType::tryFrom($context[self::CONDITION_TYPE]); diff --git a/module/Core/test/RedirectRule/Model/RedirectConditionTypeTest.php b/module/Core/test/RedirectRule/Model/RedirectConditionTypeTest.php index 1c68a0f4..84d9f511 100644 --- a/module/Core/test/RedirectRule/Model/RedirectConditionTypeTest.php +++ b/module/Core/test/RedirectRule/Model/RedirectConditionTypeTest.php @@ -14,9 +14,9 @@ class RedirectConditionTypeTest extends TestCase #[Test] #[TestWith([RedirectConditionType::QUERY_PARAM, '', false])] #[TestWith([RedirectConditionType::QUERY_PARAM, 'foo', true])] - #[TestWith([RedirectConditionType::ANY_VALUE_QUERY_PARAM, '', false])] + #[TestWith([RedirectConditionType::ANY_VALUE_QUERY_PARAM, '', true])] #[TestWith([RedirectConditionType::ANY_VALUE_QUERY_PARAM, 'foo', true])] - #[TestWith([RedirectConditionType::VALUELESS_QUERY_PARAM, '', false])] + #[TestWith([RedirectConditionType::VALUELESS_QUERY_PARAM, '', true])] #[TestWith([RedirectConditionType::VALUELESS_QUERY_PARAM, 'foo', true])] public function isValidFailsForEmptyQueryParams( RedirectConditionType $conditionType, diff --git a/module/Rest/test-api/Action/ListRedirectRulesTest.php b/module/Rest/test-api/Action/ListRedirectRulesTest.php index b24caa7a..859e5ef3 100644 --- a/module/Rest/test-api/Action/ListRedirectRulesTest.php +++ b/module/Rest/test-api/Action/ListRedirectRulesTest.php @@ -17,11 +17,6 @@ class ListRedirectRulesTest extends ApiTestCase 'matchKey' => null, 'matchValue' => 'en', ]; - private const array QUERY_FOO_BAR_CONDITION = [ - 'type' => 'query-param', - 'matchKey' => 'foo', - 'matchValue' => 'bar', - ]; #[Test] public function errorIsReturnedWhenInvalidUrlIsFetched(): void @@ -45,7 +40,11 @@ class ListRedirectRulesTest extends ApiTestCase 'priority' => 1, 'conditions' => [ self::LANGUAGE_EN_CONDITION, - self::QUERY_FOO_BAR_CONDITION, + [ + 'type' => 'any-value-query-param', + 'matchKey' => 'foo', + 'matchValue' => null, + ], ], ], [ @@ -57,7 +56,11 @@ class ListRedirectRulesTest extends ApiTestCase 'matchKey' => 'hello', 'matchValue' => 'world', ], - self::QUERY_FOO_BAR_CONDITION, + [ + 'type' => 'query-param', + 'matchKey' => 'foo', + 'matchValue' => 'bar', + ], ], ], [ diff --git a/module/Rest/test-api/Action/SetRedirectRulesTest.php b/module/Rest/test-api/Action/SetRedirectRulesTest.php index 9fc6fa9d..a9ffa206 100644 --- a/module/Rest/test-api/Action/SetRedirectRulesTest.php +++ b/module/Rest/test-api/Action/SetRedirectRulesTest.php @@ -18,11 +18,6 @@ class SetRedirectRulesTest extends ApiTestCase 'matchKey' => null, 'matchValue' => 'en', ]; - private const array QUERY_FOO_BAR_CONDITION = [ - 'type' => 'query-param', - 'matchKey' => 'foo', - 'matchValue' => 'bar', - ]; #[Test] public function errorIsReturnedWhenInvalidUrlIsProvided(): void @@ -132,7 +127,11 @@ class SetRedirectRulesTest extends ApiTestCase 'priority' => 1, 'conditions' => [ self::LANGUAGE_EN_CONDITION, - self::QUERY_FOO_BAR_CONDITION, + [ + 'type' => 'any-value-query-param', + 'matchKey' => 'foo', + 'matchValue' => null, + ], ], ], [ @@ -144,7 +143,11 @@ class SetRedirectRulesTest extends ApiTestCase 'matchKey' => 'hello', 'matchValue' => 'world', ], - self::QUERY_FOO_BAR_CONDITION, + [ + 'type' => 'query-param', + 'matchKey' => 'foo', + 'matchValue' => 'bar', + ], ], ], ]])] diff --git a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php index 50b1dae5..2e963034 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php @@ -41,7 +41,7 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF priority: 1, longUrl: 'https://example.com/english-and-foo-query', conditions: new ArrayCollection( - [RedirectCondition::forLanguage('en'), RedirectCondition::forQueryParam('foo', 'bar')], + [RedirectCondition::forLanguage('en'), RedirectCondition::forAnyValueQueryParam('foo')], ), ); $manager->persist($englishAndFooQueryRule);