Merge pull request #2271 from acelaya-forks/feature/api-key-domain-exceptions

Use more meaningful domain exceptions to represent ApiKeyService thrown errors

module/Rest/src/Exception/ApiKeyConflictException.php

@@ -0,0 +1,15 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Rest\Exception;
+
+use function sprintf;
+
+class ApiKeyConflictException extends RuntimeException implements ExceptionInterface
+{
+    public static function forName(string $name): self
+    {
+        return new self(sprintf('An API key with name "%s" already exists', $name));
+    }
+}

module/Rest/src/Exception/ApiKeyNotFoundException.php

@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Rest\Exception;
+
+use function sprintf;
+
+class ApiKeyNotFoundException extends RuntimeException implements ExceptionInterface
+{
+    public static function forName(string $name): self
+    {
+        return new self(sprintf('API key with name "%s" not found', $name));
+    }
+
+    /** @deprecated */
+    public static function forKey(string $key): self
+    {
+        return new self(sprintf('API key with key "%s" not found', $key));
+    }
+}

module/Rest/src/Service/ApiKeyService.php

@@ -10,6 +10,8 @@ use Shlinkio\Shlink\Core\Model\Renaming;
 use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
 use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
+use Shlinkio\Shlink\Rest\Exception\ApiKeyConflictException;
+use Shlinkio\Shlink\Rest\Exception\ApiKeyNotFoundException;
 
 use function sprintf;
 
@@ -71,7 +73,12 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
      */
     public function disableByName(string $apiKeyName): ApiKey
     {
-        return $this->disableApiKey($this->repo->findOneBy(['name' => $apiKeyName]));
+        $apiKey = $this->repo->findOneBy(['name' => $apiKeyName]);
+        if ($apiKey === null) {
+            throw ApiKeyNotFoundException::forName($apiKeyName);
+        }
+
+        return $this->disableApiKey($apiKey);
     }
 
     /**
@@ -79,15 +86,16 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
      */
     public function disableByKey(string $key): ApiKey
     {
-        return $this->disableApiKey($this->findByKey($key));
-    }
-
-    private function disableApiKey(ApiKey|null $apiKey): ApiKey
-    {
+        $apiKey = $this->findByKey($key);
         if ($apiKey === null) {
-            throw new InvalidArgumentException('Provided API key does not exist and can\'t be disabled');
+            throw ApiKeyNotFoundException::forKey($key);
         }
 
+        return $this->disableApiKey($apiKey);
+    }
+
+    private function disableApiKey(ApiKey $apiKey): ApiKey
+    {
         $apiKey->disable();
         $this->em->flush();
 
@@ -110,9 +118,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
     {
         $apiKey = $this->repo->findOneBy(['name' => $apiKeyRenaming->oldName]);
         if ($apiKey === null) {
-            throw new InvalidArgumentException(
-                sprintf('API key with name "%s" could not be found', $apiKeyRenaming->oldName),
-            );
+            throw ApiKeyNotFoundException::forName($apiKeyRenaming->oldName);
         }
 
         if (! $apiKeyRenaming->nameChanged()) {
@@ -121,9 +127,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
 
         $this->em->wrapInTransaction(function () use ($apiKeyRenaming, $apiKey): void {
             if ($this->repo->nameExists($apiKeyRenaming->newName)) {
-                throw new InvalidArgumentException(
-                    sprintf('Another API key with name "%s" already exists', $apiKeyRenaming->newName),
-                );
+                throw ApiKeyConflictException::forName($apiKeyRenaming->newName);
             }
 
             $apiKey->name = $apiKeyRenaming->newName;

module/Rest/src/Service/ApiKeyServiceInterface.php

@@ -8,6 +8,8 @@ use Shlinkio\Shlink\Common\Exception\InvalidArgumentException;
 use Shlinkio\Shlink\Core\Model\Renaming;
 use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
+use Shlinkio\Shlink\Rest\Exception\ApiKeyConflictException;
+use Shlinkio\Shlink\Rest\Exception\ApiKeyNotFoundException;
 
 interface ApiKeyServiceInterface
 {
@@ -21,13 +23,13 @@ interface ApiKeyServiceInterface
     public function check(string $key): ApiKeyCheckResult;
 
     /**
-     * @throws InvalidArgumentException
+     * @throws ApiKeyNotFoundException
      */
     public function disableByName(string $apiKeyName): ApiKey;
 
     /**
      * @deprecated Use `self::disableByName($name)` instead
-     * @throws InvalidArgumentException
+     * @throws ApiKeyNotFoundException
      */
     public function disableByKey(string $key): ApiKey;
 
@@ -37,7 +39,8 @@ interface ApiKeyServiceInterface
     public function listKeys(bool $enabledOnly = false): array;
 
     /**
-     * @throws InvalidArgumentException If an API key with oldName does not exist, or newName is in use by another one
+     * @throws ApiKeyNotFoundException
+     * @throws ApiKeyConflictException
      */
     public function renameApiKey(Renaming $apiKeyRenaming): ApiKey;
 }

module/Rest/test/Service/ApiKeyServiceTest.php

@@ -17,6 +17,8 @@ use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
 use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition;
 use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
+use Shlinkio\Shlink\Rest\Exception\ApiKeyConflictException;
+use Shlinkio\Shlink\Rest\Exception\ApiKeyNotFoundException;
 use Shlinkio\Shlink\Rest\Service\ApiKeyService;
 
 use function substr;
@@ -145,7 +147,7 @@ class ApiKeyServiceTest extends TestCase
     {
         $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn(null);
 
-        $this->expectException(InvalidArgumentException::class);
+        $this->expectException(ApiKeyNotFoundException::class);
 
         $this->service->{$disableMethod}('12345');
     }
@@ -217,8 +219,8 @@ class ApiKeyServiceTest extends TestCase
         $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'old'])->willReturn(null);
         $this->repo->expects($this->never())->method('nameExists');
 
-        $this->expectException(InvalidArgumentException::class);
-        $this->expectExceptionMessage('API key with name "old" could not be found');
+        $this->expectException(ApiKeyNotFoundException::class);
+        $this->expectExceptionMessage('API key with name "old" not found');
 
         $this->service->renameApiKey($renaming);
     }
@@ -246,8 +248,8 @@ class ApiKeyServiceTest extends TestCase
         $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'old'])->willReturn($apiKey);
         $this->repo->expects($this->once())->method('nameExists')->with('new')->willReturn(true);
 
-        $this->expectException(InvalidArgumentException::class);
-        $this->expectExceptionMessage('Another API key with name "new" already exists');
+        $this->expectException(ApiKeyConflictException::class);
+        $this->expectExceptionMessage('An API key with name "new" already exists');
 
         $this->service->renameApiKey($renaming);
     }