Handle @typePolicy in unions interfaces

[BoD]

In this PR:

• key fields and __typename (including fragments for possible types) are added by the Apollo compiler plugin
• a new Cache.keyFields map is generated containing the key fields of all types
• this can be passed to a new KeyFieldsCacheKeyGenerator which looks up the __typename in that map

TypePolicyCacheKeyGenerator is deprecated but still the default - a tradeoff to avoid a breaking change now that we're alpha, and since using the compiled fields still works in most cases.

[martinbonnin]

That unfolds nicely 👍

[martinbonnin]

Reminder to remove the @ApolloExperimental from v5

[martinbonnin]

Do we really want to use responseName() here?

{
   product {
     id: price
   }
}

Because responseName() is id, we'll skip adding the id keyfield below? Which doesn't look like what we want?

[BoD]

Ah interesting - that was to handle that case:

{
   product {
     myId: id
   }
}

here we still want to add id.

There is a check below to disallow using aliases that clash with key fields, but this is broken. We can make this check first. Also this needs a unit test 😅

Good catch! 👍

[martinbonnin]

Since the map is going to be used with object __typename, there is no need to codegen the interfaces key fields?

Which yields the question that new types (unknown at build time) will now have a bad cache key I think? (because the lookup will be empty?)

[BoD]

I figured we need the interface key fields for that case:

union Union1 = Type1 | Type2

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

type Type1 implements Node {
  id: ID!
}

type Type2 @typePolicy(keyFields: "serialNumber") {
  serialNumber: String!
}

When union1 returns a Type1 we need to know its key field inherited from Node.

[martinbonnin]

Right but there is no need to add the interfaces inside the codegened map, or is there?

object Schema {
  val keyFields = mapOf(
    "Type1" to setOf("id"),
    "Type2" to setOf("serialNumber"),
    // no need to codegen "Node" here?
  )
}

[BoD]

Ah! No you're right, only types are needed.

[martinbonnin]

Here, if the object isn't known at compile time, I believe we get a bad cache key even though it would be technically possible?

{
  animal { 
    __typename # even if typename is not a known animal, we can still compute a cache key 
    id 
    species
  }
}

[BoD]

Ah good point. So we should fallback to the schema type and its interface(s), found in the compiled field.

[martinbonnin]

If we want to support this, I think so. I think this is what iOS is doing too.

[martinbonnin]

Should required fields be added in the !isRoot case? I think isRoot is already adding everything needed?

• for concrete types, it adds __typename and keyfields.
• for abstract types, it adds __typename and inline fragments for all possible types.

I think that coveres it all? So probably we don't even need to track parentTypes ?

[BoD]

👀 Yeah I think that's right. I'll open a fix.

[BoD]

I remember now, it was to not add new fragments and instead re-use existing ones.

original:

query GetUser {
  user {
    ... on Person {
      name
    }
  }
}

if we only add fields in isRoot:

query GetUser {
  user {
    __typename
    ... on Person {
      name
    }
    ... on Robot {
      serialNumber
    }
    ... on Person {
      id
    }
  }
}

if we also do it in !isRoot:

query GetUser {
  user {
    __typename
    ... on Person {
      name
      id
    }
    ... on Robot {
      serialNumber
    }
  }
}

In the end in the generated code the fragments are collapsed, so we'll still get only one onPerson, right? But this removes a bit of duplication in the sent document. Not sure if it's better to be 'clever' or simple here :)

[martinbonnin]

Aren't the inline fragments unconditionally added in the root of each field?

    // Unions and interfaces without key fields: add key fields of all possible types in inline fragments
    val inlineFragmentsToAdd = if (isRoot && keyFields.isEmpty()) {

[martinbonnin]

I like the consistency of adding the inline fragments always.

PS: it would be nice to have a comment this field was automatically added by the cache compiler plugin (but I'm not sure we can do it at the moment because our AST doesn't support comments)

[BoD]

Aren't the inline fragments unconditionally added

alreadyHandledTypes (maybe badly named 😬) is the trick

consistency

Yeah that may be a bit more obvious for people looking at the resulting document 👍 . And agreed about a comment would be nice!

[martinbonnin]

alreadyHandledTypes is the trick

Ah! I see it now, thanks! I would vote for consistent/simple. We could in the future, introduce another transform that merges fields/inline fragments to reduce the document size (although I believe the "good" solution there is probably PQs)

[BoD]

-> #166