Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Entity cache: only cache data if the subgraph returns a Cache-Control header or if a TTL is configured #4880

Closed
Geal opened this issue Mar 28, 2024 · 0 comments · Fixed by #4882
Assignees

Comments

@Geal
Copy link
Contributor

Geal commented Mar 28, 2024

the intent was to cache data from subgraphs only if a Cache-Control header was returned, but there is currently a bug, where all of the data is cached even when the header is not present, and if the TTL is not configured, the data will not expire.
There is also confusion on what should be done if the TTL is configured. So, to nail down how caching should behave:

  • if not configured on any subgraphs, do not cache anything
  • enabled option, boolean, activates caching, defaults to false. It can be overridden per subgraph, so if it is set to true and a subgraph sets it to false, then all subgraph will be cached except for that specific subgraph. At this point caching is still opt-in on the part of the subgraph, it has to provide a Cache-Control header
  • ttl option, duration. A global TTL can be configured under the redis option. It can also be overridden per subgraph. If a subgraph returns a Cache-Control header without a TTL, then we will get that TTL from configuration. If the header specifies a max-age, then we will use that for expiration
  • add an option always_cache, to cache responses even when the subgraph does not provide a Cache-Control header

We also need a reasonable default TTL for caching, to make sure we can't have data stored indefinitely

In the future, once the router has more ability to recognize which parts of the schema should be cached:

  • if enabled, globally or per subgraph:
  • if the subgraph does not provide a Cache-Control header, rely on the indications from the schema
  • if the subgraph provides a Cache-Control header, follow what the header says
@Geal Geal closed this as completed in #4882 Apr 4, 2024
Geal added a commit that referenced this issue Apr 4, 2024
Fix #4880 

The entity cache plugin intended to require a `Cache-Control` header
from the subgraph to decide whether or not a response should be cached.
Unfortunately in the way tit was set up, all responses were stored.
The plugin now makes sure that the `Cache-Control` is there, and if a
subgraph does not provide it, then the aggregated `Cache-Control` header
sent to the client will contain `no-store`.

Additionally, the Router will now check that a TTL is configured for all
subgraphs, either in per subgraph configuration, or globally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant