From e8f7daac6f0c7982c16b3209a7b5844972c4cfd5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 23 Apr 2022 18:41:16 +0200 Subject: [PATCH] Converted Role constants to enum --- .../src/Command/Api/GenerateKeyCommand.php | 9 ++++-- .../CLI/src/Command/Api/ListKeysCommand.php | 6 ++-- .../Exception/InvalidRoleConfigException.php | 2 +- .../InvalidRoleConfigExceptionTest.php | 2 +- .../Domain/Repository/DomainRepository.php | 4 +-- module/Core/src/Repository/TagRepository.php | 3 +- ...Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php | 15 ++++++---- .../Rest/src/ApiKey/Model/RoleDefinition.php | 2 +- module/Rest/src/ApiKey/Role.php | 26 ++++++++-------- module/Rest/src/Entity/ApiKey.php | 30 +++++++++---------- module/Rest/src/Entity/ApiKeyRole.php | 11 +++++-- .../test/ApiKey/Model/RoleDefinitionTest.php | 4 +-- module/Rest/test/ApiKey/RoleTest.php | 9 ++---- .../Rest/test/Service/ApiKeyServiceTest.php | 2 +- 14 files changed, 66 insertions(+), 59 deletions(-) diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index 2655d1fb..b24619ef 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -73,13 +73,16 @@ class GenerateKeyCommand extends Command $authorOnly, 'a', InputOption::VALUE_NONE, - sprintf('Adds the "%s" role to the new API key.', Role::AUTHORED_SHORT_URLS), + sprintf('Adds the "%s" role to the new API key.', Role::AUTHORED_SHORT_URLS->value), ) ->addOption( $domainOnly, 'd', InputOption::VALUE_REQUIRED, - sprintf('Adds the "%s" role to the new API key, with the domain provided.', Role::DOMAIN_SPECIFIC), + sprintf( + 'Adds the "%s" role to the new API key, with the domain provided.', + Role::DOMAIN_SPECIFIC->value, + ), ) ->setHelp($help); } @@ -99,7 +102,7 @@ class GenerateKeyCommand extends Command if (! $apiKey->isAdmin()) { ShlinkTable::default($io)->render( ['Role name', 'Role metadata'], - $apiKey->mapRoles(fn (string $name, array $meta) => [$name, arrayToString($meta, 0)]), + $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, 0)]), null, 'Roles', ); diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php index 0a331086..0e98af31 100644 --- a/module/CLI/src/Command/Api/ListKeysCommand.php +++ b/module/CLI/src/Command/Api/ListKeysCommand.php @@ -60,10 +60,10 @@ class ListKeysCommand extends Command } $rowData[] = $expiration?->toAtomString() ?? '-'; $rowData[] = $apiKey->isAdmin() ? 'Admin' : implode("\n", $apiKey->mapRoles( - fn (string $roleName, array $meta) => + fn (Role $role, array $meta) => empty($meta) - ? Role::toFriendlyName($roleName) - : sprintf('%s: %s', Role::toFriendlyName($roleName), Role::domainAuthorityFromMeta($meta)), + ? Role::toFriendlyName($role) + : sprintf('%s: %s', Role::toFriendlyName($role), Role::domainAuthorityFromMeta($meta)), )); return $rowData; diff --git a/module/CLI/src/Exception/InvalidRoleConfigException.php b/module/CLI/src/Exception/InvalidRoleConfigException.php index 51adb234..ae483766 100644 --- a/module/CLI/src/Exception/InvalidRoleConfigException.php +++ b/module/CLI/src/Exception/InvalidRoleConfigException.php @@ -16,7 +16,7 @@ class InvalidRoleConfigException extends InvalidArgumentException implements Exc return new self(sprintf( 'You cannot create an API key with the "%s" role attached to the default domain. ' . 'The role is currently limited to non-default domains.', - Role::DOMAIN_SPECIFIC, + Role::DOMAIN_SPECIFIC->value, )); } } diff --git a/module/CLI/test/Exception/InvalidRoleConfigExceptionTest.php b/module/CLI/test/Exception/InvalidRoleConfigExceptionTest.php index 3b89b505..99c66ea4 100644 --- a/module/CLI/test/Exception/InvalidRoleConfigExceptionTest.php +++ b/module/CLI/test/Exception/InvalidRoleConfigExceptionTest.php @@ -20,7 +20,7 @@ class InvalidRoleConfigExceptionTest extends TestCase self::assertEquals(sprintf( 'You cannot create an API key with the "%s" role attached to the default domain. ' . 'The role is currently limited to non-default domains.', - Role::DOMAIN_SPECIFIC, + Role::DOMAIN_SPECIFIC->value, ), $e->getMessage()); } } diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 0a99b3c6..60c32499 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -7,7 +7,6 @@ namespace Shlinkio\Shlink\Core\Domain\Repository; use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; -use Happyr\DoctrineSpecification\Spec; use Shlinkio\Shlink\Core\Domain\Spec\IsDomain; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; @@ -77,10 +76,9 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe // FIXME The $apiKey->spec() method cannot be used here, as it returns a single spec which assumes the // ShortUrl is the root entity. Here, the Domain is the root entity. // Think on a way to centralize the conditional behavior and make $apiKey->spec() more flexible. - yield from $apiKey?->mapRoles(fn (string $roleName, array $meta) => match ($roleName) { + yield from $apiKey?->mapRoles(fn (Role $role, array $meta) => match ($role) { Role::DOMAIN_SPECIFIC => ['d', new IsDomain(Role::domainIdFromMeta($meta))], Role::AUTHORED_SHORT_URLS => ['s', new BelongsToApiKey($apiKey)], - default => [null, Spec::andX()], }) ?? []; } } diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index 47dd0cf5..2a144b38 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -81,14 +81,13 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito ->groupBy('t.id_0', 't.name_1'); // Apply API key role conditions to the native query too, as they will affect the amounts on the aggregates - $apiKey?->mapRoles(static fn (string $roleName, array $meta) => match ($roleName) { + $apiKey?->mapRoles(static fn (Role $role, array $meta) => match ($role) { Role::DOMAIN_SPECIFIC => $nativeQb->andWhere( $nativeQb->expr()->eq('s.domain_id', $conn->quote(Role::domainIdFromMeta($meta))), ), Role::AUTHORED_SHORT_URLS => $nativeQb->andWhere( $nativeQb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), ), - default => $nativeQb, }); if ($orderMainQuery) { diff --git a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php index b7787b1a..8df324a4 100644 --- a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php +++ b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php @@ -6,7 +6,9 @@ namespace Shlinkio\Shlink\Rest; use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\Builder\ClassMetadataBuilder; +use Doctrine\ORM\Mapping\Builder\FieldBuilder; use Doctrine\ORM\Mapping\ClassMetadata; +use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function Shlinkio\Shlink\Core\determineTableName; @@ -22,11 +24,14 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->option('unsigned', true) ->build(); - $builder->createField('roleName', Types::STRING) - ->columnName('role_name') - ->length(255) - ->nullable(false) - ->build(); + (new FieldBuilder($builder, [ + 'fieldName' => 'roleName', + 'type' => Types::STRING, + 'enumType' => Role::class, + ]))->columnName('role_name') + ->length(255) + ->nullable(false) + ->build(); $builder->createField('meta', Types::JSON) ->columnName('meta') diff --git a/module/Rest/src/ApiKey/Model/RoleDefinition.php b/module/Rest/src/ApiKey/Model/RoleDefinition.php index 20061cbc..63c9b72a 100644 --- a/module/Rest/src/ApiKey/Model/RoleDefinition.php +++ b/module/Rest/src/ApiKey/Model/RoleDefinition.php @@ -9,7 +9,7 @@ use Shlinkio\Shlink\Rest\ApiKey\Role; final class RoleDefinition { - private function __construct(public readonly string $roleName, public readonly array $meta) + private function __construct(public readonly Role $role, public readonly array $meta) { } diff --git a/module/Rest/src/ApiKey/Role.php b/module/Rest/src/ApiKey/Role.php index 28291837..64803969 100644 --- a/module/Rest/src/ApiKey/Role.php +++ b/module/Rest/src/ApiKey/Role.php @@ -2,6 +2,8 @@ declare(strict_types=1); +// phpcs:disable +// TODO Enable coding style checks again once code sniffer 3.7 is released https://github.com/squizlabs/PHP_CodeSniffer/issues/3474 namespace Shlinkio\Shlink\Rest\ApiKey; use Happyr\DoctrineSpecification\Spec; @@ -12,31 +14,24 @@ use Shlinkio\Shlink\Core\ShortUrl\Spec\BelongsToDomain; use Shlinkio\Shlink\Core\ShortUrl\Spec\BelongsToDomainInlined; use Shlinkio\Shlink\Rest\Entity\ApiKeyRole; -// TODO Convert to enum -class Role +enum Role: string { - public const AUTHORED_SHORT_URLS = 'AUTHORED_SHORT_URLS'; - public const DOMAIN_SPECIFIC = 'DOMAIN_SPECIFIC'; - private const ROLE_FRIENDLY_NAMES = [ - self::AUTHORED_SHORT_URLS => 'Author only', - self::DOMAIN_SPECIFIC => 'Domain only', - ]; + case AUTHORED_SHORT_URLS = 'AUTHORED_SHORT_URLS'; + case DOMAIN_SPECIFIC = 'DOMAIN_SPECIFIC'; public static function toSpec(ApiKeyRole $role, ?string $context = null): Specification { - return match ($role->name()) { + return match ($role->role()) { self::AUTHORED_SHORT_URLS => new BelongsToApiKey($role->apiKey(), $context), self::DOMAIN_SPECIFIC => new BelongsToDomain(self::domainIdFromMeta($role->meta()), $context), - default => Spec::andX(), }; } public static function toInlinedSpec(ApiKeyRole $role): Specification { - return match ($role->name()) { + return match ($role->role()) { self::AUTHORED_SHORT_URLS => Spec::andX(new BelongsToApiKeyInlined($role->apiKey())), self::DOMAIN_SPECIFIC => Spec::andX(new BelongsToDomainInlined(self::domainIdFromMeta($role->meta()))), - default => Spec::andX(), }; } @@ -50,8 +45,11 @@ class Role return $meta['authority'] ?? ''; } - public static function toFriendlyName(string $roleName): string + public static function toFriendlyName(Role $role): string { - return self::ROLE_FRIENDLY_NAMES[$roleName] ?? ''; + return match ($role) { + self::AUTHORED_SHORT_URLS => 'Author only', + self::DOMAIN_SPECIFIC => 'Domain only', + }; } } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index cd0aa4ff..261baee4 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -113,40 +113,40 @@ class ApiKey extends AbstractEntity return $this->roles->isEmpty(); } - public function hasRole(string $roleName): bool + public function hasRole(Role $role): bool { - return $this->roles->containsKey($roleName); + return $this->roles->containsKey($role->value); } - public function getRoleMeta(string $roleName): array + public function getRoleMeta(Role $role): array { - /** @var ApiKeyRole|null $role */ - $role = $this->roles->get($roleName); - return $role?->meta() ?? []; + /** @var ApiKeyRole|null $apiKeyRole */ + $apiKeyRole = $this->roles->get($role->value); + return $apiKeyRole?->meta() ?? []; } /** * @template T - * @param callable(string $roleName, array $meta): T $fun + * @param callable(Role $role, array $meta): T $fun * @return T[] */ public function mapRoles(callable $fun): array { - return $this->roles->map(fn (ApiKeyRole $role) => $fun($role->name(), $role->meta()))->getValues(); + return $this->roles->map(fn (ApiKeyRole $role) => $fun($role->role(), $role->meta()))->getValues(); } public function registerRole(RoleDefinition $roleDefinition): void { - $roleName = $roleDefinition->roleName; + $role = $roleDefinition->role; $meta = $roleDefinition->meta; - if ($this->hasRole($roleName)) { - /** @var ApiKeyRole $role */ - $role = $this->roles->get($roleName); - $role->updateMeta($meta); + if ($this->hasRole($role)) { + /** @var ApiKeyRole $apiKeyRole */ + $apiKeyRole = $this->roles->get($role); + $apiKeyRole->updateMeta($meta); } else { - $role = new ApiKeyRole($roleDefinition->roleName, $roleDefinition->meta, $this); - $this->roles[$roleName] = $role; + $apiKeyRole = new ApiKeyRole($roleDefinition->role, $roleDefinition->meta, $this); + $this->roles[$role->value] = $apiKeyRole; } } } diff --git a/module/Rest/src/Entity/ApiKeyRole.php b/module/Rest/src/Entity/ApiKeyRole.php index 1155c37b..607242e5 100644 --- a/module/Rest/src/Entity/ApiKeyRole.php +++ b/module/Rest/src/Entity/ApiKeyRole.php @@ -5,18 +5,25 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Entity; use Shlinkio\Shlink\Common\Entity\AbstractEntity; +use Shlinkio\Shlink\Rest\ApiKey\Role; class ApiKeyRole extends AbstractEntity { - public function __construct(private string $roleName, private array $meta, private ApiKey $apiKey) + public function __construct(private Role $roleName, private array $meta, private ApiKey $apiKey) { } - public function name(): string + public function role(): Role { return $this->roleName; } + /** @deprecated Use role() instead */ + public function name(): Role + { + return $this->role(); + } + public function meta(): array { return $this->meta; diff --git a/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php b/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php index de0763a4..ba27a02f 100644 --- a/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php +++ b/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php @@ -16,7 +16,7 @@ class RoleDefinitionTest extends TestCase { $definition = RoleDefinition::forAuthoredShortUrls(); - self::assertEquals(Role::AUTHORED_SHORT_URLS, $definition->roleName); + self::assertEquals(Role::AUTHORED_SHORT_URLS, $definition->role); self::assertEquals([], $definition->meta); } @@ -26,7 +26,7 @@ class RoleDefinitionTest extends TestCase $domain = Domain::withAuthority('foo.com')->setId('123'); $definition = RoleDefinition::forDomain($domain); - self::assertEquals(Role::DOMAIN_SPECIFIC, $definition->roleName); + self::assertEquals(Role::DOMAIN_SPECIFIC, $definition->role); self::assertEquals(['domain_id' => '123', 'authority' => 'foo.com'], $definition->meta); } } diff --git a/module/Rest/test/ApiKey/RoleTest.php b/module/Rest/test/ApiKey/RoleTest.php index 7ee23076..f3cc64b2 100644 --- a/module/Rest/test/ApiKey/RoleTest.php +++ b/module/Rest/test/ApiKey/RoleTest.php @@ -30,7 +30,6 @@ class RoleTest extends TestCase { $apiKey = ApiKey::create(); - yield 'invalid role' => [new ApiKeyRole('invalid', [], $apiKey), Spec::andX()]; yield 'author role' => [ new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey), new BelongsToApiKey($apiKey), @@ -54,7 +53,6 @@ class RoleTest extends TestCase { $apiKey = ApiKey::create(); - yield 'invalid role' => [new ApiKeyRole('invalid', [], $apiKey), Spec::andX()]; yield 'author role' => [ new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey), Spec::andX(new BelongsToApiKeyInlined($apiKey)), @@ -101,15 +99,14 @@ class RoleTest extends TestCase * @test * @dataProvider provideRoleNames */ - public function getsExpectedRoleFriendlyName(string $roleName, string $expectedFriendlyName): void + public function getsExpectedRoleFriendlyName(Role $roleName, string $expectedFriendlyName): void { self::assertEquals($expectedFriendlyName, Role::toFriendlyName($roleName)); } public function provideRoleNames(): iterable { - yield 'unknown' => ['unknown', '']; - yield Role::AUTHORED_SHORT_URLS => [Role::AUTHORED_SHORT_URLS, 'Author only']; - yield Role::DOMAIN_SPECIFIC => [Role::DOMAIN_SPECIFIC, 'Domain only']; + yield Role::AUTHORED_SHORT_URLS->value => [Role::AUTHORED_SHORT_URLS, 'Author only']; + yield Role::DOMAIN_SPECIFIC->value => [Role::DOMAIN_SPECIFIC, 'Domain only']; } } diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index 97c0772b..aba79036 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -46,7 +46,7 @@ class ApiKeyServiceTest extends TestCase self::assertEquals($date, $key->getExpirationDate()); self::assertEquals($name, $key->name()); foreach ($roles as $roleDefinition) { - self::assertTrue($key->hasRole($roleDefinition->roleName)); + self::assertTrue($key->hasRole($roleDefinition->role)); } }