diff --git a/CHANGELOG.md b/CHANGELOG.md index eb6b4236..6f492f76 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/reference/tasks/advanced_stat_counter_task.md b/docs/reference/tasks/advanced_stat_counter_task.md index 6d47abff..8853593b 100644 --- a/docs/reference/tasks/advanced_stat_counter_task.md +++ b/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 -------- diff --git a/docs/reference/tasks/column_aggregator_task.md b/docs/reference/tasks/column_aggregator_task.md index 47244242..5a72508d 100644 --- a/docs/reference/tasks/column_aggregator_task.md +++ b/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 ---------------- diff --git a/docs/reference/tasks/filter_task.md b/docs/reference/tasks/filter_task.md index 6a691b21..ed1860a5 100644 --- a/docs/reference/tasks/filter_task.md +++ b/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 ---------------- diff --git a/docs/reference/tasks/iterable_batch_task.md b/docs/reference/tasks/iterable_batch_task.md index 1cd08b39..0762c9e0 100644 --- a/docs/reference/tasks/iterable_batch_task.md +++ b/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 -------- diff --git a/docs/reference/traits/condition_trait.md b/docs/reference/traits/condition_trait.md index ec2bfd09..544c594e 100644 --- a/docs/reference/traits/condition_trait.md +++ b/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 diff --git a/docs/reference/transformers/array_filter_transformer.md b/docs/reference/transformers/array_filter_transformer.md index 13746eef..4480e66b 100644 --- a/docs/reference/transformers/array_filter_transformer.md +++ b/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 +``` diff --git a/src/Task/ColumnAggregatorTask.php b/src/Task/ColumnAggregatorTask.php index d08bfe06..a15c7938 100644 --- a/src/Task/ColumnAggregatorTask.php +++ b/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; } diff --git a/src/Task/IterableBatchTask.php b/src/Task/IterableBatchTask.php index b8840a74..f841f3f5 100644 --- a/src/Task/IterableBatchTask.php +++ b/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']); } /** diff --git a/src/Task/Reporting/AdvancedStatCounterTask.php b/src/Task/Reporting/AdvancedStatCounterTask.php index e7d85b16..e7aa97a4 100644 --- a/src/Task/Reporting/AdvancedStatCounterTask.php +++ b/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 diff --git a/src/Transformer/ConditionTrait.php b/src/Transformer/ConditionTrait.php index d5e2dec0..d91e705d 100644 --- a/src/Transformer/ConditionTrait.php +++ b/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; diff --git a/tests/Task/ColumnAggregatorTaskTest.php b/tests/Task/ColumnAggregatorTaskTest.php new file mode 100644 index 00000000..cbbb03de --- /dev/null +++ b/tests/Task/ColumnAggregatorTaskTest.php @@ -0,0 +1,99 @@ +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> $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; + } +} diff --git a/tests/Task/IterableBatchTaskTest.php b/tests/Task/IterableBatchTaskTest.php new file mode 100644 index 00000000..debf563e --- /dev/null +++ b/tests/Task/IterableBatchTaskTest.php @@ -0,0 +1,112 @@ +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 + */ + 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 + */ + 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; + } +} diff --git a/tests/Task/Reporting/AdvancedStatCounterTaskTest.php b/tests/Task/Reporting/AdvancedStatCounterTaskTest.php new file mode 100644 index 00000000..648963d2 --- /dev/null +++ b/tests/Task/Reporting/AdvancedStatCounterTaskTest.php @@ -0,0 +1,106 @@ +runTask(3, ['show_every' => 1]); + + self::assertSame([false, false, false], $skipped); + self::assertCount(3, $messages); + self::assertStringContainsString(' 1 items processed', $messages[0]); + self::assertStringContainsString(' 2 items processed', $messages[1]); + self::assertStringContainsString(' 3 items processed', $messages[2]); + } + + public function testEveryNthExecutionIsLogged(): void + { + [$messages, $skipped] = $this->runTask(7, ['show_every' => 3, 'num_items' => 10]); + + self::assertSame([true, true, false, true, true, false, true], $skipped); + self::assertCount(2, $messages); + self::assertStringContainsString(' 30 items processed', $messages[0]); + self::assertStringContainsString(' 60 items processed', $messages[1]); + } + + public function testSkipFirstExecutionsAreNotCounted(): void + { + [$messages, $skipped] = $this->runTask(4, ['show_every' => 2, 'skip_first' => 1]); + + self::assertSame([true, true, false, true], $skipped); + self::assertCount(1, $messages); + self::assertStringContainsString(' 2 items processed', $messages[0]); + } + + /** + * @param array $options + * + * @return array{list, list} + */ + private function runTask(int $executions, array $options): array + { + $logger = new class extends AbstractLogger { + /** @var list */ + public array $messages = []; + + public function log($level, string|\Stringable $message, array $context = []): void + { + $this->messages[] = (string) $message; + } + }; + + $state = $this->createState(AdvancedStatCounterTask::class, $options); + $task = new AdvancedStatCounterTask($logger); + $task->initialize($state); + + $skipped = []; + for ($i = 0; $i < $executions; ++$i) { + $state->reset(false); + $task->execute($state); + $skipped[] = $state->isSkipped(); + } + + return [$logger->messages, $skipped]; + } + + 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; + } +} diff --git a/tests/Transformer/Array/ArrayFilterTransformerTest.php b/tests/Transformer/Array/ArrayFilterTransformerTest.php new file mode 100644 index 00000000..32f5ba52 --- /dev/null +++ b/tests/Transformer/Array/ArrayFilterTransformerTest.php @@ -0,0 +1,54 @@ + 'a', 2 => 'a'], $this->filter(['a', 'b', 'a'], ['match' => ['' => 'a']])); + self::assertSame([1 => 'b'], $this->filter(['a', 'b'], ['not_match' => ['' => 'a']])); + self::assertSame([1 => 'b'], $this->filter(['a', 'b'], ['match_regexp' => ['' => '/^b$/']])); + self::assertSame([1 => 'b'], $this->filter(['', 'b'], ['not_empty' => ['' => null]])); + } + + public function testPropertyPathOnScalarIsNull(): void + { + self::assertSame([], $this->filter(['a', 'b'], ['match' => ['[key]' => 'a']])); + self::assertSame(['a', 'b'], $this->filter(['a', 'b'], ['empty' => ['[key]' => null]])); + } + + public function testConditionOnArrays(): void + { + $items = [['type' => 'product'], ['type' => 'category'], ['other' => 'x']]; + + self::assertSame([0 => ['type' => 'product']], $this->filter($items, ['match' => ['[type]' => 'product']])); + self::assertSame([2 => ['other' => 'x']], $this->filter($items, ['empty' => ['[type]' => null]])); + } + + private function filter(array $value, array $condition): array + { + $transformer = new ArrayFilterTransformer(PropertyAccess::createPropertyAccessor()); + $resolver = new OptionsResolver(); + $transformer->configureOptions($resolver); + + return $transformer->transform($value, $resolver->resolve(['condition' => $condition])); + } +}