Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis pull request implements comprehensive support for the GraphQL Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine as Execution Engine
participant Normalizer as AST Normalizer
participant Planner as Query Planner
participant Resolver as Response Resolver
participant Writer as Response Writer
Client->>Engine: Execute(operation with `@defer`)
Engine->>Normalizer: Normalize with WithInlineDefer()
Normalizer->>Normalizer: inlineFragmentExpandDefer<br/>(expand `@defer` to `@__defer_internal`)
Normalizer->>Normalizer: deferEnsureTypename<br/>(add __typename placeholders)
Normalizer-->>Engine: Normalized operation
Engine->>Planner: Plan deferred operation
Planner->>Planner: collectNodesDSVisitor<br/>(extract defer metadata)
Planner->>Planner: ProcessDefer<br/>(propagate defer parents)
Planner-->>Engine: DeferResponsePlan<br/>(root fetch + deferred groups)
Engine->>Resolver: ResolveGraphQLDeferResponse
Resolver->>Resolver: Resolve initial fetches
Resolver->>Writer: Resolve(initial data)
Writer-->>Client: Initial response
loop For each deferred group
Resolver->>Resolver: Fetch deferred nodes
Resolver->>Resolver: ResolveDefer(group data)
Resolver->>Writer: Flush(incremental chunk)
Writer-->>Client: Incremental response
end
Resolver->>Writer: Complete()
Writer-->>Client: hasNext: false
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
devsergiy
changed the title
Closed
devsergiy
force-pushed
the
feat/eng-7770-add-defer-support
branch
from
ee840d7 to
49ce82b
Compare
…th same directives in different order
add todos
add defer plan kind
fix test expectation
ysmolski
reviewed
Apr 22, 2026
| *v.currentFields[len(v.currentFields)-1].fields = append(*v.currentFields[len(v.currentFields)-1].fields, v.currentField) | ||
| *v.currentObjectFields[len(v.currentObjectFields)-1].fields = append(*v.currentObjectFields[len(v.currentObjectFields)-1].fields, v.currentField) | ||
|
|
||
| // append the current field to the list of current fields |
Contributor
There was a problem hiding this comment.
this and previous comments are like redundant.
ysmolski
reviewed
Apr 22, 2026
Comment on lines
+849
to
+854
| // When the current field has an object type, we need to push its fields slice to the stack. | ||
| // However, we can do that only after the field, which we are currently creating, will be added to the parent object fields. | ||
| // So we defer this action to be executed right after the current field is added to the parent object fields slice. | ||
| // This is more simple than analyzing resolve.Node, because this object could be nested in a list. | ||
| v.Walker.DefferOnEnterField(func() { | ||
| v.currentFields = append(v.currentFields, objectFields{ | ||
| v.currentObjectFields = append(v.currentObjectFields, objectFields{ |
Contributor
There was a problem hiding this comment.
The DefferOnEnterField name is not the best. Not mentioning the "deffer" unrelated to the "Defer" feature, maybe better name would be PostEnterField or RunAfterEnterField.
ysmolski
reviewed
Apr 22, 2026
| @@ -329,6 +345,7 @@ func (c *pathBuilderVisitor) EnterDocument(operation, definition *ast.Document) | |||
|
|
|||
| c.fieldDependenciesForPlanners = make(map[int][]int) | |||
Contributor
There was a problem hiding this comment.
this field is not used anymore
ysmolski
reviewed
Apr 22, 2026
| return false | ||
| } | ||
|
|
||
| return slices.ContainsFunc(treeNodeChildren(node), func(child int) bool { |
Contributor
There was a problem hiding this comment.
For a field with many children and many defer groups, this could scan the same children list multiple times per field. Is it possible for some user to have many defer groups in a very big query? Will this linear search affect the performance of such queries?
ysmolski
reviewed
Apr 22, 2026
| @@ -675,19 +749,23 @@ func (c *pathBuilderVisitor) hasFieldsWaitingForDependency() bool { | |||
| // in case current field has @requires directive, and we were able to plan it - it means that all fields from requires selection set was planned before that. | |||
| // So we need to notify planner of current fieldRef about dependencies on those other fields | |||
| // we know where fields were planned, because we record planner id of each planned field | |||
Contributor
There was a problem hiding this comment.
// addFieldDependencies adds dependencies between planners based on the @requires directive.
// If the current field has a @requires directive and we were able to plan it, it means that all fields
// from the requires selection set were planned before it.
// Hence, we need to notify the planner of the current fieldRef about dependencies on those other fields.
// We know where fields were planned because we record the planner ID of each planned field.
ysmolski
reviewed
Apr 22, 2026
Comment on lines
790
to
799
| notified := slices.Contains(fetchConfiguration.dependsOnFetchIDs, plannerIdx) | ||
| if !notified { | ||
|
|
||
| fetchConfiguration.dependsOnFetchIDs = append(fetchConfiguration.dependsOnFetchIDs, plannerIdx) | ||
| // sort | ||
| slices.Sort(fetchConfiguration.dependsOnFetchIDs) | ||
| // remove consecutive duplicates | ||
| fetchConfiguration.dependsOnFetchIDs = slices.Compact(fetchConfiguration.dependsOnFetchIDs) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The notified check should already prevent duplicates being added. Or if you want to sort the result then you should sort at the exit.
ysmolski
reviewed
Apr 22, 2026
| // resolveDeferredAlias decides how to alias a deferred required field. | ||
| // Precondition: v.config.deferInfo != nil && v.isRootLevel(). | ||
| // | ||
| // Decision table: |
Contributor
There was a problem hiding this comment.
slight reformat
// - __internal_{fieldName} absent → addAlias
// - __internal_{fieldName} present, same scope → reuseFieldRef
// - __internal_{fieldName} present, diff scope, __internal_{deferID}_{fieldName} absent → addAlias, includeDeferID
// - __internal_{fieldName} present, diff scope, __internal_{deferID}_{fieldName} present → reuseFieldRef
ysmolski
reviewed
Apr 22, 2026
| return v.config.operation.StringValueContentString(val.Ref) | ||
| } | ||
| return "" | ||
| } |
Contributor
There was a problem hiding this comment.
Isn't the function below (from ast_field.go) doing the same?
func (d *Document) FieldInternalDeferID(fieldRef int) (id string, exists bool) {
directiveRef, exists := d.Fields[fieldRef].Directives.HasDirectiveByNameBytes(d, literal.DEFER_INTERNAL)
if !exists {
return "", false
}
idValue, exists := d.DirectiveArgumentValueByName(directiveRef, []byte("id"))
if !exists {
return "", false
}
return d.StringValueContentString(idValue.Ref), true
}
ysmolski
reviewed
Apr 22, 2026
| v.OperationNodes = append(v.OperationNodes, selectionSetNode) | ||
| } | ||
|
|
||
| func (v *requiredFieldsVisitor) fieldHasDeferInternal(fieldRef int) bool { |
Contributor
There was a problem hiding this comment.
Could you use Document.FieldInternalDeferID from ast instead introducing this method?
ysmolski
reviewed
Apr 22, 2026
| } | ||
|
|
||
| func (v *requiredFieldsVisitor) isRootLevel() bool { | ||
| return len(v.OperationNodes) == 1 |
Contributor
There was a problem hiding this comment.
i cannot grasp it... does it mean that we do not handle deeply nested @requires fields in deferred context?
ysmolski
reviewed
Apr 22, 2026
|
|
||
| // we are skipping adding __typename field to the required fields, | ||
| // because we want to depend only on the regular key fields, not the __typename field | ||
| if !bytes.Equal(fieldName, typeNameFieldBytes) || (bytes.Equal(fieldName, typeNameFieldBytes) && v.config.isTypeNameForEntityInterface) { |
Contributor
There was a problem hiding this comment.
noticed, that this check can be simplified to
if !bytes.Equal(fieldName, typeNameFieldBytes) || v.config.isTypeNameForEntityInterface {
ysmolski
reviewed
Apr 29, 2026
|
|
||
| // DisableCalculateFieldDependencies controls whether the planner calculates | ||
| // field dependencies at all. | ||
| DisableCalculateFieldDependencies bool |
ysmolski
reviewed
Apr 29, 2026
Comment on lines
+277
to
285
| c.handleFieldRequiredByRequires(fieldCtx) | ||
| // skip to the next suggestion as we only handle requires here | ||
| continue | ||
| } | ||
|
|
||
| if suggestion.requiresKey != nil { | ||
| // add @key requirements for the field | ||
| c.handleFieldsRequiredByKey(fieldRef, parentPath, typeName, fieldName, currentPath, ds, *suggestion.requiresKey) | ||
| c.handleFieldsRequiredByKey(fieldCtx, *suggestion.requiresKey) | ||
| } |
Contributor
There was a problem hiding this comment.
Nit: handleFieldRequiredByRequires and handleFieldsRequiredByKey - naming could be better. I suggest following: queueRequiresDependencies and queueKeyFieldsForJump
ysmolski
reviewed
Apr 29, 2026
Comment on lines
+525
to
+527
| if requirements.requirementConfigs[i].selectionSet == fieldConfiguration.SelectionSet && requirements.requirementConfigs[i].dsHash == fieldCtx.dsConfig.Hash() && requirements.requirementConfigs[i].isTypenameForEntityInterface == isTypenameForEntityInterface { | ||
| if slices.IndexFunc(requirements.requirementConfigs[i].requestedByFieldRefs, func(fieldRef int) bool { | ||
| return fieldRef == requestedByFieldRef | ||
| return fieldRef == fieldCtx.fieldRef |
Contributor
There was a problem hiding this comment.
this long if is not for humans, can you break it down into individual named conditions? and see my other comment
ysmolski
reviewed
Apr 29, 2026
| if fieldCtx.deferInfo != nil { | ||
| deferID = fieldCtx.deferInfo.ID | ||
| } | ||
| existsKey := pendingFieldRequirementExistsKey{fieldCtx.dsConfig.Hash(), fieldConfiguration.SelectionSet, isTypenameForEntityInterface, deferID} |
Contributor
There was a problem hiding this comment.
please do not format code like this one unreadable line
ysmolski
reviewed
Apr 29, 2026
| } else { | ||
| for i := range requirements.requirementConfigs { | ||
| if requirements.requirementConfigs[i].selectionSet == fieldConfiguration.SelectionSet && requirements.requirementConfigs[i].dsHash == dsHash && requirements.requirementConfigs[i].isTypenameForEntityInterface == isTypenameForEntityInterface { | ||
| if requirements.requirementConfigs[i].selectionSet == fieldConfiguration.SelectionSet && requirements.requirementConfigs[i].dsHash == fieldCtx.dsConfig.Hash() && requirements.requirementConfigs[i].isTypenameForEntityInterface == isTypenameForEntityInterface { |
Contributor
There was a problem hiding this comment.
In the code above (line 510) we already checked if we have a new field requirement. Why do we do another scan on line 524?
ysmolski
reviewed
Apr 30, 2026
| requirements.existsTracker[existsKey] = struct{}{} | ||
| requirements.requirementConfigs = append(requirements.requirementConfigs, config) | ||
| } else { | ||
| for i := range requirements.requirementConfigs { |
Contributor
There was a problem hiding this comment.
same as above, why do we iterate again even if we already could found the existing requirementConfigs?
ysmolski
reviewed
Apr 30, 2026
|
|
||
| // handleRequiredField is the EnterField entry point for @requires fields. | ||
| // It builds requiredFieldInfo and dispatches to the deferred or non-deferred path. | ||
| func (v *requiredFieldsVisitor) handleRequiredField(ref int) { |
Contributor
There was a problem hiding this comment.
There are many handle* methods in this visitor. I am not sure that the comment to this function is useful to the reader. It just names things happening in the code without telling the story of the method. How about this high level description?
"It ensures that this fragment field has a representation in the operation"
And I would suggest this rename:
handleRequiredFieldtoapplyRequiredFieldhandleRequiredFieldDeferredtoapplyRequiresFieldDeferredhandleRequiredFieldNonDeferredtoapplyRequiresFieldDirect
The same thought can be applied to the "key" methods.
ysmolski
reviewed
Apr 30, 2026
Comment on lines
+50
to
+51
| func (d *extractDeferFetches) fetchGroups(deferPlan *plan.DeferResponsePlan) (root []*resolve.FetchTreeNode, deffered map[string][]*resolve.FetchTreeNode) { | ||
| fetchGroups := make(map[string][]*resolve.FetchTreeNode) |
Contributor
There was a problem hiding this comment.
uhm, you named it in the return and then used completely different variable in the body. I suggest to use fetchGroups everywhere.
ysmolski
reviewed
Apr 30, 2026
|
|
||
| for _, fetch := range deferPlan.Response.Response.Fetches.ChildNodes { | ||
| deferID := fetch.Item.Fetch.Dependencies().DeferID | ||
| if deferID == "" { |
Contributor
There was a problem hiding this comment.
Could you explain what is the meaning of empty string in deferID?
ysmolski
reviewed
Apr 30, 2026
| }) | ||
|
|
||
| if deferID != "" { | ||
| r.operation.AddDeferInternalDirectiveToField(field.Ref, deferID, "", "") |
Contributor
There was a problem hiding this comment.
Please explain empty magic values.
ysmolski
reviewed
Apr 30, 2026
| } | ||
| } | ||
|
|
||
| writer.Complete() |
Contributor
There was a problem hiding this comment.
I wonder why we Complete only here and not ever when errors are returned?
ysmolski
reviewed
Apr 30, 2026
Comment on lines
+478
to
+481
| if t.resolvable.hasErrors() { | ||
| return resolveInfo, nil | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Why the error is not returned here and no .Complete too?
ysmolski
reviewed
Apr 30, 2026
| r.typeNames = r.typeNames[:len(r.typeNames)-1] | ||
| }() | ||
|
|
||
| if r.deferMode { |
Contributor
There was a problem hiding this comment.
Please inverse and bail early to flatten the if.
As I understood the code below, it does two things: collection and rendering at the same time, with mutable state like enableDeferRender, incrementalItemWritten, deferItemDataNull being toggled inside. That means at least 4 paths (initial render, defer render, defer null-envelope, seek/passthrough only). Correct me if I wrong.
All of it makes this code very complicated and error prone. I tried to think of ways to simplify it, but quickly failed.
ysmolski
reviewed
Apr 30, 2026
| return false | ||
| } | ||
|
|
||
| func (r *Resolvable) collectDeferFields(obj *Object) (deferFields map[int]struct{}, seekFields map[int]struct{}) { |
Contributor
There was a problem hiding this comment.
IMHO deferFields should be named as renderFields - fields we should render now, in the currect pass,
and seekFields are transitFields or passThroughFields - fields we have to go through to get renderable fields.
The callee site could reflect that. seek is not the right word the meaning, unless I have not grasped some other meanting.
ysmolski
reviewed
Apr 30, 2026
| if r.deferMode { | ||
| deferFields, seekFiels := r.collectDeferFields(obj) | ||
|
|
||
| if len(deferFields) > 0 { |
Contributor
There was a problem hiding this comment.
Please, inverse the len(deferFields) > 0 and bail early. The if after this one will lose 2nd comparison too.
ysmolski
reviewed
Apr 30, 2026
| return false | ||
| } | ||
|
|
||
| // skip array if it's item do not have an object kind |
Contributor
There was a problem hiding this comment.
// Skip array if its item type is not an object kind.
ysmolski
reviewed
Apr 30, 2026
|
|
||
| for i := range obj.Fields { | ||
| if filter.enabled { | ||
| // if mode is seek |
Contributor
There was a problem hiding this comment.
As my comment suggested above, it might be more readable to name it "passThrough" instead of "seek"
ysmolski
reviewed
Apr 30, 2026
| return false | ||
| } | ||
|
|
||
| func (r *Resolvable) shoulSkipObjectFieldByTypenames(field *Field) bool { |
Contributor
There was a problem hiding this comment.
shouldSkipObjectFieldByTypenames or even shouldSkipFieldByTypeCondition
ysmolski
requested changes
Apr 30, 2026
Contributor
ysmolski
left a comment
ysmolski
left a comment
There was a problem hiding this comment.
1st pass is done. Overall - well done job! I expected it to be more complicated. Although I have not understood in full some partos of the planner, especially those visitors with big state. But hey, I tried! :) This PR adds some more tech debt in the resolver. Maybe we could avoid it? See the relevant comment. And those bools to records the state are making the code hard to decipher.
As for other items, judge from the comments if it can be improved or just needs some clarification.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defer Support Implementation
This branch adds end-to-end support for the @defer directive in GraphQL execution.
pkg/astnormalization
new rules
updated rules
operation normalizer
The order of the rules is slightly changed
pkg/ast
Added defer-related helpers to ast_field.go, ast_directive.go, ast_argument.go, ast_inline_fragment.go
pkg/asttransform
Added internal defer directive definitions to the base schema and updated fixtures
pkg/engine/plan
plan.goDeferResponsePlantype andDeferResponsePlanKindconstantplanner_configuration.gopathConfigurationgetsdeferID; path index stores full config instead of empty struct; newDeferID()/PathWithFieldRef()accessorspath_builder_visitor.gocurrentFieldInfostruct instead of passing separate arguments;objectFetchConfigurationnow havedeferIDnode_selection_visitor.godeferInfo/parentFieldDeferID; deduplication key is now per defer scoperequired_fields_visitor.go@__defer_internalannotations to the fields to put them into the correct scope; newresolveDeferredAlias()helper is added to create proper defer aliasdatasource_filter_node_suggestions.goNodeSuggestiongetsDeferInfo; newProcessDefer()propagates defer IDs to nodes parents up to the root query node or root entity nodes which requires a key - e.g. to nearest query entry pointdatasource_filter_collect_nodes_visitor.go@__defer_internalduring tree building to populateDeferInfoon suggestionsabstract_selection_rewriter*.gotypeNameSelection()takes adeferIDto give an injected__typenamea proper scopenode_selection_builder.goProcessDefer()after process if node selection is finished - to propagate defer ids to parentsvisitor.goDeferResponsePlan; misc cleanupanalyze_plan_kind.gopkg/postprocess
postprocess.go
new processor extract_defer_fetches.go
Splits a flat DeferResponsePlan fetch tree into two buckets:
pkg/engine/postprocess
postprocess.go
new processor extract_defer_fetches.go
Splits a flat DeferResponsePlan fetch tree into two buckets: - fetches without a deferID stay in the root sequence; - fetches with a deferID are grouped by that ID into DeferFetchGroup entries on Response.Defers, sorted in natural numeric order.
pkg/engine/resolve
response.go, fetch.go, node_object.go, const.go
loader.go
resolvable.go
resolve.go
New entry point ResolveGraphQLDeferResponse() is added:
Integration tests
closes ENG-8799
closes ENG-7978
closes ENG-7976
Checklist