From ee8425167338a8989fa1a30a8d4e36ce053e4aec Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Sat, 21 Sep 2024 03:12:42 +0300 Subject: [PATCH 1/7] Fix bug with prefetching on different nesting levels Allow Promise to be returned from prefetchMethod with returnRequested: true --- src/Annotations/Prefetch.php | 10 ++- .../PrefetchParameterMiddleware.php | 1 + src/Parameters/PrefetchDataParameter.php | 34 ++++++-- src/PrefetchBuffer.php | 29 +++---- src/QueryField.php | 38 ++++---- .../Controllers/BlogController.php | 21 +++++ tests/Fixtures/Integration/Models/Blog.php | 86 +++++++++++++++++++ tests/Fixtures/Integration/Models/Comment.php | 23 +++++ .../Integration/Types/CompanyType.php | 36 +++++++- tests/Fixtures/Integration/Types/PostType.php | 34 ++++++++ .../Parameters/PrefetchDataParameterTest.php | 9 +- tests/SchemaFactoryTest.php | 2 + website/docs/prefetch-method.mdx | 66 +++++++++++++- 13 files changed, 335 insertions(+), 54 deletions(-) create mode 100644 tests/Fixtures/Integration/Controllers/BlogController.php create mode 100644 tests/Fixtures/Integration/Models/Blog.php create mode 100644 tests/Fixtures/Integration/Models/Comment.php diff --git a/src/Annotations/Prefetch.php b/src/Annotations/Prefetch.php index f4fa3bbeb7..5fbc2d0fac 100644 --- a/src/Annotations/Prefetch.php +++ b/src/Annotations/Prefetch.php @@ -10,8 +10,14 @@ #[Attribute(Attribute::TARGET_PARAMETER)] class Prefetch implements ParameterAnnotationInterface { - /** @param string|(callable&array{class-string, string}) $callable */ - public function __construct(public readonly string|array $callable) + /** + * @param string|(callable&array{class-string, string}) $callable + * @param bool $returnRequested return value mapped to requested method + */ + public function __construct( + public readonly string|array $callable, + public readonly bool $returnRequested = false, + ) { } diff --git a/src/Mappers/Parameters/PrefetchParameterMiddleware.php b/src/Mappers/Parameters/PrefetchParameterMiddleware.php index bb5fef4f70..b5fb601082 100644 --- a/src/Mappers/Parameters/PrefetchParameterMiddleware.php +++ b/src/Mappers/Parameters/PrefetchParameterMiddleware.php @@ -52,6 +52,7 @@ public function mapParameter(ReflectionParameter $parameter, DocBlock $docBlock, fieldName: $method->getName(), resolver: $resolver, parameters: $parameters, + returnRequested: $prefetch->returnRequested, ); } } diff --git a/src/Parameters/PrefetchDataParameter.php b/src/Parameters/PrefetchDataParameter.php index 4b233c5e3c..3b527c7420 100644 --- a/src/Parameters/PrefetchDataParameter.php +++ b/src/Parameters/PrefetchDataParameter.php @@ -12,6 +12,7 @@ use TheCodingMachine\GraphQLite\PrefetchBuffer; use TheCodingMachine\GraphQLite\QueryField; +use function array_key_exists; use function assert; /** @@ -27,6 +28,7 @@ public function __construct( private readonly string $fieldName, private readonly mixed $resolver, public readonly array $parameters, + public readonly bool $returnRequested = false, ) { } @@ -52,24 +54,40 @@ public function resolve(object|null $source, array $args, mixed $context, Resolv // So we record all of these ->resolve() calls, collect them together and when a value is actually // needed, GraphQL calls the callback of Deferred below. That's when we call the prefetch method, // already knowing all the requested fields (source-arguments combinations). - return new Deferred(function () use ($info, $context, $args, $prefetchBuffer) { - if (! $prefetchBuffer->hasResult($args, $info)) { - $prefetchResult = $this->computePrefetch($args, $context, $info, $prefetchBuffer); - - $prefetchBuffer->storeResult($prefetchResult, $args, $info); + return new Deferred(function () use ($source, $info, $context, $args, $prefetchBuffer) { + if (! $prefetchBuffer->hasResult($source)) { + $this->computePrefetch($args, $context, $info, $prefetchBuffer); } - return $prefetchResult ?? $prefetchBuffer->getResult($args, $info); + return $prefetchBuffer->getResult($source); }); } /** @param array $args */ - private function computePrefetch(array $args, mixed $context, ResolveInfo $info, PrefetchBuffer $prefetchBuffer): mixed + private function computePrefetch(array $args, mixed $context, ResolveInfo $info, PrefetchBuffer $prefetchBuffer): void { $sources = $prefetchBuffer->getObjectsByArguments($args, $info); + $prefetchBuffer->purge($args, $info); $toPassPrefetchArgs = QueryField::paramsToArguments($this->fieldName, $this->parameters, null, $args, $context, $info, $this->resolver); - return ($this->resolver)($sources, ...$toPassPrefetchArgs); + $resolvedValues = ($this->resolver)($sources, ...$toPassPrefetchArgs); + if ($this->returnRequested) { + foreach ($resolvedValues as $key => $resolvedValue) { + if (! array_key_exists($key, $sources)) { + throw new GraphQLRuntimeException( + 'Called by Prefetch function should accept ' . + 'Array and return Array, but the function did ' . + 'not return an Array of the same length as the Array of keys.', + ); + } + $prefetchBuffer->storeResult($sources[$key], $resolvedValue); + } + } else { + foreach ($sources as $source) { + // map results to each source to support old prefetch behavior + $prefetchBuffer->storeResult($source, $resolvedValues); + } + } } /** @inheritDoc */ diff --git a/src/PrefetchBuffer.php b/src/PrefetchBuffer.php index 8b1afb02a0..5be2c4ee00 100644 --- a/src/PrefetchBuffer.php +++ b/src/PrefetchBuffer.php @@ -5,8 +5,8 @@ namespace TheCodingMachine\GraphQLite; use GraphQL\Type\Definition\ResolveInfo; +use SplObjectStorage; -use function array_key_exists; use function md5; use function serialize; @@ -18,8 +18,13 @@ class PrefetchBuffer /** @var array> An array of buffered, indexed by hash of arguments. */ private array $objects = []; - /** @var array An array of prefetch method results, indexed by hash of arguments. */ - private array $results = []; + /** @var SplObjectStorage A Storage of prefetch method results, holds source to resolved values. */ + private SplObjectStorage $results; + + public function __construct() + { + $this->results = new SplObjectStorage(); + } /** @param array $arguments The input arguments passed from GraphQL to the field. */ public function register( @@ -66,28 +71,22 @@ public function purge( unset($this->objects[$this->computeHash($arguments, $info)]); } - /** @param array $arguments The input arguments passed from GraphQL to the field. */ public function storeResult( + object $source, mixed $result, - array $arguments, - ResolveInfo|null $info = null, ): void { - $this->results[$this->computeHash($arguments, $info)] = $result; + $this->results->offsetSet($source, $result); } - /** @param array $arguments The input arguments passed from GraphQL to the field. */ public function hasResult( - array $arguments, - ResolveInfo|null $info = null, + object $source, ): bool { - return array_key_exists($this->computeHash($arguments, $info), $this->results); + return $this->results->offsetExists($source); } - /** @param array $arguments The input arguments passed from GraphQL to the field. */ public function getResult( - array $arguments, - ResolveInfo|null $info = null, + object $source, ): mixed { - return $this->results[$this->computeHash($arguments, $info)]; + return $this->results->offsetGet($source); } } diff --git a/src/QueryField.php b/src/QueryField.php index b95a9152f4..a02665899b 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -4,11 +4,9 @@ namespace TheCodingMachine\GraphQLite; -use GraphQL\Deferred; +use Closure; use GraphQL\Error\ClientAware; use GraphQL\Executor\Promise\Adapter\SyncPromise; -use GraphQL\Executor\Promise\Adapter\SyncPromiseAdapter; -use GraphQL\Executor\Promise\Promise; use GraphQL\Language\AST\FieldDefinitionNode; use GraphQL\Type\Definition\FieldDefinition; use GraphQL\Type\Definition\ListOfType; @@ -22,9 +20,6 @@ use TheCodingMachine\GraphQLite\Parameters\ParameterInterface; use TheCodingMachine\GraphQLite\Parameters\SourceParameter; -use function array_filter; -use function array_map; - /** * A GraphQL field that maps to a PHP method automatically. * @@ -90,24 +85,12 @@ public function __construct( return $result; }; - $deferred = (bool) array_filter($toPassArgs, static fn (mixed $value) => $value instanceof SyncPromise); - // GraphQL allows deferring resolving the field's value using promises, i.e. they call the resolve // function ahead of time for all of the fields (allowing us to gather all calls and do something // in batch, like prefetch) and then resolve the promises as needed. To support that for prefetch, // we're checking if any of the resolved parameters returned a promise. If they did, we know // that the value should also be resolved using a promise, so we're wrapping it in one. - return $deferred ? new Deferred(static function () use ($toPassArgs, $callResolver) { - $syncPromiseAdapter = new SyncPromiseAdapter(); - - // Wait for every deferred parameter. - $toPassArgs = array_map( - static fn (mixed $value) => $value instanceof SyncPromise ? $syncPromiseAdapter->wait(new Promise($value, $syncPromiseAdapter)) : $value, - $toPassArgs, - ); - - return $callResolver(...$toPassArgs); - }) : $callResolver(...$toPassArgs); + return $this->deferred($toPassArgs, $callResolver); }; $config += $additionalConfig; @@ -204,4 +187,21 @@ public static function paramsToArguments(string $name, array $parameters, object return $toPassArgs; } + + /** + * @param array $toPassArgs + * Create Deferred if any of arguments contain promise + */ + public static function deferred(array $toPassArgs, Closure $callResolver): mixed + { + foreach ($toPassArgs as $position => $toPassArg) { + if ($toPassArg instanceof SyncPromise) { + return $toPassArg->then(static function ($resolvedValue) use ($toPassArgs, $position, $callResolver) { + $toPassArgs[$position] = $resolvedValue; + return self::deferred($toPassArgs, $callResolver); + }); + } + } + return $callResolver(...$toPassArgs); + } } diff --git a/tests/Fixtures/Integration/Controllers/BlogController.php b/tests/Fixtures/Integration/Controllers/BlogController.php new file mode 100644 index 0000000000..1f358aa69b --- /dev/null +++ b/tests/Fixtures/Integration/Controllers/BlogController.php @@ -0,0 +1,21 @@ +id; + } + + /** + * @param Post[] $prefetchedPosts + * + * @return Post[] + */ + #[Field] + public function getPosts( + #[Prefetch('prefetchPosts', true)] + array $prefetchedPosts, + ): array { + return $prefetchedPosts; + } + + /** + * @param Blog[] $prefetchedSubBlogs + * + * @return Blog[] + */ + #[Field] + public function getSubBlogs( + #[Prefetch('prefetchSubBlogs', true)] + array $prefetchedSubBlogs, + ): array { + return $prefetchedSubBlogs; + } + + /** + * @param Blog[] $blogs + * + * @return Post[][] + */ + public static function prefetchPosts(iterable $blogs): array + { + $posts = []; + foreach ($blogs as $key => $blog) { + $blogId = $blog->getId(); + $posts[$key] = [ + new Post('post-' . $blogId . '.1'), + new Post('post-' . $blogId . '.2'), + ]; + } + + return $posts; + } + + /** + * @param Blog[] $blogs + * + * @return Blog[][] + */ + public static function prefetchSubBlogs(iterable $blogs): array + { + $subBlogs = []; + foreach ($blogs as $key => $blog) { + $blogId = $blog->getId(); + $subBlogId = $blogId * 10; + $subBlogs[$key] = [new Blog($subBlogId)]; + } + + return $subBlogs; + } +} diff --git a/tests/Fixtures/Integration/Models/Comment.php b/tests/Fixtures/Integration/Models/Comment.php new file mode 100644 index 0000000000..094e6e571c --- /dev/null +++ b/tests/Fixtures/Integration/Models/Comment.php @@ -0,0 +1,23 @@ +text; + } +} diff --git a/tests/Fixtures/Integration/Types/CompanyType.php b/tests/Fixtures/Integration/Types/CompanyType.php index 31a042479f..3d825cea8e 100644 --- a/tests/Fixtures/Integration/Types/CompanyType.php +++ b/tests/Fixtures/Integration/Types/CompanyType.php @@ -13,22 +13,27 @@ #[ExtendType(class:Company::class)] class CompanyType { - #[Field] public function getName(Company $company): string { return $company->name; } + /** @param Contact[] $contacts */ #[Field] public function getContact( Company $company, #[Prefetch('prefetchContacts')] - array $contacts - ): ?Contact { + array $contacts, + ): Contact|null { return $contacts[$company->name] ?? null; } + /** + * @param Company[] $companies + * + * @return Contact[] + */ public static function prefetchContacts(array $companies): array { $contacts = []; @@ -39,4 +44,29 @@ public static function prefetchContacts(array $companies): array return $contacts; } + + #[Field] + public function getContactRequested( + Company $company, + #[Prefetch('prefetchContactsExact', true)] + Contact $contact, + ): Contact|null { + return $contact; + } + + /** + * @param Company[] $companies + * + * @return Contact[] + */ + public static function prefetchContactsExact(array $companies): array + { + $contacts = []; + + foreach ($companies as $key => $company) { + $contacts[$key] = new Contact('Kate'); + } + + return $contacts; + } } diff --git a/tests/Fixtures/Integration/Types/PostType.php b/tests/Fixtures/Integration/Types/PostType.php index 565c783ecb..ca686c198d 100644 --- a/tests/Fixtures/Integration/Types/PostType.php +++ b/tests/Fixtures/Integration/Types/PostType.php @@ -4,8 +4,13 @@ namespace TheCodingMachine\GraphQLite\Fixtures\Integration\Types; +use Exception; +use GraphQL\Executor\Promise\Adapter\SyncPromiseAdapter; +use GraphQL\Executor\Promise\Promise; use TheCodingMachine\GraphQLite\Annotations\ExtendType; use TheCodingMachine\GraphQLite\Annotations\Field; +use TheCodingMachine\GraphQLite\Annotations\Prefetch; +use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Comment; use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Post; #[ExtendType(class:Post::class)] @@ -22,4 +27,33 @@ public function getTitle(Post $post): string { return $post->title; } + + /** + * @param Comment[] $prefetchedComments + * + * @return Comment[] + */ + #[Field] + public function getComments( + Post $post, + #[Prefetch('prefetchComments', true)] + array $prefetchedComments, + ): array { + return $prefetchedComments; + } + + /** + * @param Post[] $posts + * + * @return Promise[] + * + * @throws Exception + */ + public static function prefetchComments(array $posts): iterable + { + $syncPromiseAdapter = new SyncPromiseAdapter(); + foreach ($posts as $key => $post) { + yield $key => $syncPromiseAdapter->createFulfilled([new Comment('comment for ' . $post->title)]); + } + } } diff --git a/tests/Parameters/PrefetchDataParameterTest.php b/tests/Parameters/PrefetchDataParameterTest.php index 9d241b7ccc..132a823e35 100644 --- a/tests/Parameters/PrefetchDataParameterTest.php +++ b/tests/Parameters/PrefetchDataParameterTest.php @@ -2,7 +2,6 @@ namespace TheCodingMachine\GraphQLite\Parameters; -use Generator; use GraphQL\Deferred; use GraphQL\Executor\Promise\Adapter\SyncPromiseAdapter; use GraphQL\Executor\Promise\Promise; @@ -11,8 +10,6 @@ use PHPUnit\Framework\TestCase; use stdClass; use TheCodingMachine\GraphQLite\Context\Context; -use TheCodingMachine\GraphQLite\Middlewares\MissingAuthorizationException; -use TheCodingMachine\GraphQLite\Security\AuthenticationServiceInterface; use TheCodingMachine\GraphQLite\Types\ArgumentResolver; class PrefetchDataParameterTest extends TestCase @@ -32,7 +29,7 @@ public function testResolveWithExistingResult(): void ]; $buffer = $context->getPrefetchBuffer($parameter); - $buffer->storeResult($prefetchResult, $args); + $buffer->storeResult($source, $prefetchResult); $resolvedParameterPromise = $parameter->resolve($source, $args, $context, $this->createStub(ResolveInfo::class)); @@ -71,10 +68,10 @@ public function testResolveWithoutExistingResult(): void $resolvedParameterPromise = $parameter->resolve($source, $args, $context, $this->createStub(ResolveInfo::class)); - self::assertFalse($buffer->hasResult($args)); + self::assertFalse($buffer->hasResult($source)); self::assertSame([$source], $buffer->getObjectsByArguments($args)); self::assertSame($prefetchResult, $this->deferredValue($resolvedParameterPromise)); - self::assertTrue($buffer->hasResult($args)); + self::assertTrue($buffer->hasResult($source)); } private function deferredValue(Deferred $promise): mixed diff --git a/tests/SchemaFactoryTest.php b/tests/SchemaFactoryTest.php index 78defd5b78..03678fa1f9 100644 --- a/tests/SchemaFactoryTest.php +++ b/tests/SchemaFactoryTest.php @@ -18,6 +18,7 @@ use TheCodingMachine\GraphQLite\Containers\BasicAutoWiringContainer; use TheCodingMachine\GraphQLite\Containers\EmptyContainer; use TheCodingMachine\GraphQLite\Fixtures\Integration\Controllers\ContactController; +use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Comment; use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Company; use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Contact; use TheCodingMachine\GraphQLite\Fixtures\Integration\Models\Post; @@ -118,6 +119,7 @@ public function testCreateSchemaOnlyWithFactories(): void ContactFactory::class, ContactOtherType::class, ContactType::class, + Comment::class, Post::class, PostType::class, Company::class, diff --git a/website/docs/prefetch-method.mdx b/website/docs/prefetch-method.mdx index a12cb50891..4e3a3d0c31 100644 --- a/website/docs/prefetch-method.mdx +++ b/website/docs/prefetch-method.mdx @@ -52,19 +52,83 @@ class PostType { { // This method will receive the $prefetchedUsers as first argument. This is the return value of the "prefetchUsers" method below. // Using this prefetched list, it should be easy to map it to the post + return $prefetchedUsers[$this->getUserId()]; } /** * @param Post[] $posts * @return mixed */ - public static function prefetchUsers(iterable $posts) + public static function prefetchUsers(iterable $posts, #[Autowire] UserProvider $loader) { // This function is called only once per GraphQL request // with the list of posts. You can fetch the list of users // associated with this posts in a single request, // for instance using a "IN" query in SQL or a multi-fetch // in your cache back-end. + $userIds = []; + foreach($posts as $post){ + $userIds[] = $post->getUserId(); + } + return $loader->loadMany($userIds) + } +} +``` + +When a `#[Prefetch]` attribute is constructed with returnRequested: true $prefetchedUser argument will be the mapped +value resolved for concrete PostType, so you dont need to extract value from prefetched values + +```php +#[Type] +class PostType { + /** + * @param User $prefetchedUser + * @return User + */ + #[Field] + public function getUser(#[Prefetch("prefetchUsers", returnRequested: true)] $prefetchedUser): User + { + return $prefetchedUser; + } + + /** + * @param Post[] $posts + * @return iterable + */ + public static function prefetchUsers(iterable $posts): iterable + { + foreach($posts as $post){ + yield new User(...); + } + } +} +``` + + +`#[Prefetch('prefetchUsers', true)]` callable can also return Promise[], so you can use for example Overblog DataLoader + +```php +#[Type] +class PostType { + /** + * @param User $prefetchedUser + * @return User + */ + #[Field] + public function getUser(#[Prefetch("prefetchUsers", returnRequested: true)] $prefetchedUser): User + { + return $prefetchedUser; + } + + /** + * @param Post[] $posts + * @return iterable + */ + public static function prefetchUsers(iterable $posts, #[Autowire] DataLoader $dataloader): iterable + { + foreach($posts as $post){ + yield $dataloader->load($post->getId()); + } } } ``` From ef75504debf5077260bf68f82462fcd8c15b6d47 Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Fri, 27 Sep 2024 01:51:54 +0300 Subject: [PATCH 2/7] revert to previous Prefetch behavior --- src/Annotations/Prefetch.php | 10 +-- .../PrefetchParameterMiddleware.php | 1 - src/Parameters/PrefetchDataParameter.php | 26 ++------ src/PrefetchBuffer.php | 6 ++ src/QueryField.php | 7 +- tests/Fixtures/Integration/Models/Blog.php | 20 +++--- .../Integration/Types/CompanyType.php | 25 ------- tests/Fixtures/Integration/Types/PostType.php | 15 +++-- .../Parameters/PrefetchDataParameterTest.php | 2 +- website/docs/prefetch-method.mdx | 66 +------------------ 10 files changed, 41 insertions(+), 137 deletions(-) diff --git a/src/Annotations/Prefetch.php b/src/Annotations/Prefetch.php index 5fbc2d0fac..f4fa3bbeb7 100644 --- a/src/Annotations/Prefetch.php +++ b/src/Annotations/Prefetch.php @@ -10,14 +10,8 @@ #[Attribute(Attribute::TARGET_PARAMETER)] class Prefetch implements ParameterAnnotationInterface { - /** - * @param string|(callable&array{class-string, string}) $callable - * @param bool $returnRequested return value mapped to requested method - */ - public function __construct( - public readonly string|array $callable, - public readonly bool $returnRequested = false, - ) + /** @param string|(callable&array{class-string, string}) $callable */ + public function __construct(public readonly string|array $callable) { } diff --git a/src/Mappers/Parameters/PrefetchParameterMiddleware.php b/src/Mappers/Parameters/PrefetchParameterMiddleware.php index b5fb601082..bb5fef4f70 100644 --- a/src/Mappers/Parameters/PrefetchParameterMiddleware.php +++ b/src/Mappers/Parameters/PrefetchParameterMiddleware.php @@ -52,7 +52,6 @@ public function mapParameter(ReflectionParameter $parameter, DocBlock $docBlock, fieldName: $method->getName(), resolver: $resolver, parameters: $parameters, - returnRequested: $prefetch->returnRequested, ); } } diff --git a/src/Parameters/PrefetchDataParameter.php b/src/Parameters/PrefetchDataParameter.php index 3b527c7420..284cd364c2 100644 --- a/src/Parameters/PrefetchDataParameter.php +++ b/src/Parameters/PrefetchDataParameter.php @@ -12,7 +12,6 @@ use TheCodingMachine\GraphQLite\PrefetchBuffer; use TheCodingMachine\GraphQLite\QueryField; -use function array_key_exists; use function assert; /** @@ -28,7 +27,6 @@ public function __construct( private readonly string $fieldName, private readonly mixed $resolver, public readonly array $parameters, - public readonly bool $returnRequested = false, ) { } @@ -59,7 +57,10 @@ public function resolve(object|null $source, array $args, mixed $context, Resolv $this->computePrefetch($args, $context, $info, $prefetchBuffer); } - return $prefetchBuffer->getResult($source); + $result = $prefetchBuffer->getResult($source); + // clear internal storage + $prefetchBuffer->purgeResult($source); + return $result; }); } @@ -71,22 +72,9 @@ private function computePrefetch(array $args, mixed $context, ResolveInfo $info, $toPassPrefetchArgs = QueryField::paramsToArguments($this->fieldName, $this->parameters, null, $args, $context, $info, $this->resolver); $resolvedValues = ($this->resolver)($sources, ...$toPassPrefetchArgs); - if ($this->returnRequested) { - foreach ($resolvedValues as $key => $resolvedValue) { - if (! array_key_exists($key, $sources)) { - throw new GraphQLRuntimeException( - 'Called by Prefetch function should accept ' . - 'Array and return Array, but the function did ' . - 'not return an Array of the same length as the Array of keys.', - ); - } - $prefetchBuffer->storeResult($sources[$key], $resolvedValue); - } - } else { - foreach ($sources as $source) { - // map results to each source to support old prefetch behavior - $prefetchBuffer->storeResult($source, $resolvedValues); - } + foreach ($sources as $source) { + // map results to each source to support old prefetch behavior + $prefetchBuffer->storeResult($source, $resolvedValues); } } diff --git a/src/PrefetchBuffer.php b/src/PrefetchBuffer.php index 5be2c4ee00..69ebcb706b 100644 --- a/src/PrefetchBuffer.php +++ b/src/PrefetchBuffer.php @@ -89,4 +89,10 @@ public function getResult( ): mixed { return $this->results->offsetGet($source); } + + public function purgeResult( + object $source, + ): void { + $this->results->offsetUnset($source); + } } diff --git a/src/QueryField.php b/src/QueryField.php index a02665899b..0a63038b67 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -194,14 +194,17 @@ public static function paramsToArguments(string $name, array $parameters, object */ public static function deferred(array $toPassArgs, Closure $callResolver): mixed { + $deferredArgument = null; foreach ($toPassArgs as $position => $toPassArg) { if ($toPassArg instanceof SyncPromise) { - return $toPassArg->then(static function ($resolvedValue) use ($toPassArgs, $position, $callResolver) { + $deferredArgument = $toPassArg->then(static function ($resolvedValue) use ($toPassArgs, $position, $callResolver) { $toPassArgs[$position] = $resolvedValue; return self::deferred($toPassArgs, $callResolver); }); + break; } } - return $callResolver(...$toPassArgs); + + return $deferredArgument ?? $callResolver(...$toPassArgs); } } diff --git a/tests/Fixtures/Integration/Models/Blog.php b/tests/Fixtures/Integration/Models/Blog.php index 4adbacfabb..f3b593841a 100644 --- a/tests/Fixtures/Integration/Models/Blog.php +++ b/tests/Fixtures/Integration/Models/Blog.php @@ -23,29 +23,29 @@ public function getId(): int } /** - * @param Post[] $prefetchedPosts + * @param Post[][] $prefetchedPosts * * @return Post[] */ #[Field] public function getPosts( - #[Prefetch('prefetchPosts', true)] + #[Prefetch('prefetchPosts')] array $prefetchedPosts, ): array { - return $prefetchedPosts; + return $prefetchedPosts[$this->id]; } /** - * @param Blog[] $prefetchedSubBlogs + * @param Blog[][] $prefetchedSubBlogs * * @return Blog[] */ #[Field] public function getSubBlogs( - #[Prefetch('prefetchSubBlogs', true)] + #[Prefetch('prefetchSubBlogs')] array $prefetchedSubBlogs, ): array { - return $prefetchedSubBlogs; + return $prefetchedSubBlogs[$this->id]; } /** @@ -56,9 +56,9 @@ public function getSubBlogs( public static function prefetchPosts(iterable $blogs): array { $posts = []; - foreach ($blogs as $key => $blog) { + foreach ($blogs as $blog) { $blogId = $blog->getId(); - $posts[$key] = [ + $posts[$blog->getId()] = [ new Post('post-' . $blogId . '.1'), new Post('post-' . $blogId . '.2'), ]; @@ -75,10 +75,10 @@ public static function prefetchPosts(iterable $blogs): array public static function prefetchSubBlogs(iterable $blogs): array { $subBlogs = []; - foreach ($blogs as $key => $blog) { + foreach ($blogs as $blog) { $blogId = $blog->getId(); $subBlogId = $blogId * 10; - $subBlogs[$key] = [new Blog($subBlogId)]; + $subBlogs[$blog->id] = [new Blog($subBlogId)]; } return $subBlogs; diff --git a/tests/Fixtures/Integration/Types/CompanyType.php b/tests/Fixtures/Integration/Types/CompanyType.php index 3d825cea8e..fbada7f2aa 100644 --- a/tests/Fixtures/Integration/Types/CompanyType.php +++ b/tests/Fixtures/Integration/Types/CompanyType.php @@ -44,29 +44,4 @@ public static function prefetchContacts(array $companies): array return $contacts; } - - #[Field] - public function getContactRequested( - Company $company, - #[Prefetch('prefetchContactsExact', true)] - Contact $contact, - ): Contact|null { - return $contact; - } - - /** - * @param Company[] $companies - * - * @return Contact[] - */ - public static function prefetchContactsExact(array $companies): array - { - $contacts = []; - - foreach ($companies as $key => $company) { - $contacts[$key] = new Contact('Kate'); - } - - return $contacts; - } } diff --git a/tests/Fixtures/Integration/Types/PostType.php b/tests/Fixtures/Integration/Types/PostType.php index ca686c198d..45e9576bb1 100644 --- a/tests/Fixtures/Integration/Types/PostType.php +++ b/tests/Fixtures/Integration/Types/PostType.php @@ -29,17 +29,17 @@ public function getTitle(Post $post): string } /** - * @param Comment[] $prefetchedComments + * @param Comment[][] $prefetchedComments * * @return Comment[] */ #[Field] public function getComments( Post $post, - #[Prefetch('prefetchComments', true)] + #[Prefetch('prefetchComments')] array $prefetchedComments, ): array { - return $prefetchedComments; + return $prefetchedComments[$post->title]; } /** @@ -49,11 +49,14 @@ public function getComments( * * @throws Exception */ - public static function prefetchComments(array $posts): iterable + public static function prefetchComments(array $posts): Promise { $syncPromiseAdapter = new SyncPromiseAdapter(); - foreach ($posts as $key => $post) { - yield $key => $syncPromiseAdapter->createFulfilled([new Comment('comment for ' . $post->title)]); + $result = []; + foreach ($posts as $post) { + $result[$post->title] = [new Comment('comment for ' . $post->title)]; } + + return $syncPromiseAdapter->createFulfilled($result); } } diff --git a/tests/Parameters/PrefetchDataParameterTest.php b/tests/Parameters/PrefetchDataParameterTest.php index 132a823e35..a7e875c76a 100644 --- a/tests/Parameters/PrefetchDataParameterTest.php +++ b/tests/Parameters/PrefetchDataParameterTest.php @@ -71,7 +71,7 @@ public function testResolveWithoutExistingResult(): void self::assertFalse($buffer->hasResult($source)); self::assertSame([$source], $buffer->getObjectsByArguments($args)); self::assertSame($prefetchResult, $this->deferredValue($resolvedParameterPromise)); - self::assertTrue($buffer->hasResult($source)); + self::assertFalse($buffer->hasResult($source)); } private function deferredValue(Deferred $promise): mixed diff --git a/website/docs/prefetch-method.mdx b/website/docs/prefetch-method.mdx index 4e3a3d0c31..a12cb50891 100644 --- a/website/docs/prefetch-method.mdx +++ b/website/docs/prefetch-method.mdx @@ -52,83 +52,19 @@ class PostType { { // This method will receive the $prefetchedUsers as first argument. This is the return value of the "prefetchUsers" method below. // Using this prefetched list, it should be easy to map it to the post - return $prefetchedUsers[$this->getUserId()]; } /** * @param Post[] $posts * @return mixed */ - public static function prefetchUsers(iterable $posts, #[Autowire] UserProvider $loader) + public static function prefetchUsers(iterable $posts) { // This function is called only once per GraphQL request // with the list of posts. You can fetch the list of users // associated with this posts in a single request, // for instance using a "IN" query in SQL or a multi-fetch // in your cache back-end. - $userIds = []; - foreach($posts as $post){ - $userIds[] = $post->getUserId(); - } - return $loader->loadMany($userIds) - } -} -``` - -When a `#[Prefetch]` attribute is constructed with returnRequested: true $prefetchedUser argument will be the mapped -value resolved for concrete PostType, so you dont need to extract value from prefetched values - -```php -#[Type] -class PostType { - /** - * @param User $prefetchedUser - * @return User - */ - #[Field] - public function getUser(#[Prefetch("prefetchUsers", returnRequested: true)] $prefetchedUser): User - { - return $prefetchedUser; - } - - /** - * @param Post[] $posts - * @return iterable - */ - public static function prefetchUsers(iterable $posts): iterable - { - foreach($posts as $post){ - yield new User(...); - } - } -} -``` - - -`#[Prefetch('prefetchUsers', true)]` callable can also return Promise[], so you can use for example Overblog DataLoader - -```php -#[Type] -class PostType { - /** - * @param User $prefetchedUser - * @return User - */ - #[Field] - public function getUser(#[Prefetch("prefetchUsers", returnRequested: true)] $prefetchedUser): User - { - return $prefetchedUser; - } - - /** - * @param Post[] $posts - * @return iterable - */ - public static function prefetchUsers(iterable $posts, #[Autowire] DataLoader $dataloader): iterable - { - foreach($posts as $post){ - yield $dataloader->load($post->getId()); - } } } ``` From ffbb68b75719b50c9c94a0e90f5d701233bc6ec6 Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Fri, 27 Sep 2024 01:52:48 +0300 Subject: [PATCH 3/7] allow promise to be returned in methods --- src/QueryField.php | 25 ++++++++++++++-------- tests/Fixtures/Integration/Models/Blog.php | 13 +++++------ 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/src/QueryField.php b/src/QueryField.php index 0a63038b67..656cc99a5d 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -74,15 +74,7 @@ public function __construct( $callResolver = function (...$args) use ($originalResolver, $source, $resolver) { $result = $resolver($source, ...$args); - try { - $this->assertReturnType($result); - } catch (TypeMismatchRuntimeException $e) { - $e->addInfo($this->name, $originalResolver->toString()); - - throw $e; - } - - return $result; + return $this->unwrapReturnType($result, $originalResolver); }; // GraphQL allows deferring resolving the field's value using promises, i.e. they call the resolve @@ -98,6 +90,21 @@ public function __construct( parent::__construct($config); } + private function unwrapReturnType(mixed $result, ResolverInterface $originalResolver): mixed + { + if ($result instanceof SyncPromise) { + return $result->then(fn ($resolvedValue) => $this->unwrapReturnType($resolvedValue, $originalResolver)); + } + try { + $this->assertReturnType($result); + } catch (TypeMismatchRuntimeException $e) { + $e->addInfo($this->name, $originalResolver->toString()); + + throw $e; + } + return $result; + } + /** * This method checks the returned value of the resolver to be sure it matches the documented return type. * We are sure the returned value is of the correct type... except if the return type is type-hinted as an array. diff --git a/tests/Fixtures/Integration/Models/Blog.php b/tests/Fixtures/Integration/Models/Blog.php index f3b593841a..80812119f2 100644 --- a/tests/Fixtures/Integration/Models/Blog.php +++ b/tests/Fixtures/Integration/Models/Blog.php @@ -4,6 +4,7 @@ namespace TheCodingMachine\GraphQLite\Fixtures\Integration\Models; +use GraphQL\Deferred; use TheCodingMachine\GraphQLite\Annotations\Field; use TheCodingMachine\GraphQLite\Annotations\Prefetch; use TheCodingMachine\GraphQLite\Annotations\Type; @@ -35,17 +36,13 @@ public function getPosts( return $prefetchedPosts[$this->id]; } - /** - * @param Blog[][] $prefetchedSubBlogs - * - * @return Blog[] - */ - #[Field] + /** @param Blog[][] $prefetchedSubBlogs */ + #[Field(outputType: '[Blog!]!')] public function getSubBlogs( #[Prefetch('prefetchSubBlogs')] array $prefetchedSubBlogs, - ): array { - return $prefetchedSubBlogs[$this->id]; + ): Deferred { + return new Deferred(fn () => $prefetchedSubBlogs[$this->id]); } /** From 60124506f6aff1d52ca51ac43706de97f8699ff1 Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Tue, 1 Oct 2024 18:28:41 +0300 Subject: [PATCH 4/7] add nesting preload test --- tests/Integration/EndToEndTest.php | 115 +++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index 0d9f2086c4..24a055dd1a 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -2339,4 +2339,119 @@ public function testEndToEndSubscriptionWithInput(): void 'contactAddedWithFilter' => null, ], $this->getSuccessResult($result)); } + + public function testPrefetchingOfSameTypeInDifferentNestingLevels(): void + { + $schema = $this->mainContainer->get(Schema::class); + assert($schema instanceof Schema); + + $schema->assertValid(); + + $queryString = ' + query { + blogs { + id + subBlogs { + id + posts { + title + comments { + text + } + } + } + posts { + title + comments { + text + } + } + } + } + '; + + $result = GraphQL::executeQuery( + $schema, + $queryString, + null, + new Context(), + ); + + $this->assertSame([ + 'blogs' => [ + [ + 'id' => '1', + 'subBlogs' => [ + [ + 'id' => '10', + 'posts' => [ + [ + 'title' => 'post-10.1', + 'comments' => [ + ['text' => 'comment for post-10.1'], + ], + ], + [ + 'title' => 'post-10.2', + 'comments' => [ + ['text' => 'comment for post-10.2'], + ], + ], + ], + ], + ], + 'posts' => [ + [ + 'title' => 'post-1.1', + 'comments' => [ + ['text' => 'comment for post-1.1'], + ], + ], + [ + 'title' => 'post-1.2', + 'comments' => [ + ['text' => 'comment for post-1.2'], + ], + ], + ], + ], + [ + 'id' => '2', + 'subBlogs' => [ + [ + 'id' => '20', + 'posts' => [ + [ + 'title' => 'post-20.1', + 'comments' => [ + ['text' => 'comment for post-20.1'], + ], + ], + [ + 'title' => 'post-20.2', + 'comments' => [ + ['text' => 'comment for post-20.2'], + ], + ], + ], + ], + ], + 'posts' => [ + [ + 'title' => 'post-2.1', + 'comments' => [ + ['text' => 'comment for post-2.1'], + ], + ], + [ + 'title' => 'post-2.2', + 'comments' => [ + ['text' => 'comment for post-2.2'], + ], + ], + ], + ], + ], + ], $this->getSuccessResult($result)); + } } From dae8b0e66bf51920bf90288dd3829ecc3622376a Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Tue, 1 Oct 2024 18:28:48 +0300 Subject: [PATCH 5/7] CR fixes --- src/Parameters/PrefetchDataParameter.php | 1 + src/QueryField.php | 8 +++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/src/Parameters/PrefetchDataParameter.php b/src/Parameters/PrefetchDataParameter.php index 284cd364c2..a903272e4a 100644 --- a/src/Parameters/PrefetchDataParameter.php +++ b/src/Parameters/PrefetchDataParameter.php @@ -72,6 +72,7 @@ private function computePrefetch(array $args, mixed $context, ResolveInfo $info, $toPassPrefetchArgs = QueryField::paramsToArguments($this->fieldName, $this->parameters, null, $args, $context, $info, $this->resolver); $resolvedValues = ($this->resolver)($sources, ...$toPassPrefetchArgs); + foreach ($sources as $source) { // map results to each source to support old prefetch behavior $prefetchBuffer->storeResult($source, $resolvedValues); diff --git a/src/QueryField.php b/src/QueryField.php index 656cc99a5d..5a224b70a9 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -74,7 +74,7 @@ public function __construct( $callResolver = function (...$args) use ($originalResolver, $source, $resolver) { $result = $resolver($source, ...$args); - return $this->unwrapReturnType($result, $originalResolver); + return $this->resolveWithPromise($result, $originalResolver); }; // GraphQL allows deferring resolving the field's value using promises, i.e. they call the resolve @@ -90,11 +90,12 @@ public function __construct( parent::__construct($config); } - private function unwrapReturnType(mixed $result, ResolverInterface $originalResolver): mixed + private function resolveWithPromise(mixed $result, ResolverInterface $originalResolver): mixed { if ($result instanceof SyncPromise) { - return $result->then(fn ($resolvedValue) => $this->unwrapReturnType($resolvedValue, $originalResolver)); + return $result->then(fn ($resolvedValue) => $this->resolveWithPromise($resolvedValue, $originalResolver)); } + try { $this->assertReturnType($result); } catch (TypeMismatchRuntimeException $e) { @@ -102,6 +103,7 @@ private function unwrapReturnType(mixed $result, ResolverInterface $originalReso throw $e; } + return $result; } From b6c54c25c72273d82550e4d8f45c032a1138acba Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Tue, 8 Oct 2024 07:00:07 +0300 Subject: [PATCH 6/7] CR fixes --- src/Parameters/PrefetchDataParameter.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Parameters/PrefetchDataParameter.php b/src/Parameters/PrefetchDataParameter.php index a903272e4a..9aaab6c7a8 100644 --- a/src/Parameters/PrefetchDataParameter.php +++ b/src/Parameters/PrefetchDataParameter.php @@ -54,7 +54,7 @@ public function resolve(object|null $source, array $args, mixed $context, Resolv // already knowing all the requested fields (source-arguments combinations). return new Deferred(function () use ($source, $info, $context, $args, $prefetchBuffer) { if (! $prefetchBuffer->hasResult($source)) { - $this->computePrefetch($args, $context, $info, $prefetchBuffer); + $this->processPrefetch($args, $context, $info, $prefetchBuffer); } $result = $prefetchBuffer->getResult($source); @@ -65,7 +65,7 @@ public function resolve(object|null $source, array $args, mixed $context, Resolv } /** @param array $args */ - private function computePrefetch(array $args, mixed $context, ResolveInfo $info, PrefetchBuffer $prefetchBuffer): void + private function processPrefetch(array $args, mixed $context, ResolveInfo $info, PrefetchBuffer $prefetchBuffer): void { $sources = $prefetchBuffer->getObjectsByArguments($args, $info); $prefetchBuffer->purge($args, $info); From ca0823aafd4cf2998a0212a42c5e314e5857eb02 Mon Sep 17 00:00:00 2001 From: Andrii Sudiev Date: Tue, 8 Oct 2024 20:39:42 +0300 Subject: [PATCH 7/7] add Promise type mapping docs --- website/docs/type-mapping.mdx | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/website/docs/type-mapping.mdx b/website/docs/type-mapping.mdx index 26127719d6..d5b6c4dc29 100644 --- a/website/docs/type-mapping.mdx +++ b/website/docs/type-mapping.mdx @@ -330,6 +330,47 @@ query { } ``` +## Promise mapping + +GraphQL includes a native \GraphQL\Deferred type. +You can map the return type by adding a detailed `@return` statement in the PHPDoc. +An alternative to the `@return` statement is using `#[Field(outputType: SomeGQLType)]`. + +All the previously mentioned mappings work with Promises, except when a return type is explicitly declared +in the method signature. + +This allows you to use \Overblog\DataLoader\DataLoader as an alternative +for resolving N+1 query issues and caching intermediate results. + +```php +#[Type] +class Product +{ + // ... + + /** + * @return string + */ + #[Field] + public function getName(): Deferred + { + return new Deferred(fn() => $this->name); + } + + #[Field(outputType: "Float")] + public function getPrice(): Deferred + { + return new Deferred(fn() => $this->price); + } + + #[Field(outputType: "[String!]!")] + public function getCategories(#[Autowire('categoryDataLoader')] DataLoader $categoryDataLoader): SyncPromise + { + return $categoryDataLoader->load($this->id)->adoptedPromise; + } +} +``` + ## More scalar types Available in GraphQLite 4.0+