[4.x] Make GET requests cacheable

[larowlan]

Problem statement

At present all GET requests end up with the following HTTP Header:

Cache-Control | must-revalidate, no-cache, private

This is because the route-provider sets no_cache: TRUE

However, we go to the effort of collecting CacheableMetadata throughout the whole resolver pipeline and even make sure to return CacheableJsonResponse objects from RequestController

Whilst this is used for the results cache, it isn't used at the response level because as soon as you add no_cache: TRUE - \Drupal\Core\PageCache\ResponsePolicy\DenyNoCacheRoutes kicks in and sets the cache-policy to DENY.

This is evaluated in \Drupal\Core\EventSubscriber\FinishResponseSubscriber::onRespond and as soon as it finds something that is not cacheable, it sets this HTTP Header, which effectively means any use of GET for queries can't be cached in a CDN.

Proposed resolution

1. Revert this commit and when adding back the custom request policy, have it only deny caching where the method is not GET
2. Expand the custom request policy so that is also a tagged with page_cache_response_policy too.
3. Add test coverage that GET requests are cacheable.
4. Add test coverage that POST requests are not cached.

If this sounds acceptable, let me know and I'll work on a pull-request

[klausi]

Makes sense to me. https://graphql.org/learn/serving-over-http/ explains how to use GET requests, I think we only use POST requests at jobiqo with our Apollo frontend stack. We only use GET requests for persisted queries.

I'm thinking about security implications regarding GET requests and CSRF. You must not allow mutations through GET requests, but maybe that is not enforced? Probably unrelated to this issue since this is only about caching GET request responses.

Our protection against CSRF is using JWT for authentication, which the browser does not send along automatically like a cookie.

[larowlan]

I agree re preventing mutations, I guess we could check for query - see the PR we could also use that for cache-contexts

[Sam152]

Following!

I also noticed this when doing a performance audit. I'm considering configuring Apollo with useGETForQueries, the module already support GET based queries, so I'm guessing if there was some unhandled CSRF issue with mutations, it'd already exist and be unrelated to this caching issue.

[Sam152]

Looking into this a bit more, I believe \Drupal\Core\PageCache\RequestPolicy\CommandLineOrUnsafeMethod will already cover us for not caching POST requests. I'm wondering if all that's needed is:

         ->addOptions([
-          'no_cache' => TRUE,
         ]);

...with maybe some testing to the global max age and maybe cacheable metadata is collected/applied to internal caches like page_cache or dynamic_page_cache?

[benjy]

The above worked for me and I was able to get caching working, one thing though, the current user producer adds a cacheable dependency on the current user which turns into a max-age 0 because the current user proxy isn't a cacheable dependency, instead of adding the cache context for user.

  public function resolve(FieldContext $field_context): AccountInterface {
    // Response must be cached based on current user as a cache context,
    // otherwise a new user would became a previous user.
    $field_context->addCacheableDependency($this->currentUser);
    return $this->currentUser;
  }```

[larowlan]

I almost feel that core should throw an exception there instead of setting it to zero.
You rarely want to add a cacheable dependency on something that doesn't implement the interface

[benjy]

Yeah agreed, they could just add the type hint to the method. I'd support that given finding max-age = 0's isn't always the easiest thing for people to debug.

[larowlan]

https://www.drupal.org/project/drupal/issues/3232018

[benjy]

Another issue here, if you're trying to make caching work for authenticated users then dynamic page cache will need to know that the url query arguments are cache contexts. I added this to my schema but it will need to be added in Server::getContext

  public function getCacheContexts() {
    return [
      'url.query_args:query',
      'url.query_args:variables',
      'url.query_args:operationName',
    ];
  }

Without this, all follow-on requests after your first cached response will return the same result.

[larowlan]

Yeah we got around that in our producer - but I think it would be good to try and address that globally in graphql.

Needs work for that I think