feat: cache-control calculation from directives #133

lennyburdette · 2021-11-12T21:09:22Z

This change adds support for the @CacheControl directive in the form of a graphql-java instrumentation class, matching the behavior of the apollo-server's built-in cache control plugin:

https://github.com/apollographql/apollo-server/tree/291c17e255122d4733b23177500188d68fac55ce/packages/apollo-server-core/src/plugin/cacheControl

As well as the special _entities behavior in @apollo/subgraph:

https://github.com/apollographql/federation/blob/00c89f82bfcc2e27e6714d894874422f84ae468c/subgraph-js/src/types.ts#L76-L87

This change does not support setting cache hints dynamically in resolvers, only static cache hints in SDL directives.

The instrumentation emits the value in the response's extensions object. Apollo Gateway looks for a cache-control policy in a subgraph's HTTP headers, so it's up to the subgraph service owner to pluck the value off the extensions and add it as a response header.

lennyburdette · 2021-11-12T21:52:25Z

I think the tests are failing due to diffplug/spotless#834

martinbonnin · 2021-11-12T22:02:20Z

Can you try running./mvnw spotless:apply?

martinbonnin · 2021-11-12T22:47:53Z

From what I understand, the CI runs on Java8:

federation-jvm/.circleci/config.yml

Line 5 in 5560300

- image: cimg/openjdk:8.0

Making it run on more Java versions is certainly something to do but for this particular PR, I'd say it's fine to format the source using java8 ? PR on your PR there: lennyburdette#1

martinbonnin · 2021-11-12T23:01:02Z

I'm guessing the build itself failed because CircleCI updated their Java version to use the new "Temurin" distribution instead of the old "AdoptOpenJdk" ones. See #134

sachindshinde · 2021-11-12T23:08:33Z

@martinbonnin
Good catch on the vendor name change! I've merged in #134 .

@lennyburdette
Can you pull in the latest changes on your branch/rebase?

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

sachindshinde · 2021-11-12T23:32:19Z

@lennyburdette
Regarding this error:

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.12.2:apply (default-cli) on project federation-graphql-java-support-api: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.12.2:apply failed: java.lang.reflect.InvocationTargetException: class com.google.googlejavaformat.java.RemoveUnusedImports (in unnamed module @0x1835d3ed) cannot access class com.sun.tools.javac.util.Context (in module jdk.compiler) because module jdk.compiler does not export com.sun.tools.javac.util to unnamed module @0x1835d3ed -> [Help 1]

As you've mentioned in #133 (comment) , this is due to a breaking change with Java 16+.

