From eea0409b82d8d4b26f5e97ce22d8f86df88424f9 Mon Sep 17 00:00:00 2001 From: Heiko Jerichen Date: Fri, 28 Oct 2022 16:47:04 +0200 Subject: [PATCH 1/3] Fix bug of calling prefetch method only once with not all type objects when prefetching is on different nesting levels. --- src/QueryField.php | 9 +- .../Controllers/BlogController.php | 19 +++ tests/Fixtures/Integration/Models/Blog.php | 71 +++++++++++ tests/Fixtures/Integration/Models/Comment.php | 21 ++++ tests/Fixtures/Integration/Models/Post.php | 23 ++++ tests/Integration/EndToEndTest.php | 115 ++++++++++++++++++ 6 files changed, 253 insertions(+), 5 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/QueryField.php b/src/QueryField.php index eb010c1f92..a0693849d5 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -103,7 +103,8 @@ public function __construct(string $name, OutputType $type, array $arguments, Re $prefetchBuffer->register($source, $args); return new Deferred(function () use ($prefetchBuffer, $source, $args, $context, $info, $prefetchArgs, $prefetchMethodName, $arguments, $resolveFn, $originalResolver) { - if (! $prefetchBuffer->hasResult($args)) { + $sources = $prefetchBuffer->getObjectsByArguments($args); + if (! $prefetchBuffer->hasResult($sources)) { if ($originalResolver instanceof SourceResolverInterface) { $originalResolver->setObject($source); } @@ -111,17 +112,15 @@ public function __construct(string $name, OutputType $type, array $arguments, Re // TODO: originalPrefetchResolver and prefetchResolver needed!!! $prefetchCallable = [$originalResolver->getObject(), $prefetchMethodName]; - $sources = $prefetchBuffer->getObjectsByArguments($args); - assert(is_callable($prefetchCallable)); $toPassPrefetchArgs = $this->paramsToArguments($prefetchArgs, $source, $args, $context, $info, $prefetchCallable); array_unshift($toPassPrefetchArgs, $sources); assert(is_callable($prefetchCallable)); $prefetchResult = $prefetchCallable(...$toPassPrefetchArgs); - $prefetchBuffer->storeResult($prefetchResult, $args); + $prefetchBuffer->storeResult($prefetchResult, $sources); } else { - $prefetchResult = $prefetchBuffer->getResult($args); + $prefetchResult = $prefetchBuffer->getResult($sources); } foreach ($arguments as $argument) { diff --git a/tests/Fixtures/Integration/Controllers/BlogController.php b/tests/Fixtures/Integration/Controllers/BlogController.php new file mode 100644 index 0000000000..c3ed027d09 --- /dev/null +++ b/tests/Fixtures/Integration/Controllers/BlogController.php @@ -0,0 +1,19 @@ +id; + } + + /** + * @param Post[][] $prefetchedPosts + * @return Post[] + */ + #[Field(prefetchMethod: 'prefetchPosts')] + public function getPosts(array $prefetchedPosts): array + { + return $prefetchedPosts[$this->id] ?? []; + } + + /** + * @param Blog[][] $prefetchedSubBlogs + * @return Blog[] + */ + #[Field(prefetchMethod: 'prefetchSubBlogs')] + public function getSubBlogs(array $prefetchedSubBlogs): array + { + return $prefetchedSubBlogs[$this->id] ?? []; + } + + /** + * @param self[] $blogs + * @return Post[][] + */ + public function prefetchPosts(array $blogs): array + { + $posts = []; + foreach ($blogs as $blog) { + $blogId = $blog->getId(); + $posts[$blogId][] = new Post("post-$blogId.1"); + $posts[$blogId][] = new Post("post-$blogId.2"); + } + return $posts; + } + + /** + * @param self[] $blogs + * @return Blog[][] + */ + public function prefetchSubBlogs(array $blogs): array + { + $subBlogs = []; + foreach ($blogs as $blog) { + $blogId = $blog->getId(); + $subBlogId = $blogId * 10; + $subBlogs[$blogId][] = 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..7d853c281a --- /dev/null +++ b/tests/Fixtures/Integration/Models/Comment.php @@ -0,0 +1,21 @@ +text; + } +} diff --git a/tests/Fixtures/Integration/Models/Post.php b/tests/Fixtures/Integration/Models/Post.php index 8b611202d2..90c0e1e4f2 100644 --- a/tests/Fixtures/Integration/Models/Post.php +++ b/tests/Fixtures/Integration/Models/Post.php @@ -97,6 +97,29 @@ public function setSummary(?string $summary): void $this->summary = $summary; } + /** + * @param Comment[][] $prefetchedComments + * @return Comment[] + */ + #[Field(prefetchMethod: 'prefetchComments')] + public function getComments(array $prefetchedComments): array + { + return $prefetchedComments[$this->title] ?? []; + } + + /** + * @param self[] $posts + * @return Comment[][] + */ + public function prefetchComments(array $posts): array + { + $comments = []; + foreach ($posts as $post) { + $comments[$post->title][] = new Comment("comment for $post->title"); + } + return $comments; + } + /** * @param string $inaccessible */ diff --git a/tests/Integration/EndToEndTest.php b/tests/Integration/EndToEndTest.php index c5756cc58b..b2b7a29e7f 100644 --- a/tests/Integration/EndToEndTest.php +++ b/tests/Integration/EndToEndTest.php @@ -2500,4 +2500,119 @@ public function isAllowed(string $right, $subject = null): bool $data = $this->getSuccessResult($result); $this->assertSame(["graph", "ql"], $data['updateTrickyProduct']['list']); } + + public function testPrefetchingOfSameTypeInDifferentNestingLevels(): void + { + /** @var Schema $schema */ + $schema = $this->mainContainer->get(Schema::class); + + $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 009652b03c3ab96c65244690506bc50a6694ac95 Mon Sep 17 00:00:00 2001 From: Heiko Jerichen Date: Sun, 30 Oct 2022 17:18:02 +0100 Subject: [PATCH 2/3] Take back changes in QueryField. --- src/QueryField.php | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/QueryField.php b/src/QueryField.php index a0693849d5..eb010c1f92 100644 --- a/src/QueryField.php +++ b/src/QueryField.php @@ -103,8 +103,7 @@ public function __construct(string $name, OutputType $type, array $arguments, Re $prefetchBuffer->register($source, $args); return new Deferred(function () use ($prefetchBuffer, $source, $args, $context, $info, $prefetchArgs, $prefetchMethodName, $arguments, $resolveFn, $originalResolver) { - $sources = $prefetchBuffer->getObjectsByArguments($args); - if (! $prefetchBuffer->hasResult($sources)) { + if (! $prefetchBuffer->hasResult($args)) { if ($originalResolver instanceof SourceResolverInterface) { $originalResolver->setObject($source); } @@ -112,15 +111,17 @@ public function __construct(string $name, OutputType $type, array $arguments, Re // TODO: originalPrefetchResolver and prefetchResolver needed!!! $prefetchCallable = [$originalResolver->getObject(), $prefetchMethodName]; + $sources = $prefetchBuffer->getObjectsByArguments($args); + assert(is_callable($prefetchCallable)); $toPassPrefetchArgs = $this->paramsToArguments($prefetchArgs, $source, $args, $context, $info, $prefetchCallable); array_unshift($toPassPrefetchArgs, $sources); assert(is_callable($prefetchCallable)); $prefetchResult = $prefetchCallable(...$toPassPrefetchArgs); - $prefetchBuffer->storeResult($prefetchResult, $sources); + $prefetchBuffer->storeResult($prefetchResult, $args); } else { - $prefetchResult = $prefetchBuffer->getResult($sources); + $prefetchResult = $prefetchBuffer->getResult($args); } foreach ($arguments as $argument) { From 416ecc28402e340ec5f0c1d70b5e23f4ea8794b8 Mon Sep 17 00:00:00 2001 From: Heiko Jerichen Date: Sun, 30 Oct 2022 17:20:57 +0100 Subject: [PATCH 3/3] Add invalidation of results when registering new objects in PrefetchBuffer. --- src/PrefetchBuffer.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/PrefetchBuffer.php b/src/PrefetchBuffer.php index 5105d02249..d93d3d93c5 100644 --- a/src/PrefetchBuffer.php +++ b/src/PrefetchBuffer.php @@ -22,7 +22,10 @@ class PrefetchBuffer /** @param array $arguments The input arguments passed from GraphQL to the field. */ public function register(object $object, array $arguments): void { - $this->objects[$this->computeHash($arguments)][] = $object; + $hash = $this->computeHash($arguments); + + $this->objects[$hash][] = $object; + unset($this->results[$hash]); } /** @param array $arguments The input arguments passed from GraphQL to the field. */ @@ -44,7 +47,8 @@ public function getObjectsByArguments(array $arguments): array /** @param array $arguments The input arguments passed from GraphQL to the field. */ public function purge(array $arguments): void { - unset($this->objects[$this->computeHash($arguments)]); + $hash = $this->computeHash($arguments); + unset($this->objects[$hash], $this->results[$hash]); } /** @param array $arguments The input arguments passed from GraphQL to the field. */