Improve defaultMaxAge setting

[stevez86]

This addresses the following issue: #3559 - where a defaultMaxAge is required and there is no option to not have the blanket defaultMaxAge functionality.

If you elect not to have a defaultMaxAge of 0 set for all resolvers and types you must explicitly set it to null. For example:

const server = new ApolloServer({
  // ...
  cacheControl: {
    defaultMaxAge: null,
  },
}));

const typeDefs = `
  type Foo {
    id: ID!
    name: String!
  }

  type User {
    id: ID!
    name: String!
    status: String!
    foo: Foo!
  }

  type Query {
    user: User @cacheControl(maxAge: 0, scope: PRIVATE)
    userFoo: User @cacheControl(maxAge: 100)
  }
`;

// fooUserQuery
query fooUser {
  id
  name
  status
  foo {
    id
    name
  }
}

// userQuery
query user {
  id
  name
  foo {
    id
    name
  }
}

This is useful if you have multiple queries of identical types like in the example shown, query user will have a maxAge of 0 while query userFoo will have a maxAge of 100 which would not be possible under the current usage of defaultMaxAge because you can't set the child to have two different maxAges depending on parent.

[apollo-cla]

@stevez86: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

[glasser]

So, the way that maxAge is documented is:

By default, root fields (ie, fields on Query and Mutation) and fields returning object and interface types are considered to have a maxAge of 0 (ie, uncacheable) if they don't have a static or dynamic cache hint. (Non-root scalar fields inherit their cacheability from their parent, so that in the common case of an object type with a bunch of strings and numbers which all have the same cacheability, you just need to declare the hint on the object type.)

If I understand correctly, you are not reporting a bug in the implementation where it does not match the docs, but you're saying that the documented and implemented behavior is not useful for you. Is that correct?

I definitely agree that these are very surprising semantics; even when I personally implemented one generation of our caching support I found it odd. But I also think that just changing the default semantics of the cache control annotations incompatibly is not advisable.

Fundamentally, the problem is that it's not knowable whether your User.foo field is "just some sub-data of the User that was fetched at the same time as the User with the same caching semantics" or "an entirely different lookup that has its own caching semantics".

The choice made by the Apollo cache-control spec is to err on the side of under-caching by assuming any object-typed field is likely to be a separate lookup by default (but that scalars are likely to be "part of" their parent types).

My understanding is that this PR lets you write defaultMaxAge: null to make the opposite assumption: that by default, object-typed fields inherit from their parents. (I haven't evaluated the PR's code completely, though.)

Rather than just change the default in such a blunt fashion, it seems to me it would be better to extend the spec to make the actual semantic concept more explicit. ie, something like @cacheControl(inherit: true) to mean "this object field is just a part of its surrounding and should work like it". (Like any other @cacheControl, you could put it on the User.foo field or the Foo type.) You could imagine @cacheControl(inherit: false) switching a scalar field back to defaultMaxAge as well, though I'm not sure if that's useful.

My mind is far enough from the details of how this works today that I'm not sure if @cacheControl(inherit: true) is basically the same as @cacheControl(maxAge: null) and if it would make sense to just use that syntax instead, though I do think there's some added clarity to spelling out the concept of "inherit".

Does that make sense?

[stevez86]

If I understand correctly, you are not reporting a bug in the implementation where it does not match the docs, but you're saying that the documented and implemented behavior is not useful for you. Is that correct?

That is correct. While confusing, it's not a bug per the documentation. It's an enhancement in functionality and is closer for me to the intuitive functionality that I assumed when initially implementing caching. I only recently after a couple years fully understood what it was doing because of this PR.

The choice made by the Apollo cache-control spec is to err on the side of under-caching by assuming any object-typed field is likely to be a separate lookup by default (but that scalars are likely to be "part of" their parent types).

I agree with that approach but the way its implemented for a fairly common use case actually leads to a less conservative development environment that I believe is contrary to the goal of the Apollo team.

The problem with the current implementation is that as soon as I say I want to cache (by setting cacheControl to an object or true) I am then forced to use a defaultMaxAge which by default is 0 - there is no opt-out.

Now take my example above, which has two resolvers returning an object that needs to be cached in one instance and definitely not cached in another. The only way (that I'm familiar with) to implement this use case in the current implementation is to set a defaultMaxAge to my highest cache setting and then set the cache for the user resolver to 0. What this means is that I have to default to an unsafe (high cache) setting for my entire schema and manually make sure all protected fields are set to a cache age of 0 - definitely contrary to the methodology that you mentioned Apollo was taking.

Rather than just change the default in such a blunt fashion, it seems to me it would be better to extend the spec to make the actual semantic concept more explicit. ie, something like @CacheControl(inherit: true) to mean "this object field is just a part of its surrounding and should work like it". (Like any other @CacheControl, you could put it on the User.foo field or the Foo type.) You could imagine @CacheControl(inherit: false) switching a scalar field back to defaultMaxAge as well, though I'm not sure if that's useful.

Completely agree and I'd be happy to work towards that, but I think there is enough confusion about what defaultMaxAge actually does (#3559) (it's not clear / intuitive, even with the solid documentation around it) to warrant the null option on its own before going the extra mile. The default option is completely opt-in. Also, I'm not sure about the current state of upgrading to a version 3 - which might be a good time to look at this if its approaching soon (not sure).

Take a look at the code change (its basically 2 simple lines + tests). It doesn't break existing functionality and is opt-in. If you don't opt-in, the current implementation remains the same. If you opt-in no cache headers are set without the developer setting them. And it solves the problems and confusion from here: #3559

[glasser]

So I don't think your fix will work well, because not only does it ignore object fields nested inside cachecontrolled fields, it also ignores root fields.

So eg, with

  type Query {
    uncachedUser: User
    cachedUser: User @cacheControl(maxAge: 100)
  }

Given the query { uncachedUser { id } cachedUser { id } }, your code (with defaultMaxAge:null) wouldn't add any hints to this.hints for uncachedUser, and computeOverallCachePolicy would return 100 despite an entirely uncached field.

You could make the conditional more complex to explicitly treat root fields differently than nested object/interface fields. But I do think that making the concept of inheritance explicit is the way to go. While I think the original design of "assume every sub-object might be a separate database call" was overly conservative, "fixing" this by only providing the option of "assume every sub-object was fetched as part of its parent" seems far too blunt.

[stevez86]

I see, I didn't think of that and agree that a more explicit inheritance is a good way to go. Thank you for your feedback and time looking at this.

[glasser]

Here's an implementation of something that's similar to the inherit concept I discussed above: #5247
I'm interested in feedback from folks who were interested in this issue!