-
-
Notifications
You must be signed in to change notification settings - Fork 568
Cache result of validation #1730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 35 commits
35ce576
03db33d
c3a7eb4
a2c38f4
6782bad
6317931
73c9774
6d7ace0
708c85b
f35cb3c
d71e123
cf20ef3
60bffe6
40c000b
7d515fd
c2a82a4
51d1087
a745345
1277bb3
59a0cd7
6679cb8
8d21f38
de245ff
d166e41
4b3aafe
9683c02
02013c3
1e55f48
127b68f
b4865e9
4ec12df
707f257
4c3d419
b5fdd3e
5d9e714
b8c62dc
7a2b3dc
e8fedcc
c91420c
22e15cc
cbebfce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,20 +99,25 @@ public static function validate( | |
Schema $schema, | ||
DocumentNode $ast, | ||
?array $rules = null, | ||
?TypeInfo $typeInfo = null | ||
?TypeInfo $typeInfo = null, | ||
?ValidationCache $cache = null | ||
): array { | ||
$rules ??= static::allRules(); | ||
if (isset($cache)) { | ||
if ($cache->isValidated($schema, $ast, $rules)) { | ||
return []; | ||
} | ||
} | ||
|
||
if ($rules === []) { | ||
$finalRules = $rules ?? static::allRules(); | ||
if ($finalRules === []) { | ||
return []; | ||
} | ||
|
||
$typeInfo ??= new TypeInfo($schema); | ||
|
||
$context = new QueryValidationContext($schema, $ast, $typeInfo); | ||
|
||
$visitors = []; | ||
foreach ($rules as $rule) { | ||
foreach ($finalRules as $rule) { | ||
$visitors[] = $rule->getVisitor($context); | ||
} | ||
|
||
|
@@ -124,7 +129,13 @@ public static function validate( | |
) | ||
); | ||
|
||
return $context->getErrors(); | ||
$errors = $context->getErrors(); | ||
|
||
if (isset($cache) && $errors === []) { | ||
$cache->markValidated($schema, $ast, $rules); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch on not overwriting |
||
} | ||
|
||
return $errors; | ||
} | ||
|
||
/** | ||
|
@@ -273,7 +284,6 @@ public static function validateSDL( | |
?array $rules = null | ||
): array { | ||
$rules ??= self::sdlRules(); | ||
|
||
if ($rules === []) { | ||
return []; | ||
} | ||
|
spawnia marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
<?php declare(strict_types=1); | ||
|
||
namespace GraphQL\Validator; | ||
|
||
use GraphQL\Language\AST\DocumentNode; | ||
use GraphQL\Tests\PsrValidationCacheAdapter; | ||
use GraphQL\Type\Schema; | ||
use GraphQL\Validator\Rules\ValidationRule; | ||
|
||
/** | ||
* Implement this interface and pass an instance to GraphQL::executeQuery to enable caching of successful query validations. | ||
* | ||
* This can improve performance by skipping validation for known-good combinations of query, schema, and rules. | ||
* You are responsible for defining how cache keys are computed. | ||
* | ||
* Some things to keep in mind when generating keys: | ||
* - PHP's `serialize` method is fast, but can't handle certain structures such as closures. | ||
* - If your `schema` includes closures or is too large or complex to serialize, | ||
* consider using a build-time version number or environment-based fingerprint instead. | ||
* - Keep in mind that there are internal `rules` that are applied in addition to any you pass in, | ||
* and it's possible these may shift or expand as the library evolves, so it might make sense | ||
* to include the library version number in your keys. | ||
* | ||
* @see PsrValidationCacheAdapter for a simple reference implementation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Due to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figure devs will be savvy enough to either clone the repo or look on github, but I can remove it for now if you like. |
||
*/ | ||
interface ValidationCache | ||
{ | ||
/** | ||
* Determine whether the given schema/AST/rules set has already been successfully validated. | ||
* | ||
* This method should return true if the query has previously passed validation for the provided schema. | ||
* Only successful validations should be considered "cached" — failed validations are not cached. | ||
* | ||
* @param array<ValidationRule>|null $rules | ||
* | ||
* @return bool true if validation for the given schema + AST + rules is already known to be valid; false otherwise | ||
*/ | ||
public function isValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): bool; | ||
|
||
/** | ||
* @param array<ValidationRule>|null $rules | ||
* | ||
* Mark the given schema/AST/rules set as successfully validated. | ||
* | ||
* This is typically called after a query passes validation. | ||
* You should store enough information to recognize this combination on future requests. | ||
*/ | ||
public function markValidated(Schema $schema, DocumentNode $ast, ?array $rules = null): void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
<?php declare(strict_types=1); | ||
|
||
namespace GraphQL\Tests\Executor\TestClasses; | ||
|
||
use GraphQL\Language\AST\DocumentNode; | ||
use GraphQL\Tests\PsrValidationCacheAdapter; | ||
use GraphQL\Type\Schema; | ||
|
||
final class SpyValidationCacheAdapter extends PsrValidationCacheAdapter | ||
{ | ||
public int $isValidatedCalls = 0; | ||
|
||
public int $markValidatedCalls = 0; | ||
|
||
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); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean to expose this class to end users? The directory
/tests
and subsequently the namespaceGraphQL\Tests
is excluded from the composer package through.gitattributes
. My impression was that it serves only as a vehicle for our own unit tests and perhaps as an example implementation that we can point to.I would probably recommend users to implement the interface concretely, perhaps we can just add something like this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, this is just meant to be a sample. I'll touch that up.