From 9e9621e7b2654276fcb5d9729071094c883fa869 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 18 Jan 2022 20:06:32 +0100 Subject: [PATCH] Standardized how inlined or regular specs are applied to query builders --- module/Core/src/Repository/TagRepository.php | 21 +-------- .../Core/src/Repository/VisitRepository.php | 6 +-- module/Rest/src/ApiKey/Role.php | 27 ++++++----- .../Spec/WithApiKeySpecsEnsuringJoin.php | 9 ++-- module/Rest/src/Entity/ApiKey.php | 10 +++- module/Rest/test/ApiKey/RoleTest.php | 47 ++++++++++++------- 6 files changed, 61 insertions(+), 59 deletions(-) diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index 1aa35603..f19e8917 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -16,12 +16,6 @@ use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function Functional\map; -use function is_object; -use function method_exists; -use function sprintf; -use function strlen; -use function strpos; -use function substr_replace; use const PHP_INT_MAX; @@ -60,24 +54,11 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito } $apiKey = $filtering?->apiKey(); - $this->applySpecification($subQb, $apiKey?->spec(false, 'shortUrls'), 't'); + $this->applySpecification($subQb, new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrls', true), 't'); $subQuery = $subQb->getQuery(); $subQuerySql = $subQuery->getSQL(); - // Sadly, we need to manually interpolate the params in the query replacing the placeholders, as this is going - // to be used as a sub-query in a native query. There's no need to sanitize, though. - foreach ($subQuery->getParameters() as $param) { - $value = $param->getValue(); - $pos = strpos($subQuerySql, self::PARAM_PLACEHOLDER); - $subQuerySql = substr_replace( - $subQuerySql, - sprintf('\'%s\'', is_object($value) && method_exists($value, 'getId') ? $value->getId() : $value), - $pos === false ? -1 : $pos, - strlen(self::PARAM_PLACEHOLDER), - ); - } - // A native query builder needs to be used here, because DQL and ORM query builders do not support // sub-queries at "from" and "join" level. // If no sub-query is used, the whole list is loaded even with pagination, making it very inefficient. diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index befd104d..b43d676d 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -105,7 +105,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo $shortUrlRepo = $this->getEntityManager()->getRepository(ShortUrl::class); $shortUrlId = $shortUrlRepo->findOne($identifier, $filtering->apiKey()?->spec())?->getId() ?? '-1'; - // Parameters in this query need to be part of the query itself, as we need to use it a sub-query later + // Parameters in this query need to be part of the query itself, as we need to use it as sub-query later // Since they are not provided by the caller, it's reasonably safe $qb = $this->getEntityManager()->createQueryBuilder(); $qb->from(Visit::class, 'v') @@ -149,7 +149,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo } $this->applyDatesInline($qb, $filtering->dateRange()); - $this->applySpecification($qb, $filtering->apiKey()?->spec(true), 'v'); + $this->applySpecification($qb, $filtering->apiKey()?->inlinedSpec(), 'v'); return $qb; } @@ -174,7 +174,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo $qb = $this->createAllVisitsQueryBuilder($filtering); $qb->andWhere($qb->expr()->isNotNull('v.shortUrl')); - $this->applySpecification($qb, $filtering->apiKey()?->spec(true)); + $this->applySpecification($qb, $filtering->apiKey()?->inlinedSpec()); return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit(), $filtering->offset()); } diff --git a/module/Rest/src/ApiKey/Role.php b/module/Rest/src/ApiKey/Role.php index c3677029..557abd00 100644 --- a/module/Rest/src/ApiKey/Role.php +++ b/module/Rest/src/ApiKey/Role.php @@ -21,21 +21,22 @@ class Role self::DOMAIN_SPECIFIC => 'Domain only', ]; - public static function toSpec(ApiKeyRole $role, bool $inlined, ?string $context = null): Specification + public static function toSpec(ApiKeyRole $role, ?string $context = null): Specification { - if ($role->name() === self::AUTHORED_SHORT_URLS) { - $apiKey = $role->apiKey(); - return $inlined ? Spec::andX(new BelongsToApiKeyInlined($apiKey)) : new BelongsToApiKey($apiKey, $context); - } + return match ($role->name()) { + self::AUTHORED_SHORT_URLS => new BelongsToApiKey($role->apiKey(), $context), + self::DOMAIN_SPECIFIC => new BelongsToDomain(self::domainIdFromMeta($role->meta()), $context), + default => Spec::andX(), + }; + } - if ($role->name() === self::DOMAIN_SPECIFIC) { - $domainId = self::domainIdFromMeta($role->meta()); - return $inlined - ? Spec::andX(new BelongsToDomainInlined($domainId)) - : new BelongsToDomain($domainId, $context); - } - - return Spec::andX(); + public static function toInlinedSpec(ApiKeyRole $role): Specification + { + return match ($role->name()) { + self::AUTHORED_SHORT_URLS => Spec::andX(new BelongsToApiKeyInlined($role->apiKey())), + self::DOMAIN_SPECIFIC => Spec::andX(new BelongsToDomainInlined(self::domainIdFromMeta($role->meta()))), + default => Spec::andX(), + }; } public static function domainIdFromMeta(array $meta): string diff --git a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php index ddfabe81..56f64a6d 100644 --- a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php +++ b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php @@ -11,8 +11,11 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class WithApiKeySpecsEnsuringJoin extends BaseSpecification { - public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls') - { + public function __construct( + private ?ApiKey $apiKey, + private string $fieldToJoin = 'shortUrls', + private bool $inlined = false, + ) { parent::__construct(); } @@ -20,7 +23,7 @@ class WithApiKeySpecsEnsuringJoin extends BaseSpecification { return $this->apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX( Spec::join($this->fieldToJoin, 's'), - $this->apiKey->spec(false, $this->fieldToJoin), + $this->inlined ? $this->apiKey->inlinedSpec() : $this->apiKey->spec($this->fieldToJoin), ); } } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 121bea18..2940bc69 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -96,9 +96,15 @@ class ApiKey extends AbstractEntity return $this->key; } - public function spec(bool $inlined = false, ?string $context = null): Specification + public function spec(?string $context = null): Specification { - $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toSpec($role, $inlined, $context))->getValues(); + $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toSpec($role, $context))->getValues(); + return Spec::andX(...$specs); + } + + public function inlinedSpec(): Specification + { + $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toInlinedSpec($role))->getValues(); return Spec::andX(...$specs); } diff --git a/module/Rest/test/ApiKey/RoleTest.php b/module/Rest/test/ApiKey/RoleTest.php index 278d37ff..7ee23076 100644 --- a/module/Rest/test/ApiKey/RoleTest.php +++ b/module/Rest/test/ApiKey/RoleTest.php @@ -21,39 +21,50 @@ class RoleTest extends TestCase * @test * @dataProvider provideRoles */ - public function returnsExpectedSpec(ApiKeyRole $apiKeyRole, bool $inlined, Specification $expected): void + public function returnsExpectedSpec(ApiKeyRole $apiKeyRole, Specification $expected): void { - self::assertEquals($expected, Role::toSpec($apiKeyRole, $inlined)); + self::assertEquals($expected, Role::toSpec($apiKeyRole)); } public function provideRoles(): iterable { $apiKey = ApiKey::create(); - yield 'inline invalid role' => [new ApiKeyRole('invalid', [], $apiKey), true, Spec::andX()]; - yield 'not inline invalid role' => [new ApiKeyRole('invalid', [], $apiKey), false, Spec::andX()]; - yield 'inline author role' => [ + yield 'invalid role' => [new ApiKeyRole('invalid', [], $apiKey), Spec::andX()]; + yield 'author role' => [ new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey), - true, - Spec::andX(new BelongsToApiKeyInlined($apiKey)), - ]; - yield 'not inline author role' => [ - new ApiKeyRole(Role::AUTHORED_SHORT_URLS, [], $apiKey), - false, new BelongsToApiKey($apiKey), ]; - yield 'inline domain role' => [ - new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '123'], $apiKey), - true, - Spec::andX(new BelongsToDomainInlined('123')), - ]; - yield 'not inline domain role' => [ + yield 'domain role' => [ new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '456'], $apiKey), - false, new BelongsToDomain('456'), ]; } + /** + * @test + * @dataProvider provideInlinedRoles + */ + public function returnsExpectedInlinedSpec(ApiKeyRole $apiKeyRole, Specification $expected): void + { + self::assertEquals($expected, Role::toInlinedSpec($apiKeyRole)); + } + + public function provideInlinedRoles(): iterable + { + $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)), + ]; + yield 'domain role' => [ + new ApiKeyRole(Role::DOMAIN_SPECIFIC, ['domain_id' => '123'], $apiKey), + Spec::andX(new BelongsToDomainInlined('123')), + ]; + } + /** * @test * @dataProvider provideMetasWithDomainId