fix: include validation rule names in cache key #1598

n1ru4l · 2022-12-16T19:54:56Z

Include the validation rule names within the operation cache key.

This prevents skipping conditional validation rules in other plugins.
Please make sure your validation rules always have a unique name property.

Note: I would also prefer to hash the validation names... but we currently have no way of doing cross-platform hashing in a sync and performant way. 😅 We could use something like https://github.com/emn178/js-sha1/blob/master/src/sha1.js instead for now.

Note: In a follow-up PR we should also create a hash key per GraphQLSchema instead of resetting the whole cache in case of a schema change.

changeset-bot · 2022-12-16T19:54:59Z

🦋 Changeset detected

Latest commit: 2932675

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@envelop/validation-cache	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2022-12-16T19:57:46Z

🚀 Website Preview

The latest changes to the website are available as preview in: https://81586f0e.envelop.pages.dev

github-actions · 2022-12-16T19:57:57Z

🚀 Snapshot Release (`alpha`)

The latest changes of this PR are available as alpha on npm (based on the declared changesets):

Package	Version	Info
`@envelop/validation-cache`	`5.0.5-alpha-20221216222744-232e3b3d`	npm ↗︎ unpkg ↗︎

theguild-bot · 2022-12-16T20:00:11Z

❌ Benchmark Failed

Performance regression detected: it seems like your Pull Request adds some extra latency to the GraphQL requests, or to envelop runtime.

If the performance regression is expected, please increase the failing threshold.

     ✓ no_errors
     ✓ expected_result

     checks.............................................: 100.00% ✓ 411848     ✗ 0     
     data_received......................................: 1.6 GB  11 MB/s
     data_sent..........................................: 90 MB   597 kB/s
     envelop_total......................................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:prom-tracing }............................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     event_loop_lag.....................................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-and-no-internal-tracing }...: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-cache-jit }.......................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:envelop-just-cache }......................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:graphql-js }..............................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     ✓ { mode:prom-tracing }............................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     http_req_blocked...................................: avg=4.02µs  min=1.1µs   med=2.29µs  max=12.02ms  p(90)=3.1µs   p(95)=3.8µs  
     http_req_connecting................................: avg=334ns   min=0s      med=0s      max=8.52ms   p(90)=0s      p(95)=0s     
     http_req_duration..................................: avg=6.79ms  min=279.4µs med=5.06ms  max=111.25ms p(90)=13.43ms p(95)=21.28ms
       { expected_response:true }.......................: avg=6.79ms  min=279.4µs med=5.06ms  max=111.25ms p(90)=13.43ms p(95)=21.28ms
     ✗ { mode:envelop-cache-and-no-internal-tracing }...: avg=5.96ms  min=525.5µs med=5.48ms  max=53.47ms  p(90)=9.9ms   p(95)=12.04ms
     ✓ { mode:envelop-cache-jit }.......................: avg=3.62ms  min=279.4µs med=2.88ms  max=33.36ms  p(90)=6.94ms  p(95)=9.97ms 
     ✗ { mode:envelop-just-cache }......................: avg=5.88ms  min=555.1µs med=5.21ms  max=53.24ms  p(90)=9.99ms  p(95)=12.09ms
     ✓ { mode:graphql-js }..............................: avg=10.13ms min=817.6µs med=8.64ms  max=78.9ms   p(90)=15.95ms p(95)=18.96ms
     ✗ { mode:prom-tracing }............................: avg=25.87ms min=2.56ms  med=22.56ms max=111.25ms p(90)=42.12ms p(95)=45.87ms
     http_req_failed....................................: 0.00%   ✓ 0          ✗ 205924
     http_req_receiving.................................: avg=69.25µs min=16µs    med=33.5µs  max=25.35ms  p(90)=50.1µs  p(95)=77.7µs 
     http_req_sending...................................: avg=54.5µs  min=7.7µs   med=12.8µs  max=25.81ms  p(90)=25.6µs  p(95)=39.1µs 
     http_req_tls_handshaking...........................: avg=0s      min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s     
     http_req_waiting...................................: avg=6.66ms  min=241.1µs med=4.94ms  max=111.2ms  p(90)=13.16ms p(95)=21.17ms
     http_reqs..........................................: 205924  1372.69005/s
     iteration_duration.................................: avg=7.27ms  min=559.9µs med=5.55ms  max=111.63ms p(90)=14.09ms p(95)=21.8ms 
     iterations.........................................: 205924  1372.69005/s
     vus................................................: 10      min=10       max=10  
     vus_max............................................: 20      min=20       max=20

