fix: follow the specification for absent, null, and default input values

directives_test.go

@@ -1,6 +1,7 @@
 package graphql_test
 
 import (
+	"encoding/json"
 	"errors"
 	"testing"
 
@@ -514,3 +515,71 @@ func TestDirectivesWorksWithSkipAndIncludeDirectives_NoIncludeOrSkip(t *testing.
 		t.Fatalf("Unexpected result, Diff: %v", testutil.Diff(expected, result))
 	}
 }
+
+// The same two fields under both modes, so the @skip / @include tests below can
+// pin the opt-out column as a regression guard.
+var directivesNonSpecTestSchema, _ = graphql.NewSchema(graphql.SchemaConfig{
+	Query: graphql.NewObject(graphql.ObjectConfig{
+		Name: "TestType",
+		Fields: graphql.Fields{
+			"a": &graphql.Field{Type: graphql.String},
+			"b": &graphql.Field{Type: graphql.String},
+		},
+	}),
+	NonSpecArgumentHandling: true,
+})
+
+// runDirectiveModes executes doc through graphql.Do under both modes. Results are
+// compared as the serialised data map plus the first error, so a test can pin an
+// expected rejection as well as an expected shape.
+func runDirectiveModes(t *testing.T, doc string, vars map[string]interface{}, wantNonSpec, wantSpec string) {
+	t.Helper()
+	run := func(schema graphql.Schema) string {
+		result := graphql.Do(graphql.Params{
+			Schema:         schema,
+			RequestString:  doc,
+			VariableValues: vars,
+			RootObject:     directivesTestData,
+		})
+		data, _ := json.Marshal(result.Data)
+		if len(result.Errors) > 0 {
+			return string(data) + " ERROR: " + result.Errors[0].Message
+		}
+		return string(data)
+	}
+	if got := run(directivesNonSpecTestSchema); got != wantNonSpec {
+		t.Errorf("non-spec mode mismatch\n  got:  %s\n  want: %s", got, wantNonSpec)
+	}
+	if got := run(directivesTestSchema); got != wantSpec {
+		t.Errorf("spec mode mismatch\n  got:  %s\n  want: %s", got, wantSpec)
+	}
+}
+
+// Spec §6.3.2 CollectFields tests @skip and @include structurally rather than
+// coercing their arguments: @skip drops the selection when if is true, and
+// @include keeps it only when if is true. An if that is neither — a variable
+// carrying null, say — is simply "not true", and no error is raised.
+//
+// The one document that reaches this at runtime declares a variable with a
+// non-null default, which §5.8.5 allows at the non-null if argument, and then
+// supplies null for it.
+func TestDirectives_NullIfArgument_IsNotTrue(t *testing.T) {
+	vars := map[string]interface{}{"x": nil}
+
+	t.Run("@skip with a null if keeps the selection", func(t *testing.T) {
+		// Non-spec mode never sees the null: the variable's default replaces it.
+		runDirectiveModes(t, `query Q($x: Boolean = true){ a @skip(if: $x) b }`, vars,
+			`{"b":"b"}`, `{"a":"a","b":"b"}`)
+	})
+	t.Run("@include with a null if drops the selection", func(t *testing.T) {
+		runDirectiveModes(t, `query Q($x: Boolean = true){ a @include(if: $x) b }`, vars,
+			`{"a":"a","b":"b"}`, `{"b":"b"}`)
+	})
+	t.Run("a true value still behaves normally", func(t *testing.T) {
+		v := map[string]interface{}{"x": true}
+		runDirectiveModes(t, `query Q($x: Boolean = true){ a @skip(if: $x) b }`, v,
+			`{"b":"b"}`, `{"b":"b"}`)
+		runDirectiveModes(t, `query Q($x: Boolean = true){ a @include(if: $x) b }`, v,
+			`{"a":"a","b":"b"}`, `{"a":"a","b":"b"}`)
+	})
+}

executor.go

