3.1.1-SNAPSHOT - CacheMissException: Object 'id=xxx' has no field named '__typename' when querying cache with addTypename.set("ifAbstract")

[mateuszkwiecinski]

Summary
I wanted to try out changes introduced as a fix for: #3774 with the new addTypename.set("ifAbstract") option. I was still getting com.apollographql.apollo3.exception.CacheMissException: Object 'id=xxx' has no field named '__typename' so I tried to come up with a repro.

schema.sdl

type Viewer {
  shelf(id: ID!): Shelf!
  nodes(ids: [ID!]!): [Node]!
}

type Shelf {
  id: ID!
  books: [Book!]!
}

type Book implements Node {
  id: ID!
  name: String!
}

interface Node {
  id: ID!
}

queries

query Shelf($id: ID!) {
  viewer {
    shelf(id: $id) {
      id
      books {
        ...bookFragment
      }
    }
  }
}


query BooksByIds($ids: [ID!]!) {
  viewer {
    nodes(ids: $ids) {
      ...bookFragment
    }
  }
}


fragment bookFragment on Book {
  id,
  name
}

fails to fetch BooksByIds from cache, after pre-fetching it with Shelf query.
Am I wrong to expect ifAbstract to work in this case too? The test I added in the repro passes if I use the ifFragment option which I understood is considered as deprecated and is scheduled for removal.

Version
3.1.1-SNAPSHOT from ~yesterday.

Description
Here's full repro: mateuszkwiecinski/apollo-playground@dda7a0d

[martinbonnin]

Thanks a lot for the reproducer 💜 . This is perfect timing as we wanted to make a release soon so we'll make sure this works before releasing.

[martinbonnin]

So what's happening here is that __typename is not added in the first query but is required in the second. The transformed queries are like this:

query Shelf($id: ID!) {
  viewer {
    shelf(id: $id) {
      id
      books {
        # __typename is not added here because Book is a concrete, not-abstract type
        ...bookFragment
      }
    }
  }
}

query BooksByIds($ids: [ID!]!) {
  viewer {
    nodes(ids: $ids) {
      # __typename is added here because Node is abstract
      __typename
      ...bookFragment
    }
  }
}

I'm not sure there's a silver bullet here. I can see three possible solutions:

Solution1: "tryHard" 😅

This is the solution where we really want to minimize the number of extra __typename. We could have a global pass traversing all queries and remembering if a given type is queried through an interface (like is the case for Book through the Node interface here). If it's the case, we add __typename every time this field is queried.

Solution2: "ifImplementsAbstractType" (naming TBC)

This is the intermediate solution. In addition to abstract types, we also add __typename unconditionally whenever the field type implements an interface or is part of an union to make sure we have a __typename if the field is queried through an interface (or union)

Solution3: "always"

Just add __typename always but that means we're back to step 1.

Thoughts

I'm not sure there's a silver bullet. "ifFragments" isn't great because of #3672 (comment) so this one is certainly going to go away. For your specific case, I think 2. is the way to go (and certainly a good default).

We could go "tryHard" but TBH I don't think it's worth it. This not only would take some build time but would also make the transformed queries harder to understand.

I'll try to get solution2 in a SNAPSHOT so you can try it. Looks like you're not too wary of extra __typename, right? So it should be ok?

[martinbonnin]

Thinking a bit more about this, I think I'm overthinking this 🙃 . Even "ifImplementsAbstractType" is not really bulletproof because it's not robust to schema changes that add an interface to an existing concrete type.

All in all, for 3.2.0 I don't think we need "ifImplementsAbstractType" so unless you have a strong objection, I'll certainly remove it from the PR linked above. In the long run I think we should aim for these options:

• "always": bullet proof option if using the cache
• "ifAbstract": simple option if not using the cache
• "ifPolymorphic": bare minimum of __typename

We'll keep "ifFragments" in the short term for compat reasons and remove it in the next major version.

In your specific case, that certainly means using "always" which is back to what 2.x is doing but I don't see an easy way out of adding __typename everywhere?

[mateuszkwiecinski]

Thanks, Martin, for taking care of it almost immediately, and my apologies for not taking part in the discussion earlier 😬 Fortunately I don't have much to add 😛

If always is the only option which ensures valid cache data at a cost of adding __typename overhead to each type then that's OK for us :) That's going to be the same behaviour as in 2.x, so I think that's fine.
The only thing I'd like us to avoid is to explicitly add __typename to our queries - I'd be afraid we'd miss something, I don't think we have 100% of use cases covered with tests.

I also think these three choices you listed above would be enough in most cases, where one can pick performance vs idempotency to schema changes. The only idea not mentioned here (and I can think of right now) is to allow consumers to provide custom strategy when __typename gets added. I'm fairly sure we'd still go with always, but I can imagine someone wanting to optimize their queries and trying to remove __typename from types they know they won't ever have more than a single implementation, but yeah, that doesn't seem like a feature worth implementing right now.

I'll go ahead and close this issue. 3.2.0 introduced the always and it isn't scheduled for removal, so my original issue has been addressed, thanks!

[martinbonnin]

Thanks! And apologies for rushing a bit the release. We've been sitting on a few important changes for some time and this release was direly needed. We can always do a patch release if you find anything during testing.

Regarding "always", I don't really see a way around it. It's slightly better than 2.x in that it saves __typename in inline fragments containing inline fragments:

{
  animal {
    ... on Pet {
       # 2.x adds __typename here, "always" doesn't, it only adds at the root of the "animal" field
       ... on Cat {
         meoww
       }
    }
  }
}

allow consumers to provide custom strategy when __typename gets added

Indeed, that's something I've been contemplating for a while but feeding Gradle with custom code is awkward. Basically, we'd need to have user-supplied code as an input to a Gradle task and this comes with a few challenges like how to know whether the logic has changed (for up-to-date checks) or exposing more APIs in the Gradle plugin which wouldn't get relocated. We're doing this for OperationOutputGenerator where the user has to manage up-to-date checks manually by bumping a version but I'm not a fan of this.

After playing with Gradle a bit, I think the idiomatic way to provide user-defined logic to Gradle tasks is to actually put the logic in a new Gradle task itself. In this case, the user could create a new AddTypename task that takes the .graphql files and copies them (with added __typename) to some other places in build/something and then wires the output to the codegen with srcDir(). It's some cost to setup but I don't really see a way around this. And it's all possible with the existing APIs actually. If you ever end up needing something like this, open a new issue and we can investigate.