Skip to content

Commit

Permalink
don't call underlying services on empty queries (#519)
Browse files Browse the repository at this point in the history
* don't call underlying services on empty queries

* rename a file

* pr feedback
  • Loading branch information
temaEmelyan authored Jun 13, 2024
1 parent a81f603 commit f95b257
Show file tree
Hide file tree
Showing 14 changed files with 597 additions and 119 deletions.
10 changes: 10 additions & 0 deletions lib/src/main/java/graphql/nadel/NadelExecutionHints.kt
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import graphql.nadel.hints.AllDocumentVariablesHint
import graphql.nadel.hints.LegacyOperationNamesHint
import graphql.nadel.hints.NadelDeferSupportHint
import graphql.nadel.hints.NadelSharedTypeRenamesHint
import graphql.nadel.hints.NadelShortCircuitEmptyQueryHint
import graphql.nadel.hints.NewBatchHydrationGroupingHint
import graphql.nadel.hints.NewResultMergerAndNamespacedTypename

Expand All @@ -14,6 +15,7 @@ data class NadelExecutionHints(
val newBatchHydrationGrouping: NewBatchHydrationGroupingHint,
val deferSupport: NadelDeferSupportHint,
val sharedTypeRenames: NadelSharedTypeRenamesHint,
val shortCircuitEmptyQuery: NadelShortCircuitEmptyQueryHint,
) {
/**
* Returns a builder with the same field values as this object.
Expand All @@ -31,6 +33,7 @@ data class NadelExecutionHints(
private var newResultMergerAndNamespacedTypename = NewResultMergerAndNamespacedTypename { false }
private var newBatchHydrationGrouping = NewBatchHydrationGroupingHint { false }
private var deferSupport = NadelDeferSupportHint { false }
private var shortCircuitEmptyQuery = NadelShortCircuitEmptyQueryHint { false }
private var sharedTypeRenames = NadelSharedTypeRenamesHint { false }

constructor()
Expand All @@ -39,6 +42,7 @@ data class NadelExecutionHints(
legacyOperationNames = nadelExecutionHints.legacyOperationNames
allDocumentVariablesHint = nadelExecutionHints.allDocumentVariablesHint
newResultMergerAndNamespacedTypename = nadelExecutionHints.newResultMergerAndNamespacedTypename
shortCircuitEmptyQuery = nadelExecutionHints.shortCircuitEmptyQuery
}

fun legacyOperationNames(flag: LegacyOperationNamesHint): Builder {
Expand Down Expand Up @@ -66,6 +70,11 @@ data class NadelExecutionHints(
return this
}

fun shortCircuitEmptyQuery(flag: NadelShortCircuitEmptyQueryHint): Builder {
shortCircuitEmptyQuery = flag
return this
}

fun sharedTypeRenames(flag: NadelSharedTypeRenamesHint): Builder {
sharedTypeRenames = flag
return this
Expand All @@ -79,6 +88,7 @@ data class NadelExecutionHints(
newBatchHydrationGrouping,
deferSupport,
sharedTypeRenames,
shortCircuitEmptyQuery,
)
}
}
Expand Down
36 changes: 32 additions & 4 deletions lib/src/main/java/graphql/nadel/NextgenEngine.kt
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import graphql.GraphQLError
import graphql.execution.ExecutionIdProvider
import graphql.execution.instrumentation.InstrumentationState
import graphql.incremental.IncrementalExecutionResultImpl
import graphql.introspection.Introspection.TypeNameMetaFieldDef
import graphql.language.Document
import graphql.nadel.engine.NadelExecutionContext
import graphql.nadel.engine.NadelIncrementalResultSupport
import graphql.nadel.engine.blueprint.IntrospectionService
import graphql.nadel.engine.blueprint.NadelDefaultIntrospectionRunner
import graphql.nadel.engine.blueprint.NadelExecutionBlueprintFactory
import graphql.nadel.engine.blueprint.NadelIntrospectionRunnerFactory
Expand Down Expand Up @@ -41,10 +43,12 @@ import graphql.nadel.instrumentation.parameters.NadelInstrumentationOnErrorParam
import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.ChildStep.Companion.DocumentCompilation
import graphql.nadel.instrumentation.parameters.NadelInstrumentationTimingParameters.RootStep
import graphql.nadel.instrumentation.parameters.child
import graphql.nadel.schema.NadelDirectives.namespacedDirectiveDefinition
import graphql.nadel.util.OperationNameUtil
import graphql.normalized.ExecutableNormalizedField
import graphql.normalized.ExecutableNormalizedOperationFactory.createExecutableNormalizedOperationWithRawVariables
import graphql.normalized.VariablePredicate
import graphql.schema.GraphQLObjectType
import graphql.schema.GraphQLSchema
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
Expand Down Expand Up @@ -76,6 +80,7 @@ internal class NextgenEngine(
) {
private val coroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Default)
private val services: Map<String, Service> = services.strictAssociateBy { it.name }
private val engineSchemaIntrospectionService = IntrospectionService(engineSchema, introspectionRunnerFactory)
private val overallExecutionBlueprint = NadelExecutionBlueprintFactory.create(
engineSchema = engineSchema,
services = services,
Expand Down Expand Up @@ -302,7 +307,7 @@ internal class NextgenEngine(
operationName = getOperationName(service, executionContext),
topLevelFields = listOf(transformedQuery),
variablePredicate = jsonPredicate,
deferSupport = executionContext.hints.deferSupport.invoke()
deferSupport = executionContext.hints.deferSupport(),
)
}

Expand All @@ -317,10 +322,9 @@ internal class NextgenEngine(
hydrationDetails = executionHydrationDetails,
executableNormalizedField = transformedQuery,
)

val serviceExecution = chooseServiceExecution(service, transformedQuery, executionContext.hints)
val serviceExecResult = try {
service.serviceExecution
.execute(serviceExecParams)
serviceExecution.execute(serviceExecParams)
.asDeferred()
.await()
} catch (e: Exception) {
Expand Down Expand Up @@ -362,6 +366,30 @@ internal class NextgenEngine(
)
}

private fun chooseServiceExecution(
service: Service,
transformedQuery: ExecutableNormalizedField,
hints: NadelExecutionHints,
): ServiceExecution {
return when {
hints.shortCircuitEmptyQuery(service) && onlyTopLevelTypenameField(transformedQuery) ->
engineSchemaIntrospectionService.serviceExecution
else -> service.serviceExecution
}
}

private fun onlyTopLevelTypenameField(executableNormalizedField: ExecutableNormalizedField): Boolean {
if (executableNormalizedField.fieldName == TypeNameMetaFieldDef.name) {
return true
}
val operationType = engineSchema.getTypeAs<GraphQLObjectType>(executableNormalizedField.singleObjectTypeName)
val topLevelFieldDefinition = operationType.getField(executableNormalizedField.name)
return if (topLevelFieldDefinition.hasAppliedDirective(namespacedDirectiveDefinition.name)) {
executableNormalizedField.hasChildren()
&& executableNormalizedField.children.all { it.name == TypeNameMetaFieldDef.name }
} else false
}

private fun getDocumentVariablePredicate(hints: NadelExecutionHints, service: Service): VariablePredicate {
return if (hints.allDocumentVariablesHint.invoke(service)) {
DocumentPredicates.allVariablesPredicate
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import graphql.normalized.ExecutableNormalizedOperation
import graphql.schema.GraphQLSchema

internal class NadelFieldToService(
private val querySchema: GraphQLSchema,
querySchema: GraphQLSchema,
private val overallExecutionBlueprint: NadelOverallExecutionBlueprint,
introspectionRunnerFactory: NadelIntrospectionRunnerFactory,
private val dynamicServiceResolution: DynamicServiceResolution,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package graphql.nadel.hints

import graphql.nadel.Service

fun interface NadelShortCircuitEmptyQueryHint {
/**
* Determines whether empty queries containing only top level __typename fields should be short-circuited without
* calling the underlying service and executed on the internal introspection service
*
* @param service the service we are sending the query to
* @return true to execute the query on the internal introspection service
*/
operator fun invoke(service: Service): Boolean
}
3 changes: 1 addition & 2 deletions test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@ import java.util.concurrent.CompletableFuture
* 3. Copy paste output from selecting a test in the IntelliJ e.g. java:test://graphql.nadel.tests.EngineTests.current hydration inside a renamed field
*/
private val singleTestToRun = (System.getenv("TEST_NAME") ?: "")
.removePrefix("java:test://graphql.nadel.tests.EngineTests.current")
.removePrefix("java:test://graphql.nadel.tests.EngineTests.nextgen")
.removePrefix("java:test://graphql.nadel.tests.EngineTests/")
.removeSuffix(".yml")
.removeSuffix(".yaml")
.trim()
Expand Down
47 changes: 42 additions & 5 deletions test/src/test/kotlin/graphql/nadel/tests/hooks/remove-fields.kt
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ package graphql.nadel.tests.hooks
import graphql.ErrorClassification
import graphql.GraphQLError
import graphql.nadel.Nadel
import graphql.nadel.NadelExecutionHints
import graphql.nadel.hooks.NadelExecutionHooks
import graphql.nadel.tests.EngineTestHook
import graphql.nadel.tests.UseHook
import graphql.nadel.tests.transforms.RemoveFieldTestTransform
import graphql.GraphqlErrorException as GraphQLErrorException

