-
Notifications
You must be signed in to change notification settings - Fork 4
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
Changes from all commits
593d617
f56ef38
306d79a
6d68322
952e516
0a4c995
7dc10f5
7ba5909
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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() } | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should required fields be added in the
I think that coveres it all? So probably we don't even need to track There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👀 Yeah I think that's right. I'll open a fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 query GetUser {
user {
__typename
... on Person {
name
}
... on Robot {
serialNumber
}
... on Person {
id
}
}
} if we also do it in 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) {
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah that may be a bit more obvious for people looking at the resulting document 👍 . And agreed about a comment would be nice! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That unfolds nicely 👍 |
||
} |
There was a problem hiding this comment.
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?Because
responseName()
isid
, we'll skip adding theid
keyfield below? Which doesn't look like what we want?There was a problem hiding this comment.
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:
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! 👍