fix: variables normalization for the anonymous operations (#965)

execution/engine/extractor_test.go

@@ -25,9 +25,8 @@ func TestExtractor_ExtractFieldsFromRequest(t *testing.T) {
 	graphql.NewExtractor().ExtractFieldsFromRequest(&request, schema, &report, fields)
 
 	expectedFields := graphql.RequestTypes{
-		"Foo":   {"fooField": {}},
 		"Post":  {"description": {}, "id": {}, "user": {}},
-		"Query": {"foo": {}, "posts": {}},
+		"Query": {"posts": {}},
 		"User":  {"id": {}, "name": {}},
 	}
 

execution/graphql/normalization.go

@@ -33,11 +33,14 @@ func (r *Request) Normalize(schema *Schema, options ...astnormalization.Option)
 		}
 	}
 
-	normalizer := astnormalization.NewWithOpts(options...)
-
 	if r.OperationName != "" {
+		options = append(options, astnormalization.WithRemoveNotMatchingOperationDefinitions())
+		normalizer := astnormalization.NewWithOpts(options...)
 		normalizer.NormalizeNamedOperation(&r.document, &schema.document, []byte(r.OperationName), &report)
 	} else {
+		// TODO: we should validate count of operations - to throw an error
+		// and do full normalization for the single anonymous operation
+		normalizer := astnormalization.NewWithOpts(options...)
 		normalizer.NormalizeOperation(&r.document, &schema.document, &report)
 	}
 

execution/graphql/normalization_test.go

