Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix top level hydration #572

Merged
merged 3 commits into from
Sep 9, 2024
Merged

Fix top level hydration #572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b36bb8
Fix top level hydration
gnawf Aug 13, 2024
7de5eb6
Add plain hydration
gnawf Aug 13, 2024
edddac0
Functional solution
gnawf Sep 9, 2024
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
81 changes: 50 additions & 31 deletions lib/src/main/java/graphql/nadel/NextgenEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ 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.copy
import graphql.nadel.engine.util.getOperationKind
import graphql.nadel.engine.util.newExecutionResult
import graphql.nadel.engine.util.newGraphQLError
Expand All @@ -47,9 +46,9 @@ 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.result.NadelResultMerger
import graphql.nadel.result.NadelResultTracker
import graphql.nadel.schema.NadelDirectives.namespacedDirectiveDefinition
import graphql.nadel.util.OperationNameUtil
import graphql.normalized.ExecutableNormalizedField
import graphql.normalized.ExecutableNormalizedOperationFactory.createExecutableNormalizedOperationWithRawVariables
Expand All @@ -64,7 +63,6 @@ import kotlinx.coroutines.awaitAll
import kotlinx.coroutines.cancel
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.future.asCompletableFuture
import kotlinx.coroutines.future.asDeferred
Expand Down Expand Up @@ -285,11 +283,10 @@ internal class NextgenEngine(
field = topLevelField
)
}
val transformedQuery = queryTransform.result.single()
val result: ServiceExecutionResult = timer.time(step = RootStep.ServiceExecution.child(service.name)) {
executeService(
service = service,
transformedQuery = transformedQuery,
topLevelFields = queryTransform.result,
executionContext = executionContext,
serviceExecutionContext = serviceExecutionContext,
executionHydrationDetails = executionContext.hydrationDetails,
Expand All @@ -299,11 +296,11 @@ internal class NextgenEngine(
executionContext.incrementalResultSupport.defer(
result.incrementalItemPublisher
.asFlow()
.onEach {delayedIncrementalResult ->
.onEach { delayedIncrementalResult ->
// Transform
delayedIncrementalResult.incremental
?.filterIsInstance<DeferPayload>()
?.forEach {deferPayload ->
?.forEach { deferPayload ->
resultTransformer
.transform(
executionContext = executionContext,
Expand All @@ -314,7 +311,8 @@ internal class NextgenEngine(
service = service,
result = result,
deferPayload = deferPayload,
) }
)
}
}
)
}
Expand All @@ -338,7 +336,7 @@ internal class NextgenEngine(

private suspend fun executeService(
service: Service,
transformedQuery: ExecutableNormalizedField,
topLevelFields: List<ExecutableNormalizedField>,
executionContext: NadelExecutionContext,
serviceExecutionContext: NadelServiceExecutionContext,
executionHydrationDetails: ServiceExecutionHydrationDetails? = null,
Expand All @@ -352,9 +350,9 @@ internal class NextgenEngine(
val compileResult = timer.time(step = DocumentCompilation) {
compileToDocument(
schema = service.underlyingSchema,
operationKind = transformedQuery.getOperationKind(engineSchema),
operationKind = topLevelFields.first().getOperationKind(engineSchema),
operationName = getOperationName(service, executionContext),
topLevelFields = listOf(transformedQuery),
topLevelFields = topLevelFields,
variablePredicate = jsonPredicate,
deferSupport = executionContext.hints.deferSupport(),
)
Expand All @@ -370,9 +368,16 @@ internal class NextgenEngine(
serviceContext = executionContext.getContextForService(service).await(),
serviceExecutionContext = serviceExecutionContext,
hydrationDetails = executionHydrationDetails,
executableNormalizedField = transformedQuery,
// Prefer non __typename field first, otherwise we just get first
executableNormalizedField = topLevelFields
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So this ServiceExecutionParameters.executableNormalizedField actually needs to be deprecated and removed but we are keeping it for now for compatibility.

It's currently used to extract the cloud ID for routing.

The reason why we went from ExecutableNormalizedField to List<ExecutableNormalizedField> is because the root level hydration transform injects a __typename field.

{
  batch_hydration__myIssues__myIssueKeys: myIssueKeys
  __typename__batch_hydration__myIssues: __typename
}

You could argue that we don't need a __typename for the operation types, but it's not a big deal to just keep it in.

Plus in the future we may end up grouping more root level fields into one execution, so this sort of lays the ground work for that.

.asSequence()
.filterNot {
it.fieldName == TypeNameMetaFieldDef.name
}
.firstOrNull() ?: topLevelFields.first(),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This gets the first non __typename field, to maintain the old behavior.

(Refer to above comment).

)
val serviceExecution = chooseServiceExecution(service, transformedQuery, executionContext.hints)

val serviceExecution = getServiceExecution(service, topLevelFields, executionContext.hints)
val serviceExecResult = try {
serviceExecution.execute(serviceExecParams)
.asDeferred()
Expand Down Expand Up @@ -408,39 +413,53 @@ internal class NextgenEngine(
)
}

val transformedData: MutableJsonMap = serviceExecResult.data.let { data ->
data.takeIf { transformedQuery.resultKey in data }
?: mutableMapOf(transformedQuery.resultKey to null)
}
val transformedData: MutableJsonMap = serviceExecResult.data
gnawf marked this conversation as resolved.
Show resolved Hide resolved
.let { data ->
// Ensures data always has root fields as keys
topLevelFields
.asSequence()
.map {
it.resultKey
}
.associateWithTo(mutableMapOf()) { resultKey ->
data[resultKey]
}
}

return when(serviceExecResult) {
return when (serviceExecResult) {
is NadelServiceExecutionResultImpl -> serviceExecResult.copy(data = transformedData)
is NadelIncrementalServiceExecutionResult -> serviceExecResult.copy(data = transformedData)
}
}

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

return service.serviceExecution
}

private fun onlyTopLevelTypenameField(executableNormalizedField: ExecutableNormalizedField): Boolean {
if (executableNormalizedField.fieldName == TypeNameMetaFieldDef.name) {
private fun isOnlyTopLevelFieldTypename(topLevelFields: List<ExecutableNormalizedField>): Boolean {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code here was modified a bit.

Context: we wanted to add code that would shortcut query execution IF there was only one field and it was __typename

Note: this code is protected by a FF hints.shortCircuitEmptyQuery(service) and it's mostly off because it hasn't been fully rolled out.

val topLevelField = topLevelFields.singleOrNull() ?: return false

if (topLevelField.fieldName == TypeNameMetaFieldDef.name) {
return true
}
val operationType = engineSchema.getTypeAs<GraphQLObjectType>(executableNormalizedField.singleObjectTypeName)
val topLevelFieldDefinition = operationType.getField(executableNormalizedField.name)

val operationType = engineSchema.getTypeAs<GraphQLObjectType>(topLevelField.singleObjectTypeName)
val topLevelFieldDefinition = operationType.getField(topLevelField.name)

return if (topLevelFieldDefinition.hasAppliedDirective(namespacedDirectiveDefinition.name)) {
executableNormalizedField.hasChildren()
&& executableNormalizedField.children.all { it.name == TypeNameMetaFieldDef.name }
} else false
topLevelField.hasChildren()
&& topLevelField.children.all { it.name == TypeNameMetaFieldDef.name }
} else {
false
}
}

private fun getDocumentVariablePredicate(hints: NadelExecutionHints, service: Service): VariablePredicate {
Expand Down
27 changes: 20 additions & 7 deletions lib/src/main/java/graphql/nadel/engine/blueprint/NadelExecutionBlueprintFactory.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import graphql.schema.GraphQLFieldsContainer
import graphql.schema.GraphQLObjectType
import graphql.schema.GraphQLScalarType
import graphql.schema.GraphQLSchema
import graphql.schema.GraphQLType
import java.math.BigInteger

internal object NadelExecutionBlueprintFactory {
Expand Down Expand Up @@ -329,7 +328,7 @@ private class Factory(
return@mapNotNull null
}

val underlyingParentType = getUnderlyingType(hydratedFieldParentType)
val underlyingParentType = getUnderlyingType(hydratedFieldParentType, hydratedFieldDef)
?: error("No underlying type for: ${hydratedFieldParentType.name}")
val fieldDefs = underlyingParentType.getFieldsAlong(inputValueDef.valueSource.queryPathToField.segments)
inputValueDef.takeIf {
Expand Down Expand Up @@ -410,7 +409,7 @@ private class Factory(
private fun getBatchHydrationSourceFields(
matchStrategy: NadelBatchHydrationMatchStrategy,
hydrationArgs: List<NadelHydrationActorInputDef>,
condition: NadelHydrationCondition?
condition: NadelHydrationCondition?,
): List<NadelQueryPath> {
val paths = (when (matchStrategy) {
NadelBatchHydrationMatchStrategy.MatchIndex -> emptyList()
Expand Down Expand Up @@ -541,7 +540,7 @@ private class Factory(
val pathToField = argSourceType.pathToField
FieldResultValue(
queryPathToField = NadelQueryPath(pathToField),
fieldDefinition = getUnderlyingType(hydratedFieldParentType)
fieldDefinition = getUnderlyingType(hydratedFieldParentType, hydratedFieldDef)
?.getFieldAt(pathToField)
?: error("No field defined at: ${hydratedFieldParentType.name}.${pathToField.joinToString(".")}"),
)
Expand All @@ -561,11 +560,25 @@ private class Factory(
}
}

private fun <T : GraphQLType> getUnderlyingType(overallType: T): T? {
/**
* Gets the underlying type for an [GraphQLObjectType]
*
* The [childField] is there in case the [overallType] is an operation type.
* In that case we still need to know which service's operation type to return.
*/
private fun getUnderlyingType(
overallType: GraphQLObjectType,
childField: GraphQLFieldDefinition,
): GraphQLObjectType? {
val renameInstruction = makeTypeRenameInstruction(overallType as? GraphQLDirectiveContainer ?: return null)
val service = definitionNamesToService[overallType.name]
?: error("Unknown service for type: ${overallType.name}")
val underlyingName = renameInstruction?.underlyingName ?: overallType.name

val fieldCoordinates = makeFieldCoordinates(overallType, childField)

val service = definitionNamesToService[overallType.name]
?: coordinatesToService[fieldCoordinates]
?: error("Unable to determine service for $fieldCoordinates")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Main fix is here.
Before we would do type name -> service.
This doesn't work for operation types.
So we now fall back to field name -> service.


return service.underlyingSchema.getTypeAs(underlyingName)
}

Expand Down
62 changes: 62 additions & 0 deletions .../kotlin/graphql/nadel/tests/next/fixtures/batchHydration/BatchHydrationAtQueryTypeTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package graphql.nadel.tests.next.fixtures.batchHydration

import graphql.nadel.engine.util.strictAssociateBy
import graphql.nadel.tests.next.NadelIntegrationTest

class BatchHydrationAtQueryTypeTest : NadelIntegrationTest(
query = """
query {
myIssues {
title
}
}
""".trimIndent(),
services = listOf(
Service(
name = "issues",
overallSchema = """
type Query {
issuesByIds(ids: [ID!]!): [Issue]
myIssueKeys(limit: Int! = 25): [ID!] @hidden
myIssues: [Issue]
@hydrated(
service: "issues"
field: "issuesByIds"
arguments: [{name: "ids", value: "$source.myIssueKeys"}]
identifiedBy: "id"
)
}
type Issue {
id: ID!
title: String
}
""".trimIndent(),
runtimeWiring = { wiring ->
data class Issue(
val id: String,
val title: String,
)

val issuesByIds = listOf(
Issue(id = "hello", title = "Hello there"),
Issue(id = "afternoon", title = "Good afternoon"),
Issue(id = "bye", title = "Farewell"),
).strictAssociateBy { it.id }

wiring
.type("Query") { type ->
type
.dataFetcher("issuesByIds") { env ->
env.getArgument<List<String>>("ids")
?.map {
issuesByIds[it]
}
}
.dataFetcher("myIssueKeys") { env ->
listOf("hello", "bye")
}
}
},
),
),
)
Loading
Loading