Cache result of validation

[shmax]

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.

[shmax]

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?

[spawnia]

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.

[shmax]

I see. Lighthouse implements its own high-level query flow, and uses DocumentValidator:validate directly. In the meantime, I modified my own code to just disable validation on production (I use persisted queries), so I'm not blocked.

I'll leave you to it.

[shmax]

I see you're working on stuff around this, which is great. Holler if I can be of any help.

[Copilot]

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\ValidationCache and threads an optional cache instance through GraphQL::executeQuery(), GraphQL::promiseToExecute(), and DocumentValidator::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 as array<ValidationRule>|null|callable(...): array<ValidationRule> (see ServerConfig.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 the ValidationRule element 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.

[shmax]

@spawnia are you still planning on merging this at some point? Did you want me to address all these copilot comments?

[spawnia]

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.

[shmax]

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).

[Copilot]

⚠️ Not ready to approve

There are correctness issues where cache keys and cache calls ignore or inconsistently handle the effective validation rules, which can make caching behave incorrectly.

Review details

Comments suppressed due to low confidence (9)

src/Validator/DocumentValidator.php:109

• Cache lookup happens before default rules are resolved, and it uses the original $rules argument (possibly null). This makes cache keys inconsistent with the actual rules used for validation (allRules()), so a cache hit can be missed and later markValidated may store under a different key. Resolve $finalRules first (and short-circuit when empty) and pass $finalRules to the cache.

        if (isset($cache)
            && $cache->isValidated($schema, $ast, $rules)
        ) {
            return [];
        }

src/Validator/DocumentValidator.php:138

• markValidated() is called with the original $rules argument instead of the resolved $finalRules actually used for validation, which can store under a different key than the lookup and undermine caching correctness.

        if (isset($cache)
            && $errors === []
        ) {
            $cache->markValidated($schema, $ast, $rules);
        }

tests/PsrValidationCacheAdapter.php:43

• isValidated() computes the cache key without including the $rules argument, even though buildKey() accepts rules and includes them in the hash. This causes different validation rule sets to share the same key.

    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)

tests/PsrValidationCacheAdapter.php:60

• markValidated() computes the cache key without including the $rules argument, so successful validations with different rule sets can overwrite/share the same cache entry.

    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)
    }

tests/Executor/TestClasses/SpyValidationCacheAdapter.php:20

• Spy adapter drops the $rules argument when delegating to the parent, so tests won’t reflect real behavior when custom validation rules are used (and it further hides key-generation bugs). Pass $rules through.

    public function isValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): bool
    {
        ++$this->isValidatedCalls;

        return parent::isValidated($schema, $ast);
    }

tests/Executor/TestClasses/SpyValidationCacheAdapter.php:27

• Spy adapter drops the $rules argument when delegating to the parent. Pass $rules through so the adapter observes the same cache-key inputs as production code.

    public function markValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): void
    {
        ++$this->markValidatedCalls;

        parent::markValidated($schema, $ast);
    }

docs/executing-queries.md:277

• The @see reference uses the wrong namespace casing (GraphQl vs GraphQL), so it doesn’t point to the actual test adapter class.

 * Reference implementation of ValidationCache using PSR-16 cache.
 *
 * @see GraphQl\Tests\PsrValidationCacheAdapter
 */

docs/executing-queries.md:296

• In the sample implementation, isValidated()/markValidated() call buildKey() without passing $rules, so the example contradicts the guidance that keys should depend on the validation rule set.

    public function isValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): bool
    {
        $key = $this->buildKey($schema, $ast);
        return $this->cache->has($key);
    }

composer.json:35

• Dev dependencies pin psr/simple-cache to v1 only and symfony/cache to ^5.4 only. This can cause unnecessary dependency conflicts for contributors (PSR simple-cache has v2/v3 widely used; Symfony cache has compatible newer majors on newer PHP). Consider widening constraints while remaining compatible with supported PHP versions.

    "psr/simple-cache": "^1.0",
    "react/http": "^1.6",
    "react/promise": "^2.0 || ^3.0",
    "rector/rector": "^2.0",
    "symfony/cache": "^5.4",

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Low

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.