…y added validation rules

saihaj · 2022-12-21T19:09:52Z

packages/plugins/validation-cache/src/index.ts

+      // This would cause an issue if we are constructing the cache key here already.
+      setValidationFn((...args) => {
+        let ruleKey = '';
+        if (Array.isArray(args[2])) {


we always have second arg here?

Well, if we don't have access - we cannot create a cache key. 😅
I assume because of the useEngine call, these rules always exist:

envelop/packages/core/src/plugins/use-engine.ts

Line 27 in 2932675

engine.specifiedRules?.map(addValidationRule);

saihaj · 2022-12-21T19:11:40Z

packages/plugins/validation-cache/src/index.ts

+        const key: string = ruleKey + (context[rawDocumentSymbol] ?? print(params.documentAST));
+        const cachedResult = resultCache.get(key);
+
+        if (cachedResult !== undefined) {


Suggested change

if (cachedResult !== undefined) {

if (!!cachedResult) {

n1ru4l · 2022-12-21T19:19:21Z

packages/plugins/validation-cache/src/index.ts

+          // Note: We could also order them... but that might be too much
+          for (const rule of args[2]) {
+            ruleKey = ruleKey + rule.name;
+          }


I would also prefer to hash the validation names... but we currently have no way of doing cross-platform hashing in a sync and performant way. 😅 We could use something like emn178/js-sha1@master/src/sha1.js instead for now.

Thoughts?

fix: include validation rule names in cache key

c6cf7d4

n1ru4l mentioned this pull request Dec 16, 2022

feat: disable introspection plugin dotansimha/graphql-yoga#2231

Merged

fix: refactor validation cache implementation to respect conditionall…

2932675

…y added validation rules

n1ru4l requested review from saihaj, dotansimha and ardatan December 18, 2022 12:19

saihaj reviewed Dec 21, 2022

View reviewed changes

saihaj approved these changes Dec 21, 2022

View reviewed changes

n1ru4l commented Dec 21, 2022

View reviewed changes

dotansimha approved these changes Dec 22, 2022

View reviewed changes

n1ru4l merged commit 21a758d into main Dec 22, 2022

n1ru4l deleted the fix-include-validation-rule-names-in-cache-key branch December 22, 2022 10:09

github-actions bot mentioned this pull request Dec 22, 2022

Upcoming Release Changes #1601

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: include validation rule names in cache key #1598

fix: include validation rule names in cache key #1598

n1ru4l commented Dec 16, 2022 •

edited

Loading

changeset-bot bot commented Dec 16, 2022 •

edited

Loading

github-actions bot commented Dec 16, 2022 •

edited

Loading

github-actions bot commented Dec 16, 2022 •

edited

Loading

theguild-bot commented Dec 16, 2022 •

edited

Loading

saihaj Dec 21, 2022

n1ru4l Dec 21, 2022

saihaj Dec 21, 2022

n1ru4l Dec 21, 2022

fix: include validation rule names in cache key #1598

fix: include validation rule names in cache key #1598

Conversation

n1ru4l commented Dec 16, 2022 • edited Loading

changeset-bot bot commented Dec 16, 2022 • edited Loading

🦋 Changeset detected

github-actions bot commented Dec 16, 2022 • edited Loading

🚀 Website Preview

github-actions bot commented Dec 16, 2022 • edited Loading

🚀 Snapshot Release (alpha)

theguild-bot commented Dec 16, 2022 • edited Loading

❌ Benchmark Failed

saihaj Dec 21, 2022

Choose a reason for hiding this comment

n1ru4l Dec 21, 2022

Choose a reason for hiding this comment

saihaj Dec 21, 2022

Choose a reason for hiding this comment

n1ru4l Dec 21, 2022

Choose a reason for hiding this comment

n1ru4l commented Dec 16, 2022 •

edited

Loading

changeset-bot bot commented Dec 16, 2022 •

edited

Loading

github-actions bot commented Dec 16, 2022 •

edited

Loading

github-actions bot commented Dec 16, 2022 •

edited

Loading

🚀 Snapshot Release (`alpha`)

theguild-bot commented Dec 16, 2022 •

edited

Loading