-
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
apollo-server-plugin-response-cache: cache hint not working #3559
Comments
I'm seeing exactly this problem also. I was working on adding response caching today and couldn't figure out why I wasn't seeing any caching happening. I'm glad I'm not going mad but also disappointed that response caching is apparently broken :( |
Having the same issue, even without using the |
Having the same issue. Any idea how to solve it? |
Not seeing any responses get cached with |
It's disappointing the cache only works if you set a global caching settings:
I am using I think it is a great idea to |
Have a similar problem. Some queries are cached. Others aren't. Caching doesn't work when I do this:
It does when I do:
I've been testing cache hits through https://engine.apollographql.com/. Is there an easier way to do this? |
The query that was cached fine doesn't use variables. The ones that didn't cache did. The variables didn't change but could this have caused a cache miss? |
The cache hint is actually working but not like expected. So if you have a cache hint of 60 but your default is 10, the cache-control header will be set to 10. |
@Jylth agree that the logic is not what you'd expect, but is technically documented correctly (but could probably be improved):
My initial expectation when testing this kind of setup:
would be for Destination.distance to cache for 60 seconds, and Destination.name to cache for 10. But the result is that the entire Destination type caches for the minimum maxAge across all fields, in this case 10. |
It is documented correctly but the result is not something usable. If you can't set a overall cache, the cache doesn't work at all. This is basically the issue. I wonder if someone from Apollo actively looks at the issues? |
This is a bug. The overall cache policy is documented correctly, yes, however the documentation is very specific that the default is that things are uncacheable:
The current behavior is that I will say that the overall design approach here is fantastic! This seems like it should be an easy thing to fix frankly. |
Absolutely, I couldn't agree more. It's designed perfectly and we hope it gets fixed. I'll send this thread to the Apollo support team in several channels. Hopefully we get an answer soon. |
defaultMaxAge is only supposed to be applied to root fields and object/interface fields (based on my memory and code inspection). The example in the initial issue above correctly applies a hint to the only root field which is also the only object field. So this should work and is a bug if it doesn't. |
I implemented |
@glasser That's much appreciated, I'll try to find some time soon to create a basic repo that illustrates the issue. In the meantime, I implemented a workaround which I'll post here for others. The workaround is to use a wrapper function around every resolver function, that checks for a cache hint and adds it if it doesn't exist. As long as you remember to use the wrapper on every query resolver, it works fine. Then you set
|
@jbarefoot How is that better than setting defaultMaxAge to 60? |
@glasser It means you can have a short default expiry and override with longer values. The code I posted only sets the expiry to 60 if In my experience this is a common way to use a server cache: The default expiry is pretty short or perhaps not cached (whatever is appropriate for the context). For data that is slowly changing, set the expiry case-by-base to longer values. |
That is not my understanding of how defaultMaxAge works (though of course, there may be a bug, that's why we're here). If you set defaultMaxAge to 5 but have a schema like
Then queries against someField will be cached for 50, queries against someOtherField will be cached for 5, and queries that hit both fields will also be cached for 5 because they contain some low-TTL data. Is that not what you're experiencing? The main feature that I think is missing here (other than features that would make this model more comprehensible/inspectable) is the ability to mark an object/interface field as "inherit parent maxAge" just like scalar fields do. But I don't understand how your example differs from your example without setting defaultMaxAge (unless this is what helps us narrow down what the bug is!). I am really looking forward to the clear example of this bug given by somebody on this thread! |
@glasser Thanks for the reply. I wasn't testing field level caching, so I'm not sure on that. I will try to test it later in my schema and let you know the result. My object example would be like this:
In the above case, with defaultMaxAge=5, if you fetch
Really I just want it to not use defaultMaxAge, at all, if a cache hint has been set, either via And sorry yes let me try to get you an example git repo later this week, I know this is difficult without seeing a concrete thing that doesn't work as expected. :) |
It is how it should work but now Btw it would be also great to have ability to set default scope on all queries to
|
My solution (for now) was to make custom @CacheControl decorator (with type-graphql):
Obviously Tested only with |
This was very confusing. I expected to be able to override |
I still do not understand what the complaint is in this bug and will launch into fixing it as soon as somebody presents a self-contained reproduction! |
@glasser I'm not sure exactly what you need for a self-contained reproduction, but I feel my initial comment on expected vs actual behaviour describes the issue most here are experiencing:
If I can offer anything further that might help, please let me know. |
As I said above: a full reproduction recipe: ie, a series of commands probably starting with git clone which allows me to see the bug without having to try to set up a server from scratch and guess all the details of what you tried. (For example, this should include a sample query and some way to tell that it's not cached (perhaps a resolver that returns random values).) |
@glasser my example is unlikely to be useful to you then, we're using the neo4j-graphql-js package which requires no resolvers to be written, just types defined. The amount of interest in this thread clearly indicates a problem though, so hopefully someone can move this forward by supplying what you need. |
Came across the same issue here that my schema cache hints were not being set and no cache headers sent out at all. Fixed it with the suggestion above of defining a defaultMaxAge
This is more of a hack than anything to have any sort of caching headers sent out but will surely cause issues down the line |
Facing similar issue,
But when I try the fine grain caching at resolvers I can see the maxAge set in HTTP response but the response doesn't seem to be cached as I can see DB queries on each request
Am I doing anything wrong here? |
Please provide a full executable reproduction example, as described in my previous comments on this issue. |
@glasser I might be wrong but I get the impression this is maybe more an issue with unclear documentation than an actual bug? I haven't had time to revisit this issue but now I just learned about datasources and I feel like that's one more caching parameter I need to juggle with… I think a comprehensive blog post or example app that clearly outlines the different caching methods available with Apollo Server and the pros and cons of each one would be helpful here. |
To follow up on my last post, I only just now finally understood that this is about setting cache hints so that Apollo can figure out whether the entire request is cacheable or not; and not telling Apollo to cache individual fields. In other words, if I have 9 cached fields and 1 uncacheable fields in my request I falsely expected after reading the documentation that those 9 fields would be cached and not trigger database, API, etc. requests. Whereas the truth (as I now understand it, I might still be wrong?) is that none of my 10 fields will be cached because the entire request is not cacheable. On the other hand, if I used Datasources for those 9 fields then those database, API, etc. requests would be cached independently of whether the full GraphQL can be cached or not. Again sorry if I'm wrong and just making things more confusing, but I suspect other people might've misunderstood this as well. |
@SachaG So there's two separate things. Apollo Server has a flexible way of assigning cache hints on a per-field basis. AS then uses that data to power several features. Right now, the only features that makes use of this data are full-response features, such as the full response cache (https://www.apollographql.com/docs/apollo-server/performance/caching/#saving-full-responses-to-a-cache) and HTTP response header calculations. One could imagine building out a partial response cache as well! In a lot of cases this will require a way for the server to know how to "jump" deep into a query to re-query just the changing data. We have not built that and I do not believe it is on any particular roadmap, but it is an imaginable way to make use of the detailed per-field cache data. Datasources is basically unrelated. It's a fancy word for "we built a caching HTTP client in Node, maybe that's useful for you when writing your resolvers". Also note that there are two different things called "data sources" in AS (the caching HTTP client thing and the interface that Apollo Gateway uses to talk to implementing services) that are also unrelated. |
Yes, I saw this issue about that: #4042
Good to know, thanks for the added details! |
Hi @SachaG About this part 👇
Do you know how I can implement this or what package are you referring to? I am using REST datasource so if I can do this, then I don't need the field/type level cache control. This would behave the same for 90% of my cases. FYI, not ttl as that is only for a request so not really useful across requests. |
I don't know any more than what's in the docs and what @glasser mentioned, sorry. I haven't implemented them myself yet. |
I only want to be able to cache things that have explicit cache hints, without defining the defaultMaxAge. How do I only cache things that have explicit hints, and disable automatic root object caching? It is not possible to do that? |
@glasser I have played around with this sample now and it does indeed work with your diff. However as soon as I make the following change it stops working and I think thats where the confusion comes from for a lot of people:
My assumption would be that the
Only then does the Edit: Ok this is pretty embarrasing but here we go... Once I have that in my headers in GraphQL Playground everything seems to be working fine... Only noticed this when curling the same queries and seeing a difference in headers.. I suggest everyone recheck their responses through curl and compare. Edit 2: Opening the playground in incognito seems to send the correct caching headers. Not sure how I managed to get it into such a weird state. So looks like caching is indeed working correctly for me now Final Edit: Back to square one no idea why or how its sometimes working and sometimes not. As soon as I change my structure slightly it stops working. Need to investigate further, there is some structure change that breaks the caching |
@glasser I have a repo with reproduction for you: https://github.com/mattvb91/apollo-cache-issue-reproduction My assumption is caching will not work top level unless all sub nested objects have a caching hint on them. |
@mattvb91 I'll dig in on your reproduction, but I do think you're observing documented behavior. https://www.apollographql.com/docs/apollo-server/performance/caching/#setting-a-default-maxage says:
nestedObject is a field returning an object type is its default maxAge is 0. Whether this design for |
Yes, looking at your reproduction, this is working as intended. The theory when designing this feature was that object fields are more likely to be "looking up a different thing with different cacheability" than scalar fields. I'm not sure that this area of the product is under enough active development for it to be likely to change the semantics at this point. |
Ah i think i may have gotten that confused then in thinking the removal of Yea it does seem to be more a discussion of wether that is a good default behaviour. Thanks |
i also struggle with this problem. We want to define some named root queries that should be cached (the full response), but the default should be no-cache. There is no combination of setting the If someone could point me int the right direction, i would be very glad. |
I also noticed that only the first query has the right cacheHint:
first request responses with it seems like apollo server caches the cache hints as well! Edit: its clear. Not setting defaultMaxAge --> caching DOES NOT WORK AT ALL But setting cacheHints dynamically for every resolver also does not work properly. As mentioned above it only works shortly. After further investigation it seems that it works for the given cachehint time, after that it falls back to the global setting. Something is totally wrong in the cache logic and its not only the flawed design of defaultMaxAge Edit2: its really confusing to debug. But it seems clear that resolver-level cache hints get lost when it tries to use the cache. Edit3: it had something to do how we set cachehints dynamically, i think i solved it. But the one flaw remains: defaultMaxAge is the maximum TTL that you can set. if you omit it, no resolver will get cached |
I tried without defaultMaxAge and caching is working for me. I noticed that if we are not using cache control hints properly then it looks like it is not working. Make sure that cache hint is applied to a field and its nested non scalar fields as well then only caching will work. Otherwise when it finds nested non scalar field is missing cache control so it tries to use defaultMaxAge and which is 0 by default. Hence, it looks like caching is not working. Some points to be noted
|
In case you want to save the effort of setting up Redis, getting the config working etc, |
HUGE THANKS ankuradhey!!! |
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(noDefaultMaxAge: true)`. If this is specified on a root or object-returning or interface-returning field that does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that scalar fields all of whose ancestors have no `maxAge` due to this feature are now treated similarly to scalar root fields. Also change `info.cacheControl.cacheHint` to update when `setCacheHint` is called. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields. This addresses concerns from #4162 and #3559.
Here's an implementation of something that's similar to the |
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(noDefaultMaxAge: true)`. If this is specified on a root or object-returning or interface-returning field that does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that scalar fields all of whose ancestors have no `maxAge` due to this feature are now treated similarly to scalar root fields. Also change `info.cacheControl.cacheHint` to update when `setCacheHint` is called. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields. This addresses concerns from #4162 and #3559.
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(noDefaultMaxAge: true)`. If this is specified on a root or object-returning or interface-returning field that does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that scalar fields all of whose ancestors have no `maxAge` due to this feature are now treated similarly to scalar root fields. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields. This addresses concerns from #4162 and #3559.
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(noDefaultMaxAge: true)`. If this is specified on a root or object-returning or interface-returning field that does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that scalar fields all of whose ancestors have no `maxAge` due to this feature are now treated similarly to scalar root fields. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields. This addresses concerns from #4162 and #3559.
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(noDefaultMaxAge: true)`. If this is specified on a root or object-returning or interface-returning field that does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that scalar fields all of whose ancestors have no `maxAge` due to this feature are now treated similarly to scalar root fields. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields. This addresses concerns from #4162 and #3559.
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(noDefaultMaxAge: true)`. If this is specified on a root or object-returning or interface-returning field that does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that scalar fields all of whose ancestors have no `maxAge` due to this feature are now treated similarly to scalar root fields. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields. This addresses concerns from #4162 and #3559.
Previously, the cache control logic treats root fields and fields that return object or interface types which don't declare `maxAge` specially: they are treated as uncachable (`maxAge` 0) by default. You can change that 0 to a different number with the `defaultMaxAge` option, but you can't just make them work like scalars and not affect the cache policy at all. This PR introduces a new argument to the directive: `@cacheControl(inheritMaxAge: true)`. If this is specified on an field returning a composite type (object, interface, or union) or on the composite type itself, and it does not specify its `maxAge` in some other way (on the return value's type or via `setCacheHint`), then the field is just ignored for the sake of calculating cache policy, instead of defaulting to `defaultMaxAge`. Note that this does *not* affect root fields (scalar or composite). You still need to make sure that every root field is declared as cachable to have a cachable operation. This just lets you say that a nested composite form is "part of" its parent for the purposes of caching, just like nested scalar fields are by default. The behavior described above (and looking on the field's return type for `@cacheControl` in the first place) previously applied to fields returning object and interface types (possibly nested in some layers of not-null and/or list-of). This PR makes things more consistent by treating the third composite type (unions) in the same way. One use case for this could be in federation: `buildFederatedSchema` could add this directive to all `@external` fields, since their values are typically provided directly in the arguments to the `Query._entities` field. This addresses concerns from #4162 and #3559.
I think the main concern in this issue is addressed in #5247. |
My apollo graphql server setup uses the following:
My schema:
My responseCachePlugin config:
Neither the static cache hint like the above nor the dynamic cache hint in the resolver work. i.e., they don't cache the response from the datasource (REST API).
A bunch of other devs reported this issue on Spectrum. One of the Spectrum posts points to the problem:
github link to the line in the code where there is a potential problem:
apollo-server/packages/apollo-server-plugin-response-cache/src/ApolloServerPluginResponseCache.ts
Line 243 in 1acf62d
Spectrum post that details the above issue:
https://spectrum.chat/apollo/apollo-server/apollo-server-plugin-response-cache-is-not-working-properly~e3390cd4-9e3f-4919-88b4-92508e235e38
Other Spectrum posts that talk about this problem:
cc to: @trevor-scheer
The text was updated successfully, but these errors were encountered: