Merge pull request #2035 from acelaya-forks/feature/improve-rules-persistence

Remove name and uniqueness in redirect condition table

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"

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,

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();
 };

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, [

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
+        // Insert a rule per every device_long_url, and link it to the corresponding condition
-        $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
         $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();

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;
+        return new self(RedirectConditionType::QUERY_PARAM, $value, $param);
-        $name = sprintf('%s-%s-%s', $type->value, $param, $value);
-
-        return new self($name, $type, $value, $param);
     }
 
     public static function forLanguage(string $language): self
     {
-        $type = RedirectConditionType::LANGUAGE;
+        return new self(RedirectConditionType::LANGUAGE, $language);
-        $name = sprintf('%s-%s', $type->value, $language);
-
-        return new self($name, $type, $language);
     }
 
     public static function forDevice(DeviceType $device): self
     {
-        $type = RedirectConditionType::DEVICE;
+        return new self(RedirectConditionType::DEVICE, $device->value);
-        $name = sprintf('%s-%s', $type->value, $device->value);
-
-        return new self($name, $type, $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,

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'];
-    }
 }

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',

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);