envelop icon indicating copy to clipboard operation
envelop copied to clipboard

caching does not work if we have introspection query inside graphiql using express-graphql with @envelop/response-cache

Open xddq opened this issue 2 years ago • 4 comments

Issue workflow progress

Progress of the issue based on the Contributor Workflow

  • [x] 1. The issue provides a minimal reproduction available on Stackblitz.
    • Please install the latest @envelop/* packages that you are using.
    • Please make sure the reproduction is as small as possible.
  • [ ] 2. A failing test has been provided
  • [ ] 3. A local solution has been provided
  • [ ] 4. A pull request is pending review

Describe the bug

  • when having the text that is stored in ./graphiql-not-caching-requests in the graphiql playground the @envelop/response-cache plugin does not cache requests.
  • when having the text that is stored in ./graphiql-caching-requests it will

To Reproduce Steps to reproduce the behavior:

  • clone https://github.com/xddq/envelop-respones-cache-graphiql-issue
  • yarn
  • yarn build
  • yarn start
  • go to localhost:4000/graphql and copy paste the text of ./graphiql-not-caching-requests. Then run "hello" and see that it won't be cached.
  • go to localhost:4000/graphql and copy paste the text of ./graphiql-caching-requests. Then run "hello" and see that it will be cached.

Expected behavior

  • Cache results of the "hello" query in both cases

Environment:

  • OS:
  • NodeJS:
  • @envelop/* versions: 3.0.0
    • @envelop/core: 2.4.0

Additional context I am not sure why this is happening. One thing I did notice that when testing that the other caching plugins @envelop/parser-cache and @envelop/validation-cache still work as usual. I also don't think that this bug is severe, since it occurs only if we have the introspection query in graphiql.

xddq avatar Jul 12 '22 14:07 xddq

Introspection operations are never cached by default. This is documented on the useResponseCache page.

https://github.com/n1ru4l/envelop/blob/a15a6c1c7a0ff12c266f2078466a4829e30a94b4/packages/plugins/response-cache/src/plugin.ts#L176

n1ru4l avatar Jul 12 '22 15:07 n1ru4l

Ahhh, I did not mean the execution of the query operation "IntrospectionQuery". When the operation is "test" and the query contains "IntrospectionQuery" the result of the operation "test" won't be cached.

Maybe somewhere one could check if the operationName matched IntrospectionQuery instead of when seeing IntrospectionQuery instantly not caching. I have not looked deeper into it, just a blind guess.

If this behaviour is intended you can close the issue. Just wanted to mention it in case someone gets this behaviour and is surprised why suddenly caching does not work anymore just because there is IntrospectionQuery in the query one sends.

will-be-cached wont-be-cached

xddq avatar Jul 12 '22 15:07 xddq

We recommend only sending the actually used operation (and fragments) inside a query document in a single request. The hash is generated based on the whole incoming document. Also, the TTL is calculated on the whole incoming document.

I am pretty sure the implementation can be "improved" within this visit call here: https://github.com/n1ru4l/envelop/blob/a15a6c1c7a0ff12c266f2078466a4829e30a94b4/packages/plugins/response-cache/src/plugin.ts#L186

Contributions either for adjusting the behavior or documenting it are welcome!

A great first step here could be to send a pull request with a failing test!

n1ru4l avatar Jul 13 '22 08:07 n1ru4l

Yeah, thats just what happens in GraphiQL. I currently won't have much free time and therefore won't be able to fix the issue in the next weeks. I will write it down and perhaps go at it if its still up once I have time again.

xddq avatar Jul 16 '22 16:07 xddq