There is a workaround, which is to add these lines to your .mvn/jvm.config (create the file if it doesn't exist).

--add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED
--add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED
--add-exports jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED
--add-exports jdk.compiler/com.sun.tools.javac.tree=ALL-UNNAMED
--add-exports jdk.compiler/com.sun.tools.javac.util=ALL-UNNAMED

We can't commit this to the repo though, as these flags don't exist for Java 8.

The other option is to develop locally with Java 8 (I use jenv for switching between Java versions locally)

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

sachindshinde · 2021-11-12T23:53:15Z

@lennyburdette
I'm not sure how close this is to ready, so I won't take a look at this now, but let us know when it's ready for review! (Gravity will be mostly busy from Nov. 15th-19th, but I may have spots of availability in between and am pretty available after that.)

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

lennyburdette · 2021-11-15T19:02:37Z

@martinbonnin @sachindshinde thanks for the early review! Got the tests to run and made a first pass at fixes.

This is for a customer. Not a critical blocker, so review in the next couple weeks should be fine.

And please feel free to harshly review my Java!

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

glasser · 2021-12-08T18:35:04Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+    ExecutionResultImpl resultWithCacheControl =
+        ExecutionResultImpl.newExecutionResult()
+            .from(executionResult)
+            .addExtension(EXTENSION_KEY, state.getOverallPolicyString().get())


Extensions are really designed to be passed through to the client as the extension. I don't think it makes much sense to use it as something that is designed to be immediately stripped and returned as an HTTP header.

I especially think this will be confusing because we used to use a cacheControl extension (https://github.com/apollographql/apollo-cache-control) and graphql-java even still has built in support for that (graphql.cachecontrol.CacheControl)!

Other parts of federation-jvm use the "implement an interface on your context Object" pattern. I think something like that makes more sense. However it looks like graphql-java is migrating from "arbitrary context Object" to "specific GraphQLContext object which is a key-value map".

So I would suggest that instead of using instrumentExecutionResult you use something like the onCompleted of beginExecution, and do something like parameters.getGraphQLContext.put(CacheControlInstrumentation.RESPONSE_HEADERS, new Map("cache-control", overallPolicyString)) or something. (Note that the key in this context map can be an arbitrary object so maybe use that instead of a string?

this was helpful, thanks! i didn't realize the context api changed to be a map.

one thing i need help with though — how do i update the test apps in spring-example to pluck the value off the context and add it as an HTTP header?

sorry, i don't know anything about spring.

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

glasser · 2021-12-08T19:20:49Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+    return new SimpleInstrumentationContext<ExecutionResult>() {
+      @Override
+      public void onCompleted(ExecutionResult result, Throwable t) {
+        state.restrictCachePolicy(fieldPolicy);


If you're not implementing dynamic cache control, then you don't actually need to do this on the onCompleted; you can just do it at the end of beginField.

That said, I don't think it would be too hard to implement dynamic cache control! I'd suggest sticking something on the GraphQLContext. Then you could have a static method CacheControlInstrumentation.cacheControlContext(GraphQLContext c) { return (CacheControlContext) c.get(CACHE_CONTROL_CONTEXT) } to let resolvers access it.

You could then follow the pattern of the TS implementation where the _Entity code is just using dynamic cache control instead of being special-cased.

this is good — i'll try implementing it now that i'm using the context for things (instead of extensions).

glasser · 2021-12-08T19:22:28Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+      }
+    }
+
+    void restrict(Hint hint) {


you could probably have Hint extend CacheControlPolicy in which case this duplicate method wouldn't be needed?

glasser · 2021-12-08T19:23:18Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+    }
+
+    void restrict(Integer maxAge) {
+      this.maxAge = Optional.of(maxAge);


This method is doing a replace rather than a restrict. I see it is currently only called in a place where you've verified there's no max age already but it could be used elsewhere later... You could also just remove it and have its caller call replace instead (or well, rename it to replace).

That said it's a little weird that the way this overloaded API works is reliant on the fact that all of the fields have different types. Do we really need these single-field overloads? Vs setters with different names?

fixed this to be a true restrict — i'm open to changing the API but it makes sense to me the way it currently is 🤷🏻

glasser · 2021-12-08T19:28:17Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+    // select the
+    // most restrictive cache policy from those types.
+
+    if (unwrappedReturnType.getName().equals(_Entity.typeName)) {


Ran out of time this morning after reviewing everything except the two blocks of code around _Entity.

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

martinbonnin · 2021-12-14T21:27:39Z

Sorry for the huuuuge delay. I left a few additional comments. Mostly about style and trying to understand the @cacheControl specification a bit better.

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

martinbonnin · 2021-12-20T11:06:52Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+        CacheControlState state = parameters.getInstrumentationState();
+
+        // Attach the policy to the context object
+        state.overallPolicy.maybeAsString().ifPresent(s -> parameters.getGraphQLContext().put(CONTEXT_KEY, s));


Is it possible for state.overallPolicy to actually be null? From what I understand, as soon as there is one field in a query (which should always be the case), there should always be a maxAge? (even if it's the default one)

I have tests that assert that the overall policy is null if no field- or type-level policies are present. If we never encounter a directive, nothing happens at all.

if no field- or type-level policies are present.

Shouldn't it use the defaultMaxAge in these cases? Because there will always be one root field queried and these have a defaultMaxAge ?

ah yes, you're correct. it should be "If we never encounter a directive and no defaultMaxAge is set, nothing happens at all."

so this is related to your next question about whether max-age: 0, public should be emitted or if it should be null like the TS implementation. @glasser do you have an opinion?

Yep, exactly, it's all the same question!

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

martinbonnin · 2021-12-20T11:28:06Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+
+      if (policy.scope != null && (scope == null || !scope.equals(CacheControlScope.PRIVATE))) {
+        this.scope = policy.scope;
+      }


Nit: because you have individual, restrict(maxAge) and restrict(scope) below, I would certainly make them accept nullable values and rewrite this as:

void restrict(CacheControlPolicy policy) { restrict(policy.maxAge) restrict(policy.scope) }

martinbonnin · 2021-12-20T11:31:32Z

It's much more concise now 👍 👍 👍 . Left a few comments. The one about 0 being treated as absent feels important. Let me know what you think

lennyburdette · 2021-12-21T17:14:22Z

I cannot figure out any way to actually use the policy stored on the context when using graphql-java-kickstart/graphql-spring-boot. I looked for any beans/components that had access to both the context/datafetchingenvironment and the http response, but I came up with nothing.

I think this is possible with graphql-kotlin and DGS, so maybe I can just document how that would work? I might be able to get some help from customers on that part.

berngp · 2021-12-22T06:15:11Z

In DGS, both Kotlin and Java, the DgsContext has the request data.

glasser

very small suggestions, other than the hope that we implement dynamic cache control!

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

glasser · 2021-12-23T00:34:44Z

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

+
+      // Cache directive on the return type of this field if it's a composite type
+
+      Optional<CacheControlDirective> directive =


I'm not sure I understand the logic of what you're still using Optional for vs not (I see it's no longer in fields or params which makes sense), but if @martinbonnin is happy with this level of use then I won't complain

I'd personally ditch Optional altogether but don't mind it too much and as long as it's not public API, I think it's fine to ship like this and we can always refine later if/when we want to.

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java

lennyburdette · 2022-01-07T22:39:36Z

@berngp I don't need access to the request data, I need access to the response object so I can add a response header. Is that possible?

berngp · 2022-01-08T18:22:30Z

I don't think we offer that today but I think it is a good idea. Will figure a mechanism you could leverage to do that.

glasser

While I do believe that dynamic cache control is an important feature and that it will be much cleaner to move the Query._entities-specific code into its resolver instead of here, that doesn't need to stand in the way of merging and releasing static cache control support. Looking great!

lennyburdette · 2022-01-11T22:54:18Z

Thank you for the thorough review @martinbonnin and @glasser ! I don't have a merge button so can someone take over the release process for this change?

lennyburdette · 2022-01-11T22:54:48Z

Oh wait, I'll squash all the commits first

@CacheControl

This change adds support for the @CacheControl directive in the form of a graphql-java instrumentation class, matching the behavior of the apollo-server's built-in cache control plugin: https://github.com/apollographql/apollo-server/tree/291c17e255122d4733b23177500188d68fac55ce/packages/apollo-server-core/src/plugin/cacheControl As well as the special _entities behavior in @apollo/subgraph: https://github.com/apollographql/federation/blob/00c89f82bfcc2e27e6714d894874422f84ae468c/subgraph-js/src/types.ts#L76-L87 This change does not support setting cache hints dynamically in resolvers, only static cache hints in SDL directives. The instrumentation adds the value to the GraphQLContext object for the request. Apollo Gateway looks for a cache-control policy in a subgraph's HTTP headers, so it's up to the subgraph service owner to pluck the value off the context and add it as a response header.

martinbonnin · 2022-01-12T10:46:17Z

I can do that. @sachindshinde what do we use in this repo? It's ok to create a merge commit, right? Or do we prefer rebasing and linear history?

martinbonnin · 2022-01-13T16:03:43Z

@sachindshinde said merge commits are ok. Merging now and I will make a new mavenCentral release hopefully later today. Many thanks @lennyburdette !

setchy · 2022-01-13T17:21:58Z

Huge thank you @lennyburdette 👏

martinbonnin · 2022-01-18T22:30:19Z

This is now released in 0.8.0

This has recently been implemented by federation-jvm (see: apollographql/federation-jvm#133) A `CacheControlInstrumentation` instrumentation class must be injected, which adds the appropriate Cache-Control value into the GraphQLContext Fixes Netflix#92

This has recently been implemented by federation-jvm (see: apollographql/federation-jvm#133) A `CacheControlInstrumentation` instrumentation class must be injected, which adds the appropriate Cache-Control value into the GraphQLContext Fixes Netflix#929

This comment has been minimized.

Sign in to view

martinbonnin reviewed Nov 12, 2021

View reviewed changes

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java Outdated Show resolved Hide resolved

martinbonnin reviewed Nov 12, 2021

View reviewed changes

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java Outdated Show resolved Hide resolved

sachindshinde self-requested a review November 12, 2021 23:34

martinbonnin reviewed Nov 12, 2021

View reviewed changes

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java Outdated Show resolved Hide resolved

martinbonnin reviewed Nov 12, 2021

View reviewed changes

.../main/java/com/apollographql/federation/graphqljava/caching/CacheControlInstrumentation.java Show resolved Hide resolved

lennyburdette force-pushed the cache-control branch from 226522f to 4a5a8ff Compare November 15, 2021 18:50

lennyburdette marked this pull request as ready for review November 15, 2021 18:51

lennyburdette force-pushed the cache-control branch from 4a5a8ff to 102b1f0 Compare November 15, 2021 18:55

glasser requested changes Dec 8, 2021

View reviewed changes

martinbonnin reviewed Dec 14, 2021

View reviewed changes