Cache result of validation - #1730
Conversation
spawnia
left a comment
spawnia
left a comment
There was a problem hiding this comment.
There are some tricky details to figure out when implementing this. I really value putting in the time now to properly document them now to validate the design. I am going to try implenting the validation cache in https://github.com/nuwave/lighthouse and use that to battle-test it in one of my projects to see if anything else comes up.
|
I just found that Lighthouse already implements validation caching, see nuwave/lighthouse#2603. In general, the approach to hashing to schema and hashing the query string is the same, but there are some differences:
To resolve 1., perhaps we should add multi-phase validation to this library or some kind of special treatment for I am going to try adding a pull request to Lighthouse soon that implements its validation cache feature using the facilities offered through this pull request and perhaps extend it where its lacking. |
|
Not sure I follow. I don't use Lighthouse, and I'm not familiar with it. What does it have to do with what I'm doing in this PR? |
Lighthouse is built atop this library. It already implements a validation cache that has proven to be effective. I would like to take the lessons from the implementation effort we made there to inform the implementation in this library. This is to ensure that I can replace the custom implementation in Lighthouse with what we provide here, and we are not missing anything. |
|
I see. Lighthouse implements its own high-level query flow, and uses I'll leave you to it. |
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implement the ValidationCache interface from webonyx/graphql-php#1730 to improve validation result caching with automatic cache invalidation. Cache key now includes: - Library versions (webonyx/graphql-php and nuwave/lighthouse) - Schema hash - Query hash - Rule configuration hash (max_query_depth, disable_introspection) This eliminates the need for manual cache clearing when upgrading graphql-php or lighthouse, as the cache auto-invalidates on version changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Temporarily depend on the validation-cache branch from webonyx/graphql-php to test the ValidationCache interface integration. This will be updated to a proper version constraint once webonyx/graphql-php#1730 is merged and released. Note: This will require a major version bump for Lighthouse. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
I see you're working on stuff around this, which is great. Holler if I can be of any help. |
There was a problem hiding this comment.
Pull request overview
Adds an opt-in mechanism to cache successful GraphQL document validation results, allowing repeated executions of the same query (against the same schema/rules) to skip the validation step for performance gains.
Changes:
- Introduces
GraphQL\Validator\ValidationCacheand threads an optional cache instance throughGraphQL::executeQuery(),GraphQL::promiseToExecute(), andDocumentValidator::validate(). - Adds a PSR-16-based cache adapter and a spy adapter for testing, plus a new executor test asserting that validation is cached across repeated calls.
- Documents the new validation caching feature and updates class reference signatures; adds dev dependencies used by the new test.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/Validator/ValidationCache.php |
Defines the new cache interface used to short-circuit successful validations. |
src/Validator/DocumentValidator.php |
Implements validation short-circuiting and marking validated results. |
src/GraphQL.php |
Adds optional cache parameter and passes it into validation. |
src/Server/ServerConfig.php |
Adjusts PHPDoc types for validation rules. |
tests/PsrValidationCacheAdapter.php |
Adds a PSR-16-based reference adapter used by tests/docs. |
tests/Executor/TestClasses/SpyValidationCacheAdapter.php |
Adds spy wrapper to count cache method invocations. |
tests/Executor/ValidationWithCacheTest.php |
Adds test verifying validation runs once across repeated executions when cache is provided. |
docs/executing-queries.md |
Adds “Validation Caching” documentation and sample adapter implementation. |
docs/class-reference.md |
Updates documented method signatures to include the new cache parameter. |
composer.json |
Adds dev dependencies (psr/simple-cache, symfony/cache) used by the new test. |
Comments suppressed due to low confidence (2)
src/Server/ServerConfig.php:131
- The PHPDoc type for validation rules was widened to
array|callable|null, but the established phpstan type alias still documents this asarray<ValidationRule>|null|callable(...): array<ValidationRule>(seeServerConfig.php:31). Keeping the PHPDoc specific improves generated docs/IDE help and avoids implying arbitrary arrays are accepted.
/**
* @var array|callable|null
*
* @phpstan-var ValidationRulesOption
*/
private $validationRules;
src/Server/ServerConfig.php:331
- Same as the property PHPDoc above: the return PHPDoc was widened to
array|callable|null, which loses theValidationRuleelement type information that is documented elsewhere in this class (ServerConfig.php:31).
/**
* @return array|callable|null
*
* @phpstan-return ValidationRulesOption
*/
public function getValidationRules()
{
return $this->validationRules;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isset($cache) | ||
| && $cache->isValidated($schema, $ast, $rules) | ||
| ) { | ||
| return []; | ||
| } |
| if (isset($cache) | ||
| && $errors === [] | ||
| ) { | ||
| $cache->markValidated($schema, $ast, $rules); | ||
| } |
| public function isValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): bool | ||
| { | ||
| $key = $this->buildKey($schema, $ast); | ||
|
|
||
| return $this->cache->has($key); // @phpstan-ignore missingType.checkedException (annotated as a union with Throwable) | ||
| } |
| public function markValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): void | ||
| { | ||
| $key = $this->buildKey($schema, $ast); | ||
|
|
||
| $this->cache->set($key, true, $this->ttlSeconds); // @phpstan-ignore missingType.checkedException (annotated as a union with Throwable) | ||
| } |
| public function isValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): bool | ||
| { | ||
| ++$this->isValidatedCalls; | ||
|
|
||
| return parent::isValidated($schema, $ast); | ||
| } |
| public function markValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): void | ||
| { | ||
| ++$this->markValidatedCalls; | ||
|
|
||
| parent::markValidated($schema, $ast); | ||
| } |
| * Reference implementation of ValidationCache using PSR-16 cache. | ||
| * | ||
| * @see GraphQl\Tests\PsrValidationCacheAdapter | ||
| */ |
| public function isValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): bool | ||
| { | ||
| $key = $this->buildKey($schema, $ast); | ||
| return $this->cache->has($key); | ||
| } | ||
|
|
||
| public function markValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): void | ||
| { | ||
| $key = $this->buildKey($schema, $ast); | ||
| $this->cache->set($key, true, $this->ttlSeconds); | ||
| } |
|
@spawnia are you still planning on merging this at some point? Did you want me to address all these copilot comments? |
|
I still see it as a useful feature addition, but I don't want to make promises that I will merge. I did a big mechanical sweep over all open PRs (merge master, fix mechanical issues, request automated reviews) to try and refine them to a point where my judgement is needed again. |
|
Well, let me know if you decide to move forward (I don't personally need it, as I'm halfway through migrating my entire codebase to Node.js, anyway). |
Heyo, I remember there was some discussion about possibly disabling document validation checks a while back. I finally got around to looking into it and realized to my horror that it was indeed gobbling up a lot of resources in my app.
I don't know if you folks ever really agreed on a plan, but I thought I'd float this simple caching solution that leaves it up to the user.
Let me know what you think. If you like the general direction I can do a little polish and write better tests.