Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 13 additions & 1 deletion v2/pkg/ast/ast_variable_definition.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ type VariableDefinitionList struct {

// VariableDefinition
// example:
// $devicePicSize: Int = 100 @small
// "The device picture size" $devicePicSize: Int = 100 @small
type VariableDefinition struct {
Description Description // optional, describes the variable (September 2025 spec)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
VariableValue Value // $ Name
Colon position.Position // :
Type int // e.g. String
Expand Down Expand Up @@ -93,3 +94,14 @@ func (d *Document) VariablePathByArgumentRefAndArgumentPath(argumentRef int, arg
// The variable path should be the variable name, e.g., "a", and then the 2nd element from the path onwards
return append([]string{string(variableNameBytes)}, argumentPath[1:]...), nil
}

func (d *Document) VariableDefinitionDescriptionBytes(ref int) ByteSlice {
if !d.VariableDefinitions[ref].Description.IsDefined {
return nil
}
return d.Input.ByteSlice(d.VariableDefinitions[ref].Description.Content)
}

func (d *Document) VariableDefinitionDescriptionString(ref int) string {
return unsafebytes.BytesToString(d.VariableDefinitionDescriptionBytes(ref))
}
10 changes: 10 additions & 0 deletions v2/pkg/astimport/astimport.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ func (i *Importer) ImportArguments(refs []int, from, to *ast.Document) []int {
func (i *Importer) ImportVariableDefinition(ref int, from, to *ast.Document) int {

variableDefinition := ast.VariableDefinition{
Description: i.ImportDescription(from.VariableDefinitions[ref].Description, from, to),
VariableValue: i.ImportValue(from.VariableDefinitions[ref].VariableValue, from, to),
Type: i.ImportType(from.VariableDefinitions[ref].Type, from, to),
DefaultValue: ast.DefaultValue{
Expand All @@ -182,6 +183,7 @@ func (i *Importer) ImportVariableDefinition(ref int, from, to *ast.Document) int
func (i *Importer) ImportVariableDefinitionWithRename(ref int, from, to *ast.Document, renameTo string) int {

variableDefinition := ast.VariableDefinition{
Description: i.ImportDescription(from.VariableDefinitions[ref].Description, from, to),
VariableValue: i.ImportValue(from.VariableDefinitions[ref].VariableValue, from, to),
Type: i.ImportTypeWithRename(from.VariableDefinitions[ref].Type, from, to, renameTo),
DefaultValue: ast.DefaultValue{
Expand All @@ -199,6 +201,14 @@ func (i *Importer) ImportVariableDefinitionWithRename(ref int, from, to *ast.Doc
return len(to.VariableDefinitions) - 1
}

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)
}

func (i *Importer) ImportVariableDefinitions(refs []int, from, to *ast.Document) []int {
definitions := make([]int, len(refs))
for j, k := range refs {
Expand Down
28 changes: 28 additions & 0 deletions v2/pkg/astimport/astimport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -318,3 +318,31 @@ func TestImporter_ImportVariableDefinitions(t *testing.T) {
[]int{0, 1},
))
}

func TestImportVariableDefinitionWithDescription(t *testing.T) {
from, report := astparser.ParseGraphqlDocumentString(`
query FindEmployee(
"The unique employee identifier"
$id: ID!
) {
employee(id: $id) { id }
}
`)
require.False(t, report.HasErrors())

to := ast.NewSmallDocument()

importer := Importer{}

// Import the variable definition from the source document
opDef := from.OperationDefinitions[0]
require.Len(t, opDef.VariableDefinitions.Refs, 1)

srcVarRef := opDef.VariableDefinitions.Refs[0]
importedRef := importer.ImportVariableDefinition(srcVarRef, &from, to)

// Verify the description was preserved
importedVar := to.VariableDefinitions[importedRef]
assert.True(t, importedVar.Description.IsDefined, "imported variable should have description")
assert.Equal(t, "The unique employee identifier", to.Input.ByteSliceString(importedVar.Description.Content))
}
18 changes: 17 additions & 1 deletion v2/pkg/astparser/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -1557,6 +1557,14 @@ func (p *Parser) parseVariableDefinitionList() (list ast.VariableDefinitionList)
case keyword.RPAREN:
list.RPAREN = p.read().TextPosition
return
case keyword.STRING, keyword.BLOCKSTRING:
// Description before variable definition (September 2025 spec)
description := p.parseDescription()
if cap(list.Refs) == 0 {
list.Refs = p.document.Refs[p.document.NextRefIndex()][:0]
}
ref := p.parseVariableDefinitionWithDescription(&description)
list.Refs = append(list.Refs, ref)
case keyword.DOLLAR:
if cap(list.Refs) == 0 {
list.Refs = p.document.Refs[p.document.NextRefIndex()][:0]
Expand All @@ -1567,7 +1575,7 @@ func (p *Parser) parseVariableDefinitionList() (list ast.VariableDefinitionList)
}
list.Refs = append(list.Refs, ref)
default:
p.errUnexpectedToken(p.read(), keyword.RPAREN, keyword.DOLLAR)
p.errUnexpectedToken(p.read(), keyword.RPAREN, keyword.DOLLAR, keyword.STRING, keyword.BLOCKSTRING)
return
}

Expand All @@ -1578,9 +1586,17 @@ func (p *Parser) parseVariableDefinitionList() (list ast.VariableDefinitionList)
}

func (p *Parser) parseVariableDefinition() int {
return p.parseVariableDefinitionWithDescription(nil)
}

func (p *Parser) parseVariableDefinitionWithDescription(description *ast.Description) int {

var variableDefinition ast.VariableDefinition

if description != nil {
variableDefinition.Description = *description
}

variableDefinition.VariableValue.Kind = ast.ValueKindVariable
variableDefinition.VariableValue.Ref, variableDefinition.VariableValue.Position = p.parseVariableValue()

Expand Down
45 changes: 45 additions & 0 deletions v2/pkg/astparser/parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2986,6 +2986,51 @@ func TestParseFragmentsWithDescriptions(t *testing.T) {
assert.False(t, userWithoutDescFrag.Description.IsDefined, "UserWithoutDescription should not have description")
}

func TestParseVariablesWithDescriptions(t *testing.T) {
operationsFile, err := os.ReadFile("./testdata/variable_with_description.graphql")
if err != nil {
t.Fatal(err)
}

doc, report := ParseGraphqlDocumentBytes(operationsFile)
if report.HasErrors() {
t.Fatal(report)
}

// Verify we parsed both operations
require.Len(t, doc.OperationDefinitions, 2)

// First operation: multi-line with variable descriptions
opDef := doc.OperationDefinitions[0]
require.True(t, opDef.HasVariableDefinitions)
require.Len(t, opDef.VariableDefinitions.Refs, 3)

// First variable: $id with single-line description
var1 := doc.VariableDefinitions[opDef.VariableDefinitions.Refs[0]]
assert.True(t, var1.Description.IsDefined, "$id should have description")
assert.False(t, var1.Description.IsBlockString, "$id description should be single-line")
assert.Equal(t, "The unique employee identifier", doc.Input.ByteSliceString(var1.Description.Content))

// Second variable: $department with block string description
var2 := doc.VariableDefinitions[opDef.VariableDefinitions.Refs[1]]
assert.True(t, var2.Description.IsDefined, "$department should have description")
assert.True(t, var2.Description.IsBlockString, "$department description should be block string")
assert.Contains(t, doc.Input.ByteSliceString(var2.Description.Content), "The department to filter by")

// Third variable: $limit without description (backward compatibility)
var3 := doc.VariableDefinitions[opDef.VariableDefinitions.Refs[2]]
assert.False(t, var3.Description.IsDefined, "$limit should not have description")

// Second operation: single-line with variable description
opDef2 := doc.OperationDefinitions[1]
require.True(t, opDef2.HasVariableDefinitions)
require.Len(t, opDef2.VariableDefinitions.Refs, 1)

singleLineVar := doc.VariableDefinitions[opDef2.VariableDefinitions.Refs[0]]
assert.True(t, singleLineVar.Description.IsDefined, "single-line $id should have description")
assert.Equal(t, "The user ID", doc.Input.ByteSliceString(singleLineVar.Description.Content))
}

func BenchmarkParseStarwars(b *testing.B) {

inputFileName := "./testdata/starwars.schema.graphql"
Expand Down
20 changes: 20 additions & 0 deletions v2/pkg/astparser/testdata/variable_with_description.graphql
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you also add a test with a single line operation without line breaks?

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
"""
Get an employee by their ID
"""
query FindEmployee(
"The unique employee identifier"
$id: ID!
"""
The department to filter by.
Optional parameter for narrowing results.
"""
$department: String
$limit: Int = 10
) {
employee(id: $id) {
id
name
}
}

query SingleLine("The user ID" $id: ID!) { user(id: $id) { id } }
5 changes: 5 additions & 0 deletions v2/pkg/astprinter/astprinter.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,11 @@ func (p *printVisitor) EnterVariableDefinition(ref int) {
p.write(literal.LPAREN)
}

if p.document.VariableDefinitions[ref].Description.IsDefined {
p.must(p.document.PrintDescription(p.document.VariableDefinitions[ref].Description, nil, 0, p.out))
p.write(literal.SPACE)
}

p.must(p.document.PrintValue(p.document.VariableDefinitions[ref].VariableValue, p.out))
p.write(literal.COLON)
p.write(literal.SPACE)
Expand Down
36 changes: 36 additions & 0 deletions v2/pkg/astprinter/astprinter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -787,6 +787,42 @@ User fields fragment
"""
fragment UserFields on User {id name}`)
})
t.Run("variable descriptions", func(t *testing.T) {
t.Run("single-line description", func(t *testing.T) {
run(t, `query GetUser("The user ID" $id: ID!) {
user(id: $id) {
id
}
}`, `query GetUser("The user ID" $id: ID!){user(id: $id){id}}`)
})
t.Run("block string description", func(t *testing.T) {
run(t, `query GetUser("""The unique identifier""" $id: ID!) {
user(id: $id) {
id
}
}`, `query GetUser("""
The unique identifier
""" $id: ID!){user(id: $id){id}}`)
})
t.Run("multiple variables with mixed descriptions", func(t *testing.T) {
run(t, `query Search("The search query" $query: String!, $limit: Int) {
search(query: $query, limit: $limit) {
id
}
}`, `query Search("The search query" $query: String!, $limit: Int){search(query: $query, limit: $limit){id}}`)
})
t.Run("without description unchanged", func(t *testing.T) {
run(t, `query GetUser($id: ID!) {
user(id: $id) {
id
}
}`, `query GetUser($id: ID!){user(id: $id){id}}`)
})
t.Run("single-line operation with variable description", func(t *testing.T) {
run(t, `query GetUser("The user ID" $id: ID!) { user(id: $id) { id } }`,
`query GetUser("The user ID" $id: ID!){user(id: $id){id}}`)
})
})
}

func TestPrintArgumentWithBeforeAfterValue(t *testing.T) {
Expand Down
90 changes: 90 additions & 0 deletions v2/pkg/engine/jsonschema/variables_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,17 @@ func (v *VariablesSchemaBuilder) EnterVariableDefinition(ref int) {
v.schema.Required = append(v.schema.Required, varName)
}

// Set variable description: use the variable's own description if present (AC2),
// otherwise fall back to the field argument description from the schema (AC3)
if v.operationDocument.VariableDefinitions[ref].Description.IsDefined {
varSchema.Description = v.operationDocument.VariableDefinitionDescriptionString(ref)
} else {
// Fall back to argument description from schema definition
if desc := v.findArgumentDescriptionForVariable(varName); desc != "" {
varSchema.Description = desc
}
}

// Set default value if exists
if v.operationDocument.VariableDefinitionHasDefaultValue(ref) {
defaultValue := v.operationDocument.VariableDefinitionDefaultValue(ref)
Expand All @@ -150,6 +161,85 @@ func (v *VariablesSchemaBuilder) EnterVariableDefinition(ref int) {
v.schema.Properties[varName] = varSchema
}

// findArgumentDescriptionForVariable looks up the argument description from the schema definition
// for a variable by matching it to field arguments in the operation's root selection set.
func (v *VariablesSchemaBuilder) findArgumentDescriptionForVariable(varName string) string {
if len(v.operationDocument.OperationDefinitions) == 0 {
return ""
}

operationDef := v.operationDocument.OperationDefinitions[0]
if !operationDef.HasSelections {
return ""
}

// Determine root type name
var rootTypeName string
switch operationDef.OperationType {
case ast.OperationTypeQuery:
rootTypeName = "Query"
case ast.OperationTypeMutation:
rootTypeName = "Mutation"
case ast.OperationTypeSubscription:
rootTypeName = "Subscription"
default:
return ""
}

rootType, exists := v.definitionDocument.Index.FirstNodeByNameStr(rootTypeName)
if !exists || rootType.Kind != ast.NodeKindObjectTypeDefinition {
return ""
}

// 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)
}
}
}
}
}
}
Comment on lines +194 to +238
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 27, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

potentially it is right, in case you are not normalizing operation before applying this

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧩 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:

  1. 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.

  2. 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.

  3. 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.


return ""
}

// GetSchema returns the built schema
func (v *VariablesSchemaBuilder) GetSchema() *JsonSchema {
// If we have required fields, the root schema cannot be nullable
Expand Down
Loading
Loading