-
Notifications
You must be signed in to change notification settings - Fork 157
feat: add Description support for VariableDefinition (September 2025 spec) #1457
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
base: master
Are you sure you want to change the base?
Changes from 3 commits
5c581bb
26d4d0a
1baf038
a3d6b94
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 |
|---|---|---|
|
|
@@ -1557,6 +1557,17 @@ 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) | ||
| if cap(list.Refs) == 0 { | ||
|
Member
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. You don't have to do it twice |
||
| list.Refs = p.document.Refs[p.document.NextRefIndex()][:0] | ||
| } | ||
| list.Refs = append(list.Refs, ref) | ||
| case keyword.DOLLAR: | ||
| if cap(list.Refs) == 0 { | ||
| list.Refs = p.document.Refs[p.document.NextRefIndex()][:0] | ||
|
|
@@ -1567,7 +1578,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 | ||
| } | ||
|
|
||
|
|
@@ -1578,9 +1589,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() | ||
|
|
||
|
|
||
|
Member
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. 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,18 @@ | ||
| """ | ||
| 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 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -787,6 +787,36 @@ User fields fragment | |
| """ | ||
| fragment UserFields on User {id name}`) | ||
| }) | ||
| t.Run("variable with single-line description", func(t *testing.T) { | ||
|
Member
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. could you scope it to t.Run("variables descriptions") |
||
| 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("variable with 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("variable 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}}`) | ||
| }) | ||
| } | ||
|
|
||
| func TestPrintArgumentWithBeforeAfterValue(t *testing.T) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
|
@@ -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
Contributor
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. 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 ( Please expand traversal to cover the full operation selection tree (including fragments) and resolve argument definitions in that context. 🤖 Prompt for AI Agents
Member
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. potentially it is right, in case you are not normalizing operation before applying this
Contributor
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. 🧩 Analysis chainGood point — let me check whether normalization is always applied to the operation before this builder runs. 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 29903 🏁 Script executed: Length of output: 1801 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1175 🏁 Script executed: Length of output: 184
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 However, a few nuances are worth noting regardless of normalization:
Would it make sense to document the normalization precondition explicitly (e.g., in the 🧠 Learnings used |
||
|
|
||
| return "" | ||
| } | ||
|
|
||
| // GetSchema returns the built schema | ||
| func (v *VariablesSchemaBuilder) GetSchema() *JsonSchema { | ||
| // If we have required fields, the root schema cannot be nullable | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.