From a7a0952cc7851f6d47d74434044ea786c99982e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Sat, 21 Dec 2024 13:55:55 +0100 Subject: [PATCH 01/33] Setup validation for `@bind` directive schema definitions --- src/Schema/Directives/BindDirective.php | 138 ++++++++++++++++ .../Directives/BindingDirectiveTest.php | 153 ++++++++++++++++++ 2 files changed, 291 insertions(+) create mode 100644 src/Schema/Directives/BindDirective.php create mode 100644 tests/Integration/Schema/Directives/BindingDirectiveTest.php diff --git a/src/Schema/Directives/BindDirective.php b/src/Schema/Directives/BindDirective.php new file mode 100644 index 000000000..c004f2147 --- /dev/null +++ b/src/Schema/Directives/BindDirective.php @@ -0,0 +1,138 @@ +validateClassArg([ + 'argument' => $argDefinition->name->value, + 'field' => $parentField->name->value, + ]); + } + + public function manipulateInputFieldDefinition( + DocumentAST &$documentAST, + InputValueDefinitionNode &$inputField, + InputObjectTypeDefinitionNode &$parentInput, + ): void { + $this->validateClassArg([ + 'field' => $inputField->name->value, + 'input' => $parentInput->name->value, + ]); + } + + /** + * @param array $exceptionMessageArgs + */ + private function validateClassArg(array $exceptionMessageArgs): void + { + $directive = $this->name(); + $class = $this->directiveArgValue('class'); + + if (! class_exists($class)) { + throw new DefinitionException(sprintf( + "@$directive argument `class` defined on %s `%s` of %s `%s` " . + "must be an existing class, received `$class`.", + ...$this->spreadKeysAndValues($exceptionMessageArgs), + )); + } + + if (is_subclass_of($class, Model::class)) { + return; + } + + if (is_callable($class)) { + return; + } + + throw new DefinitionException(sprintf( + "@$directive argument `class` defined on %s `%s` of %s `%s` must be " . + "an Eloquent model or a callable class, received `$class`.", + ...$this->spreadKeysAndValues($exceptionMessageArgs), + )); + } + + /** + * @param array $keyedValues + * @return array + */ + private function spreadKeysAndValues(array $keyedValues): array + { + return Collection::make($keyedValues)->reduce(fn (array $carry, string $value, string $key): array => [ + ...$carry, $key, $value, + ], []); + } + + public function transform(mixed $argumentValue): mixed + { + return $argumentValue; + } +} diff --git a/tests/Integration/Schema/Directives/BindingDirectiveTest.php b/tests/Integration/Schema/Directives/BindingDirectiveTest.php new file mode 100644 index 000000000..5d2fd7343 --- /dev/null +++ b/tests/Integration/Schema/Directives/BindingDirectiveTest.php @@ -0,0 +1,153 @@ +schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user(user: ID! @bind(class: "NotAClass")): User! @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query { + user(user: "1") { + id + } + } + GRAPHQL); + + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( + $this->assertExceptionMessageContains( + ['@bind', 'class', 'argument `user`', 'field `user`', 'NotAClass'], + $exception, + ) + )); + } + + public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAClass(): void + { + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input RemoveUsersInput { + users: [ID!]! @bind(class: "NotAClass") + } + + type Mutation { + removeUsers(input: RemoveUsersInput!): Boolean! @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($input: RemoveUsersInput!) { + removeUsers(input: $input) + } + GRAPHQL, + [ + 'input' => [ + 'users' => ['1'], + ], + ], + ); + + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( + $this->assertExceptionMessageContains( + ['@bind', 'class', 'field `users`', 'input `RemoveUsersInput`', 'NotAClass'], + $exception, + ) + )); + } + + public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgumentIsNotAModelOrCallableClass(): void + { + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user(user: ID! @bind(class: "stdClass")): User! @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query { + user(user: "1") { + id + } + } + GRAPHQL); + + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( + $this->assertExceptionMessageContains( + ['@bind', 'class', 'argument `user`', 'field `user`', 'stdClass'], + $exception, + ) + )); + } + + public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAModelOrCallableClass(): void + { + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input RemoveUsersInput { + users: [ID!]! @bind(class: "stdClass") + } + + type Mutation { + removeUsers(input: RemoveUsersInput!): Boolean! @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($input: RemoveUsersInput!) { + removeUsers(input: $input) + } + GRAPHQL, + [ + 'input' => [ + 'users' => ['1'], + ] + ], + ); + + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( + $this->assertExceptionMessageContains( + ['@bind', 'class', 'field `users`', 'input `RemoveUsersInput`', 'stdClass'], + $exception, + ) + )); + } + + private function assertExceptionMessageContains(array $fragments, Throwable $exception): bool + { + return Collection::make($fragments) + ->each(function (string $fragment) use ($exception): void { + $this->assertStringContainsString($fragment, $exception->getMessage()); + }) + ->isNotEmpty(); + } +} From b41f64beea869d093fff0d64cf945798bccb35f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Sun, 22 Dec 2024 20:07:53 +0100 Subject: [PATCH 02/33] Implement simple model binding --- src/Binding/BindingDefinition.php | 77 ++++++++ src/Binding/ModelBinding.php | 29 +++ src/Schema/Directives/BindDirective.php | 81 +++----- .../Directives/BindingDirectiveTest.php | 175 ++++++++++++++++++ .../Directives/Fixtures/SpyResolver.php | 35 ++++ 5 files changed, 340 insertions(+), 57 deletions(-) create mode 100644 src/Binding/BindingDefinition.php create mode 100644 src/Binding/ModelBinding.php create mode 100644 tests/Integration/Schema/Directives/Fixtures/SpyResolver.php diff --git a/src/Binding/BindingDefinition.php b/src/Binding/BindingDefinition.php new file mode 100644 index 000000000..f712a411a --- /dev/null +++ b/src/Binding/BindingDefinition.php @@ -0,0 +1,77 @@ + $class + * @property-read string $column + * @property-read array $with + * @property-read bool $optional + */ +class BindingDefinition +{ + /** + * @param class-string $class + * @param array $with + */ + public function __construct( + public string $class, + public string $column, + public array $with, + public bool $optional, + ) {} + + /** + * @param array $exceptionMessagePlaceholders + */ + public function validate(array $exceptionMessagePlaceholders): void + { + if (! class_exists($this->class)) { + throw new DefinitionException(sprintf( + "@bind argument `class` defined on %s of %s must be an existing class, received `$this->class`.", + ...$this->formatExceptionMessagePlaceholders($exceptionMessagePlaceholders), + )); + } + + if ($this->isModelBinding()) { + return; + } + + if (is_callable($this->class)) { + return; + } + + throw new DefinitionException(sprintf( + "@bind argument `class` defined on %s of %s must be an Eloquent" . + "model or a callable class, received `$this->class`.", + ...$this->formatExceptionMessagePlaceholders($exceptionMessagePlaceholders), + )); + } + + /** + * @param array $placeholders + * @return array + */ + private function formatExceptionMessagePlaceholders(array $placeholders): array + { + return Collection::make($placeholders) + ->map(fn (string $value, string $key): string => "$key `$value`") + ->values() + ->all(); + } + + public function isModelBinding(): bool + { + return is_subclass_of($this->class, Model::class); + } +} diff --git a/src/Binding/ModelBinding.php b/src/Binding/ModelBinding.php new file mode 100644 index 000000000..25c20dbd4 --- /dev/null +++ b/src/Binding/ModelBinding.php @@ -0,0 +1,29 @@ + $definition + */ + public function __invoke(mixed $value, BindingDefinition $definition): Model|Collection|null + { + $binding = $definition->class::query() + ->with($definition->with) + ->whereIn($definition->column, Arr::wrap($value)) + ->get(); + + if (is_iterable($value)) { + return $binding; + } + + return $binding->first(); + } +} diff --git a/src/Schema/Directives/BindDirective.php b/src/Schema/Directives/BindDirective.php index c004f2147..295e62731 100644 --- a/src/Schema/Directives/BindDirective.php +++ b/src/Schema/Directives/BindDirective.php @@ -7,28 +7,21 @@ use GraphQL\Language\AST\InputValueDefinitionNode; use GraphQL\Language\AST\InterfaceTypeDefinitionNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; -use Illuminate\Database\Eloquent\Model; -use Illuminate\Support\Collection; -use Nuwave\Lighthouse\Exceptions\DefinitionException; +use Illuminate\Contracts\Container\Container; +use Nuwave\Lighthouse\Binding\BindingDefinition; +use Nuwave\Lighthouse\Binding\ModelBinding; use Nuwave\Lighthouse\Schema\AST\DocumentAST; -use Nuwave\Lighthouse\Support\Contracts\ArgDirective; use Nuwave\Lighthouse\Support\Contracts\ArgDirectiveForArray; use Nuwave\Lighthouse\Support\Contracts\ArgManipulator; use Nuwave\Lighthouse\Support\Contracts\ArgTransformerDirective; use Nuwave\Lighthouse\Support\Contracts\InputFieldManipulator; -use function class_exists; -use function is_callable; -use function is_subclass_of; -use function sprintf; - -class BindDirective extends BaseDirective implements - ArgTransformerDirective, - ArgDirective, - ArgDirectiveForArray, - ArgManipulator, - InputFieldManipulator +class BindDirective extends BaseDirective implements ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator { + public function __construct( + private Container $container, + ) {} + public static function definition(): string { return /** @lang GraphQL */ <<<'GRAPHQL' @@ -72,7 +65,7 @@ public function manipulateArgDefinition( FieldDefinitionNode &$parentField, ObjectTypeDefinitionNode|InterfaceTypeDefinitionNode &$parentType, ): void { - $this->validateClassArg([ + $this->bindingDefinition()->validate([ 'argument' => $argDefinition->name->value, 'field' => $parentField->name->value, ]); @@ -83,56 +76,30 @@ public function manipulateInputFieldDefinition( InputValueDefinitionNode &$inputField, InputObjectTypeDefinitionNode &$parentInput, ): void { - $this->validateClassArg([ + $this->bindingDefinition()->validate([ 'field' => $inputField->name->value, 'input' => $parentInput->name->value, ]); } - /** - * @param array $exceptionMessageArgs - */ - private function validateClassArg(array $exceptionMessageArgs): void + public function transform(mixed $argumentValue): mixed { - $directive = $this->name(); - $class = $this->directiveArgValue('class'); - - if (! class_exists($class)) { - throw new DefinitionException(sprintf( - "@$directive argument `class` defined on %s `%s` of %s `%s` " . - "must be an existing class, received `$class`.", - ...$this->spreadKeysAndValues($exceptionMessageArgs), - )); - } - - if (is_subclass_of($class, Model::class)) { - return; - } + $definition = $this->bindingDefinition(); + $bind = match ($definition->isModelBinding()) { + true => new ModelBinding(), + false => $this->container->make($definition->class), + }; - if (is_callable($class)) { - return; - } - - throw new DefinitionException(sprintf( - "@$directive argument `class` defined on %s `%s` of %s `%s` must be " . - "an Eloquent model or a callable class, received `$class`.", - ...$this->spreadKeysAndValues($exceptionMessageArgs), - )); + return $bind($argumentValue, $definition); } - /** - * @param array $keyedValues - * @return array - */ - private function spreadKeysAndValues(array $keyedValues): array - { - return Collection::make($keyedValues)->reduce(fn (array $carry, string $value, string $key): array => [ - ...$carry, $key, $value, - ], []); - } - - public function transform(mixed $argumentValue): mixed + private function bindingDefinition(): BindingDefinition { - return $argumentValue; + return new BindingDefinition( + $this->directiveArgValue('class'), + $this->directiveArgValue('column', 'id'), + $this->directiveArgValue('with', []), + $this->directiveArgValue('optional', false), + ); } } diff --git a/tests/Integration/Schema/Directives/BindingDirectiveTest.php b/tests/Integration/Schema/Directives/BindingDirectiveTest.php index 5d2fd7343..cdbd6268a 100644 --- a/tests/Integration/Schema/Directives/BindingDirectiveTest.php +++ b/tests/Integration/Schema/Directives/BindingDirectiveTest.php @@ -7,8 +7,12 @@ use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; use Tests\DBTestCase; +use Tests\Integration\Schema\Directives\Fixtures\SpyResolver; +use Tests\Utils\Models\User; use Throwable; +use function factory; + final class BindingDirectiveTest extends DBTestCase { use UsesTestSchema; @@ -142,6 +146,177 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN )); } + public function testModelBindingOnFieldArgument(): void + { + $user = factory(User::class)->create(); + $this->mockResolver(fn (mixed $root, array $args): User => $args['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user(user: ID! @bind(class: "Tests\\Utils\\Models\\User")): User! @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, ['id' => $user->getKey()]); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->getKey(), + ], + ], + ]); + } + + public function testModelCollectionBindingOnFieldArgument(): void + { + $users = factory(User::class, 2)->create(); + $resolver = new SpyResolver(return: true); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Mutation { + removeUsers( + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") + ): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($users: [ID!]!) { + removeUsers(users: $users) + } + GRAPHQL, + [ + 'users' => $users->map(fn (User $user): int => $user->getKey()), + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'removeUsers' => true, + ], + ]); + $resolver->assertArgs(function (array $args) use ($users): void { + $this->assertArrayHasKey('users', $args); + $this->assertCount($users->count(), $args['users']); + $users->each(function (User $user, int $key) use ($args): void { + $this->assertTrue($user->is($args['users'][$key])); + }); + }); + } + + public function testModelBindingOnInputField(): void + { + $user = factory(User::class)->create(); + $this->mockResolver(fn (mixed $root, array $args): User => $args['input']['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Models\\User") + } + + type Query { + user(input: UserInput!): User! @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => $user->getKey(), + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->getKey(), + ], + ], + ]); + } + + public function testModelCollectionBindingOnInputField(): void + { + $users = factory(User::class, 2)->create(); + $resolver = new SpyResolver(return: true); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input RemoveUsersInput { + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") + } + + type Mutation { + removeUsers(input: RemoveUsersInput!): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($input: RemoveUsersInput!) { + removeUsers(input: $input) + } + GRAPHQL, + [ + 'input' => [ + 'users' => $users->map(fn (User $user): int => $user->getKey()), + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'removeUsers' => true, + ], + ]); + $resolver->assertArgs(function (array $args) use ($users): void { + $this->assertArrayHasKey('input', $args); + $this->assertArrayHasKey('users', $args['input']); + $this->assertCount($users->count(), $args['input']['users']); + $users->each(function (User $user, int $key) use ($args): void { + $this->assertTrue($user->is($args['input']['users'][$key])); + }); + }); + } + private function assertExceptionMessageContains(array $fragments, Throwable $exception): bool { return Collection::make($fragments) diff --git a/tests/Integration/Schema/Directives/Fixtures/SpyResolver.php b/tests/Integration/Schema/Directives/Fixtures/SpyResolver.php new file mode 100644 index 000000000..123c3ef4d --- /dev/null +++ b/tests/Integration/Schema/Directives/Fixtures/SpyResolver.php @@ -0,0 +1,35 @@ +args = $args; + + return $this->return; + } + + public function assertArgs(callable $expect): void + { + $expect($this->args); + } +} From 145a247e0fa6fdd15ba2e789ae346e6b5385df90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Sun, 22 Dec 2024 20:20:19 +0100 Subject: [PATCH 03/33] Restructure to separate Nuwave\Lighthouse\Bind namespace --- composer.json | 1 + .../BindDefinition.php} | 4 ++-- src/{Schema/Directives => Bind}/BindDirective.php | 9 ++++----- src/Bind/BindServiceProvider.php | 15 +++++++++++++++ src/{Binding => Bind}/ModelBinding.php | 6 +++--- .../BindDirectiveTest.php} | 6 +++--- tests/TestCase.php | 2 ++ .../Fixtures => Utils/Resolvers}/SpyResolver.php | 2 +- 8 files changed, 31 insertions(+), 14 deletions(-) rename src/{Binding/BindingDefinition.php => Bind/BindDefinition.php} (97%) rename src/{Schema/Directives => Bind}/BindDirective.php (93%) create mode 100644 src/Bind/BindServiceProvider.php rename src/{Binding => Bind}/ModelBinding.php (67%) rename tests/Integration/{Schema/Directives/BindingDirectiveTest.php => Bind/BindDirectiveTest.php} (98%) rename tests/{Integration/Schema/Directives/Fixtures => Utils/Resolvers}/SpyResolver.php (90%) diff --git a/composer.json b/composer.json index 414d9a858..87680e369 100644 --- a/composer.json +++ b/composer.json @@ -111,6 +111,7 @@ "Nuwave\\Lighthouse\\LighthouseServiceProvider", "Nuwave\\Lighthouse\\Async\\AsyncServiceProvider", "Nuwave\\Lighthouse\\Auth\\AuthServiceProvider", + "Nuwave\\Lighthouse\\Bind\\BindServiceProvider", "Nuwave\\Lighthouse\\Cache\\CacheServiceProvider", "Nuwave\\Lighthouse\\GlobalId\\GlobalIdServiceProvider", "Nuwave\\Lighthouse\\OrderBy\\OrderByServiceProvider", diff --git a/src/Binding/BindingDefinition.php b/src/Bind/BindDefinition.php similarity index 97% rename from src/Binding/BindingDefinition.php rename to src/Bind/BindDefinition.php index f712a411a..005d6cb93 100644 --- a/src/Binding/BindingDefinition.php +++ b/src/Bind/BindDefinition.php @@ -1,6 +1,6 @@ $with * @property-read bool $optional */ -class BindingDefinition +class BindDefinition { /** * @param class-string $class diff --git a/src/Schema/Directives/BindDirective.php b/src/Bind/BindDirective.php similarity index 93% rename from src/Schema/Directives/BindDirective.php rename to src/Bind/BindDirective.php index 295e62731..2b225cb45 100644 --- a/src/Schema/Directives/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -1,6 +1,6 @@ directiveArgValue('class'), $this->directiveArgValue('column', 'id'), $this->directiveArgValue('with', []), diff --git a/src/Bind/BindServiceProvider.php b/src/Bind/BindServiceProvider.php new file mode 100644 index 000000000..cf2c43b4c --- /dev/null +++ b/src/Bind/BindServiceProvider.php @@ -0,0 +1,15 @@ +listen(RegisterDirectiveNamespaces::class, static fn (): string => __NAMESPACE__); + } +} diff --git a/src/Binding/ModelBinding.php b/src/Bind/ModelBinding.php similarity index 67% rename from src/Binding/ModelBinding.php rename to src/Bind/ModelBinding.php index 25c20dbd4..1be3e7832 100644 --- a/src/Binding/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -1,6 +1,6 @@ $definition + * @param \Nuwave\Lighthouse\Bind\BindDefinition<\Illuminate\Database\Eloquent\Model> $definition */ - public function __invoke(mixed $value, BindingDefinition $definition): Model|Collection|null + public function __invoke(mixed $value, BindDefinition $definition): Model|Collection|null { $binding = $definition->class::query() ->with($definition->with) diff --git a/tests/Integration/Schema/Directives/BindingDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php similarity index 98% rename from tests/Integration/Schema/Directives/BindingDirectiveTest.php rename to tests/Integration/Bind/BindDirectiveTest.php index cdbd6268a..b1d0a5bae 100644 --- a/tests/Integration/Schema/Directives/BindingDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -1,19 +1,19 @@ Date: Sun, 22 Dec 2024 20:30:02 +0100 Subject: [PATCH 04/33] Test optional model binding --- tests/Integration/Bind/BindDirectiveTest.php | 159 +++++++++++++++++++ tests/Utils/Resolvers/SpyResolver.php | 4 +- 2 files changed, 160 insertions(+), 3 deletions(-) diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index b1d0a5bae..0916f6ea9 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -178,6 +178,37 @@ public function testModelBindingOnFieldArgument(): void ]); } + public function testOptionalModelBindingOnFieldArgument(): void + { + $this->mockResolver(fn (mixed $root, array $args) => $args['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: ID! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + ): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, ['id' => '1']); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => null, + ], + ]); + } + public function testModelCollectionBindingOnFieldArgument(): void { $users = factory(User::class, 2)->create(); @@ -224,6 +255,48 @@ public function testModelCollectionBindingOnFieldArgument(): void }); } + public function testOptionalModelCollectionBindingOnFieldArgument(): void + { + $resolver = new SpyResolver(return: true); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Mutation { + removeUsers( + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + ): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($users: [ID!]!) { + removeUsers(users: $users) + } + GRAPHQL, + [ + 'users' => ['1', '2'], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'removeUsers' => true, + ], + ]); + $resolver->assertArgs(function (array $args): void { + $this->assertArrayHasKey('users', $args); + $this->assertEmpty($args['users']); + }); + } + public function testModelBindingOnInputField(): void { $user = factory(User::class)->create(); @@ -266,6 +339,45 @@ public function testModelBindingOnInputField(): void ]); } + public function testOptionalModelBindingOnInputField(): void + { + $this->mockResolver(fn (mixed $root, array $args) => $args['input']['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + } + + type Query { + user(input: UserInput!): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => '1', + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => null, + ], + ]); + } + public function testModelCollectionBindingOnInputField(): void { $users = factory(User::class, 2)->create(); @@ -317,6 +429,53 @@ public function testModelCollectionBindingOnInputField(): void }); } + public function testOptionalModelCollectionBindingOnInputField(): void + { + $resolver = new SpyResolver(return: true); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input RemoveUsersInput { + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + } + + type Mutation { + removeUsers(input: RemoveUsersInput!): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($input: RemoveUsersInput!) { + removeUsers(input: $input) + } + GRAPHQL, + [ + 'input' => [ + 'users' => ['1', '2'], + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'removeUsers' => true, + ], + ]); + $resolver->assertArgs(function (array $args): void { + $this->assertArrayHasKey('input', $args); + $this->assertArrayHasKey('users', $args['input']); + $this->assertEmpty($args['input']['users']); + }); + } + private function assertExceptionMessageContains(array $fragments, Throwable $exception): bool { return Collection::make($fragments) diff --git a/tests/Utils/Resolvers/SpyResolver.php b/tests/Utils/Resolvers/SpyResolver.php index dc9402f8a..6728d504d 100644 --- a/tests/Utils/Resolvers/SpyResolver.php +++ b/tests/Utils/Resolvers/SpyResolver.php @@ -1,6 +1,4 @@ - Date: Mon, 23 Dec 2024 21:58:11 +0100 Subject: [PATCH 05/33] Handle optional model bindings --- src/Bind/BindDefinition.php | 3 +- src/Bind/BindDirective.php | 5 +- src/Bind/BindException.php | 52 ++++ src/Bind/ModelBinding.php | 70 +++++- tests/Integration/Bind/BindDirectiveTest.php | 242 +++++++++++++++---- 5 files changed, 314 insertions(+), 58 deletions(-) create mode 100644 src/Bind/BindException.php diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index 005d6cb93..4b5e396be 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -25,6 +25,7 @@ class BindDefinition * @param array $with */ public function __construct( + public string $nodeName, public string $class, public string $column, public array $with, @@ -52,7 +53,7 @@ public function validate(array $exceptionMessagePlaceholders): void } throw new DefinitionException(sprintf( - "@bind argument `class` defined on %s of %s must be an Eloquent" . + "@bind argument `class` defined on %s of %s must be an Eloquent " . "model or a callable class, received `$this->class`.", ...$this->formatExceptionMessagePlaceholders($exceptionMessagePlaceholders), )); diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index 2b225cb45..524c62d36 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -8,6 +8,8 @@ use GraphQL\Language\AST\InterfaceTypeDefinitionNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; use Illuminate\Contracts\Container\Container; +use Illuminate\Support\Collection; +use Illuminate\Support\ItemNotFoundException; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; use Nuwave\Lighthouse\Support\Contracts\ArgDirectiveForArray; @@ -38,7 +40,7 @@ public static function definition(): string class: String! """ - Specify the column name to use when binding Eloquent models. + Specify the column name of a unique identifier to use when binding Eloquent models. By default, "id" is used the the primary key column. """ column: String! = "id" @@ -95,6 +97,7 @@ public function transform(mixed $argumentValue): mixed private function bindingDefinition(): BindDefinition { return new BindDefinition( + $this->nodeName(), $this->directiveArgValue('class'), $this->directiveArgValue('column', 'id'), $this->directiveArgValue('with', []), diff --git a/src/Bind/BindException.php b/src/Bind/BindException.php new file mode 100644 index 000000000..e32e80af0 --- /dev/null +++ b/src/Bind/BindException.php @@ -0,0 +1,52 @@ +nodeName with $definition->column `$value`.", + ); + } + + public static function notFound(mixed $value, BindDefinition $definition): self + { + return new self( + "No record found for binding $definition->nodeName with $definition->column `$value`.", + ); + } + + public static function tooManyRecordsFound(mixed $value, BindDefinition $definition): self + { + $column = Str::plural($definition->column); + + return new self( + "Unexpectedly found more records for binding $definition->nodeName with $column `$value`.", + ); + } + + public static function missingRecords(array $value, BindDefinition $definition): self + { + $column = Str::plural($definition->column); + $ids = implode(',', $value); + + return new self( + "No records found for binding $definition->nodeName with $column $ids.", + ); + } + + public function isClientSafe(): bool + { + return true; + } +} diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index 1be3e7832..a28b11ab6 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -2,28 +2,86 @@ namespace Nuwave\Lighthouse\Bind; -use Illuminate\Database\Eloquent\Collection; +use Illuminate\Database\Eloquent\Collection as EloquentCollection; use Illuminate\Database\Eloquent\Model; use Illuminate\Support\Arr; +use Illuminate\Support\Collection as IlluminateCollection; -use function is_iterable; +use function is_array; class ModelBinding { /** * @param \Nuwave\Lighthouse\Bind\BindDefinition<\Illuminate\Database\Eloquent\Model> $definition */ - public function __invoke(mixed $value, BindDefinition $definition): Model|Collection|null + public function __invoke(mixed $value, BindDefinition $definition): Model|EloquentCollection|null { $binding = $definition->class::query() ->with($definition->with) ->whereIn($definition->column, Arr::wrap($value)) ->get(); - if (is_iterable($value)) { - return $binding; + if (is_array($value)) { + return $this->modelCollection($binding, IlluminateCollection::make($value), $definition); } - return $binding->first(); + return $this->modelInstance($binding, $value, $definition); + } + + /** + * @param \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> $results + */ + private function modelInstance(EloquentCollection $results, mixed $value, BindDefinition $definition): ?Model + { + if ($results->count() > 1) { + throw BindException::multipleRecordsFound($value, $definition); + } + + $model = $results->first(); + + if ($definition->optional) { + return $model; + } + + if ($model === null) { + throw BindException::notFound($value, $definition); + } + + return $model; + } + + /** + * @param \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> $results + * @return \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> + */ + private function modelCollection( + EloquentCollection $results, + IlluminateCollection $values, + BindDefinition $definition, + ): EloquentCollection { + if ($results->count() > $values->unique()->count()) { + throw BindException::tooManyRecordsFound($values, $definition); + } + + if ($definition->optional) { + return $results->values(); + } + + $results = $results->keyBy($definition->column); + $missingResults = new IlluminateCollection(); + + foreach ($values as $value) { + if ($results->has($value)) { + continue; + } + + $missingResults->push($value); + } + + if ($missingResults->isNotEmpty()) { + throw BindException::missingRecords($missingResults->all(), $definition); + } + + return $results->values(); } } diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 0916f6ea9..70bf0f241 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -2,14 +2,15 @@ namespace Tests\Integration\Bind; -use Illuminate\Support\Collection; +use GraphQL\Error\Error; +use Nuwave\Lighthouse\Bind\BindDefinition; +use Nuwave\Lighthouse\Bind\BindException; use Nuwave\Lighthouse\Exceptions\DefinitionException; use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; use Tests\DBTestCase; use Tests\Utils\Models\User; use Tests\Utils\Resolvers\SpyResolver; -use Throwable; use function factory; @@ -38,12 +39,11 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument } GRAPHQL); - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( - $this->assertExceptionMessageContains( - ['@bind', 'class', 'argument `user`', 'field `user`', 'NotAClass'], - $exception, - ) - )); + $this->assertThrows( + $makeRequest, + DefinitionException::class, + 'argument `user` of field `user` must be an existing class', + ); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAClass(): void @@ -74,12 +74,11 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN ], ); - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( - $this->assertExceptionMessageContains( - ['@bind', 'class', 'field `users`', 'input `RemoveUsersInput`', 'NotAClass'], - $exception, - ) - )); + $this->assertThrows( + $makeRequest, + DefinitionException::class, + 'field `users` of input `RemoveUsersInput` must be an existing class', + ); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgumentIsNotAModelOrCallableClass(): void @@ -102,12 +101,11 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument } GRAPHQL); - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( - $this->assertExceptionMessageContains( - ['@bind', 'class', 'argument `user`', 'field `user`', 'stdClass'], - $exception, - ) - )); + $this->assertThrows( + $makeRequest, + DefinitionException::class, + 'argument `user` of field `user` must be an Eloquent model or a callable class', + ); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAModelOrCallableClass(): void @@ -138,12 +136,11 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN ], ); - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => ( - $this->assertExceptionMessageContains( - ['@bind', 'class', 'field `users`', 'input `RemoveUsersInput`', 'stdClass'], - $exception, - ) - )); + $this->assertThrows( + $makeRequest, + DefinitionException::class, + 'field `users` of input `RemoveUsersInput` must be an Eloquent model or a callable class', + ); } public function testModelBindingOnFieldArgument(): void @@ -166,7 +163,9 @@ public function testModelBindingOnFieldArgument(): void id } } - GRAPHQL, ['id' => $user->getKey()]); + GRAPHQL, + ['id' => $user->getKey()], + ); $response->assertGraphQLErrorFree(); $response->assertJson([ @@ -178,7 +177,35 @@ public function testModelBindingOnFieldArgument(): void ]); } - public function testOptionalModelBindingOnFieldArgument(): void + public function testMissingModelBindingOnFieldArgument(): void + { + $this->rethrowGraphQLErrors(); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user(user: ID! @bind(class: "Tests\\Utils\\Models\\User")): User! @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query { + user(user: "1") { + id + } + } + GRAPHQL); + + $this->assertThrows( + $makeRequest, + Error::class, + BindException::notFound('1', new BindDefinition('user', User::class, 'id', [], false))->getMessage(), + ); + } + + public function testMissingOptionalModelBindingOnFieldArgument(): void { $this->mockResolver(fn (mixed $root, array $args) => $args['user']); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' @@ -199,7 +226,9 @@ public function testOptionalModelBindingOnFieldArgument(): void id } } - GRAPHQL, ['id' => '1']); + GRAPHQL, + ['id' => '1'], + ); $response->assertGraphQLErrorFree(); $response->assertJson([ @@ -235,9 +264,7 @@ public function testModelCollectionBindingOnFieldArgument(): void removeUsers(users: $users) } GRAPHQL, - [ - 'users' => $users->map(fn (User $user): int => $user->getKey()), - ], + ['users' => $users->map(fn (User $user): int => $user->getKey())], ); $response->assertGraphQLErrorFree(); @@ -255,8 +282,48 @@ public function testModelCollectionBindingOnFieldArgument(): void }); } - public function testOptionalModelCollectionBindingOnFieldArgument(): void + public function testMissingModelCollectionBindingOnFieldArgument(): void { + $this->rethrowGraphQLErrors(); + $user = factory(User::class)->create(); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Mutation { + removeUsers( + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") + ): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($users: [ID!]!) { + removeUsers(users: $users) + } + GRAPHQL, + [ + 'users' => [$user->getKey(), '10'], + ], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + BindException::missingRecords(['10'], new BindDefinition('users', User::class, 'id', [], false)) + ->getMessage(), + ); + } + + public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void + { + $this->rethrowGraphQLErrors(); + $user = factory(User::class)->create(); $resolver = new SpyResolver(return: true); $this->mockResolver($resolver); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' @@ -281,7 +348,7 @@ public function testOptionalModelCollectionBindingOnFieldArgument(): void } GRAPHQL, [ - 'users' => ['1', '2'], + 'users' => [$user->getKey(), '10'], ], ); @@ -291,9 +358,10 @@ public function testOptionalModelCollectionBindingOnFieldArgument(): void 'removeUsers' => true, ], ]); - $resolver->assertArgs(function (array $args): void { + $resolver->assertArgs(function (array $args) use ($user): void { $this->assertArrayHasKey('users', $args); - $this->assertEmpty($args['users']); + $this->assertCount(1, $args['users']); + $this->assertTrue($user->is($args['users'][0])); }); } @@ -339,7 +407,45 @@ public function testModelBindingOnInputField(): void ]); } - public function testOptionalModelBindingOnInputField(): void + public function testMissingModelBindingOnInputField(): void + { + $this->rethrowGraphQLErrors(); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Models\\User") + } + + type Query { + user(input: UserInput!): User! @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => '1', + ], + ], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + BindException::notFound('1', new BindDefinition('user', User::class, 'id', [], false))->getMessage(), + ); + } + + public function testMissingOptionalModelBindingOnInputField(): void { $this->mockResolver(fn (mixed $root, array $args) => $args['input']['user']); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' @@ -429,8 +535,52 @@ public function testModelCollectionBindingOnInputField(): void }); } - public function testOptionalModelCollectionBindingOnInputField(): void + public function testMissingModelCollectionBindingOnInputField(): void + { + $this->rethrowGraphQLErrors(); + $user = factory(User::class)->create(); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input RemoveUsersInput { + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") + } + + type Mutation { + removeUsers(input: RemoveUsersInput!): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($input: RemoveUsersInput!) { + removeUsers(input: $input) + } + GRAPHQL, + [ + 'input' => [ + 'users' => [$user->getKey(), '10'], + ], + ], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + BindException::missingRecords(['10'], new BindDefinition('users', User::class, 'id', [], false)) + ->getMessage(), + ); + } + + public function testMissingOptionalModelCollectionBindingOnInputField(): void { + $this->rethrowGraphQLErrors(); + $user = factory(User::class)->create(); $resolver = new SpyResolver(return: true); $this->mockResolver($resolver); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' @@ -458,7 +608,7 @@ public function testOptionalModelCollectionBindingOnInputField(): void GRAPHQL, [ 'input' => [ - 'users' => ['1', '2'], + 'users' => [$user->getKey(), '10'], ], ], ); @@ -469,19 +619,11 @@ public function testOptionalModelCollectionBindingOnInputField(): void 'removeUsers' => true, ], ]); - $resolver->assertArgs(function (array $args): void { + $resolver->assertArgs(function (array $args) use ($user): void { $this->assertArrayHasKey('input', $args); $this->assertArrayHasKey('users', $args['input']); - $this->assertEmpty($args['input']['users']); + $this->assertCount(1, $args['input']['users']); + $this->assertTrue($user->is($args['input']['users'][0])); }); } - - private function assertExceptionMessageContains(array $fragments, Throwable $exception): bool - { - return Collection::make($fragments) - ->each(function (string $fragment) use ($exception): void { - $this->assertStringContainsString($fragment, $exception->getMessage()); - }) - ->isNotEmpty(); - } } From 518df4ca67907d4d9f6917088e0c0bf349e284b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Tue, 24 Dec 2024 07:47:34 +0100 Subject: [PATCH 06/33] Add missing tests for binding models by column --- tests/Integration/Bind/BindDirectiveTest.php | 102 ++++++++++++++++--- 1 file changed, 90 insertions(+), 12 deletions(-) diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 70bf0f241..e9edc1bb0 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -151,7 +151,7 @@ public function testModelBindingOnFieldArgument(): void type User { id: ID! } - + type Query { user(user: ID! @bind(class: "Tests\\Utils\\Models\\User")): User! @mock } @@ -212,7 +212,7 @@ public function testMissingOptionalModelBindingOnFieldArgument(): void type User { id: ID! } - + type Query { user( user: ID! @bind(class: "Tests\\Utils\\Models\\User", optional: true) @@ -238,6 +238,42 @@ public function testMissingOptionalModelBindingOnFieldArgument(): void ]); } + public function testModelBindingByColumnOnFieldArgument(): void + { + $user = factory(User::class)->create(); + $this->mockResolver(fn (mixed $root, array $args) => $args['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: String! @bind(class: "Tests\\Utils\\Models\\User", column: "email") + ): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($email: String!) { + user(user: $email) { + id + } + } + GRAPHQL, + ['email' => $user->email], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->getKey(), + ], + ], + ]); + } + public function testModelCollectionBindingOnFieldArgument(): void { $users = factory(User::class, 2)->create(); @@ -253,7 +289,7 @@ public function testModelCollectionBindingOnFieldArgument(): void users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") ): Boolean! @mock } - + type Query { ping: Boolean } @@ -296,7 +332,7 @@ public function testMissingModelCollectionBindingOnFieldArgument(): void users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") ): Boolean! @mock } - + type Query { ping: Boolean } @@ -336,7 +372,7 @@ public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", optional: true) ): Boolean! @mock } - + type Query { ping: Boolean } @@ -373,11 +409,11 @@ public function testModelBindingOnInputField(): void type User { id: ID! } - + input UserInput { user: ID! @bind(class: "Tests\\Utils\\Models\\User") } - + type Query { user(input: UserInput!): User! @mock } @@ -452,11 +488,11 @@ public function testMissingOptionalModelBindingOnInputField(): void type User { id: ID! } - + input UserInput { user: ID! @bind(class: "Tests\\Utils\\Models\\User", optional: true) } - + type Query { user(input: UserInput!): User @mock } @@ -484,6 +520,48 @@ public function testMissingOptionalModelBindingOnInputField(): void ]); } + public function testModelBindingByColumnOnInputField(): void + { + $user = factory(User::class)->create(); + $this->mockResolver(fn (mixed $root, array $args) => $args['input']['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: String! @bind(class: "Tests\\Utils\\Models\\User", column: "email") + } + + type Query { + user(input: UserInput!): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => $user->email, + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->id, + ], + ], + ]); + } + public function testModelCollectionBindingOnInputField(): void { $users = factory(User::class, 2)->create(); @@ -501,7 +579,7 @@ public function testModelCollectionBindingOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - + type Query { ping: Boolean } @@ -551,7 +629,7 @@ public function testMissingModelCollectionBindingOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - + type Query { ping: Boolean } @@ -595,7 +673,7 @@ public function testMissingOptionalModelCollectionBindingOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - + type Query { ping: Boolean } From ce51095aaa7c7135f2dd26bf57c2f14179fb5648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Tue, 24 Dec 2024 07:59:00 +0100 Subject: [PATCH 07/33] Add missing tests for binding models with eager loading --- src/Bind/BindDirective.php | 2 - tests/Integration/Bind/BindDirectiveTest.php | 88 ++++++++++++++++++++ tests/Utils/Resolvers/SpyResolver.php | 6 ++ 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index 524c62d36..f87a33f83 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -8,8 +8,6 @@ use GraphQL\Language\AST\InterfaceTypeDefinitionNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; use Illuminate\Contracts\Container\Container; -use Illuminate\Support\Collection; -use Illuminate\Support\ItemNotFoundException; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; use Nuwave\Lighthouse\Support\Contracts\ArgDirectiveForArray; diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index e9edc1bb0..ba03e192c 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -274,6 +274,47 @@ public function testModelBindingByColumnOnFieldArgument(): void ]); } + public function testModelBindingWithEagerLoadingOnFieldArgument(): void + { + $user = factory(User::class)->create(); + $resolver = new SpyResolver(fn (mixed $root, array $args) => $args['user']); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: ID! @bind(class: "Tests\\Utils\\Models\\User", with: ["company"]) + ): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, + ['id' => $user->getKey()], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->getKey(), + ], + ], + ]); + $resolver->assertArgs(function (array $args): void { + $this->assertInstanceOf(User::class, $args['user']); + $this->assertTrue($args['user']->relationLoaded('company')); + }); + } + public function testModelCollectionBindingOnFieldArgument(): void { $users = factory(User::class, 2)->create(); @@ -562,6 +603,53 @@ public function testModelBindingByColumnOnInputField(): void ]); } + public function testModelBindingWithEagerLoadingOnInputField(): void + { + $user = factory(User::class)->create(); + $resolver = new SpyResolver(fn (mixed $root, array $args) => $args['input']['user']); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Models\\User", with: ["company"]) + } + + type Query { + user(input: UserInput!): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => $user->getKey(), + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->id, + ], + ], + ]); + $resolver->assertArgs(function (array $args): void { + $this->assertInstanceOf(User::class, $args['input']['user']); + $this->assertTrue($args['input']['user']->relationLoaded('company')); + }); + } + public function testModelCollectionBindingOnInputField(): void { $users = factory(User::class, 2)->create(); diff --git a/tests/Utils/Resolvers/SpyResolver.php b/tests/Utils/Resolvers/SpyResolver.php index 6728d504d..d80d93854 100644 --- a/tests/Utils/Resolvers/SpyResolver.php +++ b/tests/Utils/Resolvers/SpyResolver.php @@ -2,6 +2,8 @@ namespace Tests\Utils\Resolvers; +use function is_callable; + /** * @template TReturn */ @@ -23,6 +25,10 @@ public function __invoke(mixed $root, array $args): mixed { $this->args = $args; + if (is_callable($this->return)) { + return ($this->return)($root, $args); + } + return $this->return; } From 8f981f756a7eb41fd91ac45e569d89f9ef8e1c2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Tue, 24 Dec 2024 08:50:37 +0100 Subject: [PATCH 08/33] Add missing tests for too many results when resolving a binding I removed the custom exception here as the is to catch BindExceptions and convert them to ValidationExceptions later on. These errors, however, are more of a misconfiguration error and should not be considered validation errors. --- src/Bind/BindException.php | 16 -- src/Bind/ModelBinding.php | 5 +- tests/Integration/Bind/BindDirectiveTest.php | 153 ++++++++++++++++++- 3 files changed, 154 insertions(+), 20 deletions(-) diff --git a/src/Bind/BindException.php b/src/Bind/BindException.php index e32e80af0..a73ec22ba 100644 --- a/src/Bind/BindException.php +++ b/src/Bind/BindException.php @@ -12,13 +12,6 @@ class BindException extends Exception implements ClientAware { - public static function multipleRecordsFound(mixed $value, BindDefinition $definition): self - { - return new self( - "Unexpectedly found multiple records for binding $definition->nodeName with $definition->column `$value`.", - ); - } - public static function notFound(mixed $value, BindDefinition $definition): self { return new self( @@ -26,15 +19,6 @@ public static function notFound(mixed $value, BindDefinition $definition): self ); } - public static function tooManyRecordsFound(mixed $value, BindDefinition $definition): self - { - $column = Str::plural($definition->column); - - return new self( - "Unexpectedly found more records for binding $definition->nodeName with $column `$value`.", - ); - } - public static function missingRecords(array $value, BindDefinition $definition): self { $column = Str::plural($definition->column); diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index a28b11ab6..1074e02af 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -4,6 +4,7 @@ use Illuminate\Database\Eloquent\Collection as EloquentCollection; use Illuminate\Database\Eloquent\Model; +use Illuminate\Database\MultipleRecordsFoundException; use Illuminate\Support\Arr; use Illuminate\Support\Collection as IlluminateCollection; @@ -34,7 +35,7 @@ public function __invoke(mixed $value, BindDefinition $definition): Model|Eloque private function modelInstance(EloquentCollection $results, mixed $value, BindDefinition $definition): ?Model { if ($results->count() > 1) { - throw BindException::multipleRecordsFound($value, $definition); + throw new MultipleRecordsFoundException($results->count()); } $model = $results->first(); @@ -60,7 +61,7 @@ private function modelCollection( BindDefinition $definition, ): EloquentCollection { if ($results->count() > $values->unique()->count()) { - throw BindException::tooManyRecordsFound($values, $definition); + throw new MultipleRecordsFoundException($results->count()); } if ($definition->optional) { diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index ba03e192c..3637edb86 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -3,6 +3,7 @@ namespace Tests\Integration\Bind; use GraphQL\Error\Error; +use Illuminate\Database\MultipleRecordsFoundException; use Nuwave\Lighthouse\Bind\BindDefinition; use Nuwave\Lighthouse\Bind\BindException; use Nuwave\Lighthouse\Exceptions\DefinitionException; @@ -315,6 +316,39 @@ public function testModelBindingWithEagerLoadingOnFieldArgument(): void }); } + public function testModelBindingWithTooManyResultsOnFieldArgument(): void + { + $this->rethrowGraphQLErrors(); + $users = factory(User::class, 2)->create(['name' => 'John Doe']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: String! @bind(class: "Tests\\Utils\\Models\\User", column: "name") + ): User @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($name: String!) { + user(user: $name) { + id + } + } + GRAPHQL, + ['name' => $users->first()->name], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + (new MultipleRecordsFoundException($users->count()))->getMessage(), + ); + } + public function testModelCollectionBindingOnFieldArgument(): void { $users = factory(User::class, 2)->create(); @@ -399,7 +433,6 @@ public function testMissingModelCollectionBindingOnFieldArgument(): void public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void { - $this->rethrowGraphQLErrors(); $user = factory(User::class)->create(); $resolver = new SpyResolver(return: true); $this->mockResolver($resolver); @@ -442,6 +475,43 @@ public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void }); } + public function testModelCollectionBindingWithTooManyResultsOnFieldArgument(): void + { + $this->rethrowGraphQLErrors(); + $users = factory(User::class, 2)->create(['name' => 'John Doe']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Mutation { + removeUsers( + users: [String!]! @bind(class: "Tests\\Utils\\Models\\User", column: "name") + ): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($users: [String!]!) { + removeUsers(users: $users) + } + GRAPHQL, + [ + 'users' => [$users->first()->name], + ], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + (new MultipleRecordsFoundException($users->count()))->getMessage(), + ); + } + public function testModelBindingOnInputField(): void { $user = factory(User::class)->create(); @@ -650,6 +720,45 @@ public function testModelBindingWithEagerLoadingOnInputField(): void }); } + public function testModelBindingWithTooManyResultsOnInputField(): void + { + $this->rethrowGraphQLErrors(); + $users = factory(User::class, 2)->create(['name' => 'Jane Doe']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: String! @bind(class: "Tests\\Utils\\Models\\User", column: "name") + } + + type Query { + user(input: UserInput!): User @mock + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => $users->first()->name, + ], + ], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + (new MultipleRecordsFoundException($users->count()))->getMessage(), + ); + } + public function testModelCollectionBindingOnInputField(): void { $users = factory(User::class, 2)->create(); @@ -745,7 +854,6 @@ public function testMissingModelCollectionBindingOnInputField(): void public function testMissingOptionalModelCollectionBindingOnInputField(): void { - $this->rethrowGraphQLErrors(); $user = factory(User::class)->create(); $resolver = new SpyResolver(return: true); $this->mockResolver($resolver); @@ -792,4 +900,45 @@ public function testMissingOptionalModelCollectionBindingOnInputField(): void $this->assertTrue($user->is($args['input']['users'][0])); }); } + + public function testModelCollectionBindingWithTooManyResultsOnInputField(): void + { + $this->rethrowGraphQLErrors(); + $users = factory(User::class, 2)->create(['name' => 'Jane Doe']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input RemoveUsersInput { + users: [String!]! @bind(class: "Tests\\Utils\\Models\\User", column: "name") + } + + type Mutation { + removeUsers(input: RemoveUsersInput!): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($input: RemoveUsersInput!) { + removeUsers(input: $input) + } + GRAPHQL, + [ + 'input' => [ + 'users' => [$users->first()->name], + ], + ], + ); + + $this->assertThrows( + $makeRequest, + Error::class, + (new MultipleRecordsFoundException($users->count()))->getMessage(), + ); + } } From 2f46f5ee55195061639304fe941868384535288c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Wed, 25 Dec 2024 07:48:27 +0100 Subject: [PATCH 09/33] Handle missing required bindings a validation errors --- src/Bind/BindDirective.php | 54 ++++++++++++----- src/Bind/BindException.php | 36 ------------ src/Bind/ModelBinding.php | 40 +++---------- src/Bind/Validation/BindingExists.php | 55 ++++++++++++++++++ tests/Integration/Bind/BindDirectiveTest.php | 61 ++++++++------------ 5 files changed, 123 insertions(+), 123 deletions(-) delete mode 100644 src/Bind/BindException.php create mode 100644 src/Bind/Validation/BindingExists.php diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index f87a33f83..533cfab5d 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -8,14 +8,16 @@ use GraphQL\Language\AST\InterfaceTypeDefinitionNode; use GraphQL\Language\AST\ObjectTypeDefinitionNode; use Illuminate\Contracts\Container\Container; +use Nuwave\Lighthouse\Bind\Validation\BindingExists; use Nuwave\Lighthouse\Schema\AST\DocumentAST; use Nuwave\Lighthouse\Schema\Directives\BaseDirective; use Nuwave\Lighthouse\Support\Contracts\ArgDirectiveForArray; use Nuwave\Lighthouse\Support\Contracts\ArgManipulator; use Nuwave\Lighthouse\Support\Contracts\ArgTransformerDirective; +use Nuwave\Lighthouse\Support\Contracts\ArgumentValidation; use Nuwave\Lighthouse\Support\Contracts\InputFieldManipulator; -class BindDirective extends BaseDirective implements ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator +class BindDirective extends BaseDirective implements ArgumentValidation, ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator { public function __construct( private Container $container, @@ -58,13 +60,24 @@ class: String! GRAPHQL; } + private function bindDefinition(): BindDefinition + { + return new BindDefinition( + $this->nodeName(), + $this->directiveArgValue('class'), + $this->directiveArgValue('column', 'id'), + $this->directiveArgValue('with', []), + $this->directiveArgValue('optional', false), + ); + } + public function manipulateArgDefinition( DocumentAST &$documentAST, InputValueDefinitionNode &$argDefinition, FieldDefinitionNode &$parentField, ObjectTypeDefinitionNode|InterfaceTypeDefinitionNode &$parentType, ): void { - $this->bindingDefinition()->validate([ + $this->bindDefinition()->validate([ 'argument' => $argDefinition->name->value, 'field' => $parentField->name->value, ]); @@ -75,15 +88,35 @@ public function manipulateInputFieldDefinition( InputValueDefinitionNode &$inputField, InputObjectTypeDefinitionNode &$parentInput, ): void { - $this->bindingDefinition()->validate([ + $this->bindDefinition()->validate([ 'field' => $inputField->name->value, 'input' => $parentInput->name->value, ]); } - public function transform(mixed $argumentValue): mixed + public function rules(): array + { + $definition = $this->bindDefinition(); + + return match ($definition->optional) { + true => [], + false => [new BindingExists($this, $definition)], + }; + } + + public function messages(): array { - $definition = $this->bindingDefinition(); + return []; + } + + public function attribute(): ?string + { + return null; + } + + public function transform(mixed $argumentValue, ?BindDefinition $definition = null): mixed + { + $definition ??= $this->bindDefinition(); $bind = match ($definition->isModelBinding()) { true => new ModelBinding(), false => $this->container->make($definition->class), @@ -91,15 +124,4 @@ public function transform(mixed $argumentValue): mixed return $bind($argumentValue, $definition); } - - private function bindingDefinition(): BindDefinition - { - return new BindDefinition( - $this->nodeName(), - $this->directiveArgValue('class'), - $this->directiveArgValue('column', 'id'), - $this->directiveArgValue('with', []), - $this->directiveArgValue('optional', false), - ); - } } diff --git a/src/Bind/BindException.php b/src/Bind/BindException.php deleted file mode 100644 index a73ec22ba..000000000 --- a/src/Bind/BindException.php +++ /dev/null @@ -1,36 +0,0 @@ -nodeName with $definition->column `$value`.", - ); - } - - public static function missingRecords(array $value, BindDefinition $definition): self - { - $column = Str::plural($definition->column); - $ids = implode(',', $value); - - return new self( - "No records found for binding $definition->nodeName with $column $ids.", - ); - } - - public function isClientSafe(): bool - { - return true; - } -} diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index 1074e02af..c7b0f4a5b 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -26,32 +26,25 @@ public function __invoke(mixed $value, BindDefinition $definition): Model|Eloque return $this->modelCollection($binding, IlluminateCollection::make($value), $definition); } - return $this->modelInstance($binding, $value, $definition); + return $this->modelInstance($binding); } /** * @param \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> $results */ - private function modelInstance(EloquentCollection $results, mixed $value, BindDefinition $definition): ?Model + private function modelInstance(EloquentCollection $results): ?Model { if ($results->count() > 1) { throw new MultipleRecordsFoundException($results->count()); } - $model = $results->first(); - - if ($definition->optional) { - return $model; - } - - if ($model === null) { - throw BindException::notFound($value, $definition); - } - - return $model; + return $results->first(); } /** + * Binding collections should be returned with the original values + * as keys to allow us to validate the binding when non-optional. + * @see \Nuwave\Lighthouse\Bind\BindDirective::rules() * @param \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> $results * @return \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> */ @@ -64,25 +57,6 @@ private function modelCollection( throw new MultipleRecordsFoundException($results->count()); } - if ($definition->optional) { - return $results->values(); - } - - $results = $results->keyBy($definition->column); - $missingResults = new IlluminateCollection(); - - foreach ($values as $value) { - if ($results->has($value)) { - continue; - } - - $missingResults->push($value); - } - - if ($missingResults->isNotEmpty()) { - throw BindException::missingRecords($missingResults->all(), $definition); - } - - return $results->values(); + return $results->keyby($definition->column); } } diff --git a/src/Bind/Validation/BindingExists.php b/src/Bind/Validation/BindingExists.php new file mode 100644 index 000000000..f100ffabb --- /dev/null +++ b/src/Bind/Validation/BindingExists.php @@ -0,0 +1,55 @@ +validator = $validator; + + return $this; + } + + /** + * @param callable(string): \Illuminate\Translation\PotentiallyTranslatedString $fail + */ + public function validate(string $attribute, mixed $value, Closure $fail): void + { + $binding = $this->directive->transform($value, $this->definition); + + if ($binding === null) { + $fail('validation.exists')->translate(); + + return; + } + + if (! is_array($value)) { + return; + } + + foreach ($value as $key => $scalarValue) { + if ($binding->has($scalarValue)) { + continue; + } + + $this->validator->addFailure("$attribute.$key", 'exists'); + } + } +} diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 3637edb86..a4e413442 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -4,8 +4,6 @@ use GraphQL\Error\Error; use Illuminate\Database\MultipleRecordsFoundException; -use Nuwave\Lighthouse\Bind\BindDefinition; -use Nuwave\Lighthouse\Bind\BindException; use Nuwave\Lighthouse\Exceptions\DefinitionException; use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; @@ -14,6 +12,7 @@ use Tests\Utils\Resolvers\SpyResolver; use function factory; +use function trans; final class BindDirectiveTest extends DBTestCase { @@ -180,7 +179,6 @@ public function testModelBindingOnFieldArgument(): void public function testMissingModelBindingOnFieldArgument(): void { - $this->rethrowGraphQLErrors(); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -191,7 +189,7 @@ public function testMissingModelBindingOnFieldArgument(): void } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' query { user(user: "1") { id @@ -199,11 +197,8 @@ public function testMissingModelBindingOnFieldArgument(): void } GRAPHQL); - $this->assertThrows( - $makeRequest, - Error::class, - BindException::notFound('1', new BindDefinition('user', User::class, 'id', [], false))->getMessage(), - ); + $response->assertOk(); + $response->assertGraphQLValidationError('user', trans('validation.exists', ['attribute' => 'user'])); } public function testMissingOptionalModelBindingOnFieldArgument(): void @@ -387,15 +382,14 @@ public function testModelCollectionBindingOnFieldArgument(): void $resolver->assertArgs(function (array $args) use ($users): void { $this->assertArrayHasKey('users', $args); $this->assertCount($users->count(), $args['users']); - $users->each(function (User $user, int $key) use ($args): void { - $this->assertTrue($user->is($args['users'][$key])); + $users->each(function (User $user) use ($args): void { + $this->assertTrue($user->is($args['users'][$user->getKey()])); }); }); } public function testMissingModelCollectionBindingOnFieldArgument(): void { - $this->rethrowGraphQLErrors(); $user = factory(User::class)->create(); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { @@ -413,7 +407,7 @@ public function testMissingModelCollectionBindingOnFieldArgument(): void } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($users: [ID!]!) { removeUsers(users: $users) } @@ -423,12 +417,8 @@ public function testMissingModelCollectionBindingOnFieldArgument(): void ], ); - $this->assertThrows( - $makeRequest, - Error::class, - BindException::missingRecords(['10'], new BindDefinition('users', User::class, 'id', [], false)) - ->getMessage(), - ); + $response->assertOk(); + $response->assertGraphQLValidationError('users.1', trans('validation.exists', ['attribute' => 'users.1'])); } public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void @@ -471,7 +461,7 @@ public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void $resolver->assertArgs(function (array $args) use ($user): void { $this->assertArrayHasKey('users', $args); $this->assertCount(1, $args['users']); - $this->assertTrue($user->is($args['users'][0])); + $this->assertTrue($user->is($args['users'][$user->getKey()])); }); } @@ -556,7 +546,6 @@ public function testModelBindingOnInputField(): void public function testMissingModelBindingOnInputField(): void { - $this->rethrowGraphQLErrors(); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -571,7 +560,7 @@ public function testMissingModelBindingOnInputField(): void } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' query ($input: UserInput!) { user(input: $input) { id @@ -585,11 +574,10 @@ public function testMissingModelBindingOnInputField(): void ], ); - $this->assertThrows( - $makeRequest, - Error::class, - BindException::notFound('1', new BindDefinition('user', User::class, 'id', [], false))->getMessage(), - ); + $response->assertOk(); + $response->assertGraphQLValidationError('input.user', trans('validation.exists', [ + 'attribute' => 'input.user', + ])); } public function testMissingOptionalModelBindingOnInputField(): void @@ -804,15 +792,14 @@ public function testModelCollectionBindingOnInputField(): void $this->assertArrayHasKey('input', $args); $this->assertArrayHasKey('users', $args['input']); $this->assertCount($users->count(), $args['input']['users']); - $users->each(function (User $user, int $key) use ($args): void { - $this->assertTrue($user->is($args['input']['users'][$key])); + $users->each(function (User $user) use ($args): void { + $this->assertTrue($user->is($args['input']['users'][$user->getKey()])); }); }); } public function testMissingModelCollectionBindingOnInputField(): void { - $this->rethrowGraphQLErrors(); $user = factory(User::class)->create(); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { @@ -832,7 +819,7 @@ public function testMissingModelCollectionBindingOnInputField(): void } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { removeUsers(input: $input) } @@ -844,12 +831,10 @@ public function testMissingModelCollectionBindingOnInputField(): void ], ); - $this->assertThrows( - $makeRequest, - Error::class, - BindException::missingRecords(['10'], new BindDefinition('users', User::class, 'id', [], false)) - ->getMessage(), - ); + $response->assertOk(); + $response->assertGraphQLValidationError('input.users.1', trans('validation.exists', [ + 'attribute' => 'input.users.1', + ])); } public function testMissingOptionalModelCollectionBindingOnInputField(): void @@ -897,7 +882,7 @@ public function testMissingOptionalModelCollectionBindingOnInputField(): void $this->assertArrayHasKey('input', $args); $this->assertArrayHasKey('users', $args['input']); $this->assertCount(1, $args['input']['users']); - $this->assertTrue($user->is($args['input']['users'][0])); + $this->assertTrue($user->is($args['input']['users'][$user->getKey()])); }); } From 371749f9a33fa940a3762b004bb85dbebae04e84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Wed, 25 Dec 2024 08:04:00 +0100 Subject: [PATCH 10/33] Rename `optional` argument to `required` on BindDirective It may be personal, but it feels more Laravel-y to me. --- src/Bind/BindDefinition.php | 3 +-- src/Bind/BindDirective.php | 18 +++++++++--------- tests/Integration/Bind/BindDirectiveTest.php | 8 ++++---- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index 4b5e396be..2c6351069 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -25,11 +25,10 @@ class BindDefinition * @param array $with */ public function __construct( - public string $nodeName, public string $class, public string $column, public array $with, - public bool $optional, + public bool $required, ) {} /** diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index 533cfab5d..ad662f720 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -51,11 +51,12 @@ class: String! with: [String!]! = [] """ - Specify whether the binding should be considered optional. When optional, the argument's - value is set to `null` when no matching binding could be resolved. When the binding - isn't optional, an exception is thrown and the field resolver will not be invoked. + Specify whether the binding should be considered required. When required, a validation error will be thrown for + the argument or any item in the argument (when the argument is an array) for which a binding instance could not be + resolved. The field resolver will not be invoked in this case. When not required, the argument value will resolve + as null or, when the argument is an array, any item in the argument value will be filtered out of the collection. """ - optional: Boolean! = false + required: Boolean! = true ) repeatable on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION GRAPHQL; } @@ -63,11 +64,10 @@ class: String! private function bindDefinition(): BindDefinition { return new BindDefinition( - $this->nodeName(), $this->directiveArgValue('class'), $this->directiveArgValue('column', 'id'), $this->directiveArgValue('with', []), - $this->directiveArgValue('optional', false), + $this->directiveArgValue('required', true), ); } @@ -98,9 +98,9 @@ public function rules(): array { $definition = $this->bindDefinition(); - return match ($definition->optional) { - true => [], - false => [new BindingExists($this, $definition)], + return match ($definition->required) { + true => [new BindingExists($this, $definition)], + false => [], }; } diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index a4e413442..22dc89c34 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -211,7 +211,7 @@ public function testMissingOptionalModelBindingOnFieldArgument(): void type Query { user( - user: ID! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + user: ID! @bind(class: "Tests\\Utils\\Models\\User", required: false) ): User @mock } GRAPHQL; @@ -433,7 +433,7 @@ public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void type Mutation { removeUsers( - users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", required: false) ): Boolean! @mock } @@ -589,7 +589,7 @@ public function testMissingOptionalModelBindingOnInputField(): void } input UserInput { - user: ID! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + user: ID! @bind(class: "Tests\\Utils\\Models\\User", required: false) } type Query { @@ -848,7 +848,7 @@ public function testMissingOptionalModelCollectionBindingOnInputField(): void } input RemoveUsersInput { - users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", optional: true) + users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", required: false) } type Mutation { From a03666ef4967f3620d529684020bb274fbfd295b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Wed, 25 Dec 2024 08:27:15 +0100 Subject: [PATCH 11/33] Cleanup exception assetions in BindDirectiveTest --- tests/Integration/Bind/BindDirectiveTest.php | 66 +++++++++----------- 1 file changed, 30 insertions(+), 36 deletions(-) diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 22dc89c34..dd946f8e6 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -4,6 +4,7 @@ use GraphQL\Error\Error; use Illuminate\Database\MultipleRecordsFoundException; +use Illuminate\Support\Str; use Nuwave\Lighthouse\Exceptions\DefinitionException; use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; @@ -39,11 +40,10 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument } GRAPHQL); - $this->assertThrows( - $makeRequest, - DefinitionException::class, + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( + $exception->getMessage(), 'argument `user` of field `user` must be an existing class', - ); + )); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAClass(): void @@ -74,11 +74,10 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN ], ); - $this->assertThrows( - $makeRequest, - DefinitionException::class, + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( + $exception->getMessage(), 'field `users` of input `RemoveUsersInput` must be an existing class', - ); + )); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgumentIsNotAModelOrCallableClass(): void @@ -101,11 +100,10 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument } GRAPHQL); - $this->assertThrows( - $makeRequest, - DefinitionException::class, + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( + $exception->getMessage(), 'argument `user` of field `user` must be an Eloquent model or a callable class', - ); + )); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAModelOrCallableClass(): void @@ -136,11 +134,10 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN ], ); - $this->assertThrows( - $makeRequest, - DefinitionException::class, + $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( + $exception->getMessage(), 'field `users` of input `RemoveUsersInput` must be an Eloquent model or a callable class', - ); + )); } public function testModelBindingOnFieldArgument(): void @@ -337,11 +334,7 @@ public function testModelBindingWithTooManyResultsOnFieldArgument(): void ['name' => $users->first()->name], ); - $this->assertThrows( - $makeRequest, - Error::class, - (new MultipleRecordsFoundException($users->count()))->getMessage(), - ); + $this->assertThrowsMultipleRecordsFoundException($makeRequest, $users->count()); } public function testModelCollectionBindingOnFieldArgument(): void @@ -495,11 +488,7 @@ public function testModelCollectionBindingWithTooManyResultsOnFieldArgument(): v ], ); - $this->assertThrows( - $makeRequest, - Error::class, - (new MultipleRecordsFoundException($users->count()))->getMessage(), - ); + $this->assertThrowsMultipleRecordsFoundException($makeRequest, $users->count()); } public function testModelBindingOnInputField(): void @@ -740,11 +729,7 @@ public function testModelBindingWithTooManyResultsOnInputField(): void ], ); - $this->assertThrows( - $makeRequest, - Error::class, - (new MultipleRecordsFoundException($users->count()))->getMessage(), - ); + $this->assertThrowsMultipleRecordsFoundException($makeRequest, $users->count()); } public function testModelCollectionBindingOnInputField(): void @@ -920,10 +905,19 @@ public function testModelCollectionBindingWithTooManyResultsOnInputField(): void ], ); - $this->assertThrows( - $makeRequest, - Error::class, - (new MultipleRecordsFoundException($users->count()))->getMessage(), - ); + $this->assertThrowsMultipleRecordsFoundException($makeRequest, $users->count()); + } + + private function assertThrowsMultipleRecordsFoundException(callable $makeRequest, int $count): void + { + $this->assertThrows($makeRequest, function (Error $exception) use ($count): bool { + $expected = new MultipleRecordsFoundException($count); + + if (! $exception->getPrevious() instanceof $expected) { + return false; + } + + return $exception->getMessage() === $expected->getMessage(); + }); } } From 6c6e4e40547f2a278a6cbbe914c83b9b9e3addf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Thu, 26 Dec 2024 15:05:34 +0100 Subject: [PATCH 12/33] Fail schema validation when `@bind` directive is defined on unsupported value type --- src/Bind/BindDefinition.php | 63 ++++++---- src/Bind/BindDirective.php | 11 +- src/Bind/ModelBinding.php | 2 +- tests/Integration/Bind/BindDirectiveTest.php | 121 +++++++++++++++---- 4 files changed, 139 insertions(+), 58 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index 2c6351069..ca1dd557d 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -2,14 +2,19 @@ namespace Nuwave\Lighthouse\Bind; +use GraphQL\Language\AST\FieldDefinitionNode; +use GraphQL\Language\AST\InputObjectTypeDefinitionNode; +use GraphQL\Language\AST\InputValueDefinitionNode; +use GraphQL\Language\AST\TypeNode; use Illuminate\Database\Eloquent\Model; -use Illuminate\Support\Collection; use Nuwave\Lighthouse\Exceptions\DefinitionException; use function class_exists; +use function implode; +use function in_array; use function is_callable; use function is_subclass_of; -use function sprintf; +use function property_exists; /** * @template TClass @@ -20,6 +25,8 @@ */ class BindDefinition { + private const SUPPORTED_VALUE_TYPES = ['ID', 'String', 'Int']; + /** * @param class-string $class * @param array $with @@ -31,16 +38,26 @@ public function __construct( public bool $required, ) {} - /** - * @param array $exceptionMessagePlaceholders - */ - public function validate(array $exceptionMessagePlaceholders): void - { + public function validate( + InputValueDefinitionNode $definitionNode, + FieldDefinitionNode|InputObjectTypeDefinitionNode $parentNode, + ): void { + $nodeName = $definitionNode->name->value; + $parentNodeName = $parentNode->name->value; + $valueType = $this->valueType($definitionNode->type); + + if (! in_array($valueType, self::SUPPORTED_VALUE_TYPES, true)) { + throw new DefinitionException( + "@bind directive defined on `$parentNodeName.$nodeName` does not support value of type `$valueType`. " . + "Expected `" . implode('`, `', self::SUPPORTED_VALUE_TYPES) . '` or a list of one of these types.' + ); + } + if (! class_exists($this->class)) { - throw new DefinitionException(sprintf( - "@bind argument `class` defined on %s of %s must be an existing class, received `$this->class`.", - ...$this->formatExceptionMessagePlaceholders($exceptionMessagePlaceholders), - )); + throw new DefinitionException( + "@bind argument `class` defined on `$parentNodeName.$nodeName` " . + "must be an existing class, received `$this->class`.", + ); } if ($this->isModelBinding()) { @@ -51,23 +68,19 @@ public function validate(array $exceptionMessagePlaceholders): void return; } - throw new DefinitionException(sprintf( - "@bind argument `class` defined on %s of %s must be an Eloquent " . - "model or a callable class, received `$this->class`.", - ...$this->formatExceptionMessagePlaceholders($exceptionMessagePlaceholders), - )); + throw new DefinitionException( + "@bind argument `class` defined on `$parentNodeName.$nodeName` must be " . + "an Eloquent model or a callable class, received `$this->class`.", + ); } - /** - * @param array $placeholders - * @return array - */ - private function formatExceptionMessagePlaceholders(array $placeholders): array + private function valueType(TypeNode $typeNode): string { - return Collection::make($placeholders) - ->map(fn (string $value, string $key): string => "$key `$value`") - ->values() - ->all(); + if (property_exists($typeNode, 'type')) { + return $this->valueType($typeNode->type); + } + + return $typeNode->name->value; } public function isModelBinding(): bool diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index ad662f720..7d3118fe2 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -77,10 +77,7 @@ public function manipulateArgDefinition( FieldDefinitionNode &$parentField, ObjectTypeDefinitionNode|InterfaceTypeDefinitionNode &$parentType, ): void { - $this->bindDefinition()->validate([ - 'argument' => $argDefinition->name->value, - 'field' => $parentField->name->value, - ]); + $this->bindDefinition()->validate($argDefinition, $parentField); } public function manipulateInputFieldDefinition( @@ -88,10 +85,7 @@ public function manipulateInputFieldDefinition( InputValueDefinitionNode &$inputField, InputObjectTypeDefinitionNode &$parentInput, ): void { - $this->bindDefinition()->validate([ - 'field' => $inputField->name->value, - 'input' => $parentInput->name->value, - ]); + $this->bindDefinition()->validate($inputField, $parentInput); } public function rules(): array @@ -117,6 +111,7 @@ public function attribute(): ?string public function transform(mixed $argumentValue, ?BindDefinition $definition = null): mixed { $definition ??= $this->bindDefinition(); + $bind = match ($definition->isModelBinding()) { true => new ModelBinding(), false => $this->container->make($definition->class), diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index c7b0f4a5b..c916f8f49 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -15,7 +15,7 @@ class ModelBinding /** * @param \Nuwave\Lighthouse\Bind\BindDefinition<\Illuminate\Database\Eloquent\Model> $definition */ - public function __invoke(mixed $value, BindDefinition $definition): Model|EloquentCollection|null + public function __invoke(int|string|array $value, BindDefinition $definition): Model|EloquentCollection|null { $binding = $definition->class::query() ->with($definition->with) diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index dd946f8e6..ee9a94ba5 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -4,7 +4,6 @@ use GraphQL\Error\Error; use Illuminate\Database\MultipleRecordsFoundException; -use Illuminate\Support\Str; use Nuwave\Lighthouse\Exceptions\DefinitionException; use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; @@ -32,18 +31,17 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $this->expectExceptionObject(new DefinitionException( + '@bind argument `class` defined on `user.user` must be an existing class, received `NotAClass`.' + )); + + $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' query { user(user: "1") { id } } GRAPHQL); - - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( - $exception->getMessage(), - 'argument `user` of field `user` must be an existing class', - )); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAClass(): void @@ -62,7 +60,11 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $this->expectExceptionObject(new DefinitionException( + '@bind argument `class` defined on `RemoveUsersInput.users` must be an existing class, received `NotAClass`.' + )); + + $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { removeUsers(input: $input) } @@ -73,11 +75,6 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN ], ], ); - - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( - $exception->getMessage(), - 'field `users` of input `RemoveUsersInput` must be an existing class', - )); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgumentIsNotAModelOrCallableClass(): void @@ -92,18 +89,18 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $this->expectExceptionObject(new DefinitionException( + '@bind argument `class` defined on `user.user` must be an ' . + 'Eloquent model or a callable class, received `stdClass`.' + )); + + $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' query { user(user: "1") { id } } GRAPHQL); - - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( - $exception->getMessage(), - 'argument `user` of field `user` must be an Eloquent model or a callable class', - )); } public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsNotAModelOrCallableClass(): void @@ -122,7 +119,12 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN } GRAPHQL; - $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + $this->expectExceptionObject(new DefinitionException( + '@bind argument `class` defined on `RemoveUsersInput.users` must be an ' . + 'Eloquent model or a callable class, received `stdClass`.' + )); + + $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { removeUsers(input: $input) } @@ -130,14 +132,85 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN [ 'input' => [ 'users' => ['1'], - ] + ], ], ); + } + + public function testSchemaValidationFailsWhenValueTypeDefinedOnFieldArgumentIsNotSupported(): void + { + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + type: UserType! + } + + enum UserType { + ADMINISTRATOR + MODERATOR + } + + type Query { + usersByType(type: UserType! @bind(class: "Tests\\Utils\\Models\\User")): User! @mock + } + GRAPHQL; + + $this->expectExceptionObject(new DefinitionException( + '@bind directive defined on `usersByType.type` does not support value of type `UserType`. ' . + 'Expected `ID`, `String`, `Int` or a list of one of these types.' + )); + + $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($type: UserType!) { + usersByType(type: $type) { + id + } + } + GRAPHQL, + ['type' => 'ADMINISTRATOR'], + ); + } - $this->assertThrows($makeRequest, fn (DefinitionException $exception): bool => Str::contains( - $exception->getMessage(), - 'field `users` of input `RemoveUsersInput` must be an Eloquent model or a callable class', + public function testSchemaValidationFailsWhenValueTypeDefinedOnInputFieldIsNotSupported(): void + { + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + type: UserType! + } + + enum UserType { + ADMINISTRATOR + MODERATOR + } + + input UsersByTypeInput { + type: UserType! @bind(class: "Tests\\Utils\\Models\\User") + } + + type Query { + usersByType(input: UsersByTypeInput!): User! @mock + } + GRAPHQL; + + $this->expectExceptionObject(new DefinitionException( + '@bind directive defined on `UsersByTypeInput.type` does not support value of type `UserType`. ' . + 'Expected `ID`, `String`, `Int` or a list of one of these types.' )); + + $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UsersByTypeInput!) { + usersByType(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'type' => 'ADMINISTRATOR', + ], + ], + ); } public function testModelBindingOnFieldArgument(): void From a291ef2f4d967964815b5e013bebb654dce4497f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 27 Dec 2024 08:15:37 +0100 Subject: [PATCH 13/33] Ensure `@bind` directive can be used multiple times in the same request --- src/Bind/BindDirective.php | 36 ++++++++----- src/Bind/PendingBinding.php | 5 ++ src/Bind/Validation/BindingExists.php | 4 +- tests/Integration/Bind/BindDirectiveTest.php | 53 ++++++++++++++++++++ 4 files changed, 82 insertions(+), 16 deletions(-) create mode 100644 src/Bind/PendingBinding.php diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index 7d3118fe2..ea0fe607a 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -19,9 +19,14 @@ class BindDirective extends BaseDirective implements ArgumentValidation, ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator { + private ?BindDefinition $definition = null; + private mixed $binding; + public function __construct( private Container $container, - ) {} + ) { + $this->binding = new PendingBinding(); + } public static function definition(): string { @@ -52,18 +57,18 @@ class: String! """ Specify whether the binding should be considered required. When required, a validation error will be thrown for - the argument or any item in the argument (when the argument is an array) for which a binding instance could not be - resolved. The field resolver will not be invoked in this case. When not required, the argument value will resolve + the argument or any item in the argument (when the argument is an array) for which a binding instance could not + be resolved. The field resolver will not be invoked in this case. When optional, the argument value will resolve as null or, when the argument is an array, any item in the argument value will be filtered out of the collection. """ required: Boolean! = true -) repeatable on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION +) on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION GRAPHQL; } private function bindDefinition(): BindDefinition { - return new BindDefinition( + return $this->definition ??= new BindDefinition( $this->directiveArgValue('class'), $this->directiveArgValue('column', 'id'), $this->directiveArgValue('with', []), @@ -90,10 +95,8 @@ public function manipulateInputFieldDefinition( public function rules(): array { - $definition = $this->bindDefinition(); - - return match ($definition->required) { - true => [new BindingExists($this, $definition)], + return match ($this->bindDefinition()->required) { + true => [new BindingExists($this)], false => [], }; } @@ -108,15 +111,22 @@ public function attribute(): ?string return null; } - public function transform(mixed $argumentValue, ?BindDefinition $definition = null): mixed + public function transform(mixed $argumentValue): mixed { - $definition ??= $this->bindDefinition(); + // When validating required bindings, the \Nuwave\Lighthouse\Bind\Validation\BindingExists validation rule + // should call transform() before it is called by the directive resolver. To avoid resolving the bindings + // multiple times, we should remember the resolved binding and reuse it every time transform() is called. + if (! $this->binding instanceof PendingBinding) { + return $this->binding; + } + + $definition = $this->bindDefinition(); $bind = match ($definition->isModelBinding()) { - true => new ModelBinding(), + true => $this->container->make(ModelBinding::class), false => $this->container->make($definition->class), }; - return $bind($argumentValue, $definition); + return $this->binding = $bind($argumentValue, $definition); } } diff --git a/src/Bind/PendingBinding.php b/src/Bind/PendingBinding.php new file mode 100644 index 000000000..74cc1b71e --- /dev/null +++ b/src/Bind/PendingBinding.php @@ -0,0 +1,5 @@ +directive->transform($value, $this->definition); + $binding = $this->directive->transform($value); if ($binding === null) { $fail('validation.exists')->translate(); diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index ee9a94ba5..b974cb787 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -8,6 +8,7 @@ use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; use Tests\DBTestCase; +use Tests\Utils\Models\Company; use Tests\Utils\Models\User; use Tests\Utils\Resolvers\SpyResolver; @@ -981,6 +982,58 @@ public function testModelCollectionBindingWithTooManyResultsOnInputField(): void $this->assertThrowsMultipleRecordsFoundException($makeRequest, $users->count()); } + public function testMultipleBindingsInSameRequest(): void + { + $user = factory(User::class)->create(); + $company = factory(Company::class)->create(); + $resolver = new SpyResolver(return: true); + $this->mockResolver($resolver); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Company { + id: ID! + } + + type Mutation { + addUserToCompany( + user: ID! @bind(class: "Tests\\Utils\\Models\\User") + company: ID! @bind(class: "Tests\\Utils\\Models\\Company") + ): Boolean! @mock + } + + type Query { + ping: Boolean + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + mutation ($user: ID!, $company: ID!) { + addUserToCompany(user: $user, company: $company) + } + GRAPHQL, + [ + 'user' => $user->getKey(), + 'company' => $company->getKey(), + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'addUserToCompany' => true, + ], + ]); + $resolver->assertArgs(function (array $args) use ($user, $company): void { + $this->assertArrayHasKey('user', $args); + $this->assertTrue($user->is($args['user'])); + $this->assertArrayHasKey('company', $args); + $this->assertTrue($company->is($args['company'])); + }); + } + private function assertThrowsMultipleRecordsFoundException(callable $makeRequest, int $count): void { $this->assertThrows($makeRequest, function (Error $exception) use ($count): bool { From aa542573d3ca351f9f84fd4e99fecf2be5e4d8a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 27 Dec 2024 15:14:46 +0100 Subject: [PATCH 14/33] Add missing tests for binding instances using callable classes --- src/Bind/BindDefinition.php | 4 +- tests/Integration/Bind/BindDirectiveTest.php | 312 +++++++++++++++++++ tests/Utils/Bind/SpyCallableClassBinding.php | 41 +++ 3 files changed, 355 insertions(+), 2 deletions(-) create mode 100644 tests/Utils/Bind/SpyCallableClassBinding.php diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index ca1dd557d..0bd78c035 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -12,8 +12,8 @@ use function class_exists; use function implode; use function in_array; -use function is_callable; use function is_subclass_of; +use function method_exists; use function property_exists; /** @@ -64,7 +64,7 @@ public function validate( return; } - if (is_callable($this->class)) { + if (method_exists($this->class, '__invoke')) { return; } diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index b974cb787..7972638be 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -4,10 +4,12 @@ use GraphQL\Error\Error; use Illuminate\Database\MultipleRecordsFoundException; +use Nuwave\Lighthouse\Bind\BindDefinition; use Nuwave\Lighthouse\Exceptions\DefinitionException; use Nuwave\Lighthouse\Testing\MocksResolvers; use Nuwave\Lighthouse\Testing\UsesTestSchema; use Tests\DBTestCase; +use Tests\Utils\Bind\SpyCallableClassBinding; use Tests\Utils\Models\Company; use Tests\Utils\Models\User; use Tests\Utils\Resolvers\SpyResolver; @@ -982,6 +984,316 @@ public function testModelCollectionBindingWithTooManyResultsOnInputField(): void $this->assertThrowsMultipleRecordsFoundException($makeRequest, $users->count()); } + public function testCallableClassBindingOnFieldArgument(): void + { + $user = factory(User::class)->make(['id' => 1]); + $this->instance(SpyCallableClassBinding::class, new SpyCallableClassBinding($user)); + $this->mockResolver(fn (mixed $root, array $args): User => $args['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user(user: ID! @bind(class: "Tests\\Utils\\Bind\\SpyCallableClassBinding")): User! @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, + ['id' => $user->getKey()], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->getKey(), + ], + ], + ]); + } + + public function testMissingCallableClassBindingOnFieldArgument(): void + { + $this->instance(SpyCallableClassBinding::class, new SpyCallableClassBinding(null)); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: ID! @bind(class: "Tests\\Utils\\Bind\\SpyCallableClassBinding") + ): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, + ['id' => '1'], + ); + + $response->assertOk(); + $response->assertGraphQLValidationError('user', trans('validation.exists', ['attribute' => 'user'])); + } + + public function testMissingOptionalCallableClassBindingOnFieldArgument(): void + { + $this->instance(SpyCallableClassBinding::class, new SpyCallableClassBinding(null)); + $this->mockResolver(fn (mixed $root, array $args) => $args['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: ID! @bind(class: "Tests\\Utils\\Bind\\SpyCallableClassBinding", required: false) + ): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, + ['id' => '1'], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => null, + ], + ]); + } + + public function testCallableClassBindingWithDirectiveArgumentsOnFieldArgument(): void + { + $callableClassBinding = new SpyCallableClassBinding(null); + $this->instance(SpyCallableClassBinding::class, $callableClassBinding); + $this->mockResolver(fn (mixed $root, array $args) => $args['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + type Query { + user( + user: ID! @bind( + class: "Tests\\Utils\\Bind\\SpyCallableClassBinding" + column: "uid" + with: ["relation"] + required: false + ) + ): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($id: ID!) { + user(user: $id) { + id + } + } + GRAPHQL, + ['id' => '1'], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => null, + ], + ]); + $callableClassBinding->assertCalledWith( + '1', + new BindDefinition(SpyCallableClassBinding::class, 'uid', ['relation'], false), + ); + } + + public function testCallableClassBindingOnInputField(): void + { + $user = factory(User::class)->make(['id' => 1]); + $this->instance(SpyCallableClassBinding::class, new SpyCallableClassBinding($user)); + $this->mockResolver(fn (mixed $root, array $args): User => $args['input']['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Bind\\SpyCallableClassBinding") + } + + type Query { + user(input: UserInput!): User! @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => $user->getKey(), + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => [ + 'id' => $user->getKey(), + ], + ], + ]); + } + + public function testMissingCallableClassBindingOnInputField(): void + { + $this->instance(SpyCallableClassBinding::class, new SpyCallableClassBinding(null)); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Bind\\SpyCallableClassBinding") + } + + type Query { + user(input: UserInput!): User! @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => '1', + ], + ], + ); + + $response->assertOk(); + $response->assertGraphQLValidationError('input.user', trans('validation.exists', ['attribute' => 'input.user'])); + } + + public function testMissingOptionalCallableClassBindingOnInputField(): void + { + $this->instance(SpyCallableClassBinding::class, new SpyCallableClassBinding(null)); + $this->mockResolver(fn (mixed $root, array $args) => $args['input']['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind(class: "Tests\\Utils\\Bind\\SpyCallableClassBinding", required: false) + } + + type Query { + user(input: UserInput!): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => '1', + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => null, + ], + ]); + } + + public function testCallableClassBindingWithDirectiveArgumentsOnInputField(): void + { + $callableClassBinding = new SpyCallableClassBinding(null); + $this->instance(SpyCallableClassBinding::class, $callableClassBinding); + $this->mockResolver(fn (mixed $root, array $args) => $args['input']['user']); + $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' + type User { + id: ID! + } + + input UserInput { + user: ID! @bind( + class: "Tests\\Utils\\Bind\\SpyCallableClassBinding" + column: "uid" + with: ["relation"] + required: false + ) + } + + type Query { + user(input: UserInput!): User @mock + } + GRAPHQL; + + $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' + query ($input: UserInput!) { + user(input: $input) { + id + } + } + GRAPHQL, + [ + 'input' => [ + 'user' => '1', + ], + ], + ); + + $response->assertGraphQLErrorFree(); + $response->assertJson([ + 'data' => [ + 'user' => null, + ], + ]); + $callableClassBinding->assertCalledWith( + '1', + new BindDefinition(SpyCallableClassBinding::class, 'uid', ['relation'], false), + ); + } + public function testMultipleBindingsInSameRequest(): void { $user = factory(User::class)->create(); diff --git a/tests/Utils/Bind/SpyCallableClassBinding.php b/tests/Utils/Bind/SpyCallableClassBinding.php new file mode 100644 index 000000000..2213661b4 --- /dev/null +++ b/tests/Utils/Bind/SpyCallableClassBinding.php @@ -0,0 +1,41 @@ +value = $value; + $this->definition = $definition; + + return $this->return; + } + + public function assertCalledWith(mixed $value, BindDefinition $definition): void + { + Assert::assertSame($value, $this->value); + Assert::assertEquals($definition, $this->definition); + } +} From 8080404cb27628711c0456870ee53840b0fe1ae7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 27 Dec 2024 15:58:14 +0100 Subject: [PATCH 15/33] Add docs --- docs/master/api-reference/directives.md | 147 +++++++++++++++++++ src/Bind/BindDirective.php | 4 +- tests/Integration/Bind/BindDirectiveTest.php | 3 +- tests/Utils/Bind/SpyCallableClassBinding.php | 4 +- 4 files changed, 152 insertions(+), 6 deletions(-) diff --git a/docs/master/api-reference/directives.md b/docs/master/api-reference/directives.md index c39bf6b0c..8ed76da43 100644 --- a/docs/master/api-reference/directives.md +++ b/docs/master/api-reference/directives.md @@ -460,6 +460,153 @@ type RoleEdge { } ``` +## @bind + +```graphql +""" +Automatically inject (model) instances directly into a resolver's arguments. For example, instead of +injecting a user's ID, you can inject the entire User model instance that matches the given ID. + +This is a GraphQL analogue for Laravel's Route Binding. +""" +directive @bind( + """ + Specify the class name of the binding to use. This can be either an Eloquent + model or callable class to bind any other instance than a model. + """ + class: String! + + """ + Specify the column name of a unique identifier to use when binding Eloquent models. + By default, "id" is used the the primary key column. + """ + column: String! = "id" + + """ + Specify the relations to eager-load when binding Eloquent models. + """ + with: [String!]! = [] + + """ + Specify whether the binding should be considered required. When required, a validation error will be thrown for + the argument or any item in the argument (when the argument is an array) for which a binding instance could not + be resolved. The field resolver will not be invoked in this case. When optional, the argument value will resolve + as null or, when the argument is an array, any item in the argument value will be filtered out of the collection. + """ + required: Boolean! = true +) on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION +``` + +Resolver model binding (like [Route Model Binding](https://laravel.com/docs/11.x/routing#route-model-binding)) allows to +inject instances directly into a resolver's arguments instead of the provided scalar identifier, eliminating the need to +manually query for the instance inside the resolver. + +### Basic usage + +```graphql +type Mutation { + addUserToCompany( + user: ID! @bind(class: "App\\Models\\User") + company: ID! @bind(class: "App\\Models\\Company") + ): Boolean! +} +``` + +```php +associate($args['company'])->save(); + } +} +``` + +### Binding instances that are not Eloquent models + +To bind instances that are not Eloquent models, callable classes can be used instead: + +```graphql +type Mutation { + updateCompanyInfo( + company: ID! @bind(class: "App\\Http\\GraphQL\\Bindings\\CompanyBinding") + ): Boolean! +} +``` + +```php +definition->required) { + return $this->companyRepository->findOrFail($value); + } + + return $this->companyRepository->find($value); + } +} +``` + +### Binding a collection of instances + +When the `@bind` directive is defined on an argument or input field with an array value, it can be used to resolve a +collection of instances. + +```graphql +type Mutation { + addUsersToCompany( + users: [ID!]! @bind(class: "App\\Models\\User") + company: ID! @bind(class: "App\\Models\\Company") + ): Boolean! +} +``` + +```php +, + * company: \App\Models\Company + * } $args + */ + public function __invoke(mixed $root, array $args): bool + { + foreach ($args['users'] as $user) { + if (! $user->associate($args['company'])->save()) { + return false; + } + } + + return true; + } +} +``` + ## @broadcast ```graphql diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index ea0fe607a..01644d417 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -32,8 +32,8 @@ public static function definition(): string { return /** @lang GraphQL */ <<<'GRAPHQL' """ -Automatically inject (model) instances directly into a resolver argument or input field. For example, instead -of injecting a user's ID, you can inject the entire User model instance that matches the given ID. +Automatically inject (model) instances directly into a resolver's arguments. For example, instead of +injecting a user's ID, you can inject the entire User model instance that matches the given ID. This is a GraphQL analogue for Laravel's Route Binding. """ diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 7972638be..86a5a307a 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -2,6 +2,7 @@ namespace Tests\Integration\Bind; +use Closure; use GraphQL\Error\Error; use Illuminate\Database\MultipleRecordsFoundException; use Nuwave\Lighthouse\Bind\BindDefinition; @@ -1346,7 +1347,7 @@ public function testMultipleBindingsInSameRequest(): void }); } - private function assertThrowsMultipleRecordsFoundException(callable $makeRequest, int $count): void + private function assertThrowsMultipleRecordsFoundException(Closure $makeRequest, int $count): void { $this->assertThrows($makeRequest, function (Error $exception) use ($count): bool { $expected = new MultipleRecordsFoundException($count); diff --git a/tests/Utils/Bind/SpyCallableClassBinding.php b/tests/Utils/Bind/SpyCallableClassBinding.php index 2213661b4..aab6e7aa7 100644 --- a/tests/Utils/Bind/SpyCallableClassBinding.php +++ b/tests/Utils/Bind/SpyCallableClassBinding.php @@ -1,6 +1,4 @@ - Date: Fri, 27 Dec 2024 21:08:06 +0100 Subject: [PATCH 16/33] Fix PHPStan errors --- src/Bind/BindDefinition.php | 9 +++++++-- src/Bind/BindDirective.php | 6 ++++++ src/Bind/ModelBinding.php | 11 ++++++++--- src/Bind/Validation/BindingExists.php | 4 ++-- tests/Utils/Bind/SpyCallableClassBinding.php | 8 ++++++++ tests/Utils/Resolvers/SpyResolver.php | 4 ++++ 6 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index 0bd78c035..fa3477263 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -5,6 +5,7 @@ use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\InputObjectTypeDefinitionNode; use GraphQL\Language\AST\InputValueDefinitionNode; +use GraphQL\Language\AST\NamedTypeNode; use GraphQL\Language\AST\TypeNode; use Illuminate\Database\Eloquent\Model; use Nuwave\Lighthouse\Exceptions\DefinitionException; @@ -17,7 +18,7 @@ use function property_exists; /** - * @template TClass + * @template-covariant TClass of object * @property-read class-string $class * @property-read string $column * @property-read array $with @@ -80,7 +81,11 @@ private function valueType(TypeNode $typeNode): string return $this->valueType($typeNode->type); } - return $typeNode->name->value; + if ($typeNode instanceof NamedTypeNode) { + return $typeNode->name->value; + } + + return ''; } public function isModelBinding(): bool diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index 01644d417..c460f7a20 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -19,6 +19,9 @@ class BindDirective extends BaseDirective implements ArgumentValidation, ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator { + /** + * @var \Nuwave\Lighthouse\Bind\BindDefinition|null + */ private ?BindDefinition $definition = null; private mixed $binding; @@ -66,6 +69,9 @@ class: String! GRAPHQL; } + /** + * @return \Nuwave\Lighthouse\Bind\BindDefinition + */ private function bindDefinition(): BindDefinition { return $this->definition ??= new BindDefinition( diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index c916f8f49..4014b18f9 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -13,7 +13,9 @@ class ModelBinding { /** + * @param int|string|array $value * @param \Nuwave\Lighthouse\Bind\BindDefinition<\Illuminate\Database\Eloquent\Model> $definition + * @return \Illuminate\Database\Eloquent\Model|\Illuminate\Database\Eloquent\Collection|null */ public function __invoke(int|string|array $value, BindDefinition $definition): Model|EloquentCollection|null { @@ -30,7 +32,7 @@ public function __invoke(int|string|array $value, BindDefinition $definition): M } /** - * @param \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> $results + * @param \Illuminate\Database\Eloquent\Collection $results */ private function modelInstance(EloquentCollection $results): ?Model { @@ -45,8 +47,11 @@ private function modelInstance(EloquentCollection $results): ?Model * Binding collections should be returned with the original values * as keys to allow us to validate the binding when non-optional. * @see \Nuwave\Lighthouse\Bind\BindDirective::rules() - * @param \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> $results - * @return \Illuminate\Database\Eloquent\Collection<\Illuminate\Database\Eloquent\Model> + * + * @param \Illuminate\Database\Eloquent\Collection $results + * @param \Illuminate\Support\Collection $values + * @param \Nuwave\Lighthouse\Bind\BindDefinition<\Illuminate\Database\Eloquent\Model> $definition + * @return \Illuminate\Database\Eloquent\Collection */ private function modelCollection( EloquentCollection $results, diff --git a/src/Bind/Validation/BindingExists.php b/src/Bind/Validation/BindingExists.php index 5e055d1d0..1504896f1 100644 --- a/src/Bind/Validation/BindingExists.php +++ b/src/Bind/Validation/BindingExists.php @@ -26,7 +26,7 @@ public function setValidator(Validator $validator): self } /** - * @param callable(string): \Illuminate\Translation\PotentiallyTranslatedString $fail + * @param \Closure(string): \Illuminate\Translation\PotentiallyTranslatedString $fail */ public function validate(string $attribute, mixed $value, Closure $fail): void { @@ -47,7 +47,7 @@ public function validate(string $attribute, mixed $value, Closure $fail): void continue; } - $this->validator->addFailure("$attribute.$key", 'exists'); + $this->validator?->addFailure("$attribute.$key", 'exists'); } } } diff --git a/tests/Utils/Bind/SpyCallableClassBinding.php b/tests/Utils/Bind/SpyCallableClassBinding.php index aab6e7aa7..89109b7ca 100644 --- a/tests/Utils/Bind/SpyCallableClassBinding.php +++ b/tests/Utils/Bind/SpyCallableClassBinding.php @@ -11,6 +11,10 @@ final class SpyCallableClassBinding { private mixed $value = null; + + /** + * @var \Nuwave\Lighthouse\Bind\BindDefinition|null + */ private ?BindDefinition $definition = null; /** @@ -21,6 +25,7 @@ public function __construct( ) {} /** + * @param \Nuwave\Lighthouse\Bind\BindDefinition $definition * @return TReturn */ public function __invoke(mixed $value, BindDefinition $definition): mixed @@ -31,6 +36,9 @@ public function __invoke(mixed $value, BindDefinition $definition): mixed return $this->return; } + /** + * @param \Nuwave\Lighthouse\Bind\BindDefinition $definition + */ public function assertCalledWith(mixed $value, BindDefinition $definition): void { Assert::assertSame($value, $this->value); diff --git a/tests/Utils/Resolvers/SpyResolver.php b/tests/Utils/Resolvers/SpyResolver.php index d80d93854..c973ad9e1 100644 --- a/tests/Utils/Resolvers/SpyResolver.php +++ b/tests/Utils/Resolvers/SpyResolver.php @@ -9,6 +9,9 @@ */ final class SpyResolver { + /** + * @var array + */ private array $args = []; /** @@ -19,6 +22,7 @@ public function __construct( ) {} /** + * @param array $args * @return TReturn */ public function __invoke(mixed $root, array $args): mixed From 23dd711b2de0dfafcfe7c42eba108535554d4ab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 27 Dec 2024 21:29:00 +0100 Subject: [PATCH 17/33] Fix backwards compatibility with Laravel 9 --- src/Bind/Validation/BindingExists.php | 34 +++++++++++++------- tests/Integration/Bind/BindDirectiveTest.php | 15 +++++---- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/Bind/Validation/BindingExists.php b/src/Bind/Validation/BindingExists.php index 1504896f1..502dbef55 100644 --- a/src/Bind/Validation/BindingExists.php +++ b/src/Bind/Validation/BindingExists.php @@ -2,15 +2,14 @@ namespace Nuwave\Lighthouse\Bind\Validation; -use Closure; -use Illuminate\Contracts\Validation\ValidationRule; +use Illuminate\Contracts\Validation\InvokableRule; use Illuminate\Contracts\Validation\ValidatorAwareRule; use Illuminate\Validation\Validator; use Nuwave\Lighthouse\Bind\BindDirective; use function is_array; -class BindingExists implements ValidationRule, ValidatorAwareRule +class BindingExists implements InvokableRule, ValidatorAwareRule { private ?Validator $validator = null; @@ -18,17 +17,16 @@ public function __construct( private BindDirective $directive, ) {} - public function setValidator(Validator $validator): self - { - $this->validator = $validator; - - return $this; - } - /** + * Because of backwards compatibility with Laravel 9, typehints for this method + * must be set through PHPDoc as the interface did not include typehints. + * @link https://laravel.com/docs/9.x/validation#using-rule-objects + * + * @param string $attribute + * @parent mixed $value * @param \Closure(string): \Illuminate\Translation\PotentiallyTranslatedString $fail */ - public function validate(string $attribute, mixed $value, Closure $fail): void + public function __invoke($attribute, $value, $fail): void { $binding = $this->directive->transform($value); @@ -50,4 +48,18 @@ public function validate(string $attribute, mixed $value, Closure $fail): void $this->validator?->addFailure("$attribute.$key", 'exists'); } } + + /** + * Because of backwards compatibility with Laravel 9, typehints for this method + * must be set through PHPDoc as the interface did not include typehints. + * @link https://laravel.com/docs/9.x/validation#custom-validation-rules + * + * @param \Illuminate\Validation\Validator $validator + */ + public function setValidator($validator): self + { + $this->validator = $validator; + + return $this; + } } diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 86a5a307a..58f3c66d0 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -1349,14 +1349,15 @@ public function testMultipleBindingsInSameRequest(): void private function assertThrowsMultipleRecordsFoundException(Closure $makeRequest, int $count): void { - $this->assertThrows($makeRequest, function (Error $exception) use ($count): bool { - $expected = new MultipleRecordsFoundException($count); + try { + $makeRequest(); + } catch (Error $error) { + $this->assertInstanceOf(MultipleRecordsFoundException::class, $error->getPrevious()); + $this->assertEquals(new MultipleRecordsFoundException($count), $error->getPrevious()); - if (! $exception->getPrevious() instanceof $expected) { - return false; - } + return; + } - return $exception->getMessage() === $expected->getMessage(); - }); + $this->fail('Request did not throw an exception.'); } } From 6030bd4689b65a012311a2d88ecdf0a5fd808e50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Sun, 29 Dec 2024 14:29:08 +0100 Subject: [PATCH 18/33] Update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a02a57e8..29b374df0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ You can find and compare releases at the [GitHub release page](https://github.co ## Unreleased +### Added + +- Add `@bind` directive as a GraphQL analogue for Laravel's Route Model Binding https://github.com/nuwave/lighthouse/pull/2645 + ## v6.47.0 ### Added From 17597f98eda7898314555593081bcc3f39ac4757 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Pelhate?= Date: Fri, 3 Jan 2025 10:44:01 +0100 Subject: [PATCH 19/33] review | Remove Laravel version in docs reference Co-authored-by: Benedikt Franke --- docs/master/api-reference/directives.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/master/api-reference/directives.md b/docs/master/api-reference/directives.md index 8ed76da43..8998d03f1 100644 --- a/docs/master/api-reference/directives.md +++ b/docs/master/api-reference/directives.md @@ -497,7 +497,7 @@ directive @bind( ) on ARGUMENT_DEFINITION | INPUT_FIELD_DEFINITION ``` -Resolver model binding (like [Route Model Binding](https://laravel.com/docs/11.x/routing#route-model-binding)) allows to +Resolver model binding (like [Route Model Binding](https://laravel.com/docs/routing#route-model-binding)) allows to inject instances directly into a resolver's arguments instead of the provided scalar identifier, eliminating the need to manually query for the instance inside the resolver. From dd871827c390e5152b22292c993386dd133c17aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 11:00:21 +0100 Subject: [PATCH 20/33] review | Update examples in docs --- docs/master/api-reference/directives.md | 57 ++++++++++++++----------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/docs/master/api-reference/directives.md b/docs/master/api-reference/directives.md index 8998d03f1..ce7abb786 100644 --- a/docs/master/api-reference/directives.md +++ b/docs/master/api-reference/directives.md @@ -513,18 +513,22 @@ type Mutation { ``` ```php -associate($args['company'])->save(); + $user = $args['user']; + $user->associate($args['company']); + + return $user->save(); } } ``` @@ -542,15 +546,13 @@ type Mutation { ``` ```php -definition->required) { + if ($definition->required) { return $this->companyRepository->findOrFail($value); } @@ -577,32 +579,37 @@ type Mutation { addUsersToCompany( users: [ID!]! @bind(class: "App\\Models\\User") company: ID! @bind(class: "App\\Models\\Company") - ): Boolean! + ): [User!]! } ``` ```php -, - * company: \App\Models\Company + * users: \Illuminate\Database\Eloquent\Collection<\App\Models\User>, + * company: \App\Models\Company, * } $args + * @return \Illuminate\Database\Eloquent\Collection<\App\Models\User> */ - public function __invoke(mixed $root, array $args): bool + public function __invoke(mixed $root, array $args): Collection { - foreach ($args['users'] as $user) { - if (! $user->associate($args['company'])->save()) { - return false; - } - } - - return true; + return Collection::make($args['users']) + ->map(function (User $user) use ($args): ?User { + $user->associate($args['company']); + + if (! $user->save()) { + return null; + } + + return $user; + }) + ->filter(); } } ``` From e64fff921bd2c6f903dd0094f7442cc26b25d4dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 11:02:49 +0100 Subject: [PATCH 21/33] review | Remove function imports --- src/Bind/BindDefinition.php | 7 ------- src/Bind/ModelBinding.php | 2 -- src/Bind/Validation/BindingExists.php | 2 -- tests/Integration/Bind/BindDirectiveTest.php | 3 --- 4 files changed, 14 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index fa3477263..a104cfffa 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -10,13 +10,6 @@ use Illuminate\Database\Eloquent\Model; use Nuwave\Lighthouse\Exceptions\DefinitionException; -use function class_exists; -use function implode; -use function in_array; -use function is_subclass_of; -use function method_exists; -use function property_exists; - /** * @template-covariant TClass of object * @property-read class-string $class diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index 4014b18f9..ffc0fc4a0 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -8,8 +8,6 @@ use Illuminate\Support\Arr; use Illuminate\Support\Collection as IlluminateCollection; -use function is_array; - class ModelBinding { /** diff --git a/src/Bind/Validation/BindingExists.php b/src/Bind/Validation/BindingExists.php index 502dbef55..3540a13a6 100644 --- a/src/Bind/Validation/BindingExists.php +++ b/src/Bind/Validation/BindingExists.php @@ -7,8 +7,6 @@ use Illuminate\Validation\Validator; use Nuwave\Lighthouse\Bind\BindDirective; -use function is_array; - class BindingExists implements InvokableRule, ValidatorAwareRule { private ?Validator $validator = null; diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 58f3c66d0..6c8cbcd6d 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -15,9 +15,6 @@ use Tests\Utils\Models\User; use Tests\Utils\Resolvers\SpyResolver; -use function factory; -use function trans; - final class BindDirectiveTest extends DBTestCase { use UsesTestSchema; From baa7b21d979c62bd7c251fb254dad3df77613e0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 11:48:33 +0100 Subject: [PATCH 22/33] review | Adhere to coding standards --- src/Bind/BindDefinition.php | 23 ++++++++++---------- src/Bind/Validation/BindingExists.php | 2 +- tests/Integration/Bind/BindDirectiveTest.php | 6 ++--- tests/Utils/Bind/SpyCallableClassBinding.php | 4 +--- 4 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index a104cfffa..aedfceff9 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -7,6 +7,7 @@ use GraphQL\Language\AST\InputValueDefinitionNode; use GraphQL\Language\AST\NamedTypeNode; use GraphQL\Language\AST\TypeNode; +use GraphQL\Type\Definition\Type; use Illuminate\Database\Eloquent\Model; use Nuwave\Lighthouse\Exceptions\DefinitionException; @@ -19,15 +20,17 @@ */ class BindDefinition { - private const SUPPORTED_VALUE_TYPES = ['ID', 'String', 'Int']; + private const SUPPORTED_VALUE_TYPES = [ + Type::ID, + Type::STRING, + Type::INT, + ]; - /** - * @param class-string $class - * @param array $with - */ public function __construct( + /** @param class-string $class */ public string $class, public string $column, + /** @param array $with */ public array $with, public bool $required, ) {} @@ -42,15 +45,13 @@ public function validate( if (! in_array($valueType, self::SUPPORTED_VALUE_TYPES, true)) { throw new DefinitionException( - "@bind directive defined on `$parentNodeName.$nodeName` does not support value of type `$valueType`. " . - "Expected `" . implode('`, `', self::SUPPORTED_VALUE_TYPES) . '` or a list of one of these types.' + "@bind directive defined on `{$parentNodeName}.{$nodeName}` does not support value of type `{$valueType}`. Expected `" . implode('`, `', self::SUPPORTED_VALUE_TYPES) . '` or a list of one of these types.', ); } if (! class_exists($this->class)) { throw new DefinitionException( - "@bind argument `class` defined on `$parentNodeName.$nodeName` " . - "must be an existing class, received `$this->class`.", + "@bind argument `class` defined on `{$parentNodeName}.{$nodeName}` must be an existing class, received `{$this->class}`.", ); } @@ -62,9 +63,9 @@ public function validate( return; } + $modelClass = Model::class; throw new DefinitionException( - "@bind argument `class` defined on `$parentNodeName.$nodeName` must be " . - "an Eloquent model or a callable class, received `$this->class`.", + "@bind argument `class` defined on `{$parentNodeName}.{$nodeName}` must extend {$modelClass} or define the method `__invoke`, but `{$this->class}` does neither.", ); } diff --git a/src/Bind/Validation/BindingExists.php b/src/Bind/Validation/BindingExists.php index 3540a13a6..2c29607bf 100644 --- a/src/Bind/Validation/BindingExists.php +++ b/src/Bind/Validation/BindingExists.php @@ -43,7 +43,7 @@ public function __invoke($attribute, $value, $fail): void continue; } - $this->validator?->addFailure("$attribute.$key", 'exists'); + $this->validator?->addFailure("{$attribute}.{$key}", 'exists'); } } diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 6c8cbcd6d..66a4b161c 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -37,7 +37,7 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument )); $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' - query { + { user(user: "1") { id } @@ -96,7 +96,7 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument )); $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' - query { + { user(user: "1") { id } @@ -261,7 +261,7 @@ public function testMissingModelBindingOnFieldArgument(): void GRAPHQL; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' - query { + { user(user: "1") { id } diff --git a/tests/Utils/Bind/SpyCallableClassBinding.php b/tests/Utils/Bind/SpyCallableClassBinding.php index 89109b7ca..534dbe4ca 100644 --- a/tests/Utils/Bind/SpyCallableClassBinding.php +++ b/tests/Utils/Bind/SpyCallableClassBinding.php @@ -5,9 +5,7 @@ use Nuwave\Lighthouse\Bind\BindDefinition; use PHPUnit\Framework\Assert; -/** - * @template TReturn - */ +/** @template TReturn */ final class SpyCallableClassBinding { private mixed $value = null; From b6855807b365d0b8b6555b02ab97d42b455a92eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 11:53:03 +0100 Subject: [PATCH 23/33] review | Use placeholder query in BindDirectiveTest --- tests/Integration/Bind/BindDirectiveTest.php | 54 ++++---------------- 1 file changed, 9 insertions(+), 45 deletions(-) diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 66a4b161c..5c5860fb3 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -426,11 +426,7 @@ public function testModelCollectionBindingOnFieldArgument(): void users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") ): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($users: [ID!]!) { @@ -468,11 +464,7 @@ public function testMissingModelCollectionBindingOnFieldArgument(): void users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User") ): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($users: [ID!]!) { @@ -503,11 +495,7 @@ public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void users: [ID!]! @bind(class: "Tests\\Utils\\Models\\User", required: false) ): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($users: [ID!]!) { @@ -546,11 +534,7 @@ public function testModelCollectionBindingWithTooManyResultsOnFieldArgument(): v users: [String!]! @bind(class: "Tests\\Utils\\Models\\User", column: "name") ): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($users: [String!]!) { @@ -823,11 +807,7 @@ public function testModelCollectionBindingOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { @@ -872,11 +852,7 @@ public function testMissingModelCollectionBindingOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { @@ -913,11 +889,7 @@ public function testMissingOptionalModelCollectionBindingOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { @@ -961,11 +933,7 @@ public function testModelCollectionBindingWithTooManyResultsOnInputField(): void type Mutation { removeUsers(input: RemoveUsersInput!): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $makeRequest = fn () => $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($input: RemoveUsersInput!) { @@ -1313,11 +1281,7 @@ public function testMultipleBindingsInSameRequest(): void company: ID! @bind(class: "Tests\\Utils\\Models\\Company") ): Boolean! @mock } - - type Query { - ping: Boolean - } - GRAPHQL; + GRAPHQL . self::PLACEHOLDER_QUERY; $response = $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' mutation ($user: ID!, $company: ID!) { From 67d3d6da00262a3160f5c6998c7113bdef4710d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 12:02:57 +0100 Subject: [PATCH 24/33] review | Remove SpyResolver in favour of inspecting resolver args by reference --- tests/Integration/Bind/BindDirectiveTest.php | 114 ++++++++++--------- tests/Utils/Resolvers/SpyResolver.php | 43 ------- 2 files changed, 60 insertions(+), 97 deletions(-) delete mode 100644 tests/Utils/Resolvers/SpyResolver.php diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 5c5860fb3..f82f0d004 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -13,7 +13,6 @@ use Tests\Utils\Bind\SpyCallableClassBinding; use Tests\Utils\Models\Company; use Tests\Utils\Models\User; -use Tests\Utils\Resolvers\SpyResolver; final class BindDirectiveTest extends DBTestCase { @@ -344,8 +343,11 @@ public function testModelBindingByColumnOnFieldArgument(): void public function testModelBindingWithEagerLoadingOnFieldArgument(): void { $user = factory(User::class)->create(); - $resolver = new SpyResolver(fn (mixed $root, array $args) => $args['user']); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return $args['user']; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -376,10 +378,8 @@ public function testModelBindingWithEagerLoadingOnFieldArgument(): void ], ], ]); - $resolver->assertArgs(function (array $args): void { - $this->assertInstanceOf(User::class, $args['user']); - $this->assertTrue($args['user']->relationLoaded('company')); - }); + $this->assertInstanceOf(User::class, $resolverArgs['user']); + $this->assertTrue($resolverArgs['user']->relationLoaded('company')); } public function testModelBindingWithTooManyResultsOnFieldArgument(): void @@ -414,8 +414,11 @@ public function testModelBindingWithTooManyResultsOnFieldArgument(): void public function testModelCollectionBindingOnFieldArgument(): void { $users = factory(User::class, 2)->create(); - $resolver = new SpyResolver(return: true); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return true; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -442,12 +445,10 @@ public function testModelCollectionBindingOnFieldArgument(): void 'removeUsers' => true, ], ]); - $resolver->assertArgs(function (array $args) use ($users): void { - $this->assertArrayHasKey('users', $args); - $this->assertCount($users->count(), $args['users']); - $users->each(function (User $user) use ($args): void { - $this->assertTrue($user->is($args['users'][$user->getKey()])); - }); + $this->assertArrayHasKey('users', $resolverArgs); + $this->assertCount($users->count(), $resolverArgs['users']); + $users->each(function (User $user) use ($resolverArgs): void { + $this->assertTrue($user->is($resolverArgs['users'][$user->getKey()])); }); } @@ -483,8 +484,11 @@ public function testMissingModelCollectionBindingOnFieldArgument(): void public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void { $user = factory(User::class)->create(); - $resolver = new SpyResolver(return: true); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return true; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -513,11 +517,9 @@ public function testMissingOptionalModelCollectionBindingOnFieldArgument(): void 'removeUsers' => true, ], ]); - $resolver->assertArgs(function (array $args) use ($user): void { - $this->assertArrayHasKey('users', $args); - $this->assertCount(1, $args['users']); - $this->assertTrue($user->is($args['users'][$user->getKey()])); - }); + $this->assertArrayHasKey('users', $resolverArgs); + $this->assertCount(1, $resolverArgs['users']); + $this->assertTrue($user->is($resolverArgs['users'][$user->getKey()])); } public function testModelCollectionBindingWithTooManyResultsOnFieldArgument(): void @@ -711,8 +713,11 @@ public function testModelBindingByColumnOnInputField(): void public function testModelBindingWithEagerLoadingOnInputField(): void { $user = factory(User::class)->create(); - $resolver = new SpyResolver(fn (mixed $root, array $args) => $args['input']['user']); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return $args['input']['user']; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -745,14 +750,12 @@ public function testModelBindingWithEagerLoadingOnInputField(): void $response->assertJson([ 'data' => [ 'user' => [ - 'id' => $user->id, + 'id' => $user->getKey(), ], ], ]); - $resolver->assertArgs(function (array $args): void { - $this->assertInstanceOf(User::class, $args['input']['user']); - $this->assertTrue($args['input']['user']->relationLoaded('company')); - }); + $this->assertInstanceOf(User::class, $resolverArgs['input']['user']); + $this->assertTrue($resolverArgs['input']['user']->relationLoaded('company')); } public function testModelBindingWithTooManyResultsOnInputField(): void @@ -793,8 +796,11 @@ public function testModelBindingWithTooManyResultsOnInputField(): void public function testModelCollectionBindingOnInputField(): void { $users = factory(User::class, 2)->create(); - $resolver = new SpyResolver(return: true); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return true; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -827,13 +833,11 @@ public function testModelCollectionBindingOnInputField(): void 'removeUsers' => true, ], ]); - $resolver->assertArgs(function (array $args) use ($users): void { - $this->assertArrayHasKey('input', $args); - $this->assertArrayHasKey('users', $args['input']); - $this->assertCount($users->count(), $args['input']['users']); - $users->each(function (User $user) use ($args): void { - $this->assertTrue($user->is($args['input']['users'][$user->getKey()])); - }); + $this->assertArrayHasKey('input', $resolverArgs); + $this->assertArrayHasKey('users', $resolverArgs['input']); + $this->assertCount($users->count(), $resolverArgs['input']['users']); + $users->each(function (User $user) use ($resolverArgs): void { + $this->assertTrue($user->is($resolverArgs['input']['users'][$user->getKey()])); }); } @@ -875,8 +879,11 @@ public function testMissingModelCollectionBindingOnInputField(): void public function testMissingOptionalModelCollectionBindingOnInputField(): void { $user = factory(User::class)->create(); - $resolver = new SpyResolver(return: true); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return true; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -909,12 +916,10 @@ public function testMissingOptionalModelCollectionBindingOnInputField(): void 'removeUsers' => true, ], ]); - $resolver->assertArgs(function (array $args) use ($user): void { - $this->assertArrayHasKey('input', $args); - $this->assertArrayHasKey('users', $args['input']); - $this->assertCount(1, $args['input']['users']); - $this->assertTrue($user->is($args['input']['users'][$user->getKey()])); - }); + $this->assertArrayHasKey('input', $resolverArgs); + $this->assertArrayHasKey('users', $resolverArgs['input']); + $this->assertCount(1, $resolverArgs['input']['users']); + $this->assertTrue($user->is($resolverArgs['input']['users'][$user->getKey()])); } public function testModelCollectionBindingWithTooManyResultsOnInputField(): void @@ -1264,8 +1269,11 @@ public function testMultipleBindingsInSameRequest(): void { $user = factory(User::class)->create(); $company = factory(Company::class)->create(); - $resolver = new SpyResolver(return: true); - $this->mockResolver($resolver); + $resolverArgs = []; + $this->mockResolver(function ($_, array $args) use (&$resolverArgs) { + $resolverArgs = $args; + return true; + }); $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' type User { id: ID! @@ -1300,12 +1308,10 @@ public function testMultipleBindingsInSameRequest(): void 'addUserToCompany' => true, ], ]); - $resolver->assertArgs(function (array $args) use ($user, $company): void { - $this->assertArrayHasKey('user', $args); - $this->assertTrue($user->is($args['user'])); - $this->assertArrayHasKey('company', $args); - $this->assertTrue($company->is($args['company'])); - }); + $this->assertArrayHasKey('user', $resolverArgs); + $this->assertTrue($user->is($resolverArgs['user'])); + $this->assertArrayHasKey('company', $resolverArgs); + $this->assertTrue($company->is($resolverArgs['company'])); } private function assertThrowsMultipleRecordsFoundException(Closure $makeRequest, int $count): void diff --git a/tests/Utils/Resolvers/SpyResolver.php b/tests/Utils/Resolvers/SpyResolver.php deleted file mode 100644 index c973ad9e1..000000000 --- a/tests/Utils/Resolvers/SpyResolver.php +++ /dev/null @@ -1,43 +0,0 @@ - - */ - private array $args = []; - - /** - * @param TReturn $return - */ - public function __construct( - private mixed $return = null, - ) {} - - /** - * @param array $args - * @return TReturn - */ - public function __invoke(mixed $root, array $args): mixed - { - $this->args = $args; - - if (is_callable($this->return)) { - return ($this->return)($root, $args); - } - - return $this->return; - } - - public function assertArgs(callable $expect): void - { - $expect($this->args); - } -} From 2ef000c8e31e033ffbb09682fbdf5ae27553f94a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 12:07:29 +0100 Subject: [PATCH 25/33] Fix BindDefinition constructor docblock --- src/Bind/BindDefinition.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index aedfceff9..e1a1a9179 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -27,10 +27,10 @@ class BindDefinition ]; public function __construct( - /** @param class-string $class */ + /** @var class-string */ public string $class, public string $column, - /** @param array $with */ + /** @var array */ public array $with, public bool $required, ) {} From c543ee99e790701784e4dc76088fbc8f7239c428 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 12:08:53 +0100 Subject: [PATCH 26/33] Fix BindDirective schema validation tests --- tests/Integration/Bind/BindDirectiveTest.php | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index f82f0d004..729991578 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -90,8 +90,7 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnFieldArgument GRAPHQL; $this->expectExceptionObject(new DefinitionException( - '@bind argument `class` defined on `user.user` must be an ' . - 'Eloquent model or a callable class, received `stdClass`.' + '@bind argument `class` defined on `user.user` must extend Illuminate\Database\Eloquent\Model or define the method `__invoke`, but `stdClass` does neither.' )); $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' @@ -120,8 +119,7 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN GRAPHQL; $this->expectExceptionObject(new DefinitionException( - '@bind argument `class` defined on `RemoveUsersInput.users` must be an ' . - 'Eloquent model or a callable class, received `stdClass`.' + '@bind argument `class` defined on `RemoveUsersInput.users` must extend Illuminate\Database\Eloquent\Model or define the method `__invoke`, but `stdClass` does neither.' )); $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' From b52dc0780308fa06453130537516ef63c97f173d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 12:10:34 +0100 Subject: [PATCH 27/33] review | Reuse existing underlying type name resolution --- src/Bind/BindDefinition.php | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index e1a1a9179..dfdc20cf2 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -5,11 +5,10 @@ use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\InputObjectTypeDefinitionNode; use GraphQL\Language\AST\InputValueDefinitionNode; -use GraphQL\Language\AST\NamedTypeNode; -use GraphQL\Language\AST\TypeNode; use GraphQL\Type\Definition\Type; use Illuminate\Database\Eloquent\Model; use Nuwave\Lighthouse\Exceptions\DefinitionException; +use Nuwave\Lighthouse\Schema\AST\ASTHelper; /** * @template-covariant TClass of object @@ -41,11 +40,11 @@ public function validate( ): void { $nodeName = $definitionNode->name->value; $parentNodeName = $parentNode->name->value; - $valueType = $this->valueType($definitionNode->type); + $typeName = ASTHelper::getUnderlyingTypeName($definitionNode); - if (! in_array($valueType, self::SUPPORTED_VALUE_TYPES, true)) { + if (! in_array($typeName, self::SUPPORTED_VALUE_TYPES, true)) { throw new DefinitionException( - "@bind directive defined on `{$parentNodeName}.{$nodeName}` does not support value of type `{$valueType}`. Expected `" . implode('`, `', self::SUPPORTED_VALUE_TYPES) . '` or a list of one of these types.', + "@bind directive defined on `{$parentNodeName}.{$nodeName}` does not support value of type `{$typeName}`. Expected `" . implode('`, `', self::SUPPORTED_VALUE_TYPES) . '` or a list of one of these types.', ); } @@ -69,19 +68,6 @@ public function validate( ); } - private function valueType(TypeNode $typeNode): string - { - if (property_exists($typeNode, 'type')) { - return $this->valueType($typeNode->type); - } - - if ($typeNode instanceof NamedTypeNode) { - return $typeNode->name->value; - } - - return ''; - } - public function isModelBinding(): bool { return is_subclass_of($this->class, Model::class); From 87c0a72a911391d0e83f9585896dd82049220fda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 14:54:38 +0100 Subject: [PATCH 28/33] =?UTF-8?q?review=20|=20Don=E2=80=99t=20validate=20v?= =?UTF-8?q?alue=20types=20on=20which=20`@bind`=20is=20defined?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/Bind/BindDefinition.php | 14 ---- tests/Integration/Bind/BindDirectiveTest.php | 76 -------------------- 2 files changed, 90 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index dfdc20cf2..c69a4d1c9 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -8,7 +8,6 @@ use GraphQL\Type\Definition\Type; use Illuminate\Database\Eloquent\Model; use Nuwave\Lighthouse\Exceptions\DefinitionException; -use Nuwave\Lighthouse\Schema\AST\ASTHelper; /** * @template-covariant TClass of object @@ -19,12 +18,6 @@ */ class BindDefinition { - private const SUPPORTED_VALUE_TYPES = [ - Type::ID, - Type::STRING, - Type::INT, - ]; - public function __construct( /** @var class-string */ public string $class, @@ -40,13 +33,6 @@ public function validate( ): void { $nodeName = $definitionNode->name->value; $parentNodeName = $parentNode->name->value; - $typeName = ASTHelper::getUnderlyingTypeName($definitionNode); - - if (! in_array($typeName, self::SUPPORTED_VALUE_TYPES, true)) { - throw new DefinitionException( - "@bind directive defined on `{$parentNodeName}.{$nodeName}` does not support value of type `{$typeName}`. Expected `" . implode('`, `', self::SUPPORTED_VALUE_TYPES) . '` or a list of one of these types.', - ); - } if (! class_exists($this->class)) { throw new DefinitionException( diff --git a/tests/Integration/Bind/BindDirectiveTest.php b/tests/Integration/Bind/BindDirectiveTest.php index 729991578..836177afa 100644 --- a/tests/Integration/Bind/BindDirectiveTest.php +++ b/tests/Integration/Bind/BindDirectiveTest.php @@ -135,82 +135,6 @@ public function testSchemaValidationFailsWhenClassArgumentDefinedOnInputFieldIsN ); } - public function testSchemaValidationFailsWhenValueTypeDefinedOnFieldArgumentIsNotSupported(): void - { - $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' - type User { - id: ID! - type: UserType! - } - - enum UserType { - ADMINISTRATOR - MODERATOR - } - - type Query { - usersByType(type: UserType! @bind(class: "Tests\\Utils\\Models\\User")): User! @mock - } - GRAPHQL; - - $this->expectExceptionObject(new DefinitionException( - '@bind directive defined on `usersByType.type` does not support value of type `UserType`. ' . - 'Expected `ID`, `String`, `Int` or a list of one of these types.' - )); - - $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' - query ($type: UserType!) { - usersByType(type: $type) { - id - } - } - GRAPHQL, - ['type' => 'ADMINISTRATOR'], - ); - } - - public function testSchemaValidationFailsWhenValueTypeDefinedOnInputFieldIsNotSupported(): void - { - $this->schema = /* @lang GraphQL */ <<<'GRAPHQL' - type User { - id: ID! - type: UserType! - } - - enum UserType { - ADMINISTRATOR - MODERATOR - } - - input UsersByTypeInput { - type: UserType! @bind(class: "Tests\\Utils\\Models\\User") - } - - type Query { - usersByType(input: UsersByTypeInput!): User! @mock - } - GRAPHQL; - - $this->expectExceptionObject(new DefinitionException( - '@bind directive defined on `UsersByTypeInput.type` does not support value of type `UserType`. ' . - 'Expected `ID`, `String`, `Int` or a list of one of these types.' - )); - - $this->graphQL(/* @lang GraphQL */ <<<'GRAPHQL' - query ($input: UsersByTypeInput!) { - usersByType(input: $input) { - id - } - } - GRAPHQL, - [ - 'input' => [ - 'type' => 'ADMINISTRATOR', - ], - ], - ); - } - public function testModelBindingOnFieldArgument(): void { $user = factory(User::class)->create(); From 10a7580faa9d75f6745b747d27f2d0b0382223b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 15:15:54 +0100 Subject: [PATCH 29/33] =?UTF-8?q?review=20|=20Add=20comment=20for=20contex?= =?UTF-8?q?t=20on=20handling=20=E2=80=9Ctoo=20few=E2=80=9D=20vs=20?= =?UTF-8?q?=E2=80=9Ctoo=20many=E2=80=9D=20results=20in=20ModelBinding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/Bind/ModelBinding.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index ffc0fc4a0..549eafe1c 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -34,6 +34,10 @@ public function __invoke(int|string|array $value, BindDefinition $definition): M */ private function modelInstance(EloquentCollection $results): ?Model { + // While "too few records" errors are handled as (client-safe) validation errors by applying + // the `BindingExists` rule on the BindDirective depending on whether the binding is required, + // "too many records" should be considered as (non-client-safe) configuration errors as it + // means the binding was not resolved using a unique identifier. if ($results->count() > 1) { throw new MultipleRecordsFoundException($results->count()); } @@ -43,7 +47,7 @@ private function modelInstance(EloquentCollection $results): ?Model /** * Binding collections should be returned with the original values - * as keys to allow us to validate the binding when non-optional. + * as keys to allow validating the binding when required. * @see \Nuwave\Lighthouse\Bind\BindDirective::rules() * * @param \Illuminate\Database\Eloquent\Collection $results @@ -56,10 +60,11 @@ private function modelCollection( IlluminateCollection $values, BindDefinition $definition, ): EloquentCollection { + /* @see self::modelInstance() */ if ($results->count() > $values->unique()->count()) { throw new MultipleRecordsFoundException($results->count()); } - return $results->keyby($definition->column); + return $results->keyBy($definition->column); } } From 28cdfb397fab3f28fc60dbb48a30371deb936789 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 15:17:33 +0100 Subject: [PATCH 30/33] review | Update examples in directives docs --- docs/master/api-reference/directives.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/docs/master/api-reference/directives.md b/docs/master/api-reference/directives.md index ce7abb786..9a6db440c 100644 --- a/docs/master/api-reference/directives.md +++ b/docs/master/api-reference/directives.md @@ -599,15 +599,13 @@ final class AddUsersToCompany */ public function __invoke(mixed $root, array $args): Collection { - return Collection::make($args['users']) + return $args['users'] ->map(function (User $user) use ($args): ?User { $user->associate($args['company']); - if (! $user->save()) { - return null; - } - - return $user; + return $user->save() + ? $user + : null; }) ->filter(); } From 541e796c51cd2e0e6976b50b8860764c7ee4d29b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 15:19:02 +0100 Subject: [PATCH 31/33] review | Remove redundant docblock from BindDefinition --- src/Bind/BindDefinition.php | 9 +-------- src/Bind/ModelBinding.php | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/src/Bind/BindDefinition.php b/src/Bind/BindDefinition.php index c69a4d1c9..ebf307508 100644 --- a/src/Bind/BindDefinition.php +++ b/src/Bind/BindDefinition.php @@ -5,17 +5,10 @@ use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Language\AST\InputObjectTypeDefinitionNode; use GraphQL\Language\AST\InputValueDefinitionNode; -use GraphQL\Type\Definition\Type; use Illuminate\Database\Eloquent\Model; use Nuwave\Lighthouse\Exceptions\DefinitionException; -/** - * @template-covariant TClass of object - * @property-read class-string $class - * @property-read string $column - * @property-read array $with - * @property-read bool $optional - */ +/** @template-covariant TClass of object */ class BindDefinition { public function __construct( diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index 549eafe1c..a5d9d57a1 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -60,7 +60,7 @@ private function modelCollection( IlluminateCollection $values, BindDefinition $definition, ): EloquentCollection { - /* @see self::modelInstance() */ + /** @see self::modelInstance() */ if ($results->count() > $values->unique()->count()) { throw new MultipleRecordsFoundException($results->count()); } From c6df8c8777c61f267aa42138760c9dc82e2d78f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 15:23:50 +0100 Subject: [PATCH 32/33] review | Cleanup internals in accordance to coding standard --- src/Bind/BindDirective.php | 31 ++++++++------------ tests/Utils/Bind/SpyCallableClassBinding.php | 12 ++------ 2 files changed, 16 insertions(+), 27 deletions(-) diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index c460f7a20..c02409be3 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -19,10 +19,9 @@ class BindDirective extends BaseDirective implements ArgumentValidation, ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator { - /** - * @var \Nuwave\Lighthouse\Bind\BindDefinition|null - */ + /** @var \Nuwave\Lighthouse\Bind\BindDefinition|null */ private ?BindDefinition $definition = null; + private mixed $binding; public function __construct( @@ -69,16 +68,14 @@ class: String! GRAPHQL; } - /** - * @return \Nuwave\Lighthouse\Bind\BindDefinition - */ + /** @return \Nuwave\Lighthouse\Bind\BindDefinition */ private function bindDefinition(): BindDefinition { return $this->definition ??= new BindDefinition( - $this->directiveArgValue('class'), - $this->directiveArgValue('column', 'id'), - $this->directiveArgValue('with', []), - $this->directiveArgValue('required', true), + class: $this->directiveArgValue('class'), + column: $this->directiveArgValue('column', 'id'), + with: $this->directiveArgValue('with', []), + required: $this->directiveArgValue('required', true), ); } @@ -101,10 +98,9 @@ public function manipulateInputFieldDefinition( public function rules(): array { - return match ($this->bindDefinition()->required) { - true => [new BindingExists($this)], - false => [], - }; + return $this->bindDefinition()->required + ? [new BindingExists($this)] + : []; } public function messages(): array @@ -128,10 +124,9 @@ public function transform(mixed $argumentValue): mixed $definition = $this->bindDefinition(); - $bind = match ($definition->isModelBinding()) { - true => $this->container->make(ModelBinding::class), - false => $this->container->make($definition->class), - }; + $bind = $definition->isModelBinding() + ? $this->container->make(ModelBinding::class) + : $this->container->make($definition->class); return $this->binding = $bind($argumentValue, $definition); } diff --git a/tests/Utils/Bind/SpyCallableClassBinding.php b/tests/Utils/Bind/SpyCallableClassBinding.php index 534dbe4ca..978275c1e 100644 --- a/tests/Utils/Bind/SpyCallableClassBinding.php +++ b/tests/Utils/Bind/SpyCallableClassBinding.php @@ -10,15 +10,11 @@ final class SpyCallableClassBinding { private mixed $value = null; - /** - * @var \Nuwave\Lighthouse\Bind\BindDefinition|null - */ + /** @var \Nuwave\Lighthouse\Bind\BindDefinition|null */ private ?BindDefinition $definition = null; - /** - * @param TReturn $return - */ public function __construct( + /** @var TReturn */ private mixed $return = null, ) {} @@ -34,9 +30,7 @@ public function __invoke(mixed $value, BindDefinition $definition): mixed return $this->return; } - /** - * @param \Nuwave\Lighthouse\Bind\BindDefinition $definition - */ + /** @param \Nuwave\Lighthouse\Bind\BindDefinition $definition */ public function assertCalledWith(mixed $value, BindDefinition $definition): void { Assert::assertSame($value, $this->value); From fd530f605f873372e40999d1658f13636716d00c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Re=CC=81mi=20Pelhate?= Date: Fri, 3 Jan 2025 15:27:12 +0100 Subject: [PATCH 33/33] review | Use protected methods and properties instead of private --- src/Bind/BindDirective.php | 8 ++++---- src/Bind/ModelBinding.php | 8 +++----- src/Bind/Validation/BindingExists.php | 4 ++-- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/Bind/BindDirective.php b/src/Bind/BindDirective.php index c02409be3..dd521415a 100644 --- a/src/Bind/BindDirective.php +++ b/src/Bind/BindDirective.php @@ -20,12 +20,12 @@ class BindDirective extends BaseDirective implements ArgumentValidation, ArgTransformerDirective, ArgDirectiveForArray, ArgManipulator, InputFieldManipulator { /** @var \Nuwave\Lighthouse\Bind\BindDefinition|null */ - private ?BindDefinition $definition = null; + protected ?BindDefinition $definition = null; - private mixed $binding; + protected mixed $binding; public function __construct( - private Container $container, + protected Container $container, ) { $this->binding = new PendingBinding(); } @@ -69,7 +69,7 @@ class: String! } /** @return \Nuwave\Lighthouse\Bind\BindDefinition */ - private function bindDefinition(): BindDefinition + protected function bindDefinition(): BindDefinition { return $this->definition ??= new BindDefinition( class: $this->directiveArgValue('class'), diff --git a/src/Bind/ModelBinding.php b/src/Bind/ModelBinding.php index a5d9d57a1..3d3b23d93 100644 --- a/src/Bind/ModelBinding.php +++ b/src/Bind/ModelBinding.php @@ -29,10 +29,8 @@ public function __invoke(int|string|array $value, BindDefinition $definition): M return $this->modelInstance($binding); } - /** - * @param \Illuminate\Database\Eloquent\Collection $results - */ - private function modelInstance(EloquentCollection $results): ?Model + /** @param \Illuminate\Database\Eloquent\Collection $results */ + protected function modelInstance(EloquentCollection $results): ?Model { // While "too few records" errors are handled as (client-safe) validation errors by applying // the `BindingExists` rule on the BindDirective depending on whether the binding is required, @@ -55,7 +53,7 @@ private function modelInstance(EloquentCollection $results): ?Model * @param \Nuwave\Lighthouse\Bind\BindDefinition<\Illuminate\Database\Eloquent\Model> $definition * @return \Illuminate\Database\Eloquent\Collection */ - private function modelCollection( + protected function modelCollection( EloquentCollection $results, IlluminateCollection $values, BindDefinition $definition, diff --git a/src/Bind/Validation/BindingExists.php b/src/Bind/Validation/BindingExists.php index 2c29607bf..98ea0f46f 100644 --- a/src/Bind/Validation/BindingExists.php +++ b/src/Bind/Validation/BindingExists.php @@ -9,10 +9,10 @@ class BindingExists implements InvokableRule, ValidatorAwareRule { - private ?Validator $validator = null; + protected ?Validator $validator = null; public function __construct( - private BindDirective $directive, + protected BindDirective $directive, ) {} /**