From 070d74830ba66b7715e72cf2a9d2747d6badb520 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 29 Feb 2024 09:05:30 +0100 Subject: [PATCH] Remove name and uniqueness in redirect condition table --- ...short-urls_{shortCode}_redirect-rules.json | 5 -- ....RedirectRule.Entity.RedirectCondition.php | 7 --- ...directRule.Entity.ShortUrlRedirectRule.php | 5 ++ .../Core/migrations/Version20240224115725.php | 2 - .../Core/migrations/Version20240226214216.php | 49 +++++++------------ .../RedirectRule/Entity/RedirectCondition.php | 18 ++----- .../Entity/RedirectConditionTest.php | 18 ------- .../test-api/Action/ListRedirectRulesTest.php | 7 +-- .../Fixtures/ShortUrlRedirectRulesFixture.php | 29 ++++------- 9 files changed, 36 insertions(+), 104 deletions(-) diff --git a/docs/swagger/paths/v3_short-urls_{shortCode}_redirect-rules.json b/docs/swagger/paths/v3_short-urls_{shortCode}_redirect-rules.json index 3854e2b9..44cd2d86 100644 --- a/docs/swagger/paths/v3_short-urls_{shortCode}_redirect-rules.json +++ b/docs/swagger/paths/v3_short-urls_{shortCode}_redirect-rules.json @@ -50,13 +50,11 @@ "priority": 1, "conditions": [ { - "name": "device-android", "type": "device", "matchValue": "android", "matchKey": null }, { - "name": "language-en-US", "type": "language", "matchValue": "en-US", "matchKey": null @@ -68,7 +66,6 @@ "priority": 2, "conditions": [ { - "name": "language-fr", "type": "language", "matchValue": "fr", "matchKey": null @@ -80,13 +77,11 @@ "priority": 3, "conditions": [ { - "name": "query-foo-bar", "type": "query", "matchKey": "foo", "matchValue": "bar" }, { - "name": "query-hello-world", "type": "query", "matchKey": "hello", "matchValue": "world" 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 2c1e1bdc..513089fa 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 @@ -22,13 +22,6 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->option('unsigned', true) ->build(); - fieldWithUtf8Charset($builder->createField('name', Types::STRING), $emConfig) - ->columnName('name') - ->length(512) - ->build(); - - $builder->addUniqueConstraint(['name'], 'UQ_name'); - (new FieldBuilder($builder, [ 'fieldName' => 'type', 'type' => Types::STRING, diff --git a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.ShortUrlRedirectRule.php b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.ShortUrlRedirectRule.php index cab72e89..3851de00 100644 --- a/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.ShortUrlRedirectRule.php +++ b/module/Core/config/entities-mappings/Shlinkio.Shlink.Core.RedirectRule.Entity.ShortUrlRedirectRule.php @@ -33,10 +33,15 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->addJoinColumn('short_url_id', 'id', nullable: false, onDelete: 'CASCADE') ->build(); + // We treat this ManyToMany relation as a unidirectional OneToMany, where conditions are persisted and deleted + // together with the rule $builder->createManyToMany('conditions', RedirectRule\Entity\RedirectCondition::class) ->setJoinTable(determineTableName('redirect_conditions_in_short_url_redirect_rules', $emConfig)) ->addInverseJoinColumn('redirect_condition_id', 'id', onDelete: 'CASCADE') ->addJoinColumn('short_url_redirect_rule_id', 'id', onDelete: 'CASCADE') ->fetchEager() // Always fetch the corresponding conditions when loading a rule + ->setOrderBy(['id' => 'ASC']) // Ensure a reliable order in the list of conditions + ->cascadePersist() // Create automatically with the rule + ->orphanRemoval() // Remove conditions when they are not linked to any rule ->build(); }; diff --git a/module/Core/migrations/Version20240224115725.php b/module/Core/migrations/Version20240224115725.php index 1e069d83..292e6dbb 100644 --- a/module/Core/migrations/Version20240224115725.php +++ b/module/Core/migrations/Version20240224115725.php @@ -34,8 +34,6 @@ final class Version20240224115725 extends AbstractMigration ]); $redirectConditions = $this->createTableWithId($schema, 'redirect_conditions'); - $redirectConditions->addColumn('name', Types::STRING, ['length' => 512]); - $redirectConditions->addUniqueIndex(['name'], 'UQ_name'); $redirectConditions->addColumn('type', Types::STRING, ['length' => 255]); $redirectConditions->addColumn('match_key', Types::STRING, [ diff --git a/module/Core/migrations/Version20240226214216.php b/module/Core/migrations/Version20240226214216.php index d7352717..74237ca0 100644 --- a/module/Core/migrations/Version20240226214216.php +++ b/module/Core/migrations/Version20240226214216.php @@ -16,36 +16,7 @@ final class Version20240226214216 extends AbstractMigration { $this->skipIf(! $schema->hasTable('device_long_urls')); - // First create redirect conditions for all device types - $qb = $this->connection->createQueryBuilder(); - $devices = $qb->select('device_type') - ->distinct() - ->from('device_long_urls') - ->executeQuery(); - - $conditionIds = []; - while ($deviceRow = $devices->fetchAssociative()) { - $deviceType = $deviceRow['device_type']; - $conditionQb = $this->connection->createQueryBuilder(); - $conditionQb->insert('redirect_conditions') - ->values([ - 'name' => ':name', - 'type' => ':type', - 'match_value' => ':match_value', - 'match_key' => ':match_key', - ]) - ->setParameters([ - 'name' => 'device-' . $deviceType, - 'type' => 'device', - 'match_value' => $deviceType, - 'match_key' => null, - ]) - ->executeStatement(); - $id = $this->connection->lastInsertId(); - $conditionIds[$deviceType] = $id; - } - - // Then insert a rule per every device_long_url, and link it to the corresponding condition + // Insert a rule per every device_long_url, and link it to the corresponding condition $qb = $this->connection->createQueryBuilder(); $rules = $qb->select('short_url_id', 'device_type', 'long_url') ->from('device_long_urls') @@ -71,6 +42,22 @@ final class Version20240226214216 extends AbstractMigration ->executeStatement(); $ruleId = $this->connection->lastInsertId(); + $deviceType = $ruleRow['device_type']; + $conditionQb = $this->connection->createQueryBuilder(); + $conditionQb->insert('redirect_conditions') + ->values([ + 'type' => ':type', + 'match_value' => ':match_value', + 'match_key' => ':match_key', + ]) + ->setParameters([ + 'type' => 'device', + 'match_value' => $deviceType, + 'match_key' => null, + ]) + ->executeStatement(); + $conditionId = $this->connection->lastInsertId(); + $relationQb = $this->connection->createQueryBuilder(); $relationQb->insert('redirect_conditions_in_short_url_redirect_rules') ->values([ @@ -78,7 +65,7 @@ final class Version20240226214216 extends AbstractMigration 'short_url_redirect_rule_id' => ':short_url_redirect_rule_id', ]) ->setParameters([ - 'redirect_condition_id' => $conditionIds[$ruleRow['device_type']], + 'redirect_condition_id' => $conditionId, 'short_url_redirect_rule_id' => $ruleId, ]) ->executeStatement(); diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index f5af53af..e5f0bae7 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -12,14 +12,12 @@ 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; class RedirectCondition extends AbstractEntity implements JsonSerializable { private function __construct( - public readonly string $name, private readonly RedirectConditionType $type, private readonly string $matchValue, private readonly ?string $matchKey = null, @@ -28,26 +26,17 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable 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); + return new self(RedirectConditionType::QUERY_PARAM, $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); + return new self(RedirectConditionType::LANGUAGE, $language); } public static function forDevice(DeviceType $device): self { - $type = RedirectConditionType::DEVICE; - $name = sprintf('%s-%s', $type->value, $device->value); - - return new self($name, $type, $device->value); + return new self(RedirectConditionType::DEVICE, $device->value); } /** @@ -103,7 +92,6 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable public function jsonSerialize(): array { return [ - 'name' => $this->name, 'type' => $this->type->value, 'matchKey' => $this->matchKey, 'matchValue' => $this->matchValue, diff --git a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php index 1eb7e0f8..eaea4c25 100644 --- a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php +++ b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php @@ -3,7 +3,6 @@ namespace ShlinkioTest\Shlink\Core\RedirectRule\Entity; use Laminas\Diactoros\ServerRequestFactory; -use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\TestCase; @@ -71,21 +70,4 @@ class RedirectConditionTest extends TestCase 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']; - yield [RedirectCondition::forDevice(DeviceType::ANDROID), 'device-android']; - yield [RedirectCondition::forDevice(DeviceType::IOS), 'device-ios']; - yield [RedirectCondition::forDevice(DeviceType::DESKTOP), 'device-desktop']; - } } diff --git a/module/Rest/test-api/Action/ListRedirectRulesTest.php b/module/Rest/test-api/Action/ListRedirectRulesTest.php index 5115d2d2..b86683c9 100644 --- a/module/Rest/test-api/Action/ListRedirectRulesTest.php +++ b/module/Rest/test-api/Action/ListRedirectRulesTest.php @@ -13,13 +13,11 @@ use function sprintf; class ListRedirectRulesTest extends ApiTestCase { private const LANGUAGE_EN_CONDITION = [ - 'name' => 'language-en', 'type' => 'language', 'matchKey' => null, 'matchValue' => 'en', ]; private const QUERY_FOO_BAR_CONDITION = [ - 'name' => 'query-foo-bar', 'type' => 'query', 'matchKey' => 'foo', 'matchValue' => 'bar', @@ -54,13 +52,12 @@ class ListRedirectRulesTest extends ApiTestCase 'longUrl' => 'https://example.com/multiple-query-params', 'priority' => 2, 'conditions' => [ - self::QUERY_FOO_BAR_CONDITION, [ - 'name' => 'query-hello-world', 'type' => 'query', 'matchKey' => 'hello', 'matchValue' => 'world', ], + self::QUERY_FOO_BAR_CONDITION, ], ], [ @@ -73,7 +70,6 @@ class ListRedirectRulesTest extends ApiTestCase 'priority' => 4, 'conditions' => [ [ - 'name' => 'device-android', 'type' => 'device', 'matchKey' => null, 'matchValue' => 'android', @@ -85,7 +81,6 @@ class ListRedirectRulesTest extends ApiTestCase 'priority' => 5, 'conditions' => [ [ - 'name' => 'device-ios', 'type' => 'device', 'matchKey' => null, 'matchValue' => 'ios', diff --git a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php index 7607724b..267969f1 100644 --- a/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php +++ b/module/Rest/test-api/Fixtures/ShortUrlRedirectRulesFixture.php @@ -25,27 +25,14 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF /** @var ShortUrl $defShortUrl */ $defShortUrl = $this->getReference('def456_short_url'); - $englishCondition = RedirectCondition::forLanguage('en'); - $manager->persist($englishCondition); - - $fooQueryCondition = RedirectCondition::forQueryParam('foo', 'bar'); - $manager->persist($fooQueryCondition); - - $helloQueryCondition = RedirectCondition::forQueryParam('hello', 'world'); - $manager->persist($helloQueryCondition); - - $androidCondition = RedirectCondition::forDevice(DeviceType::ANDROID); - $manager->persist($androidCondition); - - $iosCondition = RedirectCondition::forDevice(DeviceType::IOS); - $manager->persist($iosCondition); - // Create rules disordered to make sure the order by priority works $multipleQueryParamsRule = new ShortUrlRedirectRule( shortUrl: $defShortUrl, priority: 2, longUrl: 'https://example.com/multiple-query-params', - conditions: new ArrayCollection([$helloQueryCondition, $fooQueryCondition]), + conditions: new ArrayCollection( + [RedirectCondition::forQueryParam('hello', 'world'), RedirectCondition::forQueryParam('foo', 'bar')], + ), ); $manager->persist($multipleQueryParamsRule); @@ -53,7 +40,9 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF shortUrl: $defShortUrl, priority: 1, longUrl: 'https://example.com/english-and-foo-query', - conditions: new ArrayCollection([$englishCondition, $fooQueryCondition]), + conditions: new ArrayCollection( + [RedirectCondition::forLanguage('en'), RedirectCondition::forQueryParam('foo', 'bar')], + ), ); $manager->persist($englishAndFooQueryRule); @@ -61,7 +50,7 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF shortUrl: $defShortUrl, priority: 4, longUrl: 'https://blog.alejandrocelaya.com/android', - conditions: new ArrayCollection([$androidCondition]), + conditions: new ArrayCollection([RedirectCondition::forDevice(DeviceType::ANDROID)]), ); $manager->persist($androidRule); @@ -69,7 +58,7 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF shortUrl: $defShortUrl, priority: 3, longUrl: 'https://example.com/only-english', - conditions: new ArrayCollection([$englishCondition]), + conditions: new ArrayCollection([RedirectCondition::forLanguage('en')]), ); $manager->persist($onlyEnglishRule); @@ -77,7 +66,7 @@ class ShortUrlRedirectRulesFixture extends AbstractFixture implements DependentF shortUrl: $defShortUrl, priority: 5, longUrl: 'https://blog.alejandrocelaya.com/ios', - conditions: new ArrayCollection([$iosCondition]), + conditions: new ArrayCollection([RedirectCondition::forDevice(DeviceType::IOS)]), ); $manager->persist($iosRule);