@@ -31,6 +31,7 @@ func TestRequest_Normalize(t *testing.T) {
 	t.Run("should successfully normalize request with fragments", func(t *testing.T) {
 		schema := StarwarsSchema(t)
 		request := StarwarsRequestForQuery(t, starwars.FileFragmentsQuery)
+		request.OperationName = "Fragments"
 		documentBeforeNormalization := request.document
 
 		result, err := request.Normalize(schema)
@@ -125,27 +126,11 @@ func TestRequest_Normalize(t *testing.T) {
 		request := StarwarsRequestForQuery(t, starwars.FileMultiQueriesWithArguments)
 		request.OperationName = "GetDroid"
 
-		runNormalization(t, &request, `{"a":"1"}`, `query GetDroid($a: ID!){
+		runNormalization(t, &request, `{"a":"1"}`,
+			`query GetDroid($a: ID!){
     droid(id: $a){
         name
     }
-}
-
-query Search {
-    search(name: "C3PO"){
-        ... on Droid {
-            name
-            primaryFunction
-        }
-        ... on Human {
-            name
-            height
-        }
-        ... on Starship {
-            name
-            length
-        }
-    }
 }`)
 	})
 
@@ -154,9 +139,9 @@ query Search {
 		request := Request{
 			OperationName: "charactersByIds",
 			Variables:     stringify(map[string]interface{}{"a": 1}),
-			Query:         `query ($a: [Int]) { charactersByIds(ids: $a) { name }}`,
+			Query:         `query charactersByIds($a: [Int]) { charactersByIds(ids: $a) { name }}`,
 		}
-		runNormalizationWithSchema(t, schema, &request, `{"a":[1]}`, `query($a: [Int]){
+		runNormalizationWithSchema(t, schema, &request, `{"a":[1]}`, `query charactersByIds($a: [Int]){
     charactersByIds(ids: $a){
         name
     }
@@ -184,9 +169,9 @@ query Search {
 			Variables: stringify(map[string]interface{}{
 				"ids": 1,
 			}),
-			Query: `query($ids: [Int]) {charactersByIds(ids: $ids) { name }}`,
+			Query: `query charactersByIds($ids: [Int]) {charactersByIds(ids: $ids) { name }}`,
 		}
-		runNormalizationWithSchema(t, schema, &request, `{"ids":[1]}`, `query($ids: [Int]){
+		runNormalizationWithSchema(t, schema, &request, `{"ids":[1]}`, `query charactersByIds($ids: [Int]){
     charactersByIds(ids: $ids){
         name
     }

v2/pkg/astnormalization/astnormalization.go

@@ -71,7 +71,6 @@ package astnormalization
 import (
 	"github.com/wundergraph/graphql-go-tools/v2/pkg/ast"
 	"github.com/wundergraph/graphql-go-tools/v2/pkg/astvisitor"
-	"github.com/wundergraph/graphql-go-tools/v2/pkg/internal/unsafebytes"
 	"github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport"
 )
 
@@ -88,10 +87,10 @@ func NormalizeOperation(operation, definition *ast.Document, report *operationre
 
 func NormalizeNamedOperation(operation, definition *ast.Document, operationName []byte, report *operationreport.Report) {
 	normalizer := NewWithOpts(
+		WithRemoveNotMatchingOperationDefinitions(),
 		WithExtractVariables(),
 		WithRemoveFragmentDefinitions(),
 		WithInlineFragmentSpreads(),
-		WithRemoveNotMatchingOperationDefinitions(),
 		WithRemoveUnusedVariables(),
 	)
 	normalizer.NormalizeNamedOperation(operation, definition, operationName, report)
@@ -106,8 +105,6 @@ type walkerStage struct {
 type OperationNormalizer struct {
 	operationWalkers []walkerStage
 
-	variablesExtraction               *variablesExtractionVisitor
-	variablesDefaultValuesExtraction  *variablesDefaultValueExtractionVisitor
 	removeOperationDefinitionsVisitor *removeOperationDefinitionsVisitor
 
 	options              options
@@ -193,7 +190,21 @@ func WithNormalizeDefinition() Option {
 }
 
 func (o *OperationNormalizer) setupOperationWalkers() {
-	o.operationWalkers = make([]walkerStage, 0, 6)
+	o.operationWalkers = make([]walkerStage, 0, 9)
+
+	// NOTE: normalization rules for variables relies on the fact that
+	// we will visit only single operation, so it is important to remove non-matching operations
+	if o.options.removeNotMatchingOperationDefinitions {
+		removeNotMatchingOperationDefinitionsWalker := astvisitor.NewWalker(2)
+		// this rule do not walk deep into ast, so separate walk not expensive,
+		// but we could not mix this walk with other rules, because they need to go deep
+		o.removeOperationDefinitionsVisitor = removeOperationDefinitions(&removeNotMatchingOperationDefinitionsWalker)
+
+		o.operationWalkers = append(o.operationWalkers, walkerStage{
+			name:   "removeNotMatchingOperationDefinitions",
+			walker: &removeNotMatchingOperationDefinitionsWalker,
+		})
+	}
 
 	directivesIncludeSkip := astvisitor.NewWalker(8)
 	directiveIncludeSkip(&directivesIncludeSkip)
@@ -210,10 +221,6 @@ func (o *OperationNormalizer) setupOperationWalkers() {
 		detectVariableUsage(&directivesIncludeSkip, del)
 	}
 
-	if o.options.removeNotMatchingOperationDefinitions {
-		o.removeOperationDefinitionsVisitor = removeOperationDefinitions(&directivesIncludeSkip)
-	}
-
 	o.operationWalkers = append(o.operationWalkers, walkerStage{
 		name:   "directivesIncludeSkip, removeOperationDefinitions",
 		walker: &directivesIncludeSkip,
@@ -230,7 +237,7 @@ func (o *OperationNormalizer) setupOperationWalkers() {
 
 	if o.options.extractVariables {
 		extractVariablesWalker := astvisitor.NewWalker(8)
-		o.variablesExtraction = extractVariables(&extractVariablesWalker)
+		extractVariables(&extractVariablesWalker)
 		o.operationWalkers = append(o.operationWalkers, walkerStage{
 			name:   "extractVariables",
 			walker: &extractVariablesWalker,
@@ -270,7 +277,7 @@ func (o *OperationNormalizer) setupOperationWalkers() {
 	if o.options.extractVariables {
 		variablesProcessing := astvisitor.NewWalker(8)
 		inputCoercionForList(&variablesProcessing)
-		o.variablesDefaultValuesExtraction = extractVariablesDefaultValue(&variablesProcessing)
+		extractVariablesDefaultValue(&variablesProcessing)
 		injectInputFieldDefaults(&variablesProcessing)
 
 		o.operationWalkers = append(o.operationWalkers, walkerStage{
@@ -312,12 +319,6 @@ func (o *OperationNormalizer) NormalizeNamedOperation(operation, definition *ast
 		}
 	}
 
-	if o.variablesExtraction != nil {
-		o.variablesExtraction.operationName = operationName
-	}
-	if o.variablesDefaultValuesExtraction != nil {
-		o.variablesDefaultValuesExtraction.operationName = operationName
-	}
 	if o.removeOperationDefinitionsVisitor != nil {
 		o.removeOperationDefinitionsVisitor.operationName = operationName
 	}
@@ -337,46 +338,52 @@ func (o *OperationNormalizer) NormalizeNamedOperation(operation, definition *ast
 }
 
 type VariablesNormalizer struct {
-	pre    *astvisitor.Walker
-	post   *astvisitor.Walker
-	coerce *astvisitor.Walker
-
-	detect                  *variableUsageDetector
-	del                     *deleteUnusedVariablesVisitor
-	extractVariables        *variablesExtractionVisitor
-	extractDefaultVariables *variablesDefaultValueExtractionVisitor
+	firstDetectUnused *astvisitor.Walker
+	secondExtract     *astvisitor.Walker
+	thirdDeleteUnused *astvisitor.Walker
+	fourthCoerce      *astvisitor.Walker
 }
 
 func NewVariablesNormalizer() *VariablesNormalizer {
-	pre := astvisitor.NewWalker(8)
-	post := astvisitor.NewWalker(8)
-	coerce := astvisitor.NewWalker(0)
-	ex := extractVariables(&post)
-	def := extractVariablesDefaultValue(&post)
-	del := deleteUnusedVariables(&post)
-	det := detectVariableUsage(&pre, del)
-	inputCoercionForList(&coerce)
+	// delete unused modifying variables refs,
+	// so it is safer to run it sequentially with the extraction
+	thirdDeleteUnused := astvisitor.NewWalker(8)
+	del := deleteUnusedVariables(&thirdDeleteUnused)
+
+	// register variable usage detection on the first stage
+	// and pass usage information to the deletion visitor
+	// so it keeps variables that are defined but not used at all
+	// ensuring that validation can still catch them
+	firstDetectUnused := astvisitor.NewWalker(8)
+	detectVariableUsage(&firstDetectUnused, del)
+
+	secondExtract := astvisitor.NewWalker(8)
+	extractVariables(&secondExtract)
+	extractVariablesDefaultValue(&secondExtract)
+
+	fourthCoerce := astvisitor.NewWalker(0)
+	inputCoercionForList(&fourthCoerce)
+
 	return &VariablesNormalizer{
-		pre:                     &pre,
-		post:                    &post,
-		coerce:                  &coerce,
-		detect:                  det,
-		del:                     del,
-		extractVariables:        ex,
-		extractDefaultVariables: def,
+		firstDetectUnused: &firstDetectUnused,
+		secondExtract:     &secondExtract,
+		thirdDeleteUnused: &thirdDeleteUnused,
+		fourthCoerce:      &fourthCoerce,
 	}
 }
 
-func (v *VariablesNormalizer) NormalizeNamedOperation(operation, definition *ast.Document, operationName string, report *operationreport.Report) {
-	operationNameBytes := unsafebytes.StringToBytes(operationName)
-	v.extractVariables.operationName = operationNameBytes
-	v.extractDefaultVariables.operationName = operationNameBytes
-	v.detect.operationName = operationNameBytes
-	v.del.operationName = operationNameBytes
-	v.pre.Walk(operation, definition, report)
+func (v *VariablesNormalizer) NormalizeOperation(operation, definition *ast.Document, report *operationreport.Report) {
+	v.firstDetectUnused.Walk(operation, definition, report)
+	if report.HasErrors() {
+		return
+	}
+	v.secondExtract.Walk(operation, definition, report)
+	if report.HasErrors() {
+		return
+	}
+	v.thirdDeleteUnused.Walk(operation, definition, report)
 	if report.HasErrors() {
 		return
 	}
-	v.post.Walk(operation, definition, report)
-	v.coerce.Walk(operation, definition, report)
+	v.fourthCoerce.Walk(operation, definition, report)
 }

v2/pkg/astnormalization/astnormalization_test.go

@@ -851,11 +851,11 @@ func TestVariablesNormalizer(t *testing.T) {
 
 	normalizer := NewVariablesNormalizer()
 	report := operationreport.Report{}
-	normalizer.NormalizeNamedOperation(&operationDocument, &definitionDocument, "HttpBinPost", &report)
+	normalizer.NormalizeOperation(&operationDocument, &definitionDocument, &report)
 	require.False(t, report.HasErrors(), report.Error())
 
 	out := unsafeprinter.Print(&operationDocument)
-	require.Equal(t, `mutation HttpBinPost($bar: String!, $a: HttpBinPostInput){httpBinPost(input: $a){headers {userAgent} data {foo}}}`, out)
+	assert.Equal(t, `mutation HttpBinPost($bar: String!, $a: HttpBinPostInput){httpBinPost(input: $a){headers {userAgent} data {foo}}}`, out)
 	require.Equal(t, `{"a":{"foo":"bar","bar":null}}`, string(operationDocument.Input.Variables))
 }
 
@@ -934,21 +934,69 @@ var runWithVariablesAssert = func(t *testing.T, registerVisitor func(walker *ast
 	assert.Equal(t, expectedVariables, actualVariables)
 }
 
+// runWithVariablesAssertAndPreNormalize - runs pre-normalization functions before the main normalization function
+var runWithVariablesAssertAndPreNormalize = func(t *testing.T, registerVisitor func(walker *astvisitor.Walker), definition, operation, operationName, expectedOutput, variablesInput, expectedVariables string, prerequisites ...registerNormalizeFunc) {
+	t.Helper()
+
+	definitionDocument := unsafeparser.ParseGraphqlDocumentString(definition)
+	err := asttransform.MergeDefinitionWithBaseSchema(&definitionDocument)
+	if err != nil {
+		panic(err)
+	}
+
+	operationDocument := unsafeparser.ParseGraphqlDocumentString(operation)
+	expectedOutputDocument := unsafeparser.ParseGraphqlDocumentString(expectedOutput)
+	report := operationreport.Report{}
+
+	if variablesInput != "" {
+		operationDocument.Input.Variables = []byte(variablesInput)
+	}
+
+	additionalWalker := astvisitor.NewWalker(48)
+	for _, fn := range prerequisites {
+		fn(&additionalWalker)
+	}
+	report = operationreport.Report{}
+	additionalWalker.Walk(&operationDocument, &definitionDocument, &report)
+	if report.HasErrors() {
+		panic(report.Error())
+	}
+
+	initialWorker := astvisitor.NewWalker(48)
+	registerVisitor(&initialWorker)
+	initialWorker.Walk(&operationDocument, &definitionDocument, &report)
+	if report.HasErrors() {
+		panic(report.Error())
+	}
+
+	actualAST := mustString(astprinter.PrintString(&operationDocument))
+	expectedAST := mustString(astprinter.PrintString(&expectedOutputDocument))
+	assert.Equal(t, expectedAST, actualAST)
+	actualVariables := string(operationDocument.Input.Variables)
+	assert.Equal(t, expectedVariables, actualVariables)
+}
+
 var runWithVariablesExtraction = func(t *testing.T, normalizeFunc registerNormalizeVariablesFunc, definition, operation, operationName, expectedOutput, variablesInput, expectedVariables string, additionalNormalizers ...registerNormalizeFunc) {
 	t.Helper()
 
 	runWithVariablesAssert(t, func(walker *astvisitor.Walker) {
-		visitor := normalizeFunc(walker)
-		visitor.operationName = []byte(operationName)
+		normalizeFunc(walker)
 	}, definition, operation, operationName, expectedOutput, variablesInput, expectedVariables, additionalNormalizers...)
 }
 
+var runWithVariablesExtractionAndPreNormalize = func(t *testing.T, normalizeFunc registerNormalizeVariablesFunc, definition, operation, operationName, expectedOutput, variablesInput, expectedVariables string, prerequisites ...registerNormalizeFunc) {
+	t.Helper()
+
+	runWithVariablesAssertAndPreNormalize(t, func(walker *astvisitor.Walker) {
+		normalizeFunc(walker)
+	}, definition, operation, operationName, expectedOutput, variablesInput, expectedVariables, prerequisites...)
+}
+
 var runWithVariablesDefaultValues = func(t *testing.T, normalizeFunc registerNormalizeVariablesDefaulValueFunc, definition, operation, operationName, expectedOutput, variablesInput, expectedVariables string) {
 	t.Helper()
 
 	runWithVariablesAssert(t, func(walker *astvisitor.Walker) {
-		visitor := normalizeFunc(walker)
-		visitor.operationName = []byte(operationName)
+		normalizeFunc(walker)
 	}, definition, operation, operationName, expectedOutput, variablesInput, expectedVariables)
 }
 

v2/pkg/astnormalization/operation_definition_removal.go

@@ -32,6 +32,9 @@ func (r *removeOperationDefinitionsVisitor) EnterOperationDefinition(ref int) {
 	if !bytes.Equal(r.operation.OperationDefinitionNameBytes(ref), r.operationName) {
 		r.operationsToRemove[ref] = struct{}{}
 	}
+
+	// we do not want to visit the children of the operation definition
+	r.Walker.SkipNode()
 }
 
 func (r *removeOperationDefinitionsVisitor) LeaveDocument(operation, definition *ast.Document) {