-
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
Full query response cache plugin #2437
Conversation
7e9b0d4
to
84141bd
Compare
Added a basic end-to-end test (without any of the configurable options like session ID/privacy). |
@@ -57,6 +67,8 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> { | |||
readonly operationName?: string | null; | |||
readonly operation?: OperationDefinitionNode; | |||
|
|||
readonly overallCachePolicy?: Required<CacheHint> | undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because CacheHint
, CacheScope
, and overallCachePolicy
are tied to our caching spec, it might be better to leave them in apollo-cache-control
. We can still augment GraphQLRequestContext
with overallCachePolicy
there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by augment? Is that a TypeScript feature I'm not familiar with? I think I'm not following this suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, it's called 'module augmentation' and leads to merging of the declarations: https://www.typescriptlang.org/docs/handbook/declaration-merging.html
So you should be able to add something like this to apollo-cache-control
:
declare module 'apollo-server-core/dist/requestPipelineAPI' {
interface GraphQLRequestContext<TContext> {
readonly overallCachePolicy?: Required<CacheHint> | undefined;
}
}
As a side note, we've been thinking about extracting requestPipelineAPI
into its own apollo-server-api
package to avoid circular dependencies.
// GraphQLResponse, that result is used instead of executing the query. It is | ||
// an error for more than one plugin to return a value that resolves to a | ||
// non-null GraphQLResponse. | ||
execute?( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the idea of adding a hook like this, but the name execute
seems confusing. This isn't really execution, but rather a way to short-circuit execution. We don't invoke executionDidStart
when a response is returned from cache for example, because we don't measure execution time in that case. Maybe something like responseForOperation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also possible confusion/overlap with the addition of a pluggable executor, which was added in 185dd45 and is used to plug in Apollo Gateway. I think we want to keep this separate, both because the gateway isn't a regular plugin (it composes a schema from a service list and it needs to reload itself when service list changes, which isn't possible through the plugin API) and because we actually want to be able to cache gateway responses.
|
||
// If this hook is defined and returns false, the plugin will not read | ||
// responses from the cache. | ||
readFromCache?( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds too much like an action. Maybe shouldReadFromCache
?
|
||
// If this hook is defined and returns false, the plugin will not write the | ||
// response to the cache. | ||
writeToCache?( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe shouldWriteToCache
?
} | ||
|
||
interface BaseCacheKey { | ||
document: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A document
property containing the document text seems confusing, because we use document
on the request context to refer to the parsed DocumentNode
.
baseCacheKey = { | ||
// XXX could also have requestPipeline add the unparsed document to requestContext; | ||
// can't just use requestContext.request.query because that won't be set for APQs | ||
document: print(requestContext.document), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd really like to avoid having to print the document AST every time we need to compute a cache key. So the suggestion of putting the document text on the request context makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, we could replace the use of document
and operationName
by an operationID
, which would rely on the same normalization we use for the operation registry. So we could add an operationID
to the request context and use that across plugins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gosh, the op reg stuff is so in flux though. Are you imagining this ID as just the pair of normalized doc and ID, or as an already hashed thing? I'm sort of not psyched about 100% standardizing on a cross-codebase normalization until graphql/graphql-js#1628 is merged. Though I also think it should be kosher to change cache key calculation later if we need. (Down for adding the string to the context though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining the operation ID as a hash based on the normalized operation node + dependent fragments (so not the entire document, in case that contains multiple operations). I agree we want to use stripIgnoredTokens
there. Realistically though, we can't expect everyone to be on the latest graphql
even after that PR lands. Since it's only a small bit of code, maybe we should just include it for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be my plan, but I'd like to see it merged and ideally released first, and the current PR has a comment about how the implementation may change.
if (value === undefined) { | ||
return null; | ||
} | ||
return JSON.parse(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this currently will not return a valid GraphQLResponse
, because the errors
property is expected to contain instances of GraphQLError
, not formatted errors. I've been looking at changing this to GraphQLFormattedError
however, because I'm running into a similar problem in the gateway where we want to propagate errors from underlying services. So this gives me another data point in favor of making that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the time scale for making that change, or is that something I should take on? I'm hoping to finish this project this week due to leave concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a look at that today, to get a better sense of how much work it would be. Ideally we'd get it done this week as well, because I also need it for the gateway work. My main concern is that it's potentially breaking for people who have already adopted the plugin API. So I want to hear what Jesse thinks about that as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just to be clear, the issue is that GraphQLResponse isn't actually a json roundtrippable object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also more consistent with how the errors
field of a response is described in the spec:
https://facebook.github.io/graphql/draft/#sec-Errors. So the idea is that we'd add a separate didEncounterErrors
hook for error reporting, that will receive GraphQLError
instances. But the response would contain the errors after applying formatError
.
package.json
Outdated
@@ -58,6 +58,7 @@ | |||
"apollo-server-lambda": "file:packages/apollo-server-lambda", | |||
"apollo-server-micro": "file:packages/apollo-server-micro", | |||
"apollo-server-plugin-base": "file:packages/apollo-server-plugin-base", | |||
"apollo-server-plugin-full-query-cache": "file:packages/apollo-server-plugin-full-query-cache", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a meta point, but I'm wondering if 'full query cache' is clear enough. I think we've been using 'full query cache' and 'whole response cache' interchangeably, but I think 'full query cache' is too easily confused with the APQ cache or the document cache. Maybe we should rename this to apollo-server-plugin-response-cache
instead?
Notes:
|
|
a64b620
to
e2cc27a
Compare
I renamed the package to apollo-server-plugin-response-cache. To make future rebases easier I squashed and force pushed. Other changes will happen as additional commits, though. |
3476403
to
57b5e2e
Compare
Note that this is currently rebased on top of #2441. I think all the commits after that PR can be squashed together, but I kept them separate to make it easier to review for people who already saw the first version of this. I think this is feature complete. It needs tests for a few more features, and needs docs. I was going to add docs to the part of our docs that already describe using |
I have to run for the day — the test failure is a hapi thing, I should fix that. |
50f8b74
to
c9e2f6f
Compare
This is no longer built on top of #2441. It doesn't use the executor interface added there, but uses a separate responseForOperation hook, at @martijnwalraven 's suggestion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty great to me. I've left a few comments within, but I think we should consider releasing this as an alpha and dropping it into our (own, internal) infra as soon as possible. Thoughts?
'document' | 'operationName' | 'operation' | ||
>, | ||
): Promise<GraphQLResponse | null> { | ||
requestContext.metrics!.responseCacheHit = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we could get rid of the non-null !
assertion here with the WithRequired
changes for metrics
I've suggested previously?
}, | ||
|
||
async willSendResponse( | ||
requestContext: WithRequired<GraphQLRequestContext<any>, 'response'>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type for requestContext
should be inferred?
"references": [ | ||
{ "path": "../apollo-cache-control" }, | ||
{ "path": "../apollo-server-plugin-base" }, | ||
{ "path": "../apollo-server-caching" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm somewhat surprised we don't reference the apollo-server-env
project here, but also noticing that we don't do that anywhere else. 🤷♂️
Note: I don't think this means you should change anything, just a casual observation when I was mentally double-clicking on the dependencies.
}) { | ||
this.extensions.forEach(extension => { | ||
if (extension.didResolveOperation) { | ||
extension.didResolveOperation(o); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't guard on whether or not the hook is a typeof
===
'function'
in other places, so this isn't the exception to the pattern.
Awesome PR! Did you run benchmarks on the reponse time before/after? |
Updated based on review. Finished the tests I wanted to write. Have to head out early today so I haven't done docs yet. |
@abernix @martijnwalraven Tests and docs are good. It would be great if you could review this today for merge tomorrow! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I'm especially happy we know have docs! Apart from some small nits, this seems ready to merge.
I opened a separate PR with some typing fixes: #2479
docs/source/features/caching.md
Outdated
@@ -0,0 +1,131 @@ | |||
--- | |||
title: Caching | |||
description: Automatically set HTTP cache headers! Save full responses in a cache! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also excited about getting this in, but not sure about the exclamation marks...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it parsed better than two period sentences and it made it clearer that there are two independent features than using and
or or
but fair enough, I changed it.
@@ -323,7 +353,7 @@ export async function processGraphQLRequest<TContext>( | |||
}); | |||
} | |||
|
|||
return sendResponse(response); | |||
return sendResponse(response!!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two !
s should never be needed, and I think we can avoid a non-null type assertion by making sure we only set response
to the result of formatResponse
if it isn't null
. See 4e34508.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, I think I'm getting !
confused with another programming language. Thanks for fixing this.
// | ||
// This hook may return a promise because, for example, you might need to | ||
// validate a cookie against an external service. | ||
sessionId?( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be sessionID
? I know the current codebase isn't very consistent, but I think we said we would follow https://github.com/airbnb/javascript#naming--Acronyms-and-Initialisms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into this. apollo-server and apollo-client are almost 100% consistent in not using ID
except stand-alone as the built-in GraphQL ID
. This includes clientReferenceId
and dataIdFromObject
(an important apollo-client hook). The only instance of \BID
in our APIs in these two projects is serviceID
in the plugin API, used only by the operation registry, which we should perhaps change to serviceId
(keeping around serviceID
for compat for a bit too).
I do personally find ID
better from a aesthetic and GraphQL-ID
-compatibility standpoint but it seems pretty clear what Apollo has chosen.
Note that there's some code that tries to pass a serviceID
to the EngineReportingAgent constructor too but I'm not sure why; it doesn't read it.
docs/source/features/caching.md
Outdated
|
||
The easiest way to add cache hints is directly in your schema using the `@cacheControl` directive. Apollo Server automatically adds the definition of the `@cacheControl` directive to your schema when you create a new `ApolloServer` object with `typeDefs` and `resolvers`. | ||
|
||
You can apply `@cacheControl` to an individual field or to a type. Hints on a type apply to all fields that *return* objects of that type (not to the fields inside that type). Hints on fields override hints specified on the target type. `@cacheControl` can specify `maxAge` (in seconds, like in an HTTP `Cache-Control` header) and `scope`, which can be `PUBLIC` (the default) or `PRIVATE`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know what you mean by "Hints on a type apply to all fields that return objects of that type (not to the fields inside that type)", but I think this may require some clarification for it to make sense to people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I added a bunch of specific examples.
- A new plugin package implementing a response cache, based on apollo-cache-control hints. - GraphQLRequestContext new fields: - overallCachePolicy - documentText - metrics - New plugin hook responseForOperation. - new GraphQLExtension hook didResolveOperation, identical to the same hook in the Plugin API. Change apollo-engine-reporting to use this hook instead of executionDidStart, because executionDidStart doesn't run if the cache short-circuits execution. - apollo-engine-reporting: report whether the request was a cache hit. Also use the new requestContext.metrics object to report persisted query hit/register instead of specific extension options (though those extension options still work). - cacheControl constructor option semantic change: include the cacheControl GraphQL extension in the output with `cacheControl: true` and `cacheControl: {stripFormattedExtensions: true}` (as before), but not for `cacheControl: {otherOptions: ...}`.
Co-Authored-By: glasser <glasser@apollographql.com>
Co-Authored-By: glasser <glasser@apollographql.com>
Co-Authored-By: glasser <glasser@apollographql.com>
Co-Authored-By: glasser <glasser@apollographql.com>
Co-Authored-By: glasser <glasser@apollographql.com>
The full-query caching feature implemented in #2437 originally took the additional steps of also changing the `apollo-engine-reporting` module to utilize the new request pipeline available in Apollo Server as of #1795. While that would have been nice, there was still some uncertainty about some of the life-cycle hooks that would be necesssary to make that happen and it wasn't worth blocking the implementation of full-query caching on those stalled decisions. Therefore, the changes to utilize new functionality in the request pipeline, including what would have been a simplification of the way that `apollo-engine-reporting` would have obtained the `operationName` (something that is available via the API of the request-pipeline hooks), were backed out and will land separately in a future PR. The portion regarding `operationName` was inadvertently not backed out and instead was left leveraging a life-cycle hook which was not available to the `graphql-extensions` API: `didResolveOperation`. This means that the code was not setting `operationName` in the way it needed to and therefore `operationName` was always being left undefined (as is sometimes permitted!) with this new `apollo-engine-reporting`. This commit puts the functionality back to that which is required for the `graphql-extensions` implementation, and the commit before this (1ae7def) acts as a regression test (it should pass, as of this commit, and fail before it).
The full-query caching feature implemented in #2437 originally took the additional steps of also changing the `apollo-engine-reporting` module to utilize the new request pipeline available in Apollo Server as of #1795. While that would have been nice, there was still some uncertainty about some of the life-cycle hooks that would be necesssary to make that happen and it wasn't worth blocking the implementation of full-query caching on those stalled decisions. Therefore, the changes to utilize new functionality in the request pipeline, including what would have been a simplification of the way that `apollo-engine-reporting` would have obtained the `operationName` (something that is available via the API of the request-pipeline hooks), were backed out and will land separately in a future PR. The portion regarding `operationName` was inadvertently not backed out and instead was left leveraging a life-cycle hook which was not available to the `graphql-extensions` API: `didResolveOperation`. This means that the code was not setting `operationName` in the way it needed to and therefore `operationName` was always being left undefined (as is sometimes permitted!) with this new `apollo-engine-reporting`. This commit puts the functionality back to that which is required for the `graphql-extensions` implementation, and the commit before this (8a43341) acts as a regression test (it should pass, as of this commit, and fail before it).
* fix: Set `operationName` via `executionDidStart` and `willResolveField`. The full-query caching feature implemented in #2437 originally took the additional steps of also changing the `apollo-engine-reporting` module to utilize the new request pipeline available in Apollo Server as of #1795. While that would have been nice, there was still some uncertainty about some of the life-cycle hooks that would be necesssary to make that happen and it wasn't worth blocking the implementation of full-query caching on those stalled decisions. Therefore, the changes to utilize new functionality in the request pipeline, including what would have been a simplification of the way that `apollo-engine-reporting` would have obtained the `operationName` (something that is available via the API of the request-pipeline hooks), were backed out and will land separately in a future PR. The portion regarding `operationName` was inadvertently not backed out and instead was left leveraging a life-cycle hook which was not available to the `graphql-extensions` API: `didResolveOperation`. This means that the code was not setting `operationName` in the way it needed to and therefore `operationName` was always being left undefined (as is sometimes permitted!) with this new `apollo-engine-reporting`. This commit puts the functionality back to that which is required for the `graphql-extensions` implementation, and the commit before this (8a43341) acts as a regression test (it should pass, as of this commit, and fail before it). * Add a regression test for `operationName` not being defined. This should fail and then be fixed with an upcoming commit.
#2437. While #2437 did introduce the documentation and the appropriate configuration for it, it did so while we were still utilizing Hexo for our docs, and the configuration changes necessary for our docs' Gatsby-ification didn't happen. This puts the configuration in place for Gatsby and merges into the 2.5.0 release branch. Ref: https://github.com/apollographql/apollo-server/pull/2437/files#diff-b09bbff3e688a10b35f7de810d65c28e
The full query caching change (#2437) intended to introduce didResolveOperation to the old graphql-extensions API used by apollo-engine-reporting ("backporting" it from the newer plugin API). However, that change accidentally forgot to invoke didResolveOperation from the request pipeline! This meant that the operation name never got reported. The change was backed out in #2557. But this unfortunately re-introduced the exact bug that the change in #2437 was intended to fix: operationName was no longer set when a result is served from the cache! Additionally, it was not set if a *plugin* didResolveOperation call threw, which is what happens when the operation registry plugin forbids an operation. While we could have fixed this by reintroducing the didResolveOperation extension API, there would be a subtle requirement that the apollo-engine-reporting extension didResolveOperation be run before the possibly-throwing operation registry didResolveOperation. So instead, @abernix implemented #2711. This used `requestContext.operationName` as a fallback if neither executionDidStart nor willResolveField gets called. This will be set if the operation properly parsed, validates, and either has a specified operationName that is found in the document, or there is no specified operationName and there is exactly one operation in the document and it has a name. (Note that no version of this code ever sent the user-provided operationName in case of parse or validation errors.) The existing code is correct, but this PR cleans up a few things: - #2557 reverted the one *implementation* of the didResolveOperation extension API, and #2437 accidentally didn't contain any *callers* of the API, but it was still declared on GraphQLExtension and GraphQLExtensionStack. This PR removes those declarations (which have never been useful). - We currently look for the operation name in willResolveField. But in any case where fields are successfully being resolved, the pipeline must have managed to successfully resolve the operation and set requestContext.operationName. So we don't actually need the willResolveField code, because the "fallback" in the requestDidStart end-callback will have the same value. So take this code away. (This change is the motivation for this PR; for federation metrics I'm trying to disengage the "calculate times for fields" part of trace generation from the rest of it.) - Fix the comment in "requestDidEnd" that implied incorrectly that requestContext.operationName was the user-provided name rather than the pipeline-calculated name. Be explicit both there and in requestPipeline.ts that we are relying on the fact that the RequestContext passed to requestDidStart is mutated to add operationName before its end handler is called. This change is intended to be a no-op change (other than the removal of the never-used APIs).
The full query caching change (#2437) intended to introduce didResolveOperation to the old graphql-extensions API used by apollo-engine-reporting ("backporting" it from the newer plugin API). However, that change accidentally forgot to invoke didResolveOperation from the request pipeline! This meant that the operation name never got reported. The change was backed out in #2557. But this unfortunately re-introduced the exact bug that the change in #2437 was intended to fix: operationName was no longer set when a result is served from the cache! Additionally, it was not set if a *plugin* didResolveOperation call threw, which is what happens when the operation registry plugin forbids an operation. While we could have fixed this by reintroducing the didResolveOperation extension API, there would be a subtle requirement that the apollo-engine-reporting extension didResolveOperation be run before the possibly-throwing operation registry didResolveOperation. So instead, @abernix implemented #2711. This used `requestContext.operationName` as a fallback if neither executionDidStart nor willResolveField gets called. This will be set if the operation properly parsed, validates, and either has a specified operationName that is found in the document, or there is no specified operationName and there is exactly one operation in the document and it has a name. (Note that no version of this code ever sent the user-provided operationName in case of parse or validation errors.) The existing code is correct, but this PR cleans up a few things: - #2557 reverted the one *implementation* of the didResolveOperation extension API, and #2437 accidentally didn't contain any *callers* of the API, but it was still declared on GraphQLExtension and GraphQLExtensionStack. This PR removes those declarations (which have never been useful). - We currently look for the operation name in willResolveField. But in any case where fields are successfully being resolved, the pipeline must have managed to successfully resolve the operation and set requestContext.operationName. So we don't actually need the willResolveField code, because the "fallback" in the requestDidStart end-callback will have the same value. So take this code away. (This change is the motivation for this PR; for federation metrics I'm trying to disengage the "calculate times for fields" part of trace generation from the rest of it.) - Fix the comment in "requestDidEnd" that implied incorrectly that requestContext.operationName was the user-provided name rather than the pipeline-calculated name. Be explicit both there and in requestPipeline.ts that we are relying on the fact that the RequestContext passed to requestDidStart is mutated to add operationName before its end handler is called. This change is intended to be a no-op change (other than the removal of the never-used APIs).
The full query caching change (#2437) intended to introduce didResolveOperation to the old graphql-extensions API used by apollo-engine-reporting ("backporting" it from the newer plugin API). However, that change accidentally forgot to invoke didResolveOperation from the request pipeline! This meant that the operation name never got reported. The change was backed out in #2557. But this unfortunately re-introduced the exact bug that the change in #2437 was intended to fix: operationName was no longer set when a result is served from the cache! Additionally, it was not set if a *plugin* didResolveOperation call threw, which is what happens when the operation registry plugin forbids an operation. While we could have fixed this by reintroducing the didResolveOperation extension API, there would be a subtle requirement that the apollo-engine-reporting extension didResolveOperation be run before the possibly-throwing operation registry didResolveOperation. So instead, @abernix implemented #2711. This used `requestContext.operationName` as a fallback if neither executionDidStart nor willResolveField gets called. This will be set if the operation properly parsed, validates, and either has a specified operationName that is found in the document, or there is no specified operationName and there is exactly one operation in the document and it has a name. (Note that no version of this code ever sent the user-provided operationName in case of parse or validation errors.) The existing code is correct, but this PR cleans up a few things: - #2557 reverted the one *implementation* of the didResolveOperation extension API, and #2437 accidentally didn't contain any *callers* of the API, but it was still declared on GraphQLExtension and GraphQLExtensionStack. This PR removes those declarations (which have never been useful). - We currently look for the operation name in willResolveField. But in any case where fields are successfully being resolved, the pipeline must have managed to successfully resolve the operation and set requestContext.operationName. So we don't actually need the willResolveField code, because the "fallback" in the requestDidStart end-callback will have the same value. So take this code away. (This change is the motivation for this PR; for federation metrics I'm trying to disengage the "calculate times for fields" part of trace generation from the rest of it.) - Fix the comment in "requestDidEnd" that implied incorrectly that requestContext.operationName was the user-provided name rather than the pipeline-calculated name. Be explicit both there and in requestPipeline.ts that we are relying on the fact that the RequestContext passed to requestDidStart is mutated to add operationName before its end handler is called. This change is intended to be a no-op change (other than the removal of the never-used APIs).
The full query caching change (#2437) intended to introduce didResolveOperation to the old graphql-extensions API used by apollo-engine-reporting ("backporting" it from the newer plugin API). However, that change accidentally forgot to invoke didResolveOperation from the request pipeline! This meant that the operation name never got reported. The change was backed out in #2557. But this unfortunately re-introduced the exact bug that the change in #2437 was intended to fix: operationName was no longer set when a result is served from the cache! Additionally, it was not set if a *plugin* didResolveOperation call threw, which is what happens when the operation registry plugin forbids an operation. While we could have fixed this by reintroducing the didResolveOperation extension API, there would be a subtle requirement that the apollo-engine-reporting extension didResolveOperation be run before the possibly-throwing operation registry didResolveOperation. So instead, @abernix implemented #2711. This used `requestContext.operationName` as a fallback if neither executionDidStart nor willResolveField gets called. This will be set if the operation properly parsed, validates, and either has a specified operationName that is found in the document, or there is no specified operationName and there is exactly one operation in the document and it has a name. (Note that no version of this code ever sent the user-provided operationName in case of parse or validation errors.) The existing code is correct, but this PR cleans up a few things: - #2557 reverted the one *implementation* of the didResolveOperation extension API, and #2437 accidentally didn't contain any *callers* of the API, but it was still declared on GraphQLExtension and GraphQLExtensionStack. This PR removes those declarations (which have never been useful). - We currently look for the operation name in willResolveField. But in any case where fields are successfully being resolved, the pipeline must have managed to successfully resolve the operation and set requestContext.operationName. So we don't actually need the willResolveField code, because the "fallback" in the requestDidStart end-callback will have the same value. So take this code away. (This change is the motivation for this PR; for federation metrics I'm trying to disengage the "calculate times for fields" part of trace generation from the rest of it.) - Fix the comment in "requestDidEnd" that implied incorrectly that requestContext.operationName was the user-provided name rather than the pipeline-calculated name. Be explicit both there and in requestPipeline.ts that we are relying on the fact that the RequestContext passed to requestDidStart is mutated to add operationName before its end handler is called. This change is intended to be a no-op change (other than the removal of the never-used APIs).
Question about this: how would you manually invalidate the cache after a mutation that updates cached data? I have a couple queries,
If I Am I missing something in the PR here or should I make an issue? |
You're right that there's no invalidation support or way to get the cache key so that you can manually invalidate. #3228 is related. You are welcome to file a PR; I don't personally focus on Apollo Server these days so I'm not sure what the prioritization of such a thing is. It certainly is a major missing piece of this plugin. |
Thanks! I was thinking I I was missing something, related to hinting or the Cache-Control header. I couldn't find much info elsewhere. Thanks for the response! |
@kminehart we have the exactly same problem, and we solved it by two directives: one on queries for keeping a mapping from object global id to the list of associating cache keys, another one on mutations to purge the associating cache keys for the given global id been updated. However, this requires exposing the cache keys in context to access it downstream. So we are currently using our own fork or plugin. More detail #3228. If exposing the cache key is useful to you, we can make a PR. |
Yeah we've decided to implement our own plugin instead that supports these
kinds of operations. I imagine most of our changes match the ones you did
in your fork.
…On Tue, Mar 17, 2020 at 7:35 PM Guo Liu ***@***.***> wrote:
Question about this: how would you manually invalidate the cache after a
mutation that updates cached data?
I have a couple queries,
getSingle, getList, create, update.
If I getList and getSingle, then those responses are then cached. If I
update, then getSingle will return stale data. Yet I don't see a way here
to manually say, "invalidate the cached results for getSingle and getList
."
Am I missing something in the PR here or should I make an issue?
@kminehart <https://github.com/kminehart> we have the exactly same
problem, and we solved it by two directives: one on queries for keeping a
mapping from object global id to the list of associating cache keys,
another one on mutations to purge the associating cache keys for the given
global id been updated.
However, this requires exposing the cache keys in context to access it
downstream. So we are currently using our own fork or plugin. More detail
#3228 <#3228>. If
exposing the cache key is useful to you, we can make a PR.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2437 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABHHCW2QJ2DMGW6OKG3AUVDRIAJOVANCNFSM4G5QEX3Q>
.
|
Implements a full query cache for Apollo Server in the new package
apollo-server-plugin-response-cache
. This has similar functionality to the caching feature of the deprecated Engine proxy.Supporting this required:
GraphQLRequestContext new fields:
New plugin hook responseForOperation.
new GraphQLExtension hook didResolveOperation, identical to the same hook in
the Plugin API. Change apollo-engine-reporting to use this hook instead of
executionDidStart, because executionDidStart doesn't run if the cache
short-circuits execution.
apollo-engine-reporting: report whether the request was a cache hit. Also use
the new requestContext.metrics object to report persisted query hit/register
instead of specific extension options (though those extension options still
work).
cacheControl constructor option semantic change: include the cacheControl
GraphQL extension in the output with
cacheControl: true
andcacheControl: {stripFormattedExtensions: false}
(as before), but not forcacheControl: {otherOptions: ...}
.