diff --git a/CHANGELOG.md b/CHANGELOG.md index 18d4b0fa..9ef8d1dc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Latest * [#368](https://github.com/cleverage/process-bundle/issues/368) `YamlWriterTask` (`yaml_writer`): the parent directory is created for a local path only (no scheme, or `file://`), not for a stream wrapper (`compress.zlib://`, `s3://`...), which failed, or `php://stdout`, which created a `php:` directory. Update documentation, add tests. * [#369](https://github.com/cleverage/process-bundle/issues/369) `StopTask` (`stop`): a process that has already failed (e.g. after a task initialization failure, going on being deprecated) stays failed instead of being flagged as stopped: the `Process failed` log is written at the `failed_level` and `ProcessEvent::isProcessStopped()` is `false`, as before #337. Update documentation, add tests. * [#370](https://github.com/cleverage/process-bundle/issues/370) `RulesTransformer` (`rules`): the result of a `condition` is cast to `bool`, so a condition returning an `int` (`preg_match()`), a string, `null` or any other non-boolean value (a single variable) no longer throws a `TypeError`: the rule matches if the result is truthy. Update documentation, add tests. +* [#372](https://github.com/cleverage/process-bundle/issues/372) `ProcessConfigurationRegistry`: an unknown task in the `outputs` or `error_outputs` of a task, or in the `entry_point` or `end_point` of a process, throws an `InvalidProcessConfigurationException` naming the process, the task and the key holding the reference, instead of a `MissingTaskConfigurationException` (`No task configuration with code : `). A process whose configuration is not valid is left out of `getProcessConfigurations()` (`cleverage:process:list`, lists of the UI) with an error log on the `cleverage_process` channel, instead of making every listing fail; getting or executing this process still throws. Update documentation, add tests. v5.5 ----- diff --git a/config/services/registry.yaml b/config/services/registry.yaml index e476e992..b8c70fbb 100644 --- a/config/services/registry.yaml +++ b/config/services/registry.yaml @@ -6,8 +6,10 @@ services: - ~ - ~ - ~ + - '@logger' # Processes left out of the listings because their configuration is not valid tags: - { name: kernel.reset, method: reset } + - { name: monolog.logger, channel: cleverage_process } CleverAge\ProcessBundle\Registry\ProcessConfigurationRegistry: alias: cleverage_process.registry.process_configuration CleverAge\ProcessBundle\Registry\ProcessConfigurationRegistryInterface: diff --git a/docs/reference/01-process_definition.md b/docs/reference/01-process_definition.md index b7e91e89..a1911a3c 100644 --- a/docs/reference/01-process_definition.md +++ b/docs/reference/01-process_definition.md @@ -74,6 +74,18 @@ reads its `ui` key to configure the launch form. **tasks**: list of task definitions contained in the process, indexed by task code. See [task definition](02-task_definition.md). +### Invalid task references + +The task codes referenced by the `entry_point` and the `end_point` of the process, and by the `outputs` and the +`error_outputs` of its tasks, must be tasks of the process. They are not checked when the container is compiled, but +when the process configuration is resolved: getting or executing the process throws an +`InvalidProcessConfigurationException` naming the process, the unknown task and where it is referenced (e.g. +`Unknown task 'missing' in the 'outputs' of the task 'read' (from process: app.import)`). + +Such a process, as any process whose configuration is not valid, is left out of the lists of processes +(`cleverage:process:list`, `ProcessConfigurationRegistryInterface::getProcessConfigurations()` used e.g. by the UI), +with an error log on the `cleverage_process` channel, instead of making them fail. + ### Main branch Only the *main branch* of the process is executed: the group of connected tasks containing the `entry_point`, or else diff --git a/src/Exception/InvalidProcessConfigurationException.php b/src/Exception/InvalidProcessConfigurationException.php index 3366fe24..8ad2f891 100644 --- a/src/Exception/InvalidProcessConfigurationException.php +++ b/src/Exception/InvalidProcessConfigurationException.php @@ -44,4 +44,19 @@ public static function createEntryPointHasAncestors( "The entry-point '{$taskConfig->getCode()}' cannot have an ancestor (from process: {$processConfiguration->getCode()})" ); } + + /** + * A task code referenced by the "outputs" or "error_outputs" of a task, or by the "entry_point" or "end_point" of + * the process, is not a task of the process. + */ + public static function createUnknownTaskReference( + string $processCode, + string $key, + string $unknownTaskCode, + ?string $taskCode = null, + ): self { + $holder = null === $taskCode ? "the '{$key}'" : "the '{$key}' of the task '{$taskCode}'"; + + return new self("Unknown task '{$unknownTaskCode}' in {$holder} (from process: {$processCode})"); + } } diff --git a/src/Registry/ProcessConfigurationRegistry.php b/src/Registry/ProcessConfigurationRegistry.php index 73b067b3..f6a82d08 100644 --- a/src/Registry/ProcessConfigurationRegistry.php +++ b/src/Registry/ProcessConfigurationRegistry.php @@ -18,6 +18,7 @@ use CleverAge\ProcessBundle\Exception\InvalidProcessConfigurationException; use CleverAge\ProcessBundle\Exception\LogicException; use CleverAge\ProcessBundle\Exception\MissingProcessException; +use Psr\Log\LoggerInterface; use Psr\Log\LogLevel; /** @@ -40,6 +41,7 @@ public function __construct( protected array $rawConfiguration, protected string $defaultErrorStrategy, protected array $defaultLogLevels = [], + protected ?LoggerInterface $logger = null, ) { } @@ -54,15 +56,29 @@ public function getProcessConfiguration(string $processCode): ProcessConfigurati } /** + * The processes whose configuration is not valid are left out (with an error log): they do not prevent listing the + * other ones, getProcessConfiguration() throws when such a process is requested (e.g. to be executed). + * * @return array */ public function getProcessConfigurations(): array { + $processConfigurations = []; foreach (array_keys($this->rawConfiguration) as $processCode) { - $this->resolveConfiguration($processCode); + try { + $this->resolveConfiguration($processCode); + } catch (InvalidProcessConfigurationException|LogicException $exception) { + $this->logger?->error( + "The process {$processCode} is left out of the process list, its configuration is not valid: {$exception->getMessage()}", + ['process_code' => $processCode, 'exception' => $exception] + ); + + continue; + } + $processConfigurations[$processCode] = $this->processConfigurations[$processCode]; } - return $this->processConfigurations; + return $processConfigurations; } /** @@ -87,6 +103,8 @@ protected function resolveConfiguration(string $processCode): void $rawProcessConfiguration = $this->rawConfiguration[$processCode]; /** @var TaskConfiguration[] $taskConfigurations */ $taskConfigurations = []; + /** @var array $errorOutputsKeys */ + $errorOutputsKeys = []; foreach ($rawProcessConfiguration['tasks'] as $taskCode => $rawTaskConfiguration) { if ((is_countable($rawTaskConfiguration['errors']) ? \count($rawTaskConfiguration['errors']) : 0) > 0) { if ((is_countable($rawTaskConfiguration['error_outputs']) ? \count( @@ -97,6 +115,7 @@ protected function resolveConfiguration(string $processCode): void throw new LogicException($m); } $rawTaskConfiguration['error_outputs'] = $rawTaskConfiguration['errors']; + $errorOutputsKeys[$taskCode] = 'errors'; } if (!\is_string($rawTaskConfiguration['service'] ?? null)) { // The "task" codes are resolved into services when the container is compiled (TaskLocatorCompilerPass) @@ -128,6 +147,9 @@ protected function resolveConfiguration(string $processCode): void $rawProcessConfiguration['logs']['failed_level'] ?? $this->defaultLogLevels['failed_level'] ?? LogLevel::DEBUG ); + // Check the task references, the links below would fail with a message naming neither the process nor the task + $this->checkTaskReferences($processCode, $rawProcessConfiguration, $taskConfigurations, $errorOutputsKeys); + // Set links between tasks foreach ($taskConfigurations as $taskConfig) { foreach ($taskConfig->getOutputs() as $nextTaskCode) { @@ -166,6 +188,37 @@ protected function resolveConfiguration(string $processCode): void $this->processConfigurations[$processCode] = $processConfig; } + /** + * @param array $rawProcessConfiguration + * @param TaskConfiguration[] $taskConfigurations indexed by task code + * @param array $errorOutputsKeys "errors" for the tasks using this deprecated key + */ + protected function checkTaskReferences( + string $processCode, + array $rawProcessConfiguration, + array $taskConfigurations, + array $errorOutputsKeys, + ): void { + foreach ($taskConfigurations as $taskCode => $taskConfig) { + foreach ($taskConfig->getOutputs() as $nextTaskCode) { + if (!\array_key_exists($nextTaskCode, $taskConfigurations)) { + throw InvalidProcessConfigurationException::createUnknownTaskReference($processCode, 'outputs', $nextTaskCode, $taskCode); + } + } + foreach ($taskConfig->getErrorOutputs() as $errorTaskCode) { + if (!\array_key_exists($errorTaskCode, $taskConfigurations)) { + throw InvalidProcessConfigurationException::createUnknownTaskReference($processCode, $errorOutputsKeys[$taskCode] ?? 'error_outputs', $errorTaskCode, $taskCode); + } + } + } + foreach (['entry_point', 'end_point'] as $key) { + $referencedTaskCode = $rawProcessConfiguration[$key] ?? null; + if (\is_string($referencedTaskCode) && !\array_key_exists($referencedTaskCode, $taskConfigurations)) { + throw InvalidProcessConfigurationException::createUnknownTaskReference($processCode, $key, $referencedTaskCode); + } + } + } + protected function markErrorBranch(TaskConfiguration $taskConfig, bool $isErrorBranch = true): void { if ($taskConfig->isInErrorBranch() !== $isErrorBranch) { diff --git a/src/Registry/ProcessConfigurationRegistryInterface.php b/src/Registry/ProcessConfigurationRegistryInterface.php index d727d8cd..42d4a638 100644 --- a/src/Registry/ProcessConfigurationRegistryInterface.php +++ b/src/Registry/ProcessConfigurationRegistryInterface.php @@ -29,9 +29,10 @@ interface ProcessConfigurationRegistryInterface public function getProcessConfiguration(string $processCode): ProcessConfiguration; /** - * @return array indexed by process code + * The processes whose configuration is not valid are left out (ProcessConfigurationRegistry logs an error), they + * do not prevent listing the other ones. * - * @throws InvalidProcessConfigurationException when a process configuration is not valid + * @return array indexed by process code */ public function getProcessConfigurations(): array; diff --git a/tests/Command/ListProcessCommandTest.php b/tests/Command/ListProcessCommandTest.php index 29982424..f8222953 100644 --- a/tests/Command/ListProcessCommandTest.php +++ b/tests/Command/ListProcessCommandTest.php @@ -16,6 +16,7 @@ use CleverAge\ProcessBundle\Command\ListProcessCommand; use CleverAge\ProcessBundle\Configuration\ProcessConfiguration; use CleverAge\ProcessBundle\Configuration\TaskConfiguration; +use CleverAge\ProcessBundle\Exception\InvalidProcessConfigurationException; use CleverAge\ProcessBundle\Registry\ProcessConfigurationRegistry; use PHPUnit\Framework\Attributes\CoversClass; use PHPUnit\Framework\Attributes\UsesClass; @@ -26,6 +27,7 @@ #[UsesClass(ProcessConfiguration::class)] #[UsesClass(TaskConfiguration::class)] #[UsesClass(ProcessConfigurationRegistry::class)] +#[UsesClass(InvalidProcessConfigurationException::class)] class ListProcessCommandTest extends TestCase { public function testThePublicProcessesAreListedSortedByCode(): void @@ -82,6 +84,8 @@ private function createTester(): CommandTester [ 'test.private' => $process('Private process', false), 'test.b' => $process('', true), + // Left out of the list (with an error log) instead of making the command fail + 'test.invalid' => ['entry_point' => 'missing'] + $process('Invalid process', true), 'test.a' => $process('First process', true, [ 'task' => [ 'service' => '@app.task', diff --git a/tests/Registry/ProcessConfigurationRegistryTest.php b/tests/Registry/ProcessConfigurationRegistryTest.php index 50e09f96..1ab28cdd 100644 --- a/tests/Registry/ProcessConfigurationRegistryTest.php +++ b/tests/Registry/ProcessConfigurationRegistryTest.php @@ -15,12 +15,16 @@ use CleverAge\ProcessBundle\Configuration\ProcessConfiguration; use CleverAge\ProcessBundle\Configuration\TaskConfiguration; +use CleverAge\ProcessBundle\Exception\InvalidProcessConfigurationException; use CleverAge\ProcessBundle\Registry\ProcessConfigurationRegistry; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\UsesClass; use PHPUnit\Framework\TestCase; +use Psr\Log\AbstractLogger; #[CoversClass(ProcessConfigurationRegistry::class)] +#[CoversClass(InvalidProcessConfigurationException::class)] #[UsesClass(ProcessConfiguration::class)] #[UsesClass(TaskConfiguration::class)] class ProcessConfigurationRegistryTest extends TestCase @@ -83,4 +87,140 @@ public function testResetForgetsTheResolvedConfigurations(): void self::assertNotSame($configuration, $resolvedAgain); self::assertSame('test.process', $resolvedAgain->getCode()); } + + /** + * @return iterable, array>, string}> + */ + public static function provideUnknownTaskReferences(): iterable + { + yield 'outputs' => [ + [], + ['read' => ['outputs' => ['missing']], 'write' => []], + "Unknown task 'missing' in the 'outputs' of the task 'read' (from process: test.process)", + ]; + yield 'error_outputs' => [ + [], + ['read' => ['outputs' => ['write'], 'error_outputs' => ['missing']], 'write' => []], + "Unknown task 'missing' in the 'error_outputs' of the task 'read' (from process: test.process)", + ]; + yield 'deprecated errors' => [ + [], + ['read' => ['errors' => ['missing']], 'write' => []], + "Unknown task 'missing' in the 'errors' of the task 'read' (from process: test.process)", + ]; + yield 'entry_point' => [ + ['entry_point' => 'missing'], + ['read' => ['outputs' => ['write']], 'write' => []], + "Unknown task 'missing' in the 'entry_point' (from process: test.process)", + ]; + yield 'end_point' => [ + ['entry_point' => 'read', 'end_point' => 'missing'], + ['read' => ['outputs' => ['write']], 'write' => []], + "Unknown task 'missing' in the 'end_point' (from process: test.process)", + ]; + } + + /** + * @param array $processConfiguration + * @param array> $tasks + */ + #[DataProvider('provideUnknownTaskReferences')] + public function testUnknownTaskReferenceNamesTheProcessAndTheTask(array $processConfiguration, array $tasks, string $message): void + { + $registry = new ProcessConfigurationRegistry(['test.process' => $this->rawProcess($tasks, $processConfiguration)], 'stop'); + + $this->expectException(InvalidProcessConfigurationException::class); + $this->expectExceptionMessage($message); + + $registry->getProcessConfiguration('test.process'); + } + + public function testListingLeavesOutAndLogsTheInvalidProcesses(): void + { + $logger = new class extends AbstractLogger { + /** @var list}> */ + public array $records = []; + + public function log($level, \Stringable|string $message, array $context = []): void + { + $this->records[] = [$level, (string) $message, $context]; + } + }; + $registry = new ProcessConfigurationRegistry([ + 'test.valid' => $this->rawProcess(['read' => ['outputs' => ['write']], 'write' => []]), + 'test.unknown_output' => $this->rawProcess(['read' => ['outputs' => ['missing']]]), + 'test.no_service' => $this->rawProcess(['read' => ['service' => null]]), + 'test.other' => $this->rawProcess(['read' => []]), + ], 'stop', [], $logger); + + $configurations = $registry->getProcessConfigurations(); + + self::assertSame(['test.valid', 'test.other'], array_keys($configurations)); + self::assertSame(['test.valid', 'test.other'], array_keys($registry->getProcessConfigurations())); + self::assertCount(4, $logger->records); + [$level, $message, $context] = $logger->records[0]; + self::assertSame('error', $level); + self::assertSame("The process test.unknown_output is left out of the process list, its configuration is not valid: Unknown task 'missing' in the 'outputs' of the task 'read' (from process: test.unknown_output)", $message); + self::assertSame('test.unknown_output', $context['process_code']); + self::assertInstanceOf(InvalidProcessConfigurationException::class, $context['exception']); + self::assertStringStartsWith('The process test.no_service is left out of the process list', $logger->records[1][1]); + + // Requested by its code (e.g. to be executed), the invalid process still throws + $this->expectException(InvalidProcessConfigurationException::class); + $this->expectExceptionMessage("Unknown task 'missing' in the 'outputs' of the task 'read' (from process: test.unknown_output)"); + $registry->getProcessConfiguration('test.unknown_output'); + } + + public function testListingWithoutLogger(): void + { + $registry = new ProcessConfigurationRegistry([ + 'test.invalid' => $this->rawProcess(['read' => []], ['entry_point' => 'missing']), + 'test.valid' => $this->rawProcess(['read' => []]), + ], 'stop'); + + self::assertSame(['test.valid'], array_keys($registry->getProcessConfigurations())); + } + + public function testListingKeepsTheConfigurationsResolvedBefore(): void + { + $registry = new ProcessConfigurationRegistry([ + 'test.a' => $this->rawProcess(['read' => []]), + 'test.b' => $this->rawProcess(['read' => []]), + ], 'stop'); + $configuration = $registry->getProcessConfiguration('test.b'); + + $configurations = $registry->getProcessConfigurations(); + + self::assertSame(['test.a', 'test.b'], array_keys($configurations)); + self::assertSame($configuration, $configurations['test.b']); + } + + /** + * @param array> $tasks + * @param array $processConfiguration + * + * @return array + */ + private function rawProcess(array $tasks, array $processConfiguration = []): array + { + return $processConfiguration + [ + 'options' => [], + 'entry_point' => null, + 'end_point' => null, + 'description' => '', + 'help' => '', + 'public' => true, + 'tasks' => array_map(static fn (array $task): array => $task + [ + 'service' => '@app.task', + 'options' => [], + 'description' => '', + 'help' => '', + 'outputs' => [], + 'errors' => [], + 'error_outputs' => [], + 'error_strategy' => null, + 'log_level' => null, + ], $tasks), + ]; + } }