coverage: 84.969% (-0.01%) from 84.981% when pulling 5649e04 on markkelnar:fix/cache-key-url into 027323f on wp-graphql:develop.

Code Climate has analyzed commit 5649e04 and detected 0 issues on this pull request.

View more on Code Climate.

@markkelnar we might need to revert this.

The full endpoint is returned to allow for multi-tenant caches to not accidentally purge the incorrect thing.

see: varnish/varnish-modules#41 (comment)

For WordPress instances that are set up as sub-directory multisites (site.com/site-1/graphql, site.com/site-2/graphql), caching clients (Varnish, etc) can prefix the cache keys with the graphql endpoint to ensure they're cached uniquely.

If we use the X-GraphQL-Url to prefix our keys, and the value is only the host, not the full endpoint, then we could have collisions.

For example a query from site-1 and a query from site-2 could both return the same node (post:1, for example).

Currently they would be tagged as site.com/site-1/graphql:post:1 and site.com/site-2/graphql:post:1 respectively.

When a post in site 1 changes, it would then purge site.com/site-1/graphql:post:1 but not purge the site-2 post, and vis-versa.

If we go back to just the host, instead of the full endpoint, then we end up tagging both as: site.com:graphql:post:1 and an update to post:1 on either site will purge the cache for both sites.

If we expand to a multisite network of hundreds of sites that each have post:1 the problem gets worse.

I believe, unless I'm missing something about how Varnish + XKey, etc works, I think we probably need to keep the full endpoint here.

Some more context:

• Original Issue that lead to the X-GraphQL-Url being output: Expose GraphQL Endpoint in headers #2564
• Original PR: feat: add X-GraphQL-URL to headers of GraphQL Responses #2565

I see site_url() used in graphql_get_endpoint_url(). We should confirm that is indeed returning the value for public/queryable hostname/path, ex. domain.com/pathy/graphql/, that you are expecting and not the path to wp-admin path. We might want to use home_url() instead for the url of where requests are sent.

It might also make sense not to include the protocol (http/https) in the url cache header.

Without knowing those, we might not have a change in behavior for anyone using caching. If you are more comfortable reverting out this change and figuring that out before we try again, we can do that.

@markkelnar I set up a multisite using localwp and set it to use sub-directories.

My site is cleverly named multisite.local.

I created a 2nd site, cleverly named site-2. This site's GraphQL endpoint can be accessed at multisite.local/site-2/graphql

When I query data from site-2, the expected fully qualified endpoint is output in the headers:

I believe we will cause a regression for subdomain multisites if we make this change.