From a72e6a1d23850e9f783f23715e9b6f31db9c30cd Mon Sep 17 00:00:00 2001 From: Christian Nielsen Date: Mon, 8 Jun 2020 09:19:00 +0200 Subject: [PATCH 1/3] Revert "Revert "validate field arguments (#608)"" This reverts commit fb9dace2f231b32bf7af1d3b53923ade4a56f339. --- phpstan-baseline.neon | 15 --- src/Support/Field.php | 50 ++++++++-- src/Support/RulesInFields.php | 93 +++++++++++++++++++ src/Support/SelectFields.php | 24 ++--- tests/Unit/FieldTest.php | 11 ++- tests/Unit/GraphQLQueryTest.php | 2 +- tests/Unit/MutationTest.php | 31 ++++--- .../AccountType.php | 58 ++++++++++++ .../ProfileType.php | 52 +++++++++++ .../ValidationOfFieldArguments/TestQuery.php | 48 ++++++++++ .../ValidationOfFieldArgumentsTest.php | 82 ++++++++++++++++ 11 files changed, 416 insertions(+), 50 deletions(-) create mode 100644 src/Support/RulesInFields.php create mode 100644 tests/Unit/ValidationOfFieldArguments/AccountType.php create mode 100644 tests/Unit/ValidationOfFieldArguments/ProfileType.php create mode 100644 tests/Unit/ValidationOfFieldArguments/TestQuery.php create mode 100644 tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 5f92d2e7..1b7d4ccc 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -206,11 +206,6 @@ parameters: count: 1 path: src/Support/Field.php - - - message: "#^Method Rebing\\\\GraphQL\\\\Support\\\\Field\\:\\:instanciateSelectFields\\(\\) has parameter \\$arguments with no value type specified in iterable type array\\.$#" - count: 1 - path: src/Support/Field.php - - message: "#^Method Rebing\\\\GraphQL\\\\Support\\\\Field\\:\\:aliasArgs\\(\\) has parameter \\$arguments with no value type specified in iterable type array\\.$#" count: 1 @@ -306,16 +301,6 @@ parameters: count: 1 path: src/Support/SelectFields.php - - - message: "#^Method Rebing\\\\GraphQL\\\\Support\\\\SelectFields\\:\\:getFieldSelection\\(\\) has parameter \\$args with no value type specified in iterable type array\\.$#" - count: 1 - path: src/Support/SelectFields.php - - - - message: "#^Method Rebing\\\\GraphQL\\\\Support\\\\SelectFields\\:\\:getFieldSelection\\(\\) return type has no value type specified in iterable type array\\.$#" - count: 1 - path: src/Support/SelectFields.php - - message: "#^Method Rebing\\\\GraphQL\\\\Support\\\\SelectFields\\:\\:getSelectableFieldsAndRelations\\(\\) has parameter \\$queryArgs with no value type specified in iterable type array\\.$#" count: 1 diff --git a/src/Support/Field.php b/src/Support/Field.php index c757f364..e904a440 100644 --- a/src/Support/Field.php +++ b/src/Support/Field.php @@ -20,6 +20,13 @@ */ abstract class Field { + /** + * The depth the SelectField and ResolveInfoFieldsAndArguments classes traverse. + * + * @var int + */ + protected $depth = 5; + protected $attributes = []; /** @@ -86,6 +93,21 @@ public function getRules(array $arguments = []): array return array_merge($argsRules, $rules); } + /** + * @param array $fieldsAndArgumentsSelection + * @return void + */ + public function validateFieldArguments(array $fieldsAndArgumentsSelection): void + { + $argsRules = (new RulesInFields($this->type(), $fieldsAndArgumentsSelection))->get(); + if (count($argsRules)) { + $validator = $this->getValidator($fieldsAndArgumentsSelection, $argsRules); + if ($validator->fails()) { + throw new ValidationError('validation', $validator); + } + } + } + public function getValidator(array $args, array $rules): ValidatorContract { // allow our error messages to be customised @@ -123,6 +145,11 @@ protected function getResolver(): ?Closure } } + $fieldsAndArguments = (new ResolveInfoFieldsAndArguments($arguments[3]))->getFieldsAndArgumentsSelection($this->depth); + + // Validate arguments in fields + $this->validateFieldArguments($fieldsAndArguments); + $arguments[1] = $this->getArgs($arguments); // Authorize @@ -134,7 +161,7 @@ protected function getResolver(): ?Closure $additionalParams = array_slice($method->getParameters(), 3); - $additionalArguments = array_map(function ($param) use ($arguments) { + $additionalArguments = array_map(function ($param) use ($arguments, $fieldsAndArguments) { $className = null !== $param->getClass() ? $param->getClass()->getName() : null; if (null === $className) { @@ -142,13 +169,13 @@ protected function getResolver(): ?Closure } if (Closure::class === $param->getType()->getName()) { - return function (int $depth = null) use ($arguments): SelectFields { - return $this->instanciateSelectFields($arguments, $depth); + return function (int $depth = null) use ($arguments, $fieldsAndArguments): SelectFields { + return $this->instanciateSelectFields($arguments, $fieldsAndArguments, $depth); }; } if (SelectFields::class === $className) { - return $this->instanciateSelectFields($arguments); + return $this->instanciateSelectFields($arguments, $fieldsAndArguments, null); } if (ResolveInfo::class === $className) { @@ -165,11 +192,22 @@ protected function getResolver(): ?Closure }; } - protected function instanciateSelectFields(array $arguments, int $depth = null): SelectFields + /** + * @param array $arguments + * @param int $depth + * @param array $fieldsAndArguments + * @return SelectFields + */ + private function instanciateSelectFields(array $arguments, array $fieldsAndArguments, int $depth = null): SelectFields { $ctx = $arguments[2] ?? null; - return new SelectFields($arguments[3], $this->type(), $arguments[1], $depth ?? 5, $ctx); + if ($depth !== null && $depth !== $this->depth) { + $fieldsAndArguments = (new ResolveInfoFieldsAndArguments($arguments[3])) + ->getFieldsAndArgumentsSelection($depth); + } + + return new SelectFields($this->type(), $arguments[1], $ctx, $fieldsAndArguments); } protected function aliasArgs(array $arguments): array diff --git a/src/Support/RulesInFields.php b/src/Support/RulesInFields.php new file mode 100644 index 00000000..a917a464 --- /dev/null +++ b/src/Support/RulesInFields.php @@ -0,0 +1,93 @@ + + */ + protected $fieldsAndArguments; + + /** + * @param Type $parentType + * @param array $fieldsAndArgumentsSelection + */ + public function __construct(Type $parentType, array $fieldsAndArgumentsSelection) + { + $this->parentType = $parentType instanceof WrappingType + ? $parentType->getWrappedType(true) + : $parentType; + $this->fieldsAndArguments = $fieldsAndArgumentsSelection; + } + + /** + * @return array + */ + public function get(): array + { + return $this->getRules($this->fieldsAndArguments, null, $this->parentType); + } + + /** + * @param array|string|callable $rules + * @param array $arguments + * @return array|string + */ + protected function resolveRules($rules, array $arguments) + { + if (is_callable($rules)) { + return $rules($arguments); + } + + return $rules; + } + + /** + * Get rules from fields. + * + * @param array $fields + * @param string|null $prefix + * @param Type $parentType + * @return array + */ + protected function getRules(array $fields, ?string $prefix, Type $parentType): array + { + $rules = []; + + foreach ($fields as $name => $field) { + $key = $prefix === null ? $name : "{$prefix}.{$name}"; + + //If field doesn't exist on definition we don't select it + if (! method_exists($parentType, 'getField')) { + continue; + } + + $fieldObject = $parentType->getField($name); + + if (is_array($field['fields'])) { + $rules = $rules + $this->getRules($field['fields'], $key.'.fields', $fieldObject->getType()); + } + + $args = $fieldObject->config['args'] ?? []; + + foreach ($args as $argName => $info) { + if (isset($info['rules'])) { + $rules[$key.'.args.'.$argName] = $this->resolveRules($info['rules'], $field['args']); + } + } + } + + return $rules; + } +} diff --git a/src/Support/SelectFields.php b/src/Support/SelectFields.php index 4ba50d90..33ca3e83 100644 --- a/src/Support/SelectFields.php +++ b/src/Support/SelectFields.php @@ -7,7 +7,6 @@ use Closure; use GraphQL\Error\InvariantViolation; use GraphQL\Type\Definition\FieldDefinition; -use GraphQL\Type\Definition\ResolveInfo; use GraphQL\Type\Definition\Type as GraphqlType; use GraphQL\Type\Definition\UnionType; use GraphQL\Type\Definition\WrappingType; @@ -32,35 +31,28 @@ class SelectFields const ALWAYS_RELATION_KEY = 'ALWAYS_RELATION_KEY'; /** - * @param ResolveInfo $info * @param GraphqlType $parentType * @param array $queryArgs Arguments given with the query/mutation - * @param int $depth The depth to walk the AST and introspect for nested relations * @param mixed $ctx The GraphQL context; can be anything and is only passed through * Can be created/overridden by \Rebing\GraphQL\GraphQLController::queryContext + * @param array $fieldsAndArguments Field and argument tree */ - public function __construct(ResolveInfo $info, GraphqlType $parentType, array $queryArgs, int $depth, $ctx) + public function __construct(GraphqlType $parentType, array $queryArgs, $ctx, array $fieldsAndArguments) { if ($parentType instanceof WrappingType) { $parentType = $parentType->getWrappedType(true); } - $requestedFields = $this->getFieldSelection($info, $queryArgs, $depth); - $fields = static::getSelectableFieldsAndRelations($queryArgs, $requestedFields, $parentType, null, true, $ctx); + $requestedFields = [ + 'args' => $queryArgs, + 'fields' => $fieldsAndArguments, + ]; + + $fields = self::getSelectableFieldsAndRelations($queryArgs, $requestedFields, $parentType, null, true, $ctx); $this->select = $fields[0]; $this->relations = $fields[1]; } - protected function getFieldSelection(ResolveInfo $resolveInfo, array $args, int $depth): array - { - $resolveInfoFieldsAndArguments = new ResolveInfoFieldsAndArguments($resolveInfo); - - return [ - 'args' => $args, - 'fields' => $resolveInfoFieldsAndArguments->getFieldsAndArgumentsSelection($depth), - ]; - } - /** * Retrieve the fields (top level) and relations that * will be selected with the query. diff --git a/tests/Unit/FieldTest.php b/tests/Unit/FieldTest.php index 10bda9c0..12b8be8e 100644 --- a/tests/Unit/FieldTest.php +++ b/tests/Unit/FieldTest.php @@ -5,6 +5,8 @@ namespace Rebing\GraphQL\Tests\Unit; use Closure; +use GraphQL\Type\Definition\ResolveInfo; +use PHPUnit\Framework\MockObject\MockObject; use Rebing\GraphQL\Tests\Support\Objects\ExampleField; use Rebing\GraphQL\Tests\TestCase; @@ -15,6 +17,13 @@ protected function getFieldClass() return ExampleField::class; } + protected function resolveInfoMock(): MockObject + { + return $this->getMockBuilder(ResolveInfo::class) + ->disableOriginalConstructor() + ->getMock(); + } + /** * Test get attributes. */ @@ -47,7 +56,7 @@ public function testResolve(): void ->method('resolve'); $attributes = $field->getAttributes(); - $attributes['resolve'](null, [], [], null); + $attributes['resolve'](null, [], [], $this->resolveInfoMock()); } /** diff --git a/tests/Unit/GraphQLQueryTest.php b/tests/Unit/GraphQLQueryTest.php index b18967b0..4343db1c 100644 --- a/tests/Unit/GraphQLQueryTest.php +++ b/tests/Unit/GraphQLQueryTest.php @@ -182,7 +182,7 @@ public function testQueryWithValidationError(): void $this->assertArrayHasKey('errors', $result); $this->assertArrayHasKey('extensions', $result['errors'][0]); $this->assertArrayHasKey('validation', $result['errors'][0]['extensions']); - $this->assertTrue($result['errors'][0]['extensions']['validation']->has('index')); + $this->assertTrue($result['errors'][0]['extensions']['validation']->has('test_validation.args.index')); } /** diff --git a/tests/Unit/MutationTest.php b/tests/Unit/MutationTest.php index e07b6607..e8b5830c 100644 --- a/tests/Unit/MutationTest.php +++ b/tests/Unit/MutationTest.php @@ -4,7 +4,9 @@ namespace Rebing\GraphQL\Tests\Unit; +use GraphQL\Type\Definition\ResolveInfo; use Illuminate\Validation\Validator; +use PHPUnit\Framework\MockObject\MockObject; use Rebing\GraphQL\Error\ValidationError; use Rebing\GraphQL\Tests\Support\Objects\ExampleNestedValidationInputObject; use Rebing\GraphQL\Tests\Support\Objects\ExampleRuleTestingInputObject; @@ -32,6 +34,13 @@ protected function getEnvironmentSetUp($app) ]); } + protected function resolveInfoMock(): MockObject + { + return $this->getMockBuilder(ResolveInfo::class) + ->disableOriginalConstructor() + ->getMock(); + } + /** * Test resolve. */ @@ -66,7 +75,7 @@ public function testResolve(): void ['email' => 'test@test.com'], ], ], - ], [], null); + ], [], $this->resolveInfoMock()); } /** @@ -79,7 +88,7 @@ public function testResolveThrowValidationError(): void $attributes = $field->getAttributes(); $this->expectException(ValidationError::class); - $attributes['resolve'](null, [], [], null); + $attributes['resolve'](null, [], [], $this->resolveInfoMock()); } /** @@ -98,7 +107,7 @@ public function testValidationError(): void 'test_with_rules_non_nullable_input_object' => [ 'val' => 4, ], - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -131,7 +140,7 @@ public function testWithInput(): void 'test_with_rules_non_nullable_input_object' => [ 'val' => 4, ], - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $e) { $validator = $e->getValidator(); @@ -160,7 +169,7 @@ public function testWithEmptyInput(): void $exception = null; try { - $attributes['resolve'](null, [], [], null); + $attributes['resolve'](null, [], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -190,7 +199,7 @@ public function testWithInputDepthOne(): void try { $attributes['resolve'](null, [ 'test_with_rules' => 'test', - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -220,7 +229,7 @@ public function testWithInputWithEmptyInputObjects(): void $attributes['resolve'](null, [ 'test_with_rules_non_nullable_input_object' => [], 'test_with_rules_nullable_input_object' => [], - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -259,7 +268,7 @@ public function testWithEmptyArrayOfInputsObjects(): void try { $attributes['resolve'](null, [ 'test_with_rules_non_nullable_list_of_non_nullable_input_object' => [], - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -291,7 +300,7 @@ public function testWithArrayOfInputsObjects(): void 'val' => 1245, ], ], - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -330,7 +339,7 @@ public function testCustomValidationErrorMessages(): void 'test_with_rules_non_nullable_input_object' => [ 'nest' => ['email' => 'invalidTestEmail.com'], ], - ], [], null); + ], [], $this->resolveInfoMock()); } catch (ValidationError $exception) { } @@ -352,6 +361,6 @@ public function testRuleCallbackArgumentsMatchesTheInput(): void 'test_with_rules_callback_params' => [ 'otherValue' => 1337, ], - ], [], null); + ], [], $this->resolveInfoMock()); } } diff --git a/tests/Unit/ValidationOfFieldArguments/AccountType.php b/tests/Unit/ValidationOfFieldArguments/AccountType.php new file mode 100644 index 00000000..ad40078e --- /dev/null +++ b/tests/Unit/ValidationOfFieldArguments/AccountType.php @@ -0,0 +1,58 @@ + + */ + protected $attributes = [ + 'name' => 'AccountType', + 'description' => 'An example', + ]; + + public function fields(): array + { + return [ + 'id' => [ + 'type' => Type::int(), + 'description' => 'A test field', + ], + 'profile' => [ + 'type' => GraphQL::type('ProfileType'), + 'args' => [ + 'profileId' => [ + 'type' => Type::int(), + 'rules' => function ($args) { + Assert::assertSame([ + 'profileId' => 100, + ], $args); + + return ['required', 'integer', 'max:10']; + }, + ], + ], + ], + 'alias' => [ + 'type' => Type::string(), + 'args' => [ + 'type' => [ + 'type' => Type::string(), + 'rules' => function ($args) { + return ['regex:/^l33t|normal$/i']; + }, + ], + ], + ], + + ]; + } +} diff --git a/tests/Unit/ValidationOfFieldArguments/ProfileType.php b/tests/Unit/ValidationOfFieldArguments/ProfileType.php new file mode 100644 index 00000000..457a6fa9 --- /dev/null +++ b/tests/Unit/ValidationOfFieldArguments/ProfileType.php @@ -0,0 +1,52 @@ + + */ + protected $attributes = [ + 'name' => 'ProfileType', + 'description' => 'An example', + ]; + + public function fields(): array + { + return [ + 'name' => [ + 'type' => Type::string(), + 'description' => 'A test field', + 'args' => [ + 'includeMiddleNames' => [ + 'type' => Type::string(), + 'rules' => ['regex:/^(yes|no)$/i'], + ], + ], + ], + 'height' => [ + 'type' => Type::string(), + 'description' => 'A test field', + 'args' => [ + 'unit' => [ + 'type' => Type::string(), + 'rules' => function ($args) { + Assert::assertSame([ + 'unit' => 'not_correct', + ], $args); + + return ['regex:/^inch|cm$/i']; + }, + ], + ], + ], + ]; + } +} diff --git a/tests/Unit/ValidationOfFieldArguments/TestQuery.php b/tests/Unit/ValidationOfFieldArguments/TestQuery.php new file mode 100644 index 00000000..173aab48 --- /dev/null +++ b/tests/Unit/ValidationOfFieldArguments/TestQuery.php @@ -0,0 +1,48 @@ + + */ + protected $attributes = [ + 'name' => 'test', + ]; + + public function type(): Type + { + return GraphQL::type('AccountType'); + } + + public function args(): array + { + return [ + 'id' => [ + 'type' => Type::int(), + 'rules' => ['integer', 'max:2'], + ], + ]; + } + + /** + * @param mixed $root + * @param array $args + * @param mixed $context + * @return array + */ + public function resolve($root, array $args, $context): array + { + return [ + 'name' => 'fff', + 'profile' => null, + ]; + } +} diff --git a/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php b/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php new file mode 100644 index 00000000..8b1ecd80 --- /dev/null +++ b/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php @@ -0,0 +1,82 @@ +set('graphql.types', [ + 'AccountType' => AccountType::class, + 'ProfileType' => ProfileType::class, + ]); + + $app['config']->set('graphql.schemas.default', [ + 'query' => [ + TestQuery::class, + ], + ]); + } + + public function testRulesTakesEffect(): void + { + $graphql = <<<'GRAPHQL' +query ($profileId: Int, $height: String) { + test { + id + profile(profileId: $profileId) { + name(includeMiddleNames: "maybe") + height(unit: $height) + } + + } +} +GRAPHQL; + + $result = $this->graphql($graphql, [ + 'expectErrors' => true, + 'variables' => [ + 'profileId' => 100, + 'height' => 'not_correct', + ], + ]); + + /** @var MessageBag $messageBag */ + $messageBag = $result['errors'][0]['extensions']['validation']; + $expectedMessages = [ + 'The profile.fields.name.args.include middle names format is invalid.', + 'The profile.fields.height.args.unit format is invalid.', + 'The profile.args.profile id may not be greater than 10.', + ]; + $this->assertSame($expectedMessages, $messageBag->all()); + } + + public function testOnlyApplicableRulesTakesEffect(): void + { + $graphql = <<<'GRAPHQL' +query { + test { + id + alias(type:"not_it") + } +} +GRAPHQL; + + $result = $this->graphql($graphql, [ + 'expectErrors' => true, + 'variables' => [], + ]); + + /** @var MessageBag $messageBag */ + $messageBag = $result['errors'][0]['extensions']['validation']; + $expectedMessages = [ + 'The alias.args.type format is invalid.', + ]; + $this->assertSame($expectedMessages, $messageBag->all()); + } +} From c48de40ba7dfaf2b72c8a048ffaebf825b75a75b Mon Sep 17 00:00:00 2001 From: Christian Nielsen Date: Mon, 8 Jun 2020 09:20:59 +0200 Subject: [PATCH 2/3] fix __typename --- CHANGELOG.md | 5 +++++ src/Support/RulesInFields.php | 11 +++++++---- .../ValidationOfFieldArgumentsTest.php | 1 + 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac04fe7e..c02bb3d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ CHANGELOG [Next release](https://github.com/rebing/graphql-laravel/compare/5.1.1...master) -------------- +2019-06-8, 5.1.2 +----------------- +### Added +- Readded support for validation in field arguments (with breaking change fix) [\#630 / crissi](https://github.com/rebing/graphql-laravel/pull/630) + 2019-04-23, 5.1.1 ----------------- ### Fixed diff --git a/src/Support/RulesInFields.php b/src/Support/RulesInFields.php index a917a464..e3150363 100644 --- a/src/Support/RulesInFields.php +++ b/src/Support/RulesInFields.php @@ -4,6 +4,7 @@ namespace Rebing\GraphQL\Support; +use GraphQL\Error\InvariantViolation; use GraphQL\Type\Definition\Type; use GraphQL\Type\Definition\WrappingType; @@ -68,13 +69,15 @@ protected function getRules(array $fields, ?string $prefix, Type $parentType): a foreach ($fields as $name => $field) { $key = $prefix === null ? $name : "{$prefix}.{$name}"; - //If field doesn't exist on definition we don't select it - if (! method_exists($parentType, 'getField')) { + try { + if (! method_exists($parentType, 'getField')) { + continue; + } + $fieldObject = $parentType->getField($name); + } catch (InvariantViolation $e) { continue; } - $fieldObject = $parentType->getField($name); - if (is_array($field['fields'])) { $rules = $rules + $this->getRules($field['fields'], $key.'.fields', $fieldObject->getType()); } diff --git a/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php b/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php index 8b1ecd80..0f6d9384 100644 --- a/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php +++ b/tests/Unit/ValidationOfFieldArguments/ValidationOfFieldArgumentsTest.php @@ -29,6 +29,7 @@ public function testRulesTakesEffect(): void query ($profileId: Int, $height: String) { test { id + __typename profile(profileId: $profileId) { name(includeMiddleNames: "maybe") height(unit: $height) From 1606e9a8fa751f449d818a39fd74182c5cb669f5 Mon Sep 17 00:00:00 2001 From: Markus Podar Date: Mon, 8 Jun 2020 20:30:52 +0200 Subject: [PATCH 3/3] changelog: don't decide on date yet --- CHANGELOG.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c02bb3d4..3da5bdbe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,9 +3,6 @@ CHANGELOG [Next release](https://github.com/rebing/graphql-laravel/compare/5.1.1...master) -------------- - -2019-06-8, 5.1.2 ------------------ ### Added - Readded support for validation in field arguments (with breaking change fix) [\#630 / crissi](https://github.com/rebing/graphql-laravel/pull/630)