Create RedirectRulesDataTest

composer.json

@@ -19,6 +19,7 @@
         "ext-pdo": "*",
         "akrabat/ip-address-middleware": "^2.1",
         "cakephp/chronos": "^3.0.2",
+        "doctrine/dbal": "^4.0",
         "doctrine/migrations": "^3.6",
         "doctrine/orm": "^3.0",
         "endroid/qr-code": "^5.0",

module/Core/src/RedirectRule/Entity/RedirectCondition.php

@@ -7,6 +7,7 @@ use Psr\Http\Message\ServerRequestInterface;
 use Shlinkio\Shlink\Common\Entity\AbstractEntity;
 use Shlinkio\Shlink\Core\Model\DeviceType;
 use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType;
+use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter;
 
 use function Shlinkio\Shlink\Core\acceptLanguageToLocales;
 use function Shlinkio\Shlink\Core\ArrayUtils\some;
@@ -39,6 +40,15 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable
         return new self(RedirectConditionType::DEVICE, $device->value);
     }
 
+    public static function fromRawData(array $rawData): self
+    {
+        $type = RedirectConditionType::from($rawData[RedirectRulesInputFilter::CONDITION_TYPE]);
+        $value = $rawData[RedirectRulesInputFilter::CONDITION_MATCH_VALUE];
+        $key = $rawData[RedirectRulesInputFilter::CONDITION_MATCH_KEY] ?? null;
+
+        return new self($type, $value, $key);
+    }
+
     /**
      * Tells if this condition matches provided request
      */

module/Core/src/RedirectRule/Model/RedirectRulesData.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace Shlinkio\Shlink\Core\RedirectRule\Model;
 
+use Laminas\InputFilter\Exception\InvalidArgumentException;
 use Shlinkio\Shlink\Core\Exception\ValidationException;
 use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter;
 
@@ -15,11 +16,17 @@ readonly class RedirectRulesData
 
     public static function fromRawData(array $rawData): self
     {
-        $inputFilter = RedirectRulesInputFilter::initialize($rawData);
+        try {
-        if (! $inputFilter->isValid()) {
+            $inputFilter = RedirectRulesInputFilter::initialize($rawData);
-            throw ValidationException::fromInputFilter($inputFilter);
+            if (! $inputFilter->isValid()) {
-        }
+                throw ValidationException::fromInputFilter($inputFilter);
+            }
 
-        return new self($inputFilter->getValue(RedirectRulesInputFilter::REDIRECT_RULES));
+            return new self($inputFilter->getValue(RedirectRulesInputFilter::REDIRECT_RULES));
+        } catch (InvalidArgumentException) {
+            throw ValidationException::fromArray(
+                [RedirectRulesInputFilter::REDIRECT_RULES => RedirectRulesInputFilter::REDIRECT_RULES],
+            );
+        }
     }
 }

module/Core/src/RedirectRule/ShortUrlRedirectRuleService.php

@@ -4,10 +4,8 @@ namespace Shlinkio\Shlink\Core\RedirectRule;
 
 use Doctrine\Common\Collections\ArrayCollection;
 use Doctrine\ORM\EntityManagerInterface;
-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\Model\RedirectRulesData;
 use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter;
 use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl;
@@ -36,46 +34,39 @@ readonly class ShortUrlRedirectRuleService implements ShortUrlRedirectRuleServic
      */
     public function setRulesForShortUrl(ShortUrl $shortUrl, RedirectRulesData $data): array
     {
-        return $this->em->wrapInTransaction(function () use ($shortUrl, $data): array {
+        return $this->em->wrapInTransaction(fn () => $this->doSetRulesForShortUrl($shortUrl, $data));
-            // 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 $rule) {
-                $rule = new ShortUrlRedirectRule(
-                    shortUrl: $shortUrl,
-                    priority: $rule[RedirectRulesInputFilter::RULE_PRIORITY],
-                    longUrl: $rule[RedirectRulesInputFilter::RULE_LONG_URL],
-                    conditions: new ArrayCollection(array_map(
-                        fn (array $conditionData) => $this->createCondition($conditionData),
-                        $rule[RedirectRulesInputFilter::RULE_CONDITIONS],
-                    )),
-                );
-
-                $rules[] = $rule;
-                $this->em->persist($rule);
-            }
-
-            return $rules;
-        });
     }
 