@@ -481,10 +481,7 @@ func collectFields(p collectFieldsParams) (fields map[string][]*ast.Field) {
 // Determines if a field should be included based on the @include and @skip
 // directives, where @skip has higher precedence than @include.
 func shouldIncludeNode(eCtx *executionContext, directives []*ast.Directive) bool {
-	var (
-		skipAST, includeAST *ast.Directive
-		argValues           map[string]interface{}
-	)
+	var skipAST, includeAST *ast.Directive
 	for _, directive := range directives {
 		if directive == nil || directive.Name == nil {
 			continue
@@ -496,22 +493,40 @@ func shouldIncludeNode(eCtx *executionContext, directives []*ast.Directive) bool
 			includeAST = directive
 		}
 	}
+	nonSpec := eCtx.Schema.nonSpecArgumentHandling
 	// precedence: skipAST > includeAST
 	if skipAST != nil {
-		argValues = getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues)
-		if skipIf, ok := argValues["if"].(bool); ok && skipIf {
+		if directiveIfIsTrue(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, nonSpec) {
 			return false // excluded selectionSet's fields
 		}
 	}
 	if includeAST != nil {
-		argValues = getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues)
-		if includeIf, ok := argValues["if"].(bool); ok && !includeIf {
+		if nonSpec {
+			argValues, _ := getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, nonSpec)
+			if includeIf, ok := argValues["if"].(bool); ok && !includeIf {
+				return false // excluded selectionSet's fields
+			}
+		} else if !directiveIfIsTrue(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, nonSpec) {
+			// Spec §6.3.2: the selection is kept only when if is true.
 			return false // excluded selectionSet's fields
 		}
 	}
 	return true
 }
 
