From d39f3b42659267ea64c6f56e4e9d13c813fcff05 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 18 Jan 2022 19:38:53 +0100 Subject: [PATCH 1/3] Enhanced TagRepositoryTest and replaced inlined quoting by doctrine connection quoting --- .../Core/src/Repository/VisitRepository.php | 2 +- .../ShortUrl/Spec/BelongsToApiKeyInlined.php | 3 +- .../ShortUrl/Spec/BelongsToDomainInlined.php | 3 +- .../test-db/Repository/TagRepositoryTest.php | 31 +++++++++++++++++-- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/module/Core/src/Repository/VisitRepository.php b/module/Core/src/Repository/VisitRepository.php index 67b4256f..befd104d 100644 --- a/module/Core/src/Repository/VisitRepository.php +++ b/module/Core/src/Repository/VisitRepository.php @@ -142,7 +142,7 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo $qb->from(Visit::class, 'v') ->join('v.shortUrl', 's') ->join('s.tags', 't') - ->where($qb->expr()->eq('t.name', '\'' . $tag . '\'')); // This needs to be concatenated, not bound + ->where($qb->expr()->eq('t.name', $this->getEntityManager()->getConnection()->quote($tag))); if ($filtering->excludeBots()) { $qb->andWhere($qb->expr()->eq('v.potentialBot', 'false')); diff --git a/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php b/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php index 809d19b7..01440bb3 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php @@ -17,6 +17,7 @@ class BelongsToApiKeyInlined implements Filter public function getFilter(QueryBuilder $qb, string $dqlAlias): string { // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later - return $qb->expr()->eq('s.authorApiKey', '\'' . $this->apiKey->getId() . '\'')->__toString(); + $conn = $qb->getEntityManager()->getConnection(); + return $qb->expr()->eq('s.authorApiKey', $conn->quote($this->apiKey->getId()))->__toString(); } } diff --git a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php index 46fba689..baaed1a6 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php @@ -16,6 +16,7 @@ class BelongsToDomainInlined implements Filter public function getFilter(QueryBuilder $qb, string $context): string { // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later - return $qb->expr()->eq('s.domain', '\'' . $this->domainId . '\'')->__toString(); + $conn = $qb->getEntityManager()->getConnection(); + return $qb->expr()->eq('s.domain', $conn->quote($this->domainId))->__toString(); } } diff --git a/module/Core/test-db/Repository/TagRepositoryTest.php b/module/Core/test-db/Repository/TagRepositoryTest.php index 5ac1f3ac..d030bd03 100644 --- a/module/Core/test-db/Repository/TagRepositoryTest.php +++ b/module/Core/test-db/Repository/TagRepositoryTest.php @@ -63,19 +63,27 @@ class TagRepositoryTest extends DatabaseTestCase foreach ($names as $name) { $this->getEntityManager()->persist(new Tag($name)); } + + $apiKey = $filtering?->apiKey(); + if ($apiKey !== null) { + $this->getEntityManager()->persist($apiKey); + } + $this->getEntityManager()->flush(); [$firstUrlTags] = array_chunk($names, 3); $secondUrlTags = [$names[0]]; - $metaWithTags = fn (array $tags) => ShortUrlMeta::fromRawData(['longUrl' => '', 'tags' => $tags]); + $metaWithTags = fn (array $tags, ?ApiKey $apiKey) => ShortUrlMeta::fromRawData( + ['longUrl' => '', 'tags' => $tags, 'apiKey' => $apiKey], + ); - $shortUrl = ShortUrl::fromMeta($metaWithTags($firstUrlTags), $this->relationResolver); + $shortUrl = ShortUrl::fromMeta($metaWithTags($firstUrlTags, $apiKey), $this->relationResolver); $this->getEntityManager()->persist($shortUrl); $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance())); $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance())); $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl, Visitor::emptyInstance())); - $shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags), $this->relationResolver); + $shortUrl2 = ShortUrl::fromMeta($metaWithTags($secondUrlTags, null), $this->relationResolver); $this->getEntityManager()->persist($shortUrl2); $this->getEntityManager()->persist(Visit::forValidShortUrl($shortUrl2, Visitor::emptyInstance())); $this->getEntityManager()->flush(); @@ -181,6 +189,23 @@ class TagRepositoryTest extends DatabaseTestCase self::assertEquals($tagNames[0], $result[0]->tag()->__toString()); }, ]; + yield 'api key' => [new TagsListFiltering(null, null, null, null, ApiKey::fromMeta( + ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls()), + )), static function (array $result, array $tagNames): void { + /** @var TagInfo[] $result */ + self::assertCount(3, $result); + self::assertEquals(1, $result[0]->shortUrlsCount()); + self::assertEquals(3, $result[0]->visitsCount()); + self::assertEquals($tagNames[1], $result[0]->tag()->__toString()); + + self::assertEquals(1, $result[1]->shortUrlsCount()); + self::assertEquals(3, $result[1]->visitsCount()); + self::assertEquals($tagNames[2], $result[1]->tag()->__toString()); + + self::assertEquals(1, $result[2]->shortUrlsCount()); + self::assertEquals(3, $result[2]->visitsCount()); + self::assertEquals($tagNames[0], $result[2]->tag()->__toString()); + }]; } /** @test */ From 9e9621e7b2654276fcb5d9729071094c883fa869 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 18 Jan 2022 20:06:32 +0100 Subject: [PATCH 2/3] 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 From d0546a2ea2817eab0f77ecc2edfbdfece1d9b710 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 18 Jan 2022 20:14:24 +0100 Subject: [PATCH 3/3] Split spec to join ApiKey spec with short URLs, into inlined and regular versions --- module/Core/src/Repository/TagRepository.php | 5 ++-- .../Spec/WithApiKeySpecsEnsuringJoin.php | 9 +++---- .../WithInlinedApiKeySpecsEnsuringJoin.php | 26 +++++++++++++++++++ 3 files changed, 31 insertions(+), 9 deletions(-) create mode 100644 module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php diff --git a/module/Core/src/Repository/TagRepository.php b/module/Core/src/Repository/TagRepository.php index f19e8917..8e6e2080 100644 --- a/module/Core/src/Repository/TagRepository.php +++ b/module/Core/src/Repository/TagRepository.php @@ -13,6 +13,7 @@ use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering; use Shlinkio\Shlink\Core\Tag\Spec\CountTagsWithName; use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\ApiKey\Spec\WithApiKeySpecsEnsuringJoin; +use Shlinkio\Shlink\Rest\ApiKey\Spec\WithInlinedApiKeySpecsEnsuringJoin; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function Functional\map; @@ -21,8 +22,6 @@ use const PHP_INT_MAX; class TagRepository extends EntitySpecificationRepository implements TagRepositoryInterface { - private const PARAM_PLACEHOLDER = '?'; - public function deleteByName(array $names): int { if (empty($names)) { @@ -54,7 +53,7 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito } $apiKey = $filtering?->apiKey(); - $this->applySpecification($subQb, new WithApiKeySpecsEnsuringJoin($apiKey, 'shortUrls', true), 't'); + $this->applySpecification($subQb, new WithInlinedApiKeySpecsEnsuringJoin($apiKey, 'shortUrls'), 't'); $subQuery = $subQb->getQuery(); $subQuerySql = $subQuery->getSQL(); diff --git a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php index 56f64a6d..1f8c2fd3 100644 --- a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php +++ b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php @@ -11,11 +11,8 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class WithApiKeySpecsEnsuringJoin extends BaseSpecification { - public function __construct( - private ?ApiKey $apiKey, - private string $fieldToJoin = 'shortUrls', - private bool $inlined = false, - ) { + public function __construct(private ?ApiKey $apiKey, private string $fieldToJoin = 'shortUrls') + { parent::__construct(); } @@ -23,7 +20,7 @@ class WithApiKeySpecsEnsuringJoin extends BaseSpecification { return $this->apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX( Spec::join($this->fieldToJoin, 's'), - $this->inlined ? $this->apiKey->inlinedSpec() : $this->apiKey->spec($this->fieldToJoin), + $this->apiKey->spec($this->fieldToJoin), ); } } diff --git a/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php new file mode 100644 index 00000000..8e535570 --- /dev/null +++ b/module/Rest/src/ApiKey/Spec/WithInlinedApiKeySpecsEnsuringJoin.php @@ -0,0 +1,26 @@ +apiKey === null || $this->apiKey->isAdmin() ? Spec::andX() : Spec::andX( + Spec::join($this->fieldToJoin, 's'), + $this->apiKey->inlinedSpec(), + ); + } +}