private class RejectField(private val fieldNames: List<String>) : NadelExecutionHooks {
Expand Down Expand Up @@ -92,12 +95,46 @@ class `one-of-top-level-fields-is-removed` : EngineTestHook {
}
}

// @UseHook
@UseHook
class `top-level-field-is-removed` : EngineTestHook {
override fun makeNadel(builder: Nadel.Builder): Nadel.Builder {
return builder
.executionHooks(RejectField("commentById"))
}
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { true }
}

@UseHook
class `top-level-field-is-removed-hint-is-off` : EngineTestHook {
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { false }
}

@UseHook
class `hydration-top-level-field-is-removed` : EngineTestHook {
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { true }
}

@UseHook
class `namespaced-hydration-top-level-field-is-removed` : EngineTestHook {
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { true }
}

@UseHook
class `hidden-namespaced-hydration-top-level-field-is-removed` : EngineTestHook {
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { true }
}

@UseHook
class `namespaced-field-is-removed` : EngineTestHook {
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { true }
}

@UseHook
class `namespaced-field-is-removed-with-renames` : EngineTestHook {
override val customTransforms = listOf(RemoveFieldTestTransform())
override fun makeExecutionHints(builder: NadelExecutionHints.Builder) = builder.shortCircuitEmptyQuery { true }
}

