Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 8 additions & 8 deletions lib/src/main/java/graphql/nadel/NextgenEngine.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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

Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

So this uses the newly extracted code from NextgenEngine.isOnlyTopLevelFieldTypename

}
.forEach { (coordinates) ->
builder.dataFetcher(coordinates) { _: DataFetchingEnvironment ->
Expand Down
5 changes: 5 additions & 0 deletions lib/src/main/java/graphql/nadel/engine/util/GraphQLUtil.kt
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ import graphql.schema.GraphQLFieldDefinition
import graphql.schema.GraphQLFieldsContainer
import graphql.schema.GraphQLInputType
import graphql.schema.GraphQLInterfaceType
import graphql.schema.GraphQLNamedType
import graphql.schema.GraphQLObjectType
import graphql.schema.GraphQLSchema
import graphql.schema.GraphQLType
Expand Down Expand Up @@ -686,3 +687,7 @@ internal inline fun <reified T : SDLDefinition<*>> parseDefinition(
): T {
return Parser.parse(sdl).definitions.singleOfType()
}

fun GraphQLSchema.isOperationType(type: GraphQLNamedType): Boolean {
return type === queryType || type === mutationType || type === subscriptionType
}
33 changes: 31 additions & 2 deletions lib/src/main/java/graphql/nadel/util/NamespacedUtil.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@ package graphql.nadel.util
import graphql.language.ObjectTypeDefinition
import graphql.nadel.Service
import graphql.nadel.engine.util.isExtensionDef
import graphql.nadel.engine.util.isOperationType
import graphql.nadel.engine.util.unwrapNonNull
import graphql.nadel.schema.NadelDirectives.namespacedDirectiveDefinition
import graphql.normalized.ExecutableNormalizedField
import graphql.schema.GraphQLFieldDefinition
import graphql.schema.GraphQLObjectType
import graphql.schema.GraphQLSchema
import javax.swing.text.html.HTML.Tag.U

object NamespacedUtil {
fun serviceOwnsNamespacedField(namespacedObjectTypeName: String, service: Service): Boolean {
Expand All @@ -27,7 +31,32 @@ object NamespacedUtil {
}
}

fun isNamespacedField(definition: GraphQLFieldDefinition): Boolean {
return definition.hasAppliedDirective(namespacedDirectiveDefinition.name)
fun isNamespacedField(field: GraphQLFieldDefinition): Boolean {
return field.hasAppliedDirective(namespacedDirectiveDefinition.name)
}

/**
* So we have quite a few fields that do not have `@namespaced` that some parts of our
* code just check if a field looks like it should have `@namespaced` on it.
*
* Instead of fixing it properly, we introduce a common helper util here to identify them.
*/
fun isNamespacedFieldLike(field: GraphQLFieldDefinition): Boolean {
return isNamespacedField(field)
|| (field.arguments.isEmpty() && field.type.unwrapNonNull() is GraphQLObjectType)
}

fun isNamespacedFieldLike(service: Service, rootLevelField: ExecutableNormalizedField): Boolean {
val underlyingSchema = service.underlyingSchema
val parentType = underlyingSchema.getTypeAs<GraphQLObjectType>(rootLevelField.singleObjectTypeName)

if (!underlyingSchema.isOperationType(parentType)) {
return false
}

val field = parentType.getField(rootLevelField.name)
?: return false

return isNamespacedFieldLike(field)
}
}
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(
)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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(
),
)
}
Loading