fix(task) #22 #23 GetTask and SetTask reject non-array inputs and validate the option values given by the input

CHANGELOG.md

@@ -1,6 +1,10 @@
 Latest
 ------
 
+### Fixes
+* [#22](https://github.com/cleverage/cache-process-bundle/issues/22) Fix GetTask and SetTask: throw an explicit `\UnexpectedValueException` on a non-array input (a `\TypeError` was triggered by `array_merge()`). Update documentation, add tests.
+* [#23](https://github.com/cleverage/cache-process-bundle/issues/23) Fix GetTask and SetTask: validate the option values given by the input with the options resolver (they were used as is). Update documentation, add tests.
+
 v2.1
 ------
 

docs/index.md

@@ -41,8 +41,8 @@ services:
 `CleverAge\CacheProcessBundle\Task\AbstractCacheTask` can be extended to implement other cache operations. It extends
 [AbstractConfigurableTask](https://github.com/cleverage/process-bundle/blob/main/docs/03-custom_tasks.md), requires
 the `cleverage_cache_process.registry.adapter` service (`AdapterRegistry`) as constructor argument, defines the
-required `adapter` and `key` string options, and provides `getMergedOptions()` (options merged with the array input)
-and `$this->registry->getAdapter($code)`.
+required `adapter` and `key` string options, and provides `getMergedOptions()` (options merged with the array input,
+resolved again so that the input values are validated) and `$this->registry->getAdapter($code)`.
 
 ```php
 <?php

docs/reference/tasks/get_task.md

@@ -12,10 +12,11 @@ Task reference
 Accepted inputs
 ---------------
 
-`array` or empty (`null`): if the input is not empty, it is merged over the resolved options (`array_merge`), so
-input keys `adapter` and `key` override the options of the same name. Other input keys are ignored.
+`array` or empty (`null`): if the input is not empty, its keys `adapter` and `key` override the options of the same
+name, and are validated like the options (e.g. a non-string `key` throws an `InvalidOptionsException`). Other input
+keys are ignored.
 
-Any other non-empty input (e.g. a `string`) triggers a `\TypeError`.
+Any other non-empty input (e.g. a `string`) throws an `\UnexpectedValueException`.
 
 Possible outputs
 ----------------
@@ -85,7 +86,6 @@ Notes
 
 * `adapter` and `key` are required at configuration level, even when they are always given by the input: set them to
   a placeholder value (e.g. `key: ''`).
-* The values coming from the input are not validated by the options resolver.
 * A missing key and an item stored with a `null` value both output `null`. Chain a
   [SkipEmptyTask](https://github.com/cleverage/process-bundle/blob/main/docs/reference/tasks/skip_empty_task.md) to
   stop the branch when nothing is found.

docs/reference/tasks/set_task.md

@@ -12,10 +12,11 @@ Task reference
 Accepted inputs
 ---------------
 
-`array` or empty (`null`): if the input is not empty, it is merged over the resolved options (`array_merge`), so
-input keys `adapter`, `key` and `value` override the options of the same name. Other input keys are ignored.
+`array` or empty (`null`): if the input is not empty, its keys `adapter`, `key` and `value` override the options of the
+same name, and are validated like the options (e.g. a non-string `key` throws an `InvalidOptionsException`). Other
+input keys are ignored.
 
-Any other non-empty input (e.g. a `string`) triggers a `\TypeError`.
+Any other non-empty input (e.g. a `string`) throws an `\UnexpectedValueException`.
 
 Possible outputs
 ----------------
@@ -89,7 +90,6 @@ Notes
 * `adapter`, `key` and `value` are required at configuration level, even when they are always given by the input:
   set them to a placeholder value (e.g. `key: ''`, `value: ~`). If the input does not override the placeholder key,
   the empty key throws a `Psr\Cache\InvalidArgumentException`.
-* The values coming from the input are not validated by the options resolver.
 * No expiration is set on the item: its lifetime is the default lifetime of the adapter (see
   [Adapter](../adapter.md#notes)).
 * The item is saved immediately (`save()`, not `saveDeferred()`), an existing item with the same key is overwritten.

src/Task/AbstractCacheTask.php

@@ -17,6 +17,7 @@
 use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
 use CleverAge\ProcessBundle\Model\ProcessState;
 use Symfony\Component\OptionsResolver\Exception\AccessException;
+use Symfony\Component\OptionsResolver\Exception\ExceptionInterface;
 use Symfony\Component\OptionsResolver\Exception\UndefinedOptionsException;
 use Symfony\Component\OptionsResolver\OptionsResolver;
 
@@ -39,16 +40,25 @@ protected function configureOptions(OptionsResolver $resolver): void
     }
 
     /**
+     * Resolve the options merged with the input keys matching a defined option, the other input keys are ignored.
+     *
      * @return array<mixed>
+     *
+     * @throws ExceptionInterface
      */
     protected function getMergedOptions(ProcessState $state): array
     {
         /** @var array<mixed> $options */
         $options = $this->getOptions($state);
 
-        /** @var array<mixed> $input */
         $input = $state->getInput() ?: [];
+        if (!\is_array($input)) {
+            throw new \UnexpectedValueException(\sprintf('%s expects an array or null input, %s given', (new \ReflectionClass($this))->getShortName(), get_debug_type($input)));
+        }
 
-        return array_merge($options, $input);
+        $resolver = new OptionsResolver();
+        $this->configureOptions($resolver);
+
+        return $resolver->resolve(array_merge($options, array_intersect_key($input, array_flip($resolver->getDefinedOptions()))));
     }
 }

tests/Task/GetTaskTest.php

@@ -0,0 +1,113 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/CacheProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\CacheProcessBundle\Tests\Task;
+
+use CleverAge\CacheProcessBundle\Adapter\Adapter;
+use CleverAge\CacheProcessBundle\Registry\AdapterRegistry;
+use CleverAge\CacheProcessBundle\Task\GetTask;
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
+
+#[CoversClass(GetTask::class)]
+#[UsesClass(Adapter::class)]
+#[UsesClass(AdapterRegistry::class)]
+class GetTaskTest extends TestCase
+{
+    private Adapter $adapter;
+
+    protected function setUp(): void
+    {
+        $this->adapter = new Adapter(new ArrayAdapter(), 'memory');
+        $this->adapter->save($this->adapter->getItem('key1')->set('value1'));
+        $this->adapter->save($this->adapter->getItem('key2')->set('value2'));
+    }
+
+    public function testGetValue(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => 'key1']);
+
+        self::assertSame('value1', $this->execute($task, $state, null));
+    }
+
+    public function testGetMissingValue(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => 'missing']);
+
+        self::assertNull($this->execute($task, $state, null));
+    }
+
+    public function testInputOverridesOptions(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => '']);
+
+        self::assertSame('value1', $this->execute($task, $state, ['key' => 'key1', 'sku' => 'ignored']));
+        self::assertSame('value2', $this->execute($task, $state, ['key' => 'key2']));
+    }
+
+    public function testNonArrayInputIsRejected(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => '']);
+
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage('GetTask expects an array or null input, string given');
+        $this->execute($task, $state, 'key1');
+    }
+
+    public function testInputValuesAreValidated(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => '']);
+
+        $this->expectException(InvalidOptionsException::class);
+        $this->expectExceptionMessage('The option "key" with value 1 is expected to be of type "string", but is of type "int".');
+        $this->execute($task, $state, ['key' => 1]);
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     *
+     * @return array{GetTask, ProcessState}
+     */
+    private function createTask(array $options): array
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('get', GetTask::class, $options));
+
+        $registry = new AdapterRegistry();
+        $registry->addAdapter($this->adapter);
+        $task = new GetTask($registry);
+        $task->initialize($state);
+
+        return [$task, $state];
+    }
+
+    private function execute(GetTask $task, ProcessState $state, mixed $input): mixed
+    {
+        $state->reset(false);
+        $state->setInput($input);
+        $task->execute($state);
+
+        return $state->getOutput();
+    }
+}

tests/Task/SetTaskTest.php

@@ -0,0 +1,107 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/CacheProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\CacheProcessBundle\Tests\Task;
+
+use CleverAge\CacheProcessBundle\Adapter\Adapter;
+use CleverAge\CacheProcessBundle\Registry\AdapterRegistry;
+use CleverAge\CacheProcessBundle\Task\SetTask;
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\TestCase;
+use Symfony\Component\Cache\Adapter\ArrayAdapter;
+use Symfony\Component\OptionsResolver\Exception\InvalidOptionsException;
+
+#[CoversClass(SetTask::class)]
+#[UsesClass(Adapter::class)]
+#[UsesClass(AdapterRegistry::class)]
+class SetTaskTest extends TestCase
+{
+    private Adapter $adapter;
+
+    protected function setUp(): void
+    {
+        $this->adapter = new Adapter(new ArrayAdapter(), 'memory');
+    }
+
+    public function testSetValue(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => 'key1', 'value' => ['column1' => 'value1']]);
+
+        $this->execute($task, $state, null);
+
+        self::assertSame(['column1' => 'value1'], $this->adapter->getItem('key1')->get());
+    }
+
+    public function testInputOverridesOptions(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => '', 'value' => null]);
+
+        $this->execute($task, $state, ['key' => 'key1', 'value' => 'value1', 'sku' => 'ignored']);
+        $this->execute($task, $state, ['key' => 'key2', 'value' => 'value2']);
+
+        self::assertSame('value1', $this->adapter->getItem('key1')->get());
+        self::assertSame('value2', $this->adapter->getItem('key2')->get());
+    }
+
+    public function testNonArrayInputIsRejected(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => 'key1', 'value' => null]);
+
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage('SetTask expects an array or null input, string given');
+        $this->execute($task, $state, 'value1');
+    }
+
+    public function testInputValuesAreValidated(): void
+    {
+        [$task, $state] = $this->createTask(['adapter' => 'memory', 'key' => '', 'value' => null]);
+
+        $this->expectException(InvalidOptionsException::class);
+        $this->expectExceptionMessage('The option "adapter" with value 1 is expected to be of type "string", but is of type "int".');
+        $this->execute($task, $state, ['adapter' => 1, 'key' => 'key1', 'value' => 'value1']);
+    }
+
+    /**
+     * @param array<string, mixed> $options
+     *
+     * @return array{SetTask, ProcessState}
+     */
+    private function createTask(array $options): array
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('set', SetTask::class, $options));
+
+        $registry = new AdapterRegistry();
+        $registry->addAdapter($this->adapter);
+        $task = new SetTask($registry);
+        $task->initialize($state);
+
+        return [$task, $state];
+    }
+
+    private function execute(SetTask $task, ProcessState $state, mixed $input): void
+    {
+        $state->reset(false);
+        $state->setInput($input);
+        $task->execute($state);
+    }
+}