feat: add Description support for VariableDefinition (September 2025 spec)#1457
feat: add Description support for VariableDefinition (September 2025 spec)#1457
Conversation
📝 WalkthroughWalkthroughAdds support for GraphQL variable definition descriptions: AST now stores descriptions, parser recognizes description tokens before variables, printer emits variable descriptions, JSON Schema builder sources descriptions from variables or corresponding argument definitions, and tests/fixtures updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Source as GraphQL Source
participant Parser as Parser
participant AST as AST Document
participant Printer as Printer
participant Output as Formatted Output
Source->>Parser: Raw GraphQL with variable descriptions
Parser->>Parser: parseVariableDefinitionList()
Parser->>Parser: detectDescription() → STRING / BLOCKSTRING
Parser->>Parser: parseVariableDefinitionWithDescription()
Parser->>AST: Create VariableDefinition with Description
AST->>Printer: Walk AST (EnterVariableDefinition)
Printer->>Printer: Check Description.IsDefined
rect rgba(100, 200, 100, 0.5)
Printer->>Printer: PrintDescription() if present
end
Printer->>Printer: Format variable name, type, default, directives
Printer->>Output: Emit formatted variable definition
sequenceDiagram
participant Ops as GraphQL Operation
participant SchemaBuilder as Schema Builder
participant Schema as GraphQL Schema
participant JSONSchema as JSON Schema
Ops->>SchemaBuilder: Process VariableDefinition
SchemaBuilder->>SchemaBuilder: Check variable Description.IsDefined
rect rgba(100, 150, 200, 0.5)
alt Variable has description
SchemaBuilder->>JSONSchema: Use variable description
else Variable lacks description
SchemaBuilder->>Ops: findArgumentDescriptionForVariable(varName)
Ops->>SchemaBuilder: Return matched argument reference
SchemaBuilder->>Schema: Resolve argument definition
Schema-->>SchemaBuilder: Return argument description (if any)
SchemaBuilder->>JSONSchema: Use argument description (if present)
end
end
JSONSchema->>JSONSchema: Populate property description
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
baf9ecb to
6b83257
Compare
…spec) Implements Description? on VariableDefinition per the September 2025 GraphQL spec (graphql-spec PR #1170), following the same pattern established in PR #1331 for OperationDefinition and FragmentDefinition. - Parser: handle optional string/block-string description before $Variable - Printer: output variable descriptions when present - JSON Schema Builder: propagate variable descriptions to JSON Schema property description fields, with fallback to argument descriptions from the schema definition when no variable description is provided Resolves ENG-9301
6b83257 to
5c581bb
Compare
… ahmet/eng-9301-september-2025-spec-add-description-support-for
…cription-support-for
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.
|
@claude review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
v2/pkg/astparser/parser.go (1)
1560-1570: Minor duplication inlist.Refsinitialization can be collapsed.Both branches initialize
list.Refstwice around parse calls. A small helper/guard would reduce noise and keep both paths consistent.Also applies to: 1572-1579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/astparser/parser.go` around lines 1560 - 1570, The duplicated initialization of list.Refs around the parse calls should be collapsed: ensure list.Refs is initialized once before calling p.parseDescription() / p.parseVariableDefinitionWithDescription by extracting the guard into a small helper or moving the cap(list.Refs) check (using p.document.Refs[p.document.NextRefIndex()][:0]) to a single pre-check. Update the branches that handle keyword.STRING/keyword.BLOCKSTRING (and the similar block at the other occurrence) to call that single-init helper or perform the one-time initialization before invoking p.parseDescription() and p.parseVariableDefinitionWithDescription so list.Refs is only set once.v2/pkg/astprinter/astprinter_test.go (1)
790-819: Consider adding onerunIndentcase for variable descriptions.These new cases validate compact output well; adding an indented-mode assertion would harden formatting behavior for multiline/block descriptions too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/astprinter/astprinter_test.go` around lines 790 - 819, Add a complementary indented-mode test by calling runIndent in the same test block suite (e.g. alongside the "variable with block string description" case) to assert indented formatting for variable descriptions; use the same input used in that case and provide the expected pretty-printed, multi-line indented output as the second argument to runIndent so the test verifies block-string and multiline variable descriptions in indented mode (refer to the existing run and runIndent test helpers and the "variable with block string description" test name to locate where to add it).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v2/pkg/ast/ast_variable_definition.go`:
- Line 21: The import path that reconstructs VariableDefinition instances omits
the new Description field, causing metadata loss; update the reconstruction
logic in the import code (the block in astimport.go that builds
VariableDefinition objects around the 162-180 area) to copy the source
Description into the target VariableDefinition.Description when
creating/importing variables, and ensure any helper like ImportVariable or
ImportOperation propagation also preserves Description; add/update a unit test
that imports an operation with a variable Description and asserts the imported
VariableDefinition.Description is preserved.
In `@v2/pkg/engine/jsonschema/variables_schema.go`:
- Around line 194-238: The current fallback only checks root selection-set
fields and direct ValueKindVariable arguments, missing variables in nested
fields, inline fragments, fragment spreads, directives, and wrapped values;
update the logic that starts at operationDef.SelectionSet to recursively
traverse the full selection tree (follow SelectionSetRefs, resolve
FragmentSpread refs via v.operationDocument.FragmentDefinitions, handle
InlineFragment selections, and descend into nested field selections) and when
inspecting arguments use ArgumentValue to recursively unwrap List/Object values
to find any nested Variable kinds (using VariableValueNameString); for each
located variable, resolve its argument definition by locating the exact field's
definition based on the parent type (not just rootType.Ref) via
v.definitionDocument.ObjectTypeDefinitions and
FieldDefinitionArgumentsDefinitions and return
InputValueDefinitionDescriptionString when found.
---
Nitpick comments:
In `@v2/pkg/astparser/parser.go`:
- Around line 1560-1570: The duplicated initialization of list.Refs around the
parse calls should be collapsed: ensure list.Refs is initialized once before
calling p.parseDescription() / p.parseVariableDefinitionWithDescription by
extracting the guard into a small helper or moving the cap(list.Refs) check
(using p.document.Refs[p.document.NextRefIndex()][:0]) to a single pre-check.
Update the branches that handle keyword.STRING/keyword.BLOCKSTRING (and the
similar block at the other occurrence) to call that single-init helper or
perform the one-time initialization before invoking p.parseDescription() and
p.parseVariableDefinitionWithDescription so list.Refs is only set once.
In `@v2/pkg/astprinter/astprinter_test.go`:
- Around line 790-819: Add a complementary indented-mode test by calling
runIndent in the same test block suite (e.g. alongside the "variable with block
string description" case) to assert indented formatting for variable
descriptions; use the same input used in that case and provide the expected
pretty-printed, multi-line indented output as the second argument to runIndent
so the test verifies block-string and multiline variable descriptions in
indented mode (refer to the existing run and runIndent test helpers and the
"variable with block string description" test name to locate where to add it).
🪄 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: 63defb37-2976-4475-892f-7d83e80b0ea1
📒 Files selected for processing (8)
v2/pkg/ast/ast_variable_definition.gov2/pkg/astparser/parser.gov2/pkg/astparser/parser_test.gov2/pkg/astparser/testdata/variable_with_description.graphqlv2/pkg/astprinter/astprinter.gov2/pkg/astprinter/astprinter_test.gov2/pkg/engine/jsonschema/variables_schema.gov2/pkg/engine/jsonschema/variables_schema_test.go
| // Iterate through root fields in the operation's selection set | ||
| selectionSetRef := operationDef.SelectionSet | ||
| for _, selectionRef := range v.operationDocument.SelectionSets[selectionSetRef].SelectionRefs { | ||
| selection := v.operationDocument.Selections[selectionRef] | ||
| if selection.Kind != ast.SelectionKindField { | ||
| continue | ||
| } | ||
|
|
||
| fieldRef := selection.Ref | ||
| // Check if this field has arguments referencing our variable | ||
| if !v.operationDocument.FieldHasArguments(fieldRef) { | ||
| continue | ||
| } | ||
|
|
||
| for _, argRef := range v.operationDocument.Fields[fieldRef].Arguments.Refs { | ||
| argValue := v.operationDocument.ArgumentValue(argRef) | ||
| if argValue.Kind != ast.ValueKindVariable { | ||
| continue | ||
| } | ||
| argVarName := v.operationDocument.VariableValueNameString(argValue.Ref) | ||
| if argVarName != varName { | ||
| continue | ||
| } | ||
|
|
||
| // Found the argument that references this variable. | ||
| // Look up the corresponding argument definition in the schema. | ||
| argName := v.operationDocument.ArgumentNameString(argRef) | ||
| fieldName := v.operationDocument.FieldNameString(fieldRef) | ||
|
|
||
| // Find this field in the root type definition | ||
| for _, fieldDefRef := range v.definitionDocument.ObjectTypeDefinitions[rootType.Ref].FieldsDefinition.Refs { | ||
| if v.definitionDocument.FieldDefinitionNameString(fieldDefRef) != fieldName { | ||
| continue | ||
| } | ||
| // Find the argument definition | ||
| for _, argDefRef := range v.definitionDocument.FieldDefinitionArgumentsDefinitions(fieldDefRef) { | ||
| if v.definitionDocument.InputValueDefinitionNameString(argDefRef) == argName { | ||
| if v.definitionDocument.InputValueDefinitions[argDefRef].Description.IsDefined { | ||
| return v.definitionDocument.InputValueDefinitionDescriptionString(argDefRef) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fallback lookup misses valid variable usages outside root field arguments.
At Line 194, lookup only scans root selection-set fields and only direct argument values (ValueKindVariable). Variables used in nested fields, fragment spreads/inline fragments, directives, or wrapped structures won’t get fallback descriptions, so AC3 is only partially satisfied.
Please expand traversal to cover the full operation selection tree (including fragments) and resolve argument definitions in that context.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@v2/pkg/engine/jsonschema/variables_schema.go` around lines 194 - 238, The
current fallback only checks root selection-set fields and direct
ValueKindVariable arguments, missing variables in nested fields, inline
fragments, fragment spreads, directives, and wrapped values; update the logic
that starts at operationDef.SelectionSet to recursively traverse the full
selection tree (follow SelectionSetRefs, resolve FragmentSpread refs via
v.operationDocument.FragmentDefinitions, handle InlineFragment selections, and
descend into nested field selections) and when inspecting arguments use
ArgumentValue to recursively unwrap List/Object values to find any nested
Variable kinds (using VariableValueNameString); for each located variable,
resolve its argument definition by locating the exact field's definition based
on the parent type (not just rootType.Ref) via
v.definitionDocument.ObjectTypeDefinitions and
FieldDefinitionArgumentsDefinitions and return
InputValueDefinitionDescriptionString when found.
There was a problem hiding this comment.
potentially it is right, in case you are not normalizing operation before applying this
There was a problem hiding this comment.
🧩 Analysis chain
Good point — let me check whether normalization is always applied to the operation before this builder runs.
🏁 Script executed:
#!/bin/bash
# Find all call sites of BuildJsonSchema and NewVariablesSchemaBuilder
rg -n "BuildJsonSchema\|NewVariablesSchemaBuilder\|NewVariablesSchemaBuilderWithOptions" --type go -C 5
Length of output: 0
🏁 Script executed:
#!/bin/bash
rg -n -e "BuildJsonSchema" -e "NewVariablesSchemaBuilder" --type go -C 6
Length of output: 29903
🏁 Script executed:
#!/bin/bash
# Search for external callers of the jsonschema package
rg -n "jsonschema\." --type go -g '!*variables_schema*' -C 4
# Also look for normalization happening before schema building in the engine
rg -n "Normalize\|astnormalization" --type go -C 3 | head -80
Length of output: 1801
🏁 Script executed:
#!/bin/bash
# Find files importing the engine/jsonschema package
rg -n "engine/jsonschema" --type go -C 3
# Also check where normalization is applied relative to JSON schema building
rg -n "NormalizeOperation\|NormalizeNamedOperation\|astnormalization\.NewNormalizer" --type go -C 3 | head -100
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if variables_schema.go is a new file (git status)
git log --oneline -5 -- v2/pkg/engine/jsonschema/variables_schema.go
# Search for Normalize usage
rg -n "Normalize" --type go -l | head -20
# Check if there's a normalization step documented in the package or any README
fd "README" --type f | head -10
Length of output: 1175
🏁 Script executed:
#!/bin/bash
# Check how the jsonschema package is referenced across the repo
rg -rn "variables_schema\|VariablesSchema\|BuildJson" --type go -l
# Look at the MCP-related code to see if normalization happens before BuildJsonSchema
rg -rn "BuildJsonSchema\|VariablesSchemaBuilder" --type go
# Check normalization in planner.go context
rg -n "Normalize" v2/pkg/engine/plan/planner.go -C 3 | head -50
Length of output: 184
@devsergiy — that's a sharp observation. From inspecting the codebase, BuildJsonSchema/VariablesSchemaBuilder currently have no external callers outside their own test file, and those tests call it on a raw parsed (non-normalized) ast.Document. So the contract of "normalized vs. not" is currently undefined.
You're right that if normalization is guaranteed to run first, the limitation largely goes away — fragment spreads would be inlined, nested selections would be flattened to root fields, and the root-selection-set scan in findArgumentDescriptionForVariable would be sufficient for all practical cases.
However, a few nuances are worth noting regardless of normalization:
-
AC2 (variable's own description) is unaffected — it reads directly from
VariableDefinitions[ref].Description, so it works correctly whether or not the operation is normalized. -
AC3 (schema argument description fallback) is the impacted path — if an operation is not normalized before this builder runs, variables in nested fields or referenced via fragment spreads won't receive the fallback description from the schema. This is the scenario the original comment flagged.
-
Normalization doesn't fully eliminate the concern — even after normalization, a variable passed only to a nested (non-root) field argument wouldn't be matched by the current root-only scan, since normalization inlines fragments but doesn't move all selections to depth-1.
Would it make sense to document the normalization precondition explicitly (e.g., in the BuildJsonSchema godoc), so callers know what to expect for AC3? Alternatively, if this is always expected to be called post-normalization in practice, the original comment can be treated as a known, acceptable limitation scoped to best-effort fallback behaviour.
🧠 Learnings used
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1456
File: v2/pkg/engine/plan/cost.go:657-672
Timestamp: 2026-03-24T09:11:24.567Z
Learning: In wundergraph/graphql-go-tools (v2/pkg/engine/plan/cost.go), the execution engine converts all inline literal argument values into variables before cost validation runs. Therefore, in `validateSliceArguments`, it is correct to only check `arg.hasVariable` when counting satisfied slicing arguments — there is no need to additionally handle `arg.intValue > 0` for literal values. Suggesting such a change is a false positive for this codebase.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1341
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor_federation.go:406-429
Timestamp: 2025-11-19T09:42:17.644Z
Learning: In the wundergraph/graphql-go-tools gRPC datasource implementation (v2/pkg/engine/datasource/grpc_datasource), field resolvers must have arguments. The system does not currently support defining field resolvers without arguments. This invariant ensures that the `parentCallID` increment in `enterFieldResolver` is always matched by a decrement in `LeaveField` (which checks `r.operation.FieldHasArguments(ref)`).
Learnt from: ysmolski
Repo: wundergraph/graphql-go-tools PR: 1456
File: v2/pkg/engine/plan/cost.go:245-255
Timestamp: 2026-03-24T08:45:07.824Z
Learning: In wundergraph/graphql-go-tools (v2/pkg/engine/plan/cost.go), when validating `RequireOneSlicingArgument` for abstract fields, the intended behavior is: if ANY implementing concrete type has `RequireOneSlicingArgument=true`, the operation must satisfy the slicing argument constraint. The `requiringOneArgImplementingField` method intentionally returns the first matching implementing type's `FieldListSize` to trigger this strict validation. An operation cannot be considered valid for an abstract field if any concrete resolution would fail the slicing argument requirement.
Learnt from: Noroth
Repo: wundergraph/graphql-go-tools PR: 1363
File: v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go:223-238
Timestamp: 2026-01-22T13:32:05.068Z
Learning: In v2/pkg/engine/datasource/grpc_datasource/execution_plan_visitor.go, ensure that inline fragments do not generate dedicated messages. Treat inline fragments as meta information about potentially selectable types (Oneof in protobuf). When EnterSelectionSet visits a field with an interface/union type, apply meta information directly and do not traverse a second selection for the inline fragment. Inline fragments should be handled via the parent field’s message (ResolveInlineFragment path) and added to FieldSelectionSet in EnterField; nested structure is managed by the parent, not by separate fragment messages.
There was a problem hiding this comment.
could you also add a test with a single line operation without line breaks?
| list.Refs = p.document.Refs[p.document.NextRefIndex()][:0] | ||
| } | ||
| ref := p.parseVariableDefinitionWithDescription(&description) | ||
| if cap(list.Refs) == 0 { |
There was a problem hiding this comment.
You don't have to do it twice
| """ | ||
| fragment UserFields on User {id name}`) | ||
| }) | ||
| t.Run("variable with single-line description", func(t *testing.T) { |
There was a problem hiding this comment.
could you scope it to t.Run("variables descriptions")
| // Iterate through root fields in the operation's selection set | ||
| selectionSetRef := operationDef.SelectionSet | ||
| for _, selectionRef := range v.operationDocument.SelectionSets[selectionSetRef].SelectionRefs { | ||
| selection := v.operationDocument.Selections[selectionRef] | ||
| if selection.Kind != ast.SelectionKindField { | ||
| continue | ||
| } | ||
|
|
||
| fieldRef := selection.Ref | ||
| // Check if this field has arguments referencing our variable | ||
| if !v.operationDocument.FieldHasArguments(fieldRef) { | ||
| continue | ||
| } | ||
|
|
||
| for _, argRef := range v.operationDocument.Fields[fieldRef].Arguments.Refs { | ||
| argValue := v.operationDocument.ArgumentValue(argRef) | ||
| if argValue.Kind != ast.ValueKindVariable { | ||
| continue | ||
| } | ||
| argVarName := v.operationDocument.VariableValueNameString(argValue.Ref) | ||
| if argVarName != varName { | ||
| continue | ||
| } | ||
|
|
||
| // Found the argument that references this variable. | ||
| // Look up the corresponding argument definition in the schema. | ||
| argName := v.operationDocument.ArgumentNameString(argRef) | ||
| fieldName := v.operationDocument.FieldNameString(fieldRef) | ||
|
|
||
| // Find this field in the root type definition | ||
| for _, fieldDefRef := range v.definitionDocument.ObjectTypeDefinitions[rootType.Ref].FieldsDefinition.Refs { | ||
| if v.definitionDocument.FieldDefinitionNameString(fieldDefRef) != fieldName { | ||
| continue | ||
| } | ||
| // Find the argument definition | ||
| for _, argDefRef := range v.definitionDocument.FieldDefinitionArgumentsDefinitions(fieldDefRef) { | ||
| if v.definitionDocument.InputValueDefinitionNameString(argDefRef) == argName { | ||
| if v.definitionDocument.InputValueDefinitions[argDefRef].Description.IsDefined { | ||
| return v.definitionDocument.InputValueDefinitionDescriptionString(argDefRef) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
potentially it is right, in case you are not normalizing operation before applying this
| assert.JSONEq(t, expectedJSON, string(data), "JSON schema does not match expected structure") | ||
| }) | ||
|
|
||
| t.Run("variable with description propagated to JSON schema", func(t *testing.T) { |
There was a problem hiding this comment.
per coderabbit comment it makes sense to add a test for that
an example
https://graphql.org/learn/queries/#using-variables-inside-fragments
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
v2/pkg/astimport/astimport.go (1)
165-210:⚠️ Potential issue | 🟡 MinorPreserve
Description.IsBlockStringwhen importing.
ImportDescriptionrebuilds viato.ImportDescription(descStr), which infers block-string mode from content. This can flip a single-line block-string description into a normal string during import.💡 Proposed fix
func (i *Importer) ImportDescription(description ast.Description, from, to *ast.Document) ast.Description { if !description.IsDefined { return ast.Description{} } - descStr := from.Input.ByteSliceString(description.Content) - return to.ImportDescription(descStr) + return ast.Description{ + IsDefined: true, + IsBlockString: description.IsBlockString, + Content: to.Input.AppendInputBytes(from.Input.ByteSlice(description.Content)), + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v2/pkg/astimport/astimport.go` around lines 165 - 210, ImportDescription currently rebuilds the description via to.ImportDescription(descStr) which can flip Description.IsBlockString; change Importer.ImportDescription so after obtaining descStr := from.Input.ByteSliceString(description.Content) and calling desc := to.ImportDescription(descStr) you explicitly set desc.IsBlockString = description.IsBlockString (and preserve description.IsDefined) before returning desc, ensuring the original IsBlockString flag is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@v2/pkg/astimport/astimport.go`:
- Around line 165-210: ImportDescription currently rebuilds the description via
to.ImportDescription(descStr) which can flip Description.IsBlockString; change
Importer.ImportDescription so after obtaining descStr :=
from.Input.ByteSliceString(description.Content) and calling desc :=
to.ImportDescription(descStr) you explicitly set desc.IsBlockString =
description.IsBlockString (and preserve description.IsDefined) before returning
desc, ensuring the original IsBlockString flag is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a941a9c5-1676-45f8-bbf7-10572d31e095
📒 Files selected for processing (6)
v2/pkg/astimport/astimport.gov2/pkg/astimport/astimport_test.gov2/pkg/astparser/parser.gov2/pkg/astparser/parser_test.gov2/pkg/astparser/testdata/variable_with_description.graphqlv2/pkg/astprinter/astprinter_test.go
✅ Files skipped from review due to trivial changes (1)
- v2/pkg/astparser/testdata/variable_with_description.graphql
Implements
Description?onVariableDefinitionper the September 2025 GraphQL spec (graphql-spec #1170). Follows the same pattern as #1331 (OperationDefinition/FragmentDefinition).$Variabledescription, with fallback to schema argument descriptionResolves ENG-9301