From d8ede3263fd6fd32e9ee43c7f5321fc3632ef274 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 2 Mar 2024 22:44:22 +0100 Subject: [PATCH 01/10] Implement command to manage redirect rules for a short URL --- config/autoload/rabbit.local.php.dist | 1 + config/test/test_config.global.php | 2 +- docker-compose.yml | 6 +- module/CLI/config/cli.config.php | 3 + module/CLI/config/dependencies.config.php | 8 + .../ManageRedirectRulesCommand.php | 272 ++++++++++++++++++ module/Core/functions/array-utils.php | 17 ++ .../RedirectRule/Entity/RedirectCondition.php | 14 + .../Entity/ShortUrlRedirectRule.php | 12 +- .../Model/RedirectConditionType.php | 2 +- .../ShortUrlRedirectRuleService.php | 40 +-- .../ShortUrlRedirectRuleServiceInterface.php | 5 + .../Model/Validation/ShortUrlInputFilter.php | 3 + .../test-api/Action/ListRedirectRulesTest.php | 4 +- .../test-api/Action/SetRedirectRulesTest.php | 4 +- 15 files changed, 365 insertions(+), 28 deletions(-) create mode 100644 module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php diff --git a/config/autoload/rabbit.local.php.dist b/config/autoload/rabbit.local.php.dist index 83cd4a88..b758528e 100644 --- a/config/autoload/rabbit.local.php.dist +++ b/config/autoload/rabbit.local.php.dist @@ -7,6 +7,7 @@ return [ 'rabbitmq' => [ 'enabled' => true, 'host' => 'shlink_rabbitmq', + 'port' => '5673', 'user' => 'rabbit', 'password' => 'rabbit', ], diff --git a/config/test/test_config.global.php b/config/test/test_config.global.php index 55f06bbf..aad5e9d0 100644 --- a/config/test/test_config.global.php +++ b/config/test/test_config.global.php @@ -52,7 +52,7 @@ $buildDbConnection = static function (): array { 'postgres' => [ 'driver' => 'pdo_pgsql', 'host' => $isCi ? '127.0.0.1' : 'shlink_db_postgres', - 'port' => $isCi ? '5433' : '5432', + 'port' => $isCi ? '5434' : '5432', 'user' => 'postgres', 'password' => 'root', 'dbname' => 'shlink_test', diff --git a/docker-compose.yml b/docker-compose.yml index 3f65e4bb..ccc5fc2d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -81,7 +81,7 @@ services: container_name: shlink_db_postgres image: postgres:12.2-alpine ports: - - "5433:5432" + - "5434:5432" volumes: - ./:/home/shlink/www - ./data/infra/database_pg:/var/lib/postgresql/data @@ -153,8 +153,8 @@ services: container_name: shlink_rabbitmq image: rabbitmq:3.11-management-alpine ports: - - "15672:15672" - - "5672:5672" + - "15673:15672" + - "5673:5672" environment: RABBITMQ_DEFAULT_USER: "rabbit" RABBITMQ_DEFAULT_PASS: "rabbit" diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index bcd4fd3c..94237c15 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -37,6 +37,9 @@ return [ Command\Db\CreateDatabaseCommand::NAME => Command\Db\CreateDatabaseCommand::class, Command\Db\MigrateDatabaseCommand::NAME => Command\Db\MigrateDatabaseCommand::class, + + Command\RedirectRule\ManageRedirectRulesCommand::NAME => + Command\RedirectRule\ManageRedirectRulesCommand::class, ], ], diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 2736a21e..c2b61e19 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Doctrine\NoDbNameConnectionFactory; use Shlinkio\Shlink\Core\Domain\DomainService; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; +use Shlinkio\Shlink\Core\RedirectRule; use Shlinkio\Shlink\Core\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\Tag\TagService; @@ -66,6 +67,8 @@ return [ Command\Domain\ListDomainsCommand::class => ConfigAbstractFactory::class, Command\Domain\DomainRedirectsCommand::class => ConfigAbstractFactory::class, Command\Domain\GetDomainVisitsCommand::class => ConfigAbstractFactory::class, + + Command\RedirectRule\ManageRedirectRulesCommand::class => ConfigAbstractFactory::class, ], ], @@ -117,6 +120,11 @@ return [ Command\Domain\DomainRedirectsCommand::class => [DomainService::class], Command\Domain\GetDomainVisitsCommand::class => [Visit\VisitsStatsHelper::class, ShortUrlStringifier::class], + Command\RedirectRule\ManageRedirectRulesCommand::class => [ + ShortUrl\ShortUrlResolver::class, + RedirectRule\ShortUrlRedirectRuleService::class, + ], + Command\Db\CreateDatabaseCommand::class => [ LockFactory::class, Util\ProcessRunner::class, diff --git a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php new file mode 100644 index 00000000..84741bbe --- /dev/null +++ b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php @@ -0,0 +1,272 @@ +setName(self::NAME) + ->setDescription('Set redirect rules for a short URL') + ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code which rules we want to set.') + ->addOption('domain', 'd', InputOption::VALUE_REQUIRED, 'The domain for the short code.'); + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $identifier = ShortUrlIdentifier::fromCli($input); + + try { + $shortUrl = $this->shortUrlResolver->resolveShortUrl($identifier); + } catch (ShortUrlNotFoundException) { + $io->error(sprintf('Short URL for %s not found', $identifier->__toString())); + return ExitCode::EXIT_FAILURE; + } + + $rulesToSave = $this->processRules($shortUrl, $io, $this->ruleService->rulesForShortUrl($shortUrl)); + if ($rulesToSave !== null) { + $this->ruleService->saveRulesForShortUrl($shortUrl, $rulesToSave); + } + + return ExitCode::EXIT_SUCCESS; + } + + /** + * @param ShortUrlRedirectRule[] $rules + * @return ShortUrlRedirectRule[]|null + */ + private function processRules(ShortUrl $shortUrl, SymfonyStyle $io, array $rules): ?array + { + $amountOfRules = count($rules); + + if ($amountOfRules === 0) { + $io->comment('No rules found.'); + } else { + $listing = map( + $rules, + function (ShortUrlRedirectRule $rule, string|int|float $index) use ($amountOfRules): array { + $priority = ((int) $index) + 1; + $conditions = $rule->mapConditions(static fn (RedirectCondition $condition): string => sprintf( + '%s', + $condition->toHumanFriendly(), + )); + + return [ + str_pad((string) $priority, strlen((string) $amountOfRules), '0', STR_PAD_LEFT), + implode(' AND ', $conditions), + $rule->longUrl, + ]; + }, + ); + $io->table(['Priority', 'Conditions', 'Redirect to'], $listing); + } + + $action = $io->choice( + 'What do you want to do next?', + [ + 'Add new rule', + 'Remove existing rule', + 'Re-arrange rule', + 'Discard changes', + 'Save and exit', + ], + 'Save and exit', + ); + + return match ($action) { + 'Add new rule' => $this->processRules($shortUrl, $io, $this->addRule($shortUrl, $io, $rules)), + 'Remove existing rule' => $this->processRules($shortUrl, $io, $this->removeRule($io, $rules)), + 'Re-arrange rule' => $this->processRules($shortUrl, $io, $this->reArrangeRule($io, $rules)), + 'Save and exit' => $rules, + default => null, + }; + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function addRule(ShortUrl $shortUrl, SymfonyStyle $io, array $currentRules): array + { + $higherPriority = count($currentRules); + $priority = $this->askPriority($io, $higherPriority + 1); + $longUrl = $this->askLongUrl($io); + $conditions = []; + + do { + $type = RedirectConditionType::from( + $io->choice('Type of the condition?', enumValues(RedirectConditionType::class)), + ); + $conditions[] = match ($type) { + RedirectConditionType::DEVICE => RedirectCondition::forDevice( + DeviceType::from($io->choice('Device to match?', enumValues(DeviceType::class))), + ), + RedirectConditionType::LANGUAGE => RedirectCondition::forLanguage( + $this->askMandatory('Language to match?', $io), + ), + RedirectConditionType::QUERY_PARAM => RedirectCondition::forQueryParam( + $this->askMandatory('Query param name?', $io), + $this->askOptional('Query param value?', $io), + ), + }; + + $continue = $io->confirm('Do you want to add another condition?'); + } while ($continue); + + $newRule = new ShortUrlRedirectRule($shortUrl, $priority, $longUrl, new ArrayCollection($conditions)); + $rulesBefore = array_slice($currentRules, 0, $priority - 1); + $rulesAfter = array_slice($currentRules, $priority - 1); + + return [...$rulesBefore, $newRule, ...$rulesAfter]; + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function removeRule(SymfonyStyle $io, array $currentRules): array + { + if (empty($currentRules)) { + $io->warning('There are no rules to remove'); + return $currentRules; + } + + $index = $this->askRule('What rule do you want to delete?', $io, $currentRules); + unset($currentRules[$index]); + return array_values($currentRules); + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function reArrangeRule(SymfonyStyle $io, array $currentRules): array + { + if (empty($currentRules)) { + $io->warning('There are no rules to re-arrange'); + return $currentRules; + } + + $oldIndex = $this->askRule('What rule do you want to re-arrange?', $io, $currentRules); + $newIndex = $this->askPriority($io, count($currentRules)) - 1; + + // Temporarily get rule from array and unset it + $rule = $currentRules[$oldIndex]; + unset($currentRules[$oldIndex]); + + // Reindex remaining rules + $currentRules = array_values($currentRules); + + $rulesBefore = array_slice($currentRules, 0, $newIndex); + $rulesAfter = array_slice($currentRules, $newIndex); + + return [...$rulesBefore, $rule, ...$rulesAfter]; + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function askRule(string $message, SymfonyStyle $io, array $currentRules): int + { + $choices = []; + foreach ($currentRules as $index => $rule) { + $choices[$rule->longUrl] = $index + 1; + } + + $resp = $io->choice($message, array_flip($choices)); + return $choices[$resp] - 1; + } + + private function askPriority(SymfonyStyle $io, int $max): int + { + return $io->ask( + 'Rule priority (the lower the value, the higher the priority)', + (string) $max, + function (string $answer) use ($max): int { + if (! is_numeric($answer)) { + throw new InvalidArgumentException('The priority must be a numeric positive value'); + } + + $priority = (int) $answer; + return max(1, min($max, $priority)); + }, + ); + } + + private function askLongUrl(SymfonyStyle $io): string + { + return $io->ask( + 'Long URL to redirect when the rule matches', + validator: function (string $answer): string { + $validator = ShortUrlInputFilter::longUrlValidators(); + if (! $validator->isValid($answer)) { + throw new InvalidArgumentException(implode(', ', $validator->getMessages())); + } + + return $answer; + }, + ); + } + + private function askMandatory(string $message, SymfonyStyle $io): string + { + return $io->ask($message, validator: function (?string $answer): string { + if ($answer === null) { + throw new InvalidArgumentException('The value is mandatory'); + } + return trim($answer); + }); + } + + private function askOptional(string $message, SymfonyStyle $io): string + { + return $io->ask($message, validator: fn (?string $answer) => $answer === null ? '' : trim($answer)); + } +} diff --git a/module/Core/functions/array-utils.php b/module/Core/functions/array-utils.php index 5fb636e6..7b9ca7e5 100644 --- a/module/Core/functions/array-utils.php +++ b/module/Core/functions/array-utils.php @@ -72,3 +72,20 @@ function select_keys(array $array, array $keys): array ARRAY_FILTER_USE_KEY, ); } + +/** + * @template T + * @template R + * @param iterable $collection + * @param callable(T $value, string|number $key): R $callback + * @return R[] + */ +function map(iterable $collection, callable $callback): array +{ + $aggregation = []; + foreach ($collection as $key => $value) { + $aggregation[$key] = $callback($value, $key); + } + + return $aggregation; +} diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 72cfdf49..29123733 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -13,6 +13,7 @@ use function Shlinkio\Shlink\Core\acceptLanguageToLocales; use function Shlinkio\Shlink\Core\ArrayUtils\some; use function Shlinkio\Shlink\Core\normalizeLocale; use function Shlinkio\Shlink\Core\splitLocale; +use function sprintf; use function strtolower; use function trim; @@ -107,4 +108,17 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable 'matchValue' => $this->matchValue, ]; } + + public function toHumanFriendly(): string + { + return match ($this->type) { + RedirectConditionType::DEVICE => sprintf('device is %s', $this->matchValue), + RedirectConditionType::LANGUAGE => sprintf('%s language is accepted', $this->matchValue), + RedirectConditionType::QUERY_PARAM => sprintf( + 'query string contains %s=%s', + $this->matchKey, + $this->matchValue, + ), + }; + } } diff --git a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php index 4469a620..57ad7092 100644 --- a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php +++ b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php @@ -15,7 +15,7 @@ use function Shlinkio\Shlink\Core\ArrayUtils\every; class ShortUrlRedirectRule extends AbstractEntity implements JsonSerializable { /** - * @param Collection $conditions + * @param Collection $conditions */ public function __construct( private readonly ShortUrl $shortUrl, // No need to read this field. It's used by doctrine @@ -41,6 +41,16 @@ class ShortUrlRedirectRule extends AbstractEntity implements JsonSerializable $this->conditions->clear(); } + /** + * @template R + * @param callable(RedirectCondition $condition): R $callback + * @return R[] + */ + public function mapConditions(callable $callback): array + { + return $this->conditions->map($callback(...))->toArray(); + } + public function jsonSerialize(): array { return [ diff --git a/module/Core/src/RedirectRule/Model/RedirectConditionType.php b/module/Core/src/RedirectRule/Model/RedirectConditionType.php index 51076068..c00cca7f 100644 --- a/module/Core/src/RedirectRule/Model/RedirectConditionType.php +++ b/module/Core/src/RedirectRule/Model/RedirectConditionType.php @@ -6,5 +6,5 @@ enum RedirectConditionType: string { case DEVICE = 'device'; case LANGUAGE = 'language'; - case QUERY_PARAM = 'query'; + case QUERY_PARAM = 'query-param'; } diff --git a/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php b/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php index 1a770ae9..40bbb0de 100644 --- a/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php +++ b/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php @@ -34,23 +34,6 @@ readonly class ShortUrlRedirectRuleService implements ShortUrlRedirectRuleServic */ public function setRulesForShortUrl(ShortUrl $shortUrl, RedirectRulesData $data): array { - return $this->em->wrapInTransaction(fn () => $this->doSetRulesForShortUrl($shortUrl, $data)); - } - - /** - * @return ShortUrlRedirectRule[] - */ - private function doSetRulesForShortUrl(ShortUrl $shortUrl, RedirectRulesData $data): array - { - // First, delete existing rules for the short URL - $oldRules = $this->rulesForShortUrl($shortUrl); - foreach ($oldRules as $oldRule) { - $oldRule->clearConditions(); // This will trigger the orphan removal of old conditions - $this->em->remove($oldRule); - } - $this->em->flush(); - - // Then insert new rules $rules = []; foreach ($data->rules as $index => $rule) { $rule = new ShortUrlRedirectRule( @@ -64,9 +47,30 @@ readonly class ShortUrlRedirectRuleService implements ShortUrlRedirectRuleServic ); $rules[] = $rule; - $this->em->persist($rule); } + $this->saveRulesForShortUrl($shortUrl, $rules); return $rules; } + + /** + * @param ShortUrlRedirectRule[] $rules + */ + public function saveRulesForShortUrl(ShortUrl $shortUrl, array $rules): void + { + $this->em->wrapInTransaction(function () use ($shortUrl, $rules): void { + // First, delete existing rules for the short URL + $oldRules = $this->rulesForShortUrl($shortUrl); + foreach ($oldRules as $oldRule) { + $oldRule->clearConditions(); // This will trigger the orphan removal of old conditions + $this->em->remove($oldRule); + } + $this->em->flush(); + + // Then insert new rules + foreach ($rules as $rule) { + $this->em->persist($rule); + } + }); + } } diff --git a/module/Core/src/RedirectRule/ShortUrlRedirectRuleServiceInterface.php b/module/Core/src/RedirectRule/ShortUrlRedirectRuleServiceInterface.php index 7fc34a1b..186be87e 100644 --- a/module/Core/src/RedirectRule/ShortUrlRedirectRuleServiceInterface.php +++ b/module/Core/src/RedirectRule/ShortUrlRedirectRuleServiceInterface.php @@ -17,4 +17,9 @@ interface ShortUrlRedirectRuleServiceInterface * @return ShortUrlRedirectRule[] */ public function setRulesForShortUrl(ShortUrl $shortUrl, RedirectRulesData $data): array; + + /** + * @param ShortUrlRedirectRule[] $rules + */ + public function saveRulesForShortUrl(ShortUrl $shortUrl, array $rules): void; } diff --git a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php index 22000e2c..e8d35284 100644 --- a/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/ShortUrl/Model/Validation/ShortUrlInputFilter.php @@ -124,6 +124,9 @@ class ShortUrlInputFilter extends InputFilter $this->add($apiKeyInput); } + /** + * @todo Extract to its own validator class + */ public static function longUrlValidators(bool $allowNull = false): Validator\ValidatorChain { $emptyModifiers = [ diff --git a/module/Rest/test-api/Action/ListRedirectRulesTest.php b/module/Rest/test-api/Action/ListRedirectRulesTest.php index b86683c9..c53986c1 100644 --- a/module/Rest/test-api/Action/ListRedirectRulesTest.php +++ b/module/Rest/test-api/Action/ListRedirectRulesTest.php @@ -18,7 +18,7 @@ class ListRedirectRulesTest extends ApiTestCase 'matchValue' => 'en', ]; private const QUERY_FOO_BAR_CONDITION = [ - 'type' => 'query', + 'type' => 'query-param', 'matchKey' => 'foo', 'matchValue' => 'bar', ]; @@ -53,7 +53,7 @@ class ListRedirectRulesTest extends ApiTestCase 'priority' => 2, 'conditions' => [ [ - 'type' => 'query', + 'type' => 'query-param', 'matchKey' => 'hello', 'matchValue' => 'world', ], diff --git a/module/Rest/test-api/Action/SetRedirectRulesTest.php b/module/Rest/test-api/Action/SetRedirectRulesTest.php index c70fd0ea..a1172d65 100644 --- a/module/Rest/test-api/Action/SetRedirectRulesTest.php +++ b/module/Rest/test-api/Action/SetRedirectRulesTest.php @@ -19,7 +19,7 @@ class SetRedirectRulesTest extends ApiTestCase 'matchValue' => 'en', ]; private const QUERY_FOO_BAR_CONDITION = [ - 'type' => 'query', + 'type' => 'query-param', 'matchKey' => 'foo', 'matchValue' => 'bar', ]; @@ -75,7 +75,7 @@ class SetRedirectRulesTest extends ApiTestCase 'priority' => 2, 'conditions' => [ [ - 'type' => 'query', + 'type' => 'query-param', 'matchKey' => 'hello', 'matchValue' => 'world', ], From 3bfb29a51c0b4e2cd2635a069cf96a7e9be3d8f4 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 08:47:31 +0100 Subject: [PATCH 02/10] Test new methods for RedirectCondition and ShortUrlRedirectRule --- .../Entity/ShortUrlRedirectRuleTest.php | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php b/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php index 19e431db..15df256f 100644 --- a/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php +++ b/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php @@ -6,9 +6,11 @@ use Doctrine\Common\Collections\ArrayCollection; use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\RedirectRule\Entity\RedirectCondition; use Shlinkio\Shlink\Core\RedirectRule\Entity\ShortUrlRedirectRule; +use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; class ShortUrlRedirectRuleTest extends TestCase @@ -51,6 +53,39 @@ class ShortUrlRedirectRuleTest extends TestCase self::assertEmpty($conditions); } + #[Test, DataProvider('provideConditionMappingCallbacks')] + public function conditionsCanBeMapped(callable $callback, array $expectedResult): void + { + $conditions = new ArrayCollection( + [RedirectCondition::forLanguage('en-UK'), RedirectCondition::forQueryParam('foo', 'bar')], + ); + $rule = $this->createRule($conditions); + + $result = $rule->mapConditions($callback); + + self::assertEquals($expectedResult, $result); + } + + public static function provideConditionMappingCallbacks(): iterable + { + yield 'json-serialized conditions' => [fn (RedirectCondition $cond) => $cond->jsonSerialize(), [ + [ + 'type' => RedirectConditionType::LANGUAGE->value, + 'matchKey' => null, + 'matchValue' => 'en-UK', + ], + [ + 'type' => RedirectConditionType::QUERY_PARAM->value, + 'matchKey' => 'foo', + 'matchValue' => 'bar', + ], + ]]; + yield 'human-friendly conditions' => [fn (RedirectCondition $cond) => $cond->toHumanFriendly(), [ + 'en-UK language is accepted', + 'query string contains foo=bar', + ]]; + } + /** * @param ArrayCollection $conditions */ From a843c59d779e62aea2d4ef0cb92acb751ff3d070 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 09:09:19 +0100 Subject: [PATCH 03/10] Fix inconsistencies when editing rules and saving a mix of new and old ones --- .../ManageRedirectRulesCommand.php | 1 + .../Entity/ShortUrlRedirectRule.php | 10 ++++++++++ .../ShortUrlRedirectRuleService.php | 20 ++++++++++++++++++- .../Entity/ShortUrlRedirectRuleTest.php | 1 - 4 files changed, 30 insertions(+), 2 deletions(-) diff --git a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php index 84741bbe..ee8dc328 100644 --- a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php +++ b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php @@ -76,6 +76,7 @@ class ManageRedirectRulesCommand extends Command $rulesToSave = $this->processRules($shortUrl, $io, $this->ruleService->rulesForShortUrl($shortUrl)); if ($rulesToSave !== null) { $this->ruleService->saveRulesForShortUrl($shortUrl, $rulesToSave); + $io->success('Rules properly saved'); } return ExitCode::EXIT_SUCCESS; diff --git a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php index 57ad7092..5f76d998 100644 --- a/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php +++ b/module/Core/src/RedirectRule/Entity/ShortUrlRedirectRule.php @@ -25,6 +25,16 @@ class ShortUrlRedirectRule extends AbstractEntity implements JsonSerializable ) { } + public function withPriority(int $newPriority): self + { + return new self( + $this->shortUrl, + $newPriority, + $this->longUrl, + $this->conditions, + ); + } + /** * Tells if this condition matches provided request */ diff --git a/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php b/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php index 40bbb0de..01ba0a8f 100644 --- a/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php +++ b/module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use function array_map; +use function Shlinkio\Shlink\Core\ArrayUtils\map; readonly class ShortUrlRedirectRuleService implements ShortUrlRedirectRuleServiceInterface { @@ -49,7 +50,7 @@ readonly class ShortUrlRedirectRuleService implements ShortUrlRedirectRuleServic $rules[] = $rule; } - $this->saveRulesForShortUrl($shortUrl, $rules); + $this->doSetRulesForShortUrl($shortUrl, $rules); return $rules; } @@ -57,6 +58,23 @@ readonly class ShortUrlRedirectRuleService implements ShortUrlRedirectRuleServic * @param ShortUrlRedirectRule[] $rules */ public function saveRulesForShortUrl(ShortUrl $shortUrl, array $rules): void + { + $normalizedAndDetachedRules = map($rules, function (ShortUrlRedirectRule $rule, int|string|float $priority) { + // Make sure all rules and conditions are detached so that the EM considers them new. + $rule->mapConditions(fn (RedirectCondition $cond) => $this->em->detach($cond)); + $this->em->detach($rule); + + // Normalize priorities so that they are sequential + return $rule->withPriority(((int) $priority) + 1); + }); + + $this->doSetRulesForShortUrl($shortUrl, $normalizedAndDetachedRules); + } + + /** + * @param ShortUrlRedirectRule[] $rules + */ + public function doSetRulesForShortUrl(ShortUrl $shortUrl, array $rules): void { $this->em->wrapInTransaction(function () use ($shortUrl, $rules): void { // First, delete existing rules for the short URL diff --git a/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php b/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php index 15df256f..d61bc6fa 100644 --- a/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php +++ b/module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php @@ -6,7 +6,6 @@ use Doctrine\Common\Collections\ArrayCollection; use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; -use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\RedirectRule\Entity\RedirectCondition; use Shlinkio\Shlink\Core\RedirectRule\Entity\ShortUrlRedirectRule; From a45550b0c60fba317e638cceaaeab2b65c5c35f0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 09:48:56 +0100 Subject: [PATCH 04/10] Extract logic to determine list of rules from ManageRedirectRulesCommand to a helper service --- module/CLI/config/dependencies.config.php | 6 +- .../ManageRedirectRulesCommand.php | 216 +---------------- module/CLI/src/Input/DateOption.php | 10 +- .../src/RedirectRule/RedirectRuleHandler.php | 221 ++++++++++++++++++ .../RedirectRuleHandlerInterface.php | 20 ++ 5 files changed, 251 insertions(+), 222 deletions(-) create mode 100644 module/CLI/src/RedirectRule/RedirectRuleHandler.php create mode 100644 module/CLI/src/RedirectRule/RedirectRuleHandlerInterface.php diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index c2b61e19..0c709788 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -11,7 +11,7 @@ use Shlinkio\Shlink\Common\Doctrine\NoDbNameConnectionFactory; use Shlinkio\Shlink\Core\Domain\DomainService; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; -use Shlinkio\Shlink\Core\RedirectRule; +use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectRuleService; use Shlinkio\Shlink\Core\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\Tag\TagService; @@ -34,6 +34,7 @@ return [ PhpExecutableFinder::class => InvokableFactory::class, GeoLite\GeolocationDbUpdater::class => ConfigAbstractFactory::class, + RedirectRule\RedirectRuleHandler::class => InvokableFactory::class, Util\ProcessRunner::class => ConfigAbstractFactory::class, ApiKey\RoleResolver::class => ConfigAbstractFactory::class, @@ -122,7 +123,8 @@ return [ Command\RedirectRule\ManageRedirectRulesCommand::class => [ ShortUrl\ShortUrlResolver::class, - RedirectRule\ShortUrlRedirectRuleService::class, + ShortUrlRedirectRuleService::class, + RedirectRule\RedirectRuleHandler::class, ], Command\Db\CreateDatabaseCommand::class => [ diff --git a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php index ee8dc328..e36fcf59 100644 --- a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php +++ b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php @@ -4,18 +4,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\RedirectRule; -use Doctrine\Common\Collections\ArrayCollection; +use Shlinkio\Shlink\CLI\RedirectRule\RedirectRuleHandlerInterface; use Shlinkio\Shlink\CLI\Util\ExitCode; -use Shlinkio\Shlink\Core\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; -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\Model\RedirectConditionType; use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectRuleServiceInterface; -use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; -use Shlinkio\Shlink\Core\ShortUrl\Model\Validation\ShortUrlInputFilter; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -24,22 +17,7 @@ use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; -use function array_flip; -use function array_slice; -use function array_values; -use function count; -use function implode; -use function is_numeric; -use function max; -use function min; -use function Shlinkio\Shlink\Core\ArrayUtils\map; -use function Shlinkio\Shlink\Core\enumValues; use function sprintf; -use function str_pad; -use function strlen; -use function trim; - -use const STR_PAD_LEFT; class ManageRedirectRulesCommand extends Command { @@ -48,6 +26,7 @@ class ManageRedirectRulesCommand extends Command public function __construct( protected readonly ShortUrlResolverInterface $shortUrlResolver, protected readonly ShortUrlRedirectRuleServiceInterface $ruleService, + protected readonly RedirectRuleHandlerInterface $ruleHandler, ) { parent::__construct(); } @@ -73,7 +52,7 @@ class ManageRedirectRulesCommand extends Command return ExitCode::EXIT_FAILURE; } - $rulesToSave = $this->processRules($shortUrl, $io, $this->ruleService->rulesForShortUrl($shortUrl)); + $rulesToSave = $this->ruleHandler->manageRules($io, $shortUrl, $this->ruleService->rulesForShortUrl($shortUrl)); if ($rulesToSave !== null) { $this->ruleService->saveRulesForShortUrl($shortUrl, $rulesToSave); $io->success('Rules properly saved'); @@ -81,193 +60,4 @@ class ManageRedirectRulesCommand extends Command return ExitCode::EXIT_SUCCESS; } - - /** - * @param ShortUrlRedirectRule[] $rules - * @return ShortUrlRedirectRule[]|null - */ - private function processRules(ShortUrl $shortUrl, SymfonyStyle $io, array $rules): ?array - { - $amountOfRules = count($rules); - - if ($amountOfRules === 0) { - $io->comment('No rules found.'); - } else { - $listing = map( - $rules, - function (ShortUrlRedirectRule $rule, string|int|float $index) use ($amountOfRules): array { - $priority = ((int) $index) + 1; - $conditions = $rule->mapConditions(static fn (RedirectCondition $condition): string => sprintf( - '%s', - $condition->toHumanFriendly(), - )); - - return [ - str_pad((string) $priority, strlen((string) $amountOfRules), '0', STR_PAD_LEFT), - implode(' AND ', $conditions), - $rule->longUrl, - ]; - }, - ); - $io->table(['Priority', 'Conditions', 'Redirect to'], $listing); - } - - $action = $io->choice( - 'What do you want to do next?', - [ - 'Add new rule', - 'Remove existing rule', - 'Re-arrange rule', - 'Discard changes', - 'Save and exit', - ], - 'Save and exit', - ); - - return match ($action) { - 'Add new rule' => $this->processRules($shortUrl, $io, $this->addRule($shortUrl, $io, $rules)), - 'Remove existing rule' => $this->processRules($shortUrl, $io, $this->removeRule($io, $rules)), - 'Re-arrange rule' => $this->processRules($shortUrl, $io, $this->reArrangeRule($io, $rules)), - 'Save and exit' => $rules, - default => null, - }; - } - - /** - * @param ShortUrlRedirectRule[] $currentRules - */ - private function addRule(ShortUrl $shortUrl, SymfonyStyle $io, array $currentRules): array - { - $higherPriority = count($currentRules); - $priority = $this->askPriority($io, $higherPriority + 1); - $longUrl = $this->askLongUrl($io); - $conditions = []; - - do { - $type = RedirectConditionType::from( - $io->choice('Type of the condition?', enumValues(RedirectConditionType::class)), - ); - $conditions[] = match ($type) { - RedirectConditionType::DEVICE => RedirectCondition::forDevice( - DeviceType::from($io->choice('Device to match?', enumValues(DeviceType::class))), - ), - RedirectConditionType::LANGUAGE => RedirectCondition::forLanguage( - $this->askMandatory('Language to match?', $io), - ), - RedirectConditionType::QUERY_PARAM => RedirectCondition::forQueryParam( - $this->askMandatory('Query param name?', $io), - $this->askOptional('Query param value?', $io), - ), - }; - - $continue = $io->confirm('Do you want to add another condition?'); - } while ($continue); - - $newRule = new ShortUrlRedirectRule($shortUrl, $priority, $longUrl, new ArrayCollection($conditions)); - $rulesBefore = array_slice($currentRules, 0, $priority - 1); - $rulesAfter = array_slice($currentRules, $priority - 1); - - return [...$rulesBefore, $newRule, ...$rulesAfter]; - } - - /** - * @param ShortUrlRedirectRule[] $currentRules - */ - private function removeRule(SymfonyStyle $io, array $currentRules): array - { - if (empty($currentRules)) { - $io->warning('There are no rules to remove'); - return $currentRules; - } - - $index = $this->askRule('What rule do you want to delete?', $io, $currentRules); - unset($currentRules[$index]); - return array_values($currentRules); - } - - /** - * @param ShortUrlRedirectRule[] $currentRules - */ - private function reArrangeRule(SymfonyStyle $io, array $currentRules): array - { - if (empty($currentRules)) { - $io->warning('There are no rules to re-arrange'); - return $currentRules; - } - - $oldIndex = $this->askRule('What rule do you want to re-arrange?', $io, $currentRules); - $newIndex = $this->askPriority($io, count($currentRules)) - 1; - - // Temporarily get rule from array and unset it - $rule = $currentRules[$oldIndex]; - unset($currentRules[$oldIndex]); - - // Reindex remaining rules - $currentRules = array_values($currentRules); - - $rulesBefore = array_slice($currentRules, 0, $newIndex); - $rulesAfter = array_slice($currentRules, $newIndex); - - return [...$rulesBefore, $rule, ...$rulesAfter]; - } - - /** - * @param ShortUrlRedirectRule[] $currentRules - */ - private function askRule(string $message, SymfonyStyle $io, array $currentRules): int - { - $choices = []; - foreach ($currentRules as $index => $rule) { - $choices[$rule->longUrl] = $index + 1; - } - - $resp = $io->choice($message, array_flip($choices)); - return $choices[$resp] - 1; - } - - private function askPriority(SymfonyStyle $io, int $max): int - { - return $io->ask( - 'Rule priority (the lower the value, the higher the priority)', - (string) $max, - function (string $answer) use ($max): int { - if (! is_numeric($answer)) { - throw new InvalidArgumentException('The priority must be a numeric positive value'); - } - - $priority = (int) $answer; - return max(1, min($max, $priority)); - }, - ); - } - - private function askLongUrl(SymfonyStyle $io): string - { - return $io->ask( - 'Long URL to redirect when the rule matches', - validator: function (string $answer): string { - $validator = ShortUrlInputFilter::longUrlValidators(); - if (! $validator->isValid($answer)) { - throw new InvalidArgumentException(implode(', ', $validator->getMessages())); - } - - return $answer; - }, - ); - } - - private function askMandatory(string $message, SymfonyStyle $io): string - { - return $io->ask($message, validator: function (?string $answer): string { - if ($answer === null) { - throw new InvalidArgumentException('The value is mandatory'); - } - return trim($answer); - }); - } - - private function askOptional(string $message, SymfonyStyle $io): string - { - return $io->ask($message, validator: fn (?string $answer) => $answer === null ? '' : trim($answer)); - } } diff --git a/module/CLI/src/Input/DateOption.php b/module/CLI/src/Input/DateOption.php index 41407d23..6183a6c5 100644 --- a/module/CLI/src/Input/DateOption.php +++ b/module/CLI/src/Input/DateOption.php @@ -14,14 +14,10 @@ use Throwable; use function is_string; use function sprintf; -class DateOption +readonly class DateOption { - public function __construct( - private readonly Command $command, - private readonly string $name, - string $shortcut, - string $description, - ) { + public function __construct(private Command $command, private string $name, string $shortcut, string $description) + { $command->addOption($name, $shortcut, InputOption::VALUE_REQUIRED, $description); } diff --git a/module/CLI/src/RedirectRule/RedirectRuleHandler.php b/module/CLI/src/RedirectRule/RedirectRuleHandler.php new file mode 100644 index 00000000..8b1592b7 --- /dev/null +++ b/module/CLI/src/RedirectRule/RedirectRuleHandler.php @@ -0,0 +1,221 @@ +newLine(); + $io->text('// No rules found.'); + } else { + $listing = map( + $rules, + function (ShortUrlRedirectRule $rule, string|int|float $index) use ($amountOfRules): array { + $priority = ((int) $index) + 1; + $conditions = $rule->mapConditions(static fn (RedirectCondition $condition): string => sprintf( + '%s', + $condition->toHumanFriendly(), + )); + + return [ + str_pad((string) $priority, strlen((string) $amountOfRules), '0', STR_PAD_LEFT), + implode(' AND ', $conditions), + $rule->longUrl, + ]; + }, + ); + $io->table(['Priority', 'Conditions', 'Redirect to'], $listing); + } + + $action = $io->choice( + 'What do you want to do next?', + [ + 'Add new rule', + 'Remove existing rule', + 'Re-arrange rule', + 'Discard changes', + 'Save and exit', + ], + 'Save and exit', + ); + + return match ($action) { + 'Add new rule' => $this->manageRules($io, $shortUrl, $this->addRule($shortUrl, $io, $rules)), + 'Remove existing rule' => $this->manageRules($io, $shortUrl, $this->removeRule($io, $rules)), + 'Re-arrange rule' => $this->manageRules($io, $shortUrl, $this->reArrangeRule($io, $rules)), + 'Save and exit' => $rules, + default => null, + }; + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function addRule(ShortUrl $shortUrl, StyleInterface $io, array $currentRules): array + { + $higherPriority = count($currentRules); + $priority = $this->askPriority($io, $higherPriority + 1); + $longUrl = $this->askLongUrl($io); + $conditions = []; + + do { + $type = RedirectConditionType::from( + $io->choice('Type of the condition?', enumValues(RedirectConditionType::class)), + ); + $conditions[] = match ($type) { + RedirectConditionType::DEVICE => RedirectCondition::forDevice( + DeviceType::from($io->choice('Device to match?', enumValues(DeviceType::class))), + ), + RedirectConditionType::LANGUAGE => RedirectCondition::forLanguage( + $this->askMandatory('Language to match?', $io), + ), + RedirectConditionType::QUERY_PARAM => RedirectCondition::forQueryParam( + $this->askMandatory('Query param name?', $io), + $this->askOptional('Query param value?', $io), + ), + }; + + $continue = $io->confirm('Do you want to add another condition?'); + } while ($continue); + + $newRule = new ShortUrlRedirectRule($shortUrl, $priority, $longUrl, new ArrayCollection($conditions)); + $rulesBefore = array_slice($currentRules, 0, $priority - 1); + $rulesAfter = array_slice($currentRules, $priority - 1); + + return [...$rulesBefore, $newRule, ...$rulesAfter]; + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function removeRule(StyleInterface $io, array $currentRules): array + { + if (empty($currentRules)) { + $io->warning('There are no rules to remove'); + return $currentRules; + } + + $index = $this->askRule('What rule do you want to delete?', $io, $currentRules); + unset($currentRules[$index]); + return array_values($currentRules); + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function reArrangeRule(StyleInterface $io, array $currentRules): array + { + if (empty($currentRules)) { + $io->warning('There are no rules to re-arrange'); + return $currentRules; + } + + $oldIndex = $this->askRule('What rule do you want to re-arrange?', $io, $currentRules); + $newIndex = $this->askPriority($io, count($currentRules)) - 1; + + // Temporarily get rule from array and unset it + $rule = $currentRules[$oldIndex]; + unset($currentRules[$oldIndex]); + + // Reindex remaining rules + $currentRules = array_values($currentRules); + + $rulesBefore = array_slice($currentRules, 0, $newIndex); + $rulesAfter = array_slice($currentRules, $newIndex); + + return [...$rulesBefore, $rule, ...$rulesAfter]; + } + + /** + * @param ShortUrlRedirectRule[] $currentRules + */ + private function askRule(string $message, StyleInterface $io, array $currentRules): int + { + $choices = []; + foreach ($currentRules as $index => $rule) { + $choices[$rule->longUrl] = $index + 1; + } + + $resp = $io->choice($message, array_flip($choices)); + return $choices[$resp] - 1; + } + + private function askPriority(StyleInterface $io, int $max): int + { + return $io->ask( + 'Rule priority (the lower the value, the higher the priority)', + (string) $max, + function (string $answer) use ($max): int { + if (! is_numeric($answer)) { + throw new InvalidArgumentException('The priority must be a numeric positive value'); + } + + $priority = (int) $answer; + return max(1, min($max, $priority)); + }, + ); + } + + private function askLongUrl(StyleInterface $io): string + { + return $io->ask( + 'Long URL to redirect when the rule matches', + validator: function (string $answer): string { + $validator = ShortUrlInputFilter::longUrlValidators(); + if (! $validator->isValid($answer)) { + throw new InvalidArgumentException(implode(', ', $validator->getMessages())); + } + + return $answer; + }, + ); + } + + private function askMandatory(string $message, StyleInterface $io): string + { + return $io->ask($message, validator: function (?string $answer): string { + if ($answer === null) { + throw new InvalidArgumentException('The value is mandatory'); + } + return trim($answer); + }); + } + + private function askOptional(string $message, StyleInterface $io): string + { + return $io->ask($message, validator: fn (?string $answer) => $answer === null ? '' : trim($answer)); + } +} diff --git a/module/CLI/src/RedirectRule/RedirectRuleHandlerInterface.php b/module/CLI/src/RedirectRule/RedirectRuleHandlerInterface.php new file mode 100644 index 00000000..16022768 --- /dev/null +++ b/module/CLI/src/RedirectRule/RedirectRuleHandlerInterface.php @@ -0,0 +1,20 @@ + Date: Sun, 3 Mar 2024 10:10:39 +0100 Subject: [PATCH 05/10] Reduce duplicated code when parsing short codes and domains from CLI --- .../ManageRedirectRulesCommand.php | 17 ++++++---- .../ShortUrl/DeleteShortUrlCommand.php | 18 +++++----- .../ShortUrl/DeleteShortUrlVisitsCommand.php | 26 ++++++-------- .../ShortUrl/GetShortUrlVisitsCommand.php | 19 ++++++----- .../Command/ShortUrl/ResolveUrlCommand.php | 19 ++++++----- module/CLI/src/Input/EndDateOption.php | 4 +-- .../CLI/src/Input/ShortUrlIdentifierInput.php | 34 +++++++++++++++++++ module/CLI/src/Input/StartDateOption.php | 4 +-- .../src/ShortUrl/Model/ShortUrlIdentifier.php | 13 ------- 9 files changed, 89 insertions(+), 65 deletions(-) create mode 100644 module/CLI/src/Input/ShortUrlIdentifierInput.php diff --git a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php index e36fcf59..13b6d1cc 100644 --- a/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php +++ b/module/CLI/src/Command/RedirectRule/ManageRedirectRulesCommand.php @@ -4,16 +4,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\RedirectRule; +use Shlinkio\Shlink\CLI\Input\ShortUrlIdentifierInput; use Shlinkio\Shlink\CLI\RedirectRule\RedirectRuleHandlerInterface; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\RedirectRule\ShortUrlRedirectRuleServiceInterface; -use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Symfony\Component\Console\Command\Command; -use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; -use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; @@ -23,27 +21,32 @@ class ManageRedirectRulesCommand extends Command { public const NAME = 'short-url:manage-rules'; + private readonly ShortUrlIdentifierInput $shortUrlIdentifierInput; + public function __construct( protected readonly ShortUrlResolverInterface $shortUrlResolver, protected readonly ShortUrlRedirectRuleServiceInterface $ruleService, protected readonly RedirectRuleHandlerInterface $ruleHandler, ) { parent::__construct(); + $this->shortUrlIdentifierInput = new ShortUrlIdentifierInput( + $this, + shortCodeDesc: 'The short code which rules we want to set.', + domainDesc: 'The domain for the short code.', + ); } protected function configure(): void { $this ->setName(self::NAME) - ->setDescription('Set redirect rules for a short URL') - ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code which rules we want to set.') - ->addOption('domain', 'd', InputOption::VALUE_REQUIRED, 'The domain for the short code.'); + ->setDescription('Set redirect rules for a short URL'); } protected function execute(InputInterface $input, OutputInterface $output): int { $io = new SymfonyStyle($input, $output); - $identifier = ShortUrlIdentifier::fromCli($input); + $identifier = $this->shortUrlIdentifierInput->toShortUrlIdentifier($input); try { $shortUrl = $this->shortUrlResolver->resolveShortUrl($identifier); diff --git a/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php b/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php index 8196bbfe..63e9dab5 100644 --- a/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/DeleteShortUrlCommand.php @@ -4,12 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; +use Shlinkio\Shlink\CLI\Input\ShortUrlIdentifierInput; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception; use Shlinkio\Shlink\Core\ShortUrl\DeleteShortUrlServiceInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Symfony\Component\Console\Command\Command; -use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; @@ -21,9 +21,16 @@ class DeleteShortUrlCommand extends Command { public const NAME = 'short-url:delete'; + private readonly ShortUrlIdentifierInput $shortUrlIdentifierInput; + public function __construct(private readonly DeleteShortUrlServiceInterface $deleteShortUrlService) { parent::__construct(); + $this->shortUrlIdentifierInput = new ShortUrlIdentifierInput( + $this, + shortCodeDesc: 'The short code for the short URL to be deleted', + domainDesc: 'The domain if the short code does not belong to the default one', + ); } protected function configure(): void @@ -31,26 +38,19 @@ class DeleteShortUrlCommand extends Command $this ->setName(self::NAME) ->setDescription('Deletes a short URL') - ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code for the short URL to be deleted') ->addOption( 'ignore-threshold', 'i', InputOption::VALUE_NONE, 'Ignores the safety visits threshold check, which could make short URLs with many visits to be ' . 'accidentally deleted', - ) - ->addOption( - 'domain', - 'd', - InputOption::VALUE_REQUIRED, - 'The domain if the short code does not belong to the default one', ); } protected function execute(InputInterface $input, OutputInterface $output): int { $io = new SymfonyStyle($input, $output); - $identifier = ShortUrlIdentifier::fromCli($input); + $identifier = $this->shortUrlIdentifierInput->toShortUrlIdentifier($input); $ignoreThreshold = $input->getOption('ignore-threshold'); try { diff --git a/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php b/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php index 5d122ea7..a720e12d 100644 --- a/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/DeleteShortUrlVisitsCommand.php @@ -5,13 +5,11 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; use Shlinkio\Shlink\CLI\Command\Visit\AbstractDeleteVisitsCommand; +use Shlinkio\Shlink\CLI\Input\ShortUrlIdentifierInput; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; -use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlVisitsDeleterInterface; -use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; -use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Style\SymfonyStyle; use function sprintf; @@ -20,32 +18,28 @@ class DeleteShortUrlVisitsCommand extends AbstractDeleteVisitsCommand { public const NAME = 'short-url:visits-delete'; + private readonly ShortUrlIdentifierInput $shortUrlIdentifierInput; + public function __construct(private readonly ShortUrlVisitsDeleterInterface $deleter) { parent::__construct(); + $this->shortUrlIdentifierInput = new ShortUrlIdentifierInput( + $this, + shortCodeDesc: 'The short code for the short URL which visits will be deleted', + domainDesc: 'The domain if the short code does not belong to the default one', + ); } protected function configure(): void { $this ->setName(self::NAME) - ->setDescription('Deletes visits from a short URL') - ->addArgument( - 'shortCode', - InputArgument::REQUIRED, - 'The short code for the short URL which visits will be deleted', - ) - ->addOption( - 'domain', - 'd', - InputOption::VALUE_REQUIRED, - 'The domain if the short code does not belong to the default one', - ); + ->setDescription('Deletes visits from a short URL'); } protected function doExecute(InputInterface $input, SymfonyStyle $io): int { - $identifier = ShortUrlIdentifier::fromCli($input); + $identifier = $this->shortUrlIdentifierInput->toShortUrlIdentifier($input); try { $result = $this->deleter->deleteShortUrlVisits($identifier); $io->success(sprintf('Successfully deleted %s visits', $result->affectedItems)); diff --git a/module/CLI/src/Command/ShortUrl/GetShortUrlVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetShortUrlVisitsCommand.php index a6a4f31d..8a662209 100644 --- a/module/CLI/src/Command/ShortUrl/GetShortUrlVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetShortUrlVisitsCommand.php @@ -5,14 +5,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; use Shlinkio\Shlink\CLI\Command\Visit\AbstractVisitsListCommand; +use Shlinkio\Shlink\CLI\Input\ShortUrlIdentifierInput; use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Common\Util\DateRange; -use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; -use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; -use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; @@ -20,18 +18,23 @@ class GetShortUrlVisitsCommand extends AbstractVisitsListCommand { public const NAME = 'short-url:visits'; + private ShortUrlIdentifierInput $shortUrlIdentifierInput; + protected function configure(): void { $this ->setName(self::NAME) - ->setDescription('Returns the detailed visits information for provided short code') - ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code which visits we want to get.') - ->addOption('domain', 'd', InputOption::VALUE_REQUIRED, 'The domain for the short code.'); + ->setDescription('Returns the detailed visits information for provided short code'); + $this->shortUrlIdentifierInput = new ShortUrlIdentifierInput( + $this, + shortCodeDesc: 'The short code which visits we want to get.', + domainDesc: 'The domain for the short code.', + ); } protected function interact(InputInterface $input, OutputInterface $output): void { - $shortCode = $input->getArgument('shortCode'); + $shortCode = $this->shortUrlIdentifierInput->shortCode($input); if (! empty($shortCode)) { return; } @@ -45,7 +48,7 @@ class GetShortUrlVisitsCommand extends AbstractVisitsListCommand protected function getVisitsPaginator(InputInterface $input, DateRange $dateRange): Paginator { - $identifier = ShortUrlIdentifier::fromCli($input); + $identifier = $this->shortUrlIdentifierInput->toShortUrlIdentifier($input); return $this->visitsHelper->visitsForShortUrl($identifier, new VisitsParams($dateRange)); } diff --git a/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php b/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php index d41d292e..0a207b68 100644 --- a/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php +++ b/module/CLI/src/Command/ShortUrl/ResolveUrlCommand.php @@ -4,14 +4,12 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Command\ShortUrl; +use Shlinkio\Shlink\CLI\Input\ShortUrlIdentifierInput; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; -use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Symfony\Component\Console\Command\Command; -use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; -use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; @@ -21,23 +19,28 @@ class ResolveUrlCommand extends Command { public const NAME = 'short-url:parse'; + private readonly ShortUrlIdentifierInput $shortUrlIdentifierInput; + public function __construct(private readonly ShortUrlResolverInterface $urlResolver) { parent::__construct(); + $this->shortUrlIdentifierInput = new ShortUrlIdentifierInput( + $this, + shortCodeDesc: 'The short code to parse', + domainDesc: 'The domain to which the short URL is attached.', + ); } protected function configure(): void { $this ->setName(self::NAME) - ->setDescription('Returns the long URL behind a short code') - ->addArgument('shortCode', InputArgument::REQUIRED, 'The short code to parse') - ->addOption('domain', 'd', InputOption::VALUE_REQUIRED, 'The domain to which the short URL is attached.'); + ->setDescription('Returns the long URL behind a short code'); } protected function interact(InputInterface $input, OutputInterface $output): void { - $shortCode = $input->getArgument('shortCode'); + $shortCode = $this->shortUrlIdentifierInput->shortCode($input); if (! empty($shortCode)) { return; } @@ -54,7 +57,7 @@ class ResolveUrlCommand extends Command $io = new SymfonyStyle($input, $output); try { - $url = $this->urlResolver->resolveShortUrl(ShortUrlIdentifier::fromCli($input)); + $url = $this->urlResolver->resolveShortUrl($this->shortUrlIdentifierInput->toShortUrlIdentifier($input)); $output->writeln(sprintf('Long URL: %s', $url->getLongUrl())); return ExitCode::EXIT_SUCCESS; } catch (ShortUrlNotFoundException $e) { diff --git a/module/CLI/src/Input/EndDateOption.php b/module/CLI/src/Input/EndDateOption.php index 000a135e..8e6df28a 100644 --- a/module/CLI/src/Input/EndDateOption.php +++ b/module/CLI/src/Input/EndDateOption.php @@ -11,9 +11,9 @@ use Symfony\Component\Console\Output\OutputInterface; use function sprintf; -class EndDateOption +readonly final class EndDateOption { - private readonly DateOption $dateOption; + private DateOption $dateOption; public function __construct(Command $command, string $descriptionHint) { diff --git a/module/CLI/src/Input/ShortUrlIdentifierInput.php b/module/CLI/src/Input/ShortUrlIdentifierInput.php new file mode 100644 index 00000000..c07de779 --- /dev/null +++ b/module/CLI/src/Input/ShortUrlIdentifierInput.php @@ -0,0 +1,34 @@ +addArgument('shortCode', InputArgument::REQUIRED, $shortCodeDesc) + ->addOption('domain', 'd', InputOption::VALUE_REQUIRED, $domainDesc); + } + + public function shortCode(InputInterface $input): ?string + { + return $input->getArgument('shortCode'); + } + + public function toShortUrlIdentifier(InputInterface $input): ShortUrlIdentifier + { + $shortCode = $input->getArgument('shortCode'); + $domain = $input->getOption('domain'); + + return ShortUrlIdentifier::fromShortCodeAndDomain($shortCode, $domain); + } +} diff --git a/module/CLI/src/Input/StartDateOption.php b/module/CLI/src/Input/StartDateOption.php index 0954e82f..6a7857d7 100644 --- a/module/CLI/src/Input/StartDateOption.php +++ b/module/CLI/src/Input/StartDateOption.php @@ -11,9 +11,9 @@ use Symfony\Component\Console\Output\OutputInterface; use function sprintf; -class StartDateOption +readonly final class StartDateOption { - private readonly DateOption $dateOption; + private DateOption $dateOption; public function __construct(Command $command, string $descriptionHint) { diff --git a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php index 7ec19df6..a7c2e2ff 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php @@ -6,7 +6,6 @@ namespace Shlinkio\Shlink\Core\ShortUrl\Model; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; -use Symfony\Component\Console\Input\InputInterface; use function sprintf; @@ -32,18 +31,6 @@ final readonly class ShortUrlIdentifier return new self($shortCode, $domain); } - public static function fromCli(InputInterface $input): self - { - // Using getArguments and getOptions instead of getArgument(...) and getOption(...) because - // the later throw an exception if requested options are not defined - /** @var string $shortCode */ - $shortCode = $input->getArguments()['shortCode'] ?? ''; - /** @var string|null $domain */ - $domain = $input->getOptions()['domain'] ?? null; - - return new self($shortCode, $domain); - } - public static function fromShortUrl(ShortUrl $shortUrl): self { $domain = $shortUrl->getDomain(); From c9d1a955b997a6606bee94943719fcf8cd2d15a8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 10:27:21 +0100 Subject: [PATCH 06/10] Add ManageRedirectRulesCommand unit test --- .../ManageRedirectRulesCommandTest.php | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 module/CLI/test/Command/RedirectRule/ManageRedirectRulesCommandTest.php diff --git a/module/CLI/test/Command/RedirectRule/ManageRedirectRulesCommandTest.php b/module/CLI/test/Command/RedirectRule/ManageRedirectRulesCommandTest.php new file mode 100644 index 00000000..79859d23 --- /dev/null +++ b/module/CLI/test/Command/RedirectRule/ManageRedirectRulesCommandTest.php @@ -0,0 +1,95 @@ +shortUrlResolver = $this->createMock(ShortUrlResolverInterface::class); + $this->ruleService = $this->createMock(ShortUrlRedirectRuleServiceInterface::class); + $this->ruleHandler = $this->createMock(RedirectRuleHandlerInterface::class); + + $this->commandTester = CliTestUtils::testerForCommand(new ManageRedirectRulesCommand( + $this->shortUrlResolver, + $this->ruleService, + $this->ruleHandler, + )); + } + + #[Test] + public function errorIsReturnedIfShortUrlCannotBeFound(): void + { + $this->shortUrlResolver->expects($this->once())->method('resolveShortUrl')->with( + ShortUrlIdentifier::fromShortCodeAndDomain('foo'), + )->willThrowException(new ShortUrlNotFoundException('')); + $this->ruleService->expects($this->never())->method('rulesForShortUrl'); + $this->ruleService->expects($this->never())->method('saveRulesForShortUrl'); + $this->ruleHandler->expects($this->never())->method('manageRules'); + + $exitCode = $this->commandTester->execute(['shortCode' => 'foo']); + $output = $this->commandTester->getDisplay(); + + self::assertEquals(ExitCode::EXIT_FAILURE, $exitCode); + self::assertStringContainsString('Short URL for foo not found', $output); + } + + #[Test] + public function savesNoRulesIfManageResultIsNull(): void + { + $shortUrl = ShortUrl::withLongUrl('https://example.com'); + + $this->shortUrlResolver->expects($this->once())->method('resolveShortUrl')->with( + ShortUrlIdentifier::fromShortCodeAndDomain('foo'), + )->willReturn($shortUrl); + $this->ruleService->expects($this->once())->method('rulesForShortUrl')->with($shortUrl)->willReturn([]); + $this->ruleHandler->expects($this->once())->method('manageRules')->willReturn(null); + $this->ruleService->expects($this->never())->method('saveRulesForShortUrl'); + + $exitCode = $this->commandTester->execute(['shortCode' => 'foo']); + $output = $this->commandTester->getDisplay(); + + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); + self::assertStringNotContainsString('Rules properly saved', $output); + } + + #[Test] + public function savesRulesIfManageResultIsAnArray(): void + { + $shortUrl = ShortUrl::withLongUrl('https://example.com'); + + $this->shortUrlResolver->expects($this->once())->method('resolveShortUrl')->with( + ShortUrlIdentifier::fromShortCodeAndDomain('foo'), + )->willReturn($shortUrl); + $this->ruleService->expects($this->once())->method('rulesForShortUrl')->with($shortUrl)->willReturn([]); + $this->ruleHandler->expects($this->once())->method('manageRules')->willReturn([]); + $this->ruleService->expects($this->once())->method('saveRulesForShortUrl')->with($shortUrl, []); + + $exitCode = $this->commandTester->execute(['shortCode' => 'foo']); + $output = $this->commandTester->getDisplay(); + + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); + self::assertStringContainsString('Rules properly saved', $output); + } +} From eb40dc2d5d1ba9dcb7e85f3702026163da3f18d3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 10:36:17 +0100 Subject: [PATCH 07/10] Add unit test for ShortUrlRedirectRuleService::saveRulesForShortUrl --- .../ShortUrlRedirectRuleServiceTest.php | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/module/Core/test/RedirectRule/ShortUrlRedirectRuleServiceTest.php b/module/Core/test/RedirectRule/ShortUrlRedirectRuleServiceTest.php index b0b6d4f2..103c6fd0 100644 --- a/module/Core/test/RedirectRule/ShortUrlRedirectRuleServiceTest.php +++ b/module/Core/test/RedirectRule/ShortUrlRedirectRuleServiceTest.php @@ -132,4 +132,40 @@ class ShortUrlRedirectRuleServiceTest extends TestCase self::assertCount(0, $result); } + + #[Test] + public function saveRulesForShortUrlDetachesAllEntitiesAndArrangesPriorities(): void + { + $shortUrl = ShortUrl::withLongUrl('https://example.com'); + $rules = [ + new ShortUrlRedirectRule($shortUrl, 8, 'https://example.com', new ArrayCollection([ + RedirectCondition::forLanguage('es-ES'), + RedirectCondition::forDevice(DeviceType::ANDROID), + ])), + new ShortUrlRedirectRule($shortUrl, 3, 'https://example.com', new ArrayCollection([ + RedirectCondition::forQueryParam('foo', 'bar'), + RedirectCondition::forQueryParam('bar', 'foo'), + ])), + new ShortUrlRedirectRule($shortUrl, 15, 'https://example.com', new ArrayCollection([ + RedirectCondition::forDevice(DeviceType::IOS), + ])), + ]; + + // Detach will be called 8 times: 3 rules + 5 conditions + $this->em->expects($this->exactly(8))->method('detach'); + $this->em->expects($this->once())->method('wrapInTransaction')->willReturnCallback( + fn (callable $callback) => $callback(), + ); + + // Persist will be called for each of the three rules. Their priorities should be consecutive starting at 1 + $cont = 0; + $this->em->expects($this->exactly(3))->method('persist')->with($this->callback( + function (ShortUrlRedirectRule $rule) use (&$cont): bool { + $cont++; + return $rule->jsonSerialize()['priority'] === $cont; + }, + )); + + $this->ruleService->saveRulesForShortUrl($shortUrl, $rules); + } } From 8751d6c31525c0c07ec7f8fdb0851600bdd82936 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 12:51:17 +0100 Subject: [PATCH 08/10] Add unit test for RedirectRuleHandler --- .../src/RedirectRule/RedirectRuleHandler.php | 32 +-- .../RedirectRuleHandlerAction.php | 12 + .../RedirectRule/RedirectRuleHandlerTest.php | 252 ++++++++++++++++++ 3 files changed, 281 insertions(+), 15 deletions(-) create mode 100644 module/CLI/src/RedirectRule/RedirectRuleHandlerAction.php create mode 100644 module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php diff --git a/module/CLI/src/RedirectRule/RedirectRuleHandler.php b/module/CLI/src/RedirectRule/RedirectRuleHandler.php index 8b1592b7..7470d3b5 100644 --- a/module/CLI/src/RedirectRule/RedirectRuleHandler.php +++ b/module/CLI/src/RedirectRule/RedirectRuleHandler.php @@ -60,24 +60,26 @@ class RedirectRuleHandler implements RedirectRuleHandlerInterface $io->table(['Priority', 'Conditions', 'Redirect to'], $listing); } - $action = $io->choice( + $action = RedirectRuleHandlerAction::from($io->choice( 'What do you want to do next?', - [ - 'Add new rule', - 'Remove existing rule', - 'Re-arrange rule', - 'Discard changes', - 'Save and exit', - ], - 'Save and exit', - ); + enumValues(RedirectRuleHandlerAction::class), + RedirectRuleHandlerAction::SAVE->value, + )); return match ($action) { - 'Add new rule' => $this->manageRules($io, $shortUrl, $this->addRule($shortUrl, $io, $rules)), - 'Remove existing rule' => $this->manageRules($io, $shortUrl, $this->removeRule($io, $rules)), - 'Re-arrange rule' => $this->manageRules($io, $shortUrl, $this->reArrangeRule($io, $rules)), - 'Save and exit' => $rules, - default => null, + RedirectRuleHandlerAction::ADD => $this->manageRules( + $io, + $shortUrl, + $this->addRule($shortUrl, $io, $rules), + ), + RedirectRuleHandlerAction::REMOVE => $this->manageRules($io, $shortUrl, $this->removeRule($io, $rules)), + RedirectRuleHandlerAction::RE_ARRANGE => $this->manageRules( + $io, + $shortUrl, + $this->reArrangeRule($io, $rules), + ), + RedirectRuleHandlerAction::SAVE => $rules, + RedirectRuleHandlerAction::DISCARD => null, }; } diff --git a/module/CLI/src/RedirectRule/RedirectRuleHandlerAction.php b/module/CLI/src/RedirectRule/RedirectRuleHandlerAction.php new file mode 100644 index 00000000..a3aff06d --- /dev/null +++ b/module/CLI/src/RedirectRule/RedirectRuleHandlerAction.php @@ -0,0 +1,12 @@ +io = $this->createMock(StyleInterface::class); + $this->shortUrl = ShortUrl::withLongUrl('https://example.com'); + $this->cond1 = RedirectCondition::forLanguage('es-AR'); + $this->cond2 = RedirectCondition::forQueryParam('foo', 'bar'); + $this->cond3 = RedirectCondition::forDevice(DeviceType::ANDROID); + $this->rules = [ + new ShortUrlRedirectRule($this->shortUrl, 3, 'https://example.com/one', new ArrayCollection( + [$this->cond1], + )), + new ShortUrlRedirectRule($this->shortUrl, 8, 'https://example.com/two', new ArrayCollection( + [$this->cond2, $this->cond3], + )), + new ShortUrlRedirectRule($this->shortUrl, 5, 'https://example.com/three', new ArrayCollection( + [$this->cond1, $this->cond3], + )), + ]; + + $this->handler = new RedirectRuleHandler(); + } + + #[Test, DataProvider('provideExitActions')] + public function commentIsDisplayedWhenRulesListIsEmpty( + RedirectRuleHandlerAction $action, + ?array $expectedResult, + ): void { + $this->io->expects($this->once())->method('choice')->willReturn($action->value); + $this->io->expects($this->once())->method('newLine'); + $this->io->expects($this->once())->method('text')->with('// No rules found.'); + $this->io->expects($this->never())->method('table'); + + $result = $this->handler->manageRules($this->io, $this->shortUrl, []); + + self::assertEquals($expectedResult, $result); + } + + #[Test, DataProvider('provideExitActions')] + public function rulesAreDisplayedWhenRulesListIsEmpty( + RedirectRuleHandlerAction $action, + ): void { + $comment = fn (string $value) => sprintf('%s', $value); + + $this->io->expects($this->once())->method('choice')->willReturn($action->value); + $this->io->expects($this->never())->method('newLine'); + $this->io->expects($this->never())->method('text'); + $this->io->expects($this->once())->method('table')->with($this->isType('array'), [ + ['1', $comment($this->cond1->toHumanFriendly()), 'https://example.com/one'], + [ + '2', + $comment($this->cond2->toHumanFriendly()) . ' AND ' . $comment($this->cond3->toHumanFriendly()), + 'https://example.com/two', + ], + [ + '3', + $comment($this->cond1->toHumanFriendly()) . ' AND ' . $comment($this->cond3->toHumanFriendly()), + 'https://example.com/three', + ], + ]); + + $this->handler->manageRules($this->io, $this->shortUrl, $this->rules); + } + + public static function provideExitActions(): iterable + { + yield 'discard' => [RedirectRuleHandlerAction::DISCARD, null]; + yield 'save' => [RedirectRuleHandlerAction::SAVE, []]; + } + + #[Test, DataProvider('provideDeviceConditions')] + /** + * @param RedirectCondition[] $expectedConditions + */ + public function newRulesCanBeAdded( + RedirectConditionType $type, + array $expectedConditions, + bool $continue = false, + ): void { + $this->io->expects($this->any())->method('ask')->willReturnCallback( + fn (string $message): string|int => match ($message) { + 'Rule priority (the lower the value, the higher the priority)' => 2, // Add in between existing rules + 'Long URL to redirect when the rule matches' => 'https://example.com/new-two', + 'Language to match?' => 'en-US', + 'Query param name?' => 'foo', + 'Query param value?' => 'bar', + default => '', + }, + ); + $this->io->expects($this->any())->method('choice')->willReturnCallback( + function (string $message) use (&$callIndex, $type): string { + $callIndex++; + + if ($message === 'Type of the condition?') { + return $type->value; + } elseif ($message === 'Device to match?') { + return DeviceType::ANDROID->value; + } + + // First we select remove action to trigger code branch, then save to finish execution + $action = $callIndex === 1 ? RedirectRuleHandlerAction::ADD : RedirectRuleHandlerAction::SAVE; + return $action->value; + }, + ); + + $continueCallCount = 0; + $this->io->method('confirm')->willReturnCallback(function () use (&$continueCallCount, $continue) { + $continueCallCount++; + return $continueCallCount < 2 && $continue; + }); + + $result = $this->handler->manageRules($this->io, $this->shortUrl, $this->rules); + + self::assertEquals([ + $this->rules[0], + new ShortUrlRedirectRule($this->shortUrl, 2, 'https://example.com/new-two', new ArrayCollection( + $expectedConditions, + )), + $this->rules[1], + $this->rules[2], + ], $result); + } + + public static function provideDeviceConditions(): iterable + { + yield 'device' => [RedirectConditionType::DEVICE, [RedirectCondition::forDevice(DeviceType::ANDROID)]]; + yield 'language' => [RedirectConditionType::LANGUAGE, [RedirectCondition::forLanguage('en-US')]]; + yield 'query param' => [RedirectConditionType::QUERY_PARAM, [RedirectCondition::forQueryParam('foo', 'bar')]]; + yield 'multiple query params' => [ + RedirectConditionType::QUERY_PARAM, + [RedirectCondition::forQueryParam('foo', 'bar'), RedirectCondition::forQueryParam('foo', 'bar')], + true, + ]; + } + + #[Test] + public function existingRulesCanBeRemoved(): void + { + $callIndex = 0; + $this->io->expects($this->exactly(3))->method('choice')->willReturnCallback( + function (string $message) use (&$callIndex): string { + $callIndex++; + + if ($message === 'What rule do you want to delete?') { + return 'https://example.com/two'; // Second rule to be removed + } + + // First we select remove action to trigger code branch, then save to finish execution + $action = $callIndex === 1 ? RedirectRuleHandlerAction::REMOVE : RedirectRuleHandlerAction::SAVE; + return $action->value; + }, + ); + $this->io->expects($this->never())->method('warning'); + + $result = $this->handler->manageRules($this->io, $this->shortUrl, $this->rules); + + self::assertEquals([$this->rules[0], $this->rules[2]], $result); + } + + #[Test] + public function warningIsPrintedWhenTryingToRemoveRuleFromEmptyList(): void + { + $callIndex = 0; + $this->io->expects($this->exactly(2))->method('choice')->willReturnCallback( + function () use (&$callIndex): string { + $callIndex++; + $action = $callIndex === 1 ? RedirectRuleHandlerAction::REMOVE : RedirectRuleHandlerAction::DISCARD; + return $action->value; + }, + ); + $this->io->expects($this->once())->method('warning')->with('There are no rules to remove'); + + $this->handler->manageRules($this->io, $this->shortUrl, []); + } + + #[Test] + public function existingRulesCanBeReArranged(): void + { + $this->io->expects($this->any())->method('ask')->willReturnCallback( + fn (string $message): string|int => match ($message) { + 'Rule priority (the lower the value, the higher the priority)' => 1, + default => '', + }, + ); + $this->io->expects($this->exactly(3))->method('choice')->willReturnCallback( + function (string $message) use (&$callIndex): string { + $callIndex++; + + if ($message === 'What rule do you want to re-arrange?') { + return 'https://example.com/two'; // Second rule to be re-arrange + } + + // First we select remove action to trigger code branch, then save to finish execution + $action = $callIndex === 1 ? RedirectRuleHandlerAction::RE_ARRANGE : RedirectRuleHandlerAction::SAVE; + return $action->value; + }, + ); + $this->io->expects($this->never())->method('warning'); + + $result = $this->handler->manageRules($this->io, $this->shortUrl, $this->rules); + + self::assertEquals([$this->rules[1], $this->rules[0], $this->rules[2]], $result); + } + + #[Test] + public function warningIsPrintedWhenTryingToReArrangeRuleFromEmptyList(): void + { + $callIndex = 0; + $this->io->expects($this->exactly(2))->method('choice')->willReturnCallback( + function () use (&$callIndex): string { + $callIndex++; + $action = $callIndex === 1 ? RedirectRuleHandlerAction::RE_ARRANGE : RedirectRuleHandlerAction::DISCARD; + return $action->value; + }, + ); + $this->io->expects($this->once())->method('warning')->with('There are no rules to re-arrange'); + + $this->handler->manageRules($this->io, $this->shortUrl, []); + } +} From 63c533fa62359beacd9a5e469d9592fd7c3d7429 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 12:59:58 +0100 Subject: [PATCH 09/10] Fix incorrect rule selection when deleting rules with same long URL --- module/CLI/src/RedirectRule/RedirectRuleHandler.php | 4 +++- module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/module/CLI/src/RedirectRule/RedirectRuleHandler.php b/module/CLI/src/RedirectRule/RedirectRuleHandler.php index 7470d3b5..068cdc74 100644 --- a/module/CLI/src/RedirectRule/RedirectRuleHandler.php +++ b/module/CLI/src/RedirectRule/RedirectRuleHandler.php @@ -168,7 +168,9 @@ class RedirectRuleHandler implements RedirectRuleHandlerInterface { $choices = []; foreach ($currentRules as $index => $rule) { - $choices[$rule->longUrl] = $index + 1; + $priority = $index + 1; + $key = sprintf('%s - %s', $priority, $rule->longUrl); + $choices[$key] = $priority; } $resp = $io->choice($message, array_flip($choices)); diff --git a/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php b/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php index df448a8d..0c0b7d12 100644 --- a/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php +++ b/module/CLI/test/RedirectRule/RedirectRuleHandlerTest.php @@ -174,7 +174,7 @@ class RedirectRuleHandlerTest extends TestCase $callIndex++; if ($message === 'What rule do you want to delete?') { - return 'https://example.com/two'; // Second rule to be removed + return '2 - https://example.com/two'; // Second rule to be removed } // First we select remove action to trigger code branch, then save to finish execution @@ -219,7 +219,7 @@ class RedirectRuleHandlerTest extends TestCase $callIndex++; if ($message === 'What rule do you want to re-arrange?') { - return 'https://example.com/two'; // Second rule to be re-arrange + return '2 - https://example.com/two'; // Second rule to be re-arrange } // First we select remove action to trigger code branch, then save to finish execution From 4aa65f750e28379f07c1c4b75cc1b8f352733a86 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 3 Mar 2024 13:16:37 +0100 Subject: [PATCH 10/10] Add CLI test for manage redirects command, to cover validation errors --- .../Command/ManageRedirectRulesTest.php | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 module/CLI/test-cli/Command/ManageRedirectRulesTest.php diff --git a/module/CLI/test-cli/Command/ManageRedirectRulesTest.php b/module/CLI/test-cli/Command/ManageRedirectRulesTest.php new file mode 100644 index 00000000..3da0a767 --- /dev/null +++ b/module/CLI/test-cli/Command/ManageRedirectRulesTest.php @@ -0,0 +1,33 @@ +exec([ManageRedirectRulesCommand::NAME, 'abc123'], [ + '0', // Add new rule + 'not-a-number', // Invalid priority + '1', // Valid priority, to continue execution + 'invalid-long-url', // Invalid long URL + 'https://example.com', // Valid long URL, to continue execution + '1', // Language condition type + '', // Invalid required language + 'es-ES', // Valid language, to continue execution + 'no', // Do not add more conditions + '4', // Discard changes + ]); + + self::assertStringContainsString('The priority must be a numeric positive value', $output); + self::assertStringContainsString('The input is not valid', $output); + self::assertStringContainsString('The value is mandatory', $output); + } +}