Repository: jensneuse/graphql-go-tools (go)

Issue:
Variable validation for list variables is too strict
The following operation does not validate but should:
```
# Operation
query myQuery($arg: [String!]) {
    someQuery(arg: $arg)
}

# Schema
type Query {
    someQuery(arg: [String]): String
}
```
In this example, `[String!]` is more strict than `[String]` and is compatible per the GraphQL spec: https://spec.graphql.org/June2018/#AreTypesCompatible().

Validation returns the error:
```
value: $arg doesn't satisfy inputType: [String]
```

Pull request description:
Fix validation of non-null types in list arguments
This PR fixes #318.

Per the GraphQL spec[1], non-null variable types are compatible with
nullable types, even when such types are list items. This PR adjusts the
varialbe validation algorithm so non-null types are allowed to match
nullable types when they appear in lists (to arbitrary depth).

[1] https://spec.graphql.org/June2018/#AreTypesCompatible()

Patch:
diff --git a/pkg/astvalidation/operation_rule_valid_arguments.go b/pkg/astvalidation/operation_rule_valid_arguments.go
index 8a09271b..b5b11c7e 100644
--- a/pkg/astvalidation/operation_rule_valid_arguments.go
+++ b/pkg/astvalidation/operation_rule_valid_arguments.go
@@ -32,7 +32,6 @@ func (v *validArgumentsVisitor) EnterDocument(operation, definition *ast.Documen
 }
 
 func (v *validArgumentsVisitor) EnterArgument(ref int) {
-
 	definition, exists := v.ArgumentInputValueDefinition(ref)
 
 	if !exists {
@@ -47,7 +46,6 @@ func (v *validArgumentsVisitor) EnterArgument(ref int) {
 }
 
 func (v *validArgumentsVisitor) validateIfValueSatisfiesInputFieldDefinition(value ast.Value, inputValueDefinition int) {
-
 	var satisfied bool
 
 	switch value.Kind {
@@ -156,7 +154,6 @@ func (v *validArgumentsVisitor) nullValueSatisfiesInputValueDefinition(inputValu
 }
 
 func (v *validArgumentsVisitor) enumValueSatisfiesInputValueDefinition(enumValue, inputValueDefinition int) bool {
-
 	definitionTypeName := v.definition.ResolveTypeNameBytes(v.definition.InputValueDefinitions[inputValueDefinition].Type)
 	node, exists := v.definition.Index.FirstNodeByNameBytes(definitionTypeName)
 	if !exists {
@@ -187,35 +184,51 @@ func (v *validArgumentsVisitor) variableValueSatisfiesInputValueDefinition(varia
 }
 
 func (v *validArgumentsVisitor) operationTypeSatisfiesDefinitionType(operationType int, definitionType int, hasDefaultValue bool) bool {
-
-	if operationType == -1 || definitionType == -1 {
-		return false
-	}
-
-	if v.operation.Types[operationType].TypeKind != ast.TypeKindNonNull &&
-		v.definition.Types[definitionType].TypeKind == ast.TypeKindNonNull &&
-		hasDefaultValue &&
-		v.definition.Types[definitionType].OfType != -1 {
+	opKind := v.operation.Types[operationType].TypeKind
+	defKind := v.definition.Types[definitionType].TypeKind
+
+	// A nullable op type is compatible with a non-null def type if the def has
+	// a default value. Strip the def non-null and continue comparing. This
+	// logic is only valid before any unnesting of types occurs, which is why
+	// it's outside the for loop below.
+	//
+	// Example:
+	// Op:  someField(arg: Boolean): String
+	// Def: someField(arg: Boolean! = false): String  #  Boolean! -> Boolean
+	if opKind != ast.TypeKindNonNull && defKind == ast.TypeKindNonNull && hasDefaultValue {
 		definitionType = v.definition.Types[definitionType].OfType
 	}
 
-	if v.operation.Types[operationType].TypeKind == ast.TypeKindNonNull &&
-		v.definition.Types[definitionType].TypeKind != ast.TypeKindNonNull &&
-		v.operation.Types[operationType].OfType != -1 {
-		operationType = v.operation.Types[operationType].OfType
-	}
-
+	// Unnest the op and def arg types until a named type is reached,
+	// then compare.
 	for {
 		if operationType == -1 || definitionType == -1 {
 			return false
 		}
-		if v.operation.Types[operationType].TypeKind != v.definition.Types[definitionType].TypeKind {
+		opKind = v.operation.Types[operationType].TypeKind
+		defKind = v.definition.Types[definitionType].TypeKind
+
+		// If the op arg type is stricter than the def arg type, that's okay.
+		// Strip the op non-null and continue comparing.
+		//
+		// Example:
+		// Op:  someField(arg: Boolean!): String  # Boolean! -> Boolean
+		// Def: someField(arg: Boolean): String
+		if opKind == ast.TypeKindNonNull && defKind != ast.TypeKindNonNull {
+			operationType = v.operation.Types[operationType].OfType
+			continue
+		}
+
+		if opKind != defKind {
 			return false
 		}
-		if v.operation.Types[operationType].TypeKind == ast.TypeKindNamed {
+		if opKind == ast.TypeKindNamed {
+			// defKind is also a named type because at this point both kinds
+			// are the same! Compare the names.
 			return bytes.Equal(v.operation.Input.ByteSlice(v.operation.Types[operationType].Name),
 				v.definition.Input.ByteSlice(v.definition.Types[definitionType].Name))
 		}
+		// Both types are non-null or list. Unnest and continue comparing.
 		operationType = v.operation.Types[operationType].OfType
 		definitionType = v.definition.Types[definitionType].OfType
 	}


Test patch:
diff --git a/pkg/astvalidation/operation_validation_test.go b/pkg/astvalidation/operation_validation_test.go
index 1ca26dcb..a8485c81 100644
--- a/pkg/astvalidation/operation_validation_test.go
+++ b/pkg/astvalidation/operation_validation_test.go
@@ -33,7 +33,6 @@ func withExpectNormalizationError() option {
 }
 
 func TestExecutionValidation(t *testing.T) {
-
 	must := func(err error) {
 		if report, ok := err.(operationreport.Report); ok {
 			if report.HasErrors() {
@@ -59,7 +58,6 @@ func TestExecutionValidation(t *testing.T) {
 	}
 
 	runWithDefinition := func(definitionInput, operationInput string, rule Rule, expectation ValidationState, opts ...option) {
-
 		var options options
 		for _, opt := range opts {
 			opt(&options)
@@ -3535,50 +3533,93 @@ func TestExecutionValidation(t *testing.T) {
 								}`,
 					ValidArguments(), Invalid)
 			})
+			// Non-null types are compatible with nullable types.
 			t.Run("172", func(t *testing.T) {
-				run(`query nonNullListToList($nonNullBooleanList: [Boolean]!) {
+				run(`query nonNullListToList($nonNullListOfBoolean: [Boolean]!) {
 								arguments {
-									booleanListArgField(booleanListArg: $nonNullBooleanList)
+									nonNullListOfBooleanArgField(nonNullListOfBooleanArg: $nonNullListOfBoolean)
 								}
 							}`,
 					ValidArguments(), Valid)
 			})
 			t.Run("172 variant", func(t *testing.T) {
-				run(`query nonNullListToList {
+				run(`query listToList($listOfBoolean: [Boolean]) {
+									arguments {
+										listOfBooleanArgField(listOfBooleanArg: $listOfBoolean)
+									}
+								}`,
+					ValidArguments(), Valid)
+			})
+			t.Run("172 variant", func(t *testing.T) {
+				run(`query listOfNonNullToList($listOfNonNullBoolean: [Boolean!]) {
+									arguments {
+										listOfBooleanArgField(listOfBooleanArg: $listOfNonNullBoolean)
+									}
+								}`,
+					ValidArguments(), Valid)
+			})
+			t.Run("172 variant", func(t *testing.T) {
+				run(`query nonNullListOfNonNullToList($nonNullListOfNonNullBoolean: [Boolean!]!) {
 									arguments {
-										booleanListArgField(booleanListArg: [true,false,true])
+										listOfBooleanArgField(listOfBooleanArg: $nonNullListOfNonNullBoolean)
+									}
+								}`,
+					ValidArguments(), Valid)
+			})
+			t.Run("172 variant", func(t *testing.T) {
+				run(`query nonNullListToListLiteral {
+									arguments {
+										nonNullListOfBooleanArgField(nonNullListOfBooleanArg: [true,false,true])
 									}
 								}`,
 					Values(), Valid)
 			})
+			// Types in lists must match
 			t.Run("172 variant", func(t *testing.T) {
-				run(`query nonNullListToList {
+				run(`query listContainingIncorrectType {
 									arguments {
-										booleanListArgField(booleanListArg: [true,false,"123"])
+										nonNullListOfBooleanArgField(nonNullListOfBooleanArg: [true,false,"123"])
 									}
 								}`,
 					Values(), Invalid)
 			})
 			t.Run("172 variant", func(t *testing.T) {
-				run(`query nonNullListToList {
+				run(`query listContainingIncorrectType {
 									arguments {
-										booleanListArgField(booleanListArg: [true,false,123])
+										nonNullListOfBooleanArgField(nonNullListOfBooleanArg: [true,false,123])
 									}
 								}`,
 					Values(), Invalid)
 			})
+			// Nullable types are NOT compatible with non-null types.
 			t.Run("172 variant", func(t *testing.T) {
-				run(`query nonNullListToList($nonNullBooleanList: [Boolean]) {
+				run(`query listToListOfNonNull($listOfBoolean: [Boolean]) {
 									arguments {
-										booleanListArgField(booleanListArg: $nonNullBooleanList)
+										listOfNonNullBooleanArgField(listOfNonNullBooleanArg: $listOfBoolean)
+									}
+								}`,
+					ValidArguments(), Invalid)
+			})
+			t.Run("172 variant", func(t *testing.T) {
+				run(`query nonNullListToListOfNonNull($nonNullListOfBoolean: [Boolean]!) {
+									arguments {
+										listOfNonNullBooleanArgField(listOfNonNullBooleanArg: $nonNullListOfBoolean)
+									}
+								}`,
+					ValidArguments(), Invalid)
+			})
+			t.Run("172 variant", func(t *testing.T) {
+				run(`query listOfNonNullToNonNullList($listOfNonNullBoolean: [Boolean!]) {
+									arguments {
+										nonNullListOfBooleanArgField(nonNullListOfBooleanArg: $listOfNonNullBoolean)
 									}
 								}`,
 					ValidArguments(), Invalid)
 			})
 			t.Run("173", func(t *testing.T) {
-				run(`query listToNonNullList($booleanList: [Boolean]) {
+				run(`query listToNonNullList($listOfBoolean: [Boolean]) {
 									arguments {
-										nonNullBooleanListField(nonNullBooleanListArg: $booleanList)
+										nonNullListOfBooleanArgField(nonNullListOfBooleanArg: $listOfBoolean)
 									}
 								}`,
 					ValidArguments(), Invalid)
@@ -3586,7 +3627,7 @@ func TestExecutionValidation(t *testing.T) {
 			t.Run("174", func(t *testing.T) {
 				run(`query booleanArgQueryWithDefault($booleanArg: Boolean) {
 									arguments {
-										optionalNonNullBooleanArgField(optionalBooleanArg: $booleanArg)
+										nonNullBooleanWithDefaultArgField(nonNullBooleanWithDefaultArg: $booleanArg)
 									}
 								}`,
 					ValidArguments(), Valid)
@@ -3742,7 +3783,6 @@ func TestValidationEdgeCases(t *testing.T) {
 }
 
 func BenchmarkValidation(b *testing.B) {
-
 	must := func(err error) {
 		if err != nil {
 			panic(err)
@@ -3757,7 +3797,6 @@ func BenchmarkValidation(b *testing.B) {
 	}
 
 	run := func(b *testing.B, definition, operation string, state ValidationState) {
-
 		op, def := mustDocument(astparser.ParseGraphqlDocumentString(operation)), mustDocument(astparser.ParseGraphqlDocumentString(definition))
 		report := operationreport.Report{}
 		astnormalization.NormalizeOperation(&op, &def, &report)
@@ -4288,13 +4327,15 @@ type Arguments {
 
 type ValidArguments {
 	multipleReqs(x: Int!, y: Int!): Int!
-	booleanArgField(booleanArg: Boolean): Boolean
 	floatArgField(floatArg: Float): Float
 	intArgField(intArg: Int): Int
-	nonNullBooleanArgField(nonNullBooleanArg: Boolean!): Boolean!
-	nonNullBooleanListField(nonNullBooleanListArg: [Boolean]!): Boolean!
-	booleanListArgField(booleanListArg: [Boolean]!): [Boolean]
-	optionalNonNullBooleanArgField(optionalBooleanArg: Boolean! = false): Boolean!
+	booleanArgField(booleanArg: Boolean): String
+	nonNullBooleanArgField(nonNullBooleanArg: Boolean!): String
+	listOfBooleanArgField(listOfBooleanArg: [Boolean]): String
+	listOfNonNullBooleanArgField(listOfNonNullBooleanArg: [Boolean!]): String
+	nonNullListOfBooleanArgField(nonNullListOfBooleanArg: [Boolean]!): String
+	nonNullListOfNonNullBooleanArgField(nonNullListOfNonNullBooleanArg: [Boolean!]!): String
+	nonNullBooleanWithDefaultArgField(nonNullBooleanWithDefaultArg: Boolean! = false): String
 }
 
 enum DogCommand { SIT, DOWN, HEEL }
@@ -11279,4 +11320,4 @@ input UpdateRegionGameInput {
 
 input UserPreferencesInput {
     notifications: PreNotificationsInput!
-}`
\ No newline at end of file
+}`