From 2e52d8f8579cd077ffd2ed7d77df8a8f48dbef01 Mon Sep 17 00:00:00 2001 From: Valerio Pizzichini Date: Wed, 17 Jan 2024 15:31:43 +0100 Subject: [PATCH] feat: make location parse configurable on version 5.X --- .github/workflows/validate.yml | 46 +++++++---- src/GraphQL.php | 13 +++- src/Schema/Directives/ThrottleDirective.php | 1 + src/Schema/Types/Scalars/DateScalar.php | 1 + src/lighthouse.php | 59 ++++++++------ tests/Integration/ErrorTest.php | 77 ++++++++++++++++++- .../Integration/Validation/ValidationTest.php | 41 +++++++++- tests/SerializingArrayStore.php | 10 +-- 8 files changed, 201 insertions(+), 47 deletions(-) diff --git a/.github/workflows/validate.yml b/.github/workflows/validate.yml index 1b4ef34a60..8aa5714e07 100644 --- a/.github/workflows/validate.yml +++ b/.github/workflows/validate.yml @@ -9,8 +9,8 @@ env: REQUIRED_PHP_EXTENSIONS: mbstring, mysqli, pdo_mysql, redis LIGHTHOUSE_TEST_DB_USER: root LIGHTHOUSE_TEST_DB_PASSWORD: root - LIGHTHOUSE_TEST_DB_HOST: localhost - LIGHTHOUSE_TEST_DB_UNIX_SOCKET: /var/run/mysqld/mysqld.sock + LIGHTHOUSE_TEST_DB_HOST: 127.0.0.1 + LIGHTHOUSE_TEST_DB_PORT: 33060 LIGHTHOUSE_TEST_REDIS_HOST: 127.0.0.1 # Using ubuntu-18.04 because it has MySQL 5.7. @@ -18,7 +18,7 @@ env: jobs: static-analysis: - runs-on: ubuntu-18.04 + runs-on: ubuntu-latest strategy: fail-fast: false @@ -62,6 +62,8 @@ jobs: laravel-version: ^6 - php-version: 8.1 laravel-version: ^7 + - php-version: 8.1 + laravel-version: ^8 - php-version: 8.2 laravel-version: ^6 - php-version: 8.2 @@ -95,7 +97,17 @@ jobs: - run: vendor/bin/phpstan tests: - runs-on: ubuntu-18.04 + runs-on: ubuntu-latest + + services: + mysql: + image: mysql:5.7 + env: + MYSQL_ROOT_PASSWORD: root + MYSQL_DATABASE: test + ports: + - 33060:3306 + options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=10 strategy: matrix: @@ -191,14 +203,24 @@ jobs: - run: composer require illuminate/contracts:${{ matrix.laravel-version }} --no-interaction --prefer-dist --no-progress - - run: | - sudo systemctl start mysql.service - mysql --user=root --password=root --execute='CREATE DATABASE test;' + - name: Check MySQL version + run: mysql --host 127.0.0.1 --port ${{ job.services.mysql.ports['3306'] }} -uroot -proot -e "SELECT @@VERSION" - - run: vendor/bin/phpunit --colors=always --verbose + - name: Execute test + run: vendor/bin/phpunit --colors=always --verbose coverage: - runs-on: ubuntu-18.04 + runs-on: ubuntu-latest + + services: + mysql: + image: mysql:5.7 + env: + MYSQL_ROOT_PASSWORD: root + MYSQL_DATABASE: test + ports: + - 33060:3306 + options: --health-cmd="mysqladmin ping" --health-interval=10s --health-timeout=5s --health-retries=10 strategy: matrix: @@ -235,17 +257,13 @@ jobs: - run: composer require illuminate/contracts:${{ matrix.laravel-version }} --no-interaction --prefer-dist --no-progress - - run: | - sudo systemctl start mysql.service - mysql --user=root --password=root --execute='CREATE DATABASE test;' - - run: vendor/bin/phpunit --coverage-clover=coverage.xml - name: "Upload to Codecov" uses: codecov/codecov-action@v2 benchmarks: - runs-on: ubuntu-18.04 + runs-on: ubuntu-latest strategy: matrix: diff --git a/src/GraphQL.php b/src/GraphQL.php index 53886c2060..86f3aa11c0 100644 --- a/src/GraphQL.php +++ b/src/GraphQL.php @@ -216,7 +216,7 @@ public function parse(string $query): DocumentNode $cacheConfig = $this->configRepository->get('lighthouse.query_cache'); if (! $cacheConfig['enable']) { - return Parser::parse($query); + return $this->parseQuery($query); } $cacheFactory = Container::getInstance()->make(CacheFactory::class); @@ -227,8 +227,8 @@ public function parse(string $query): DocumentNode return $store->remember( 'lighthouse:query:' . hash('sha256', $query), $cacheConfig['ttl'], - static function () use ($query): DocumentNode { - return Parser::parse($query); + function () use ($query): DocumentNode { + return $this->parseQuery($query); } ); } @@ -450,4 +450,11 @@ public function prepSchema(): Schema { return $this->schemaBuilder->schema(); } + + protected function parseQuery(string $query): DocumentNode + { + return Parser::parse($query, [ + 'noLocation' => ! $this->configRepository->get('lighthouse.parse_source_location'), + ]); + } } diff --git a/src/Schema/Directives/ThrottleDirective.php b/src/Schema/Directives/ThrottleDirective.php index c0edcf29ab..a75c12162d 100644 --- a/src/Schema/Directives/ThrottleDirective.php +++ b/src/Schema/Directives/ThrottleDirective.php @@ -78,6 +78,7 @@ public function handleField(FieldValue $fieldValue, \Closure $next): FieldValue // @phpstan-ignore-next-line won't be executed on Laravel < 8 $limiter = $this->limiter->limiter($name); + // @phpstan-ignore-next-line won't be executed on Laravel < 8 $limiterResponse = $limiter($this->request); // @phpstan-ignore-next-line won't be executed on Laravel < 8 if ($limiterResponse instanceof Unlimited) { diff --git a/src/Schema/Types/Scalars/DateScalar.php b/src/Schema/Types/Scalars/DateScalar.php index 21982ce08e..2f345437e4 100644 --- a/src/Schema/Types/Scalars/DateScalar.php +++ b/src/Schema/Types/Scalars/DateScalar.php @@ -77,6 +77,7 @@ protected function tryParsingDate($value, string $exceptionClass): IlluminateCar || CarbonImmutable::class === get_class($value) ) ) { + // @phpstan-ignore-next-line We need to type hint $value into one of the two types or static analysis will throw assert($value instanceof CarbonCarbon || $value instanceof CarbonImmutable); $carbon = IlluminateCarbon::create( diff --git a/src/lighthouse.php b/src/lighthouse.php index c23fdf0d55..d050fdc6d2 100644 --- a/src/lighthouse.php +++ b/src/lighthouse.php @@ -28,11 +28,11 @@ * make sure to return spec-compliant responses in case an error is thrown. */ 'middleware' => [ - \Nuwave\Lighthouse\Support\Http\Middleware\AcceptJson::class, + Nuwave\Lighthouse\Support\Http\Middleware\AcceptJson::class, // Logs in a user if they are authenticated. In contrast to Laravel's 'auth' // middleware, this delegates auth and permission checks to the field level. - \Nuwave\Lighthouse\Support\Http\Middleware\AttemptAuthentication::class, + Nuwave\Lighthouse\Support\Http\Middleware\AttemptAuthentication::class, // Logs every incoming GraphQL query. // \Nuwave\Lighthouse\Support\Http\Middleware\LogGraphQLQueries::class, @@ -152,12 +152,25 @@ */ 'ttl' => env( 'LIGHTHOUSE_QUERY_CACHE_TTL', - \Nuwave\Lighthouse\Support\AppVersion::atLeast(5.8) + Nuwave\Lighthouse\Support\AppVersion::atLeast(5.8) ? 24 * 60 * 60 // 1 day in seconds : 24 * 60 // 1 day in minutes ), ], + /* + |-------------------------------------------------------------------------- + | Parse source location + |-------------------------------------------------------------------------- + | + | Should the source location be included in the AST nodes resulting from query parsing? + | Setting this to `false` improves performance, but omits the key `locations` from errors, + | see https://spec.graphql.org/October2021/#sec-Errors.Error-result-format. + | + */ + + 'parse_source_location' => true, + /* |-------------------------------------------------------------------------- | Namespaces @@ -192,11 +205,11 @@ */ 'security' => [ - 'max_query_complexity' => \GraphQL\Validator\Rules\QueryComplexity::DISABLED, - 'max_query_depth' => \GraphQL\Validator\Rules\QueryDepth::DISABLED, + 'max_query_complexity' => GraphQL\Validator\Rules\QueryComplexity::DISABLED, + 'max_query_depth' => GraphQL\Validator\Rules\QueryDepth::DISABLED, 'disable_introspection' => (bool) env('LIGHTHOUSE_SECURITY_DISABLE_INTROSPECTION', false) - ? \GraphQL\Validator\Rules\DisableIntrospection::ENABLED - : \GraphQL\Validator\Rules\DisableIntrospection::DISABLED, + ? GraphQL\Validator\Rules\DisableIntrospection::ENABLED + : GraphQL\Validator\Rules\DisableIntrospection::DISABLED, ], /* @@ -251,7 +264,7 @@ | */ - 'debug' => env('LIGHTHOUSE_DEBUG', \GraphQL\Error\DebugFlag::INCLUDE_DEBUG_MESSAGE | \GraphQL\Error\DebugFlag::INCLUDE_TRACE), + 'debug' => env('LIGHTHOUSE_DEBUG', GraphQL\Error\DebugFlag::INCLUDE_DEBUG_MESSAGE | GraphQL\Error\DebugFlag::INCLUDE_TRACE), /* |-------------------------------------------------------------------------- @@ -265,11 +278,11 @@ */ 'error_handlers' => [ - \Nuwave\Lighthouse\Execution\AuthenticationErrorHandler::class, - \Nuwave\Lighthouse\Execution\AuthorizationErrorHandler::class, - \Nuwave\Lighthouse\Execution\ValidationErrorHandler::class, - \Nuwave\Lighthouse\Execution\ExtensionErrorHandler::class, - \Nuwave\Lighthouse\Execution\ReportingErrorHandler::class, + Nuwave\Lighthouse\Execution\AuthenticationErrorHandler::class, + Nuwave\Lighthouse\Execution\AuthorizationErrorHandler::class, + Nuwave\Lighthouse\Execution\ValidationErrorHandler::class, + Nuwave\Lighthouse\Execution\ExtensionErrorHandler::class, + Nuwave\Lighthouse\Execution\ReportingErrorHandler::class, ], /* @@ -284,14 +297,14 @@ */ 'field_middleware' => [ - \Nuwave\Lighthouse\Schema\Directives\TrimDirective::class, - \Nuwave\Lighthouse\Schema\Directives\ConvertEmptyStringsToNullDirective::class, - \Nuwave\Lighthouse\Schema\Directives\SanitizeDirective::class, - \Nuwave\Lighthouse\Validation\ValidateDirective::class, - \Nuwave\Lighthouse\Schema\Directives\TransformArgsDirective::class, - \Nuwave\Lighthouse\Schema\Directives\SpreadDirective::class, - \Nuwave\Lighthouse\Schema\Directives\RenameArgsDirective::class, - \Nuwave\Lighthouse\Schema\Directives\DropArgsDirective::class, + Nuwave\Lighthouse\Schema\Directives\TrimDirective::class, + Nuwave\Lighthouse\Schema\Directives\ConvertEmptyStringsToNullDirective::class, + Nuwave\Lighthouse\Schema\Directives\SanitizeDirective::class, + Nuwave\Lighthouse\Validation\ValidateDirective::class, + Nuwave\Lighthouse\Schema\Directives\TransformArgsDirective::class, + Nuwave\Lighthouse\Schema\Directives\SpreadDirective::class, + Nuwave\Lighthouse\Schema\Directives\RenameArgsDirective::class, + Nuwave\Lighthouse\Schema\Directives\DropArgsDirective::class, ], /* @@ -453,13 +466,13 @@ ], 'pusher' => [ 'driver' => 'pusher', - 'routes' => \Nuwave\Lighthouse\Subscriptions\SubscriptionRouter::class . '@pusher', + 'routes' => Nuwave\Lighthouse\Subscriptions\SubscriptionRouter::class . '@pusher', 'connection' => 'pusher', ], 'echo' => [ 'driver' => 'echo', 'connection' => env('LIGHTHOUSE_SUBSCRIPTION_REDIS_CONNECTION', 'default'), - 'routes' => \Nuwave\Lighthouse\Subscriptions\SubscriptionRouter::class . '@echoRoutes', + 'routes' => Nuwave\Lighthouse\Subscriptions\SubscriptionRouter::class . '@echoRoutes', ], ], diff --git a/tests/Integration/ErrorTest.php b/tests/Integration/ErrorTest.php index b8e7b16eef..4eb44cd305 100644 --- a/tests/Integration/ErrorTest.php +++ b/tests/Integration/ErrorTest.php @@ -4,7 +4,6 @@ use GraphQL\Error\DebugFlag; use GraphQL\Error\Error; -use GraphQL\Error\FormattedError; use Illuminate\Contracts\Config\Repository as ConfigRepository; use Tests\TestCase; @@ -41,6 +40,81 @@ public function testRejectsInvalidQuery(): void ); } + public function testReturnsFullGraphQLError(): void + { + $message = 'some error'; + $this->mockResolver(static function () use ($message): Error { + return new Error($message); + }); + + $this->schema = /** @lang GraphQL */ ' + type Query { + foo: ID @mock + } + '; + + $this + ->graphQL(/** @lang GraphQL */' + { + foo + } + ') + ->assertStatus(200) + ->assertJson([ + 'data' => [ + 'foo' => null, + ], + 'errors' => [ + [ + 'message' => $message, + 'path' => ['foo'], + 'locations' => [ + [ + 'line' => 3, + 'column' => 17, + ], + ], + ], + ], + ]); + } + + public function testReturnsGraphQLErrorWithoutLocationNode(): void + { + $config = $this->app->make(ConfigRepository::class); + $config->set('lighthouse.parse_source_location', false); + + $message = 'some error'; + $this->mockResolver(static function () use ($message): Error { + return new Error($message); + }); + + $this->schema = /** @lang GraphQL */ ' + type Query { + foo: ID @mock + } + '; + + $this + ->graphQL(/** @lang GraphQL */ ' + { + foo + } + ') + ->assertStatus(200) + ->assertJson([ + 'data' => [ + 'foo' => null, + ], + 'errors' => [ + [ + 'message' => $message, + 'path' => ['foo'], + ], + ], + ]); + } + public function testIgnoresInvalidJSONVariables(): void { $this @@ -237,3 +311,4 @@ public function testAssertGraphQLErrorNonClientSafe(): void ->assertGraphQLError($exception); } } + diff --git a/tests/Integration/Validation/ValidationTest.php b/tests/Integration/Validation/ValidationTest.php index be260212e0..e3aade31d0 100644 --- a/tests/Integration/Validation/ValidationTest.php +++ b/tests/Integration/Validation/ValidationTest.php @@ -20,7 +20,6 @@ protected function getEnvironmentSetUp($app): void { parent::getEnvironmentSetUp($app); - /** @var \Illuminate\Contracts\Config\Repository $config */ $config = $app->make(ConfigRepository::class); // Ensure we test for the result the end user receives @@ -93,6 +92,46 @@ public function testFullValidationError(): void ]); } + public function testFullValidationErrorWithoutLocationParse(): void + { + $config = $this->app->make(ConfigRepository::class); + $config->set('lighthouse.parse_source_location', false); + $this->schema = /** @lang GraphQL */ ' + type Query { + foo( + bar: String @rules(apply: ["required"]) + ): Int + } + '; + + $this + ->graphQL(/** @lang GraphQL */ ' + { + foo + } + ') + ->assertExactJson([ + 'errors' => [ + [ + 'message' => 'Validation failed for the field [foo].', + 'extensions' => [ + 'category' => ValidationException::CATEGORY, + ValidationException::CATEGORY => [ + 'bar' => [ + 'The bar field is required.', + ], + ], + ], + 'path' => ['foo'], + ], + ], + 'data' => [ + 'foo' => null, + ], + ]); + } + + public function testRunsOnNonRootFields(): void { $this->schema = /** @lang GraphQL */ ' diff --git a/tests/SerializingArrayStore.php b/tests/SerializingArrayStore.php index 9509a1a569..2c6ab52337 100644 --- a/tests/SerializingArrayStore.php +++ b/tests/SerializingArrayStore.php @@ -63,9 +63,9 @@ public function put($key, $value, $seconds): bool /** * Get the expiration time of the key. * - * @param int $seconds + * @param int|float $seconds */ - protected function calculateExpiration($seconds): int + protected function calculateExpiration($seconds) { return $this->toTimestamp($seconds); } @@ -73,12 +73,12 @@ protected function calculateExpiration($seconds): int /** * Get the UNIX timestamp for the given number of seconds. * - * @param int $seconds + * @param int|float $seconds */ - protected function toTimestamp($seconds): int + protected function toTimestamp($seconds) { return $seconds > 0 - ? $this->availableAt($seconds) + ? $this->availableAt((int) $seconds) : 0; } }