feat: Specify caching fields with typePolicy directive

[x-sheep]

This is based on the discussions within #549, and matches the functionality of the typePolicy directive in Apollo Kotlin (specifically, the non-experimental keyFields argument)

[netlify]

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	c40b446

[netlify]

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	c40b446

[svc-apollo-docs]

❌ Docs Preview Failed

Error

Error: ENOSPC: no space left on device, mkdir '/tmp/librarian/remote-sources/x-sheep/apollo-ios-dev/typepolicy'

[AnthonyMDev]

Wow! Thank you so much for taking the time to implement this! It's been something we've wanted to do for a long time, but there have always been other, higher priorities.

Honestly, I had assumed this was going to require a LOT more work than this, which is why we hadn't done it yet. But this seems to be a complete implementation of the feature. I can't believe it's only 550 lines!

This is some really, really great work @x-sheep! I have a few questions and suggestions, but this is nearly ready to ship. Would love to get eyes on this from @calvincestari and @BobaFetters on the iOS team, as well as a quick look from @martinbonnin or @BoD to ensure that the behavior is aligned with the Kotlin codebase.

We'll also need to add some new documentation for this feature, but we can probably borrow heavily from the documentation on the Kotlin side.

[AnthonyMDev]

I assumed we would implement this by adding keyFields to both GraphQLObjectType and GraphQLInterfaceType and then looking for them on the object, and if they don't exist, then going through the interfaces.

The way you are implementing this, we do that logic in the typePolicyDirectiveFor in the JS code, computing what keyFields each object would have and then applying them to all the objects. I do actually think that feels better!

---

There is a possibility of using the keyFields on interfaces with "unknown types", but it has enough complications that I think it's probably okay to leave that out of scope for now. But just some thoughts on how that could work for future reference:

If a type is added to a schema after the client has been shipped, but it implements an interface that had a defined typePolicy, we should theoretically be able to compute a cache key for it without even knowing about that object type. This would require us to hold onto the keyFields of the interface types themselves though, and then apply the logic for determining keyFields at runtime rather than at "code-gen-time".

This also runs into some minor edge cases (eg. if the type implements multiple interfaces that have conflicting @typePolicy directives) which would need to be addressed.

We also wouldn't be able to actually know all of the interface types that an unknown type implemented. We could only infer that if a type with an unknown __typename is returned by the server for a field of an interface type, then that type must implement that interface. But it could also implement other interfaces and be returned as the type for a field with another interface that has a different @typePolicy. In that case, the cache would not normalize the object properly because it would compute it with two different cache keys.

This is a super niche and likely very rare edge case, but I think it is enough of a consideration that we should not handle computing cache keys for unknown types via inheritance of an interface's @typePolicy at this time. I have a vision for a future in which we can do introspection queries and apply new type information to the schema at runtime. If that ever comes to fruition, we could reconsider this.

[AnthonyMDev]

Nit: Should keyFields be optional, and if there are none we return nil instead of an empty array?

[x-sheep]

I made keyFields required, just to make sure the data is loaded correctly from the Javascript environment. The same field is Optional in the generated code.

[martinbonnin]

This is a super niche and likely very rare edge case, but I think it is enough of a consideration that we should not handle computing cache keys for unknown types via inheritance of an interface's @typePolicy at this time.

Potential counter argument:

interface Node @typePolicy(keyFields: "id") {
  id: ID! 
}

In schemas with global ids, that's a convenient way do add id to all objects that inherit Node. Agree it raises a bunch of questions about potential inconsistencies. Maybe those consistencies could be checked on the server side, not sure.

FWIW, Kotlin currently keep tracks of key fields on interfaces at runtime too. Also because we don't have a global "typename -> object" dictionnary at runtime and didn't want to introduce it just for @typePolicy. So there's a small discrepency there but I think it's ok, maybe even good at this point. We can iterate various approaches and get field feedback before commiting to the final behaviour.

[x-sheep]

extend interface Node @typePolicy(keyFields: "id")

Every type I want to use in my specific iOS project has a base interface with the ID field, so it's important to me that the directive works on interfaces.

If a GQL server introduces a new type for an old endpoint used by a Client app that isn't up to date, it will now just miss the directive because the Object declaration doesn't exist. It's still possible to cache it anyway using Programmatic Cache Keys.

[x-sheep]

Should this be addressed in a separate PR? If yes, should it be done before or after merging this @typePolicy branch?

