-
Notifications
You must be signed in to change notification settings - Fork 654
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
Disable key fields check if unnecessary and optimize its perf #3720
Changes from 4 commits
c45114b
ee489d4
8eaa0d6
16d4a7b
bf5a76d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,14 +3,28 @@ package com.apollographql.apollo3.ast | |
private class CheckKeyFieldsScope( | ||
val schema: Schema, | ||
val allFragmentDefinitions: Map<String, GQLFragmentDefinition>, | ||
) | ||
) { | ||
private val implementedTypesCache = mutableMapOf<String, Set<String>>() | ||
fun implementedTypes(name: String) = implementedTypesCache.getOrPut(name) { | ||
schema.implementedTypes(name) | ||
} | ||
|
||
private val keyFieldsCache = mutableMapOf<String, Set<String>>() | ||
fun keyFields(name: String) = keyFieldsCache.getOrPut(name) { | ||
schema.keyFields(name) | ||
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. 👍 |
||
} | ||
} | ||
|
||
fun checkKeyFields(operation: GQLOperationDefinition, schema: Schema, allFragmentDefinitions: Map<String, GQLFragmentDefinition>) { | ||
val parentType = operation.rootTypeDefinition(schema)!!.name | ||
CheckKeyFieldsScope(schema, allFragmentDefinitions).checkField("Operation(${operation.name})", operation.selectionSet.selections, parentType) | ||
} | ||
|
||
fun checkKeyFields(fragmentDefinition: GQLFragmentDefinition, schema: Schema, allFragmentDefinitions: Map<String, GQLFragmentDefinition>) { | ||
fun checkKeyFields( | ||
fragmentDefinition: GQLFragmentDefinition, | ||
schema: Schema, | ||
allFragmentDefinitions: Map<String, GQLFragmentDefinition>, | ||
) { | ||
CheckKeyFieldsScope(schema, allFragmentDefinitions).checkField("Fragment(${fragmentDefinition.name})", fragmentDefinition.selectionSet.selections, fragmentDefinition.typeCondition.name) | ||
} | ||
|
||
|
@@ -25,9 +39,9 @@ private fun CheckKeyFieldsScope.checkField( | |
} | ||
|
||
private fun CheckKeyFieldsScope.checkFieldSet(path: String, selections: List<GQLSelection>, parentType: String, possibleType: String) { | ||
val implementedTypes = schema.implementedTypes(possibleType) | ||
val implementedTypes = implementedTypes(possibleType) | ||
|
||
val mergedFields = collectFields(selections, parentType, implementedTypes).groupBy { | ||
val mergedFields = collectFieldsCached(selections, parentType, implementedTypes).groupBy { | ||
it.field.name | ||
}.values | ||
|
||
|
@@ -36,7 +50,7 @@ private fun CheckKeyFieldsScope.checkFieldSet(path: String, selections: List<GQL | |
val fieldNames = mergedFields.map { it.first().field } | ||
.filter { it.alias == null } | ||
.map { it.name }.toSet() | ||
val keyFieldNames = schema.keyFields(possibleType) | ||
val keyFieldNames = keyFields(possibleType) | ||
|
||
val missingFieldNames = keyFieldNames.subtract(fieldNames) | ||
check(missingFieldNames.isEmpty()) { | ||
|
@@ -53,6 +67,15 @@ private fun CheckKeyFieldsScope.checkFieldSet(path: String, selections: List<GQL | |
|
||
private class FieldWithParent(val field: GQLField, val parentType: String) | ||
|
||
private val collectFieldsCache = mutableMapOf<String, List<FieldWithParent>>() | ||
private fun CheckKeyFieldsScope.collectFieldsCached( | ||
selections: List<GQLSelection>, | ||
parentType: String, | ||
implementedTypes: Set<String>, | ||
) = collectFieldsCache.getOrPut("$selections $parentType $implementedTypes") { | ||
collectFields(selections, parentType, implementedTypes) | ||
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. Do you have any idea the cache hit ratio for this? I'd expect it to be relatively low? 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. With the test in the PR I can see that the method was run |
||
} | ||
|
||
private fun CheckKeyFieldsScope.collectFields( | ||
selections: List<GQLSelection>, | ||
parentType: String, | ||
|
@@ -75,15 +98,15 @@ private fun CheckKeyFieldsScope.collectFields( | |
return@flatMap emptyList() | ||
} | ||
|
||
collectFields(it.selectionSet.selections, it.typeCondition.name, implementedTypes) | ||
collectFieldsCached(it.selectionSet.selections, it.typeCondition.name, implementedTypes) | ||
} | ||
is GQLFragmentSpread -> { | ||
if (it.directives.hasCondition()) { | ||
return@flatMap emptyList() | ||
} | ||
|
||
val fragmentDefinition = allFragmentDefinitions[it.name]!! | ||
collectFields(fragmentDefinition.selectionSet.selections, fragmentDefinition.typeCondition.name, implementedTypes) | ||
collectFieldsCached(fragmentDefinition.selectionSet.selections, fragmentDefinition.typeCondition.name, implementedTypes) | ||
} | ||
} | ||
} | ||
|
@@ -94,4 +117,4 @@ private fun List<GQLDirective>?.hasCondition(): Boolean { | |
it.name == "skip" && (it.arguments!!.arguments.first().value as? GQLStringValue)?.value != "false" | ||
|| it.name == "include" && (it.arguments!!.arguments.first().value as? GQLStringValue)?.value != "true" | ||
} ?: false | ||
} | ||
} |
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.
Note for our future selves: we'll want to support
@typePolicy
on interfaces and unions (see #3356). This will need to be updated when this happens