Don't factor $context in to cache key
#12086
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously we were serializing `$context` into the cache key for graphql queries. While this was good for uniqueness it could make the caches _too_ unique. When the cache is first being generated, fields are collected and memoized which altered the `$context` variable. When two queries were run right after each other, the first cache would miss, generate the cache, alter the context and cause the second query to miss the cache as well. This solves that by ignoring the `$context` all together when generating the `$cacheKey`. The `$cacheKey` is already made up of a hash of the query, serialized variables and the operation name. The `$context` can't be altered without a plugin so it seems overkill to include within the `$cacheKey`. Worth noting that this could cause some confusing situations in development if a developer is changing the context via module or plugin but doesn't change the query. With this change and caching enabled they would get stale results.
|
I did some testing, everything from editing entry types, globals, adding, renaming and removing fields and the cache still seems to work as expected on my testing environment. In case there is a situation where the context was the deciding factor I personally would consider replacing it with |
|
@brandonkelly this is ready for you now. Thank you @Wiejeben for testing it out! 🙏 |
|
Merged for 4.3 🎉 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously we were serializing
$contextinto the cache key for graphql queries. This was good for uniqueness but it could make the caches too unique. When the cache is first being generated, fields are collected and memoized which altered the$contextvariable. When two queries were run right after each other, the first cache would miss, generate the cache, alter the context and cause the second query to miss the cache as well (#11994).This solves that by ignoring the
$contextall together when generating the$cacheKey. The$cacheKeyis already made up of a hash of the query, serialized variables and the operation name. The$contextcan't be altered without a plugin so it seems overkill to include within the$cacheKey.Worth noting that this could cause some confusing situations in development if a developer is changing the context via module or plugin but doesn't change the query. With this change and caching enabled they would get stale results.
Related issues
Fixes #11994