+// Reports whether the directive's "if" argument resolves to true. Spec §6.3.2
+// CollectFields asks only that question of @skip and @include — it does not
+// coerce their arguments — so a value that is not true, including one a coercion
+// failure left unusable, simply answers false rather than raising an error.
+func directiveIfIsTrue(argDefs []*Argument, argASTs []*ast.Argument, variableValues map[string]interface{}, nonSpec bool) bool {
+	argValues, err := getArgumentValues(argDefs, argASTs, variableValues, nonSpec)
+	if err != nil {
+		return false
+	}
+	ifValue, ok := argValues["if"].(bool)
+	return ok && ifValue
+}
+
 // Determines if a fragment is applicable to the given type.
 func doesFragmentConditionMatch(eCtx *executionContext, fragment ast.Node, ttype *Object) bool {
 
@@ -624,7 +639,12 @@ func resolveField(eCtx *executionContext, parentType *Object, source interface{}
 	// Build a map of arguments from the field.arguments AST, using the
 	// variables scope to fulfill any variable references.
 	// TODO: find a way to memoize, in case this field is within a List type.
-	args := getArgumentValues(fieldDef.Args, fieldAST.Arguments, eCtx.VariableValues)
+	args, argErr := getArgumentValues(fieldDef.Args, fieldAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling)
+	if argErr != nil {
+		// Same idiom as a failing resolver below: the deferred recover turns this
+		// into a field error via handleFieldError.
+		panic(argErr)
+	}
 
 	info := ResolveInfo{
 		FieldName:      fieldName,

rules.go

@@ -68,13 +68,15 @@ func reportError(context *ValidationContext, message string, nodes []ast.Node) (
 // A GraphQL document is only valid if all field argument literal values are
 // of the type expected by their position.
 func ArgumentsOfCorrectTypeRule(context *ValidationContext) *ValidationRuleInstance {
+	nonSpec := context.Schema().nonSpecArgumentHandling
+
 	visitorOpts := &visitor.VisitorOptions{
 		KindFuncMap: map[string]visitor.NamedVisitFuncs{
 			kinds.Argument: {
 				Kind: func(p visitor.VisitFuncParams) (string, interface{}) {
 					if argAST, ok := p.Node.(*ast.Argument); ok {
 						if argDef := context.Argument(); argDef != nil {
-							if isValid, messages := isValidLiteralValue(argDef.Type, argAST.Value); !isValid {
+							if isValid, messages := isValidLiteralValue(argDef.Type, argAST.Value, nonSpec); !isValid {
 								var messagesStr, argNameValue string
 								if argAST.Name != nil {
 									argNameValue = argAST.Name.Value
@@ -108,6 +110,8 @@ func ArgumentsOfCorrectTypeRule(context *ValidationContext) *ValidationRuleInsta
 // A GraphQL document is only valid if all variable default values are of the
 // type expected by their definition.
 func DefaultValuesOfCorrectTypeRule(context *ValidationContext) *ValidationRuleInstance {
+	nonSpec := context.Schema().nonSpecArgumentHandling
+
 	visitorOpts := &visitor.VisitorOptions{
 		KindFuncMap: map[string]visitor.NamedVisitFuncs{
 			kinds.VariableDefinition: {
@@ -123,16 +127,21 @@ func DefaultValuesOfCorrectTypeRule(context *ValidationContext) *ValidationRuleI
 						}
 						ttype := context.InputType()
 
-						// when input variable value must be nonNull, and set default value is unnecessary
-						if ttype, ok := ttype.(*NonNull); ok && defaultValue != nil {
-							reportError(
-								context,
-								fmt.Sprintf(`Variable "$%v" of type "%v" is required and will not use the default value. Perhaps you meant to use type "%v".`,
-									name, ttype, ttype.OfType),
-								[]ast.Node{defaultValue},
-							)
+						// Spec §6.1.2 applies a variable's default before checking the
+						// non-null requirement, so a non-null variable may declare one and
+						// nothing in the Validation section forbids it. Only a schema that
+						// opted into NonSpecArgumentHandling keeps rejecting it.
+						if nonSpec {
+							if ttype, ok := ttype.(*NonNull); ok && defaultValue != nil {
+								reportError(
+									context,
+									fmt.Sprintf(`Variable "$%v" of type "%v" is required and will not use the default value. Perhaps you meant to use type "%v".`,
+										name, ttype, ttype.OfType),
+									[]ast.Node{defaultValue},
+								)
+							}
 						}
-						if isValid, messages := isValidLiteralValue(ttype, defaultValue); !isValid && defaultValue != nil {
+						if isValid, messages := isValidLiteralValue(ttype, defaultValue, nonSpec); !isValid && defaultValue != nil {
 							if len(messages) > 0 {
 								messagesStr = "\n" + strings.Join(messages, "\n")
 							}
@@ -1247,6 +1256,14 @@ func PossibleFragmentSpreadsRule(context *ValidationContext) *ValidationRuleInst
 // have been provided.
 func ProvidedNonNullArgumentsRule(context *ValidationContext) *ValidationRuleInstance {
 
+	// Spec §5.4.2.1: an argument is required only when its type is non-null and
+	// it declares no default value. Nullish is the test that coercion applies, so
+	// a default that coercion will not substitute leaves the argument required. A
+	// schema that opted into NonSpecArgumentHandling keeps the older, stricter
+	// reading, under which every non-null argument is required whether or not it
+	// has a default.
+	nonSpec := context.Schema().nonSpecArgumentHandling
+
 	visitorOpts := &visitor.VisitorOptions{
 		KindFuncMap: map[string]visitor.NamedVisitFuncs{
 			kinds.Field: {
@@ -1271,7 +1288,7 @@ func ProvidedNonNullArgumentsRule(context *ValidationContext) *ValidationRuleIns
 						for _, argDef := range fieldDef.Args {
 							argAST, _ := argASTMap[argDef.Name()]
 							if argAST == nil {
-								if argDefType, ok := argDef.Type.(*NonNull); ok {
+								if argDefType, ok := argDef.Type.(*NonNull); ok && (nonSpec || isNullish(argDef.DefaultValue)) {
 									fieldName := ""
 									if fieldAST.Name != nil {
 										fieldName = fieldAST.Name.Value
@@ -1312,7 +1329,7 @@ func ProvidedNonNullArgumentsRule(context *ValidationContext) *ValidationRuleIns
 						for _, argDef := range directiveDef.Args {
 							argAST, _ := argASTMap[argDef.Name()]
 							if argAST == nil {
-								if argDefType, ok := argDef.Type.(*NonNull); ok {
+								if argDefType, ok := argDef.Type.(*NonNull); ok && (nonSpec || isNullish(argDef.DefaultValue)) {
 									directiveName := ""
 									if directiveAST.Name != nil {
 										directiveName = directiveAST.Name.Value
@@ -1656,8 +1673,33 @@ func effectiveType(varType Type, varDef *ast.VariableDefinition) Type {
 	return NewNonNull(varType)
 }
 
+// allowedVariableUsage implements spec §5.8.5 IsVariableUsageAllowed. A nullable
+// variable is allowed at a non-null location when either the variable or the
+// location declares a default value: whichever one exists covers the case where
+// the caller supplies nothing.
+func allowedVariableUsage(schema *Schema, varType Type, varDefaultValue ast.Value,
+	locationType Input, locationDefaultValue interface{}) bool {
+	if locType, ok := locationType.(*NonNull); ok {
+		if _, isNonNull := varType.(*NonNull); !isNonNull {
+			// The parser does not accept the null literal, so a default value that
+			// exists is necessarily not null. Revisit if that ever changes.
+			hasNonNullVariableDefaultValue := varDefaultValue != nil
+			// Nullish is the test that coercion applies to a declared default, so a
+			// default it will not substitute does not count as one here either.
+			hasLocationDefaultValue := !isNullish(locationDefaultValue)
+			if !hasNonNullVariableDefaultValue && !hasLocationDefaultValue {
+				return false
+			}
+			ofType, _ := locType.OfType.(Input)
+			return isTypeSubTypeOf(schema, varType, ofType)
+		}
+	}
+	return isTypeSubTypeOf(schema, varType, locationType)
+}
+
 // VariablesInAllowedPositionRule Variables passed to field arguments conform to type
 func VariablesInAllowedPositionRule(context *ValidationContext) *ValidationRuleInstance {
+	nonSpec := context.Schema().nonSpecArgumentHandling
 
 	varDefMap := map[string]*ast.VariableDefinition{}
 
@@ -1683,7 +1725,15 @@ func VariablesInAllowedPositionRule(context *ValidationContext) *ValidationRuleI
 								if err != nil {
 									varType = nil
 								}
-								if varType != nil && !isTypeSubTypeOf(context.Schema(), effectiveType(varType, varDef), usage.Type) {
+								allowed := true
+								if varType != nil {
+									if nonSpec {
+										allowed = isTypeSubTypeOf(context.Schema(), effectiveType(varType, varDef), usage.Type)
+									} else {
+										allowed = allowedVariableUsage(context.Schema(), varType, varDef.DefaultValue, usage.Type, usage.LocationDefaultValue)
+									}
+								}
+								if varType != nil && !allowed {
 									reportError(
 										context,
 										fmt.Sprintf(`Variable "$%v" of type "%v" used in position `+
@@ -1724,7 +1774,7 @@ func VariablesInAllowedPositionRule(context *ValidationContext) *ValidationRuleI
 //
 // Note that this only validates literal values, variables are assumed to
 // provide values of the correct type.
-func isValidLiteralValue(ttype Input, valueAST ast.Value) (bool, []string) {
+func isValidLiteralValue(ttype Input, valueAST ast.Value, nonSpec bool) (bool, []string) {
 	if _, ok := ttype.(*NonNull); !ok {
 		if valueAST == nil {
 			return true, nil
@@ -1749,21 +1799,21 @@ func isValidLiteralValue(ttype Input, valueAST ast.Value) (bool, []string) {
 			return false, []string{"Expected non-null value, found null."}
 		}
 		ofType, _ := ttype.OfType.(Input)
-		return isValidLiteralValue(ofType, valueAST)
+		return isValidLiteralValue(ofType, valueAST, nonSpec)
 	case *List:
 		// Lists accept a non-list value as a list of one.
 		itemType, _ := ttype.OfType.(Input)
 		if valueAST, ok := valueAST.(*ast.ListValue); ok {
 			messagesReduce := []string{}
 			for _, value := range valueAST.Values {
-				_, messages := isValidLiteralValue(itemType, value)
+				_, messages := isValidLiteralValue(itemType, value, nonSpec)
 				for idx, message := range messages {
 					messagesReduce = append(messagesReduce, fmt.Sprintf(`In element #%v: %v`, idx+1, message))
 				}
 			}
 			return (len(messagesReduce) == 0), messagesReduce
 		}
-		return isValidLiteralValue(itemType, valueAST)
+		return isValidLiteralValue(itemType, valueAST, nonSpec)
 	case *InputObject:
 		// Input objects check each defined field and look for undefined fields.
 		valueAST, ok := valueAST.(*ast.ObjectValue)
@@ -1786,10 +1836,21 @@ func isValidLiteralValue(ttype Input, valueAST ast.Value) (bool, []string) {
 		// Ensure every defined field is valid.
 		for fieldName, field := range fields {
 			var fieldASTValue ast.Value
-			if fieldAST := fieldASTMap[fieldName]; fieldAST != nil {
+			fieldAST, ok := fieldASTMap[fieldName]
+			if ok && fieldAST != nil {
 				fieldASTValue = fieldAST.Value
 			}
-			if isValid, messages := isValidLiteralValue(field.Type, fieldASTValue); !isValid {
+			// Spec §3.10 Input Coercion and §5.6.4 Input Object Required Fields: a
+			// field that declares a default value is optional even when its type is
+			// non-null. A field written in the literal is still validated.
+			//
+			// Nullish is the same test that coercion applies, so a default that
+			// coercion will not substitute does not make the field optional here
+			// either.
+			if !nonSpec && !ok && !isNullish(field.DefaultValue) {
+				continue
+			}
+			if isValid, messages := isValidLiteralValue(field.Type, fieldASTValue, nonSpec); !isValid {
 				for _, message := range messages {
 					messagesReduce = append(messagesReduce, fmt.Sprintf("In field \"%v\": %v", fieldName, message))
 				}