feat(registry) #33 Give the service ids in the error on duplicate client codes

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.

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 <code> 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 <code> is already defined by service "<id>", cannot register service "<id>"`) when the registry is
+instantiated; a task referencing an unknown code throws a
 `CleverAge\RestProcessBundle\Exception\MissingClientException` (`No rest client with code : <code>`).
 
 Implementing a client

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]

src/DependencyInjection/Compiler/RegisterClientsPass.php

@@ -0,0 +1,36 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/RestProcessBundle 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\RestProcessBundle\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+/**
+ * Adds the tagged REST clients to the client registry, with their service id.
+ */
+class RegisterClientsPass implements CompilerPassInterface
+{
+    public function process(ContainerBuilder $container): void
+    {
+        if (!$container->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]);
+        }
+    }
+}

src/Registry/ClientRegistry.php

@@ -24,12 +24,25 @@ class ClientRegistry
     /** @var ClientInterface[] */
     private array $clients = [];
 
-    public function addClient(ClientInterface $client): void
+    /** @var array<string, string|null> 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;
     }
 
     /**

tests/DependencyInjection/Compiler/RegisterClientsPassTest.php

@@ -0,0 +1,77 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/RestProcessBundle 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\RestProcessBundle\Tests\DependencyInjection\Compiler;
+
+use CleverAge\RestProcessBundle\Client\Client;
+use CleverAge\RestProcessBundle\DependencyInjection\Compiler\RegisterClientsPass;
+use CleverAge\RestProcessBundle\Registry\ClientRegistry;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\HttpClient\MockHttpClient;
+
+#[CoversClass(RegisterClientsPass::class)]
+#[UsesClass(ClientRegistry::class)]
+#[UsesClass(Client::class)]
+class RegisterClientsPassTest extends TestCase
+{
+    public function testClientsAreRegistered(): void
+    {
+        $container = $this->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<string, string> $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;
+    }
+}

tests/Registry/ClientRegistryTest.php

@@ -0,0 +1,75 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/RestProcessBundle 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\RestProcessBundle\Tests\Registry;
+
+use CleverAge\RestProcessBundle\Client\ClientInterface;
+use CleverAge\RestProcessBundle\Exception\MissingClientException;
+use CleverAge\RestProcessBundle\Registry\ClientRegistry;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\TestCase;
+
+#[CoversClass(ClientRegistry::class)]
+#[UsesClass(MissingClientException::class)]
+class ClientRegistryTest extends TestCase
+{
+    public function testGetClient(): void
+    {
+        $registry = new ClientRegistry();
+        $client = $this->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;
+    }
+}