From 6bf938eead3b880a6a3b11cfc40f46b96f5a130a Mon Sep 17 00:00:00 2001 From: Artyom Emelyanenko Date: Tue, 5 Mar 2024 10:41:53 +1100 Subject: [PATCH 1/3] don't call underlying services on empty queries --- .../java/graphql/nadel/NadelExecutionHints.kt | 14 ++- .../main/java/graphql/nadel/NextgenEngine.kt | 38 +++++- .../transform/query/NadelFieldToService.kt | 2 +- .../nadel/hints/ShortCircuitEmptyQueryHint.kt | 14 +++ .../kotlin/graphql/nadel/tests/EngineTests.kt | 3 +- .../nadel/tests/hooks/remove-fields.kt | 47 +++++++- .../transforms/RemoveFieldTestTransform.kt | 11 +- ...d-hydration-top-level-field-is-removed.yml | 109 +++++++++++++++++ .../hydration-top-level-field-is-removed.yml | 98 ++++++++++++++++ ...mespaced-field-is-removed-with-renames.yml | 57 +++++++++ .../namespaced-field-is-removed.yml | 57 +++++++++ ...d-hydration-top-level-field-is-removed.yml | 105 +++++++++++++++++ ...top-level-field-is-removed-hint-is-off.yml | 62 ++++++++++ .../top-level-field-is-removed.yml | 110 ++---------------- 14 files changed, 605 insertions(+), 122 deletions(-) create mode 100644 lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt create mode 100644 test/src/test/resources/fixtures/field removed/hidden-namespaced-hydration-top-level-field-is-removed.yml create mode 100644 test/src/test/resources/fixtures/field removed/hydration-top-level-field-is-removed.yml create mode 100644 test/src/test/resources/fixtures/field removed/namespaced-field-is-removed-with-renames.yml create mode 100644 test/src/test/resources/fixtures/field removed/namespaced-field-is-removed.yml create mode 100644 test/src/test/resources/fixtures/field removed/namespaced-hydration-top-level-field-is-removed.yml create mode 100644 test/src/test/resources/fixtures/field removed/top-level-field-is-removed-hint-is-off.yml diff --git a/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt b/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt index ec8a33f0a..2da038b08 100644 --- a/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt +++ b/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt @@ -1,10 +1,11 @@ package graphql.nadel import graphql.nadel.hints.AllDocumentVariablesHint -import graphql.nadel.hints.NadelDeferSupportHint import graphql.nadel.hints.LegacyOperationNamesHint +import graphql.nadel.hints.NadelDeferSupportHint import graphql.nadel.hints.NewBatchHydrationGroupingHint import graphql.nadel.hints.NewResultMergerAndNamespacedTypename +import graphql.nadel.hints.ShortCircuitEmptyQueryHint data class NadelExecutionHints( val legacyOperationNames: LegacyOperationNamesHint, @@ -12,6 +13,7 @@ data class NadelExecutionHints( val newResultMergerAndNamespacedTypename: NewResultMergerAndNamespacedTypename, val newBatchHydrationGrouping: NewBatchHydrationGroupingHint, val deferSupport: NadelDeferSupportHint, + val shortCircuitEmptyQuery: ShortCircuitEmptyQueryHint, ) { /** * Returns a builder with the same field values as this object. @@ -29,6 +31,7 @@ data class NadelExecutionHints( private var newResultMergerAndNamespacedTypename = NewResultMergerAndNamespacedTypename { false } private var newBatchHydrationGrouping = NewBatchHydrationGroupingHint { false } private var deferSupport = NadelDeferSupportHint { false } + private var shortCircuitEmptyQuery = ShortCircuitEmptyQueryHint { false } constructor() @@ -36,6 +39,7 @@ data class NadelExecutionHints( legacyOperationNames = nadelExecutionHints.legacyOperationNames allDocumentVariablesHint = nadelExecutionHints.allDocumentVariablesHint newResultMergerAndNamespacedTypename = nadelExecutionHints.newResultMergerAndNamespacedTypename + shortCircuitEmptyQuery = nadelExecutionHints.shortCircuitEmptyQuery } fun legacyOperationNames(flag: LegacyOperationNamesHint): Builder { @@ -63,13 +67,19 @@ data class NadelExecutionHints( return this } + fun shortCircuitEmptyQuery(flag: ShortCircuitEmptyQueryHint): Builder { + shortCircuitEmptyQuery = flag + return this + } + fun build(): NadelExecutionHints { return NadelExecutionHints( legacyOperationNames, allDocumentVariablesHint, newResultMergerAndNamespacedTypename, newBatchHydrationGrouping, - deferSupport + deferSupport, + shortCircuitEmptyQuery, ) } } diff --git a/lib/src/main/java/graphql/nadel/NextgenEngine.kt b/lib/src/main/java/graphql/nadel/NextgenEngine.kt index d4e7ca88a..692b0ed9b 100644 --- a/lib/src/main/java/graphql/nadel/NextgenEngine.kt +++ b/lib/src/main/java/graphql/nadel/NextgenEngine.kt @@ -6,8 +6,10 @@ import graphql.ExecutionResult import graphql.GraphQLError import graphql.execution.ExecutionIdProvider import graphql.execution.instrumentation.InstrumentationState +import graphql.introspection.Introspection.TypeNameMetaFieldDef import graphql.language.Document import graphql.nadel.engine.NadelExecutionContext +import graphql.nadel.engine.blueprint.IntrospectionService import graphql.nadel.engine.blueprint.NadelDefaultIntrospectionRunner import graphql.nadel.engine.blueprint.NadelExecutionBlueprintFactory import graphql.nadel.engine.blueprint.NadelIntrospectionRunnerFactory @@ -39,10 +41,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 @@ -72,6 +76,7 @@ internal class NextgenEngine( ) { private val coroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Default) private val services: Map = services.strictAssociateBy { it.name } + private val engineSchemaIntrospectionService = IntrospectionService(engineSchema, introspectionRunnerFactory) private val overallExecutionBlueprint = NadelExecutionBlueprintFactory.create( engineSchema = engineSchema, services = services, @@ -270,7 +275,7 @@ internal class NextgenEngine( operationName = getOperationName(service, executionContext), topLevelFields = listOf(transformedQuery), variablePredicate = jsonPredicate, - deferSupport = executionContext.hints.deferSupport.invoke() + deferSupport = executionContext.hints.deferSupport(), ) } @@ -285,10 +290,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) { @@ -330,6 +334,32 @@ 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(executableNormalizedField.singleObjectTypeName) + val topLevelFieldDefinition = + operationType.fieldDefinitions.firstOrNull { it.name == executableNormalizedField.name } + ?: return false + 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 diff --git a/lib/src/main/java/graphql/nadel/engine/transform/query/NadelFieldToService.kt b/lib/src/main/java/graphql/nadel/engine/transform/query/NadelFieldToService.kt index ba6b38a16..6b5f76a50 100644 --- a/lib/src/main/java/graphql/nadel/engine/transform/query/NadelFieldToService.kt +++ b/lib/src/main/java/graphql/nadel/engine/transform/query/NadelFieldToService.kt @@ -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, diff --git a/lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt b/lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt new file mode 100644 index 000000000..4aa5ea6bd --- /dev/null +++ b/lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt @@ -0,0 +1,14 @@ +package graphql.nadel.hints + +import graphql.nadel.Service + +fun interface ShortCircuitEmptyQueryHint { + /** + * 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 +} diff --git a/test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt b/test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt index 65efc2f76..d6fa1e8ca 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/EngineTests.kt @@ -36,8 +36,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() diff --git a/test/src/test/kotlin/graphql/nadel/tests/hooks/remove-fields.kt b/test/src/test/kotlin/graphql/nadel/tests/hooks/remove-fields.kt index 670c5a0ae..8c665c2db 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/hooks/remove-fields.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/hooks/remove-fields.kt @@ -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) : NadelExecutionHooks { @@ -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 diff --git a/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt b/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt index b2f42f962..02da60953 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt @@ -13,7 +13,6 @@ 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 @@ -77,11 +76,11 @@ class RemoveFieldTestTransform : NadelTransform { state: GraphQLError, nodes: JsonNodes, ): List { - val parentNodes = JsonNodeExtractor.getNodesAt( - data = result.data, - queryPath = underlyingParentField?.queryPath ?: NadelQueryPath.root, - flatten = true, - ) + val parentNodes = JsonNodes(result.data) + .getNodesAt( + queryPath = underlyingParentField?.queryPath ?: NadelQueryPath.root, + flatten = true, + ) return parentNodes.map { parentNode -> NadelResultInstruction.Set( diff --git a/test/src/test/resources/fixtures/field removed/hidden-namespaced-hydration-top-level-field-is-removed.yml b/test/src/test/resources/fixtures/field removed/hidden-namespaced-hydration-top-level-field-is-removed.yml new file mode 100644 index 000000000..ad1f0847a --- /dev/null +++ b/test/src/test/resources/fixtures/field removed/hidden-namespaced-hydration-top-level-field-is-removed.yml @@ -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 + } + } + } diff --git a/test/src/test/resources/fixtures/field removed/hydration-top-level-field-is-removed.yml b/test/src/test/resources/fixtures/field removed/hydration-top-level-field-is-removed.yml new file mode 100644 index 000000000..3a9957ed6 --- /dev/null +++ b/test/src/test/resources/fixtures/field removed/hydration-top-level-field-is-removed.yml @@ -0,0 +1,98 @@ +name: "hydration top level field is removed" +enabled: true +# language=GraphQL +overallSchema: + IssueService: | + type Query { + issueById(id: ID): Issue + } + type Issue { + id: ID + comment: Comment @hydrated( + service: "CommentService" + field: "commentById" + arguments: [ + {name: "id", value: "$source.commentId"} + ] + ) + } + CommentService: | + directive @toBeDeleted on FIELD_DEFINITION + type Query { + commentById(id: ID): Comment @toBeDeleted + } + type Comment { + id: ID + } +# language=GraphQL +underlyingSchema: + IssueService: | + type Query { + issueById(id: ID): Issue + } + type Issue { + id: ID + commentId: ID + } + CommentService: | + type Query { + commentById(id: ID): Comment + } + 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 + } + } + } diff --git a/test/src/test/resources/fixtures/field removed/namespaced-field-is-removed-with-renames.yml b/test/src/test/resources/fixtures/field removed/namespaced-field-is-removed-with-renames.yml new file mode 100644 index 000000000..534314429 --- /dev/null +++ b/test/src/test/resources/fixtures/field removed/namespaced-field-is-removed-with-renames.yml @@ -0,0 +1,57 @@ +name: "namespaced field is removed with renames" +enabled: true +# language=GraphQL +overallSchema: + CommentService: | + directive @toBeDeleted on FIELD_DEFINITION + directive @namespaced on FIELD_DEFINITION + type Query { + commentApi: CommentApi @namespaced + } + type CommentApi @renamed(from: "CommentApiUnderlying") { + commentById(id: ID): Comment @toBeDeleted @renamed(from: "commentByIdUnderlying") + } + type Comment { + id: ID + } +# language=GraphQL +underlyingSchema: + CommentService: | + type Query { + commentApi: CommentApiUnderlying + } + type CommentApiUnderlying { + commentByIdUnderlying(id: ID): Comment + } + type Comment { + id: ID + } +# language=GraphQL +query: | + query { + commentApi { + commentById(id: "C1") { + id + } + } + } +variables: { } +serviceCalls: [ ] +# language=JSON +response: |- + { + "errors": [ + { + "locations": [], + "message": "null", + "extensions": { + "classification": "ValidationError" + } + } + ], + "data": { + "commentApi": { + "commentById": null + } + } + } diff --git a/test/src/test/resources/fixtures/field removed/namespaced-field-is-removed.yml b/test/src/test/resources/fixtures/field removed/namespaced-field-is-removed.yml new file mode 100644 index 000000000..40d5804cf --- /dev/null +++ b/test/src/test/resources/fixtures/field removed/namespaced-field-is-removed.yml @@ -0,0 +1,57 @@ +name: "namespaced field is removed" +enabled: true +# language=GraphQL +overallSchema: + CommentService: | + directive @toBeDeleted on FIELD_DEFINITION + directive @namespaced on FIELD_DEFINITION + type Query { + commentApi: CommentApi @namespaced + } + type CommentApi { + commentById(id: ID): Comment @toBeDeleted + } + type Comment { + id: ID + } +# language=GraphQL +underlyingSchema: + CommentService: | + type Query { + commentApi: CommentApi + } + type CommentApi { + commentById(id: ID): Comment + } + type Comment { + id: ID + } +# language=GraphQL +query: | + query { + commentApi { + commentById(id: "C1") { + id + } + } + } +variables: { } +serviceCalls: [ ] +# language=JSON +response: |- + { + "errors": [ + { + "locations": [], + "message": "null", + "extensions": { + "classification": "ValidationError" + } + } + ], + "data": { + "commentApi": { + "commentById": null + } + } + } diff --git a/test/src/test/resources/fixtures/field removed/namespaced-hydration-top-level-field-is-removed.yml b/test/src/test/resources/fixtures/field removed/namespaced-hydration-top-level-field-is-removed.yml new file mode 100644 index 000000000..d1470c0ae --- /dev/null +++ b/test/src/test/resources/fixtures/field removed/namespaced-hydration-top-level-field-is-removed.yml @@ -0,0 +1,105 @@ +name: "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 + } + type CommentApi { + commentById(id: ID): Comment @toBeDeleted + } + 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 + } + type CommentApi { + commentById(id: ID): Comment + } + 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 + } + } + } diff --git a/test/src/test/resources/fixtures/field removed/top-level-field-is-removed-hint-is-off.yml b/test/src/test/resources/fixtures/field removed/top-level-field-is-removed-hint-is-off.yml new file mode 100644 index 000000000..f61962199 --- /dev/null +++ b/test/src/test/resources/fixtures/field removed/top-level-field-is-removed-hint-is-off.yml @@ -0,0 +1,62 @@ +name: "top-level-field-is-removed-hint-is-off" +enabled: true +# language=GraphQL +overallSchema: + CommentService: | + directive @toBeDeleted on FIELD_DEFINITION + type Query { + commentById(id: ID): Comment @toBeDeleted + } + type Comment { + id: ID + } +# language=GraphQL +underlyingSchema: + CommentService: | + type Query { + commentById(id: ID): Comment + } + type Comment { + id: ID + } +# language=GraphQL +query: | + query { + commentById(id: "C1") { + id + } + } +variables: { } +serviceCalls: + - serviceName: "CommentService" + request: + # language=GraphQL + query: | + { + uuid_typename: __typename + } + variables: { } + # language=JSON + response: |- + { + "data": { + "uuid_typename": "Query" + }, + "extensions": {} + } +# language=JSON +response: |- + { + "errors": [ + { + "locations": [], + "message": "null", + "extensions": { + "classification": "ValidationError" + } + } + ], + "data": { + "commentById": null + } + } diff --git a/test/src/test/resources/fixtures/field removed/top-level-field-is-removed.yml b/test/src/test/resources/fixtures/field removed/top-level-field-is-removed.yml index d3947ae7e..70deed0ab 100644 --- a/test/src/test/resources/fixtures/field removed/top-level-field-is-removed.yml +++ b/test/src/test/resources/fixtures/field removed/top-level-field-is-removed.yml @@ -1,107 +1,23 @@ name: "top level field is removed" -enabled: false +enabled: true # language=GraphQL overallSchema: - IssueService: | - type Query { - issues(jql: String): [Issue] - issueById(id: String): Issue - } - type Issue { - id: ID - key: String - summary: String - description: String - epic: Epic - reporter: User - @hydrated( - service: "UserService" - field: "userById" - arguments: [{name: "id" value: "$source.reporterId"}] - ) - comments: [Comment] - @hydrated( - service: "CommentService" - field: "commentById" - arguments: [{name: "id" value: "$source.commentIds"}] - ) - } - type Epic { - id: ID - title: String - description: String - } - UserService: | - type Query { - users(ids: [ID]): [User] - userById(id: ID): User - } - type User { - userId: ID - displayName: String - avatarUrl: String - } CommentService: | + directive @toBeDeleted on FIELD_DEFINITION type Query { - comments(ids: [ID]): [Comment] - commentById(id: ID): Comment + commentById(id: ID): Comment @toBeDeleted } type Comment { id: ID - commentText: String @renamed(from: "text") - created: String - author: User - @hydrated( - service: "UserService" - field: "userById" - arguments: [{name: "id" value: "$source.authorId"}] - ) } # language=GraphQL underlyingSchema: - IssueService: | - type Epic { - description: String - id: ID - title: String - } - - type Issue { - commentIds: [ID] - description: String - epic: Epic - id: ID - key: String - reporterId: ID - summary: String - } - - type Query { - issueById(id: String): Issue - issues(jql: String): [Issue] - } CommentService: | - type Comment { - authorId: ID - created: String - id: ID - text: String - } - type Query { commentById(id: ID): Comment - comments(ids: [ID]): [Comment] - } - UserService: | - type Query { - userById(id: ID): User - users(ids: [ID]): [User] } - - type User { - avatarUrl: String - displayName: String - userId: ID + type Comment { + id: ID } # language=GraphQL query: | @@ -117,24 +33,14 @@ response: |- { "errors": [ { - "path": [ - "commentById" - ], - "message": "removed field", + "locations": [], + "message": "null", "extensions": { - "classification": "ExecutionAborted" + "classification": "ValidationError" } } ], "data": { "commentById": null - }, - "extensions": { - "resultComplexity": { - "totalNodeCount": 0, - "serviceNodeCounts": {}, - "fieldRenamesCount": 0, - "typeRenamesCount": 0 - } } } From e12efc9772e1f331afebadaca5baaaedc266af0c Mon Sep 17 00:00:00 2001 From: Artyom Emelyanenko Date: Thu, 23 May 2024 10:21:05 +1000 Subject: [PATCH 2/3] rename a file --- lib/src/main/java/graphql/nadel/NadelExecutionHints.kt | 10 ++++------ ...QueryHint.kt => NadelShortCircuitEmptyQueryHint.kt} | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) rename lib/src/main/java/graphql/nadel/hints/{ShortCircuitEmptyQueryHint.kt => NadelShortCircuitEmptyQueryHint.kt} (90%) diff --git a/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt b/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt index 87c2a2165..669e607af 100644 --- a/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt +++ b/lib/src/main/java/graphql/nadel/NadelExecutionHints.kt @@ -3,11 +3,10 @@ package graphql.nadel import graphql.nadel.hints.AllDocumentVariablesHint import graphql.nadel.hints.LegacyOperationNamesHint import graphql.nadel.hints.NadelDeferSupportHint -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 -import graphql.nadel.hints.ShortCircuitEmptyQueryHint data class NadelExecutionHints( val legacyOperationNames: LegacyOperationNamesHint, @@ -16,7 +15,7 @@ data class NadelExecutionHints( val newBatchHydrationGrouping: NewBatchHydrationGroupingHint, val deferSupport: NadelDeferSupportHint, val sharedTypeRenames: NadelSharedTypeRenamesHint, - val shortCircuitEmptyQuery: ShortCircuitEmptyQueryHint, + val shortCircuitEmptyQuery: NadelShortCircuitEmptyQueryHint, ) { /** * Returns a builder with the same field values as this object. @@ -34,7 +33,7 @@ data class NadelExecutionHints( private var newResultMergerAndNamespacedTypename = NewResultMergerAndNamespacedTypename { false } private var newBatchHydrationGrouping = NewBatchHydrationGroupingHint { false } private var deferSupport = NadelDeferSupportHint { false } - private var shortCircuitEmptyQuery = ShortCircuitEmptyQueryHint { false } + private var shortCircuitEmptyQuery = NadelShortCircuitEmptyQueryHint { false } private var sharedTypeRenames = NadelSharedTypeRenamesHint { false } constructor() @@ -71,7 +70,7 @@ data class NadelExecutionHints( return this } - fun shortCircuitEmptyQuery(flag: ShortCircuitEmptyQueryHint): Builder { + fun shortCircuitEmptyQuery(flag: NadelShortCircuitEmptyQueryHint): Builder { shortCircuitEmptyQuery = flag return this } @@ -88,7 +87,6 @@ data class NadelExecutionHints( newResultMergerAndNamespacedTypename, newBatchHydrationGrouping, deferSupport, - deferSupport, sharedTypeRenames, shortCircuitEmptyQuery, ) diff --git a/lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt b/lib/src/main/java/graphql/nadel/hints/NadelShortCircuitEmptyQueryHint.kt similarity index 90% rename from lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt rename to lib/src/main/java/graphql/nadel/hints/NadelShortCircuitEmptyQueryHint.kt index 4aa5ea6bd..2bcd94830 100644 --- a/lib/src/main/java/graphql/nadel/hints/ShortCircuitEmptyQueryHint.kt +++ b/lib/src/main/java/graphql/nadel/hints/NadelShortCircuitEmptyQueryHint.kt @@ -2,7 +2,7 @@ package graphql.nadel.hints import graphql.nadel.Service -fun interface ShortCircuitEmptyQueryHint { +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 From bc5d3085c2d4d7d2b8fac040a1c623c2b4260e04 Mon Sep 17 00:00:00 2001 From: Artyom Emelyanenko Date: Thu, 13 Jun 2024 10:42:38 +1000 Subject: [PATCH 3/3] pr feedback --- lib/src/main/java/graphql/nadel/NextgenEngine.kt | 4 +--- .../tests/transforms/RemoveFieldTestTransform.kt | 11 ++++------- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/lib/src/main/java/graphql/nadel/NextgenEngine.kt b/lib/src/main/java/graphql/nadel/NextgenEngine.kt index 5a074058c..0de29dcb2 100644 --- a/lib/src/main/java/graphql/nadel/NextgenEngine.kt +++ b/lib/src/main/java/graphql/nadel/NextgenEngine.kt @@ -383,9 +383,7 @@ internal class NextgenEngine( return true } val operationType = engineSchema.getTypeAs(executableNormalizedField.singleObjectTypeName) - val topLevelFieldDefinition = - operationType.fieldDefinitions.firstOrNull { it.name == executableNormalizedField.name } - ?: return false + val topLevelFieldDefinition = operationType.getField(executableNormalizedField.name) return if (topLevelFieldDefinition.hasAppliedDirective(namespacedDirectiveDefinition.name)) { executableNormalizedField.hasChildren() && executableNormalizedField.children.all { it.name == TypeNameMetaFieldDef.name } diff --git a/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt b/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt index b434a40b1..dbcc8ff12 100644 --- a/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt +++ b/test/src/test/kotlin/graphql/nadel/tests/transforms/RemoveFieldTestTransform.kt @@ -17,7 +17,6 @@ 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 @@ -77,12 +76,10 @@ class RemoveFieldTestTransform : NadelTransform { state: GraphQLError, nodes: JsonNodes, ): List { - val parentNodes = JsonNodes(result.data) - .getNodesAt( - queryPath = underlyingParentField?.queryPath ?: NadelQueryPath.root, - flatten = true, - ) - + val parentNodes = nodes.getNodesAt( + queryPath = underlyingParentField?.queryPath ?: NadelQueryPath.root, + flatten = true, + ) return parentNodes.map { parentNode -> NadelResultInstruction.Set( subject = parentNode,