diff --git a/CHANGELOG.md b/CHANGELOG.md index 885bc7a33..0cdc31d7a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,16 @@ You can find and compare releases at the [GitHub release page](https://github.co ## Unreleased +## v15.29.0 + +### Changed + +- Optimize `Deferred` execution https://github.com/webonyx/graphql-php/pull/1805 + +### Deprecated + +- Deprecate `GraphQL\Deferred::create()` in favor of constructor https://github.com/webonyx/graphql-php/pull/1805 + ## v15.28.0 ### Changed diff --git a/benchmarks/DeferredBench.php b/benchmarks/DeferredBench.php new file mode 100644 index 000000000..c63ed6915 --- /dev/null +++ b/benchmarks/DeferredBench.php @@ -0,0 +1,80 @@ + 'value'); + SyncPromiseQueue::run(); + } + + public function benchNestedDeferred(): void + { + new Deferred(static fn () => new Deferred(static fn () => null)); + SyncPromiseQueue::run(); + } + + public function benchChain5(): void + { + $deferred = new Deferred(static fn () => 'value'); + $deferred->then(static fn ($v) => $v) + ->then(static fn ($v) => $v) + ->then(static fn ($v) => $v) + ->then(static fn ($v) => $v) + ->then(static fn ($v) => $v); + SyncPromiseQueue::run(); + } + + public function benchChain100(): void + { + $deferred = new Deferred(static fn () => 'value'); + $promise = $deferred; + for ($i = 0; $i < 100; ++$i) { + $promise = $promise->then(static fn ($v) => $v); + } + SyncPromiseQueue::run(); + } + + public function benchManyDeferreds(): void + { + $fn = static fn () => null; + for ($i = 0; $i < 1000; ++$i) { + new Deferred($fn); + } + SyncPromiseQueue::run(); + } + + public function benchManyNestedDeferreds(): void + { + for ($i = 0; $i < 5000; ++$i) { + new Deferred(static fn () => new Deferred(static fn () => null)); + } + SyncPromiseQueue::run(); + } + + public function bench1000Chains(): void + { + $promises = []; + for ($i = 0; $i < 1000; ++$i) { + $d = new Deferred(static fn () => $i); + $promises[] = $d->then(static fn ($v) => $v) + ->then(static fn ($v) => $v) + ->then(static fn ($v) => $v); + } + SyncPromiseQueue::run(); + } +} diff --git a/benchmarks/HugeSchemaBench.php b/benchmarks/HugeSchemaBench.php index fe6bb1821..1f58835a5 100644 --- a/benchmarks/HugeSchemaBench.php +++ b/benchmarks/HugeSchemaBench.php @@ -14,11 +14,11 @@ * * @OutputTimeUnit("milliseconds", precision=3) * - * @Warmup(1) + * @Warmup(2) * - * @Revs(5) + * @Revs(10) * - * @Iterations(1) + * @Iterations(3) */ class HugeSchemaBench { @@ -50,6 +50,7 @@ public function benchSchema(): void ->getTypeMap(); } + /** @Revs(1000) */ public function benchSchemaLazy(): void { $this->createLazySchema(); diff --git a/benchmarks/StarWarsBench.php b/benchmarks/StarWarsBench.php index 394e554ad..b56057055 100644 --- a/benchmarks/StarWarsBench.php +++ b/benchmarks/StarWarsBench.php @@ -11,11 +11,11 @@ * * @OutputTimeUnit("milliseconds", precision=3) * - * @Warmup(2) + * @Warmup(5) * - * @Revs(10) + * @Revs(100) * - * @Iterations(2) + * @Iterations(10) */ class StarWarsBench { diff --git a/composer.json b/composer.json index 1df1a50a2..c11bae3ec 100644 --- a/composer.json +++ b/composer.json @@ -64,7 +64,7 @@ }, "scripts": { "baseline": "phpstan --generate-baseline", - "bench": "phpbench run", + "bench": "php -d zend.assertions=-1 vendor/bin/phpbench run --report=default", "check": [ "@fix", "@stan", @@ -78,6 +78,6 @@ "php-cs-fixer": "make php-cs-fixer", "rector": "rector process", "stan": "phpstan --verbose", - "test": "php -d zend.exception_ignore_args=Off -d zend.assertions=On -d assert.active=On -d assert.exception=On vendor/bin/phpunit" + "test": "php -d zend.exception_ignore_args=0 -d zend.assertions=1 -d assert.active=1 -d assert.exception=1 vendor/bin/phpunit" } } diff --git a/docs/class-reference.md b/docs/class-reference.md index ca04e7f7c..3da1d4bbd 100644 --- a/docs/class-reference.md +++ b/docs/class-reference.md @@ -1778,6 +1778,25 @@ function createRejected(Throwable $reason): GraphQL\Executor\Promise\Promise function all(iterable $promisesOrValues): GraphQL\Executor\Promise\Promise ``` +## GraphQL\Deferred + +User-facing promise class for deferred field resolution. + +@phpstan-type Executor callable(): mixed + +### GraphQL\Deferred Methods + +```php +/** + * Create a new Deferred promise and enqueue its execution. + * + * @api + * + * @param Executor $executor + */ +function __construct(callable $executor) +``` + ## GraphQL\Validator\DocumentValidator Implements the "Validation" section of the spec. diff --git a/generate-class-reference.php b/generate-class-reference.php index dff2bace2..8c51d1ebc 100644 --- a/generate-class-reference.php +++ b/generate-class-reference.php @@ -23,6 +23,7 @@ GraphQL\Executor\ScopedContext::class => [], GraphQL\Executor\ExecutionResult::class => [], GraphQL\Executor\Promise\PromiseAdapter::class => [], + GraphQL\Deferred::class => [], GraphQL\Validator\DocumentValidator::class => [], GraphQL\Error\Error::class => ['constants' => true], GraphQL\Error\Warning::class => ['constants' => true], diff --git a/phpbench.json b/phpbench.json index 28c601c87..b47419350 100644 --- a/phpbench.json +++ b/phpbench.json @@ -3,5 +3,11 @@ "runner.path": "benchmarks", "runner.file_pattern": "*Bench.php", "runner.retry_threshold": 5, - "runner.time_unit": "milliseconds" + "runner.time_unit": "milliseconds", + "runner.progress": "plain", + "report.generators": { + "default": { + "extends": "aggregate" + } + } } diff --git a/src/Deferred.php b/src/Deferred.php index 69837cc07..22d410c28 100644 --- a/src/Deferred.php +++ b/src/Deferred.php @@ -3,21 +3,55 @@ namespace GraphQL; use GraphQL\Executor\Promise\Adapter\SyncPromise; +use GraphQL\Executor\Promise\Adapter\SyncPromiseQueue; /** - * @phpstan-import-type Executor from SyncPromise + * User-facing promise class for deferred field resolution. + * + * @phpstan-type Executor callable(): mixed */ class Deferred extends SyncPromise { - /** @param Executor $executor */ - public static function create(callable $executor): self + /** + * Executor for deferred promises. + * + * @var (callable(): mixed)|null + */ + protected $executor; + + /** + * Create a new Deferred promise and enqueue its execution. + * + * @api + * + * @param Executor $executor + */ + public function __construct(callable $executor) { - return new self($executor); + $this->executor = $executor; + + SyncPromiseQueue::enqueue(function (): void { + $executor = $this->executor; + assert($executor !== null, 'Always set in constructor, this callback runs only once.'); + $this->executor = null; + + try { + $this->resolve($executor()); + } catch (\Throwable $e) { + $this->reject($e); + } + }); } - /** @param Executor $executor */ - public function __construct(callable $executor) + /** + * Alias for __construct. + * + * @param Executor $executor + * + * @deprecated TODO remove in next major version, use new Deferred() instead + */ + public static function create(callable $executor): self { - parent::__construct($executor); + return new self($executor); } } diff --git a/src/Executor/Promise/Adapter/AmpPromiseAdapter.php b/src/Executor/Promise/Adapter/AmpPromiseAdapter.php index 735a37bdc..25e9fc84b 100644 --- a/src/Executor/Promise/Adapter/AmpPromiseAdapter.php +++ b/src/Executor/Promise/Adapter/AmpPromiseAdapter.php @@ -41,10 +41,9 @@ public function then(Promise $promise, ?callable $onFulfilled = null, ?callable } }; - $adoptedPromise = $promise->adoptedPromise; - assert($adoptedPromise instanceof AmpPromise); - - $adoptedPromise->onResolve($onResolve); + $ampPromise = $promise->adoptedPromise; + assert($ampPromise instanceof AmpPromise); + $ampPromise->onResolve($onResolve); return new Promise($deferred->promise(), $this); } diff --git a/src/Executor/Promise/Adapter/ReactPromiseAdapter.php b/src/Executor/Promise/Adapter/ReactPromiseAdapter.php index e38027001..7eb153c13 100644 --- a/src/Executor/Promise/Adapter/ReactPromiseAdapter.php +++ b/src/Executor/Promise/Adapter/ReactPromiseAdapter.php @@ -28,34 +28,34 @@ public function convertThenable($thenable): Promise /** @throws InvariantViolation */ public function then(Promise $promise, ?callable $onFulfilled = null, ?callable $onRejected = null): Promise { - $adoptedPromise = $promise->adoptedPromise; - assert($adoptedPromise instanceof ReactPromiseInterface); + $reactPromise = $promise->adoptedPromise; + assert($reactPromise instanceof ReactPromiseInterface); - return new Promise($adoptedPromise->then($onFulfilled, $onRejected), $this); + return new Promise($reactPromise->then($onFulfilled, $onRejected), $this); } /** @throws InvariantViolation */ public function create(callable $resolver): Promise { - $promise = new ReactPromise($resolver); + $reactPromise = new ReactPromise($resolver); - return new Promise($promise, $this); + return new Promise($reactPromise, $this); } /** @throws InvariantViolation */ public function createFulfilled($value = null): Promise { - $promise = resolve($value); + $reactPromise = resolve($value); - return new Promise($promise, $this); + return new Promise($reactPromise, $this); } /** @throws InvariantViolation */ public function createRejected(\Throwable $reason): Promise { - $promise = reject($reason); + $reactPromise = reject($reason); - return new Promise($promise, $this); + return new Promise($reactPromise, $this); } /** @throws InvariantViolation */ @@ -70,11 +70,11 @@ public function all(iterable $promisesOrValues): Promise $promisesOrValuesArray = is_array($promisesOrValues) ? $promisesOrValues : iterator_to_array($promisesOrValues); - $promise = all($promisesOrValuesArray)->then(static fn ($values): array => array_map( + $reactPromise = all($promisesOrValuesArray)->then(static fn (array $values): array => array_map( static fn ($key) => $values[$key], array_keys($promisesOrValuesArray), )); - return new Promise($promise, $this); + return new Promise($reactPromise, $this); } } diff --git a/src/Executor/Promise/Adapter/SyncPromise.php b/src/Executor/Promise/Adapter/SyncPromise.php index 9bc1ac079..87cfb1b8e 100644 --- a/src/Executor/Promise/Adapter/SyncPromise.php +++ b/src/Executor/Promise/Adapter/SyncPromise.php @@ -5,68 +5,50 @@ use GraphQL\Error\InvariantViolation; /** - * Simplistic (yet full-featured) implementation of Promises A+ spec for regular PHP `sync` mode - * (using queue to defer promises execution). + * Synchronous promise implementation following Promises A+ spec. * - * Library users are not supposed to use SyncPromise class in their resolvers. - * Instead, they should use @see \GraphQL\Deferred which enforces `$executor` callback in the constructor. + * Uses a hybrid approach for optimal memory and performance: + * - Lightweight closures in queue (fast execution) + * - Heavy payload (callbacks) stored on promise objects and cleared after use * - * Root SyncPromise without explicit `$executor` will never resolve (actually throw while trying). - * The whole point of Deferred is to ensure it never happens and that any resolver creates - * at least one $executor to start the promise chain. + * Library users should use @see \GraphQL\Deferred to create promises. * * @phpstan-type Executor callable(): mixed */ class SyncPromise { - public const PENDING = 'pending'; - public const FULFILLED = 'fulfilled'; - public const REJECTED = 'rejected'; + public const PENDING = 0; + public const FULFILLED = 1; + public const REJECTED = 2; - public string $state = self::PENDING; + /** + * Current promise state. + * + * @var 0|1|2 + */ + public int $state = self::PENDING; - /** @var mixed */ + /** + * Resolved value or rejection reason. + * + * @var mixed + */ public $result; /** - * Promises created in `then` method of this promise and awaiting resolution of this promise. + * Promises created in `then` method awaiting resolution. * * @var array< * int, * array{ * self, * (callable(mixed): mixed)|null, - * (callable(\Throwable): mixed)|null - * } + * (callable(\Throwable): mixed)|null, + * }, * > */ protected array $waiting = []; - public static function runQueue(): void - { - $q = self::getQueue(); - while (! $q->isEmpty()) { - $task = $q->dequeue(); - $task(); - } - } - - /** @param Executor|null $executor */ - public function __construct(?callable $executor = null) - { - if ($executor === null) { - return; - } - - self::getQueue()->enqueue(function () use ($executor): void { - try { - $this->resolve($executor()); - } catch (\Throwable $e) { - $this->reject($e); - } - }); - } - /** * @param mixed $value * @@ -77,7 +59,7 @@ public function resolve($value): self switch ($this->state) { case self::PENDING: if ($value === $this) { - throw new \Exception('Cannot resolve promise with self'); + throw new \Exception('Cannot resolve promise with self.'); } if (is_object($value) && method_exists($value, 'then')) { @@ -85,7 +67,7 @@ public function resolve($value): self function ($resolvedValue): void { $this->resolve($resolvedValue); }, - function ($reason): void { + function (\Throwable $reason): void { $this->reject($reason); } ); @@ -99,12 +81,12 @@ function ($reason): void { break; case self::FULFILLED: if ($this->result !== $value) { - throw new \Exception('Cannot change value of fulfilled promise'); + throw new \Exception('Cannot change value of fulfilled promise.'); } break; case self::REJECTED: - throw new \Exception('Cannot resolve rejected promise'); + throw new \Exception('Cannot resolve rejected promise.'); } return $this; @@ -125,59 +107,17 @@ public function reject(\Throwable $reason): self break; case self::REJECTED: if ($reason !== $this->result) { - throw new \Exception('Cannot change rejection reason'); + throw new \Exception('Cannot change rejection reason.'); } break; case self::FULFILLED: - throw new \Exception('Cannot reject fulfilled promise'); + throw new \Exception('Cannot reject fulfilled promise.'); } return $this; } - /** @throws InvariantViolation */ - private function enqueueWaitingPromises(): void - { - if ($this->state === self::PENDING) { - throw new InvariantViolation('Cannot enqueue derived promises when parent is still pending'); - } - - foreach ($this->waiting as $descriptor) { - self::getQueue()->enqueue(function () use ($descriptor): void { - [$promise, $onFulfilled, $onRejected] = $descriptor; - - if ($this->state === self::FULFILLED) { - try { - $promise->resolve($onFulfilled === null ? $this->result : $onFulfilled($this->result)); - } catch (\Throwable $e) { - $promise->reject($e); - } - } elseif ($this->state === self::REJECTED) { - try { - if ($onRejected === null) { - $promise->reject($this->result); - } else { - $promise->resolve($onRejected($this->result)); - } - } catch (\Throwable $e) { - $promise->reject($e); - } - } - }); - } - - $this->waiting = []; - } - - /** @return \SplQueue */ - public static function getQueue(): \SplQueue - { - static $queue; - - return $queue ??= new \SplQueue(); - } - /** * @param (callable(mixed): mixed)|null $onFulfilled * @param (callable(\Throwable): mixed)|null $onRejected @@ -186,31 +126,65 @@ public static function getQueue(): \SplQueue */ public function then(?callable $onFulfilled = null, ?callable $onRejected = null): self { - if ($this->state === self::REJECTED && $onRejected === null) { + if ($this->state === self::REJECTED + && $onRejected === null + ) { return $this; } - if ($this->state === self::FULFILLED && $onFulfilled === null) { + if ($this->state === self::FULFILLED + && $onFulfilled === null + ) { return $this; } - $tmp = new self(); - $this->waiting[] = [$tmp, $onFulfilled, $onRejected]; + $child = new self(); + + $this->waiting[] = [$child, $onFulfilled, $onRejected]; if ($this->state !== self::PENDING) { $this->enqueueWaitingPromises(); } - return $tmp; + return $child; } - /** - * @param callable(\Throwable): mixed $onRejected - * - * @throws InvariantViolation - */ - public function catch(callable $onRejected): self + /** @throws InvariantViolation */ + private function enqueueWaitingPromises(): void { - return $this->then(null, $onRejected); + if ($this->state === self::PENDING) { + throw new InvariantViolation('Cannot enqueue derived promises when parent is still pending.'); + } + + $waiting = $this->waiting; + if ($waiting === []) { + return; + } + + $this->waiting = []; + + $result = $this->result; + + SyncPromiseQueue::enqueue(static function () use ($waiting, $result): void { + foreach ($waiting as [$child, $onFulfilled, $onRejected]) { + try { + if ($result instanceof \Throwable) { + if ($onRejected === null) { + $child->reject($result); + } else { + $child->resolve($onRejected($result)); + } + } else { + $child->resolve( + $onFulfilled === null + ? $result + : $onFulfilled($result) + ); + } + } catch (\Throwable $e) { + $child->reject($e); + } + } + }); } } diff --git a/src/Executor/Promise/Adapter/SyncPromiseAdapter.php b/src/Executor/Promise/Adapter/SyncPromiseAdapter.php index 19ea4c3de..d59151c8c 100644 --- a/src/Executor/Promise/Adapter/SyncPromiseAdapter.php +++ b/src/Executor/Promise/Adapter/SyncPromiseAdapter.php @@ -23,10 +23,10 @@ public function isThenable($value): bool public function convertThenable($thenable): Promise { if (! $thenable instanceof SyncPromise) { - // End-users should always use Deferred (and don't use SyncPromise directly) - $deferred = Deferred::class; + // End-users should always use Deferred, not SyncPromise directly + $deferredClass = Deferred::class; $safeThenable = Utils::printSafe($thenable); - throw new InvariantViolation("Expected instance of {$deferred}, got {$safeThenable}"); + throw new InvariantViolation("Expected instance of {$deferredClass}, got {$safeThenable}."); } return new Promise($thenable, $this); @@ -35,10 +35,10 @@ public function convertThenable($thenable): Promise /** @throws InvariantViolation */ public function then(Promise $promise, ?callable $onFulfilled = null, ?callable $onRejected = null): Promise { - $adoptedPromise = $promise->adoptedPromise; - assert($adoptedPromise instanceof SyncPromise); + $syncPromise = $promise->adoptedPromise; + assert($syncPromise instanceof SyncPromise); - return new Promise($adoptedPromise->then($onFulfilled, $onRejected), $this); + return new Promise($syncPromise->then($onFulfilled, $onRejected), $this); } /** @@ -47,18 +47,18 @@ public function then(Promise $promise, ?callable $onFulfilled = null, ?callable */ public function create(callable $resolver): Promise { - $promise = new SyncPromise(); + $syncPromise = new SyncPromise(); try { $resolver( - [$promise, 'resolve'], - [$promise, 'reject'] + [$syncPromise, 'resolve'], + [$syncPromise, 'reject'] ); } catch (\Throwable $e) { - $promise->reject($e); + $syncPromise->reject($e); } - return new Promise($promise, $this); + return new Promise($syncPromise, $this); } /** @@ -67,9 +67,9 @@ public function create(callable $resolver): Promise */ public function createFulfilled($value = null): Promise { - $promise = new SyncPromise(); + $syncPromise = new SyncPromise(); - return new Promise($promise->resolve($value), $this); + return new Promise($syncPromise->resolve($value), $this); } /** @@ -78,9 +78,9 @@ public function createFulfilled($value = null): Promise */ public function createRejected(\Throwable $reason): Promise { - $promise = new SyncPromise(); + $syncPromise = new SyncPromise(); - return new Promise($promise->reject($reason), $this); + return new Promise($syncPromise->reject($reason), $this); } /** @@ -114,10 +114,11 @@ static function ($value) use (&$result, $index, &$count, &$resolveAllWhenFinishe }, [$all, 'reject'] ); - } else { - $result[$index] = $promiseOrValue; - ++$count; + continue; } + + $result[$index] = $promiseOrValue; + ++$count; } $resolveAllWhenFinished(); @@ -135,16 +136,12 @@ static function ($value) use (&$result, $index, &$count, &$resolveAllWhenFinishe public function wait(Promise $promise) { $this->beforeWait($promise); - $taskQueue = SyncPromise::getQueue(); $syncPromise = $promise->adoptedPromise; assert($syncPromise instanceof SyncPromise); - while ( - $syncPromise->state === SyncPromise::PENDING - && ! $taskQueue->isEmpty() - ) { - SyncPromise::runQueue(); + while ($syncPromise->state === SyncPromise::PENDING) { + SyncPromiseQueue::run(); $this->onWait($promise); } @@ -156,7 +153,7 @@ public function wait(Promise $promise) throw $syncPromise->result; } - throw new InvariantViolation('Could not resolve promise'); + throw new InvariantViolation('Could not resolve promise.'); } /** Execute just before starting to run promise completion. */ diff --git a/src/Executor/Promise/Adapter/SyncPromiseQueue.php b/src/Executor/Promise/Adapter/SyncPromiseQueue.php new file mode 100644 index 000000000..b47d01f0a --- /dev/null +++ b/src/Executor/Promise/Adapter/SyncPromiseQueue.php @@ -0,0 +1,46 @@ +enqueue($task); + } + + /** Process all queued promises until the queue is empty. */ + public static function run(): void + { + $queue = self::queue(); + while (! $queue->isEmpty()) { + $task = $queue->dequeue(); + $task(); + } + } + + public static function isEmpty(): bool + { + return self::queue()->isEmpty(); + } + + public static function count(): int + { + return self::queue()->count(); + } + + /** @return \SplQueue */ + private static function queue(): \SplQueue + { + /** @var \SplQueue|null $queue */ + static $queue; + + return $queue ??= new \SplQueue(); + } +} diff --git a/src/Executor/Promise/Promise.php b/src/Executor/Promise/Promise.php index c50886af2..28db6fbaa 100644 --- a/src/Executor/Promise/Promise.php +++ b/src/Executor/Promise/Promise.php @@ -25,7 +25,8 @@ class Promise public function __construct($adoptedPromise, PromiseAdapter $adapter) { if ($adoptedPromise instanceof self) { - throw new InvariantViolation('Expecting promise from adapted system, got ' . self::class); + $selfClass = self::class; + throw new InvariantViolation("Expected promise from adapted system, got {$selfClass}."); } $this->adoptedPromise = $adoptedPromise; diff --git a/tests/Executor/DeferredFieldsTest.php b/tests/Executor/DeferredFieldsTest.php index a66a44952..c6b6867ef 100644 --- a/tests/Executor/DeferredFieldsTest.php +++ b/tests/Executor/DeferredFieldsTest.php @@ -668,4 +668,287 @@ private function assertPathsMatch(array $expectedPaths): void self::assertContains($expectedPath, $this->paths, 'Missing path: ' . json_encode($expectedPath, JSON_THROW_ON_ERROR)); } } + + /** + * Test that DataLoader-style batching works correctly with many Deferred objects. + * + * DataLoaders work by: + * 1. Collecting all entity IDs during field resolution (buffering) + * 2. Making a single batch query when the queue is processed + * 3. Distributing results to waiting promises + * + * This test verifies that all IDs are collected before any batch is executed, + * which is essential for the DataLoader pattern to work correctly. + * + * The test creates 600 items to exceed any reasonable batch size threshold, + * ensuring that incremental processing (if implemented) doesn't break batching. + * + * @see https://github.com/webonyx/graphql-php/issues/1803 + * @see https://github.com/webonyx/graphql-php/issues/972 + */ + public function testDataLoaderPatternWithManyDeferredObjects(): void + { + $itemCount = 600; + + $authorData = []; + for ($i = 1; $i <= 100; ++$i) { + $authorData[$i] = ['id' => $i, 'name' => "Author {$i}"]; + } + + $bookData = []; + for ($i = 1; $i <= $itemCount; ++$i) { + $bookData[$i] = [ + 'id' => $i, + 'title' => "Book {$i}", + 'authorId' => ($i % 100) + 1, + ]; + } + + $authorBuffer = []; + $loadedAuthors = []; + $batchLoadCount = 0; + + $loadAuthor = function (int $authorId) use (&$authorBuffer, &$loadedAuthors, &$batchLoadCount, $authorData): Deferred { + $authorBuffer[] = $authorId; + + return new Deferred(static function () use ($authorId, &$authorBuffer, &$loadedAuthors, &$batchLoadCount, $authorData): ?array { + if ($authorBuffer !== []) { + ++$batchLoadCount; + foreach ($authorBuffer as $id) { + $loadedAuthors[$id] = $authorData[$id] ?? null; + } + $authorBuffer = []; + } + + return $loadedAuthors[$authorId] ?? null; + }); + }; + + $authorType = new ObjectType([ + 'name' => 'Author', + 'fields' => [ + 'id' => Type::int(), + 'name' => Type::string(), + ], + ]); + + $bookType = new ObjectType([ + 'name' => 'Book', + 'fields' => [ + 'id' => Type::int(), + 'title' => Type::string(), + 'author' => [ + 'type' => $authorType, + 'resolve' => static fn (array $book) => $loadAuthor($book['authorId']), + ], + ], + ]); + + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'books' => [ + 'type' => Type::nonNull(Type::listOf(Type::nonNull($bookType))), + 'resolve' => static fn (): array => array_values($bookData), + ], + ], + ]); + + $schema = new Schema(['query' => $queryType]); + $query = Parser::parse('{ books { id title author { id name } } }'); + + $result = Executor::execute($schema, $query); + $resultArray = $result->toArray(); + + self::assertArrayNotHasKey('errors', $resultArray, 'Query should not produce errors'); + self::assertArrayHasKey('data', $resultArray); + self::assertCount($itemCount, $resultArray['data']['books']); + + $nullAuthorCount = 0; + foreach ($resultArray['data']['books'] as $book) { + if ($book['author'] === null) { + ++$nullAuthorCount; + } + } + + self::assertSame( + 0, + $nullAuthorCount, + "Expected 0 null authors, but found {$nullAuthorCount}. " + . 'This indicates that batch loading was triggered before all IDs were collected.' + ); + + self::assertSame( + 1, + $batchLoadCount, + "Expected exactly 1 batch load, but got {$batchLoadCount}. " + . 'Multiple batch loads indicate that the queue was processed before all Deferred objects were created.' + ); + } + + /** + * Test nested Deferred fields with DataLoader pattern. + * + * Reproduces the bug from https://github.com/webonyx/graphql-php/issues/1803 where nested lists returned null + * when using DataLoader-style batching with many items. + * + * The scenario: eventDays -> event -> eventDays + * Each level uses Deferred with buffered batch loading. + * + * @see https://github.com/webonyx/graphql-php/issues/1803 + */ + public function testNestedDataLoaderPattern(): void + { + $eventCount = 50; + $daysPerEvent = 15; + + $events = []; + for ($i = 1; $i <= $eventCount; ++$i) { + $events[$i] = ['id' => $i, 'name' => "Event {$i}"]; + } + + $eventDays = []; + $dayId = 1; + for ($eventId = 1; $eventId <= $eventCount; ++$eventId) { + for ($day = 1; $day <= $daysPerEvent; ++$day) { + $eventDays[$dayId] = [ + 'id' => $dayId, + 'eventId' => $eventId, + 'date' => '2025-01-' . str_pad((string) $day, 2, '0', STR_PAD_LEFT), + ]; + ++$dayId; + } + } + + $eventBuffer = []; + $loadedEvents = []; + $eventBatchCount = 0; + + $loadEvent = function (int $eventId) use (&$eventBuffer, &$loadedEvents, &$eventBatchCount, $events): Deferred { + $eventBuffer[] = $eventId; + + return new Deferred(static function () use ($eventId, &$eventBuffer, &$loadedEvents, &$eventBatchCount, $events): ?array { + if ($eventBuffer !== []) { + ++$eventBatchCount; + foreach ($eventBuffer as $id) { + $loadedEvents[$id] = $events[$id] ?? null; + } + $eventBuffer = []; + } + + return $loadedEvents[$eventId] ?? null; + }); + }; + + $eventDaysBuffer = []; + $loadedEventDays = []; + $eventDaysBatchCount = 0; + + $loadEventDays = function (int $eventId) use (&$eventDaysBuffer, &$loadedEventDays, &$eventDaysBatchCount, $eventDays): Deferred { + $eventDaysBuffer[] = $eventId; + + return new Deferred(static function () use ($eventId, &$eventDaysBuffer, &$loadedEventDays, &$eventDaysBatchCount, $eventDays): array { + if ($eventDaysBuffer !== []) { + ++$eventDaysBatchCount; + foreach ($eventDaysBuffer as $eId) { + $loadedEventDays[$eId] = array_values(array_filter( + $eventDays, + static fn (array $day): bool => $day['eventId'] === $eId + )); + } + $eventDaysBuffer = []; + } + + return $loadedEventDays[$eventId] ?? []; + }); + }; + + $eventType = null; + + $eventDayType = new ObjectType([ + 'name' => 'EventDay', + 'fields' => function () use (&$eventType, $loadEvent): array { + return [ + 'id' => Type::int(), + 'date' => Type::string(), + 'event' => [ + 'type' => $eventType, + 'resolve' => static fn (array $day) => $loadEvent($day['eventId']), + ], + ]; + }, + ]); + + $eventType = new ObjectType([ + 'name' => 'Event', + 'fields' => fn (): array => [ + 'id' => Type::int(), + 'name' => Type::string(), + 'eventDays' => [ + 'type' => Type::nonNull(Type::listOf(Type::nonNull($eventDayType))), + 'resolve' => static fn (array $event) => $loadEventDays($event['id']), + ], + ], + ]); + + $queryType = new ObjectType([ + 'name' => 'Query', + 'fields' => [ + 'eventDays' => [ + 'type' => Type::nonNull(Type::listOf(Type::nonNull($eventDayType))), + 'resolve' => static fn (): array => array_values($eventDays), + ], + ], + ]); + + $schema = new Schema(['query' => $queryType]); + $query = Parser::parse(' + { + eventDays { + id + date + event { + id + name + eventDays { + id + date + } + } + } + } + '); + + $result = Executor::execute($schema, $query); + $resultArray = $result->toArray(); + + self::assertArrayNotHasKey('errors', $resultArray, 'Query should not produce errors'); + self::assertArrayHasKey('data', $resultArray); + self::assertCount($eventCount * $daysPerEvent, $resultArray['data']['eventDays']); + + $nullEventCount = 0; + $nullNestedDaysCount = 0; + + foreach ($resultArray['data']['eventDays'] as $day) { + if ($day['event'] === null) { + ++$nullEventCount; + } elseif ($day['event']['eventDays'] === null) { + ++$nullNestedDaysCount; + } + } + + self::assertSame( + 0, + $nullEventCount, + "Expected 0 null events, but found {$nullEventCount}. " + . 'This indicates premature batch processing broke DataLoader batching.' + ); + + self::assertSame( + 0, + $nullNestedDaysCount, + "Expected 0 null nested eventDays, but found {$nullNestedDaysCount}. " + . 'This indicates premature batch processing broke nested DataLoader batching.' + ); + } } diff --git a/tests/Executor/Promise/SyncPromiseAdapterTest.php b/tests/Executor/Promise/SyncPromiseAdapterTest.php index 51c825a98..4c5465f9d 100644 --- a/tests/Executor/Promise/SyncPromiseAdapterTest.php +++ b/tests/Executor/Promise/SyncPromiseAdapterTest.php @@ -6,6 +6,7 @@ use GraphQL\Error\InvariantViolation; use GraphQL\Executor\Promise\Adapter\SyncPromise; use GraphQL\Executor\Promise\Adapter\SyncPromiseAdapter; +use GraphQL\Executor\Promise\Adapter\SyncPromiseQueue; use GraphQL\Executor\Promise\Promise; use PHPUnit\Framework\TestCase; @@ -67,7 +68,7 @@ public function testCreatePromise(): void } /** @param mixed $expectedNextValue */ - private static function assertValidPromise(Promise $promise, ?string $expectedNextReason, $expectedNextValue, string $expectedNextState): void + private static function assertValidPromise(Promise $promise, ?string $expectedNextReason, $expectedNextValue, int $expectedNextState): void { self::assertInstanceOf(SyncPromise::class, $promise->adoptedPromise); @@ -90,7 +91,7 @@ static function (\Throwable $reason) use (&$actualNextReason, &$onRejectedCalled self::assertFalse($onFulfilledCalled); self::assertFalse($onRejectedCalled); - SyncPromise::runQueue(); + SyncPromiseQueue::run(); if ($expectedNextState !== SyncPromise::PENDING) { if ($expectedNextReason === null) { diff --git a/tests/Executor/Promise/SyncPromiseTest.php b/tests/Executor/Promise/SyncPromiseTest.php index 9cf39109f..48872b1c1 100644 --- a/tests/Executor/Promise/SyncPromiseTest.php +++ b/tests/Executor/Promise/SyncPromiseTest.php @@ -4,6 +4,7 @@ use GraphQL\Error\InvariantViolation; use GraphQL\Executor\Promise\Adapter\SyncPromise; +use GraphQL\Executor\Promise\Adapter\SyncPromiseQueue; use GraphQL\Tests\TestCaseBase; final class SyncPromiseTest extends TestCaseBase @@ -14,7 +15,7 @@ final class SyncPromiseTest extends TestCaseBase * ?callable, * ?string, * ?string, - * string, + * int, * }> */ public static function fulfilledPromiseResolveData(): iterable @@ -23,27 +24,22 @@ public static function fulfilledPromiseResolveData(): iterable $onFulfilledReturnsSameValue = static fn ($value) => $value; - $onFulfilledReturnsOtherValue = static fn ($value): string => 'other-' . $value; + $onFulfilledReturnsOtherValue = static fn ($value): string => "other-{$value}"; $onFulfilledThrows = static function (): void { - throw new \Exception('onFulfilled throws this!'); + throw new \Exception('onFulfilled throws this.'); }; yield ['test-value', null, 'test-value', null, SyncPromise::FULFILLED]; yield [uniqid(), $onFulfilledReturnsNull, null, null, SyncPromise::FULFILLED]; yield ['test-value', $onFulfilledReturnsSameValue, 'test-value', null, SyncPromise::FULFILLED]; yield ['test-value-2', $onFulfilledReturnsOtherValue, 'other-test-value-2', null, SyncPromise::FULFILLED]; - yield ['test-value-3', $onFulfilledThrows, null, 'onFulfilled throws this!', SyncPromise::REJECTED]; + yield ['test-value-3', $onFulfilledThrows, null, 'onFulfilled throws this.', SyncPromise::REJECTED]; } /** @dataProvider fulfilledPromiseResolveData */ - public function testFulfilledPromiseCannotChangeValue( - string $resolvedValue, - ?callable $onFulfilled, - ?string $expectedNextValue, - ?string $expectedNextReason, - ?string $expectedNextState - ): void { + public function testFulfilledPromiseCannotChangeValue(string $resolvedValue): void + { $promise = new SyncPromise(); self::assertSame(SyncPromise::PENDING, $promise->state); @@ -51,18 +47,13 @@ public function testFulfilledPromiseCannotChangeValue( self::assertSame(SyncPromise::FULFILLED, $promise->state); $this->expectException(\Throwable::class); - $this->expectExceptionMessage('Cannot change value of fulfilled promise'); - $promise->resolve($resolvedValue . '-other-value'); + $this->expectExceptionMessage('Cannot change value of fulfilled promise.'); + $promise->resolve("{$resolvedValue}-other-value"); } /** @dataProvider fulfilledPromiseResolveData */ - public function testFulfilledPromiseCannotBeRejected( - string $resolvedValue, - ?callable $onFulfilled, - ?string $expectedNextValue, - ?string $expectedNextReason, - ?string $expectedNextState - ): void { + public function testFulfilledPromiseCannotBeRejected(string $resolvedValue): void + { $promise = new SyncPromise(); self::assertSame(SyncPromise::PENDING, $promise->state); @@ -70,7 +61,7 @@ public function testFulfilledPromiseCannotBeRejected( self::assertSame(SyncPromise::FULFILLED, $promise->state); $this->expectException(\Throwable::class); - $this->expectExceptionMessage('Cannot reject fulfilled promise'); + $this->expectExceptionMessage('Cannot reject fulfilled promise.'); $promise->reject(new \Exception('anything')); } @@ -84,7 +75,7 @@ public function testFulfilledPromise( ?callable $onFulfilled, $expectedNextValue, ?string $expectedNextReason, - ?string $expectedNextState + ?int $expectedNextState ): void { $promise = new SyncPromise(); self::assertSame(SyncPromise::PENDING, $promise->state); @@ -125,7 +116,7 @@ static function () use (&$onRejectedCalled): void { self::assertNotSame($nextPromise, $nextPromise2); } - SyncPromise::runQueue(); + SyncPromiseQueue::run(); self::assertValidPromise($nextPromise2, $expectedNextValue, $expectedNextReason, $expectedNextState); self::assertValidPromise($nextPromise3, $expectedNextValue, $expectedNextReason, $expectedNextState); @@ -140,7 +131,7 @@ private static function assertValidPromise( SyncPromise $promise, $expectedNextValue, ?string $expectedNextReason, - ?string $expectedNextState + ?int $expectedNextState ): void { $actualNextValue = null; $actualNextReason = null; @@ -161,7 +152,7 @@ static function (\Throwable $reason) use (&$actualNextReason, &$onRejectedCalled self::assertFalse($onFulfilledCalled); self::assertFalse($onRejectedCalled); - SyncPromise::runQueue(); + SyncPromiseQueue::run(); if ($expectedNextReason === null) { self::assertTrue($onFulfilledCalled); // @phpstan-ignore-line value is mutable @@ -176,7 +167,15 @@ static function (\Throwable $reason) use (&$actualNextReason, &$onRejectedCalled self::assertEquals($expectedNextState, $promise->state); } - /** @return iterable */ + /** + * @return iterable + */ public static function rejectedPromiseData(): iterable { $onRejectedReturnsNull = static fn () => null; @@ -200,13 +199,8 @@ public static function rejectedPromiseData(): iterable } /** @dataProvider rejectedPromiseData */ - public function testRejectedPromiseCannotChangeReason( - \Throwable $rejectedReason, - ?callable $onRejected, - ?string $expectedNextValue, - ?string $expectedNextReason, - string $expectedNextState - ): void { + public function testRejectedPromiseCannotChangeReason(\Throwable $rejectedReason): void + { $promise = new SyncPromise(); self::assertSame(SyncPromise::PENDING, $promise->state); @@ -214,18 +208,13 @@ public function testRejectedPromiseCannotChangeReason( self::assertSame(SyncPromise::REJECTED, $promise->state); $this->expectException(\Throwable::class); - $this->expectExceptionMessage('Cannot change rejection reason'); + $this->expectExceptionMessage('Cannot change rejection reason.'); $promise->reject(new \Exception('other-reason')); } /** @dataProvider rejectedPromiseData */ - public function testRejectedPromiseCannotBeResolved( - \Throwable $rejectedReason, - ?callable $onRejected, - ?string $expectedNextValue, - ?string $expectedNextReason, - string $expectedNextState - ): void { + public function testRejectedPromiseCannotBeResolved(\Throwable $rejectedReason): void + { $promise = new SyncPromise(); self::assertSame(SyncPromise::PENDING, $promise->state); @@ -233,7 +222,7 @@ public function testRejectedPromiseCannotBeResolved( self::assertSame(SyncPromise::REJECTED, $promise->state); $this->expectException(\Throwable::class); - $this->expectExceptionMessage('Cannot resolve rejected promise'); + $this->expectExceptionMessage('Cannot resolve rejected promise.'); $promise->resolve('anything'); } @@ -243,7 +232,7 @@ public function testRejectedPromise( ?callable $onRejected, ?string $expectedNextValue, ?string $expectedNextReason, - string $expectedNextState + int $expectedNextState ): void { $promise = new SyncPromise(); self::assertSame(SyncPromise::PENDING, $promise->state); @@ -255,14 +244,14 @@ public function testRejectedPromise( $promise->reject(new \Exception('other-reason')); self::fail('Expected exception not thrown'); } catch (\Throwable $e) { - self::assertSame('Cannot change rejection reason', $e->getMessage()); + self::assertSame('Cannot change rejection reason.', $e->getMessage()); } try { $promise->resolve('anything'); self::fail('Expected exception not thrown'); } catch (\Throwable $e) { - self::assertSame('Cannot resolve rejected promise', $e->getMessage()); + self::assertSame('Cannot resolve rejected promise.', $e->getMessage()); } $nextPromise = $promise->then( @@ -296,7 +285,7 @@ static function () use (&$onFulfilledCalled): void { self::assertNotSame($nextPromise, $nextPromise2); } - SyncPromise::runQueue(); + SyncPromiseQueue::run(); self::assertValidPromise($nextPromise2, $expectedNextValue, $expectedNextReason, $expectedNextState); self::assertValidPromise($nextPromise3, $expectedNextValue, $expectedNextReason, $expectedNextState); @@ -311,7 +300,7 @@ public function testPendingPromise(): void $promise->resolve($promise); self::fail('Expected exception not thrown'); } catch (\Throwable $e) { - self::assertSame('Cannot resolve promise with self', $e->getMessage()); + self::assertSame('Cannot resolve promise with self.', $e->getMessage()); self::assertSame(SyncPromise::PENDING, $promise->state); } @@ -371,15 +360,15 @@ public function testPendingPromiseThen(): void self::assertSame(SyncPromise::PENDING, $promise->state); self::assertSame(SyncPromise::PENDING, $nextPromise->state); - // Make sure that it queues derivative promises until resolution: + // Make sure that it queues derivative promises until resolution $onFulfilledCount = 0; - $onRejectedCount = 0; - $onFulfilled = static function ($value) use (&$onFulfilledCount): int { + $onFulfilled = static function () use (&$onFulfilledCount): int { ++$onFulfilledCount; return $onFulfilledCount; }; + $onRejectedCount = 0; $onRejected = static function ($reason) use (&$onRejectedCount): void { ++$onRejectedCount; @@ -390,12 +379,12 @@ public function testPendingPromiseThen(): void $nextPromise3 = $promise->then($onFulfilled, $onRejected); $nextPromise4 = $promise->then($onFulfilled, $onRejected); - self::assertCount(0, SyncPromise::getQueue()); + self::assertSame(0, SyncPromiseQueue::count()); self::assertSame(0, $onFulfilledCount); self::assertSame(0, $onRejectedCount); $promise->resolve(1); - self::assertCount(4, SyncPromise::getQueue()); + self::assertSame(1, SyncPromiseQueue::count()); self::assertSame(0, $onFulfilledCount); // @phpstan-ignore-line side-effects self::assertSame(0, $onRejectedCount); // @phpstan-ignore-line side-effects self::assertSame(SyncPromise::PENDING, $nextPromise->state); @@ -403,8 +392,8 @@ public function testPendingPromiseThen(): void self::assertSame(SyncPromise::PENDING, $nextPromise3->state); self::assertSame(SyncPromise::PENDING, $nextPromise4->state); - SyncPromise::runQueue(); - self::assertCount(0, SyncPromise::getQueue()); + SyncPromiseQueue::run(); + self::assertSame(0, SyncPromiseQueue::count()); self::assertSame(3, $onFulfilledCount); // @phpstan-ignore-line side-effects self::assertSame(0, $onRejectedCount); // @phpstan-ignore-line side-effects self::assertValidPromise($nextPromise, 1, null, SyncPromise::FULFILLED); @@ -415,7 +404,7 @@ public function testPendingPromiseThen(): void public function testRunEmptyQueue(): void { - SyncPromise::runQueue(); + SyncPromiseQueue::run(); $this->assertDidNotCrash(); } }