From 2c073af4a1b344631857070fe67e78bd6497dabd Mon Sep 17 00:00:00 2001 From: Carla Gouveia Date: Sat, 26 Aug 2023 17:52:12 -0400 Subject: [PATCH 01/11] Added ability to control field visibility on schema definition. --- src/Executor/ReferenceExecutor.php | 4 +++ src/Type/Definition/FieldDefinition.php | 20 +++++++++++ src/Type/Introspection.php | 23 ++++++++----- tests/Executor/ExecutorSchemaTest.php | 10 +++++- tests/Type/IntrospectionTest.php | 46 +++++++++++++++++++++++++ 5 files changed, 94 insertions(+), 9 deletions(-) diff --git a/src/Executor/ReferenceExecutor.php b/src/Executor/ReferenceExecutor.php index fa3c63379..f35976ed2 100644 --- a/src/Executor/ReferenceExecutor.php +++ b/src/Executor/ReferenceExecutor.php @@ -601,6 +601,10 @@ protected function resolveField(ObjectType $parentType, $rootValue, \ArrayObject return static::$UNDEFINED; } + if (! $fieldDef->isVisible($contextValue)) { + return static::$UNDEFINED; + } + $returnType = $fieldDef->getType(); // The resolve function's optional 3rd argument is a context value that // is provided to every resolve function within an execution. It is commonly diff --git a/src/Type/Definition/FieldDefinition.php b/src/Type/Definition/FieldDefinition.php index 7b9389b11..a51d12f94 100644 --- a/src/Type/Definition/FieldDefinition.php +++ b/src/Type/Definition/FieldDefinition.php @@ -16,12 +16,14 @@ * * @phpstan-type FieldType (Type&OutputType)|callable(): (Type&OutputType) * @phpstan-type ComplexityFn callable(int, array): int + * @phpstan-type VisibilityFn callable(mixed): bool * @phpstan-type FieldDefinitionConfig array{ * name: string, * type: FieldType, * resolve?: FieldResolver|null, * args?: ArgumentListConfig|null, * description?: string|null, + * visible?: VisibilityFn|bool, * deprecationReason?: string|null, * astNode?: FieldDefinitionNode|null, * complexity?: ComplexityFn|null @@ -31,6 +33,7 @@ * resolve?: FieldResolver|null, * args?: ArgumentListConfig|null, * description?: string|null, + * visible?: VisibilityFn|bool, * deprecationReason?: string|null, * astNode?: FieldDefinitionNode|null, * complexity?: ComplexityFn|null @@ -64,6 +67,13 @@ class FieldDefinition public ?string $description; + /** + * @var callable|bool + * + * @phpstan-var VisibilityFn|bool + */ + public $visible; + public ?string $deprecationReason; public ?FieldDefinitionNode $astNode; @@ -94,6 +104,7 @@ public function __construct(array $config) ? Argument::listFromConfig($config['args']) : []; $this->description = $config['description'] ?? null; + $this->visible = $config['visible'] ?? true; $this->deprecationReason = $config['deprecationReason'] ?? null; $this->astNode = $config['astNode'] ?? null; $this->complexityFn = $config['complexity'] ?? null; @@ -181,6 +192,15 @@ public function getType(): Type return $this->type ??= Schema::resolveType($this->config['type']); } + public function isVisible(mixed $context): bool + { + if (is_callable($this->visible)) { + return call_user_func($this->visible, $context); + } + + return true; + } + public function isDeprecated(): bool { return (bool) $this->deprecationReason; diff --git a/src/Type/Introspection.php b/src/Type/Introspection.php index ab6a39b98..3b2087185 100644 --- a/src/Type/Introspection.php +++ b/src/Type/Introspection.php @@ -352,17 +352,24 @@ public static function _type(): ObjectType 'defaultValue' => false, ], ], - 'resolve' => static function (Type $type, $args): ?array { + 'resolve' => static function (Type $type, $args, $context): ?array { if ($type instanceof ObjectType || $type instanceof InterfaceType) { $fields = $type->getFields(); - if (! ($args['includeDeprecated'] ?? false)) { - $fields = \array_filter( - $fields, - static fn (FieldDefinition $field): bool => $field->deprecationReason === null - || $field->deprecationReason === '' - ); - } + $fields = \array_filter( + $fields, + static function (FieldDefinition $field) use ($args, $context): bool { + if (! $field->isVisible($context)) { + return false; + } + + if ($field->isDeprecated() && ! ($args['includeDeprecated'] ?? false)) { + return false; + } + + return true; + } + ); return $fields; } diff --git a/tests/Executor/ExecutorSchemaTest.php b/tests/Executor/ExecutorSchemaTest.php index 0fed1d087..aa2d6a336 100644 --- a/tests/Executor/ExecutorSchemaTest.php +++ b/tests/Executor/ExecutorSchemaTest.php @@ -30,6 +30,12 @@ public function testExecutesUsingASchema(): void 'url' => ['type' => Type::string()], 'width' => ['type' => Type::int()], 'height' => ['type' => Type::int()], + 'mimetype' => [ + 'type' => Type::string(), + 'visible' => function ($context): bool { + return false; + }, + ], ], ]); @@ -107,7 +113,8 @@ public function testExecutesUsingASchema(): void pic(width: 640, height: 480) { url, width, - height + height, + mimetype }, recentArticle { ...articleFields, @@ -222,6 +229,7 @@ private function article(string $id): array 'url' => "cdn://{$uid}", 'width' => $width, 'height' => $height, + 'mimetype' => 'image/gif', ]; $johnSmith = [ diff --git a/tests/Type/IntrospectionTest.php b/tests/Type/IntrospectionTest.php index a6402d9dc..dbc9baa8f 100644 --- a/tests/Type/IntrospectionTest.php +++ b/tests/Type/IntrospectionTest.php @@ -1725,4 +1725,50 @@ public function testExecutesAnIntrospectionQueryWithoutCallingGlobalFieldResolve GraphQL::executeQuery($schema, $source, null, null, null, null, $fieldResolver); self::assertEmpty($calledForFields); } + + /** @see it('does not expose invisible fields') */ + public function testDoesNotExposeInvisibleFields(): void + { + $TestType = new ObjectType([ + 'name' => 'TestType', + 'fields' => [ + 'nonVisible' => [ + 'type' => Type::string(), + 'visible' => function ($context): bool { + return false; + }, + ], + 'visible' => [ + 'type' => Type::string(), + ], + ], + ]); + + $schema = new Schema(['query' => $TestType]); + $request = ' + { + __type(name: "TestType") { + name + fields { + name + } + } + } + '; + + $expected = [ + 'data' => [ + '__type' => [ + 'name' => 'TestType', + 'fields' => [ + [ + 'name' => 'visible', + ], + ], + ], + ], + ]; + + self::assertEquals($expected, GraphQL::executeQuery($schema, $request)->toArray()); + } } From e33712fdcbf3f66f5ed46805a4e8f9633ea110f2 Mon Sep 17 00:00:00 2001 From: Carla Gouveia Date: Sat, 2 Sep 2023 13:05:11 -0400 Subject: [PATCH 02/11] Adjusted code based on PR feedbacks. --- src/Type/Definition/FieldDefinition.php | 8 +++----- tests/Executor/ExecutorSchemaTest.php | 20 ++++++++++++++------ tests/Type/IntrospectionTest.php | 25 +++++++++++++++++++------ 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/src/Type/Definition/FieldDefinition.php b/src/Type/Definition/FieldDefinition.php index a51d12f94..c8a89e991 100644 --- a/src/Type/Definition/FieldDefinition.php +++ b/src/Type/Definition/FieldDefinition.php @@ -194,11 +194,9 @@ public function getType(): Type public function isVisible(mixed $context): bool { - if (is_callable($this->visible)) { - return call_user_func($this->visible, $context); - } - - return true; + return is_callable($this->visible) + ? ($this->visible)($context) + : $this->visible; } public function isDeprecated(): bool diff --git a/tests/Executor/ExecutorSchemaTest.php b/tests/Executor/ExecutorSchemaTest.php index aa2d6a336..1a35349d1 100644 --- a/tests/Executor/ExecutorSchemaTest.php +++ b/tests/Executor/ExecutorSchemaTest.php @@ -28,15 +28,23 @@ public function testExecutesUsingASchema(): void 'name' => 'Image', 'fields' => [ 'url' => ['type' => Type::string()], - 'width' => ['type' => Type::int()], - 'height' => ['type' => Type::int()], + 'width' => [ + 'type' => Type::int(), + 'visible' => fn ($context): bool => true, + ], + 'height' => [ + 'type' => Type::int(), + 'visible' => true, + ], 'mimetype' => [ 'type' => Type::string(), - 'visible' => function ($context): bool { - return false; - }, + 'visible' => fn ($context): bool => false, ], - ], + 'size' => [ + 'type' => Type::string(), + 'visible' => false, + ], + ], ]); $BlogAuthor = new ObjectType([ diff --git a/tests/Type/IntrospectionTest.php b/tests/Type/IntrospectionTest.php index dbc9baa8f..8c363636a 100644 --- a/tests/Type/IntrospectionTest.php +++ b/tests/Type/IntrospectionTest.php @@ -1726,17 +1726,15 @@ public function testExecutesAnIntrospectionQueryWithoutCallingGlobalFieldResolve self::assertEmpty($calledForFields); } - /** @see it('does not expose invisible fields') */ - public function testDoesNotExposeInvisibleFields(): void + /** @dataProvider invisibleFieldDataProvider */ + public function testDoesNotExposeInvisibleFields($visible): void { $TestType = new ObjectType([ 'name' => 'TestType', 'fields' => [ 'nonVisible' => [ 'type' => Type::string(), - 'visible' => function ($context): bool { - return false; - }, + 'visible' => $visible, ], 'visible' => [ 'type' => Type::string(), @@ -1769,6 +1767,21 @@ public function testDoesNotExposeInvisibleFields(): void ], ]; - self::assertEquals($expected, GraphQL::executeQuery($schema, $request)->toArray()); + self::assertSame($expected, GraphQL::executeQuery($schema, $request)->toArray()); } + + /** + * @return array + */ + public static function invisibleFieldDataProvider(): array + { + return [ + [ + fn ($context): bool => false, + ], + [ + false, + ] + ]; + } } From b3ad2c1d6dbbf8fc8686215c1641551db1b6a773 Mon Sep 17 00:00:00 2001 From: Carla Gouveia Date: Sat, 23 Sep 2023 17:44:03 -0400 Subject: [PATCH 03/11] Added validation for invisible fields. --- src/Type/Definition/FieldDefinition.php | 6 +-- src/Validator/DocumentValidator.php | 2 + src/Validator/Rules/NoInvisibleFields.php | 38 +++++++++++++++++++ tests/Executor/ExecutorSchemaTest.php | 20 +++++----- tests/StarWarsSchema.php | 5 +++ tests/StarWarsValidationTest.php | 13 +++++++ tests/Type/IntrospectionTest.php | 34 +++++++++-------- tests/Validator/NoInvisibleFieldsTest.php | 45 +++++++++++++++++++++++ tests/Validator/ValidatorTestCase.php | 4 ++ 9 files changed, 138 insertions(+), 29 deletions(-) create mode 100644 src/Validator/Rules/NoInvisibleFields.php create mode 100644 tests/Validator/NoInvisibleFieldsTest.php diff --git a/src/Type/Definition/FieldDefinition.php b/src/Type/Definition/FieldDefinition.php index c8a89e991..40e7d4e5f 100644 --- a/src/Type/Definition/FieldDefinition.php +++ b/src/Type/Definition/FieldDefinition.php @@ -194,9 +194,9 @@ public function getType(): Type public function isVisible(mixed $context): bool { - return is_callable($this->visible) - ? ($this->visible)($context) - : $this->visible; + return is_callable($this->visible) + ? ($this->visible)($context) + : $this->visible; } public function isDeprecated(): bool diff --git a/src/Validator/DocumentValidator.php b/src/Validator/DocumentValidator.php index 82a7d8488..cbb263707 100644 --- a/src/Validator/DocumentValidator.php +++ b/src/Validator/DocumentValidator.php @@ -19,6 +19,7 @@ use GraphQL\Validator\Rules\LoneAnonymousOperation; use GraphQL\Validator\Rules\LoneSchemaDefinition; use GraphQL\Validator\Rules\NoFragmentCycles; +use GraphQL\Validator\Rules\NoInvisibleFields; use GraphQL\Validator\Rules\NoUndefinedVariables; use GraphQL\Validator\Rules\NoUnusedFragments; use GraphQL\Validator\Rules\NoUnusedVariables; @@ -170,6 +171,7 @@ public static function defaultRules(): array UniqueVariableNames::class => new UniqueVariableNames(), NoUndefinedVariables::class => new NoUndefinedVariables(), NoUnusedVariables::class => new NoUnusedVariables(), + NoInvisibleFields::class => new NoInvisibleFields(), KnownDirectives::class => new KnownDirectives(), UniqueDirectivesPerLocation::class => new UniqueDirectivesPerLocation(), KnownArgumentNames::class => new KnownArgumentNames(), diff --git a/src/Validator/Rules/NoInvisibleFields.php b/src/Validator/Rules/NoInvisibleFields.php new file mode 100644 index 000000000..4c9b1428f --- /dev/null +++ b/src/Validator/Rules/NoInvisibleFields.php @@ -0,0 +1,38 @@ + function (FieldNode $node) use ($context): void { + $fieldDef = $context->getFieldDef(); + if ($fieldDef === null) { + return; + } + + $parentType = $context->getParentType(); + if (! $parentType instanceof NamedType) { + return; + } + + if ($fieldDef->isVisible($context)) { + return; + } + + // Report an error, including helpful suggestions. + $context->reportError(new Error( + "Cannot query field \"{$node->name->value}\" on type \"{$parentType->name}\"." + )); + }, + ]; + } +} diff --git a/tests/Executor/ExecutorSchemaTest.php b/tests/Executor/ExecutorSchemaTest.php index 1a35349d1..5154e083f 100644 --- a/tests/Executor/ExecutorSchemaTest.php +++ b/tests/Executor/ExecutorSchemaTest.php @@ -29,22 +29,22 @@ public function testExecutesUsingASchema(): void 'fields' => [ 'url' => ['type' => Type::string()], 'width' => [ - 'type' => Type::int(), - 'visible' => fn ($context): bool => true, + 'type' => Type::int(), + 'visible' => fn ($context): bool => true, ], 'height' => [ - 'type' => Type::int(), - 'visible' => true, + 'type' => Type::int(), + 'visible' => true, ], 'mimetype' => [ 'type' => Type::string(), - 'visible' => fn ($context): bool => false, + 'visible' => fn ($context): bool => false, ], - 'size' => [ - 'type' => Type::string(), - 'visible' => false, - ], - ], + 'size' => [ + 'type' => Type::string(), + 'visible' => false, + ], + ], ]); $BlogAuthor = new ObjectType([ diff --git a/tests/StarWarsSchema.php b/tests/StarWarsSchema.php index ccd67c33e..a2db75671 100644 --- a/tests/StarWarsSchema.php +++ b/tests/StarWarsSchema.php @@ -129,6 +129,11 @@ public static function build(): Schema 'type' => Type::string(), 'description' => 'All secrets about their past.', ], + 'secretName' => [ + 'type' => Type::string(), + 'description' => 'The secret name of the character.', + 'visible' => false, + ], ]; }, 'resolveType' => static function (array $obj) use (&$humanType, &$droidType): ObjectType { diff --git a/tests/StarWarsValidationTest.php b/tests/StarWarsValidationTest.php index 07e7a59d4..3e1a6abf3 100644 --- a/tests/StarWarsValidationTest.php +++ b/tests/StarWarsValidationTest.php @@ -69,6 +69,19 @@ public function testThatNonExistentFieldsAreInvalid(): void self::assertCount(1, $errors); } + public function testThatInvisibleFieldsAreInvalid(): void + { + $query = ' + query HeroSpaceshipQuery { + hero { + secretName + } + } + '; + $errors = $this->validationErrors($query); + self::assertCount(1, $errors); + } + /** @see it('Requires fields on objects') */ public function testRequiresFieldsOnObjects(): void { diff --git a/tests/Type/IntrospectionTest.php b/tests/Type/IntrospectionTest.php index 8c363636a..ebd503386 100644 --- a/tests/Type/IntrospectionTest.php +++ b/tests/Type/IntrospectionTest.php @@ -1726,8 +1726,12 @@ public function testExecutesAnIntrospectionQueryWithoutCallingGlobalFieldResolve self::assertEmpty($calledForFields); } - /** @dataProvider invisibleFieldDataProvider */ - public function testDoesNotExposeInvisibleFields($visible): void + /** + * @param callable|bool $visible + * + * @dataProvider invisibleFieldDataProvider + */ + public function testDoesNotExposeInvisibleFields(mixed $visible): void { $TestType = new ObjectType([ 'name' => 'TestType', @@ -1770,18 +1774,16 @@ public function testDoesNotExposeInvisibleFields($visible): void self::assertSame($expected, GraphQL::executeQuery($schema, $request)->toArray()); } - /** - * @return array - */ - public static function invisibleFieldDataProvider(): array - { - return [ - [ - fn ($context): bool => false, - ], - [ - false, - ] - ]; - } + /** @return array> */ + public static function invisibleFieldDataProvider(): array + { + return [ + [ + fn ($context): bool => false, + ], + [ + false, + ], + ]; + } } diff --git a/tests/Validator/NoInvisibleFieldsTest.php b/tests/Validator/NoInvisibleFieldsTest.php new file mode 100644 index 000000000..195019244 --- /dev/null +++ b/tests/Validator/NoInvisibleFieldsTest.php @@ -0,0 +1,45 @@ +expectPassesRule( + new NoInvisibleFields(), + ' + fragment objectFieldSelection on Dog { + __typename + name + } + ' + ); + } + + public function testFailsValidationOnInvisibleField(): void + { + $this->expectFailsRule( + new NoInvisibleFields(), + ' + fragment objectFieldSelection on Dog { + __typename + secretName + } + ', + [ + [ + 'message' => 'Cannot query field "secretName" on type "Dog".', + ], + ] + ); + } +} diff --git a/tests/Validator/ValidatorTestCase.php b/tests/Validator/ValidatorTestCase.php index 78ca2da5c..3db1c2047 100644 --- a/tests/Validator/ValidatorTestCase.php +++ b/tests/Validator/ValidatorTestCase.php @@ -118,6 +118,10 @@ public static function getTestSchema(): Schema 'type' => Type::boolean(), 'args' => ['x' => ['type' => Type::int()], 'y' => ['type' => Type::int()]], ], + 'secretName' => [ + 'type' => Type::string(), + 'visible' => false, + ], ], 'interfaces' => [$Being, $Pet, $Canine], ]); From ebbd20e724f956552835694dcd7b85d06fdc7e92 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 26 Sep 2023 09:31:04 +0200 Subject: [PATCH 04/11] Apply rector and clean up --- src/Type/Introspection.php | 13 +++++-------- tests/Type/IntrospectionTest.php | 14 ++++---------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/src/Type/Introspection.php b/src/Type/Introspection.php index 3b2087185..ce87d9d89 100644 --- a/src/Type/Introspection.php +++ b/src/Type/Introspection.php @@ -356,22 +356,19 @@ public static function _type(): ObjectType if ($type instanceof ObjectType || $type instanceof InterfaceType) { $fields = $type->getFields(); - $fields = \array_filter( + return \array_filter( $fields, static function (FieldDefinition $field) use ($args, $context): bool { if (! $field->isVisible($context)) { return false; } - if ($field->isDeprecated() && ! ($args['includeDeprecated'] ?? false)) { - return false; - } - - return true; + return ! ( + $field->isDeprecated() + && ! ($args['includeDeprecated'] ?? false) + ); } ); - - return $fields; } return null; diff --git a/tests/Type/IntrospectionTest.php b/tests/Type/IntrospectionTest.php index ebd503386..c70b660a7 100644 --- a/tests/Type/IntrospectionTest.php +++ b/tests/Type/IntrospectionTest.php @@ -1774,16 +1774,10 @@ public function testDoesNotExposeInvisibleFields(mixed $visible): void self::assertSame($expected, GraphQL::executeQuery($schema, $request)->toArray()); } - /** @return array> */ - public static function invisibleFieldDataProvider(): array + /** @return iterable */ + public static function invisibleFieldDataProvider(): iterable { - return [ - [ - fn ($context): bool => false, - ], - [ - false, - ], - ]; + yield [fn ($context): bool => false]; + yield [false]; } } From f731f79181290696213ba6031538c55b5e4a8d9a Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 26 Sep 2023 10:03:05 +0200 Subject: [PATCH 05/11] Simplify implementation and tests --- Makefile | 1 + src/Executor/ReferenceExecutor.php | 6 +-- src/Type/Definition/FieldDefinition.php | 6 +-- .../HasFieldsTypeImplementation.php | 11 ++++- src/Type/Introspection.php | 15 +++---- src/Validator/DocumentValidator.php | 2 - src/Validator/Rules/FieldsOnCorrectType.php | 2 +- src/Validator/Rules/NoInvisibleFields.php | 38 ---------------- tests/Executor/ExecutorSchemaTest.php | 4 +- tests/Type/IntrospectionTest.php | 8 ++-- tests/Validator/FieldsOnCorrectTypeTest.php | 14 ++++++ tests/Validator/NoInvisibleFieldsTest.php | 45 ------------------- 12 files changed, 43 insertions(+), 109 deletions(-) delete mode 100644 src/Validator/Rules/NoInvisibleFields.php delete mode 100644 tests/Validator/NoInvisibleFieldsTest.php diff --git a/Makefile b/Makefile index 04e388edd..55d700aa4 100644 --- a/Makefile +++ b/Makefile @@ -31,6 +31,7 @@ bench: ## Runs benchmarks with phpbench .PHONY: docs docs: ## Generate the class-reference docs php generate-class-reference.php + prettier --write docs/class-reference.md vendor: composer.json composer.lock composer install diff --git a/src/Executor/ReferenceExecutor.php b/src/Executor/ReferenceExecutor.php index f35976ed2..2aeed90b7 100644 --- a/src/Executor/ReferenceExecutor.php +++ b/src/Executor/ReferenceExecutor.php @@ -597,11 +597,7 @@ protected function resolveField(ObjectType $parentType, $rootValue, \ArrayObject $fieldName = $fieldNode->name->value; $fieldDef = $this->getFieldDef($exeContext->schema, $parentType, $fieldName); - if ($fieldDef === null) { - return static::$UNDEFINED; - } - - if (! $fieldDef->isVisible($contextValue)) { + if ($fieldDef === null || ! $fieldDef->isVisible()) { return static::$UNDEFINED; } diff --git a/src/Type/Definition/FieldDefinition.php b/src/Type/Definition/FieldDefinition.php index 40e7d4e5f..496502ee2 100644 --- a/src/Type/Definition/FieldDefinition.php +++ b/src/Type/Definition/FieldDefinition.php @@ -16,7 +16,7 @@ * * @phpstan-type FieldType (Type&OutputType)|callable(): (Type&OutputType) * @phpstan-type ComplexityFn callable(int, array): int - * @phpstan-type VisibilityFn callable(mixed): bool + * @phpstan-type VisibilityFn callable(): bool * @phpstan-type FieldDefinitionConfig array{ * name: string, * type: FieldType, @@ -192,10 +192,10 @@ public function getType(): Type return $this->type ??= Schema::resolveType($this->config['type']); } - public function isVisible(mixed $context): bool + public function isVisible(): bool { return is_callable($this->visible) - ? ($this->visible)($context) + ? ($this->visible)() : $this->visible; } diff --git a/src/Type/Definition/HasFieldsTypeImplementation.php b/src/Type/Definition/HasFieldsTypeImplementation.php index d7478f634..a8cef1f9b 100644 --- a/src/Type/Definition/HasFieldsTypeImplementation.php +++ b/src/Type/Definition/HasFieldsTypeImplementation.php @@ -83,6 +83,15 @@ public function getFieldNames(): array { $this->initializeFields(); - return \array_keys($this->fields); + $visibleFields = array_filter( + $this->getFields(), + fn (FieldDefinition $fieldDefinition): bool => $fieldDefinition->isVisible() + ); + $fieldNames = array_map( + fn (FieldDefinition $fieldDefinition): string => $fieldDefinition->getName(), + $visibleFields + ); + + return array_values($fieldNames); } } diff --git a/src/Type/Introspection.php b/src/Type/Introspection.php index ce87d9d89..d7a1494f3 100644 --- a/src/Type/Introspection.php +++ b/src/Type/Introspection.php @@ -358,15 +358,12 @@ public static function _type(): ObjectType return \array_filter( $fields, - static function (FieldDefinition $field) use ($args, $context): bool { - if (! $field->isVisible($context)) { - return false; - } - - return ! ( - $field->isDeprecated() - && ! ($args['includeDeprecated'] ?? false) - ); + static function (FieldDefinition $field) use ($args): bool { + return $field->isVisible() + && ! ( + $field->isDeprecated() + && ! ($args['includeDeprecated'] ?? false) + ); } ); } diff --git a/src/Validator/DocumentValidator.php b/src/Validator/DocumentValidator.php index cbb263707..82a7d8488 100644 --- a/src/Validator/DocumentValidator.php +++ b/src/Validator/DocumentValidator.php @@ -19,7 +19,6 @@ use GraphQL\Validator\Rules\LoneAnonymousOperation; use GraphQL\Validator\Rules\LoneSchemaDefinition; use GraphQL\Validator\Rules\NoFragmentCycles; -use GraphQL\Validator\Rules\NoInvisibleFields; use GraphQL\Validator\Rules\NoUndefinedVariables; use GraphQL\Validator\Rules\NoUnusedFragments; use GraphQL\Validator\Rules\NoUnusedVariables; @@ -171,7 +170,6 @@ public static function defaultRules(): array UniqueVariableNames::class => new UniqueVariableNames(), NoUndefinedVariables::class => new NoUndefinedVariables(), NoUnusedVariables::class => new NoUnusedVariables(), - NoInvisibleFields::class => new NoInvisibleFields(), KnownDirectives::class => new KnownDirectives(), UniqueDirectivesPerLocation::class => new UniqueDirectivesPerLocation(), KnownArgumentNames::class => new KnownArgumentNames(), diff --git a/src/Validator/Rules/FieldsOnCorrectType.php b/src/Validator/Rules/FieldsOnCorrectType.php index 80ad50c3f..38902dbbc 100644 --- a/src/Validator/Rules/FieldsOnCorrectType.php +++ b/src/Validator/Rules/FieldsOnCorrectType.php @@ -20,7 +20,7 @@ public function getVisitor(QueryValidationContext $context): array return [ NodeKind::FIELD => function (FieldNode $node) use ($context): void { $fieldDef = $context->getFieldDef(); - if ($fieldDef !== null) { + if ($fieldDef !== null && $fieldDef->isVisible()) { return; } diff --git a/src/Validator/Rules/NoInvisibleFields.php b/src/Validator/Rules/NoInvisibleFields.php deleted file mode 100644 index 4c9b1428f..000000000 --- a/src/Validator/Rules/NoInvisibleFields.php +++ /dev/null @@ -1,38 +0,0 @@ - function (FieldNode $node) use ($context): void { - $fieldDef = $context->getFieldDef(); - if ($fieldDef === null) { - return; - } - - $parentType = $context->getParentType(); - if (! $parentType instanceof NamedType) { - return; - } - - if ($fieldDef->isVisible($context)) { - return; - } - - // Report an error, including helpful suggestions. - $context->reportError(new Error( - "Cannot query field \"{$node->name->value}\" on type \"{$parentType->name}\"." - )); - }, - ]; - } -} diff --git a/tests/Executor/ExecutorSchemaTest.php b/tests/Executor/ExecutorSchemaTest.php index 5154e083f..2350b58d5 100644 --- a/tests/Executor/ExecutorSchemaTest.php +++ b/tests/Executor/ExecutorSchemaTest.php @@ -30,7 +30,7 @@ public function testExecutesUsingASchema(): void 'url' => ['type' => Type::string()], 'width' => [ 'type' => Type::int(), - 'visible' => fn ($context): bool => true, + 'visible' => fn (): bool => true, ], 'height' => [ 'type' => Type::int(), @@ -38,7 +38,7 @@ public function testExecutesUsingASchema(): void ], 'mimetype' => [ 'type' => Type::string(), - 'visible' => fn ($context): bool => false, + 'visible' => fn (): bool => false, ], 'size' => [ 'type' => Type::string(), diff --git a/tests/Type/IntrospectionTest.php b/tests/Type/IntrospectionTest.php index c70b660a7..0683bbf22 100644 --- a/tests/Type/IntrospectionTest.php +++ b/tests/Type/IntrospectionTest.php @@ -7,6 +7,7 @@ use GraphQL\Language\SourceLocation; use GraphQL\Tests\ErrorHelper; use GraphQL\Type\Definition\EnumType; +use GraphQL\Type\Definition\FieldDefinition; use GraphQL\Type\Definition\InputObjectType; use GraphQL\Type\Definition\ObjectType; use GraphQL\Type\Definition\ResolveInfo; @@ -19,6 +20,7 @@ use function Safe\json_encode; +/** @phpstan-import-type VisibilityFn from FieldDefinition */ final class IntrospectionTest extends TestCase { use ArraySubsetAsserts; @@ -1727,7 +1729,7 @@ public function testExecutesAnIntrospectionQueryWithoutCallingGlobalFieldResolve } /** - * @param callable|bool $visible + * @param VisibilityFn|bool $visible * * @dataProvider invisibleFieldDataProvider */ @@ -1774,10 +1776,10 @@ public function testDoesNotExposeInvisibleFields(mixed $visible): void self::assertSame($expected, GraphQL::executeQuery($schema, $request)->toArray()); } - /** @return iterable */ + /** @return iterable */ public static function invisibleFieldDataProvider(): iterable { - yield [fn ($context): bool => false]; + yield [fn (): bool => false]; yield [false]; } } diff --git a/tests/Validator/FieldsOnCorrectTypeTest.php b/tests/Validator/FieldsOnCorrectTypeTest.php index 8aef374f2..c0f70f344 100644 --- a/tests/Validator/FieldsOnCorrectTypeTest.php +++ b/tests/Validator/FieldsOnCorrectTypeTest.php @@ -367,4 +367,18 @@ public function testLimitsLotsOfFieldSuggestions(): void ) ); } + + public function testFailsValidationOnInvisibleField(): void + { + $this->expectFailsRule( + new FieldsOnCorrectType(), + <<<'GRAPHQL' + fragment DogWithInvisibleField on Dog { + __typename + secretName + } + GRAPHQL, + [$this->undefinedField('secretName', 'Dog', [], ['nickname'], 3, 3)] + ); + } } diff --git a/tests/Validator/NoInvisibleFieldsTest.php b/tests/Validator/NoInvisibleFieldsTest.php deleted file mode 100644 index 195019244..000000000 --- a/tests/Validator/NoInvisibleFieldsTest.php +++ /dev/null @@ -1,45 +0,0 @@ -expectPassesRule( - new NoInvisibleFields(), - ' - fragment objectFieldSelection on Dog { - __typename - name - } - ' - ); - } - - public function testFailsValidationOnInvisibleField(): void - { - $this->expectFailsRule( - new NoInvisibleFields(), - ' - fragment objectFieldSelection on Dog { - __typename - secretName - } - ', - [ - [ - 'message' => 'Cannot query field "secretName" on type "Dog".', - ], - ] - ); - } -} From 0b08fcd29a44aef58c3e6728506fc76f8b54cedd Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 26 Sep 2023 10:05:44 +0200 Subject: [PATCH 06/11] Call visible fn only once --- src/Type/Definition/FieldDefinition.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/Type/Definition/FieldDefinition.php b/src/Type/Definition/FieldDefinition.php index 496502ee2..c36f76275 100644 --- a/src/Type/Definition/FieldDefinition.php +++ b/src/Type/Definition/FieldDefinition.php @@ -194,9 +194,11 @@ public function getType(): Type public function isVisible(): bool { - return is_callable($this->visible) - ? ($this->visible)() - : $this->visible; + if (is_bool($this->visible)) { + return $this->visible; + } + + return $this->visible = ($this->visible)(); } public function isDeprecated(): bool From 28ef48a2bd25c0fcacba71efd466321a14c2a268 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 26 Sep 2023 10:12:29 +0200 Subject: [PATCH 07/11] minimize contact points --- src/Type/Definition/HasFieldsType.php | 9 ++++++++ .../HasFieldsTypeImplementation.php | 18 +++++++++------ src/Type/Introspection.php | 22 +++++++++---------- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/src/Type/Definition/HasFieldsType.php b/src/Type/Definition/HasFieldsType.php index bbbfb3270..4bab0bac9 100644 --- a/src/Type/Definition/HasFieldsType.php +++ b/src/Type/Definition/HasFieldsType.php @@ -21,6 +21,15 @@ public function findField(string $name): ?FieldDefinition; public function getFields(): array; /** + * @throws InvariantViolation + * + * @return array + */ + public function getVisibleFields(): array; + + /** + * Get all field names, including only visible fields. + * * @throws InvariantViolation * * @return array diff --git a/src/Type/Definition/HasFieldsTypeImplementation.php b/src/Type/Definition/HasFieldsTypeImplementation.php index a8cef1f9b..023fae17a 100644 --- a/src/Type/Definition/HasFieldsTypeImplementation.php +++ b/src/Type/Definition/HasFieldsTypeImplementation.php @@ -78,20 +78,24 @@ public function getFields(): array return $this->fields; } + public function getVisibleFields(): array + { + return array_filter( + $this->getFields(), + fn (FieldDefinition $fieldDefinition): bool => $fieldDefinition->isVisible() + ); + } + /** @throws InvariantViolation */ public function getFieldNames(): array { $this->initializeFields(); - $visibleFields = array_filter( - $this->getFields(), - fn (FieldDefinition $fieldDefinition): bool => $fieldDefinition->isVisible() - ); - $fieldNames = array_map( + $visibleFieldNames = array_map( fn (FieldDefinition $fieldDefinition): string => $fieldDefinition->getName(), - $visibleFields + $this->getVisibleFields() ); - return array_values($fieldNames); + return array_values($visibleFieldNames); } } diff --git a/src/Type/Introspection.php b/src/Type/Introspection.php index d7a1494f3..7469f5787 100644 --- a/src/Type/Introspection.php +++ b/src/Type/Introspection.php @@ -352,20 +352,18 @@ public static function _type(): ObjectType 'defaultValue' => false, ], ], - 'resolve' => static function (Type $type, $args, $context): ?array { + 'resolve' => static function (Type $type, $args): ?array { if ($type instanceof ObjectType || $type instanceof InterfaceType) { - $fields = $type->getFields(); + $fields = $type->getVisibleFields(); - return \array_filter( - $fields, - static function (FieldDefinition $field) use ($args): bool { - return $field->isVisible() - && ! ( - $field->isDeprecated() - && ! ($args['includeDeprecated'] ?? false) - ); - } - ); + if (! ($args['includeDeprecated'] ?? false)) { + return \array_filter( + $fields, + static fn (FieldDefinition $field): bool => ! $field->isDeprecated() + ); + } + + return $fields; } return null; From f1f8be9ac64bd633029d41c571271f4f837e7104 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 26 Sep 2023 10:15:15 +0200 Subject: [PATCH 08/11] isDeprecated --- src/Type/Introspection.php | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/src/Type/Introspection.php b/src/Type/Introspection.php index 7469f5787..9aaa5341a 100644 --- a/src/Type/Introspection.php +++ b/src/Type/Introspection.php @@ -396,10 +396,7 @@ public static function _type(): ObjectType if (! ($args['includeDeprecated'] ?? false)) { return \array_filter( $values, - static function (EnumValueDefinition $value): bool { - return $value->deprecationReason === null - || $value->deprecationReason === ''; - } + static fn (EnumValueDefinition $value): bool => ! $value->isDeprecated() ); } @@ -424,8 +421,7 @@ static function (EnumValueDefinition $value): bool { if (! ($args['includeDeprecated'] ?? false)) { return \array_filter( $fields, - static fn (InputObjectField $field): bool => $field->deprecationReason === null - || $field->deprecationReason === '', + static fn (InputObjectField $field): bool => ! $field->isDeprecated(), ); } @@ -520,8 +516,7 @@ public static function _field(): ObjectType if (! ($args['includeDeprecated'] ?? false)) { return \array_filter( $values, - static fn (Argument $value): bool => $value->deprecationReason === null - || $value->deprecationReason === '', + static fn (Argument $value): bool => ! $value->isDeprecated(), ); } @@ -534,8 +529,7 @@ public static function _field(): ObjectType ], 'isDeprecated' => [ 'type' => Type::nonNull(Type::boolean()), - 'resolve' => static fn (FieldDefinition $field): bool => $field->deprecationReason !== null - && $field->deprecationReason !== '', + 'resolve' => static fn (FieldDefinition $field): bool => $field->isDeprecated(), ], 'deprecationReason' => [ 'type' => Type::string(), @@ -592,8 +586,7 @@ public static function _inputValue(): ObjectType 'isDeprecated' => [ 'type' => Type::nonNull(Type::boolean()), /** @param Argument|InputObjectField $inputValue */ - 'resolve' => static fn ($inputValue): bool => $inputValue->deprecationReason !== null - && $inputValue->deprecationReason !== '', + 'resolve' => static fn ($inputValue): bool => $inputValue->isDeprecated(), ], 'deprecationReason' => [ 'type' => Type::string(), @@ -624,8 +617,7 @@ public static function _enumValue(): ObjectType ], 'isDeprecated' => [ 'type' => Type::nonNull(Type::boolean()), - 'resolve' => static fn (EnumValueDefinition $enumValue): bool => $enumValue->deprecationReason !== null - && $enumValue->deprecationReason !== '', + 'resolve' => static fn (EnumValueDefinition $enumValue): bool => $enumValue->isDeprecated(), ], 'deprecationReason' => [ 'type' => Type::string(), From cda80c3e2369c2dc4903c8261e0f4e5a3aab3d7f Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Tue, 26 Sep 2023 10:16:31 +0200 Subject: [PATCH 09/11] fix PHP 7.4 --- tests/Type/IntrospectionTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Type/IntrospectionTest.php b/tests/Type/IntrospectionTest.php index 0683bbf22..c484db3a5 100644 --- a/tests/Type/IntrospectionTest.php +++ b/tests/Type/IntrospectionTest.php @@ -1733,7 +1733,7 @@ public function testExecutesAnIntrospectionQueryWithoutCallingGlobalFieldResolve * * @dataProvider invisibleFieldDataProvider */ - public function testDoesNotExposeInvisibleFields(mixed $visible): void + public function testDoesNotExposeInvisibleFields($visible): void { $TestType = new ObjectType([ 'name' => 'TestType', From d84d73c2a19ab3ab2dcc24fb35dc9a0c5f73ebd8 Mon Sep 17 00:00:00 2001 From: Carla Gouveia Date: Thu, 28 Sep 2023 10:52:46 -0400 Subject: [PATCH 10/11] Documented `visible` option on field configuration. --- docs/type-definitions/object-types.md | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/docs/type-definitions/object-types.md b/docs/type-definitions/object-types.md index e073d2418..a70909b83 100644 --- a/docs/type-definitions/object-types.md +++ b/docs/type-definitions/object-types.md @@ -58,14 +58,15 @@ This example uses **inline** style for Object Type definitions, but you can also ## Configuration options -| Option | Type | Notes | -| ------------ | --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| name | `string` | **Required.** Unique name of this object type within Schema | -| fields | `array` or `callable` | **Required**. An array describing object fields or callable returning such an array. See [field configuration options](#field-configuration-options) section below for expected structure of each array entry. See also the section on [Circular types](#recurring-and-circular-types) for an explanation of when to use callable for this option. | -| description | `string` | Plain-text description of this type for clients (e.g. used by [GraphiQL](https://github.com/graphql/graphiql) for auto-generated documentation) | -| interfaces | `array` or `callable` | List of interfaces implemented by this type or callable returning such a list. See [Interface Types](interfaces.md) for details. See also the section on [Circular types](#recurring-and-circular-types) for an explanation of when to use callable for this option. | -| isTypeOf | `callable` | **function ($value, $context, [ResolveInfo](../class-reference.md#graphqltypedefinitionresolveinfo) $info): bool**
Expected to return **true** if **$value** qualifies for this type (see section about [Abstract Type Resolution](interfaces.md#interface-role-in-data-fetching) for explanation). | -| resolveField | `callable` | **function ($value, array $args, $context, [ResolveInfo](../class-reference.md#graphqltypedefinitionresolveinfo) $info): mixed**
Given the **$value** of this type, it is expected to return value for a field defined in **$info->fieldName**. A good place to define a type-specific strategy for field resolution. See section on [Data Fetching](../data-fetching.md) for details. | +| Option | Type | Notes | +|--------------|------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| name | `string` | **Required.** Unique name of this object type within Schema | +| fields | `array` or `callable` | **Required**. An array describing object fields or callable returning such an array. See [field configuration options](#field-configuration-options) section below for expected structure of each array entry. See also the section on [Circular types](#recurring-and-circular-types) for an explanation of when to use callable for this option. | +| description | `string` | Plain-text description of this type for clients (e.g. used by [GraphiQL](https://github.com/graphql/graphiql) for auto-generated documentation) | +| interfaces | `array` or `callable` | List of interfaces implemented by this type or callable returning such a list. See [Interface Types](interfaces.md) for details. See also the section on [Circular types](#recurring-and-circular-types) for an explanation of when to use callable for this option. | +| isTypeOf | `callable` | **function ($value, $context, [ResolveInfo](../class-reference.md#graphqltypedefinitionresolveinfo) $info): bool**
Expected to return **true** if **$value** qualifies for this type (see section about [Abstract Type Resolution](interfaces.md#interface-role-in-data-fetching) for explanation). | +| resolveField | `callable` | **function ($value, array $args, $context, [ResolveInfo](../class-reference.md#graphqltypedefinitionresolveinfo) $info): mixed**
Given the **$value** of this type, it is expected to return value for a field defined in **$info->fieldName**. A good place to define a type-specific strategy for field resolution. See section on [Data Fetching](../data-fetching.md) for details. | +| visible | `bool` or `callable` | **function ($context): mixed**
You can customize the visibility of fields by defining them as visible or not visible using a `bool` or your own custom logic by sending a `callable` that receives the `$context`. In introspection, the field will not be included in the result and if a query references that field, it will return a validation error. | ### Field configuration options From 37eb433469df929664a5e94922c422e01857fbd4 Mon Sep 17 00:00:00 2001 From: Carla Gouveia Date: Fri, 29 Sep 2023 14:05:22 -0400 Subject: [PATCH 11/11] Update docs/type-definitions/object-types.md Co-authored-by: Benedikt Franke --- docs/type-definitions/object-types.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/type-definitions/object-types.md b/docs/type-definitions/object-types.md index a70909b83..a5a98bf34 100644 --- a/docs/type-definitions/object-types.md +++ b/docs/type-definitions/object-types.md @@ -66,7 +66,7 @@ This example uses **inline** style for Object Type definitions, but you can also | interfaces | `array` or `callable` | List of interfaces implemented by this type or callable returning such a list. See [Interface Types](interfaces.md) for details. See also the section on [Circular types](#recurring-and-circular-types) for an explanation of when to use callable for this option. | | isTypeOf | `callable` | **function ($value, $context, [ResolveInfo](../class-reference.md#graphqltypedefinitionresolveinfo) $info): bool**
Expected to return **true** if **$value** qualifies for this type (see section about [Abstract Type Resolution](interfaces.md#interface-role-in-data-fetching) for explanation). | | resolveField | `callable` | **function ($value, array $args, $context, [ResolveInfo](../class-reference.md#graphqltypedefinitionresolveinfo) $info): mixed**
Given the **$value** of this type, it is expected to return value for a field defined in **$info->fieldName**. A good place to define a type-specific strategy for field resolution. See section on [Data Fetching](../data-fetching.md) for details. | -| visible | `bool` or `callable` | **function ($context): mixed**
You can customize the visibility of fields by defining them as visible or not visible using a `bool` or your own custom logic by sending a `callable` that receives the `$context`. In introspection, the field will not be included in the result and if a query references that field, it will return a validation error. | +| visible | `bool` or `callable` | Defaults to `true`. The given callable receives no arguments and is expected to return a `bool`, it is called once when the field may be accessed. The field is treated as if it were not defined at all when this is `false`. | ### Field configuration options