feat: defer additional fixes#1464
feat: defer additional fixes#1464devsergiy wants to merge 2 commits intofeat/eng-7770-add-defer-supportfrom
Conversation
devsergiy
requested review from
SkArchon and
StarpTech
and removed request for
a team
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Normalizer
participant Planner
participant PostProcessor
participant Executor
participant DataSource
Client->>Normalizer: Send query with `@defer`
Normalizer->>Planner: Produce normalized AST with defer IDs
Planner->>PostProcessor: Produce planned fetch trees grouped by DeferID
PostProcessor->>Executor: Emit primary plan + deferred groups
Executor->>DataSource: Execute primary fetches
DataSource-->>Executor: Primary data
Executor-->>Client: Primary response (hasNext: true/false)
loop For each deferred group (ordered)
Executor->>DataSource: Fetch deferred group (with DeferID)
DataSource-->>Executor: Incremental data/chunk
Executor-->>Client: Deferred chunk envelope (hasNext flag)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/defer-design.md`:
- Line 334: The markdown has heading level jumps: change the two headings
currently using "######" (the "Normal envelope (`deferItemDataNull=false`)"
heading and the similar heading in the incremental rendering subsection) to one
level higher "#####" so they sit under the existing "####" section and resolve
MD001; update both occurrences accordingly to maintain consistent heading
hierarchy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6482f06a-05fd-44f1-bef5-5c7bde984015
📒 Files selected for processing (1)
docs/defer-design.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/defer-design.md`:
- Around line 349-357: Update the "Null envelope (`deferItemDataNull=true`)"
description to match the wire-format: state that printDeferEnvelopeNullData
emits the null item ({"data": null, "path": [...], "errors": [...]}) inside the
standard {"incremental": [ ... ]} envelope rather than omitting the outer
wrapper; change the phrase "the outer `{\"incremental\": [` wrapper are never
written" to "the item is emitted inside the standard `{\"incremental\": [ ...
]}` chunk envelope and the `{\"data\": {` opener is skipped for that item," and
apply the same wording correction to the other occurrence referenced (lines
416-420).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: efeed565-a6d7-4b62-aead-60c60b77a0f2
📒 Files selected for processing (1)
docs/defer-design.md
| ##### Null envelope (`deferItemDataNull=true`) | ||
|
|
||
| `printDeferEnvelopeNullData` writes the entire item as | ||
| ```json | ||
| {"data": null, "path": [...], "errors": [...]} | ||
| ``` | ||
|
|
||
| in one shot — the normal `{"data": {` opener and the outer `{"incremental": [` wrapper are never written. The walker returns immediately without descending further. | ||
|
|
There was a problem hiding this comment.
Null-envelope description contradicts the documented wire format.
The null-envelope section says the outer {"incremental":[ ... ]} wrapper is never written, but the wire-format section documents null responses with that wrapper. Please align the rendering description to avoid implementer confusion.
📝 Proposed doc fix
-`printDeferEnvelopeNullData` writes the entire item as
+`printDeferEnvelopeNullData` writes the null-data item as
```json
{"data": null, "path": [...], "errors": [...]}-in one shot — the normal {"data": { opener and the outer {"incremental": [ wrapper are never written. The walker returns immediately without descending further.
+in one shot — the normal {"data": { opener is skipped for that item. The item is then emitted inside the standard {"incremental": [ ... ]} chunk envelope. The walker returns immediately without descending further.
</details>
Also applies to: 416-420
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/defer-design.md around lines 349 - 357, Update the "Null envelope
(deferItemDataNull=true)" description to match the wire-format: state that
printDeferEnvelopeNullData emits the null item ({"data": null, "path": [...],
"errors": [...]}) inside the standard {"incremental": [ ... ]} envelope rather
than omitting the outer wrapper; change the phrase "the outer {\"incremental\": [ wrapper are never written" to "the item is emitted inside the standard
{\"incremental\": [ ... ]} chunk envelope and the {\"data\": { opener is
skipped for that item," and apply the same wording correction to the other
occurrence referenced (lines 416-420).
</details>
<!-- fingerprinting:phantom:triton:hawk:21fc2da2-0188-40e2-a9bb-a36a5dd0647a -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| Resolves which datasource(s) handle each field. Also detects fields that require additional data to be fetched — `@key` fields for entity resolution and `@requires` fields for computed fields — and injects them directly into the operation AST in the correct defer scope. | ||
|
|
||
| **`@requires` fields** are stamped with the same `@__defer_internal` as the field that needs them. They must be present in the same deferred fetch so the field resolver has the data it depends on. |
There was a problem hiding this comment.
what if multiple fields have overlapping requires?
There was a problem hiding this comment.
Currently, for such schemas
graphql-go-tools/execution/engine/execution_engine_defer_test.go
Lines 746 to 799 in f59a327
Such a query with 2 fields requires selecting a different nested field in settings, but this fields are in the same defer scope
graphql-go-tools/execution/engine/execution_engine_defer_test.go
Lines 1366 to 1376 in f59a327
Current logic always ads an alias to the required fields in defer scope, but reuses already existing alias
So operation with added required fields will look like this
query DeferAllFields {
user {
name @__defer_internal(id: "1")
billing @__defer_internal(id: "1") {
plan @__defer_internal(id: "1")
}
settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
}
account @__defer_internal(id: "1") {
type @__defer_internal(id: "1")
}
notifications @__defer_internal(id: "1")
___typename: __typename
__internal_billing: billing @__defer_internal(id: "1") {
plan @__defer_internal(id: "1")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
language @__defer_internal(id: "1")
}
__internal_name: name @__defer_internal(id: "1")
__typename
id
}
}In case of such query, when the fields with requirements in different defer scopes
graphql-go-tools/execution/engine/execution_engine_defer_test.go
Lines 1512 to 1520 in f59a327
The query will look like this
query DeferDerivedFieldsOnly {
user {
name
billing {
plan
}
settings {
region
language
}
account @__defer_internal(id: "1") {
type @__defer_internal(id: "1")
}
notifications @__defer_internal(id: "2")
__internal_billing: billing @__defer_internal(id: "1") {
plan @__defer_internal(id: "1")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
}
__internal_name: name @__defer_internal(id: "2")
__internal_2_settings: settings @__defer_internal(id: "2") {
language @__defer_internal(id: "2")
}
__typename
id
}
}We always alias requirements because in situation like this
query {
user {
settings {
a
}
... @defer {
fieldRequiresSettingsB
fieldRequiresSettingsC
}
}
}We can't reuse settings, because it is not in the defer scope, and it will mean that we will fetch required fields with primary response, not via deffer group
There was a problem hiding this comment.
hmm, after writing this comment I realized that there could be small improvement
settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
language @__defer_internal(id: "1")
}In this case the existing field falls into the same defer scope, so we could actually reuse it
But in the case of such an operation
settings @__defer_internal(id: "1") {
anyField @__defer_internal(id: "1")
region @__defer_internal(id: "2")
}
__internal_settings: settings @__defer_internal(id: "1") {
region @__defer_internal(id: "1")
language @__defer_internal(id: "1")
}when needed region field is in the other defer scope, we will need to add an alias to this nested field, which currently is not implemented, as an alias is added only at the root of the requires selection set
Using aliases is a tradeoff to be able to properly distribute dependencies between fields, to assign them to a proper planner
In the last example region will be returned to the client only with defer group 2
Potentially, I could mark a field that should be returned later, but fetched earlier via the other fetch
| - `@defer` is removed from every fragment (inline or named spread) | ||
| - Every field inside is stamped with `@__defer_internal(id, parentDeferId, label)` | ||
| - defer IDs are assigned sequentially in AST walk order | ||
| - A `___typename` placeholder is injected into any selection set where all children are deferred |
There was a problem hiding this comment.
Again, as I said ___ vs __ might be not the best idea. Its very hard to distiungish them.
There was a problem hiding this comment.
per discussion will change it to internal__typename
|
|
||
| 1. Checks `@defer(if: false)` — if disabled, removes `@defer` from the fragment but does not stamp any fields (they are treated as non-deferred). | ||
| 2. Removes `@defer` from the fragment node itself. | ||
| 3. Assigns a sequential integer ID to this defer group. IDs are assigned in AST walk order, so they reflect the order in which `@defer` fragments appear in the document. |
There was a problem hiding this comment.
So IDs on internal defers are, in fact, defer groups?
There was a problem hiding this comment.
kinda
It means that a single deferID could produce multiple fetches, which are grouped in GraphQLDeferResponse by deferID, e.g. it is a separate fetch tree
…fer counter starting from index 1
3 participants
@coderabbitai summary
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.