From 8298ef36f8191461dd7baa45e2d29e9239152ba8 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 18 Nov 2024 09:51:27 +0100 Subject: [PATCH] Use more meaningful domain exceptions to represent ApiKeyService thrown errors --- .../src/Exception/ApiKeyConflictException.php | 15 ++++++++++ .../src/Exception/ApiKeyNotFoundException.php | 21 +++++++++++++ module/Rest/src/Service/ApiKeyService.php | 30 +++++++++++-------- .../src/Service/ApiKeyServiceInterface.php | 9 ++++-- .../Rest/test/Service/ApiKeyServiceTest.php | 12 ++++---- 5 files changed, 66 insertions(+), 21 deletions(-) create mode 100644 module/Rest/src/Exception/ApiKeyConflictException.php create mode 100644 module/Rest/src/Exception/ApiKeyNotFoundException.php diff --git a/module/Rest/src/Exception/ApiKeyConflictException.php b/module/Rest/src/Exception/ApiKeyConflictException.php new file mode 100644 index 00000000..a1ffce03 --- /dev/null +++ b/module/Rest/src/Exception/ApiKeyConflictException.php @@ -0,0 +1,15 @@ +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; diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index be7b9191..745355d7 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/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; } diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index bf80ae60..81bec4ea 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/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); }