-
Notifications
You must be signed in to change notification settings - Fork 28
Filter objectTypeNames for non-batched hydration #706
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,15 @@ interface NadelGenericHydrationInstruction { | |
| */ | ||
| val backingFieldDef: GraphQLFieldDefinition | ||
|
|
||
| /** | ||
| * The names of the object types the backing field returns. | ||
| * | ||
| * Note: when virtual types are involved these are the virtual types. | ||
| * | ||
| * Could be empty if there are no object types involved e.g. returns scalar, enum etc. | ||
| */ | ||
| val backingFieldReturnsObjectTypeNames: Set<String> | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Precomputes this info and stores it. |
||
|
|
||
| /** | ||
| * The container of the backing field in the overall schema referenced by [queryPathToBackingField]. | ||
| */ | ||
|
|
@@ -101,6 +110,7 @@ data class NadelHydrationFieldInstruction( | |
| override val sourceFields: List<NadelQueryPath>, | ||
| override val backingFieldDef: GraphQLFieldDefinition, | ||
| override val backingFieldContainer: GraphQLFieldsContainer, | ||
| override val backingFieldReturnsObjectTypeNames: Set<String>, | ||
| override val condition: NadelHydrationCondition?, | ||
| /** | ||
| * Hydration can bring about virtual types. | ||
|
|
@@ -124,6 +134,7 @@ data class NadelBatchHydrationFieldInstruction( | |
| override val sourceFields: List<NadelQueryPath>, | ||
| override val backingFieldDef: GraphQLFieldDefinition, | ||
| override val backingFieldContainer: GraphQLFieldsContainer, | ||
| override val backingFieldReturnsObjectTypeNames: Set<String>, | ||
| override val condition: NadelHydrationCondition?, | ||
| val batchSize: Int, | ||
| val batchHydrationMatchStrategy: NadelBatchHydrationMatchStrategy, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| package graphql.nadel.engine.transform.hydration | ||
|
|
||
| import graphql.nadel.NadelExecutionHints | ||
| import graphql.nadel.Service | ||
| import graphql.nadel.engine.NadelExecutionContext | ||
| import graphql.nadel.engine.blueprint.NadelBatchHydrationFieldInstruction | ||
|
|
@@ -40,7 +41,13 @@ internal object NadelHydrationFieldsBuilder { | |
| makeBackingQueries( | ||
| instruction = instruction, | ||
| fieldArguments = args, | ||
| fieldChildren = deepClone(virtualField.children), | ||
| fieldChildren = deepClone( | ||
| if (executionContext.hints.hydrationFilterObjectTypes()) { | ||
| filterChildren(instruction, virtualField.children) | ||
| } else { | ||
| virtualField.children | ||
| } | ||
| ), | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the fix needed. |
||
| executionBlueprint = executionBlueprint, | ||
| ) | ||
| } | ||
|
|
@@ -56,30 +63,37 @@ internal object NadelHydrationFieldsBuilder { | |
| } | ||
|
|
||
| fun makeBatchBackingQueries( | ||
| executionHints: NadelExecutionHints, | ||
| executionBlueprint: NadelOverallExecutionBlueprint, | ||
| instruction: NadelBatchHydrationFieldInstruction, | ||
| aliasHelper: NadelAliasHelper, | ||
| virtualField: ExecutableNormalizedField, | ||
| argBatches: List<Map<NadelHydrationArgument, NormalizedInputValue>>, | ||
| ): List<ExecutableNormalizedField> { | ||
| val backingFieldOverallObjectTypeNames = getBackingFieldOverallObjectTypenames(instruction, executionBlueprint) | ||
| val fieldChildren = deepClone(fields = virtualField.children) | ||
| .mapNotNull { childField -> | ||
| val objectTypesAreNotReturnedByBackingField = | ||
| backingFieldOverallObjectTypeNames.none { it in childField.objectTypeNames } | ||
| val fieldChildren = if (executionHints.hydrationFilterObjectTypes()) { | ||
| deepClone(fields = filterChildren(instruction, virtualField.children)) + | ||
| makeObjectIdFields(executionBlueprint, aliasHelper, instruction) | ||
| } else { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So thing is, we applied this type name filtering for batch hydrations but not for non-batched hydration. I unified the behavior and also moved the Also moved the filtering to before FF is here to ensure smooth rollout. |
||
| val backingFieldOverallObjectTypeNames = | ||
| getBackingFieldOverallObjectTypenames(instruction, executionBlueprint) | ||
| deepClone(fields = virtualField.children) | ||
| .mapNotNull { childField -> | ||
| val objectTypesAreNotReturnedByBackingField = | ||
| backingFieldOverallObjectTypeNames.none { it in childField.objectTypeNames } | ||
|
|
||
| if (objectTypesAreNotReturnedByBackingField) { | ||
| null | ||
| } else { | ||
| childField.toBuilder() | ||
| .clearObjectTypesNames() | ||
| .objectTypeNames(childField.objectTypeNames.filter { it in backingFieldOverallObjectTypeNames }) | ||
| .build() | ||
| if (objectTypesAreNotReturnedByBackingField) { | ||
| null | ||
| } else { | ||
| childField.toBuilder() | ||
| .clearObjectTypesNames() | ||
| .objectTypeNames(childField.objectTypeNames.filter { it in backingFieldOverallObjectTypeNames }) | ||
| .build() | ||
| } | ||
| } | ||
| } | ||
| .let { children -> | ||
| children + makeObjectIdFields(executionBlueprint, aliasHelper, instruction) | ||
| } | ||
| .let { children -> | ||
| children + makeObjectIdFields(executionBlueprint, aliasHelper, instruction) | ||
| } | ||
| } | ||
|
|
||
| return argBatches.map { argBatch -> | ||
| makeBackingQueries( | ||
|
|
@@ -91,6 +105,29 @@ internal object NadelHydrationFieldsBuilder { | |
| } | ||
| } | ||
|
|
||
| private fun filterChildren( | ||
| instruction: NadelGenericHydrationInstruction, | ||
| children: List<ExecutableNormalizedField>, | ||
| ): List<ExecutableNormalizedField> { | ||
| return children | ||
| .mapNotNull { childField -> | ||
| val legalObjectTypeNames = | ||
| childField.objectTypeNames.intersect(instruction.backingFieldReturnsObjectTypeNames) | ||
|
|
||
| if (legalObjectTypeNames.isEmpty()) { | ||
| null | ||
| } else if (legalObjectTypeNames == childField.objectTypeNames) { | ||
| childField | ||
| } else { | ||
| childField.toBuilder() | ||
| .clearObjectTypesNames() | ||
| .objectTypeNames(legalObjectTypeNames.toList()) | ||
| .build() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Deprecated("Will be removed once NadelHydrationFilterObjectTypesHint is rolled out") | ||
| private fun getBackingFieldOverallObjectTypenames( | ||
| instruction: NadelBatchHydrationFieldInstruction, | ||
| executionBlueprint: NadelOverallExecutionBlueprint, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,7 +16,6 @@ import graphql.nadel.engine.transform.getInstructionsForNode | |
| import graphql.nadel.engine.transform.hydration.NadelHydrationFieldsBuilder | ||
| import graphql.nadel.engine.transform.hydration.NadelHydrationUtil.getInstructionsToAddErrors | ||
| import graphql.nadel.engine.transform.hydration.batch.NadelBatchHydrationTransform.State | ||
| import graphql.nadel.engine.transform.hydration.batch.NadelNewBatchHydrator.SourceObjectMetadata | ||
| import graphql.nadel.engine.transform.hydration.batch.indexing.NadelBatchHydrationIndexBasedIndexer | ||
| import graphql.nadel.engine.transform.hydration.batch.indexing.NadelBatchHydrationIndexKey | ||
| import graphql.nadel.engine.transform.hydration.batch.indexing.NadelBatchHydrationIndexer | ||
|
|
@@ -444,6 +443,7 @@ internal class NadelNewBatchHydrator( | |
|
|
||
| val queries = NadelHydrationFieldsBuilder | ||
| .makeBatchBackingQueries( | ||
| executionHints = executionContext.hints, | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to evaluate FF for new behavior. |
||
| executionBlueprint = executionBlueprint, | ||
| instruction = instruction, | ||
| aliasHelper = aliasHelper, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,13 +60,11 @@ import graphql.schema.GraphQLCodeRegistry | |
| 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 | ||
| import graphql.schema.GraphQLTypeUtil | ||
| import graphql.schema.GraphQLUnionType | ||
| import graphql.schema.GraphQLUnmodifiedType | ||
| import graphql.schema.idl.TypeUtil | ||
| import kotlinx.coroutines.future.asDeferred | ||
|
|
@@ -525,20 +523,31 @@ internal suspend fun NadelInstrumentation.beginExecute( | |
| * Turns GraphQL types to object types when possible e.g. finds concrete implementations | ||
| * for interfaces, gets object types inside unions, and returns objects as is. | ||
| */ | ||
| fun resolveObjectTypes( | ||
| inline fun resolveObjectTypes( | ||
| schema: GraphQLSchema, | ||
| type: GraphQLType, | ||
| onNotObjectType: (GraphQLType) -> Nothing, | ||
| ): List<GraphQLObjectType> { | ||
| return when (val unwrappedType = type.unwrapAll()) { | ||
| is GraphQLObjectType -> listOf(unwrappedType) | ||
| is GraphQLUnionType -> unwrappedType.types.flatMap { | ||
| resolveObjectTypes(schema, type = it, onNotObjectType) | ||
| } | ||
|
|
||
| is GraphQLInterfaceType -> schema.getImplementations(unwrappedType) | ||
| else -> onNotObjectType(unwrappedType) | ||
| } | ||
| return type.unwrapAll() | ||
| .whenType( | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Edited this to use the new |
||
| enumType = onNotObjectType, | ||
| inputObjectType = onNotObjectType, | ||
| interfaceType = { interfaceType -> | ||
| schema.getImplementations(interfaceType) | ||
| .map { objectImpl -> objectImpl } | ||
| }, | ||
| objectType = { | ||
| listOf(it) | ||
| }, | ||
| scalarType = onNotObjectType, | ||
| unionType = { unionType -> | ||
| unionType.types | ||
| .map { memberType -> | ||
| memberType as? GraphQLObjectType | ||
| ?: throw IllegalArgumentException("Member type must be object but found ${memberType?.javaClass?.name}") | ||
| } | ||
| }, | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| package graphql.nadel.hints | ||
|
|
||
| fun interface NadelHydrationFilterObjectTypesHint { | ||
| operator fun invoke(): Boolean | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import graphql.nadel.definition.hydration.NadelHydrationDefinition | |
| import graphql.nadel.definition.virtualType.hasVirtualTypeDefinition | ||
| import graphql.nadel.engine.blueprint.NadelBatchHydrationFieldInstruction | ||
| import graphql.nadel.engine.blueprint.NadelHydrationFieldInstruction | ||
| import graphql.nadel.engine.blueprint.NadelVirtualTypeContext | ||
| import graphql.nadel.engine.blueprint.hydration.NadelBatchHydrationMatchStrategy | ||
| import graphql.nadel.engine.blueprint.hydration.NadelHydrationArgument | ||
| import graphql.nadel.engine.blueprint.hydration.NadelHydrationCondition | ||
|
|
@@ -25,6 +26,7 @@ import graphql.nadel.engine.util.isList | |
| import graphql.nadel.engine.util.isNonNull | ||
| import graphql.nadel.engine.util.makeFieldCoordinates | ||
| import graphql.nadel.engine.util.partitionCount | ||
| import graphql.nadel.engine.util.resolveObjectTypes | ||
| import graphql.nadel.engine.util.singleOfTypeOrNull | ||
| import graphql.nadel.engine.util.startsWith | ||
| import graphql.nadel.engine.util.unwrapAll | ||
|
|
@@ -279,6 +281,7 @@ class NadelHydrationValidation internal constructor( | |
| timeout = hydrationDefinition.timeout, | ||
| sourceFields = sourceFields, | ||
| backingFieldDef = backingField, | ||
| backingFieldReturnsObjectTypeNames = getReturnsObjectTypeNames(backingField, virtualTypeContext), | ||
| backingFieldContainer = backingFieldContainer, | ||
| condition = hydrationCondition, | ||
| virtualTypeContext = virtualTypeContext, | ||
|
|
@@ -356,6 +359,7 @@ class NadelHydrationValidation internal constructor( | |
| sourceFields = sourceFields, | ||
| backingFieldDef = backingField, | ||
| backingFieldContainer = backingFieldContainer, | ||
| backingFieldReturnsObjectTypeNames = getReturnsObjectTypeNames(backingField, null), | ||
| condition = hydrationCondition, | ||
| batchSize = hydrationDefinition.batchSize, | ||
| batchHydrationMatchStrategy = matchStrategy, | ||
|
|
@@ -744,4 +748,21 @@ class NadelHydrationValidation internal constructor( | |
| } | ||
| return false | ||
| } | ||
|
|
||
| context(NadelValidationContext) | ||
| private fun getReturnsObjectTypeNames( | ||
| backingField: GraphQLFieldDefinition, | ||
| virtualTypeContext: NadelVirtualTypeContext?, | ||
| ): Set<String> { | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Function that precomputes returned object type names |
||
| val returnsObjectTypes = resolveObjectTypes(engineSchema, backingField.type) { return emptySet() } | ||
| .mapTo(mutableSetOf()) { it.name } | ||
|
|
||
| return if (virtualTypeContext == null) { | ||
| returnsObjectTypes | ||
| } else { | ||
| returnsObjectTypes.mapTo(mutableSetOf()) { | ||
| virtualTypeContext.backingTypeToVirtualType[it] ?: it | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remanent code, not used