// @UseHook
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,10 @@ import graphql.nadel.engine.transform.query.NadelQueryPath
import graphql.nadel.engine.transform.query.NadelQueryTransformer
import graphql.nadel.engine.transform.result.NadelResultInstruction
import graphql.nadel.engine.transform.result.NadelResultKey
import graphql.nadel.engine.transform.result.json.JsonNodeExtractor
import graphql.nadel.engine.transform.result.json.JsonNodes
import graphql.nadel.engine.util.queryPath
import graphql.normalized.ExecutableNormalizedField
import graphql.schema.GraphQLObjectType
import graphql.validation.ValidationError
import graphql.validation.ValidationError.newValidationError
import graphql.validation.ValidationErrorType

Expand Down Expand Up @@ -78,12 +76,10 @@ class RemoveFieldTestTransform : NadelTransform<GraphQLError> {
state: GraphQLError,
nodes: JsonNodes,
): List<NadelResultInstruction> {
val parentNodes = JsonNodeExtractor.getNodesAt(
data = result.data,
val parentNodes = nodes.getNodesAt(
queryPath = underlyingParentField?.queryPath ?: NadelQueryPath.root,
flatten = true,
)

return parentNodes.map { parentNode ->
NadelResultInstruction.Set(
subject = parentNode,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
name: "hidden namespaced hydration top level field is removed"
enabled: true
# language=GraphQL
overallSchema:
IssueService: |
directive @namespaced on FIELD_DEFINITION
type Query {
issueById(id: ID): Issue @namespaced
}
type Issue {
id: ID
comment: Comment @hydrated(
service: "CommentService"
field: "commentApi.commentById"
arguments: [
{name: "id", value: "$source.commentId"}
]
)
}
CommentService: |
directive @toBeDeleted on FIELD_DEFINITION
type Query {
commentApi: CommentApi @namespaced @hidden
echo: String
}
type CommentApi {
commentById(id: ID): Comment @toBeDeleted @hidden
echo: String
}
type Comment {
id: ID
}
# language=GraphQL
underlyingSchema:
IssueService: |
type Query {
issueById(id: ID): Issue
}
type Issue {
id: ID
commentId: ID
}
CommentService: |
type Query {
commentApi: CommentApi
echo: String
}
type CommentApi {
commentById(id: ID): Comment
echo: String
}
type Comment {
id: ID
}
# language=GraphQL
query: |
query {
issueById(id: "C1") {
id
comment {
id
}
}
}
variables: { }
serviceCalls:
- serviceName: "IssueService"
request:
# language=GraphQL
query: |
{
issueById(id: "C1") {
__typename__hydration__comment: __typename
hydration__comment__commentId: commentId
id
}
}
variables: { }
# language=JSON
response: |-
{
"data": {
"issueById": {
"__typename__hydration__comment": "Issue",
"hydration__comment__commentId": "C1",
"id": "C1"
}
},
"extensions": {}
}
# language=JSON
response: |-
{
"errors": [
{
"locations": [],
"message": "An error has occurred",
"extensions": {
"classification": "ValidationError"
}
}
],
"data": {
"issueById": {
"id": "C1",
"comment": null
}
}
}
Loading

0 comments on commit f95b257

Please sign in to comment.