-
Notifications
You must be signed in to change notification settings - Fork 28
Fix short circuit introspection for namespace like fields #697
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
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 |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ import graphql.nadel.engine.transform.result.NadelResultTransformer | |
| import graphql.nadel.engine.util.MutableJsonMap | ||
| import graphql.nadel.engine.util.beginExecute | ||
| import graphql.nadel.engine.util.compileToDocument | ||
| import graphql.nadel.engine.util.getFieldDefinitionSequence | ||
| import graphql.nadel.engine.util.getOperationDefinitionOrNull | ||
| import graphql.nadel.engine.util.getOperationKind | ||
| import graphql.nadel.engine.util.newExecutionResult | ||
|
|
@@ -51,6 +52,8 @@ import graphql.nadel.instrumentation.parameters.child | |
| import graphql.nadel.result.NadelResultMerger | ||
| import graphql.nadel.result.NadelResultTracker | ||
| import graphql.nadel.time.NadelInternalLatencyTracker | ||
| import graphql.nadel.util.NamespacedUtil | ||
| import graphql.nadel.util.NamespacedUtil.isNamespacedFieldLike | ||
| import graphql.nadel.util.OperationNameUtil | ||
| import graphql.nadel.validation.NadelSchemaValidation | ||
| import graphql.normalized.ExecutableNormalizedField | ||
|
|
@@ -479,16 +482,13 @@ internal class NextgenEngine( | |
| ): Boolean { | ||
| val topLevelField = topLevelFields.singleOrNull() ?: return false | ||
|
|
||
| if (topLevelField.fieldName == TypeNameMetaFieldDef.name) { | ||
| if (topLevelField.name == TypeNameMetaFieldDef.name) { | ||
| return true | ||
| } | ||
| val operationType = service.underlyingSchema.getTypeAs<GraphQLObjectType>(topLevelField.singleObjectTypeName) | ||
| val topLevelFieldDefinition = operationType.getField(topLevelField.name) | ||
| val isNamespacedLike = topLevelFieldDefinition?.arguments?.isEmpty() == true | ||
|
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. So the bug here is that the logic we use to identify "namespaced fields" is different here and in the internal introspection service. This leads to a bug because this code thinks more fields can be handled by the internal introspection service, but in reality it can't so it fails. |
||
| && topLevelFieldDefinition.type is GraphQLObjectType | ||
| return isNamespacedLike && | ||
| topLevelField.hasChildren() && | ||
| topLevelField.children.all { it.name == TypeNameMetaFieldDef.name } | ||
|
|
||
| return isNamespacedFieldLike(service, topLevelField) | ||
| && topLevelField.hasChildren() | ||
| && topLevelField.children.all { it.name == TypeNameMetaFieldDef.name } | ||
| } | ||
|
|
||
| private fun getDocumentVariablePredicate(hints: NadelExecutionHints, service: Service): VariablePredicate { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import graphql.nadel.engine.util.makeFieldCoordinates | |
| import graphql.nadel.engine.util.toBuilder | ||
| import graphql.nadel.engine.util.toBuilderWithoutTypes | ||
| import graphql.nadel.util.NamespacedUtil.isNamespacedField | ||
| import graphql.nadel.util.NamespacedUtil.isNamespacedFieldLike | ||
| import graphql.schema.DataFetchingEnvironment | ||
| import graphql.schema.FieldCoordinates | ||
| import graphql.schema.GraphQLFieldDefinition | ||
|
|
@@ -70,7 +71,7 @@ open class NadelDefaultIntrospectionRunner(schema: GraphQLSchema) : ServiceExecu | |
| // This inserts data fetchers for namespaced fields so we can handle their __typename internally | ||
| getFieldsWithCoordinates(schema.queryType, schema.mutationType, schema.subscriptionType) | ||
| .filter { (_, field) -> | ||
| isNamespacedField(field) | ||
| isNamespacedFieldLike(field) | ||
|
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. So this uses the newly extracted code from |
||
| } | ||
| .forEach { (coordinates) -> | ||
| builder.dataFetcher(coordinates) { _: DataFetchingEnvironment -> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| package graphql.nadel.tests.next.fixtures.introspection | ||
|
|
||
| import graphql.nadel.NadelExecutionHints | ||
| import graphql.nadel.tests.next.NadelIntegrationTest | ||
|
|
||
| class NamespaceLikeShortCircuitsIntrospectionTest : NadelIntegrationTest( | ||
| query = """ | ||
| { | ||
| test { | ||
| __typename | ||
| } | ||
| } | ||
| """.trimIndent(), | ||
| services = listOf( | ||
| Service( | ||
| name = "myService", | ||
| overallSchema = """ | ||
| type Query { | ||
| test: TestQuery | ||
| } | ||
| type TestQuery { | ||
| echo: String | ||
| } | ||
| """.trimIndent(), | ||
| runtimeWiring = { wiring -> | ||
| wiring | ||
| .type("Query") { type -> | ||
| type | ||
| .dataFetcher("echo") { env -> | ||
| "echo" | ||
| } | ||
| } | ||
| }, | ||
| ), | ||
| ), | ||
| ) { | ||
| override fun makeExecutionHints(): NadelExecutionHints.Builder { | ||
| return super.makeExecutionHints() | ||
| .shortCircuitEmptyQuery { true } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // @formatter:off | ||
| package graphql.nadel.tests.next.fixtures.introspection | ||
|
|
||
| import graphql.nadel.tests.next.ExpectedNadelResult | ||
| import graphql.nadel.tests.next.ExpectedServiceCall | ||
| import graphql.nadel.tests.next.TestSnapshot | ||
| import graphql.nadel.tests.next.listOfJsonStrings | ||
| import kotlin.Suppress | ||
| import kotlin.collections.List | ||
| import kotlin.collections.listOf | ||
|
|
||
| private suspend fun main() { | ||
| graphql.nadel.tests.next.update<NamespaceLikeShortCircuitsIntrospectionTest>() | ||
| } | ||
|
|
||
| /** | ||
| * This class is generated. Do NOT modify. | ||
| * | ||
| * Refer to [graphql.nadel.tests.next.UpdateTestSnapshots] | ||
| */ | ||
| @Suppress("unused") | ||
| public class NamespaceLikeShortCircuitsIntrospectionTestSnapshot : TestSnapshot() { | ||
| override val calls: List<ExpectedServiceCall> = listOf( | ||
| ) | ||
|
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. So here the calls are empty because the tests are executed by our internal introspection service. |
||
|
|
||
| /** | ||
| * ```json | ||
| * { | ||
| * "data": { | ||
| * "test": { | ||
| * "__typename": "TestQuery" | ||
| * } | ||
| * } | ||
| * } | ||
| * ``` | ||
| */ | ||
| override val result: ExpectedNadelResult = ExpectedNadelResult( | ||
| result = """ | ||
| | { | ||
| | "data": { | ||
| | "test": { | ||
| | "__typename": "TestQuery" | ||
|
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. Prior to this PR's fix, it would error out like so {
"errors": [
{
"message": "Exception while fetching data (/test) : Internal error: should never happen: This data fetcher should NEVER be called from Nadel",
"locations": [
{
"column": 2,
"line": 1
}
],
"path": [
"test"
],
"extensions": {
"classification": "DataFetchingException"
}
}
],
"data": {
"test": null
}
} |
||
| | } | ||
| | } | ||
| | } | ||
| """.trimMargin(), | ||
| delayedResults = listOfJsonStrings( | ||
| ), | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,47 @@ | ||
| package graphql.nadel.tests.next.fixtures.introspection | ||
|
|
||
| import graphql.nadel.NadelExecutionHints | ||
| import graphql.nadel.tests.next.NadelIntegrationTest | ||
|
|
||
| class NamespaceMutationLikeShortCircuitsIntrospectionTest : NadelIntegrationTest( | ||
| query = """ | ||
| mutation { | ||
| test { | ||
| __typename | ||
| } | ||
| } | ||
| """.trimIndent(), | ||
| services = listOf( | ||
| Service( | ||
| name = "myService", | ||
| overallSchema = """ | ||
| type Query { | ||
| test: TestQuery | ||
| } | ||
| type TestQuery { | ||
| echo: String | ||
| } | ||
| type Mutation { | ||
| test: TestMutation | ||
| } | ||
| type TestMutation { | ||
| echo: String | ||
| } | ||
| """.trimIndent(), | ||
| runtimeWiring = { wiring -> | ||
| wiring | ||
| .type("Query") { type -> | ||
| type | ||
| .dataFetcher("echo") { env -> | ||
| "echo" | ||
| } | ||
| } | ||
| }, | ||
| ), | ||
| ), | ||
| ) { | ||
| override fun makeExecutionHints(): NadelExecutionHints.Builder { | ||
| return super.makeExecutionHints() | ||
| .shortCircuitEmptyQuery { true } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| // @formatter:off | ||
| package graphql.nadel.tests.next.fixtures.introspection | ||
|
|
||
| import graphql.nadel.tests.next.ExpectedNadelResult | ||
| import graphql.nadel.tests.next.ExpectedServiceCall | ||
| import graphql.nadel.tests.next.TestSnapshot | ||
| import graphql.nadel.tests.next.listOfJsonStrings | ||
| import kotlin.Suppress | ||
| import kotlin.collections.List | ||
| import kotlin.collections.listOf | ||
|
|
||
| private suspend fun main() { | ||
| graphql.nadel.tests.next.update<NamespaceMutationLikeShortCircuitsIntrospectionTest>() | ||
| } | ||
|
|
||
| /** | ||
| * This class is generated. Do NOT modify. | ||
| * | ||
| * Refer to [graphql.nadel.tests.next.UpdateTestSnapshots] | ||
| */ | ||
| @Suppress("unused") | ||
| public class NamespaceMutationLikeShortCircuitsIntrospectionTestSnapshot : TestSnapshot() { | ||
| override val calls: List<ExpectedServiceCall> = listOf( | ||
| ) | ||
|
|
||
| /** | ||
| * ```json | ||
| * { | ||
| * "data": { | ||
| * "test": { | ||
| * "__typename": "TestMutation" | ||
| * } | ||
| * } | ||
| * } | ||
| * ``` | ||
| */ | ||
| override val result: ExpectedNadelResult = ExpectedNadelResult( | ||
| result = """ | ||
| | { | ||
| | "data": { | ||
| | "test": { | ||
| | "__typename": "TestMutation" | ||
| | } | ||
| | } | ||
| | } | ||
| """.trimMargin(), | ||
| delayedResults = listOfJsonStrings( | ||
| ), | ||
| ) | ||
| } |
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.
Extracted this code into common
NamespacedUtil.isNamespaceFieldLike