[AnthonyMDev]

I think we do need to add the keyFields to interface types and add that to the handling of keyFields for a type that is not a known generated type.

Even if we aren't handling all the cases of "unknown types" this is important because we are currently building a feature to allow many concrete types to not be generated (if they aren't directly referenced, but only can be returned for a field from an interface).

So this means that ALOT of known types would not normalize properly as well, as they would appear at run-time to be unrecognized types.

---

I really would have loved for us to be able to solve the issues with handling unknown types here in a more elegant way, but for now, I think this is what we need to do. Which, to be fair, is actually just mirroring what Apollo Kotlin does currently, and they haven't seen a lot of issues come up on that side. The only thing that's not working properly here is handling cache normalization for an unknown (or not-generated) type that, conforms to two interfaces with conflicting @typePolicy directives. But it's a very rare, minor edge case, and there isn't a great way to fix for that right now. So I think that we can't let that hold up pushing this feature.

[martinbonnin]

+1. Also about this part:

The only thing that's not working properly here is handling cache normalization for an unknown (or not-generated) type that, conforms to two interfaces with conflicting @typePolicy directives.

Longer term, we could add linting/schema checks to catch that before the schema is actually put in production.

[x-sheep]

This PR already throws an error if any type is found that has conflicting typePolicies through its inherited interfaces. As soon as the schema is updated, any new conflict will become apparent. The fact that it'll use the typePolicy of just one of the interfaces until then is unfortunate, but IMHO very unlikely.

I'm not aware of what the changes look like when Apollo iOS generates less code, but if the entire schema is still validated in the Javascript/IR step this will still catch conflicting policies (regardless of their usage by the client).

I can update this PR to add typePolicy information to the Interfaces as well as the Objects, so it will also run the above validation on interfaces without (known) object types.

[x-sheep]

I've now added keyFields to the generated Interface instances.

[x-sheep]

I added the directive to one of the schemas, but I didn't regenerate the example sources since that would add another 100 files to the Pull Request

[AnthonyMDev]

That's fine while we do code review. But we should go ahead and generate those files and verify they look correct and compile prior to merging.

[martinbonnin]

This is a super niche and likely very rare edge case, but I think it is enough of a consideration that we should not handle computing cache keys for unknown types via inheritance of an interface's @typePolicy at this time.

Potential counter argument:

interface Node @typePolicy(keyFields: "id") {
  id: ID! 
}

In schemas with global ids, that's a convenient way do add id to all objects that inherit Node. Agree it raises a bunch of questions about potential inconsistencies. Maybe those consistencies could be checked on the server side, not sure.

FWIW, Kotlin currently keep tracks of key fields on interfaces at runtime too. Also because we don't have a global "typename -> object" dictionnary at runtime and didn't want to introduce it just for @typePolicy. So there's a small discrepency there but I think it's ok, maybe even good at this point. We can iterate various approaches and get field feedback before commiting to the final behaviour.

[AnthonyMDev]

That's fine while we do code review. But we should go ahead and generate those files and verify they look correct and compile prior to merging.

[AnthonyMDev]

Thinking about this more, it seems more complicated than we realized. We are in the process of implementing a feature to reduce the number of generated object types. Specifically because of this common practice of using a Node interface. We've had complaints from users of massive generated schemas that are problematic for binary size.

We intend on removing the generation of all of the object types for every type that implements the referenced interfaces. Unless the concrete types are directly referenced themselves, we don't want to generate all of them anymore.

In this situation, types that are not referenced directly will behave the same way as "unknown types" (types added to the schema after code-gen). This makes these edge cases a LOT more likely to appear and means that the approach we're using here of adding keyFields to only the object types will likely not be sufficient.

We are definitely going to need to add the keyFields to the interface types as well. In most cases, when the object type does not exist, we could infer the @typePolicy from the interface type for a field. But this still winds up causing a lot of complications that I'm not quite sure how to address. It sounds like in Apollo Kotlin we aren't really handling those complications right now. But I'd like us to try and come up with an approach to those issues here.

1. What if a type that does implement an interface with a @typePolicy is returned for a field of another interface that does not have a @typePolicy? We wouldn't know that it has an appropriate @typePolicy to use at run time.

2. What if a type is returned from two fields with different interface types that have conflicting @typePolicy directives?

[x-sheep]

Is this the correct SelectionSet?