Skip to content

Commit 039d318

Browse files
gnawfgnawf
authored
Fix short circuit introspection for namespace like fields (#697)
* Fix short circuit introspection for namespace like fields * Fix test and add better namespace like test
1 parent f86b8ce commit 039d318

12 files changed

Lines changed: 538 additions & 11 deletions

‎lib/src/main/java/graphql/nadel/NextgenEngine.kt‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import graphql.nadel.engine.transform.result.NadelResultTransformer
2929
import graphql.nadel.engine.util.MutableJsonMap
3030
import graphql.nadel.engine.util.beginExecute
3131
import graphql.nadel.engine.util.compileToDocument
32+
import graphql.nadel.engine.util.getFieldDefinitionSequence
3233
import graphql.nadel.engine.util.getOperationDefinitionOrNull
3334
import graphql.nadel.engine.util.getOperationKind
3435
import graphql.nadel.engine.util.newExecutionResult
@@ -51,6 +52,8 @@ import graphql.nadel.instrumentation.parameters.child
5152
import graphql.nadel.result.NadelResultMerger
5253
import graphql.nadel.result.NadelResultTracker
5354
import graphql.nadel.time.NadelInternalLatencyTracker
55+
import graphql.nadel.util.NamespacedUtil
56+
import graphql.nadel.util.NamespacedUtil.isNamespacedFieldLike
5457
import graphql.nadel.util.OperationNameUtil
5558
import graphql.nadel.validation.NadelSchemaValidation
5659
import graphql.normalized.ExecutableNormalizedField
@@ -479,16 +482,13 @@ internal class NextgenEngine(
479482
): Boolean {
480483
val topLevelField = topLevelFields.singleOrNull() ?: return false
481484

482-
if (topLevelField.fieldName == TypeNameMetaFieldDef.name) {
485+
if (topLevelField.name == TypeNameMetaFieldDef.name) {
483486
return true
484487
}
485-
val operationType = service.underlyingSchema.getTypeAs<GraphQLObjectType>(topLevelField.singleObjectTypeName)
486-
val topLevelFieldDefinition = operationType.getField(topLevelField.name)
487-
val isNamespacedLike = topLevelFieldDefinition?.arguments?.isEmpty() == true
488-
&& topLevelFieldDefinition.type is GraphQLObjectType
489-
return isNamespacedLike &&
490-
topLevelField.hasChildren() &&
491-
topLevelField.children.all { it.name == TypeNameMetaFieldDef.name }
488+
489+
return isNamespacedFieldLike(service, topLevelField)
490+
&& topLevelField.hasChildren()
491+
&& topLevelField.children.all { it.name == TypeNameMetaFieldDef.name }
492492
}
493493

494494
private fun getDocumentVariablePredicate(hints: NadelExecutionHints, service: Service): VariablePredicate {

‎lib/src/main/java/graphql/nadel/engine/blueprint/IntrospectionService.kt‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import graphql.nadel.engine.util.makeFieldCoordinates
1515
import graphql.nadel.engine.util.toBuilder
1616
import graphql.nadel.engine.util.toBuilderWithoutTypes
1717
import graphql.nadel.util.NamespacedUtil.isNamespacedField
18+
import graphql.nadel.util.NamespacedUtil.isNamespacedFieldLike
1819
import graphql.schema.DataFetchingEnvironment
1920
import graphql.schema.FieldCoordinates
2021
import graphql.schema.GraphQLFieldDefinition
@@ -70,7 +71,7 @@ open class NadelDefaultIntrospectionRunner(schema: GraphQLSchema) : ServiceExecu
7071
// This inserts data fetchers for namespaced fields so we can handle their __typename internally
7172
getFieldsWithCoordinates(schema.queryType, schema.mutationType, schema.subscriptionType)
7273
.filter { (_, field) ->
73-
isNamespacedField(field)
74+
isNamespacedFieldLike(field)
7475
}
7576
.forEach { (coordinates) ->
7677
builder.dataFetcher(coordinates) { _: DataFetchingEnvironment ->

‎lib/src/main/java/graphql/nadel/engine/util/GraphQLUtil.kt‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,7 @@ import graphql.schema.GraphQLFieldDefinition
6161
import graphql.schema.GraphQLFieldsContainer
6262
import graphql.schema.GraphQLInputType
6363
import graphql.schema.GraphQLInterfaceType
64+
import graphql.schema.GraphQLNamedType
6465
import graphql.schema.GraphQLObjectType
6566
import graphql.schema.GraphQLSchema
6667
import graphql.schema.GraphQLType
@@ -686,3 +687,7 @@ internal inline fun <reified T : SDLDefinition<*>> parseDefinition(
686687
): T {
687688
return Parser.parse(sdl).definitions.singleOfType()
688689
}
690+
691+
fun GraphQLSchema.isOperationType(type: GraphQLNamedType): Boolean {
692+
return type === queryType || type === mutationType || type === subscriptionType
693+
}

‎lib/src/main/java/graphql/nadel/util/NamespacedUtil.kt‎

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,14 @@ package graphql.nadel.util
33
import graphql.language.ObjectTypeDefinition
44
import graphql.nadel.Service
55
import graphql.nadel.engine.util.isExtensionDef
6+
import graphql.nadel.engine.util.isOperationType
7+
import graphql.nadel.engine.util.unwrapNonNull
68
import graphql.nadel.schema.NadelDirectives.namespacedDirectiveDefinition
79
import graphql.normalized.ExecutableNormalizedField
810
import graphql.schema.GraphQLFieldDefinition
11+
import graphql.schema.GraphQLObjectType
912
import graphql.schema.GraphQLSchema
13+
import javax.swing.text.html.HTML.Tag.U
1014

1115
object NamespacedUtil {
1216
fun serviceOwnsNamespacedField(namespacedObjectTypeName: String, service: Service): Boolean {
@@ -27,7 +31,32 @@ object NamespacedUtil {
2731
}
2832
}
2933

30-
fun isNamespacedField(definition: GraphQLFieldDefinition): Boolean {
31-
return definition.hasAppliedDirective(namespacedDirectiveDefinition.name)
34+
fun isNamespacedField(field: GraphQLFieldDefinition): Boolean {
35+
return field.hasAppliedDirective(namespacedDirectiveDefinition.name)
36+
}
37+
38+
/**
39+
* So we have quite a few fields that do not have `@namespaced` that some parts of our
40+
* code just check if a field looks like it should have `@namespaced` on it.
41+
*
42+
* Instead of fixing it properly, we introduce a common helper util here to identify them.
43+
*/
44+
fun isNamespacedFieldLike(field: GraphQLFieldDefinition): Boolean {
45+
return isNamespacedField(field)
46+
|| (field.arguments.isEmpty() && field.type.unwrapNonNull() is GraphQLObjectType)
47+
}
48+
49+
fun isNamespacedFieldLike(service: Service, rootLevelField: ExecutableNormalizedField): Boolean {
50+
val underlyingSchema = service.underlyingSchema
51+
val parentType = underlyingSchema.getTypeAs<GraphQLObjectType>(rootLevelField.singleObjectTypeName)
52+
53+
if (!underlyingSchema.isOperationType(parentType)) {
54+
return false
55+
}
56+
57+
val field = parentType.getField(rootLevelField.name)
58+
?: return false
59+
60+
return isNamespacedFieldLike(field)
3261
}
3362
}

‎test/src/test/kotlin/graphql/nadel/tests/next/fixtures/introspection/NamespaceLikeShortCircuitsIntrospectionTest.kt‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
package graphql.nadel.tests.next.fixtures.introspection
2+
3+
import graphql.nadel.NadelExecutionHints
4+
import graphql.nadel.tests.next.NadelIntegrationTest
5+
6+
class NamespaceLikeShortCircuitsIntrospectionTest : NadelIntegrationTest(
7+
query = """
8+
{
9+
test {
10+
__typename
11+
}
12+
}
13+
""".trimIndent(),
14+
services = listOf(
15+
Service(
16+
name = "myService",
17+
overallSchema = """
18+
type Query {
19+
test: TestQuery
20+
}
21+
type TestQuery {
22+
echo: String
23+
}
24+
""".trimIndent(),
25+
runtimeWiring = { wiring ->
26+
wiring
27+
.type("Query") { type ->
28+
type
29+
.dataFetcher("echo") { env ->
30+
"echo"
31+
}
32+
}
33+
},
34+
),
35+
),
36+
) {
37+
override fun makeExecutionHints(): NadelExecutionHints.Builder {
38+
return super.makeExecutionHints()
39+
.shortCircuitEmptyQuery { true }
40+
}
41+
}

‎test/src/test/kotlin/graphql/nadel/tests/next/fixtures/introspection/NamespaceLikeShortCircuitsIntrospectionTestSnapshot.kt‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// @formatter:off
2+
package graphql.nadel.tests.next.fixtures.introspection
3+
4+
import graphql.nadel.tests.next.ExpectedNadelResult
5+
import graphql.nadel.tests.next.ExpectedServiceCall
6+
import graphql.nadel.tests.next.TestSnapshot
7+
import graphql.nadel.tests.next.listOfJsonStrings
8+
import kotlin.Suppress
9+
import kotlin.collections.List
10+
import kotlin.collections.listOf
11+
12+
private suspend fun main() {
13+
graphql.nadel.tests.next.update<NamespaceLikeShortCircuitsIntrospectionTest>()
14+
}
15+
16+
/**
17+
* This class is generated. Do NOT modify.
18+
*
19+
* Refer to [graphql.nadel.tests.next.UpdateTestSnapshots]
20+
*/
21+
@Suppress("unused")
22+
public class NamespaceLikeShortCircuitsIntrospectionTestSnapshot : TestSnapshot() {
23+
override val calls: List<ExpectedServiceCall> = listOf(
24+
)
25+
26+
/**
27+
* ```json
28+
* {
29+
* "data": {
30+
* "test": {
31+
* "__typename": "TestQuery"
32+
* }
33+
* }
34+
* }
35+
* ```
36+
*/
37+
override val result: ExpectedNadelResult = ExpectedNadelResult(
38+
result = """
39+
| {
40+
| "data": {
41+
| "test": {
42+
| "__typename": "TestQuery"
43+
| }
44+
| }
45+
| }
46+
""".trimMargin(),
47+
delayedResults = listOfJsonStrings(
48+
),
49+
)
50+
}

‎test/src/test/kotlin/graphql/nadel/tests/next/fixtures/introspection/NamespaceMutationLikeShortCircuitsIntrospectionTest.kt‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
package graphql.nadel.tests.next.fixtures.introspection
2+
3+
import graphql.nadel.NadelExecutionHints
4+
import graphql.nadel.tests.next.NadelIntegrationTest
5+
6+
class NamespaceMutationLikeShortCircuitsIntrospectionTest : NadelIntegrationTest(
7+
query = """
8+
mutation {
9+
test {
10+
__typename
11+
}
12+
}
13+
""".trimIndent(),
14+
services = listOf(
15+
Service(
16+
name = "myService",
17+
overallSchema = """
18+
type Query {
19+
test: TestQuery
20+
}
21+
type TestQuery {
22+
echo: String
23+
}
24+
type Mutation {
25+
test: TestMutation
26+
}
27+
type TestMutation {
28+
echo: String
29+
}
30+
""".trimIndent(),
31+
runtimeWiring = { wiring ->
32+
wiring
33+
.type("Query") { type ->
34+
type
35+
.dataFetcher("echo") { env ->
36+
"echo"
37+
}
38+
}
39+
},
40+
),
41+
),
42+
) {
43+
override fun makeExecutionHints(): NadelExecutionHints.Builder {
44+
return super.makeExecutionHints()
45+
.shortCircuitEmptyQuery { true }
46+
}
47+
}

‎test/src/test/kotlin/graphql/nadel/tests/next/fixtures/introspection/NamespaceMutationLikeShortCircuitsIntrospectionTestSnapshot.kt‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
// @formatter:off
2+
package graphql.nadel.tests.next.fixtures.introspection
3+
4+
import graphql.nadel.tests.next.ExpectedNadelResult
5+
import graphql.nadel.tests.next.ExpectedServiceCall
6+
import graphql.nadel.tests.next.TestSnapshot
7+
import graphql.nadel.tests.next.listOfJsonStrings
8+
import kotlin.Suppress
9+
import kotlin.collections.List
10+
import kotlin.collections.listOf
11+
12+
private suspend fun main() {
13+
graphql.nadel.tests.next.update<NamespaceMutationLikeShortCircuitsIntrospectionTest>()
14+
}
15+
16+
/**
17+
* This class is generated. Do NOT modify.
18+
*
19+
* Refer to [graphql.nadel.tests.next.UpdateTestSnapshots]
20+
*/
21+
@Suppress("unused")
22+
public class NamespaceMutationLikeShortCircuitsIntrospectionTestSnapshot : TestSnapshot() {
23+
override val calls: List<ExpectedServiceCall> = listOf(
24+
)
25+
26+
/**
27+
* ```json
28+
* {
29+
* "data": {
30+
* "test": {
31+
* "__typename": "TestMutation"
32+
* }
33+
* }
34+
* }
35+
* ```
36+
*/
37+
override val result: ExpectedNadelResult = ExpectedNadelResult(
38+
result = """
39+
| {
40+
| "data": {
41+
| "test": {
42+
| "__typename": "TestMutation"
43+
| }
44+
| }
45+
| }
46+
""".trimMargin(),
47+
delayedResults = listOfJsonStrings(
48+
),
49+
)
50+
}

0 commit comments

Comments
 (0)