From f7c0486101a2bc80402607d72c8153e67cbc3635 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 10 Dec 2021 12:52:36 +0100 Subject: [PATCH 1/4] Added swagger:validate to ci and ci:parallel commands --- composer.json | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/composer.json b/composer.json index fedb863f..abc6dc79 100644 --- a/composer.json +++ b/composer.json @@ -53,12 +53,12 @@ "shlinkio/shlink-importer": "dev-main#d099072 as 2.5", "shlinkio/shlink-installer": "dev-develop#7dd00fb as 6.3", "shlinkio/shlink-ip-geolocation": "^2.2", - "symfony/console": "^5.4", - "symfony/filesystem": "^5.4", - "symfony/lock": "^5.4", + "symfony/console": "^6.0 || ^5.4", + "symfony/filesystem": "^6.0 || ^5.4", + "symfony/lock": "^6.0 || ^5.4", "symfony/mercure": "^0.6", - "symfony/process": "^5.4", - "symfony/string": "^5.4" + "symfony/process": "^6.0 || ^5.4", + "symfony/string": "^6.0 || ^5.4" }, "require-dev": { "cebe/php-openapi": "^1.5", @@ -107,11 +107,12 @@ "ci": [ "@cs", "@stan", + "@swagger:validate", "@test:ci", "@infect:ci" ], "ci:parallel": [ - "@parallel cs stan test:unit:ci test:db:sqlite:ci test:db:mysql test:db:maria test:db:postgres test:db:ms", + "@parallel cs stan swagger:validate test:unit:ci test:db:sqlite:ci test:db:mysql test:db:maria test:db:postgres test:db:ms", "@parallel test:api infect:ci:unit infect:ci:db" ], "cs": "phpcs", @@ -151,7 +152,7 @@ "clean:dev": "rm -f data/database.sqlite && rm -f config/params/generated_config.php" }, "scripts-descriptions": { - "ci": "Alias for \"cs\", \"stan\", \"test:ci\" and \"infect:ci\"", + "ci": "Alias for \"cs\", \"stan\", \"swagger:validate\", \"test:ci\" and \"infect:ci\"", "ci:parallel": "Same as \"ci\", but parallelizing tasks as much as possible", "cs": "Checks coding styles", "cs:fix": "Fixes coding styles, when possible", From 0786a962e79ac1f2a58d51b6a2fe088bb2b7894f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 10 Dec 2021 13:42:33 +0100 Subject: [PATCH 2/4] Increased MIS to 83% --- composer.json | 4 + .../CLI/src/Command/Tag/ListTagsCommand.php | 3 +- .../test/Command/Api/ListKeysCommandTest.php | 27 ++++--- .../test/Command/Tag/ListTagsCommandTest.php | 18 +++-- module/Core/src/Entity/Visit.php | 2 +- .../ShortUrl/Spec/BelongsToApiKeyInlined.php | 2 +- .../ShortUrl/Spec/BelongsToDomainInlined.php | 2 +- module/Rest/src/Action/Tag/ListTagsAction.php | 2 +- .../src/Middleware/BodyParserMiddleware.php | 4 +- .../Request/DomainRedirectsRequestTest.php | 73 +++++++++++++++++++ 10 files changed, 114 insertions(+), 23 deletions(-) create mode 100644 module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php diff --git a/composer.json b/composer.json index abc6dc79..baa1b95c 100644 --- a/composer.json +++ b/composer.json @@ -147,6 +147,10 @@ "@parallel test:unit:ci test:db:sqlite:ci", "@infect:ci" ], + "infect:test:unit": [ + "@test:unit:ci", + "@infect:ci:unit" + ], "swagger:validate": "php-openapi validate docs/swagger/swagger.json", "swagger:inline": "php-openapi inline docs/swagger/swagger.json docs/swagger/swagger-inlined.json", "clean:dev": "rm -f data/database.sqlite && rm -f config/params/generated_config.php" diff --git a/module/CLI/src/Command/Tag/ListTagsCommand.php b/module/CLI/src/Command/Tag/ListTagsCommand.php index 61d4e6e0..9eebe36f 100644 --- a/module/CLI/src/Command/Tag/ListTagsCommand.php +++ b/module/CLI/src/Command/Tag/ListTagsCommand.php @@ -45,7 +45,8 @@ class ListTagsCommand extends Command return map( $tags, - fn (TagInfo $tagInfo) => [(string) $tagInfo->tag(), $tagInfo->shortUrlsCount(), $tagInfo->visitsCount()], + static fn (TagInfo $tagInfo) => + [$tagInfo->tag()->__toString(), $tagInfo->shortUrlsCount(), $tagInfo->visitsCount()], ); } } diff --git a/module/CLI/test/Command/Api/ListKeysCommandTest.php b/module/CLI/test/Command/Api/ListKeysCommandTest.php index a124993f..68c1e844 100644 --- a/module/CLI/test/Command/Api/ListKeysCommandTest.php +++ b/module/CLI/test/Command/Api/ListKeysCommandTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\CLI\Command\Api; +use Cake\Chronos\Chronos; use PHPUnit\Framework\TestCase; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\CLI\Command\Api\ListKeysCommand; @@ -45,19 +46,25 @@ class ListKeysCommandTest extends TestCase public function provideKeysAndOutputs(): iterable { + $dateInThePast = Chronos::createFromFormat('Y-m-d H:i:s', '2020-01-01 00:00:00'); + yield 'all keys' => [ - [$apiKey1 = ApiKey::create(), $apiKey2 = ApiKey::create(), $apiKey3 = ApiKey::create()], + [ + $apiKey1 = ApiKey::create()->disable(), + $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($dateInThePast)), + $apiKey3 = ApiKey::create(), + ], false, <<commandTester->execute([]); $output = $this->commandTester->getDisplay(); - self::assertStringContainsString('| foo', $output); - self::assertStringContainsString('| bar', $output); - self::assertStringContainsString('| 10 ', $output); - self::assertStringContainsString('| 2 ', $output); - self::assertStringContainsString('| 7 ', $output); - self::assertStringContainsString('| 32 ', $output); + self::assertEquals( + <<shouldHaveBeenCalled(); } } diff --git a/module/Core/src/Entity/Visit.php b/module/Core/src/Entity/Visit.php index 8174e8be..c509bcc3 100644 --- a/module/Core/src/Entity/Visit.php +++ b/module/Core/src/Entity/Visit.php @@ -103,7 +103,7 @@ class Visit extends AbstractEntity implements JsonSerializable } try { - return (string) IpAddress::fromString($address)->getAnonymizedCopy(); + return IpAddress::fromString($address)->getAnonymizedCopy()->__toString(); } catch (InvalidArgumentException) { return null; } diff --git a/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php b/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php index 6b103058..809d19b7 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToApiKeyInlined.php @@ -17,6 +17,6 @@ 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 (string) $qb->expr()->eq('s.authorApiKey', '\'' . $this->apiKey->getId() . '\''); + return $qb->expr()->eq('s.authorApiKey', '\'' . $this->apiKey->getId() . '\'')->__toString(); } } diff --git a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php index 4ce130b7..46fba689 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php @@ -16,6 +16,6 @@ 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 (string) $qb->expr()->eq('s.domain', '\'' . $this->domainId . '\''); + return $qb->expr()->eq('s.domain', '\'' . $this->domainId . '\'')->__toString(); } } diff --git a/module/Rest/src/Action/Tag/ListTagsAction.php b/module/Rest/src/Action/Tag/ListTagsAction.php index 89371b71..3d34bd19 100644 --- a/module/Rest/src/Action/Tag/ListTagsAction.php +++ b/module/Rest/src/Action/Tag/ListTagsAction.php @@ -38,7 +38,7 @@ class ListTagsAction extends AbstractRestAction } $tagsInfo = $this->tagService->tagsInfo($apiKey); - $data = map($tagsInfo, fn (TagInfo $info) => (string) $info->tag()); + $data = map($tagsInfo, static fn (TagInfo $info) => $info->tag()->__toString()); return new JsonResponse([ 'tags' => [ diff --git a/module/Rest/src/Middleware/BodyParserMiddleware.php b/module/Rest/src/Middleware/BodyParserMiddleware.php index c7e99121..2711d900 100644 --- a/module/Rest/src/Middleware/BodyParserMiddleware.php +++ b/module/Rest/src/Middleware/BodyParserMiddleware.php @@ -54,7 +54,7 @@ class BodyParserMiddleware implements MiddlewareInterface, RequestMethodInterfac private function parseFromJson(Request $request): Request { - $rawBody = (string) $request->getBody(); + $rawBody = $request->getBody()->__toString(); if (empty($rawBody)) { return $request; } @@ -68,7 +68,7 @@ class BodyParserMiddleware implements MiddlewareInterface, RequestMethodInterfac */ private function parseFromUrlEncoded(Request $request): Request { - $rawBody = (string) $request->getBody(); + $rawBody = $request->getBody()->__toString(); if (empty($rawBody)) { return $request; } diff --git a/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php b/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php new file mode 100644 index 00000000..55828368 --- /dev/null +++ b/module/Rest/test/Action/Domain/Request/DomainRedirectsRequestTest.php @@ -0,0 +1,73 @@ +expectException(ValidationException::class); + DomainRedirectsRequest::fromRawData($data); + } + + public function provideInvalidData(): iterable + { + yield 'missing domain' => [[]]; + yield 'invalid domain' => [['domain' => 'foo:bar:baz']]; + } + + /** + * @test + * @dataProvider provideValidData + */ + public function isProperlyCastToNotFoundRedirects( + array $data, + ?NotFoundRedirectConfigInterface $defaults, + string $expectedAuthority, + ?string $expectedBaseUrlRedirect, + ?string $expectedRegular404Redirect, + ?string $expectedInvalidShortUrlRedirect, + ): void { + $request = DomainRedirectsRequest::fromRawData($data); + $notFound = $request->toNotFoundRedirects($defaults); + + self::assertEquals($expectedAuthority, $request->authority()); + self::assertEquals($expectedBaseUrlRedirect, $notFound->baseUrlRedirect()); + self::assertEquals($expectedRegular404Redirect, $notFound->regular404Redirect()); + self::assertEquals($expectedInvalidShortUrlRedirect, $notFound->invalidShortUrlRedirect()); + } + + public function provideValidData(): iterable + { + yield 'no values' => [['domain' => 'foo'], null, 'foo', null, null, null]; + yield 'some values' => [['domain' => 'foo', 'regular404Redirect' => 'bar'], null, 'foo', null, 'bar', null]; + yield 'fallbacks' => [ + ['domain' => 'domain', 'baseUrlRedirect' => 'bar'], + new NotFoundRedirectOptions(['regular404' => 'fallback', 'invalidShortUrl' => 'fallback2']), + 'domain', + 'bar', + 'fallback', + 'fallback2', + ]; + yield 'fallback ignored' => [ + ['domain' => 'domain', 'regular404Redirect' => 'bar', 'invalidShortUrlRedirect' => null], + new NotFoundRedirectOptions(['regular404' => 'fallback', 'invalidShortUrl' => 'fallback2']), + 'domain', + null, + 'bar', + null, + ]; + } +} From 3f3cf5e20edee042a9a38f3ce714b76a55bfbaed Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 10 Dec 2021 14:00:55 +0100 Subject: [PATCH 3/4] Explicitly required an MSI of 83 for unit tests --- composer.json | 2 +- .../Action/Visit/OrphanVisitsActionTest.php | 7 ++-- .../test/ApiKey/Model/RoleDefinitionTest.php | 32 +++++++++++++++++++ 3 files changed, 38 insertions(+), 3 deletions(-) create mode 100644 module/Rest/test/ApiKey/Model/RoleDefinitionTest.php diff --git a/composer.json b/composer.json index baa1b95c..bb9319c1 100644 --- a/composer.json +++ b/composer.json @@ -140,7 +140,7 @@ "test:db:ms": "DB_DRIVER=mssql composer test:db:sqlite", "test:api": "bin/test/run-api-tests.sh", "infect:ci:base": "infection --threads=4 --log-verbosity=default --only-covered --only-covering-test-cases --skip-initial-tests", - "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=80", + "infect:ci:unit": "@infect:ci:base --coverage=build/coverage-unit --min-msi=83", "infect:ci:db": "@infect:ci:base --coverage=build/coverage-db --min-msi=95 --configuration=infection-db.json", "infect:ci": "@parallel infect:ci:unit infect:ci:db", "infect:test": [ diff --git a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php index 9fec7e1f..43209e51 100644 --- a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php @@ -45,13 +45,16 @@ class OrphanVisitsActionTest extends TestCase $orphanVisits = $this->visitsHelper->orphanVisits(Argument::type(VisitsParams::class))->willReturn( new Paginator(new ArrayAdapter($visits)), ); + $visitsAmount = count($visits); $transform = $this->orphanVisitTransformer->transform(Argument::type(Visit::class))->willReturn([]); + /** @var JsonResponse $response */ $response = $this->action->handle(ServerRequestFactory::fromGlobals()); + $payload = $response->getPayload(); - self::assertInstanceOf(JsonResponse::class, $response); + self::assertCount($visitsAmount, $payload['visits']['data']); self::assertEquals(200, $response->getStatusCode()); $orphanVisits->shouldHaveBeenCalledOnce(); - $transform->shouldHaveBeenCalledTimes(count($visits)); + $transform->shouldHaveBeenCalledTimes($visitsAmount); } } diff --git a/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php b/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php new file mode 100644 index 00000000..8e6a58ad --- /dev/null +++ b/module/Rest/test/ApiKey/Model/RoleDefinitionTest.php @@ -0,0 +1,32 @@ +roleName()); + self::assertEquals([], $definition->meta()); + } + + /** @test */ + public function forDomainCreatesRoleDefinitionAsExpected(): void + { + $domain = Domain::withAuthority('foo.com')->setId('123'); + $definition = RoleDefinition::forDomain($domain); + + self::assertEquals(Role::DOMAIN_SPECIFIC, $definition->roleName()); + self::assertEquals(['domain_id' => '123', 'authority' => 'foo.com'], $definition->meta()); + } +} From bfea3f35f022a557974a8fb972de1e2c41f4d8e2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 10 Dec 2021 14:01:58 +0100 Subject: [PATCH 4/4] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dcc88c9b..263ae923 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1218](https://github.com/shlinkio/shlink/issues/1218) Updated to symfony/mercure 0.6. * [#1223](https://github.com/shlinkio/shlink/issues/1223) Updated to phpstan 1.0. * Added `domain` field to `DeleteShortUrlException` exception. +* [#1001](https://github.com/shlinkio/shlink/issues/1001) Increased required MSI to 83%. ### Deprecated * *Nothing*