Remove name and uniqueness in redirect condition table

2025-02-25 18:45:27 -06:00 · 2024-02-29 09:05:30 +01:00
parent 23c07c4e82
commit 070d74830b
9 changed files with 36 additions and 104 deletions
--- 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"
--- 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,
--- 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();
 };
--- 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, [
--- 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();
--- 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,
--- 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'];
-    }
 }
--- 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',
--- 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);