diff --git a/CHANGELOG.md b/CHANGELOG.md index ada64a1..5d0a2bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ Latest ### Changes * [#31](https://github.com/cleverage/rest-process-bundle/issues/31) Add missing tests: RequestTask and Client with a mocked HTTP client, MissingClientException, bundle and DI extension. * [#39](https://github.com/cleverage/rest-process-bundle/issues/39) Add `Client::getUri()`, deprecate the misspelled `geUri()`; type `MissingClientException::create()`. Update documentation, add tests. +* [#33](https://github.com/cleverage/rest-process-bundle/issues/33) Give the ids of both services in the error on duplicate client codes: the clients are registered by a compiler pass of the bundle, `ClientRegistry::addClient()` gets an optional `$serviceId` argument. Update documentation, add tests. ### Fixes * [#35](https://github.com/cleverage/rest-process-bundle/issues/35) Fix RequestTask: a `3xx` / `4xx` / `5xx` status code listed in `valid_response_code` outputs the response body (the task still failed). Update documentation, add tests. diff --git a/docs/reference/client.md b/docs/reference/client.md index 4a23290..183630e 100644 --- a/docs/reference/client.md +++ b/docs/reference/client.md @@ -70,8 +70,10 @@ Registration ------------ Every service tagged `cleverage.rest.client` is added to the registry (a compiler pass calls -`ClientRegistry::addClient()` for each of them). Two clients with the same code throw an `UnexpectedValueException` -(`Client is already defined`) when the registry is instantiated; a task referencing an unknown code throws a +`ClientRegistry::addClient()` for each of them, with the service id). Two clients with the same code throw an +`UnexpectedValueException` giving the ids of both services +(`Client is already defined by service "", cannot register service ""`) when the registry is +instantiated; a task referencing an unknown code throws a `CleverAge\RestProcessBundle\Exception\MissingClientException` (`No rest client with code : `). Implementing a client diff --git a/src/CleverAgeRestProcessBundle.php b/src/CleverAgeRestProcessBundle.php index 78f0d68..34e31cb 100644 --- a/src/CleverAgeRestProcessBundle.php +++ b/src/CleverAgeRestProcessBundle.php @@ -13,7 +13,7 @@ namespace CleverAge\RestProcessBundle; -use CleverAge\ProcessBundle\DependencyInjection\Compiler\RegistryCompilerPass; +use CleverAge\RestProcessBundle\DependencyInjection\Compiler\RegisterClientsPass; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpKernel\Bundle\Bundle; @@ -24,13 +24,7 @@ class CleverAgeRestProcessBundle extends Bundle */ public function build(ContainerBuilder $container): void { - $container->addCompilerPass( - new RegistryCompilerPass( - 'cleverage_rest_process.registry.client', - 'cleverage.rest.client', - 'addClient' - ) - ); + $container->addCompilerPass(new RegisterClientsPass()); } #[\Override] diff --git a/src/DependencyInjection/Compiler/RegisterClientsPass.php b/src/DependencyInjection/Compiler/RegisterClientsPass.php new file mode 100644 index 0000000..81a3aad --- /dev/null +++ b/src/DependencyInjection/Compiler/RegisterClientsPass.php @@ -0,0 +1,36 @@ +has('cleverage_rest_process.registry.client')) { + return; + } + + $definition = $container->findDefinition('cleverage_rest_process.registry.client'); + foreach (array_keys($container->findTaggedServiceIds('cleverage.rest.client')) as $id) { + $definition->addMethodCall('addClient', [new Reference($id), $id]); + } + } +} diff --git a/src/Registry/ClientRegistry.php b/src/Registry/ClientRegistry.php index 828852c..54c0c2a 100644 --- a/src/Registry/ClientRegistry.php +++ b/src/Registry/ClientRegistry.php @@ -24,12 +24,25 @@ class ClientRegistry /** @var ClientInterface[] */ private array $clients = []; - public function addClient(ClientInterface $client): void + /** @var array Service ids of the clients, indexed by code */ + private array $serviceIds = []; + + /** + * @param string|null $serviceId Id of the client service, used to identify the clients with the same code + */ + public function addClient(ClientInterface $client, ?string $serviceId = null): void { - if (\array_key_exists($client->getCode(), $this->getClients())) { - throw new \UnexpectedValueException("Client {$client->getCode()} is already defined"); + $code = $client->getCode(); + if (\array_key_exists($code, $this->getClients())) { + $message = "Client {$code} is already defined"; + if (null !== $this->serviceIds[$code] && null !== $serviceId) { + $message .= " by service \"{$this->serviceIds[$code]}\", cannot register service \"{$serviceId}\""; + } + + throw new \UnexpectedValueException($message); } - $this->clients[$client->getCode()] = $client; + $this->clients[$code] = $client; + $this->serviceIds[$code] = $serviceId; } /** diff --git a/tests/DependencyInjection/Compiler/RegisterClientsPassTest.php b/tests/DependencyInjection/Compiler/RegisterClientsPassTest.php new file mode 100644 index 0000000..76fe226 --- /dev/null +++ b/tests/DependencyInjection/Compiler/RegisterClientsPassTest.php @@ -0,0 +1,77 @@ +createContainer(['app.client' => 'client', 'app.other' => 'other']); + $container->compile(true); + + /** @var ClientRegistry $registry */ + $registry = $container->get('cleverage_rest_process.registry.client'); + self::assertSame('client', $registry->getClient('client')->getCode()); + self::assertSame('other', $registry->getClient('other')->getCode()); + } + + public function testDuplicateCodeGivesTheServiceIds(): void + { + $container = $this->createContainer(['app.client' => 'client', 'app.client_duplicate' => 'client']); + $container->compile(true); + + $this->expectException(\UnexpectedValueException::class); + $this->expectExceptionMessage('Client client is already defined by service "app.client", cannot register service "app.client_duplicate"'); + $container->get('cleverage_rest_process.registry.client'); + } + + public function testWithoutRegistry(): void + { + $container = new ContainerBuilder(); + (new RegisterClientsPass())->process($container); + + self::assertFalse($container->has('cleverage_rest_process.registry.client')); + } + + /** + * @param array $clients Codes of the clients, indexed by service id + */ + private function createContainer(array $clients): ContainerBuilder + { + $container = new ContainerBuilder(); + $container->addCompilerPass(new RegisterClientsPass()); + $container->setDefinition('cleverage_rest_process.registry.client', new Definition(ClientRegistry::class)) + ->setPublic(true); + foreach ($clients as $id => $code) { + $container->setDefinition($id, new Definition(Client::class, [new Definition(MockHttpClient::class), new Definition(NullLogger::class), $code, 'https://example.com'])) + ->addTag('cleverage.rest.client'); + } + + return $container; + } +} diff --git a/tests/Registry/ClientRegistryTest.php b/tests/Registry/ClientRegistryTest.php new file mode 100644 index 0000000..56caf9f --- /dev/null +++ b/tests/Registry/ClientRegistryTest.php @@ -0,0 +1,75 @@ +createClient('client'); + $other = $this->createClient('other'); + $registry->addClient($client, 'app.client'); + $registry->addClient($other); + + self::assertSame($client, $registry->getClient('client')); + self::assertSame($other, $registry->getClient('other')); + self::assertTrue($registry->hasClient('client')); + self::assertSame(['client' => $client, 'other' => $other], $registry->getClients()); + } + + public function testMissingClient(): void + { + $this->expectException(MissingClientException::class); + $this->expectExceptionMessage('No rest client with code : missing'); + (new ClientRegistry())->getClient('missing'); + } + + public function testDuplicateCodeGivesTheServiceIds(): void + { + $registry = new ClientRegistry(); + $registry->addClient($this->createClient('client'), 'app.client'); + + $this->expectException(\UnexpectedValueException::class); + $this->expectExceptionMessage('Client client is already defined by service "app.client", cannot register service "app.client_duplicate"'); + $registry->addClient($this->createClient('client'), 'app.client_duplicate'); + } + + public function testDuplicateCodeWithoutServiceIds(): void + { + $registry = new ClientRegistry(); + $registry->addClient($this->createClient('client')); + + $this->expectException(\UnexpectedValueException::class); + $this->expectExceptionMessageMatches('/^Client client is already defined$/'); + $registry->addClient($this->createClient('client'), 'app.client_duplicate'); + } + + private function createClient(string $code): ClientInterface + { + $client = $this->createStub(ClientInterface::class); + $client->method('getCode')->willReturn($code); + + return $client; + } +}