Add a mechanism to detect when an object has an id but we're not requesting it

[edenman]

Is your feature request related to a problem? Please describe. Our app has been plagued by issues where the cache gets corrupted because a query forgets to request the id of a top-level object. Example flow:

• MeQuery runs and requests me { id } and some other fields.
• The cache has a root record lookup from Me -> [the actual id]
• Another query FooQuery requests me { some other fields }
• The root record lookup is overwritten for Me. Now cache lookup fails for MeQuery because the root record lookup is broken.

Describe the solution you'd like I'd love for Apollo to not overwrite the root record lookup in these cases. Failing that, I'd love a way to force my app to crash if the schema has an id value but the id is not requested. I tried adding an ApolloInterceptor to do that but 1) there doesn't seem to be any way to introspect the requested fields 2) there definitely doesn't seem to be any way to compare those with the schema fields.

[sav007]

Not really sure that crashing will be a right approach here. As it looks like use case specific behaviour, some cases might need the existing behaviour.

Another idea is kinda related to this https://github.com/apollographql/apollo-android/issues/1458 as well, is to have some plugin configuration to enable / disable auto include __typename and ID into each selection set.

[edenman]

yeah sorry, I'm not suggesting that there's a built-in crash behavior, just that I have the ability to add such a behavior to my app.

I'd be totally in favor of auto-including ID for any object that has one. Or adding introspection fields/methods on the codegen'd object.

[martinbonnin]

Apollo Android 3 allows defining cache keys at build time using the @typePolicy directive.

While that's not 100% what this issue is about (as it will not catch the missing ids if for some reason, they need to be defined programmatically), this should considerably simplify the cache setup. Can you take a look at the Apollo Android 3 release candidate and let us know what you think?

[lwasyl]

Just from a cursory glance, without trying this out in practice, do I understand correctly that this would require defining a cache ID for each type that we use that has an ID? My first thought is that it's much less convenient than the old approach, since we know we have id field in some 99% of the types. At the same time, it's much safer if it's validated in compile-time somehow. Can you clarify:

• how, with the new approach, how can I be sure that we don't forget to add @typePolicy extension when fetching a new type with some new query?
• what happens if there's no type policy defined for a type?

this should considerably simplify the cache setup

Right now our CacheKeyResolver is ~26 lines, because most types have an id: String! field, and the solution from this issue was to force fetching id if it exists to make sure the cache resolver can use it. We don't have to modify the resolver as long as we have and fetch the id. It seems to me that with the new approach, it'd be much easier to forget to extend a type with type policy than to forget fetching an id from the object. Btw for context, we fetch ~120 distinct types across our responses right now, which is not much, but it'd be at least 120 type policies defined. I'm concerned that without automated checks, this is not very safe to maintain

[martinbonnin]

Excellent point. As a preamble, I'll say that both approaches will keep being maintained in the long term. Using programmatic ids like on 2.x is (and will be) definitely possible.

Now, when it comes to having to write @typePolicy multiple times (~120 in your case), you're right that it's cumbersome and error prone. I'd argue that adding @typePolicy at the schema level is still better than having to add id to each individual query and nested field. But still far from ideal indeed.

The medium term plan for this would be to introduce schema transforms. I don't have a specific API in mind but we could think of something like this in your Gradle configuration:

apollo {
  service("starwars") {
    keyFieldsForTypesContaining("id")
  }
}

Moving forward, it could be a different syntax or GraphQL extensions to allow more arbitrary transforms of the schema (non-nullability comes to mind). This is not in 3.0 yet mainly because we wanted to ship something and this is the kind of stuff that can be done without breaking the API (because it's mainly additions). But definitely something we could add. Would that work for your use case?

[lwasyl]

adding @typePolicy at the schema level is still better than having to add id to each individual query and nested field.

Do you mean that the query would not have to define that it fetches id, but codegen will fetch it anyway because the extension defines a type policy with id field for it? That is, I would define type policy for type Foo, and then I can do foo { bar, baz } and it'll fetch id regardless?

About schema transformations: I'm not sure I understand how the transformations would work, what would happen with the example keyFieldsForTypesContaining("id") declaration?

[martinbonnin]

That is, I would define type policy for type Foo, and then I can do foo { bar, baz } and it'll fetch id regardless?

Yes, just like for __typename, the codegen will add the id field automagically.

what would happen with the example keyFieldsForTypesContaining("id") declaration?

You can think of it scanning all the types that have an "id" field and generating the type extensions automatically:

extend type Repository @typePolicy(keyFields: "id")
extend type User @typePolicy(keyFields: "id")
extend type Commit @typePolicy(keyFields: "id")
# etc 120 times...

# if a type doesn't have an id field, it's not extented

Whether this is effectively readable in extra.graphql or any other file is still TBD. It might be useful for debugging/incremental Gradle compilation but it's not strictly required.

[lwasyl]

👍 Thanks for clarifying, so as far as I understand the new type policy doesn't exactly address this issue, but some mechanism like keyFieldsForTypesContaining definitely would 🙂

[martinbonnin]

Thanks for sharing feedback about this. This is super helpful 🙏 . We'll update this post when keyFieldsForTypesContaining is in (or any other automated way to setup the @typePolicy)