-
Notifications
You must be signed in to change notification settings - Fork 2k
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
maxAge of 0 results in no cache-control header #2605
Comments
I didn't encounter the issue mentioned, but the same code block might cause this issue. Basically if you set Unless your |
This is a pretty serious bug, no? Specifying At best this incorrectly conflates these two directives, causing sub-optimal caching, at worst it allows information (that has been explicitly marked as private) to be leaked/shared between requests from different users. If i'm understanding correctly, due to the way cache hints from multiple queries are combine (by min'ing the max ages), it's possible that adding a field to a query can cause the cache control header to be dropped, even if all the existing fields in the query are Eg. with this type: type User {
id: ID
name: String
email: String @cacheControl(scope: PRIVATE)
notes: String @cacheControl(scope: PRIVATE, maxAge: 0)
} This query will return with a cache control header of query {
User (id: "123") { id name email }
} But if you also hit the query {
User (id: "123") { id name email notes }
} This implicitly makes the entire result cacheable, even though it's pretty clear the developer intended the opposite. |
In case you don't want to wait for a fix, here a shameless plug: |
I came here with the same problem and while there is still no fix I thought I would share my "workaround" that can be adapted. The following works as an apollo plugin which sets the cache control header if it hasn't already been set. export class CacheControlFallbackPlugin<TContext> implements ApolloServerPlugin<TContext> {
public requestDidStart(): GraphQLRequestListener<TContext> {
return {
willSendResponse(requestContext: GraphQLRequestContextWillSendResponse<TContext>): ValueOrPromise<void> {
const existingCacheHeader = requestContext.response.http?.headers.get('Cache-Control');
if (!existingCacheHeader) {
// This can be tailored....
requestContext.response.http?.headers.set('Cache-Control', 'no-store, max-age=0');
}
},
};
}
} |
I can confirm @liamchilds approach is working, I'm using exactly the same in my project. |
The "workaround" seems reasonable. The approach Apollo Server's cache plugin has always taken was to only send cache headers for cacheable responses, on the assumption that clients wouldn't choose to cache responses without explicit header noting it's OK. If that assumption is wrong, fixing it with your own plugin seems reasonable. Alternatively, instead of looking at the existing header, you can just look at the request's cache policy and see if it's cacheable. The API for this is changing a bit in Apollo Server 3, where it'll be: export class CacheControlFallbackPlugin<TContext> implements ApolloServerPlugin<TContext> {
public async requestDidStart(): Promise<GraphQLRequestListener<TContext>> {
return {
async willSendResponse(requestContext: GraphQLRequestContextWillSendResponse<TContext>): Promise<void> {
if (!requestContext.overallCachePolicy.policyIfCacheable()) {
// This can be tailored....
requestContext.response.http?.headers.set('Cache-Control', 'no-store, max-age=0');
}
},
};
}
} In general my philosophy about the cache control plugin is that the main thing it does is calculate I think this is working as intended. |
It seems the docs have been rewritten 5 months ago to indicate that no header is sent if However, I still find this behaviour weird and misleading. The resulting reduced DX is pointed out by disclaimers in the documents, e.g. this one:
The use of the cache plugin is not very straight-forward in this respect. |
In an ideal world, how would Apollo Server know when to send a |
I would probably see The point is that a specific value (may it be |
I needed to be able to explicitly add max-age: 0, and not send header s if not set. For this i used a decorator, and a custom plugin. The decorator sets a context value, that the plugin checks it and does what is needed. The plugin is added to apollo-server plugins array. Note: Used typescript. Decorator:
Plugin:
Usage, resolver:
|
This is still so broken.
Removing the Documenting the broken behaviour doesn't make the behaviour any less broken. |
I don't really disagree with you. That said, a bit of a challenge here is that there does appear to be multiple desired outcomes here. I can see an argument for "as long as you have the (installed-by-default) cache control plugin installed (and haven't disabled HTTP header calculation), we should always set a cache-control header". But I also see @thehideki81 wants to only set max-age=0 if a field that explicitly uses CacheControl is invoked. So improving this for your use can might not be right for @thehideki81 's use case. And it does seem that the "always send a cache-control header" use case can be addressed by just setting That said, you do have a point that HTTP RFCs encourage everyone to set |
@molomby How would you feel about changing the behavior of |
(We could also keep the current semantics for |
As pointed out in #2605, browsers cache many GET requests by default, so "not cacheable" shouldn't be the same as "don't set a cache-control header". This PR makes the backwards-incompatible change to the cache control plugin to make it always set the cache-control header to something if it is enabled (with calculateHttpHeaders not set to false), perhaps `no-store`. (If some other plugin or error already set the header, it does not override it with `no-store`.) To restore AS3 behavior: ApolloServerPluginCacheControl({ calculateHttpHeaders: 'if-cacheable' }) Fixes #2605.
As pointed out in #2605, browsers cache many GET requests by default, so "not cacheable" shouldn't be the same as "don't set a cache-control header". This PR makes the backwards-incompatible change to the cache control plugin to make it always set the cache-control header to something if it is enabled (with calculateHttpHeaders not set to false), perhaps `no-store`. (If some other plugin or error already set the header, it does not override it with `no-store`.) To restore AS3 behavior: ApolloServerPluginCacheControl({ calculateHttpHeaders: 'if-cacheable' }) Fixes #2605.
OK, in AS4 we will set |
Problem
We're facing issues with persisted queries using
GET
requests and IE11. This lovely browser is serving responses without acache-control
header from its cache. While this is ok for most of our current application's queries, some queries should not be cached.We consider leaving out
cache-control
headers for amaxAge
setting of0
a bug. Also thescope
is ignored whileCache-Control: private
would be a totally reasonable header.Workaround
A workaround for us is to annotate specific types with a
maxAge
of1
second, which is ugly but works for these kind of requests (we don't expect the user to be faster, but in case he is, it results in glitches).Furthermore, this workaround for types containing other types is not that easy: as soon as another type without a cache hint is added to a type, the
cache-control
header is no longer sent to the browser because the defaultmaxAge
is0
and therefore lower than1
. We need to annotate all types used in a type as well, which results in a mess: some types by itself might benefit from a differentcacheControl
settings in certain situations.This code is responsible for leaving out the headers for situations when the lowest
maxAge
is0
:apollo-server/packages/apollo-cache-control/src/index.ts
Lines 171 to 178 in 8215787
Possible solution
A possible solution for our case would be to still send headers in case the lowest
maxAge
is0
.I'd be happy to submit a PR for a proper solution that enables consumers of
apollo-server
to specify amaxAge
of0
orscope
only if this is a reasonable approach.However, it seems more cache requirements are out there (eg. the old and long untouched #1295 as well as #1424, which might also work for our situation) so this solution might not fit to the overall roadmap. Unfortunately for us #2360 doesn't mention any plans for
cacheControl
.The text was updated successfully, but these errors were encountered: