Skip to content
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

Make cache policy more useful #5248

Merged
merged 4 commits into from
Jun 2, 2021
Merged

Make cache policy more useful #5248

merged 4 commits into from
Jun 2, 2021

Conversation

glasser
Copy link
Member

@glasser glasser commented May 28, 2021

Previously, all the logic around updating the cache policy with new
hints was hardcoded inside the cache control plugin. Also,
overallCachePolicy undefined was used to represent two distinct
states: "we don't know anything about cache policy yet" and "definitely
uncacheable". There was a PolicyUpdater class that was part of the CC
plugin that handled the logic of updating a policy.

After this PR, overallCachePolicy is always defined. The former case
is maxAge === undefined and the latter case is maxAge === 0. This
object also has some helpful methods that don't exist on CacheHint:

  • restrict and replace applies a CacheHint to it; the former can
    only make the policy more restrictive, whereas the latter overrides
    the current values of whatever fields are defined in its argument
  • cacheablePolicy either returns null (if we have no information about
    cacheability or if we know it's not cacheable) or returns a
    CacheHint whose maxAge is positive and whose scope is defined.
    These make PolicyUpdater unnecessary.

Inside a resolver, info.cacheControl.cacheHint also has these methods!
So while you can still do info.cacheControl.setCacheHint(hint) (which
is identical to info.cacheControl.cacheHint.replace(hint)), you also
have the ability to do info.cacheControl.cacheHint.restrict(hint) if
that's what you'd like to do.

Additionally, the maxAge and scope fields on
info.cacheControl.cacheHint now update if you call restrict,
replace, or setCacheHint.

Also, resolvers now have access to info.cacheControl.cacheHintFromType
which it can use to extract cache hints from the schema. (This may be
helpful for a resolver that returns an abstract type, such as
Federation's Query._entities).

Also, extend type @cacheControl(...) directives are now honored
instead of silently ignored. There's also a cache for directive parsing
instead of examining the AST in detail every single time a field is
resolved.

This change is mostly backwards-compatible, unless you accessed
requestContext.overallCachePolicy directly (eg from a plugin). In that
case:

  • If you relied on requestContext.overallCachePolicy being undefined
    to mean "not cacheable", you should instead see if
    requestContext.overallCachePolicy.policyIfCacheable() is null.
  • If you assigned a CacheHint to requestContext.overallCachePolicy,
    you should pass it to requestContext.overallCachePolicy.replace()
    instead.

@glasser glasser force-pushed the glasser/cache-policy branch 2 times, most recently from 1df9ee2 to cdedd85 Compare May 28, 2021 04:34
export function ApolloServerPluginCacheControl(
options: ApolloServerPluginCacheControlOptions = Object.create(null),
): InternalApolloServerPlugin {
const typeCacheHintCache = new Map<GraphQLCompositeType, CacheHint>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to double-check that these are acceptable map keys. ie (a) we need to get identical objects repeateedly and (b) the object can't be the same when the schema is updated.

Also maybe this needs to be replaced with an LRU cache since otherwise this is a memory leak as the schema changes over time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to an LRU cache.

Previously, all the logic around updating the cache policy with new
hints was hardcoded inside the cache control plugin. Also,
`overallCachePolicy` undefined was used to represent two distinct
states: "we don't know anything about cache policy yet" and "definitely
uncacheable". There was a `PolicyUpdater` class that was part of the CC
plugin that handled the logic of updating a policy.

After this PR, `overallCachePolicy` is always defined. The former case
is `maxAge === undefined` and the latter case is `maxAge === 0`. This
object also has some helpful methods that don't exist on `CacheHint`:
- `restrict` and `replace` applies a `CacheHint` to it; the former can
  only make the policy more restrictive, whereas the latter overrides
  the current values of whatever fields are defined in its argument
- `cacheablePolicy` either returns null (if we have no information about
  cacheability or if we know it's not cacheable) or returns a
  `CacheHint` whose maxAge is positive and whose scope is defined.
These make `PolicyUpdater` unnecessary.

Inside a resolver, `info.cacheControl.cacheHint` also has these methods!
So while you can still do `info.cacheControl.setCacheHint(hint)` (which
is identical to `info.cacheControl.cacheHint.replace(hint)`), you also
have the ability to do `info.cacheControl.cacheHint.restrict(hint)` if
that's what you'd like to do.

Additionally, the `maxAge` and `scope` fields on
`info.cacheControl.cacheHint` now update if you call `restrict`,
`replace`, or `setCacheHint`.

Also, resolvers now have access to `info.cacheControl.cacheHintFromType`
which it can use to extract cache hints from the schema. (This may be
helpful for a resolver that returns an abstract type, such as
Federation's `Query._entities`).

Also, `extend type @CacheControl(...)` directives are now honored
instead of silently ignored. There's also a cache for directive parsing
instead of examining the AST in detail every single time a field is
resolved.

This change is mostly backwards-compatible, unless you accessed
`requestContext.overallCachePolicy` directly (eg from a plugin). In that
case:
- If you relied on `requestContext.overallCachePolicy` being undefined
  to mean "not cacheable", you should instead see if
  `requestContext.overallCachePolicy.policyIfCacheable()` is null.
- If you assigned a `CacheHint` to `requestContext.overallCachePolicy`,
  you should pass it to `requestContext.overallCachePolicy.replace()`
  instead.
@glasser glasser force-pushed the glasser/cache-policy branch from cdedd85 to ad6cfa3 Compare June 2, 2021 18:45
@glasser glasser force-pushed the glasser/cache-policy branch from 3700077 to b19b1e8 Compare June 2, 2021 19:12
@glasser glasser merged commit 8bc07a0 into release-3.0 Jun 2, 2021
@glasser glasser deleted the glasser/cache-policy branch June 2, 2021 19:26
benweatherman added a commit to apollographql/federation that referenced this pull request May 27, 2022
This got changed in apollographql/apollo-server#5248 but the code using it calls out we need to maintain compatibility with Apollo Server https://github.com/apollographql/federation/blob/d900859c766645cd617eb865864d326c0431a380/gateway-js/src/datasources/RemoteGraphQLDataSource.ts#L116-L124

This fixes the following Typescript validation error:

```
gateway-js/src/datasources/RemoteGraphQLDataSource.ts:122:7 - error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?

122       options.incomingRequestContext.overallCachePolicy?.restrict
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

It _seems_ like we should've run into this previously, since this the ternary version of this check was fixed in TS 3.9.0 according to microsoft/TypeScript/#36048
benweatherman added a commit to apollographql/federation that referenced this pull request Jun 24, 2022
This got changed in apollographql/apollo-server#5248 but the code using it calls out we need to maintain compatibility with Apollo Server https://github.com/apollographql/federation/blob/d900859c766645cd617eb865864d326c0431a380/gateway-js/src/datasources/RemoteGraphQLDataSource.ts#L116-L124

This fixes the following Typescript validation error:

```
gateway-js/src/datasources/RemoteGraphQLDataSource.ts:122:7 - error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?

122       options.incomingRequestContext.overallCachePolicy?.restrict
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

It _seems_ like we should've run into this previously, since this the ternary version of this check was fixed in TS 3.9.0 according to microsoft/TypeScript/#36048
benweatherman added a commit to apollographql/federation that referenced this pull request Jun 24, 2022
* chore(deps): update all non-major dependencies

* Treat GraphQLRequestContext.overallCachePolicy as optional

This got changed in apollographql/apollo-server#5248 but the code using it calls out we need to maintain compatibility with Apollo Server https://github.com/apollographql/federation/blob/d900859c766645cd617eb865864d326c0431a380/gateway-js/src/datasources/RemoteGraphQLDataSource.ts#L116-L124

This fixes the following Typescript validation error:

```
gateway-js/src/datasources/RemoteGraphQLDataSource.ts:122:7 - error TS2774: This condition will always return true since this function is always defined. Did you mean to call it instead?

122       options.incomingRequestContext.overallCachePolicy?.restrict
          ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```

It _seems_ like we should've run into this previously, since this the ternary version of this check was fixed in TS 3.9.0 according to microsoft/TypeScript/#36048

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Ben <benweatherman@gmail.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant