Skip to content

Handle @typePolicy in unions interfaces #156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
May 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle/libs.versions.toml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[versions]
kotlin-plugin = "2.1.0"
kotlin-plugin = "2.1.21"
android-plugin = "8.7.0"
apollo = "4.2.0"
okio = "3.9.0"
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
public final class com/apollographql/cache/apollocompilerplugin/ApolloCacheCompilerPlugin : com/apollographql/apollo/compiler/ApolloCompilerPlugin {
public fun <init> (Ljava/lang/String;Lcom/apollographql/apollo/compiler/ApolloCompilerPluginLogger;)V
public fun foreignSchemas ()Ljava/util/List;
public fun schemaListener ()Lcom/apollographql/apollo/compiler/SchemaListener;
}

public final class com/apollographql/cache/apollocompilerplugin/ApolloCacheCompilerPluginProvider : com/apollographql/apollo/compiler/ApolloCompilerPluginProvider {
public fun <init> ()V
public fun create (Lcom/apollographql/apollo/compiler/ApolloCompilerPluginEnvironment;)Lcom/apollographql/apollo/compiler/ApolloCompilerPlugin;
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
@file:OptIn(ApolloExperimental::class)

package com.apollographql.cache.apollocompilerplugin

import com.apollographql.apollo.annotations.ApolloExperimental
import com.apollographql.apollo.compiler.ApolloCompilerPlugin
import com.apollographql.apollo.compiler.ApolloCompilerPluginEnvironment
import com.apollographql.apollo.compiler.ApolloCompilerPluginProvider
import com.apollographql.cache.apollocompilerplugin.internal.ApolloCacheCompilerPlugin

class ApolloCacheCompilerPluginProvider : ApolloCompilerPluginProvider {
override fun create(environment: ApolloCompilerPluginEnvironment): ApolloCompilerPlugin {
return ApolloCacheCompilerPlugin(environment)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
@file:OptIn(ApolloExperimental::class)

package com.apollographql.cache.apollocompilerplugin.internal

import com.apollographql.apollo.annotations.ApolloExperimental
import com.apollographql.apollo.annotations.ApolloInternal
import com.apollographql.apollo.ast.GQLField
import com.apollographql.apollo.ast.GQLFragmentDefinition
import com.apollographql.apollo.ast.GQLFragmentSpread
import com.apollographql.apollo.ast.GQLInlineFragment
import com.apollographql.apollo.ast.GQLInterfaceTypeDefinition
import com.apollographql.apollo.ast.GQLNamedType
import com.apollographql.apollo.ast.GQLOperationDefinition
import com.apollographql.apollo.ast.GQLSelection
import com.apollographql.apollo.ast.GQLUnionTypeDefinition
import com.apollographql.apollo.ast.Schema
import com.apollographql.apollo.ast.SourceAwareException
import com.apollographql.apollo.ast.definitionFromScope
import com.apollographql.apollo.ast.rawType
import com.apollographql.apollo.ast.responseName
import com.apollographql.apollo.ast.rootTypeDefinition
import com.apollographql.apollo.compiler.DocumentTransform

/**
* Add key fields and `__typename` to selections on types that declare them via `@typePolicy`.
*/
internal class AddKeyFieldsDocumentTransform : DocumentTransform {
override fun transform(schema: Schema, fragment: GQLFragmentDefinition): GQLFragmentDefinition {
return fragment.withRequiredFields(schema)
}

override fun transform(schema: Schema, operation: GQLOperationDefinition): GQLOperationDefinition {
return operation.withRequiredFields(schema)
}

private fun GQLOperationDefinition.withRequiredFields(schema: Schema): GQLOperationDefinition {
val parentType = rootTypeDefinition(schema)!!.name
return copy(
selections = selections.withRequiredFields(
schema = schema,
parentType = parentType,
parentFields = emptySet(),
isRoot = false,
)
)
}

private fun GQLFragmentDefinition.withRequiredFields(schema: Schema): GQLFragmentDefinition {
return copy(
selections = selections.withRequiredFields(
schema = schema,
parentType = typeCondition.name,
parentFields = emptySet(),
isRoot = true,
),
)
}

/**
* @param isRoot: whether this selection set is considered a valid root for adding __typename
* This is the case for field selection sets but also fragments since fragments can be executed from the cache
*/
@OptIn(ApolloInternal::class)
private fun List<GQLSelection>.withRequiredFields(
schema: Schema,
parentType: String,
parentFields: Set<String>,
isRoot: Boolean,
): List<GQLSelection> {
if (isEmpty()) {
return this
}
val keyFields = schema.keyFields(parentType)

this.filterIsInstance<GQLField>().forEach {
// Disallow fields whose alias conflicts with a key field, or is "__typename"
if (keyFields.contains(it.alias) || it.alias == "__typename") {
throw SourceAwareException(
error = "Apollo: Field '${it.alias}: ${it.name}' in $parentType conflicts with key fields",
sourceLocation = it.sourceLocation
)
}
}

val fieldNames = parentFields + this.filterIsInstance<GQLField>().map { it.responseName() }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👍


val alreadyHandledTypes = mutableSetOf<String>()
var newSelections = this.map {
when (it) {
is GQLInlineFragment -> {
alreadyHandledTypes += it.typeCondition?.name ?: parentType
it.copy(
selections = it.selections.withRequiredFields(
schema = schema,
parentType = it.typeCondition?.name ?: parentType,
parentFields = fieldNames + keyFields,
isRoot = false
)
)
}

is GQLFragmentSpread -> it
is GQLField -> it.withRequiredFields(
schema = schema,
parentType = parentType
)
}
}

// Unions and interfaces without key fields: add key fields of all possible types in inline fragments
val inlineFragmentsToAdd = if (isRoot && keyFields.isEmpty()) {
val parentTypeDefinition = schema.typeDefinition(parentType)
val possibleTypes = if (parentTypeDefinition is GQLInterfaceTypeDefinition || parentTypeDefinition is GQLUnionTypeDefinition) {
schema.possibleTypes(parentTypeDefinition)
} else {
emptySet()
} - alreadyHandledTypes
possibleTypes.associateWith { schema.keyFields(it) }.mapNotNull { (possibleType, keyFields) ->
val fieldNamesToAddInInlineFragment = keyFields - fieldNames
if (fieldNamesToAddInInlineFragment.isNotEmpty()) {
GQLInlineFragment(
typeCondition = GQLNamedType(null, possibleType),
selections = fieldNamesToAddInInlineFragment.map { buildField(it) },
directives = emptyList(),
sourceLocation = null,
)
} else {
null
}
}
} else {
emptySet()
}

val fieldNamesToAdd = (keyFields - fieldNames)
newSelections = newSelections + fieldNamesToAdd.map { buildField(it) } + inlineFragmentsToAdd
newSelections = if (isRoot) {
// Remove the __typename if it exists and add it again at the top, so we're guaranteed to have it at the beginning of json parsing.
// Also remove any @include/@skip directive on __typename.
listOf(buildField("__typename")) + newSelections.filter { (it as? GQLField)?.name != "__typename" }
} else {
newSelections
}
Comment on lines +137 to +143
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()) {

Copy link
Contributor

@martinbonnin martinbonnin May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

@BoD BoD May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> #166


return newSelections
}

private fun GQLField.withRequiredFields(
schema: Schema,
parentType: String,
): GQLField {
val typeDefinition = definitionFromScope(schema, parentType)!!
val newSelectionSet = selections.withRequiredFields(
schema = schema,
parentType = typeDefinition.type.rawType().name,
parentFields = emptySet(),
isRoot = true
)
return copy(selections = newSelectionSet)
}

@OptIn(ApolloExperimental::class)
private fun buildField(name: String): GQLField {
return GQLField(
name = name,
arguments = emptyList(),
selections = emptyList(),
sourceLocation = null,
directives = emptyList(),
alias = null,
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
@file:OptIn(ApolloExperimental::class)

package com.apollographql.cache.apollocompilerplugin.internal

import com.apollographql.apollo.annotations.ApolloExperimental
import com.apollographql.apollo.ast.ForeignSchema
import com.apollographql.apollo.compiler.ApolloCompilerPlugin
import com.apollographql.apollo.compiler.ApolloCompilerPluginEnvironment
import com.apollographql.apollo.compiler.DocumentTransform
import com.apollographql.apollo.compiler.SchemaListener

internal class ApolloCacheCompilerPlugin(
private val environment: ApolloCompilerPluginEnvironment,
) : ApolloCompilerPlugin {
override fun foreignSchemas(): List<ForeignSchema> {
return listOf(ForeignSchema("cache", "v0.1", cacheControlGQLDefinitions))
}

override fun schemaListener(): SchemaListener {
return CacheSchemaListener(environment)
}

override fun documentTransform(): DocumentTransform {
return AddKeyFieldsDocumentTransform()
}
Comment on lines +15 to +25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That unfolds nicely 👍

}
Loading