#206 - Fix AdvancedStatCounterTask, IterableBatchTask, ConditionTrait and ColumnAggregatorTask edge cases

CHANGELOG.md

@@ -16,6 +16,7 @@ Latest
 * [#203](https://github.com/cleverage/process-bundle/issues/203) Fix `ProcessState::addErrorContextValue()`: accept any value type, so that iterable tasks on an empty iterable are skipped and PropertySetterTask reports the original exception instead of a `TypeError`. Update documentation, add tests.
 * [#204](https://github.com/cleverage/process-bundle/issues/204) Fix CachedTransformer, SlugifyTransformer and TypeSetterTransformer edge cases: accept a non-string input in `cached` (key built after `key_transformers`), reject an invalid `transliterator` in `slugify` with an `InvalidOptionsException`, remove the unreachable error branch of `type_setter`. Update documentation, add tests.
 * [#205](https://github.com/cleverage/process-bundle/issues/205) Fix FileSplitterTask: produced files lost the first line, doubled line breaks and ended with an empty file; each file now contains exactly `max_lines` lines of the source. Update documentation, add tests.
+* [#206](https://github.com/cleverage/process-bundle/issues/206) Fix AdvancedStatCounterTask, IterableBatchTask, ConditionTrait and ColumnAggregatorTask edge cases: log the first counted execution with correct counts, accept a `null` `batch_count` (only flush at the end), allow scalar inputs in conditions (`''` path on the whole value), aggregate `null` column values. Update documentation, add tests.
 
 ## Deprecated
 * [#189](https://github.com/cleverage/process-bundle/issues/189) EventDispatcherTask: when `event_name` is set, listening to `CleverAge\ProcessBundle\Event\EventDispatcherTaskEvent` is deprecated (the event is still dispatched under its class name, with an `E_USER_DEPRECATED` error, if it has listeners). Listen to the configured `event_name` instead: the BC layer will be removed in v6.0.

docs/reference/tasks/advanced_stat_counter_task.md

@@ -17,7 +17,7 @@ Input is ignored, only the number of executions matters.
 Possible outputs
 ----------------
 
-`null` when statistics are logged, otherwise the task is skipped (nothing is sent to the outputs). It is meant to be
+`null` when statistics are logged (with `show_every: 1`, on every counted execution), otherwise the task is skipped (nothing is sent to the outputs). It is meant to be
 used as the last task of a branch.
 
 The logged message has the following format:
@@ -33,7 +33,7 @@ Options
 |--------------|-------|:--------:|---------|---------------------------------------------------------------------------------------|
 | `num_items`  | `int` |          | `1`     | Number of items represented by one execution (multiplier of the counter)              |
 | `skip_first` | `int` |          | `0`     | Number of first executions to ignore (the elapsed time still starts at the first one) |
-| `show_every` | `int` |          | `1`     | Log the statistics every N executions (the first counted execution is never logged)   |
+| `show_every` | `int` |          | `1`     | Log the statistics every N counted executions (the N-th, 2N-th, ...)                  |
 
 Examples
 --------

docs/reference/tasks/column_aggregator_task.md

@@ -13,8 +13,8 @@ Task reference
 Accepted inputs
 ---------------
 
-`array`: an associative array that should contain the configured `columns` (a column whose value is `null` is
-considered missing)
+`array`: an associative array that should contain the configured `columns` (a column is present as soon as its key
+exists, even if its value is `null`)
 
 Possible outputs
 ----------------

docs/reference/tasks/filter_task.md

@@ -12,7 +12,8 @@ Task reference
 Accepted inputs
 ---------------
 
-`array` or `object`: values are read with the Symfony PropertyAccessor. A non-readable property is considered `null`.
+`any`: on an `array` or an `object`, values are read with the Symfony PropertyAccessor; a non-readable property is
+considered `null`. On a scalar input, use the empty path `''` to test the whole value (any other path gives `null`).
 
 Possible outputs
 ----------------

docs/reference/tasks/iterable_batch_task.md

@@ -26,9 +26,9 @@ Possible outputs
 Options
 -------
 
-| Code          | Type  | Required | Default | Description                                 |
-|---------------|-------|:--------:|---------|---------------------------------------------|
-| `batch_count` | `int` |          | `10`    | Number of inputs to buffer before iterating |
+| Code          | Type        | Required | Default | Description                                                                                     |
+|---------------|-------------|:--------:|---------|-------------------------------------------------------------------------------------------------|
+| `batch_count` | `int\|null` |          | `10`    | Number of inputs to buffer before iterating; if `null`, all inputs are buffered until the flush |
 
 Examples
 --------

docs/reference/traits/condition_trait.md

@@ -38,7 +38,9 @@ An invalid regular expression makes both `match_regexp` and `not_match_regexp` f
 * Call `ConditionTrait::checkCondition` with the input and the resolved conditions; it returns `true` if all
   conditions match
 
-The input must be an `array` or an `object` as soon as a condition is defined (a scalar input raises a `TypeError`).
+The input can be of any type: on a scalar input (e.g. the items of a list of strings in
+[ArrayFilterTransformer](../transformers/array_filter_transformer.md)), use the empty path `''` to test the whole value;
+any other path gives `null`, like a missing key.
 
 ## Examples
 

docs/reference/transformers/array_filter_transformer.md

@@ -28,8 +28,8 @@ Options
 | `condition` | `array` |          | `[]`    | Conditions each element must match, see [ConditionTrait](../traits/condition_trait.md)        |
 
 The `condition` option accepts the following keys, each one being a map of `property_path: value`. Properties are read
-from each element with the Symfony PropertyAccessor (an empty path `''` targets the element itself); an unreadable
-property is considered `null`.
+from each element with the Symfony PropertyAccessor (an empty path `''` targets the element itself, which is the way
+to filter a list of scalars); an unreadable property, or any non-empty path on a scalar element, is considered `null`.
 
 | Code               | Type    | Required | Default | Description                                                        |
 |--------------------|---------|:--------:|---------|--------------------------------------------------------------------|
@@ -66,3 +66,13 @@ array_filter:
     match_regexp:
       '[sku]': '/^A/'
 ```
+
+* Keep only the `a` values of a list of strings: `[a, b, a]` gives `{0: a, 2: a}`
+
+```yaml
+# Transformer options level
+array_filter:
+  condition:
+    match:
+      '': a
+```

src/Task/ColumnAggregatorTask.php

@@ -45,7 +45,8 @@ public function execute(ProcessState $state): void
 
         $missingColumns = [];
         foreach ($columns as $column) {
-            if (!isset($input[$column])) {
+            $hasColumn = \is_array($input) ? \array_key_exists($column, $input) : isset($input[$column]);
+            if (!$hasColumn) {
                 $missingColumns[] = $column;
                 continue;
             }

src/Task/IterableBatchTask.php

@@ -90,7 +90,7 @@ protected function configureOptions(OptionsResolver $resolver): void
             'batch_count' => 10,
         ]);
 
-        $resolver->setAllowedTypes('batch_count', 'integer');
+        $resolver->setAllowedTypes('batch_count', ['integer', 'null']);
     }
 
     /**

src/Task/Reporting/AdvancedStatCounterTask.php

@@ -49,7 +49,8 @@ public function execute(ProcessState $state): void
 
             return;
         }
-        if ($this->counter > 0 && 0 === $this->counter % $this->getOption($state, 'show_every')) {
+        ++$this->counter;
+        if (0 === $this->counter % $this->getOption($state, 'show_every')) {
             $diff = $now->diff($this->lastUpdate);
             $fullText = "Last iteration {$diff->format('%H:%I:%S')} ago";
             $items = $this->getOption($state, 'num_items') * $this->counter;
@@ -67,7 +68,6 @@ public function execute(ProcessState $state): void
         } else {
             $state->setSkipped(true);
         }
-        ++$this->counter;
     }
 
     protected function configureOptions(OptionsResolver $resolver): void

src/Transformer/ConditionTrait.php

@@ -107,7 +107,7 @@ protected function configureConditionOptions(OptionsResolver $resolver): void
      * Softly check if an input key match a value, or not.
      */
     protected function checkValue(
-        object|array $input,
+        mixed $input,
         string $key,
         mixed $value,
         bool $shouldMatch = true,
@@ -141,7 +141,7 @@ protected function checkValue(
     /**
      * Check if the input property is empty or not.
      */
-    protected function checkEmpty(object|array $input, string $key): bool
+    protected function checkEmpty(mixed $input, string $key): bool
     {
         $currentValue = $this->getValue($input, $key);
 
@@ -150,12 +150,14 @@ protected function checkEmpty(object|array $input, string $key): bool
 
     /**
      * Soft value getter (return the value or null).
+     *
+     * The empty key targets the whole input; any other key on a scalar input gives null (like a missing key).
      */
-    protected function getValue(object|array $input, string $key): mixed
+    protected function getValue(mixed $input, string $key): mixed
     {
         if ('' === $key) {
             $currentValue = $input;
-        } elseif ($this->accessor->isReadable($input, $key)) {
+        } elseif ((\is_array($input) || \is_object($input)) && $this->accessor->isReadable($input, $key)) {
             $currentValue = $this->accessor->getValue($input, $key);
         } else {
             $currentValue = null;

tests/Task/ColumnAggregatorTaskTest.php

@@ -0,0 +1,99 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle 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\ProcessBundle\Tests\Task;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\ColumnAggregatorTask;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\PropertyAccess\PropertyAccess;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(ColumnAggregatorTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class ColumnAggregatorTaskTest extends TestCase
+{
+    public function testRowsAreAggregatedByColumn(): void
+    {
+        $output = $this->aggregate(
+            ['columns' => ['a', 'b']],
+            [['a' => 'x', 'b' => 1], ['a' => 'y']],
+            ['ignore_missing' => true],
+        );
+
+        self::assertSame([
+            'a' => ['column' => 'a', 'values' => [['a' => 'x', 'b' => 1], ['a' => 'y']]],
+            'b' => ['column' => 'b', 'values' => [['a' => 'x', 'b' => 1]]],
+        ], $output);
+    }
+
+    public function testNullColumnIsNotMissing(): void
+    {
+        $output = $this->aggregate(['columns' => ['a', 'b']], [['a' => 'x', 'b' => null]]);
+
+        self::assertSame([
+            'a' => ['column' => 'a', 'values' => [['a' => 'x', 'b' => null]]],
+            'b' => ['column' => 'b', 'values' => [['a' => 'x', 'b' => null]]],
+        ], $output);
+    }
+
+    public function testMissingColumnThrows(): void
+    {
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage('Missing columns [b] in input');
+
+        $this->aggregate(['columns' => ['a', 'b']], [['a' => 'x']]);
+    }
+
+    /**
+     * @param list<array<string, mixed>> $inputs
+     */
+    private function aggregate(array $options, array $inputs, array $extraOptions = []): mixed
+    {
+        $state = $this->createState(ColumnAggregatorTask::class, $options + $extraOptions);
+        $task = new ColumnAggregatorTask(PropertyAccess::createPropertyAccessor(), new NullLogger());
+        $task->initialize($state);
+
+        foreach ($inputs as $input) {
+            $state->reset(false);
+            $state->setInput($input);
+            $task->execute($state);
+        }
+
+        $state->reset(true);
+        $task->proceed($state);
+
+        return $state->getOutput();
+    }
+
+    private function createState(string $class, array $options): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('task', $class, $options));
+
+        return $state;
+    }
+}

tests/Task/IterableBatchTaskTest.php

@@ -0,0 +1,112 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle 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\ProcessBundle\Tests\Task;
+
+use CleverAge\ProcessBundle\Configuration\ProcessConfiguration;
+use CleverAge\ProcessBundle\Configuration\TaskConfiguration;
+use CleverAge\ProcessBundle\Context\ContextualOptionResolver;
+use CleverAge\ProcessBundle\Model\AbstractConfigurableTask;
+use CleverAge\ProcessBundle\Model\ProcessHistory;
+use CleverAge\ProcessBundle\Model\ProcessState;
+use CleverAge\ProcessBundle\Task\IterableBatchTask;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(IterableBatchTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(AbstractConfigurableTask::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(TaskConfiguration::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ContextualOptionResolver::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessHistory::class)]
+#[\PHPUnit\Framework\Attributes\UsesClass(ProcessState::class)]
+class IterableBatchTaskTest extends TestCase
+{
+    public function testBatchCountIteratesOverTheBuffer(): void
+    {
+        [$task, $state] = $this->createTask(['batch_count' => 2]);
+
+        self::assertSame([null], $this->execute($task, $state, 'a'));
+        self::assertSame(['a', 'b'], $this->execute($task, $state, 'b'));
+        self::assertSame([null], $this->execute($task, $state, 'c'));
+        self::assertSame(['c'], $this->flush($task, $state));
+    }
+
+    public function testNullBatchCountOnlyOutputsOnFlush(): void
+    {
+        [$task, $state] = $this->createTask(['batch_count' => null]);
+
+        self::assertSame([null], $this->execute($task, $state, 'a'));
+        self::assertSame([null], $this->execute($task, $state, 'b'));
+        self::assertSame([null], $this->execute($task, $state, 'c'));
+        self::assertSame(['a', 'b', 'c'], $this->flush($task, $state));
+    }
+
+    /**
+     * @return array{IterableBatchTask, ProcessState}
+     */
+    private function createTask(array $options): array
+    {
+        $state = $this->createState(IterableBatchTask::class, $options);
+        $task = new IterableBatchTask(new NullLogger());
+        $task->initialize($state);
+
+        return [$task, $state];
+    }
+
+    /**
+     * Execute the task like the ProcessManager does, collecting outputs (null when skipped).
+     *
+     * @return list<mixed>
+     */
+    private function execute(IterableBatchTask $task, ProcessState $state, mixed $input): array
+    {
+        $outputs = [];
+        do {
+            $state->reset(false);
+            $state->setInput($input);
+            $task->execute($state);
+            $outputs[] = $state->isSkipped() ? null : $state->getOutput();
+        } while ($task->next($state));
+
+        return $outputs;
+    }
+
+    /**
+     * @return list<mixed>
+     */
+    private function flush(IterableBatchTask $task, ProcessState $state): array
+    {
+        $outputs = [];
+        do {
+            $state->reset(true);
+            $task->flush($state);
+            if (!$state->isSkipped()) {
+                $outputs[] = $state->getOutput();
+            }
+        } while ($task->next($state));
+
+        return $outputs;
+    }
+
+    private function createState(string $class, array $options): ProcessState
+    {
+        $processConfiguration = new ProcessConfiguration('test', []);
+        $state = new ProcessState($processConfiguration, new ProcessHistory($processConfiguration));
+        $state->setContextualOptionResolver(new ContextualOptionResolver());
+        $state->setContext([]);
+        $state->setTaskConfiguration(new TaskConfiguration('task', $class, $options));
+
+        return $state;
+    }
+}