False-positive when validating a a query that uses an input object with missing fields

[mattjohnsonpint]

If a schema uses a GraphQL input type, and one tries to validate a query that is missing one or more of the fields of that input type, I would expect a validation error. On an un-normalized document, it works correctly. But after normalization, the validation passes as if there was no error.

The following test code demonstrates 8 test cases. 7 pass, and 1 fails. The failing cases passes if you comment out the astnormalization.WithExtractVariables() option. However, that's not a practical workaround since it's set by default in a few places internally within GraphQL Go Tools.

package main

import (
	"fmt"
	"testing"

	"github.com/wundergraph/graphql-go-tools/v2/pkg/ast"
	"github.com/wundergraph/graphql-go-tools/v2/pkg/astnormalization"
	"github.com/wundergraph/graphql-go-tools/v2/pkg/astparser"
	"github.com/wundergraph/graphql-go-tools/v2/pkg/asttransform"
	"github.com/wundergraph/graphql-go-tools/v2/pkg/astvalidation"
	"github.com/wundergraph/graphql-go-tools/v2/pkg/operationreport"
)

func TestQueryPlain(t *testing.T) {
	schema := `
	type Query {
		test(a: Int!, b: String!): Int!
	}`

	validQuery := `query { test(a: 123, b:"xyz") }`
	invalidQuery := `query { test(a: 123) }`

	runTests(t, schema, validQuery, invalidQuery)
}

func TestQueryWithInputObject(t *testing.T) {
	schema := `
	type Query {
		test(foo: FooInput!): Int!
	}
	
	input FooInput {
		a: Int!
		b: String!
	}`

	validQuery := `query { test(foo: {a: 123, b:"xyz"}) }`
	invalidQuery := `query { test(foo: {a: 123}) }`

	runTests(t, schema, validQuery, invalidQuery)
}

func runTests(t *testing.T, schema, validQuery, invalidQuery string) {
	testCases := []struct {
		name      string
		valid     bool
		normalize bool
	}{
		{"valid original", true, false},
		{"valid normalized", true, true},
		{"invalid original", false, false},
		{"invalid normalized", false, true},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			if tc.valid {
				err := validate(schema, validQuery, tc.normalize)
				if err != nil {
					t.Fatal(err)
				}
			} else {
				err := validate(schema, invalidQuery, tc.normalize)
				if err == nil {
					t.Fatal("expected validation error")
				} else {
					t.Log(err)
				}
			}
		})
	}
}

func validate(schema, operation string, normalize bool) error {
	var schemaDoc *ast.Document
	if doc, report := astparser.ParseGraphqlDocumentString(schema); report.HasErrors() {
		return fmt.Errorf("failed to parse schema: %s", report.Error())
	} else {
		schemaDoc = &doc
		if err := asttransform.MergeDefinitionWithBaseSchema(schemaDoc); err != nil {
			return fmt.Errorf("failed to merge schema: %w", err)
		}
	}

	var operationDoc *ast.Document
	if doc, report := astparser.ParseGraphqlDocumentString(operation); report.HasErrors() {
		return fmt.Errorf("failed to parse operation: %s", report.Error())
	} else {
		operationDoc = &doc
	}

	if normalize {
		normalizeOperation(operationDoc, schemaDoc)
	}

	report := &operationreport.Report{}
	validator := astvalidation.DefaultOperationValidator()
	validator.Validate(operationDoc, schemaDoc, report)
	if report.HasErrors() {
		return fmt.Errorf("validation failed: %s", report.Error())
	}

	return nil
}

func normalizeOperation(operationDoc, schemaDoc *ast.Document) error {

	// These options are the same as the ones used in execution library:
	//
	// https://github.com/wundergraph/graphql-go-tools/blob/3c6d147f708a76ef3a62007407afa94d50ab0dfb/execution/graphql/normalization.go#L26-L31

	normalizer := astnormalization.NewWithOpts(
		astnormalization.WithExtractVariables(), // removing this option makes the test pass
		astnormalization.WithRemoveFragmentDefinitions(),
		astnormalization.WithRemoveUnusedVariables(),
		astnormalization.WithInlineFragmentSpreads(),
	)

	report := &operationreport.Report{}
	normalizer.NormalizeOperation(operationDoc, schemaDoc, report)
	if report.HasErrors() {
		return fmt.Errorf("normalization failed: %s", report.Error())
	}

	return nil
}

[jensneuse]

I think the VariablesValidator (or similar name, I don't fully recall) is responsible for this. We've split validation into two parts.

[mattjohnsonpint]

FYI, it's still failing on rc 81. I presume #888 was only part of the fix, since you said it was split into two parts.