Cache control support with federation #870

mandiwise · 2021-07-10T03:11:49Z

This PR allows you to use the @cacheControl directive with Apollo Federation. To take advantage of it, you need to be using Apollo Server 3.0.2 or newer in your gateway and all your subgraphs.

Specifically, this PR:

Changes the Query._entities resolver in subgraphs to respect @cacheControl directives on the individual types that are in the _Entity union. If all entity types returned in a given call have @cacheControl directives (or use info.cacheControl in their __resolveReference), then the field's cache control policy comes from the most restrictive of these directives instead of the default of defaultMaxAge (eg uncacheable by default). (If any entity type returned in a call does not specify a cache control policy, then it still defaults to defaultMaxAge).
Provide the full incoming GraphQLRequestContext to GraphQLDataSource.process. (Also make it possible for the method to tell if it's running a health check or fetching schemas, as has been requested.)
In RemoteGraphQLDataSource, process HTTP cache-control response headers from subgraphs and use them to restrict the operation's overall cache policy. As usual with Apollo Server cache policy, if there is no cache-control header from a subgraph then assume the whole operation is not cacheable.

(If you want to support cache control with a non-Apollo Server subgraph, just make sure it returns a proper cache-control HTTP header.)

Fixes #356.
Fixes apollographql/apollo-server#5449.
Aided by apollographql/apollo-server#5248.

glasser · 2021-07-15T23:30:11Z

The replace method is used instead of the restrict because the default maxAge of 0 for the _entites field will never be overwritten by any value >0 set on the member type's definition when restrict is used. This may create unpredictable results if two different entity types were named in the representations field argument, but it may be fine to omit this edge case for now.

This is a bug in the new AS3 cache control logic. My intention is that defaultMaxAge should not be applied if you use dynamic cache control methods. I will try to get this fix into an AS 3.0.01 ASAP. Thanks for discovering it. apollographql/apollo-server#5488

glasser · 2021-07-15T23:31:34Z

federation-js/src/types.ts

@@ -13,6 +13,17 @@ import {
  isObjectType,
 } from 'graphql';
 import { PromiseOrValue } from 'graphql/jsutils/PromiseOrValue';
+import { CacheHint, CachePolicy } from 'apollo-server-types';
+
+declare module 'graphql/type/definition' {


This is probably not required any more once you rebase on #875 ?

This still seems to be necessary even after a rebase

My worry is that if you end up loading both this and apollo-server-core in your project, TypeScript will be sad to see the same declaration twice.

I'm thinking of making a chance so that the type under cacheControl is exported from apollo-server-types, so you can do something like:

if ((info as any).cacheControl?.cacheHintFromType) { const cacheControl: ResolveInfoCacheControl = info.cacheControl; const cacheHint = cacheControl.cacheHintFromType(type);

glasser · 2021-07-15T23:31:57Z

federation-js/src/types.ts

@@ -87,6 +98,15 @@ export const entitiesField: GraphQLFieldConfig<any, any> = {
        );
      }

+      if (info.cacheControl && info.cacheControl.cacheHint.replace) {


perhaps

if (info.cacheControl?.cacheHint?.replace) {

glasser · 2021-07-15T23:32:30Z

federation-js/src/types.ts

+          info.cacheControl.cacheHintFromType(type);
+
+        if (cacheHint) {
+          info.cacheControl.cacheHint.replace(cacheHint);


This should be restrict, but requires the bug fix mentioned above.

Fix released in 3.0.1 so it should be able to be restrict. I think it's fine for us to release this with the caveat that it doesn't work with 3.0.0.

Updated to use the restrict method with 3.0.1

glasser

Let me get out another version of AS with the type I'm mentioning here.

Then the other trick is, it would be nice to have tests, but I could write them as part of the half of apollographql/apollo-server#5449 I was going to do... So maybe I should just augment this PR with that work when I get the time for it.

glasser · 2021-07-19T21:43:26Z

federation-js/src/types.ts

+        const cacheHint: CacheHint | undefined =
+          info.cacheControl.cacheHintFromType(type);
+
+        if (cacheHint?.maxAge || cacheHint?.scope) {


I think this could just be if (cacheHint)

glasser · 2021-07-19T21:50:35Z

federation-js/src/types.ts

@@ -13,6 +13,17 @@ import {
  isObjectType,
 } from 'graphql';
 import { PromiseOrValue } from 'graphql/jsutils/PromiseOrValue';
+import { CacheHint, CachePolicy } from 'apollo-server-types';
+
+declare module 'graphql/type/definition' {


My worry is that if you end up loading both this and apollo-server-core in your project, TypeScript will be sad to see the same declaration twice.

I'm thinking of making a chance so that the type under cacheControl is exported from apollo-server-types, so you can do something like:

if ((info as any).cacheControl?.cacheHintFromType) { const cacheControl: ResolveInfoCacheControl = info.cacheControl; const cacheHint = cacheControl.cacheHintFromType(type);

This makes `info.cacheControl` available to TypeScript packages that depend on `apollo-server-types` rather than only being declared deep inside `apollo-server-core`. Additionally, it gives a name to type used for `info.cacheControl`. Intended for use cases like apollographql/federation#870 We've run into tricky issues with `declare module` before so if this ends up causing more problems than it's worth, we may revert it.

) This makes `info.cacheControl` available to TypeScript packages that depend on `apollo-server-types` rather than only being declared deep inside `apollo-server-core`. Additionally, it gives a name to type used for `info.cacheControl`. Intended for use cases like apollographql/federation#870 We've run into tricky issues with `declare module` before so if this ends up causing more problems than it's worth, we may revert it.

glasser · 2021-07-20T21:10:40Z

@mandiwise I rebased, upgraded apollo-server-types to the version I just released, and fixed the above. Later I will add my promised half of the project and write some tests.

glasser · 2021-07-21T05:30:01Z

@mandiwise: I squashed your commits together and rewrote the PR description. This PR now implements cache control for federation, end-to-end. It requires Apollo Server 3.0.2 to work properly. It is tested end to end. I have not had a chance to seek out and update docs in this repo or apollo-server yet, and might not get to that this week.

vany0114 · 2021-07-21T17:02:01Z

This is great guys @mandiwise @glasser thanks for tackle this, I've been waiting for this. Do you guys already have an ETA to release it?

glasser · 2021-07-21T17:38:44Z

Once we've got docs and code review!

netlify · 2021-07-22T15:43:32Z

👷 Deploy request for apollo-federation-docs pending review.
Visit the deploys page to approve it

🔨 Explore the source changes: 3742084fa6cb3c11c5d4ee76a39f2bd18bf8975a

glasser · 2021-07-22T16:39:46Z

gateway-js/src/datasources/types.ts

+   * one of the strings if this operation is generated by the gateway without an
+   * incoming request.
+   */
+  topLevelRequestContext:


@trevor-scheer Do you like this name?

Do you like this type or should we go with something more like {kind: 'health check'}|{kind: 'loading schema'}|{kind: 'incoming operation', requestContext: GraphQLRequestContext}

OK, I think we definitely want the kind sort of thing in case we want more information associated with health checks etc, so I've rebased the PR so it does that. Still curious if you like the topLevelRequestContext name :) I think it's better than incomingRequestContext because not all are based on incoming requests. Of course the fact that it doesn't always have a requestContext on it is a little odd. 🤷

mandiwise · 2021-07-22T22:26:52Z

The caching docs have been updated in this PR: apollographql/apollo-server#5536

glasser · 2021-07-23T01:46:42Z

I could possibly move the kind up a level to be a top level option and then have incomingRequestContext: GraphQLRequestContext be a top level option that exists when kind is incoming operation. Thoughts @abernix ?

(edit: I did this)

glasser

Adding my approval for technical reasons, but this still needs review by somebody else. To be organized by @abernix.

trevor-scheer

💵 🎛 🥳

trevor-scheer · 2021-07-26T23:23:35Z

gateway-js/src/datasources/__tests__/RemoteGraphQLDataSource.test.ts

@@ -15,6 +15,14 @@ beforeEach(() => {
  fetch.mockReset();
 });

+// Right now, none of these tests care what's on topLevelRequestContext, so we


Suggested change

// Right now, none of these tests care what's on topLevelRequestContext, so we

// Right now, none of these tests care what's on incomingRequestContext, so we

trevor-scheer · 2021-07-26T23:35:27Z

gateway-js/src/datasources/types.ts

+      /**
+       * Equivalent to incomingRequestContext.context (provided here for
+       * backwards compatibility): the object created by the Apollo Server
+       * `context` function.
+       */


Can we add a @deprecated annotation on this doc block?

Suggested change

/**

* Equivalent to incomingRequestContext.context (provided here for

* backwards compatibility): the object created by the Apollo Server

* `context` function.

*/

/**

* Equivalent to incomingRequestContext.context (provided here for

* backwards compatibility): the object created by the Apollo Server

* `context` function.

*

* @deprecated Use `incomingRequestContext.context` instead

*/

Done (with a few more words)

trevor-scheer · 2021-07-26T23:39:45Z

gateway-js/src/datasources/types.ts

+      context: GraphQLRequestContext<TContext>['context'];
+    }
+  | {
+      kind: 'health check';


This is maybe just a bit off of convention. Tempting to make this an enum or enum-esque a la graphql's Kind https://github.com/graphql/graphql-js/blob/main/src/language/kinds.ts

That being said, it's not like TS struggles at all with these string types - it'll auto-complete and type-check these kind properties just fine as-is.

OK, enum it is.

This lets the data source pay attention to any piece of the incoming request (say, its HTTP structure, or the incoming operation itself), not just the user context object. In the two cases where we use the data source without an incoming request (health checks and schema loading via introspection), pass a special string to indicate this, which lets data sources handle these operations specially. Fixes apollographql#419. Fixes apollographql#835.

Use them to restrict the overall cache policy. This behavior can be disabled by passing `honorSubgraphCacheControlHeader: false` to the RemoteGraphQLDataSource constructor (if you are setting your cache policy some other way).

glasser · 2021-08-03T05:43:01Z

Merged!

Let's do a minor version bump release of federation and gateway tomorrow, and then merge apollographql/apollo-server#5536 too (adding version number references to it).

vany0114 · 2021-09-16T22:51:41Z

@glasser Guys I'm trying to use the cache control in a federated schema (using the versions mentioned here) using type-graphql but it's not working. I used to have a workaround that was merging the schemas and it was working fine, but after I moved to the latest version I obviously removed it (the workaround) but is not working anymore.

workaround:

const finalSchema = mergeSchemas({
    schemaDirectives: {
      cacheControl: CacheControlDirective,
    },
    schemas: [federatedSchema, schema],
  });

I'm registering the cacheContol directive in the subgraph like this:

  SchemaDirectiveVisitor.visitSchemaDirectives(schema, {
    cacheControl: CacheControlDirective,
  });

Also the enum:

registerEnumType(CacheScope, {
  name: 'CacheControlScope',
  description: 'Scopt of Cache Control',
});

Any ideas what might I be missing?

glasser · 2021-09-21T20:56:22Z

@vany0114 I'm not really familiar enough with the details of what you're doing to help out. For example, I'm not sure what the CacheControlDirective symbol you have above is; I don't know much about type-graphql; and I'm not even sure if you're doing this in a subgraph or in your gateway. Can you provide a full reproduction example (a repo to git clone or a codesandbox.io sandbox, with full instructions of what to do to see the issue)?

mandiwise requested review from glasser and trevor-scheer July 10, 2021 03:12

glasser mentioned this pull request Jul 15, 2021

cache control: defaultMaxAge shouldn't be applied if you use dynamic cache control apollographql/apollo-server#5488

Closed

glasser requested changes Jul 15, 2021

View reviewed changes

glasser mentioned this pull request Jul 16, 2021

cache control: don't apply default max age until after resolver runs apollographql/apollo-server#5492

Merged

glasser reviewed Jul 19, 2021

View reviewed changes

glasser mentioned this pull request Jul 19, 2021

apollo-server-types: move info.cacheControl 'declare module' here apollographql/apollo-server#5512

Merged

glasser force-pushed the main branch from e9b7b21 to 1c968d7 Compare July 20, 2021 21:24

This was referenced Jul 20, 2021

Missing operationName in __ApolloServiceHealthCheck__ query. #835

Closed

Add operationName variable for health_check_query #837

Closed

glasser changed the title ~~Check for cache hint when resolving entities field~~ Cache control support with federation Jul 20, 2021

glasser force-pushed the main branch 2 times, most recently from 9b46f6c to 10b4a30 Compare July 21, 2021 05:14

glasser force-pushed the main branch from 10b4a30 to a7de345 Compare July 21, 2021 05:32

glasser mentioned this pull request Jul 21, 2021

RemoteGraphQLDataSource: allow per-request control over url, method #901

Open

glasser reviewed Jul 22, 2021

View reviewed changes

glasser requested review from abernix and glasser and removed request for glasser July 22, 2021 16:59

glasser force-pushed the main branch from e9eb0c4 to 2bd447e Compare July 22, 2021 18:10

mandiwise mentioned this pull request Jul 22, 2021

Update caching docs to include federation support. apollographql/apollo-server#5536

Merged

glasser force-pushed the main branch from 2bd447e to 0de10fe Compare July 23, 2021 18:37

glasser approved these changes Jul 23, 2021

View reviewed changes

glasser force-pushed the main branch from 623b2e5 to 3742084 Compare July 23, 2021 18:57

trevor-scheer approved these changes Jul 26, 2021

View reviewed changes

buildFederatedSchema: Check for cache hint when resolving entities field

5ac76f2

glasser force-pushed the main branch from 3742084 to 2c67e28 Compare August 3, 2021 05:20

glasser added 4 commits August 2, 2021 22:31

End-to-end test for federated cache control

9976b82

CHANGELOG update

467ad6b

glasser force-pushed the main branch from 2c67e28 to 467ad6b Compare August 3, 2021 05:33

glasser enabled auto-merge August 3, 2021 05:35

glasser merged commit 616caf2 into apollographql:main Aug 3, 2021

abernix mentioned this pull request Jan 14, 2022

whole response caching apollographql/router#326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache control support with federation #870

Cache control support with federation #870

mandiwise commented Jul 10, 2021 •

edited by glasser

Loading

glasser commented Jul 15, 2021

glasser Jul 15, 2021

mandiwise Jul 17, 2021

glasser Jul 19, 2021

glasser Jul 15, 2021

mandiwise Jul 17, 2021

glasser Jul 15, 2021

glasser Jul 16, 2021

mandiwise Jul 17, 2021

glasser left a comment

glasser Jul 19, 2021

glasser Jul 19, 2021

glasser commented Jul 20, 2021

glasser commented Jul 21, 2021

vany0114 commented Jul 21, 2021

glasser commented Jul 21, 2021

netlify bot commented Jul 22, 2021 •

edited

Loading

glasser Jul 22, 2021

glasser Jul 22, 2021

mandiwise commented Jul 22, 2021

glasser commented Jul 23, 2021 •

edited

Loading

glasser left a comment

trevor-scheer left a comment

trevor-scheer Jul 26, 2021

glasser Aug 3, 2021

trevor-scheer Jul 26, 2021

glasser Aug 3, 2021

trevor-scheer Jul 26, 2021

glasser Aug 3, 2021

glasser commented Aug 3, 2021

vany0114 commented Sep 16, 2021

glasser commented Sep 21, 2021

	// Right now, none of these tests care what's on topLevelRequestContext, so we
	// Right now, none of these tests care what's on incomingRequestContext, so we

Cache control support with federation #870

Cache control support with federation #870

Conversation

mandiwise commented Jul 10, 2021 • edited by glasser Loading

glasser commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Jul 20, 2021

glasser commented Jul 21, 2021

vany0114 commented Jul 21, 2021

glasser commented Jul 21, 2021

netlify bot commented Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandiwise commented Jul 22, 2021

glasser commented Jul 23, 2021 • edited Loading

glasser left a comment

Choose a reason for hiding this comment

trevor-scheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Aug 3, 2021

vany0114 commented Sep 16, 2021

glasser commented Sep 21, 2021

mandiwise commented Jul 10, 2021 •

edited by glasser

Loading

netlify bot commented Jul 22, 2021 •

edited

Loading

glasser commented Jul 23, 2021 •

edited

Loading