-    private function createCondition(array $rawConditionData): RedirectCondition
+    /**
+     * @return ShortUrlRedirectRule[]
+     */
+    private function doSetRulesForShortUrl(ShortUrl $shortUrl, RedirectRulesData $data): array
     {
-        $type = RedirectConditionType::from($rawConditionData[RedirectRulesInputFilter::CONDITION_TYPE]);
+        // First, delete existing rules for the short URL
-        $value = $rawConditionData[RedirectRulesInputFilter::CONDITION_MATCH_VALUE];
+        $oldRules = $this->rulesForShortUrl($shortUrl);
-        $key = $rawConditionData[RedirectRulesInputFilter::CONDITION_MATCH_KEY];
+        foreach ($oldRules as $oldRule) {
+            $oldRule->clearConditions(); // This will trigger the orphan removal of old conditions
+            $this->em->remove($oldRule);
+        }
+        $this->em->flush();
 
-        return match ($type) {
+        // Then insert new rules
-            RedirectConditionType::DEVICE => RedirectCondition::forDevice(DeviceType::from($value)),
+        $rules = [];
-            RedirectConditionType::LANGUAGE => RedirectCondition::forLanguage($value),
+        foreach ($data->rules as $rule) {
-            RedirectConditionType::QUERY_PARAM => RedirectCondition::forQueryParam($key, $value),
+            $rule = new ShortUrlRedirectRule(
-        };
+                shortUrl: $shortUrl,
+                priority: $rule[RedirectRulesInputFilter::RULE_PRIORITY],
+                longUrl: $rule[RedirectRulesInputFilter::RULE_LONG_URL],
+                conditions: new ArrayCollection(array_map(
+                    RedirectCondition::fromRawData(...),
+                    $rule[RedirectRulesInputFilter::RULE_CONDITIONS],
+                )),
+            );
+
+            $rules[] = $rule;
+            $this->em->persist($rule);
+        }
+
+        return $rules;
     }
 }

module/Core/test/RedirectRule/Entity/ShortUrlRedirectRuleTest.php

@@ -20,7 +20,7 @@ class ShortUrlRedirectRuleTest extends TestCase
             ->withHeader('Accept-Language', 'en-UK')
             ->withQueryParams(['foo' => 'bar']);
 
-        $result = $this->createRule($conditions)->matchesRequest($request);
+        $result = $this->createRule(new ArrayCollection($conditions))->matchesRequest($request);
 
         self::assertEquals($expectedResult, $result);
     }
@@ -38,12 +38,25 @@ class ShortUrlRedirectRuleTest extends TestCase
         ];
     }
 
+    #[Test]
+    public function conditionsCanBeCleared(): void
+    {
+        $conditions = new ArrayCollection(
+            [RedirectCondition::forLanguage('en-UK'), RedirectCondition::forQueryParam('foo', 'bar')],
+        );
+        $rule = $this->createRule($conditions);
+
+        self::assertNotEmpty($conditions);
+        $rule->clearConditions();
+        self::assertEmpty($conditions);
+    }
+
     /**
-     * @param RedirectCondition[] $conditions
+     * @param ArrayCollection<RedirectCondition> $conditions
      */
-    private function createRule(array $conditions): ShortUrlRedirectRule
+    private function createRule(ArrayCollection $conditions): ShortUrlRedirectRule
     {
         $shortUrl = ShortUrl::withLongUrl('https://s.test');
-        return new ShortUrlRedirectRule($shortUrl, 1, '', new ArrayCollection($conditions));
+        return new ShortUrlRedirectRule($shortUrl, 1, '', $conditions);
     }
 }

module/Core/test/RedirectRule/Model/RedirectRulesDataTest.php

@@ -0,0 +1,66 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Core\RedirectRule\Model;
+
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\Attributes\TestWith;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Core\Exception\ValidationException;
+use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectRulesData;
+
+class RedirectRulesDataTest extends TestCase
+{
+    #[Test]
+    #[TestWith([['redirectRules' => ['foo']]])]
+    #[TestWith([['redirectRules' => [
+        ['priority' => 'foo'],
+    ]]])]
+    #[TestWith([['redirectRules' => [
+        [
+            'priority' => 4,
+            'longUrl' => 34,
+        ],
+    ]]])]
+    #[TestWith([['redirectRules' => [
+        [
+            'priority' => 4,
+            'longUrl' => 'https://example.com',
+            'conditions' => [
+                [
+                    'type' => 'invalid',
+                ],
+            ],
+        ],
+    ]]])]
+    #[TestWith([['redirectRules' => [
+        [
+            'priority' => 4,
+            'longUrl' => 'https://example.com',
+            'conditions' => [
+                [
+                    'type' => 'device',
+                    'matchValue' => 'invalid-device',
+                    'matchKey' => null,
+                ],
+            ],
+        ],
+    ]]])]
+    #[TestWith([['redirectRules' => [
+        [
+            'priority' => 4,
+            'longUrl' => 'https://example.com',
+            'conditions' => [
+                [
+                    'type' => 'language',
+                ],
+            ],
+        ],
+    ]]])]
+    public function throwsWhenProvidedDataIsInvalid(array $invalidData): void
+    {
+        $this->expectException(ValidationException::class);
+        RedirectRulesData::fromRawData($invalidData);
+    }
+}