fix(caching): Support caching of GET requests #1197
Conversation
There was a problem hiding this comment.
Would we be able to simplify this since Drupal core's CommandLineOrUnsafeMethod already prevents caching of non-HEAD/GET requests (since the HTTP standard discourages it). I think relying on Drupal core's behaviour here is fine, if someone for some reason has overwritten this then they probably know what they're doing or we can't help them anyway.
That would mean we can remove DenyPost and the service definition and changing the RouterProvider with the added test coverage would be sufficient.
tests/src/Kernel/GraphQLTestBase.php
Outdated
| protected function configureCachePolicy(int $max_age = 900) { | ||
| $this->container->set('dynamic_page_cache_request_policy', (new ChainRequestPolicy()) | ||
| ->addPolicy(new DenyPost())); | ||
| $this->container->set('page_cache_request_policy', (new ChainRequestPolicy()) | ||
| ->addPolicy(new NoSessionOpen($this->container->get('session_configuration'))) | ||
| ->addPolicy(new DenyPost())); | ||
| // Turn on caching. | ||
| $this->config('system.performance')->set('cache.page.max_age', $max_age)->save(); | ||
| } |
There was a problem hiding this comment.
Is this needed in the test? Kernel tests install the graphql module and it should load the services for core and the module on its own. I believe configuring this in the test bypasses actually testing our configuration (i.e. removing the module's service definition could still let the test pass even though we may not want it to).
There was a problem hiding this comment.
yes, it is needed - as per the comment core adds in the CommandLineOrUnsafeMethod by default which means in the test it is always DENY because kernel tests are command line based
All this is doing is recreating those services, but without CommandLineOrUnsafeMethod
There was a problem hiding this comment.
Alternatively, I could write these tests as a functional test, but I didn't know if there was a reason the module didn't have any yet, so didn't want to introduce one without checking
There was a problem hiding this comment.
That's what I get for doing reviews at 8 AM without coffee. Totally missing a wonderfully written comment.
There was a problem hiding this comment.
Thanks, I'm wondering why we had no_cache in the first place on our routes.
Could there be a scenario where the page cache would behave unexpectedly? Thinking of problems where private information of one user gets into the page cache and is then served to other users. But I think that should be covered by our cache metadata. We should check that the defaults are set up correctly or if we also need to do something there.
|
thanks @klausi - pushed those changes in terms of caching, the resolvers need to take care of adding all the required metadata this includes for the query too, e.g. cache context url.params.query needs to be added. we could do this by default if it was something everything needed - thoughts? can you trigger a test run? |
|
What's outstanding to get this over the line? I'm looking at migrating from JSON:API to GraphQL and caching would be one of the top priorities in making that decision. |
There was a problem hiding this comment.
I'm happy to have this committed :D
It's indeed the responsibility of all the resolvers of providing proper cacheability information. All the logic in the GraphQL module already exists to collect and propagate that.
With the insights I've gained in the past months I don't see this change as being much different from what happens already for POST'ed queries (i.e. if this would leak private info, so would our current POST implementations). Because although we do not cache at the request level, we cache with the exact same information at the result level.
There was a problem hiding this comment.
Also double-checked this and look good to me! GET requests use query parameters to pass the GraphQL query that would be in the POST body otherwise. Page caches like Varnish should take URL query parameters into account, so that should also not be a problem here.
Thank you!
Fixes #1196