From 847c88538b58b3ef7054b1551eaa9af679785c9b Mon Sep 17 00:00:00 2001 From: Akira HIGUCHI Date: Tue, 12 May 2026 22:05:35 +0900 Subject: [PATCH 01/10] fix: preserve absent vs explicit-null distinction in argument coercion Variables declared in an operation but not supplied by the caller used to arrive at resolvers as explicit nil, making it impossible to tell "field omitted" from "field explicitly null". Restore the three-state semantics required by the spec (CoerceArgumentValues / CoerceVariableValues) while keeping the existing behavior of preserving explicit nulls. - getVariableValues: only insert a coerced value when the caller supplied the variable or when the definition declares a default value. - getArgumentValues: treat an argument that resolves to an unprovided variable reference as absent, but still surface explicit nulls. - valueFromAST (InputObject): fields whose values come from unprovided variables stay absent in the resulting map. - Add argument_coercion_test.go covering the three states for scalars, input objects, and input-object literals. --- argument_coercion_test.go | 137 ++++++++++++++++++++++++++++++++++++++ values.go | 70 +++++++++---------- 2 files changed, 170 insertions(+), 37 deletions(-) create mode 100644 argument_coercion_test.go diff --git a/argument_coercion_test.go b/argument_coercion_test.go new file mode 100644 index 0000000..104c9d5 --- /dev/null +++ b/argument_coercion_test.go @@ -0,0 +1,137 @@ +package graphql_test + +import ( + "encoding/json" + "sort" + "testing" + + "github.com/tailor-platform/graphql" + "github.com/tailor-platform/graphql/testutil" +) + +// Serialises p.Args so tests can tell "absent", "null", and "value" apart. +func probeArgs(p graphql.ResolveParams) (interface{}, error) { + keys := make([]string, 0, len(p.Args)) + for k := range p.Args { + keys = append(keys, k) + } + sort.Strings(keys) + out := map[string]interface{}{"keys": keys} + for _, k := range keys { + v := p.Args[k] + if v == nil { + out[k] = "null" + } else { + out[k] = v + } + } + b, _ := json.Marshal(out) + return string(b), nil +} + +var coercionProbeInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeInput", + Fields: graphql.InputObjectConfigFieldMap{ + "a": &graphql.InputObjectFieldConfig{Type: graphql.String}, + "b": &graphql.InputObjectFieldConfig{Type: graphql.String}, + }, +}) + +var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ + Name: "CoercionProbeQuery", + Fields: graphql.Fields{ + "probe": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{Type: graphql.String}, + "b": &graphql.ArgumentConfig{Type: graphql.String}, + }, + Resolve: probeArgs, + }, + "probeObject": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeInputObject}, + }, + Resolve: func(p graphql.ResolveParams) (interface{}, error) { + obj, _ := p.Args["input"].(map[string]interface{}) + keys := make([]string, 0, len(obj)) + for k := range obj { + keys = append(keys, k) + } + sort.Strings(keys) + b, _ := json.Marshal(map[string]interface{}{ + "keys": keys, + "obj": obj, + }) + return string(b), nil + }, + }, + }, +}) + +var coercionProbeSchema, _ = graphql.NewSchema(graphql.SchemaConfig{Query: coercionProbeType}) + +func runProbe(t *testing.T, field, doc string, vars map[string]interface{}, want string) { + t.Helper() + parsed := testutil.TestParse(t, doc) + result := testutil.TestExecute(t, graphql.ExecuteParams{ + Schema: coercionProbeSchema, + AST: parsed, + Args: vars, + }) + if len(result.Errors) > 0 { + t.Fatalf("unexpected errors: %v", result.Errors) + } + data, _ := result.Data.(map[string]interface{}) + got, _ := data[field].(string) + if got != want { + t.Fatalf("probe mismatch\n got: %s\n want: %s", got, want) + } +} + +func TestArgumentCoercion_ScalarVariable_PreservesThreeStates(t *testing.T) { + doc := `query Probe($a: String, $b: String) { probe(a: $a, b: $b) }` + + t.Run("variable omitted -> argument absent", func(t *testing.T) { + runProbe(t, "probe", doc, + map[string]interface{}{"a": "x"}, + `{"a":"x","keys":["a"]}`) + }) + t.Run("variable explicitly null -> argument present as null", func(t *testing.T) { + runProbe(t, "probe", doc, + map[string]interface{}{"a": "x", "b": nil}, + `{"a":"x","b":"null","keys":["a","b"]}`) + }) + t.Run("variable with value -> argument present with value", func(t *testing.T) { + runProbe(t, "probe", doc, + map[string]interface{}{"a": "x", "b": "y"}, + `{"a":"x","b":"y","keys":["a","b"]}`) + }) +} + +func TestArgumentCoercion_InputObjectVariable_PreservesThreeStates(t *testing.T) { + doc := `query Probe($a: String, $b: String) { probeObject(input: {a: $a, b: $b}) }` + + t.Run("nested variable omitted -> field absent in object", func(t *testing.T) { + runProbe(t, "probeObject", doc, + map[string]interface{}{"a": "x"}, + `{"keys":["a"],"obj":{"a":"x"}}`) + }) + t.Run("nested variable explicitly null -> field present as null", func(t *testing.T) { + runProbe(t, "probeObject", doc, + map[string]interface{}{"a": "x", "b": nil}, + `{"keys":["a","b"],"obj":{"a":"x","b":null}}`) + }) + t.Run("nested variable with value -> field present with value", func(t *testing.T) { + runProbe(t, "probeObject", doc, + map[string]interface{}{"a": "x", "b": "y"}, + `{"keys":["a","b"],"obj":{"a":"x","b":"y"}}`) + }) +} + +func TestArgumentCoercion_InputObjectLiteral_OmittedFieldStaysAbsent(t *testing.T) { + doc := `{ probeObject(input: {a: "x"}) }` + runProbe(t, "probeObject", doc, nil, + `{"keys":["a"],"obj":{"a":"x"}}`) +} diff --git a/values.go b/values.go index 4ed4b46..3a49bc1 100644 --- a/values.go +++ b/values.go @@ -27,9 +27,12 @@ func getVariableValues( continue } varName := defAST.Variable.Name.Value - if varValue, err := getVariableValue(schema, defAST, inputs[varName]); err != nil { + input, provided := inputs[varName] + varValue, err := getVariableValue(schema, defAST, input) + if err != nil { return values, err - } else { + } + if provided || defAST.DefaultValue != nil { values[varName] = varValue } } @@ -50,28 +53,33 @@ func getArgumentValues( } results := map[string]interface{}{} for _, argDef := range argDefs { - var ( - tmp interface{} - value ast.Value - isUndefined bool - ) - if tmpValue, ok := argASTMap[argDef.PrivateName]; ok { - value = tmpValue.Value - } else { - isUndefined = true - } - if tmp = valueFromAST(value, argDef.Type, variableValues); isNullish(tmp) { + var value ast.Value + argAST, ok := argASTMap[argDef.PrivateName] + if ok { + value = argAST.Value + } + isUndefined := !ok || isUnprovidedVariable(value, variableValues) + tmp := valueFromAST(value, argDef.Type, variableValues) + if isNullish(tmp) { tmp = argDef.DefaultValue } - if !isUndefined && tmp == nil { - results[argDef.PrivateName] = nil - } else if !isNullish(tmp) { + if !isUndefined || !isNullish(tmp) { results[argDef.PrivateName] = tmp } } return results } +// Returns true if value is a reference to a variable the caller did not supply. +func isUnprovidedVariable(value ast.Value, variables map[string]interface{}) bool { + v, ok := value.(*ast.Variable) + if !ok || v.Name == nil { + return false + } + _, provided := variables[v.Name.Value] + return !provided +} + // Given a variable definition, and any value of input, return a value which // adheres to the variable definition, or throw an error. func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, input interface{}) (interface{}, error) { @@ -381,16 +389,12 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac } return append(values, valueFromAST(valueAST, ttype.OfType, variables)) case *InputObject: - var ( - ok bool - ov *ast.ObjectValue - of *ast.ObjectField - ) - if ov, ok = valueAST.(*ast.ObjectValue); !ok { + ov, ok := valueAST.(*ast.ObjectValue) + if !ok { return nil } fieldASTs := map[string]*ast.ObjectField{} - for _, of = range ov.Fields { + for _, of := range ov.Fields { if of == nil || of.Name == nil { continue } @@ -398,20 +402,12 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac } obj := map[string]interface{}{} for name, field := range ttype.Fields() { - var ( - value interface{} - isUndefined bool - ) - if of, ok = fieldASTs[name]; ok { - value = valueFromAST(of.Value, field.Type, variables) - } else { - isUndefined = true - value = field.DefaultValue - } - if !isUndefined && value == nil { - obj[name] = nil - } else if !isNullish(value) { - obj[name] = value + of, ok := fieldASTs[name] + supplied := ok && !isUnprovidedVariable(of.Value, variables) + if supplied { + obj[name] = valueFromAST(of.Value, field.Type, variables) + } else if !isNullish(field.DefaultValue) { + obj[name] = field.DefaultValue } } return obj From 8028a83ae9f2e0bc5679fb26afe9c696f3ea0bf8 Mon Sep 17 00:00:00 2001 From: ikawaha Date: Thu, 6 Aug 2026 22:49:44 +0900 Subject: [PATCH 02/10] fix: gate spec-compliant argument coercion behind SchemaConfig flag --- argument_coercion_test.go | 551 +++++++++++++++++++++- executor.go | 6 +- rules.go | 4 +- rules_provided_non_null_arguments_test.go | 93 ++++ schema.go | 26 + subscription.go | 2 +- values.go | 81 +++- values_test.go | 23 +- 8 files changed, 735 insertions(+), 51 deletions(-) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index 104c9d5..5d302d3 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -29,6 +29,22 @@ func probeArgs(p graphql.ResolveParams) (interface{}, error) { return string(b), nil } +// Serialises the "input" argument so tests can tell an absent input-object +// field apart from one present as null. +func probeObjectArgs(p graphql.ResolveParams) (interface{}, error) { + obj, _ := p.Args["input"].(map[string]interface{}) + keys := make([]string, 0, len(obj)) + for k := range obj { + keys = append(keys, k) + } + sort.Strings(keys) + b, _ := json.Marshal(map[string]interface{}{ + "keys": keys, + "obj": obj, + }) + return string(b), nil +} + var coercionProbeInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ Name: "CoercionProbeInput", Fields: graphql.InputObjectConfigFieldMap{ @@ -37,6 +53,40 @@ var coercionProbeInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ }, }) +// Same shape as coercionProbeInputObject, but field "a" declares a default so +// tests can pin down how a default interacts with absent / explicit null. +var coercionProbeDefaultInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeDefaultInput", + Fields: graphql.InputObjectConfigFieldMap{ + "a": &graphql.InputObjectFieldConfig{Type: graphql.String, DefaultValue: "FIELDDEF"}, + "b": &graphql.InputObjectFieldConfig{Type: graphql.String}, + }, +}) + +var coercionProbeNestedInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeNestedInput", + Fields: graphql.InputObjectConfigFieldMap{ + "inner": &graphql.InputObjectFieldConfig{Type: coercionProbeInputObject}, + }, +}) + +// Two levels deep, with the default declared on the innermost field, so the +// recursive coercion paths are exercised rather than just the top level. +var coercionProbeNestedDefaultInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeNestedDefaultInput", + Fields: graphql.InputObjectConfigFieldMap{ + "inner": &graphql.InputObjectFieldConfig{Type: coercionProbeDefaultInputObject}, + }, +}) + +// Three levels deep: proves the rule keeps holding as recursion gets deeper. +var coercionProbeDeepInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeDeepInput", + Fields: graphql.InputObjectConfigFieldMap{ + "level2": &graphql.InputObjectFieldConfig{Type: coercionProbeNestedDefaultInputObject}, + }, +}) + var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ Name: "CoercionProbeQuery", Fields: graphql.Fields{ @@ -48,35 +98,103 @@ var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ }, Resolve: probeArgs, }, + "probeArgDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{Type: graphql.String, DefaultValue: "ARGDEF"}, + }, + Resolve: probeArgs, + }, + // Non-null argument carrying a default: spec §5.4.2.1 says it is + // optional, so omitting it must validate and resolve to the default. + "probeNonNullDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: "NNDEF", + }, + }, + Resolve: probeArgs, + }, + "probeList": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{Type: graphql.NewList(graphql.String)}, + }, + Resolve: probeArgs, + }, "probeObject": &graphql.Field{ Type: graphql.String, Args: graphql.FieldConfigArgument{ "input": &graphql.ArgumentConfig{Type: coercionProbeInputObject}, }, - Resolve: func(p graphql.ResolveParams) (interface{}, error) { - obj, _ := p.Args["input"].(map[string]interface{}) - keys := make([]string, 0, len(obj)) - for k := range obj { - keys = append(keys, k) - } - sort.Strings(keys) - b, _ := json.Marshal(map[string]interface{}{ - "keys": keys, - "obj": obj, - }) - return string(b), nil + Resolve: probeObjectArgs, + }, + "probeObjectDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeDefaultInputObject}, + }, + Resolve: probeObjectArgs, + }, + "probeNested": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeNestedInputObject}, }, + Resolve: probeObjectArgs, + }, + "probeNestedDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeNestedDefaultInputObject}, + }, + Resolve: probeObjectArgs, + }, + "probeDeep": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeDeepInputObject}, + }, + Resolve: probeObjectArgs, + }, + "probeObjectList": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: graphql.NewList(coercionProbeDefaultInputObject)}, + }, + Resolve: probeArgs, + }, + // Same argument as probeObject, but serialised with probeArgs so tests + // can tell an absent input-object argument from one that is null. + "probeObjectRaw": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeInputObject}, + }, + Resolve: probeArgs, }, }, }) -var coercionProbeSchema, _ = graphql.NewSchema(graphql.SchemaConfig{Query: coercionProbeType}) +// The same probe types under both coercion modes. SpecCompliantArgumentCoercion +// is opt-in, so the zero-valued config is the behaviour shipped before this +// change and must stay byte-for-byte identical. +var coercionProbeLegacySchema, _ = graphql.NewSchema(graphql.SchemaConfig{ + Query: coercionProbeType, +}) -func runProbe(t *testing.T, field, doc string, vars map[string]interface{}, want string) { +var coercionProbeSpecSchema, _ = graphql.NewSchema(graphql.SchemaConfig{ + Query: coercionProbeType, + SpecCompliantArgumentCoercion: true, +}) + +func execProbe(t *testing.T, schema graphql.Schema, field, doc string, vars map[string]interface{}) string { t.Helper() parsed := testutil.TestParse(t, doc) result := testutil.TestExecute(t, graphql.ExecuteParams{ - Schema: coercionProbeSchema, + Schema: schema, AST: parsed, Args: vars, }) @@ -85,8 +203,25 @@ func runProbe(t *testing.T, field, doc string, vars map[string]interface{}, want } data, _ := result.Data.(map[string]interface{}) got, _ := data[field].(string) - if got != want { - t.Fatalf("probe mismatch\n got: %s\n want: %s", got, want) + return got +} + +// runProbe asserts both coercion modes agree — the cases the flag does not +// change. +func runProbe(t *testing.T, field, doc string, vars map[string]interface{}, want string) { + t.Helper() + runProbeModes(t, field, doc, vars, want, want) +} + +// runProbeModes pins down a case where the flag changes the outcome: the legacy +// column is the regression guard, the spec column is the fix. +func runProbeModes(t *testing.T, field, doc string, vars map[string]interface{}, wantLegacy, wantSpec string) { + t.Helper() + if got := execProbe(t, coercionProbeLegacySchema, field, doc, vars); got != wantLegacy { + t.Errorf("legacy mode mismatch\n got: %s\n want: %s", got, wantLegacy) + } + if got := execProbe(t, coercionProbeSpecSchema, field, doc, vars); got != wantSpec { + t.Errorf("spec mode mismatch\n got: %s\n want: %s", got, wantSpec) } } @@ -94,8 +229,9 @@ func TestArgumentCoercion_ScalarVariable_PreservesThreeStates(t *testing.T) { doc := `query Probe($a: String, $b: String) { probe(a: $a, b: $b) }` t.Run("variable omitted -> argument absent", func(t *testing.T) { - runProbe(t, "probe", doc, + runProbeModes(t, "probe", doc, map[string]interface{}{"a": "x"}, + `{"a":"x","b":"null","keys":["a","b"]}`, `{"a":"x","keys":["a"]}`) }) t.Run("variable explicitly null -> argument present as null", func(t *testing.T) { @@ -114,8 +250,9 @@ func TestArgumentCoercion_InputObjectVariable_PreservesThreeStates(t *testing.T) doc := `query Probe($a: String, $b: String) { probeObject(input: {a: $a, b: $b}) }` t.Run("nested variable omitted -> field absent in object", func(t *testing.T) { - runProbe(t, "probeObject", doc, + runProbeModes(t, "probeObject", doc, map[string]interface{}{"a": "x"}, + `{"keys":["a","b"],"obj":{"a":"x","b":null}}`, `{"keys":["a"],"obj":{"a":"x"}}`) }) t.Run("nested variable explicitly null -> field present as null", func(t *testing.T) { @@ -135,3 +272,379 @@ func TestArgumentCoercion_InputObjectLiteral_OmittedFieldStaysAbsent(t *testing. runProbe(t, "probeObject", doc, nil, `{"keys":["a"],"obj":{"a":"x"}}`) } + +func TestArgumentCoercion_ScalarArgument_OmittedFromQueryStaysAbsent(t *testing.T) { + // The argument is not written in the query at all: CoerceArgumentValues + // leaves hasValue false and there is no default, so nothing is added. + runProbe(t, "probe", `{ probe(a: "x") }`, nil, + `{"a":"x","keys":["a"]}`) +} + +// Spec: CoerceArgumentValues (§6.4.1). The argument default applies only when +// the caller supplied no value at all. An explicit null is a supplied value and +// must survive as null rather than fall back to the default. +func TestArgumentCoercion_ArgumentDefault_AppliesOnlyWhenValueAbsent(t *testing.T) { + doc := `query Probe($a: String) { probeArgDefault(a: $a) }` + + t.Run("argument omitted from query -> default", func(t *testing.T) { + runProbe(t, "probeArgDefault", `{ probeArgDefault }`, nil, + `{"a":"ARGDEF","keys":["a"]}`) + }) + t.Run("variable omitted -> default", func(t *testing.T) { + runProbe(t, "probeArgDefault", doc, + map[string]interface{}{}, + `{"a":"ARGDEF","keys":["a"]}`) + }) + t.Run("variable explicitly null -> null, not the default", func(t *testing.T) { + runProbeModes(t, "probeArgDefault", doc, + map[string]interface{}{"a": nil}, + `{"a":"ARGDEF","keys":["a"]}`, + `{"a":"null","keys":["a"]}`) + }) + t.Run("variable with value -> value", func(t *testing.T) { + runProbe(t, "probeArgDefault", doc, + map[string]interface{}{"a": "v"}, + `{"a":"v","keys":["a"]}`) + }) +} + +// Spec: input object field defaults (§3.10 Input Coercion), reached through an +// object literal written in the query document. +func TestArgumentCoercion_InputObjectLiteralFieldDefault_AppliesOnlyWhenValueAbsent(t *testing.T) { + doc := `query Probe($a: String) { probeObjectDefault(input: {a: $a}) }` + + t.Run("field omitted from literal -> default", func(t *testing.T) { + runProbe(t, "probeObjectDefault", `{ probeObjectDefault(input: {}) }`, nil, + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`) + }) + t.Run("nested variable omitted -> default", func(t *testing.T) { + runProbeModes(t, "probeObjectDefault", doc, + map[string]interface{}{}, + `{"keys":["a"],"obj":{"a":null}}`, + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`) + }) + t.Run("nested variable explicitly null -> null, not the default", func(t *testing.T) { + runProbe(t, "probeObjectDefault", doc, + map[string]interface{}{"a": nil}, + `{"keys":["a"],"obj":{"a":null}}`) + }) + t.Run("nested variable with value -> value", func(t *testing.T) { + runProbe(t, "probeObjectDefault", doc, + map[string]interface{}{"a": "v"}, + `{"keys":["a"],"obj":{"a":"v"}}`) + }) +} + +// The whole input object arrives as one variable, so presence is decided by +// whether the key exists in the supplied JSON object (coerceValue path). +func TestArgumentCoercion_WholeObjectVariable_PreservesThreeStates(t *testing.T) { + doc := `query Probe($in: CoercionProbeInput) { probeObject(input: $in) }` + + t.Run("key absent in object -> field absent", func(t *testing.T) { + runProbe(t, "probeObject", doc, + map[string]interface{}{"in": map[string]interface{}{"a": "x"}}, + `{"keys":["a"],"obj":{"a":"x"}}`) + }) + t.Run("key explicitly null -> field present as null", func(t *testing.T) { + runProbe(t, "probeObject", doc, + map[string]interface{}{"in": map[string]interface{}{"a": "x", "b": nil}}, + `{"keys":["a","b"],"obj":{"a":"x","b":null}}`) + }) + t.Run("key with value -> field present with value", func(t *testing.T) { + runProbe(t, "probeObject", doc, + map[string]interface{}{"in": map[string]interface{}{"a": "x", "b": "y"}}, + `{"keys":["a","b"],"obj":{"a":"x","b":"y"}}`) + }) +} + +func TestArgumentCoercion_WholeObjectVariableFieldDefault_AppliesOnlyWhenValueAbsent(t *testing.T) { + doc := `query Probe($in: CoercionProbeDefaultInput) { probeObjectDefault(input: $in) }` + + t.Run("key absent in object -> default", func(t *testing.T) { + runProbe(t, "probeObjectDefault", doc, + map[string]interface{}{"in": map[string]interface{}{}}, + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`) + }) + t.Run("key explicitly null -> null, not the default", func(t *testing.T) { + runProbeModes(t, "probeObjectDefault", doc, + map[string]interface{}{"in": map[string]interface{}{"a": nil}}, + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`, + `{"keys":["a"],"obj":{"a":null}}`) + }) + t.Run("key with value -> value", func(t *testing.T) { + runProbe(t, "probeObjectDefault", doc, + map[string]interface{}{"in": map[string]interface{}{"a": "v"}}, + `{"keys":["a"],"obj":{"a":"v"}}`) + }) +} + +// Spec: CoerceVariableValues (§6.1.2). Same rule one level up — the variable +// default applies only when the caller supplied no value for that variable. +func TestArgumentCoercion_VariableDefault_AppliesOnlyWhenValueAbsent(t *testing.T) { + doc := `query Probe($a: String = "VARDEF") { probe(a: $a) }` + + t.Run("variable omitted -> variable default", func(t *testing.T) { + runProbe(t, "probe", doc, + map[string]interface{}{}, + `{"a":"VARDEF","keys":["a"]}`) + }) + t.Run("variable explicitly null -> null, not the default", func(t *testing.T) { + runProbeModes(t, "probe", doc, + map[string]interface{}{"a": nil}, + `{"a":"VARDEF","keys":["a"]}`, + `{"a":"null","keys":["a"]}`) + }) + t.Run("variable with value -> value", func(t *testing.T) { + runProbe(t, "probe", doc, + map[string]interface{}{"a": "v"}, + `{"a":"v","keys":["a"]}`) + }) +} + +// A variable default and an argument default in the same position: the variable +// default wins because it makes the argument "supplied". +func TestArgumentCoercion_VariableDefaultTakesPrecedenceOverArgumentDefault(t *testing.T) { + doc := `query Probe($a: String = "VARDEF") { probeArgDefault(a: $a) }` + + t.Run("variable omitted -> variable default, not argument default", func(t *testing.T) { + runProbe(t, "probeArgDefault", doc, + map[string]interface{}{}, + `{"a":"VARDEF","keys":["a"]}`) + }) + t.Run("variable explicitly null -> null, neither default", func(t *testing.T) { + runProbeModes(t, "probeArgDefault", doc, + map[string]interface{}{"a": nil}, + `{"a":"VARDEF","keys":["a"]}`, + `{"a":"null","keys":["a"]}`) + }) +} + +func TestArgumentCoercion_ListArgument_PreservesThreeStates(t *testing.T) { + doc := `query Probe($a: [String]) { probeList(a: $a) }` + + t.Run("variable omitted -> argument absent", func(t *testing.T) { + runProbeModes(t, "probeList", doc, + map[string]interface{}{}, + `{"a":"null","keys":["a"]}`, + `{"keys":[]}`) + }) + t.Run("variable explicitly null -> argument present as null", func(t *testing.T) { + runProbe(t, "probeList", doc, + map[string]interface{}{"a": nil}, + `{"a":"null","keys":["a"]}`) + }) + t.Run("variable with value -> argument present with value", func(t *testing.T) { + runProbe(t, "probeList", doc, + map[string]interface{}{"a": []interface{}{"x"}}, + `{"a":["x"],"keys":["a"]}`) + }) + t.Run("unprovided variable inside a list literal -> null item", func(t *testing.T) { + runProbe(t, "probeList", `query Probe($v: String) { probeList(a: ["x", $v]) }`, + map[string]interface{}{}, + `{"a":["x",null],"keys":["a"]}`) + }) +} + +func TestArgumentCoercion_NestedInputObject_PreservesAbsentAndNull(t *testing.T) { + t.Run("literal: unprovided variable in nested object stays absent", func(t *testing.T) { + runProbeModes(t, "probeNested", + `query Probe($a: String) { probeNested(input: {inner: {a: $a}}) }`, + map[string]interface{}{}, + `{"keys":["inner"],"obj":{"inner":{"a":null}}}`, + `{"keys":["inner"],"obj":{"inner":{}}}`) + }) + t.Run("literal: explicit null in nested object stays null", func(t *testing.T) { + runProbe(t, "probeNested", + `query Probe($a: String) { probeNested(input: {inner: {a: $a}}) }`, + map[string]interface{}{"a": nil}, + `{"keys":["inner"],"obj":{"inner":{"a":null}}}`) + }) + t.Run("whole variable: absent nested key stays absent", func(t *testing.T) { + runProbe(t, "probeNested", + `query Probe($in: CoercionProbeNestedInput) { probeNested(input: $in) }`, + map[string]interface{}{"in": map[string]interface{}{"inner": map[string]interface{}{}}}, + `{"keys":["inner"],"obj":{"inner":{}}}`) + }) + t.Run("whole variable: explicit null nested key stays null", func(t *testing.T) { + runProbe(t, "probeNested", + `query Probe($in: CoercionProbeNestedInput) { probeNested(input: $in) }`, + map[string]interface{}{"in": map[string]interface{}{"inner": map[string]interface{}{"a": nil}}}, + `{"keys":["inner"],"obj":{"inner":{"a":null}}}`) + }) +} + +// The nested object itself — not one of its fields — is the thing that is +// absent or null. +func TestArgumentCoercion_NestedInputObject_ObjectValuedFieldAbsentVsNull(t *testing.T) { + litDoc := `query Probe($innerVar: CoercionProbeInput) { probeNested(input: {inner: $innerVar}) }` + varDoc := `query Probe($in: CoercionProbeNestedInput) { probeNested(input: $in) }` + + t.Run("literal: unprovided object variable -> field absent", func(t *testing.T) { + runProbeModes(t, "probeNested", litDoc, + map[string]interface{}{}, + `{"keys":["inner"],"obj":{"inner":null}}`, + `{"keys":[],"obj":{}}`) + }) + t.Run("literal: explicitly null object variable -> field present as null", func(t *testing.T) { + runProbe(t, "probeNested", litDoc, + map[string]interface{}{"innerVar": nil}, + `{"keys":["inner"],"obj":{"inner":null}}`) + }) + t.Run("whole variable: object key absent -> field absent", func(t *testing.T) { + runProbe(t, "probeNested", varDoc, + map[string]interface{}{"in": map[string]interface{}{}}, + `{"keys":[],"obj":{}}`) + }) + t.Run("whole variable: object key explicitly null -> field present as null", func(t *testing.T) { + runProbe(t, "probeNested", varDoc, + map[string]interface{}{"in": map[string]interface{}{"inner": nil}}, + `{"keys":["inner"],"obj":{"inner":null}}`) + }) +} + +// The input-object argument itself is absent or null, one level above the +// object's fields. +func TestArgumentCoercion_InputObjectArgument_AbsentVsNull(t *testing.T) { + doc := `query Probe($in: CoercionProbeInput) { probeObjectRaw(input: $in) }` + + t.Run("variable omitted -> argument absent", func(t *testing.T) { + runProbeModes(t, "probeObjectRaw", doc, + map[string]interface{}{}, + `{"input":"null","keys":["input"]}`, + `{"keys":[]}`) + }) + t.Run("variable explicitly null -> argument present as null", func(t *testing.T) { + runProbe(t, "probeObjectRaw", doc, + map[string]interface{}{"in": nil}, + `{"input":"null","keys":["input"]}`) + }) + t.Run("variable with value -> argument present with value", func(t *testing.T) { + runProbe(t, "probeObjectRaw", doc, + map[string]interface{}{"in": map[string]interface{}{"a": "x"}}, + `{"input":{"a":"x"},"keys":["input"]}`) + }) +} + +// The default lives on a field one level down, so this only passes if the +// absent/null rule is applied by the recursive step and not just at the top. +func TestArgumentCoercion_NestedFieldDefault_AppliesOnlyWhenValueAbsent(t *testing.T) { + litDoc := `query Probe($a: String) { probeNestedDefault(input: {inner: {a: $a}}) }` + varDoc := `query Probe($in: CoercionProbeNestedDefaultInput) { probeNestedDefault(input: $in) }` + + t.Run("literal: nested field omitted -> default", func(t *testing.T) { + runProbe(t, "probeNestedDefault", + `{ probeNestedDefault(input: {inner: {}}) }`, nil, + `{"keys":["inner"],"obj":{"inner":{"a":"FIELDDEF"}}}`) + }) + t.Run("literal: nested variable omitted -> default", func(t *testing.T) { + runProbeModes(t, "probeNestedDefault", litDoc, + map[string]interface{}{}, + `{"keys":["inner"],"obj":{"inner":{"a":null}}}`, + `{"keys":["inner"],"obj":{"inner":{"a":"FIELDDEF"}}}`) + }) + t.Run("literal: nested variable explicitly null -> null, not the default", func(t *testing.T) { + runProbe(t, "probeNestedDefault", litDoc, + map[string]interface{}{"a": nil}, + `{"keys":["inner"],"obj":{"inner":{"a":null}}}`) + }) + t.Run("whole variable: nested key absent -> default", func(t *testing.T) { + runProbe(t, "probeNestedDefault", varDoc, + map[string]interface{}{"in": map[string]interface{}{"inner": map[string]interface{}{}}}, + `{"keys":["inner"],"obj":{"inner":{"a":"FIELDDEF"}}}`) + }) + t.Run("whole variable: nested key explicitly null -> null, not the default", func(t *testing.T) { + runProbeModes(t, "probeNestedDefault", varDoc, + map[string]interface{}{"in": map[string]interface{}{"inner": map[string]interface{}{"a": nil}}}, + `{"keys":["inner"],"obj":{"inner":{"a":"FIELDDEF"}}}`, + `{"keys":["inner"],"obj":{"inner":{"a":null}}}`) + }) +} + +func TestArgumentCoercion_DeeplyNestedFieldDefault_AppliesOnlyWhenValueAbsent(t *testing.T) { + litDoc := `query Probe($a: String) { probeDeep(input: {level2: {inner: {a: $a}}}) }` + varDoc := `query Probe($in: CoercionProbeDeepInput) { probeDeep(input: $in) }` + + t.Run("literal: three levels down, variable omitted -> default", func(t *testing.T) { + runProbeModes(t, "probeDeep", litDoc, + map[string]interface{}{}, + `{"keys":["level2"],"obj":{"level2":{"inner":{"a":null}}}}`, + `{"keys":["level2"],"obj":{"level2":{"inner":{"a":"FIELDDEF"}}}}`) + }) + t.Run("literal: three levels down, explicitly null -> null, not the default", func(t *testing.T) { + runProbe(t, "probeDeep", litDoc, + map[string]interface{}{"a": nil}, + `{"keys":["level2"],"obj":{"level2":{"inner":{"a":null}}}}`) + }) + t.Run("whole variable: three levels down, key absent -> default", func(t *testing.T) { + runProbe(t, "probeDeep", varDoc, + map[string]interface{}{"in": map[string]interface{}{ + "level2": map[string]interface{}{"inner": map[string]interface{}{}}, + }}, + `{"keys":["level2"],"obj":{"level2":{"inner":{"a":"FIELDDEF"}}}}`) + }) + t.Run("whole variable: three levels down, explicitly null -> null, not the default", func(t *testing.T) { + runProbeModes(t, "probeDeep", varDoc, + map[string]interface{}{"in": map[string]interface{}{ + "level2": map[string]interface{}{"inner": map[string]interface{}{"a": nil}}, + }}, + `{"keys":["level2"],"obj":{"level2":{"inner":{"a":"FIELDDEF"}}}}`, + `{"keys":["level2"],"obj":{"level2":{"inner":{"a":null}}}}`) + }) +} + +// Input objects inside a list: the recursive step runs per element, so each +// element must keep its own absent / null / value state. +func TestArgumentCoercion_ListOfInputObjects_PreservesPerElementState(t *testing.T) { + t.Run("literal: element field omitted -> default", func(t *testing.T) { + runProbeModes(t, "probeObjectList", + `query Probe($a: String) { probeObjectList(input: [{a: $a}]) }`, + map[string]interface{}{}, + `{"input":[{"a":null}],"keys":["input"]}`, + `{"input":[{"a":"FIELDDEF"}],"keys":["input"]}`) + }) + t.Run("literal: element field explicitly null -> null, not the default", func(t *testing.T) { + runProbe(t, "probeObjectList", + `query Probe($a: String) { probeObjectList(input: [{a: $a}]) }`, + map[string]interface{}{"a": nil}, + `{"input":[{"a":null}],"keys":["input"]}`) + }) + t.Run("whole variable: per-element null and absent are independent", func(t *testing.T) { + runProbeModes(t, "probeObjectList", + `query Probe($in: [CoercionProbeDefaultInput]) { probeObjectList(input: $in) }`, + map[string]interface{}{"in": []interface{}{ + map[string]interface{}{"a": nil}, + map[string]interface{}{}, + }}, + `{"input":[{"a":"FIELDDEF"},{"a":"FIELDDEF"}],"keys":["input"]}`, + `{"input":[{"a":null},{"a":"FIELDDEF"}],"keys":["input"]}`) + }) +} + +// Spec §5.4.2.1, end to end through graphql.Do so document validation runs too. +// This relaxation is not gated by SpecCompliantArgumentCoercion: it only lets +// queries through that previously failed, so it cannot break a working one. +func TestArgumentCoercion_NonNullArgumentWithDefault_IsOptionalInBothModes(t *testing.T) { + for _, tc := range []struct { + mode string + schema graphql.Schema + }{ + {"legacy", coercionProbeLegacySchema}, + {"spec", coercionProbeSpecSchema}, + } { + t.Run(tc.mode, func(t *testing.T) { + result := graphql.Do(graphql.Params{ + Schema: tc.schema, + RequestString: `{ probeNonNullDefault }`, + }) + if len(result.Errors) > 0 { + t.Fatalf("unexpected errors: %v", result.Errors) + } + data, _ := result.Data.(map[string]interface{}) + got, _ := data["probeNonNullDefault"].(string) + want := `{"a":"NNDEF","keys":["a"]}` + if got != want { + t.Fatalf("probe mismatch\n got: %s\n want: %s", got, want) + } + }) + } +} diff --git a/executor.go b/executor.go index 0b35b07..889788f 100644 --- a/executor.go +++ b/executor.go @@ -498,13 +498,13 @@ func shouldIncludeNode(eCtx *executionContext, directives []*ast.Directive) bool } // precedence: skipAST > includeAST if skipAST != nil { - argValues = getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues) + argValues = getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, eCtx.Schema.specCompliantArgumentCoercion) if skipIf, ok := argValues["if"].(bool); ok && skipIf { return false // excluded selectionSet's fields } } if includeAST != nil { - argValues = getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues) + argValues = getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, eCtx.Schema.specCompliantArgumentCoercion) if includeIf, ok := argValues["if"].(bool); ok && !includeIf { return false // excluded selectionSet's fields } @@ -624,7 +624,7 @@ 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 := getArgumentValues(fieldDef.Args, fieldAST.Arguments, eCtx.VariableValues, eCtx.Schema.specCompliantArgumentCoercion) info := ResolveInfo{ FieldName: fieldName, diff --git a/rules.go b/rules.go index 4fc35f3..7268c60 100644 --- a/rules.go +++ b/rules.go @@ -1271,7 +1271,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 && argDef.DefaultValue == nil { fieldName := "" if fieldAST.Name != nil { fieldName = fieldAST.Name.Value @@ -1312,7 +1312,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 && argDef.DefaultValue == nil { directiveName := "" if directiveAST.Name != nil { directiveName = directiveAST.Name.Value diff --git a/rules_provided_non_null_arguments_test.go b/rules_provided_non_null_arguments_test.go index dc3c055..195e9d5 100644 --- a/rules_provided_non_null_arguments_test.go +++ b/rules_provided_non_null_arguments_test.go @@ -175,3 +175,96 @@ func TestValidate_ProvidedNonNullArguments_DirectiveArguments_WithDirectiveWithM testutil.RuleError(`Directive "@skip" argument "if" of type "Boolean!" is required but not provided.`, 4, 18), }) } + +// Spec §5.4.2.1: "An argument is required if the argument type is non-null and +// does not have a default value. Otherwise, the argument is optional." +// See graphql-go/graphql#739. +func TestValidate_ProvidedNonNullArguments_FieldArguments_NoErrorOnNonNullArgumentWithDefaultValue(t *testing.T) { + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "fieldWithDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "arg": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.Boolean), + DefaultValue: true, + }, + }, + }, + }, + }), + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` + { + fieldWithDefault + } + `) +} + +func TestValidate_ProvidedNonNullArguments_FieldArguments_StillErrorsOnNonNullArgumentWithoutDefaultValue(t *testing.T) { + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "fieldWithoutDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "arg": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.Boolean), + }, + }, + }, + }, + }), + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` + { + fieldWithoutDefault + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Field "fieldWithoutDefault" argument "arg" of type "Boolean!" is required but not provided.`, 3, 11), + }) +} + +func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NoErrorOnNonNullArgumentWithDefaultValue(t *testing.T) { + deferDirective := graphql.NewDirective(graphql.DirectiveConfig{ + Name: "defer", + Locations: []string{ + graphql.DirectiveLocationFragmentSpread, + graphql.DirectiveLocationInlineFragment, + }, + Args: graphql.FieldConfigArgument{ + "if": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.Boolean), + DefaultValue: true, + }, + }, + }) + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "a": &graphql.Field{Type: graphql.String}, + }, + }), + Directives: []*graphql.Directive{deferDirective}, + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` + { + ... on Query @defer { + a + } + } + `) +} diff --git a/schema.go b/schema.go index f4d7484..4943a8c 100644 --- a/schema.go +++ b/schema.go @@ -7,6 +7,23 @@ type SchemaConfig struct { Types []Type Directives []*Directive Extensions []Extension + + // SpecCompliantArgumentCoercion opts this schema into the argument and + // variable coercion rules described by the GraphQL specification + // (CoerceArgumentValues §6.4.1, CoerceVariableValues §6.1.2 and input + // object coercion §3.10): + // + // - A variable the caller did not supply leaves its argument absent from + // ResolveParams.Args instead of materialising it as nil, so a resolver + // can tell "not provided" from "explicitly null". + // - A default value applies only when no value was supplied. An explicit + // null stays null instead of falling back to the default. + // + // It is opt-in because both rules change what resolvers observe: code + // written against the previous behaviour may rely on every declared + // argument being present, or on an explicit null being replaced by the + // default. Leaving this false keeps that behaviour byte-for-byte. + SpecCompliantArgumentCoercion bool } type TypeMap map[string]Type @@ -43,6 +60,8 @@ type Schema struct { implementations map[string][]*Object possibleTypeMap map[string]map[string]bool extensions []Extension + + specCompliantArgumentCoercion bool } func NewSchema(config SchemaConfig) (Schema, error) { @@ -65,6 +84,7 @@ func NewSchema(config SchemaConfig) (Schema, error) { schema.queryType = config.Query schema.mutationType = config.Mutation schema.subscriptionType = config.Subscription + schema.specCompliantArgumentCoercion = config.SpecCompliantArgumentCoercion // Provide specified directives (e.g. @include and @skip) by default. schema.directives = config.Directives @@ -210,6 +230,12 @@ func (gq *Schema) SubscriptionType() *Object { return gq.subscriptionType } +// SpecCompliantArgumentCoercion reports whether this schema coerces arguments +// and variables by the specification's rules. See SchemaConfig for details. +func (gq *Schema) SpecCompliantArgumentCoercion() bool { + return gq.specCompliantArgumentCoercion +} + func (gq *Schema) Directives() []*Directive { return gq.directives } diff --git a/subscription.go b/subscription.go index bdfd282..64946f0 100644 --- a/subscription.go +++ b/subscription.go @@ -166,7 +166,7 @@ func ExecuteSubscription(p ExecuteParams) chan *Result { Key: responseName, } - args := getArgumentValues(fieldDef.Args, fieldNode.Arguments, exeContext.VariableValues) + args := getArgumentValues(fieldDef.Args, fieldNode.Arguments, exeContext.VariableValues, exeContext.Schema.specCompliantArgumentCoercion) info := ResolveInfo{ FieldName: fieldName, FieldASTs: fieldNodes, diff --git a/values.go b/values.go index 3a49bc1..7ea15f7 100644 --- a/values.go +++ b/values.go @@ -28,11 +28,11 @@ func getVariableValues( } varName := defAST.Variable.Name.Value input, provided := inputs[varName] - varValue, err := getVariableValue(schema, defAST, input) + varValue, err := getVariableValue(schema, defAST, input, provided) if err != nil { return values, err } - if provided || defAST.DefaultValue != nil { + if !schema.specCompliantArgumentCoercion || provided || defAST.DefaultValue != nil { values[varName] = varValue } } @@ -43,7 +43,7 @@ func getVariableValues( // definitions and list of argument AST nodes. func getArgumentValues( argDefs []*Argument, argASTs []*ast.Argument, - variableValues map[string]interface{}) map[string]interface{} { + variableValues map[string]interface{}, specCompliant bool) map[string]interface{} { argASTMap := map[string]*ast.Argument{} for _, argAST := range argASTs { @@ -58,9 +58,25 @@ func getArgumentValues( if ok { value = argAST.Value } + if !specCompliant { + isUndefined := !ok + tmp := valueFromAST(value, argDef.Type, variableValues, specCompliant) + if isNullish(tmp) { + tmp = argDef.DefaultValue + } + if !isUndefined && tmp == nil { + results[argDef.PrivateName] = nil + } else if !isNullish(tmp) { + results[argDef.PrivateName] = tmp + } + continue + } + // hasValue is false when the argument is not written in the query, or + // when it references a variable the caller did not supply. Only then + // does the default apply — an explicit null is a supplied value. isUndefined := !ok || isUnprovidedVariable(value, variableValues) - tmp := valueFromAST(value, argDef.Type, variableValues) - if isNullish(tmp) { + tmp := valueFromAST(value, argDef.Type, variableValues, specCompliant) + if isUndefined && isNullish(tmp) { tmp = argDef.DefaultValue } if !isUndefined || !isNullish(tmp) { @@ -82,7 +98,8 @@ func isUnprovidedVariable(value ast.Value, variables map[string]interface{}) boo // Given a variable definition, and any value of input, return a value which // adheres to the variable definition, or throw an error. -func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, input interface{}) (interface{}, error) { +func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, input interface{}, provided bool) (interface{}, error) { + specCompliant := schema.specCompliantArgumentCoercion ttype, err := typeFromAST(schema, definitionAST.Type) if err != nil { return nil, err @@ -104,11 +121,14 @@ func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, inpu isValid, messages := isValidInputValue(input, ttype) if isValid { if isNullish(input) { - if definitionAST.DefaultValue != nil { - return valueFromAST(definitionAST.DefaultValue, ttype, nil), nil + // The default stands in for a value the caller did not supply. In + // spec-compliant mode an explicitly supplied null is a value, so it + // must not be replaced by the default. + if definitionAST.DefaultValue != nil && !(specCompliant && provided) { + return valueFromAST(definitionAST.DefaultValue, ttype, nil, specCompliant), nil } } - return coerceValue(ttype, input), nil + return coerceValue(ttype, input, specCompliant), nil } if isNullish(input) { return "", gqlerrors.NewError( @@ -143,24 +163,24 @@ func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, inpu } // Given a type and any value, return a runtime value coerced to match the type. -func coerceValue(ttype Input, value interface{}) interface{} { +func coerceValue(ttype Input, value interface{}, specCompliant bool) interface{} { if isNullish(value) { return nil } switch ttype := ttype.(type) { case *NonNull: - return coerceValue(ttype.OfType, value) + return coerceValue(ttype.OfType, value, specCompliant) case *List: var values = []interface{}{} valType := reflect.ValueOf(value) if valType.Kind() == reflect.Slice { for i := 0; i < valType.Len(); i++ { val := valType.Index(i).Interface() - values = append(values, coerceValue(ttype.OfType, val)) + values = append(values, coerceValue(ttype.OfType, val, specCompliant)) } return values } - return append(values, coerceValue(ttype.OfType, value)) + return append(values, coerceValue(ttype.OfType, value, specCompliant)) case *InputObject: var obj = map[string]interface{}{} valueMap, _ := value.(map[string]interface{}) @@ -173,7 +193,13 @@ func coerceValue(ttype Input, value interface{}) interface{} { if !ok && isNullish(field.DefaultValue) { continue } - fieldValue := coerceValue(field.Type, v) + // The key is present and holds null: the caller supplied a value, so + // the field's default must not stand in for it. + if specCompliant && ok && isNullish(v) { + obj[name] = nil + continue + } + fieldValue := coerceValue(field.Type, v, specCompliant) if isNullish(fieldValue) { fieldValue = field.DefaultValue } @@ -362,7 +388,7 @@ func isIterable(src interface{}) bool { * | Int / Float | Number | * */ -func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interface{}) interface{} { +func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interface{}, specCompliant bool) interface{} { if valueAST == nil { return nil } @@ -378,16 +404,16 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac } switch ttype := ttype.(type) { case *NonNull: - return valueFromAST(valueAST, ttype.OfType, variables) + return valueFromAST(valueAST, ttype.OfType, variables, specCompliant) case *List: values := []interface{}{} if valueAST, ok := valueAST.(*ast.ListValue); ok { for _, itemAST := range valueAST.Values { - values = append(values, valueFromAST(itemAST, ttype.OfType, variables)) + values = append(values, valueFromAST(itemAST, ttype.OfType, variables, specCompliant)) } return values } - return append(values, valueFromAST(valueAST, ttype.OfType, variables)) + return append(values, valueFromAST(valueAST, ttype.OfType, variables, specCompliant)) case *InputObject: ov, ok := valueAST.(*ast.ObjectValue) if !ok { @@ -403,9 +429,26 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac obj := map[string]interface{}{} for name, field := range ttype.Fields() { of, ok := fieldASTs[name] + if !specCompliant { + var value interface{} + if ok { + value = valueFromAST(of.Value, field.Type, variables, specCompliant) + } else { + value = field.DefaultValue + } + if ok && value == nil { + obj[name] = nil + } else if !isNullish(value) { + obj[name] = value + } + continue + } + // The field is written in the literal and does not reference an + // unsupplied variable: the caller supplied a value, so the field's + // default must not stand in for it. supplied := ok && !isUnprovidedVariable(of.Value, variables) if supplied { - obj[name] = valueFromAST(of.Value, field.Type, variables) + obj[name] = valueFromAST(of.Value, field.Type, variables, specCompliant) } else if !isNullish(field.DefaultValue) { obj[name] = field.DefaultValue } diff --git a/values_test.go b/values_test.go index 12bb5b9..d999c7c 100644 --- a/values_test.go +++ b/values_test.go @@ -56,15 +56,24 @@ func Test_coerceValue(t *testing.T) { }, } + // None of these cases involve a default value, so both coercion modes must + // agree on all of them. for name, tc := range testCases { name, tc := name, tc - t.Run(name, func(t *testing.T) { - t.Parallel() - - got := coerceValue(tc.input.ttype, tc.input.value) - if !reflect.DeepEqual(tc.expected, got) { - t.Errorf("unexpected result, expected: %v, got: %v", tc.expected, got) + for _, specCompliant := range []bool{false, true} { + specCompliant := specCompliant + mode := "legacy" + if specCompliant { + mode = "spec" } - }) + t.Run(name+"/"+mode, func(t *testing.T) { + t.Parallel() + + got := coerceValue(tc.input.ttype, tc.input.value, specCompliant) + if !reflect.DeepEqual(tc.expected, got) { + t.Errorf("unexpected result, expected: %v, got: %v", tc.expected, got) + } + }) + } } } From b8e4d0276f45623e90771c292331312b6889f27b Mon Sep 17 00:00:00 2001 From: ikawaha Date: Mon, 10 Aug 2026 17:15:28 +0900 Subject: [PATCH 03/10] fix: make spec-compliant argument coercion the default --- argument_coercion_test.go | 25 +++++++++++++++++-------- schema.go | 27 +++++++++++---------------- values.go | 18 +++++++++++++++++- 3 files changed, 45 insertions(+), 25 deletions(-) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index 5d302d3..4eafba5 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -178,16 +178,17 @@ var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ }, }) -// The same probe types under both coercion modes. SpecCompliantArgumentCoercion -// is opt-in, so the zero-valued config is the behaviour shipped before this -// change and must stay byte-for-byte identical. -var coercionProbeLegacySchema, _ = graphql.NewSchema(graphql.SchemaConfig{ +// The same probe types under both coercion modes. Spec-compliant coercion is +// the default, so the zero-valued config exercises the fix; +// LegacyArgumentCoercion opts back out and must reproduce the behaviour shipped +// before this change byte-for-byte. +var coercionProbeSpecSchema, _ = graphql.NewSchema(graphql.SchemaConfig{ Query: coercionProbeType, }) -var coercionProbeSpecSchema, _ = graphql.NewSchema(graphql.SchemaConfig{ - Query: coercionProbeType, - SpecCompliantArgumentCoercion: true, +var coercionProbeLegacySchema, _ = graphql.NewSchema(graphql.SchemaConfig{ + Query: coercionProbeType, + LegacyArgumentCoercion: true, }) func execProbe(t *testing.T, schema graphql.Schema, field, doc string, vars map[string]interface{}) string { @@ -306,6 +307,14 @@ func TestArgumentCoercion_ArgumentDefault_AppliesOnlyWhenValueAbsent(t *testing. map[string]interface{}{"a": "v"}, `{"a":"v","keys":["a"]}`) }) + // A literal the argument's type cannot parse is neither absent nor null. The + // specification calls for a field error (§6.4.1); this implementation has + // always fallen back to the default instead, and such a document fails + // validation anyway, so both modes keep that behaviour. + t.Run("literal cannot be parsed -> default in both modes", func(t *testing.T) { + runProbe(t, "probeArgDefault", `{ probeArgDefault(a: WRONG_TYPE) }`, nil, + `{"a":"ARGDEF","keys":["a"]}`) + }) } // Spec: input object field defaults (§3.10 Input Coercion), reached through an @@ -621,7 +630,7 @@ func TestArgumentCoercion_ListOfInputObjects_PreservesPerElementState(t *testing } // Spec §5.4.2.1, end to end through graphql.Do so document validation runs too. -// This relaxation is not gated by SpecCompliantArgumentCoercion: it only lets +// This relaxation is not affected by LegacyArgumentCoercion: it only lets // queries through that previously failed, so it cannot break a working one. func TestArgumentCoercion_NonNullArgumentWithDefault_IsOptionalInBothModes(t *testing.T) { for _, tc := range []struct { diff --git a/schema.go b/schema.go index 4943a8c..2983bde 100644 --- a/schema.go +++ b/schema.go @@ -8,10 +8,10 @@ type SchemaConfig struct { Directives []*Directive Extensions []Extension - // SpecCompliantArgumentCoercion opts this schema into the argument and - // variable coercion rules described by the GraphQL specification - // (CoerceArgumentValues §6.4.1, CoerceVariableValues §6.1.2 and input - // object coercion §3.10): + // LegacyArgumentCoercion restores the argument and variable coercion + // behaviour of releases before the coercion fix. Leave it false: the zero + // value follows the GraphQL specification (CoerceArgumentValues §6.4.1, + // CoerceVariableValues §6.1.2 and input object coercion §3.10): // // - A variable the caller did not supply leaves its argument absent from // ResolveParams.Args instead of materialising it as nil, so a resolver @@ -19,11 +19,12 @@ type SchemaConfig struct { // - A default value applies only when no value was supplied. An explicit // null stays null instead of falling back to the default. // - // It is opt-in because both rules change what resolvers observe: code - // written against the previous behaviour may rely on every declared - // argument being present, or on an explicit null being replaced by the - // default. Leaving this false keeps that behaviour byte-for-byte. - SpecCompliantArgumentCoercion bool + // Older releases collapsed both distinctions: every declared argument + // arrived present, and an explicit null was replaced by the default. Set + // this to true to keep that behaviour byte-for-byte while migrating code + // that depends on it. The switch exists only to ease that migration and is + // expected to be removed once no schema needs it. + LegacyArgumentCoercion bool } type TypeMap map[string]Type @@ -84,7 +85,7 @@ func NewSchema(config SchemaConfig) (Schema, error) { schema.queryType = config.Query schema.mutationType = config.Mutation schema.subscriptionType = config.Subscription - schema.specCompliantArgumentCoercion = config.SpecCompliantArgumentCoercion + schema.specCompliantArgumentCoercion = !config.LegacyArgumentCoercion // Provide specified directives (e.g. @include and @skip) by default. schema.directives = config.Directives @@ -230,12 +231,6 @@ func (gq *Schema) SubscriptionType() *Object { return gq.subscriptionType } -// SpecCompliantArgumentCoercion reports whether this schema coerces arguments -// and variables by the specification's rules. See SchemaConfig for details. -func (gq *Schema) SpecCompliantArgumentCoercion() bool { - return gq.specCompliantArgumentCoercion -} - func (gq *Schema) Directives() []*Directive { return gq.directives } diff --git a/values.go b/values.go index 7ea15f7..cdd4899 100644 --- a/values.go +++ b/values.go @@ -76,7 +76,12 @@ func getArgumentValues( // does the default apply — an explicit null is a supplied value. isUndefined := !ok || isUnprovidedVariable(value, variableValues) tmp := valueFromAST(value, argDef.Type, variableValues, specCompliant) - if isUndefined && isNullish(tmp) { + // A literal the argument's type cannot parse also leaves tmp nullish. The + // specification calls for a field error there (CoerceArgumentValues + // §6.4.1); this implementation has always fallen back to the default + // instead, and such a document fails validation anyway, so that case is + // left as it is. Only a supplied null keeps its null. + if isNullish(tmp) && !isProvidedNullVariable(value, variableValues) { tmp = argDef.DefaultValue } if !isUndefined || !isNullish(tmp) { @@ -96,6 +101,17 @@ func isUnprovidedVariable(value ast.Value, variables map[string]interface{}) boo return !provided } +// Returns true if value is a reference to a variable the caller supplied as +// null. Such a null is a value of its own, so no default may stand in for it. +func isProvidedNullVariable(value ast.Value, variables map[string]interface{}) bool { + v, ok := value.(*ast.Variable) + if !ok || v.Name == nil { + return false + } + supplied, provided := variables[v.Name.Value] + return provided && isNullish(supplied) +} + // Given a variable definition, and any value of input, return a value which // adheres to the variable definition, or throw an error. func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, input interface{}, provided bool) (interface{}, error) { From fcab3dcc4096e19011dbf3948b2fbb18bb415043 Mon Sep 17 00:00:00 2001 From: ikawaha Date: Mon, 17 Aug 2026 13:36:31 +0900 Subject: [PATCH 04/10] fix: gate required-argument validation behind the argument-handling opt-out --- argument_coercion_test.go | 84 +++++++++++++---------- executor.go | 6 +- rules.go | 10 ++- rules_provided_non_null_arguments_test.go | 56 +++++++++++++-- schema.go | 32 +++++---- subscription.go | 2 +- values.go | 50 +++++++------- values_test.go | 12 ++-- 8 files changed, 160 insertions(+), 92 deletions(-) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index 4eafba5..ac3a8b2 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -178,17 +178,17 @@ var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ }, }) -// The same probe types under both coercion modes. Spec-compliant coercion is -// the default, so the zero-valued config exercises the fix; -// LegacyArgumentCoercion opts back out and must reproduce the behaviour shipped -// before this change byte-for-byte. +// The same probe types under both modes. Spec-compliant handling is the +// default, so the zero-valued config exercises the fix; +// NonSpecArgumentHandling opts back out and must reproduce the behaviour +// shipped before this change byte-for-byte. var coercionProbeSpecSchema, _ = graphql.NewSchema(graphql.SchemaConfig{ Query: coercionProbeType, }) -var coercionProbeLegacySchema, _ = graphql.NewSchema(graphql.SchemaConfig{ - Query: coercionProbeType, - LegacyArgumentCoercion: true, +var coercionProbeNonSpecSchema, _ = graphql.NewSchema(graphql.SchemaConfig{ + Query: coercionProbeType, + NonSpecArgumentHandling: true, }) func execProbe(t *testing.T, schema graphql.Schema, field, doc string, vars map[string]interface{}) string { @@ -214,12 +214,12 @@ func runProbe(t *testing.T, field, doc string, vars map[string]interface{}, want runProbeModes(t, field, doc, vars, want, want) } -// runProbeModes pins down a case where the flag changes the outcome: the legacy -// column is the regression guard, the spec column is the fix. -func runProbeModes(t *testing.T, field, doc string, vars map[string]interface{}, wantLegacy, wantSpec string) { +// runProbeModes pins down a case where the flag changes the outcome: the +// non-spec column is the regression guard, the spec column is the fix. +func runProbeModes(t *testing.T, field, doc string, vars map[string]interface{}, wantNonSpec, wantSpec string) { t.Helper() - if got := execProbe(t, coercionProbeLegacySchema, field, doc, vars); got != wantLegacy { - t.Errorf("legacy mode mismatch\n got: %s\n want: %s", got, wantLegacy) + if got := execProbe(t, coercionProbeNonSpecSchema, field, doc, vars); got != wantNonSpec { + t.Errorf("non-spec mode mismatch\n got: %s\n want: %s", got, wantNonSpec) } if got := execProbe(t, coercionProbeSpecSchema, field, doc, vars); got != wantSpec { t.Errorf("spec mode mismatch\n got: %s\n want: %s", got, wantSpec) @@ -630,30 +630,40 @@ func TestArgumentCoercion_ListOfInputObjects_PreservesPerElementState(t *testing } // Spec §5.4.2.1, end to end through graphql.Do so document validation runs too. -// This relaxation is not affected by LegacyArgumentCoercion: it only lets -// queries through that previously failed, so it cannot break a working one. -func TestArgumentCoercion_NonNullArgumentWithDefault_IsOptionalInBothModes(t *testing.T) { - for _, tc := range []struct { - mode string - schema graphql.Schema - }{ - {"legacy", coercionProbeLegacySchema}, - {"spec", coercionProbeSpecSchema}, - } { - t.Run(tc.mode, func(t *testing.T) { - result := graphql.Do(graphql.Params{ - Schema: tc.schema, - RequestString: `{ probeNonNullDefault }`, - }) - if len(result.Errors) > 0 { - t.Fatalf("unexpected errors: %v", result.Errors) - } - data, _ := result.Data.(map[string]interface{}) - got, _ := data["probeNonNullDefault"].(string) - want := `{"a":"NNDEF","keys":["a"]}` - if got != want { - t.Fatalf("probe mismatch\n got: %s\n want: %s", got, want) - } - }) +// A non-null argument carrying a default is optional, so omitting it validates +// and resolves to the default. +func TestArgumentCoercion_NonNullArgumentWithDefault_IsOptionalInSpecMode(t *testing.T) { + result := graphql.Do(graphql.Params{ + Schema: coercionProbeSpecSchema, + RequestString: `{ probeNonNullDefault }`, + }) + if len(result.Errors) > 0 { + t.Fatalf("unexpected errors: %v", result.Errors) + } + data, _ := result.Data.(map[string]interface{}) + got, _ := data["probeNonNullDefault"].(string) + want := `{"a":"NNDEF","keys":["a"]}` + if got != want { + t.Fatalf("probe mismatch\n got: %s\n want: %s", got, want) + } +} + +// The stricter reading of §5.4.2.1 is part of what NonSpecArgumentHandling +// restores: a schema that opts out treats every non-null argument as required, +// so the same document fails validation instead of reaching a resolver. +func TestArgumentCoercion_NonNullArgumentWithDefault_IsRequiredInNonSpecMode(t *testing.T) { + result := graphql.Do(graphql.Params{ + Schema: coercionProbeNonSpecSchema, + RequestString: `{ probeNonNullDefault }`, + }) + if len(result.Errors) != 1 { + t.Fatalf("expected exactly one validation error, got: %v", result.Errors) + } + want := `Field "probeNonNullDefault" argument "a" of type "String!" is required but not provided.` + if got := result.Errors[0].Message; got != want { + t.Fatalf("error mismatch\n got: %s\n want: %s", got, want) + } + if result.Data != nil { + t.Fatalf("expected no data on a validation failure, got: %v", result.Data) } } diff --git a/executor.go b/executor.go index 889788f..48782a3 100644 --- a/executor.go +++ b/executor.go @@ -498,13 +498,13 @@ func shouldIncludeNode(eCtx *executionContext, directives []*ast.Directive) bool } // precedence: skipAST > includeAST if skipAST != nil { - argValues = getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, eCtx.Schema.specCompliantArgumentCoercion) + argValues = getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) if skipIf, ok := argValues["if"].(bool); ok && skipIf { return false // excluded selectionSet's fields } } if includeAST != nil { - argValues = getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, eCtx.Schema.specCompliantArgumentCoercion) + argValues = getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) if includeIf, ok := argValues["if"].(bool); ok && !includeIf { return false // excluded selectionSet's fields } @@ -624,7 +624,7 @@ 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, eCtx.Schema.specCompliantArgumentCoercion) + args := getArgumentValues(fieldDef.Args, fieldAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) info := ResolveInfo{ FieldName: fieldName, diff --git a/rules.go b/rules.go index 7268c60..d06547b 100644 --- a/rules.go +++ b/rules.go @@ -1247,6 +1247,12 @@ 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. 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 +1277,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 && argDef.DefaultValue == nil { + if argDefType, ok := argDef.Type.(*NonNull); ok && (nonSpec || argDef.DefaultValue == nil) { fieldName := "" if fieldAST.Name != nil { fieldName = fieldAST.Name.Value @@ -1312,7 +1318,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 && argDef.DefaultValue == nil { + if argDefType, ok := argDef.Type.(*NonNull); ok && (nonSpec || argDef.DefaultValue == nil) { directiveName := "" if directiveAST.Name != nil { directiveName = directiveAST.Name.Value diff --git a/rules_provided_non_null_arguments_test.go b/rules_provided_non_null_arguments_test.go index 195e9d5..32baf26 100644 --- a/rules_provided_non_null_arguments_test.go +++ b/rules_provided_non_null_arguments_test.go @@ -176,10 +176,10 @@ func TestValidate_ProvidedNonNullArguments_DirectiveArguments_WithDirectiveWithM }) } -// Spec §5.4.2.1: "An argument is required if the argument type is non-null and -// does not have a default value. Otherwise, the argument is optional." -// See graphql-go/graphql#739. -func TestValidate_ProvidedNonNullArguments_FieldArguments_NoErrorOnNonNullArgumentWithDefaultValue(t *testing.T) { +// One field whose non-null argument declares a default value, under whichever +// mode the caller asks for. +func nonNullArgWithDefaultSchema(t *testing.T, nonSpec bool) graphql.Schema { + t.Helper() schema, err := graphql.NewSchema(graphql.SchemaConfig{ Query: graphql.NewObject(graphql.ObjectConfig{ Name: "Query", @@ -195,10 +195,19 @@ func TestValidate_ProvidedNonNullArguments_FieldArguments_NoErrorOnNonNullArgume }, }, }), + NonSpecArgumentHandling: nonSpec, }) if err != nil { t.Fatalf("Unexpected error, got: %v", err) } + return schema +} + +// Spec §5.4.2.1: "An argument is required if the argument type is non-null and +// does not have a default value. Otherwise, the argument is optional." +// See graphql-go/graphql#739. +func TestValidate_ProvidedNonNullArguments_FieldArguments_NoErrorOnNonNullArgumentWithDefaultValue(t *testing.T) { + schema := nonNullArgWithDefaultSchema(t, false) testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` { fieldWithDefault @@ -206,6 +215,19 @@ func TestValidate_ProvidedNonNullArguments_FieldArguments_NoErrorOnNonNullArgume `) } +// NonSpecArgumentHandling restores the older, stricter reading, under which a +// non-null argument is required whether or not it declares a default. +func TestValidate_ProvidedNonNullArguments_FieldArguments_NonSpecErrorsOnNonNullArgumentWithDefaultValue(t *testing.T) { + schema := nonNullArgWithDefaultSchema(t, true) + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` + { + fieldWithDefault + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Field "fieldWithDefault" argument "arg" of type "Boolean!" is required but not provided.`, 3, 11), + }) +} + func TestValidate_ProvidedNonNullArguments_FieldArguments_StillErrorsOnNonNullArgumentWithoutDefaultValue(t *testing.T) { schema, err := graphql.NewSchema(graphql.SchemaConfig{ Query: graphql.NewObject(graphql.ObjectConfig{ @@ -234,7 +256,10 @@ func TestValidate_ProvidedNonNullArguments_FieldArguments_StillErrorsOnNonNullAr }) } -func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NoErrorOnNonNullArgumentWithDefaultValue(t *testing.T) { +// One directive whose non-null argument declares a default value, under +// whichever mode the caller asks for. +func nonNullDirectiveArgWithDefaultSchema(t *testing.T, nonSpec bool) graphql.Schema { + t.Helper() deferDirective := graphql.NewDirective(graphql.DirectiveConfig{ Name: "defer", Locations: []string{ @@ -255,11 +280,17 @@ func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NoErrorOnNonNullAr "a": &graphql.Field{Type: graphql.String}, }, }), - Directives: []*graphql.Directive{deferDirective}, + Directives: []*graphql.Directive{deferDirective}, + NonSpecArgumentHandling: nonSpec, }) if err != nil { t.Fatalf("Unexpected error, got: %v", err) } + return schema +} + +func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NoErrorOnNonNullArgumentWithDefaultValue(t *testing.T) { + schema := nonNullDirectiveArgWithDefaultSchema(t, false) testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` { ... on Query @defer { @@ -268,3 +299,16 @@ func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NoErrorOnNonNullAr } `) } + +func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NonSpecErrorsOnNonNullArgumentWithDefaultValue(t *testing.T) { + schema := nonNullDirectiveArgWithDefaultSchema(t, true) + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` + { + ... on Query @defer { + a + } + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Directive "@defer" argument "if" of type "Boolean!" is required but not provided.`, 3, 24), + }) +} diff --git a/schema.go b/schema.go index 2983bde..67365e4 100644 --- a/schema.go +++ b/schema.go @@ -8,23 +8,31 @@ type SchemaConfig struct { Directives []*Directive Extensions []Extension - // LegacyArgumentCoercion restores the argument and variable coercion - // behaviour of releases before the coercion fix. Leave it false: the zero - // value follows the GraphQL specification (CoerceArgumentValues §6.4.1, - // CoerceVariableValues §6.1.2 and input object coercion §3.10): + // NonSpecArgumentHandling restores argument handling that contradicts the + // GraphQL specification. Leave it false: the zero value follows the + // specification (CoerceArgumentValues §6.4.1, CoerceVariableValues §6.1.2, + // input object coercion §3.10 and required arguments §5.4.2.1): // // - A variable the caller did not supply leaves its argument absent from // ResolveParams.Args instead of materialising it as nil, so a resolver // can tell "not provided" from "explicitly null". // - A default value applies only when no value was supplied. An explicit // null stays null instead of falling back to the default. + // - A non-null argument that declares a default value is optional, so a + // document may omit it and still validate. // - // Older releases collapsed both distinctions: every declared argument - // arrived present, and an explicit null was replaced by the default. Set - // this to true to keep that behaviour byte-for-byte while migrating code - // that depends on it. The switch exists only to ease that migration and is - // expected to be removed once no schema needs it. - LegacyArgumentCoercion bool + // Releases before that fix collapsed the first two distinctions — every + // declared argument arrived present, and an explicit null was replaced by + // the default — and rejected any document that omitted a non-null argument, + // default or not. None of that was a deliberate design: it was a defect, + // and a schema that sets this flag keeps diverging from the specification + // and from every other GraphQL implementation. + // + // Deprecated: this exists only so an application built against the defect + // keeps working while it migrates, and it will be removed once no schema + // needs it. Set it to true to reproduce the old behaviour byte-for-byte, + // then migrate off it. + NonSpecArgumentHandling bool } type TypeMap map[string]Type @@ -62,7 +70,7 @@ type Schema struct { possibleTypeMap map[string]map[string]bool extensions []Extension - specCompliantArgumentCoercion bool + nonSpecArgumentHandling bool } func NewSchema(config SchemaConfig) (Schema, error) { @@ -85,7 +93,7 @@ func NewSchema(config SchemaConfig) (Schema, error) { schema.queryType = config.Query schema.mutationType = config.Mutation schema.subscriptionType = config.Subscription - schema.specCompliantArgumentCoercion = !config.LegacyArgumentCoercion + schema.nonSpecArgumentHandling = config.NonSpecArgumentHandling // Provide specified directives (e.g. @include and @skip) by default. schema.directives = config.Directives diff --git a/subscription.go b/subscription.go index 64946f0..8f87de1 100644 --- a/subscription.go +++ b/subscription.go @@ -166,7 +166,7 @@ func ExecuteSubscription(p ExecuteParams) chan *Result { Key: responseName, } - args := getArgumentValues(fieldDef.Args, fieldNode.Arguments, exeContext.VariableValues, exeContext.Schema.specCompliantArgumentCoercion) + args := getArgumentValues(fieldDef.Args, fieldNode.Arguments, exeContext.VariableValues, exeContext.Schema.nonSpecArgumentHandling) info := ResolveInfo{ FieldName: fieldName, FieldASTs: fieldNodes, diff --git a/values.go b/values.go index cdd4899..85b8e4e 100644 --- a/values.go +++ b/values.go @@ -32,7 +32,7 @@ func getVariableValues( if err != nil { return values, err } - if !schema.specCompliantArgumentCoercion || provided || defAST.DefaultValue != nil { + if schema.nonSpecArgumentHandling || provided || defAST.DefaultValue != nil { values[varName] = varValue } } @@ -43,7 +43,7 @@ func getVariableValues( // definitions and list of argument AST nodes. func getArgumentValues( argDefs []*Argument, argASTs []*ast.Argument, - variableValues map[string]interface{}, specCompliant bool) map[string]interface{} { + variableValues map[string]interface{}, nonSpec bool) map[string]interface{} { argASTMap := map[string]*ast.Argument{} for _, argAST := range argASTs { @@ -58,9 +58,9 @@ func getArgumentValues( if ok { value = argAST.Value } - if !specCompliant { + if nonSpec { isUndefined := !ok - tmp := valueFromAST(value, argDef.Type, variableValues, specCompliant) + tmp := valueFromAST(value, argDef.Type, variableValues, nonSpec) if isNullish(tmp) { tmp = argDef.DefaultValue } @@ -75,7 +75,7 @@ func getArgumentValues( // when it references a variable the caller did not supply. Only then // does the default apply — an explicit null is a supplied value. isUndefined := !ok || isUnprovidedVariable(value, variableValues) - tmp := valueFromAST(value, argDef.Type, variableValues, specCompliant) + tmp := valueFromAST(value, argDef.Type, variableValues, nonSpec) // A literal the argument's type cannot parse also leaves tmp nullish. The // specification calls for a field error there (CoerceArgumentValues // §6.4.1); this implementation has always fallen back to the default @@ -115,7 +115,7 @@ func isProvidedNullVariable(value ast.Value, variables map[string]interface{}) b // Given a variable definition, and any value of input, return a value which // adheres to the variable definition, or throw an error. func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, input interface{}, provided bool) (interface{}, error) { - specCompliant := schema.specCompliantArgumentCoercion + nonSpec := schema.nonSpecArgumentHandling ttype, err := typeFromAST(schema, definitionAST.Type) if err != nil { return nil, err @@ -137,14 +137,14 @@ func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, inpu isValid, messages := isValidInputValue(input, ttype) if isValid { if isNullish(input) { - // The default stands in for a value the caller did not supply. In - // spec-compliant mode an explicitly supplied null is a value, so it - // must not be replaced by the default. - if definitionAST.DefaultValue != nil && !(specCompliant && provided) { - return valueFromAST(definitionAST.DefaultValue, ttype, nil, specCompliant), nil + // The default stands in for a value the caller did not supply. By the + // specification an explicitly supplied null is a value, so it must not + // be replaced by the default. + if definitionAST.DefaultValue != nil && (nonSpec || !provided) { + return valueFromAST(definitionAST.DefaultValue, ttype, nil, nonSpec), nil } } - return coerceValue(ttype, input, specCompliant), nil + return coerceValue(ttype, input, nonSpec), nil } if isNullish(input) { return "", gqlerrors.NewError( @@ -179,24 +179,24 @@ func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, inpu } // Given a type and any value, return a runtime value coerced to match the type. -func coerceValue(ttype Input, value interface{}, specCompliant bool) interface{} { +func coerceValue(ttype Input, value interface{}, nonSpec bool) interface{} { if isNullish(value) { return nil } switch ttype := ttype.(type) { case *NonNull: - return coerceValue(ttype.OfType, value, specCompliant) + return coerceValue(ttype.OfType, value, nonSpec) case *List: var values = []interface{}{} valType := reflect.ValueOf(value) if valType.Kind() == reflect.Slice { for i := 0; i < valType.Len(); i++ { val := valType.Index(i).Interface() - values = append(values, coerceValue(ttype.OfType, val, specCompliant)) + values = append(values, coerceValue(ttype.OfType, val, nonSpec)) } return values } - return append(values, coerceValue(ttype.OfType, value, specCompliant)) + return append(values, coerceValue(ttype.OfType, value, nonSpec)) case *InputObject: var obj = map[string]interface{}{} valueMap, _ := value.(map[string]interface{}) @@ -211,11 +211,11 @@ func coerceValue(ttype Input, value interface{}, specCompliant bool) interface{} } // The key is present and holds null: the caller supplied a value, so // the field's default must not stand in for it. - if specCompliant && ok && isNullish(v) { + if !nonSpec && ok && isNullish(v) { obj[name] = nil continue } - fieldValue := coerceValue(field.Type, v, specCompliant) + fieldValue := coerceValue(field.Type, v, nonSpec) if isNullish(fieldValue) { fieldValue = field.DefaultValue } @@ -404,7 +404,7 @@ func isIterable(src interface{}) bool { * | Int / Float | Number | * */ -func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interface{}, specCompliant bool) interface{} { +func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interface{}, nonSpec bool) interface{} { if valueAST == nil { return nil } @@ -420,16 +420,16 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac } switch ttype := ttype.(type) { case *NonNull: - return valueFromAST(valueAST, ttype.OfType, variables, specCompliant) + return valueFromAST(valueAST, ttype.OfType, variables, nonSpec) case *List: values := []interface{}{} if valueAST, ok := valueAST.(*ast.ListValue); ok { for _, itemAST := range valueAST.Values { - values = append(values, valueFromAST(itemAST, ttype.OfType, variables, specCompliant)) + values = append(values, valueFromAST(itemAST, ttype.OfType, variables, nonSpec)) } return values } - return append(values, valueFromAST(valueAST, ttype.OfType, variables, specCompliant)) + return append(values, valueFromAST(valueAST, ttype.OfType, variables, nonSpec)) case *InputObject: ov, ok := valueAST.(*ast.ObjectValue) if !ok { @@ -445,10 +445,10 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac obj := map[string]interface{}{} for name, field := range ttype.Fields() { of, ok := fieldASTs[name] - if !specCompliant { + if nonSpec { var value interface{} if ok { - value = valueFromAST(of.Value, field.Type, variables, specCompliant) + value = valueFromAST(of.Value, field.Type, variables, nonSpec) } else { value = field.DefaultValue } @@ -464,7 +464,7 @@ func valueFromAST(valueAST ast.Value, ttype Input, variables map[string]interfac // default must not stand in for it. supplied := ok && !isUnprovidedVariable(of.Value, variables) if supplied { - obj[name] = valueFromAST(of.Value, field.Type, variables, specCompliant) + obj[name] = valueFromAST(of.Value, field.Type, variables, nonSpec) } else if !isNullish(field.DefaultValue) { obj[name] = field.DefaultValue } diff --git a/values_test.go b/values_test.go index d999c7c..59b70db 100644 --- a/values_test.go +++ b/values_test.go @@ -60,16 +60,16 @@ func Test_coerceValue(t *testing.T) { // agree on all of them. for name, tc := range testCases { name, tc := name, tc - for _, specCompliant := range []bool{false, true} { - specCompliant := specCompliant - mode := "legacy" - if specCompliant { - mode = "spec" + for _, nonSpec := range []bool{false, true} { + nonSpec := nonSpec + mode := "spec" + if nonSpec { + mode = "nonSpec" } t.Run(name+"/"+mode, func(t *testing.T) { t.Parallel() - got := coerceValue(tc.input.ttype, tc.input.value, specCompliant) + got := coerceValue(tc.input.ttype, tc.input.value, nonSpec) if !reflect.DeepEqual(tc.expected, got) { t.Errorf("unexpected result, expected: %v, got: %v", tc.expected, got) } From 292cbb87eeb974c33df20af43fc653e05183c264 Mon Sep 17 00:00:00 2001 From: ikawaha Date: Mon, 17 Aug 2026 20:01:56 +0900 Subject: [PATCH 05/10] fix: apply the non-null-with-default rule to variables and input fields --- argument_coercion_test.go | 253 +++++++++++++++++++ executor.go | 27 +- rules.go | 81 ++++-- rules_arguments_of_correct_type_test.go | 61 +++++ rules_default_values_of_correct_type_test.go | 74 ++++-- rules_variables_in_allowed_position_test.go | 71 ++++++ subscription.go | 9 +- type_info.go | 45 +++- validator.go | 8 +- values.go | 61 ++++- values_test.go | 33 +++ variables_test.go | 24 +- 12 files changed, 674 insertions(+), 73 deletions(-) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index ac3a8b2..5f39b57 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/tailor-platform/graphql" + "github.com/tailor-platform/graphql/gqlerrors" "github.com/tailor-platform/graphql/testutil" ) @@ -87,6 +88,35 @@ var coercionProbeDeepInputObject = graphql.NewInputObject(graphql.InputObjectCon }, }) +// A non-null field that declares a default. Spec §3.10 and §5.6.4 make such a +// field optional, so a document may omit it and take the default. +var coercionProbeNonNullFieldDefaultInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeNonNullFieldDefaultInput", + Fields: graphql.InputObjectConfigFieldMap{ + "a": &graphql.InputObjectFieldConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: "FIELDDEF", + }, + }, +}) + +// The same field one level down, so the recursive paths are exercised too. +var coercionProbeNonNullFieldNestedInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeNonNullFieldNestedInput", + Fields: graphql.InputObjectConfigFieldMap{ + "inner": &graphql.InputObjectFieldConfig{Type: coercionProbeNonNullFieldDefaultInputObject}, + }, +}) + +// A non-null field with no default: genuinely required, so it stays required in +// both modes. +var coercionProbeRequiredInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeRequiredInput", + Fields: graphql.InputObjectConfigFieldMap{ + "a": &graphql.InputObjectFieldConfig{Type: graphql.NewNonNull(graphql.String)}, + }, +}) + var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ Name: "CoercionProbeQuery", Fields: graphql.Fields{ @@ -175,6 +205,34 @@ var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ }, Resolve: probeArgs, }, + "probeNonNullFieldDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeNonNullFieldDefaultInputObject}, + }, + Resolve: probeObjectArgs, + }, + "probeNonNullFieldNested": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeNonNullFieldNestedInputObject}, + }, + Resolve: probeObjectArgs, + }, + "probeNonNullFieldList": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: graphql.NewList(coercionProbeNonNullFieldDefaultInputObject)}, + }, + Resolve: probeArgs, + }, + "probeObjectRequired": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeRequiredInputObject}, + }, + Resolve: probeObjectArgs, + }, }, }) @@ -226,6 +284,37 @@ func runProbeModes(t *testing.T, field, doc string, vars map[string]interface{}, } } +// execDo runs the document through graphql.Do, so document validation runs as +// well as coercion. An error comes back as a string so a test can pin an +// expected rejection instead of failing the run. +func execDo(t *testing.T, schema graphql.Schema, field, doc string, vars map[string]interface{}) string { + t.Helper() + result := graphql.Do(graphql.Params{ + Schema: schema, + RequestString: doc, + VariableValues: vars, + }) + if len(result.Errors) > 0 { + return "ERROR: " + result.Errors[0].Message + } + data, _ := result.Data.(map[string]interface{}) + got, _ := data[field].(string) + return got +} + +// runDoModes pins a case where the flag changes the outcome, end to end through +// graphql.Do: the non-spec column is the regression guard, the spec column is +// the fix. +func runDoModes(t *testing.T, field, doc string, vars map[string]interface{}, wantNonSpec, wantSpec string) { + t.Helper() + if got := execDo(t, coercionProbeNonSpecSchema, field, doc, vars); got != wantNonSpec { + t.Errorf("non-spec mode mismatch\n got: %s\n want: %s", got, wantNonSpec) + } + if got := execDo(t, coercionProbeSpecSchema, field, doc, vars); got != wantSpec { + t.Errorf("spec mode mismatch\n got: %s\n want: %s", got, wantSpec) + } +} + func TestArgumentCoercion_ScalarVariable_PreservesThreeStates(t *testing.T) { doc := `query Probe($a: String, $b: String) { probe(a: $a, b: $b) }` @@ -667,3 +756,167 @@ func TestArgumentCoercion_NonNullArgumentWithDefault_IsRequiredInNonSpecMode(t * t.Fatalf("expected no data on a validation failure, got: %v", result.Data) } } + +// Spec §3.10 Input Coercion and §5.6.4 Input Object Required Fields: an input +// field is required only when its type is non-null AND it declares no default +// value. A field that declares a default may be omitted, and the default applies. +func TestArgumentCoercion_NonNullInputFieldWithDefault_IsOptional(t *testing.T) { + t.Run("literal omits the field -> default", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldDefault", + `{ probeNonNullFieldDefault(input: {}) }`, nil, + "ERROR: Argument \"input\" has invalid value {}.\nIn field \"a\": Expected \"String!\", found null.", + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`) + }) + t.Run("variable object omits the key -> default", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldDefault", + `query Probe($in: CoercionProbeNonNullFieldDefaultInput) { probeNonNullFieldDefault(input: $in) }`, + map[string]interface{}{"in": map[string]interface{}{}}, + "ERROR: Variable \"$in\" got invalid value {}.\nIn field \"a\": Expected \"String!\", found null.", + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`) + }) + t.Run("nested literal omits the field -> default", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldNested", + `{ probeNonNullFieldNested(input: {inner: {}}) }`, nil, + "ERROR: Argument \"input\" has invalid value {inner: {}}.\nIn field \"inner\": In field \"a\": Expected \"String!\", found null.", + `{"keys":["inner"],"obj":{"inner":{"a":"FIELDDEF"}}}`) + }) + t.Run("list element omits the field -> default", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldList", + `query Probe($in: [CoercionProbeNonNullFieldDefaultInput]) { probeNonNullFieldList(input: $in) }`, + map[string]interface{}{"in": []interface{}{map[string]interface{}{}}}, + "ERROR: Variable \"$in\" got invalid value [{}].\nIn element #1: In field \"a\": Expected \"String!\", found null.", + `{"input":[{"a":"FIELDDEF"}],"keys":["input"]}`) + }) +} + +// An explicit null is a supplied value, so no default stands in for it and a +// non-null field must still reject it — in both modes. +func TestArgumentCoercion_NonNullInputFieldWithDefault_RejectsExplicitNull(t *testing.T) { + want := "ERROR: Variable \"$in\" got invalid value {\"a\":null}.\nIn field \"a\": Expected \"String!\", found null." + runDoModes(t, "probeNonNullFieldDefault", + `query Probe($in: CoercionProbeNonNullFieldDefaultInput) { probeNonNullFieldDefault(input: $in) }`, + map[string]interface{}{"in": map[string]interface{}{"a": nil}}, want, want) +} + +// A non-null field that declares no default is genuinely required, in both modes. +func TestArgumentCoercion_NonNullInputFieldWithoutDefault_StaysRequired(t *testing.T) { + want := "ERROR: Argument \"input\" has invalid value {}.\nIn field \"a\": Expected \"String!\", found null." + runDoModes(t, "probeObjectRequired", `{ probeObjectRequired(input: {}) }`, nil, want, want) +} + +// Spec §6.1.2 CoerceVariableValues: the default is applied when the caller +// supplied no value, and that check comes before the non-null requirement. A +// non-null variable may therefore declare a default and be omitted. +func TestArgumentCoercion_NonNullVariableWithDefault_UsesDefaultWhenOmitted(t *testing.T) { + doc := `query Probe($a: String! = "VARDEF") { probe(a: $a) }` + nonSpecWant := `ERROR: Variable "$a" of type "String!" is required and will not use the default value. Perhaps you meant to use type "String".` + + t.Run("variable omitted -> default", func(t *testing.T) { + runDoModes(t, "probe", doc, map[string]interface{}{}, + nonSpecWant, `{"a":"VARDEF","keys":["a"]}`) + }) + t.Run("variable with value -> value", func(t *testing.T) { + runDoModes(t, "probe", doc, map[string]interface{}{"a": "v"}, + nonSpecWant, `{"a":"v","keys":["a"]}`) + }) + t.Run("variable explicitly null -> error in both modes", func(t *testing.T) { + runDoModes(t, "probe", doc, map[string]interface{}{"a": nil}, + nonSpecWant, `ERROR: Variable "$a" of required type "String!" was not provided.`) + }) +} + +// Only the blanket "a non-null variable must not declare a default" rejection +// goes away. A default whose type does not match is still rejected. +func TestArgumentCoercion_NonNullVariableWithWronglyTypedDefault_StaysAnError(t *testing.T) { + runDoModes(t, "probe", `query Probe($a: String! = 123) { probe(a: $a) }`, map[string]interface{}{}, + `ERROR: Variable "$a" of type "String!" is required and will not use the default value. Perhaps you meant to use type "String".`, + "ERROR: Variable \"$a\" has invalid default value: 123.\nExpected type \"String\", found 123.") +} + +// execProbeErrors runs the document through graphql.Execute, which skips +// document validation, so a coercion-level error can be observed on its own. +func execProbeErrors(t *testing.T, schema graphql.Schema, doc string, vars map[string]interface{}) []gqlerrors.FormattedError { + t.Helper() + parsed := testutil.TestParse(t, doc) + result := graphql.Execute(graphql.ExecuteParams{ + Schema: schema, + AST: parsed, + Args: vars, + }) + return result.Errors +} + +// Spec §6.4.1 ②: a non-null argument whose value resolves to null is a field +// error. The default only stands in for a value the caller did not supply, so it +// cannot rescue a supplied null. Observed through graphql.Execute because +// document validation rejects this document earlier until §5.8.5 is implemented. +func TestArgumentCoercion_NonNullArgumentGivenExplicitNull_IsAFieldError(t *testing.T) { + doc := `query Probe($x: String) { probeNonNullDefault(a: $x) }` + vars := map[string]interface{}{"x": nil} + + if errs := execProbeErrors(t, coercionProbeNonSpecSchema, doc, vars); len(errs) != 0 { + t.Errorf("non-spec mode: expected no error, got: %v", errs) + } + + errs := execProbeErrors(t, coercionProbeSpecSchema, doc, vars) + if len(errs) != 1 { + t.Fatalf("spec mode: expected exactly one field error, got: %v", errs) + } + want := `Argument "a" of non-null type "String!" must not be null.` + if errs[0].Message != want { + t.Errorf("message mismatch\n got: %s\n want: %s", errs[0].Message, want) + } +} + +// The same argument still takes its default when the caller supplied nothing. +func TestArgumentCoercion_NonNullArgumentWithDefault_StillTakesDefault(t *testing.T) { + doc := `query Probe($x: String) { probeNonNullDefault(a: $x) }` + for _, tc := range []struct { + mode string + schema graphql.Schema + }{ + {"non-spec", coercionProbeNonSpecSchema}, + {"spec", coercionProbeSpecSchema}, + } { + t.Run(tc.mode, func(t *testing.T) { + if got := execProbe(t, tc.schema, "probeNonNullDefault", doc, map[string]interface{}{}); got != `{"a":"NNDEF","keys":["a"]}` { + t.Errorf("probe mismatch, got: %s", got) + } + }) + } +} + +// Spec §5.8.5 IsVariableUsageAllowed: a nullable variable may be used at a +// non-null location when that location declares a default value. The default +// covers the case where the caller supplies nothing. +func TestArgumentCoercion_NullableVariableAtNonNullArgumentWithDefault_IsAllowed(t *testing.T) { + doc := `query Probe($x: String) { probeNonNullDefault(a: $x) }` + nonSpecWant := `ERROR: Variable "$x" of type "String" used in position expecting type "String!".` + + t.Run("variable omitted -> argument default", func(t *testing.T) { + runDoModes(t, "probeNonNullDefault", doc, map[string]interface{}{}, + nonSpecWant, `{"a":"NNDEF","keys":["a"]}`) + }) + t.Run("variable explicitly null -> field error", func(t *testing.T) { + runDoModes(t, "probeNonNullDefault", doc, map[string]interface{}{"x": nil}, + nonSpecWant, `ERROR: Argument "a" of non-null type "String!" must not be null.`) + }) +} + +// The spec names ObjectField alongside Argument, so an input object field that +// declares a default permits the same usage. +func TestArgumentCoercion_NullableVariableAtNonNullInputFieldWithDefault_IsAllowed(t *testing.T) { + runDoModes(t, "probeNonNullFieldDefault", + `query Probe($x: String) { probeNonNullFieldDefault(input: {a: $x}) }`, + map[string]interface{}{}, + `ERROR: Variable "$x" of type "String" used in position expecting type "String!".`, + `{"keys":["a"],"obj":{"a":"FIELDDEF"}}`) +} + +// A non-null location that declares no default still rejects a nullable variable. +func TestArgumentCoercion_NullableVariableAtNonNullWithoutDefault_StaysRejected(t *testing.T) { + want := `ERROR: Variable "$x" of type "String" used in position expecting type "String!".` + runDoModes(t, "probeObjectRequired", + `query Probe($x: String) { probeObjectRequired(input: {a: $x}) }`, + map[string]interface{}{}, want, want) +} diff --git a/executor.go b/executor.go index 48782a3..6f2cdcd 100644 --- a/executor.go +++ 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 @@ -498,13 +495,24 @@ func shouldIncludeNode(eCtx *executionContext, directives []*ast.Directive) bool } // precedence: skipAST > includeAST if skipAST != nil { - argValues = getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) + argValues, err := getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) + if err != nil { + // @skip declares "if: Boolean!" with no default, so a null can only get + // here if both variable coercion and validation let it through. Record + // the error rather than dropping it, and leave the node out. + eCtx.Errors = append(eCtx.Errors, gqlerrors.FormatErrorsFromError(err)...) + return false // excluded selectionSet's fields + } if skipIf, ok := argValues["if"].(bool); ok && skipIf { return false // excluded selectionSet's fields } } if includeAST != nil { - argValues = getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) + argValues, err := getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) + if err != nil { + eCtx.Errors = append(eCtx.Errors, gqlerrors.FormatErrorsFromError(err)...) + return false // excluded selectionSet's fields + } if includeIf, ok := argValues["if"].(bool); ok && !includeIf { return false // excluded selectionSet's fields } @@ -624,7 +632,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, eCtx.Schema.nonSpecArgumentHandling) + 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, diff --git a/rules.go b/rules.go index d06547b..87f67dc 100644 --- a/rules.go +++ b/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") } @@ -1662,8 +1671,31 @@ 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 + hasLocationDefaultValue := locationDefaultValue != nil + 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{} @@ -1689,7 +1721,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 `+ @@ -1730,7 +1770,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 @@ -1755,21 +1795,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) @@ -1792,10 +1832,17 @@ 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. + if !nonSpec && !ok && field.DefaultValue != nil { + 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)) } diff --git a/rules_arguments_of_correct_type_test.go b/rules_arguments_of_correct_type_test.go index 638951c..406fa04 100644 --- a/rules_arguments_of_correct_type_test.go +++ b/rules_arguments_of_correct_type_test.go @@ -799,3 +799,64 @@ func TestValidate_ArgValuesOfCorrectType_DirectiveArguments_WithDirectivesWithIn ), }) } + +// One field whose input object declares a non-null field with a default value, +// under whichever mode the caller asks for. +func nonNullInputFieldWithDefaultSchema(t *testing.T, nonSpec bool) graphql.Schema { + t.Helper() + input := graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "FieldDefaultInput", + Fields: graphql.InputObjectConfigFieldMap{ + "a": &graphql.InputObjectFieldConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: "FIELDDEF", + }, + }, + }) + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "f": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: input}, + }, + }, + }, + }), + NonSpecArgumentHandling: nonSpec, + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + return schema +} + +// Spec §5.6.4: an input field is required only when its type is non-null AND it +// declares no default value. +func TestValidate_ArgumentsOfCorrectType_NonNullInputFieldWithDefaultMayBeOmitted(t *testing.T) { + schema := nonNullInputFieldWithDefaultSchema(t, false) + testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.ArgumentsOfCorrectTypeRule, ` + { + f(input: {}) + } + `) +} + +// NonSpecArgumentHandling keeps the older reading, under which every non-null +// input field is required whether or not it declares a default. +func TestValidate_ArgumentsOfCorrectType_NonSpecKeepsNonNullInputFieldRequired(t *testing.T) { + schema := nonNullInputFieldWithDefaultSchema(t, true) + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.ArgumentsOfCorrectTypeRule, ` + { + f(input: {}) + } + `, []gqlerrors.FormattedError{ + testutil.RuleError( + `Argument "input" has invalid value {}.`+ + "\nIn field \"a\": Expected \"String!\", found null.", + 3, 20, + ), + }) +} diff --git a/rules_default_values_of_correct_type_test.go b/rules_default_values_of_correct_type_test.go index e7eede0..ca18730 100644 --- a/rules_default_values_of_correct_type_test.go +++ b/rules_default_values_of_correct_type_test.go @@ -33,24 +33,18 @@ func TestValidate_VariableDefaultValuesOfCorrectType_VariablesWithValidDefaultVa } `) } -func TestValidate_VariableDefaultValuesOfCorrectType_NoRequiredVariablesWithDefaultValues(t *testing.T) { - testutil.ExpectFailsRule(t, graphql.DefaultValuesOfCorrectTypeRule, ` - query UnreachableDefaultValues($a: Int! = 3, $b: String! = "default") { + +// Spec §6.1.2 applies a variable's default before checking the non-null +// requirement, so a non-null variable may declare one and the default is +// reached. This test asserted the opposite before the coercion fix; the +// rejection now lives behind NonSpecArgumentHandling and is covered by +// TestValidate_VariableDefaultValuesOfCorrectType_NonSpecRejectsNonNullVariableDefault. +func TestValidate_VariableDefaultValuesOfCorrectType_NonNullVariablesWithDefaultValues(t *testing.T) { + testutil.ExpectPassesRule(t, graphql.DefaultValuesOfCorrectTypeRule, ` + query NonNullDefaultValues($a: Int! = 3, $b: String! = "default") { dog { name } } - `, - []gqlerrors.FormattedError{ - testutil.RuleError( - `Variable "$a" of type "Int!" is required and will not `+ - `use the default value. Perhaps you meant to use type "Int".`, - 2, 49, - ), - testutil.RuleError( - `Variable "$b" of type "String!" is required and will not `+ - `use the default value. Perhaps you meant to use type "String".`, - 2, 66, - ), - }) + `) } func TestValidate_VariableDefaultValuesOfCorrectType_VariablesWithInvalidDefaultValues(t *testing.T) { testutil.ExpectFailsRule(t, graphql.DefaultValuesOfCorrectTypeRule, ` @@ -105,3 +99,51 @@ func TestValidate_VariableDefaultValuesOfCorrectType_ListVariablesWithInvalidIte func TestValidate_VariableDefaultValuesOfCorrectType_InvalidNonNull(t *testing.T) { testutil.ExpectPassesRule(t, graphql.DefaultValuesOfCorrectTypeRule, `query($g:e!){a}`) } + +// One nullable argument, so a non-null variable can be used at it, under +// whichever mode the caller asks for. +func nonNullVariableDefaultSchema(t *testing.T, nonSpec bool) graphql.Schema { + t.Helper() + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "f": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{Type: graphql.String}, + }, + }, + }, + }), + NonSpecArgumentHandling: nonSpec, + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + return schema +} + +// Spec §6.1.2 applies a variable's default before checking the non-null +// requirement, and nothing in the Validation section forbids a non-null variable +// from declaring one. +func TestValidate_VariableDefaultValuesOfCorrectType_NonNullVariableMayHaveDefault(t *testing.T) { + schema := nonNullVariableDefaultSchema(t, false) + testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.DefaultValuesOfCorrectTypeRule, ` + query Probe($a: String! = "VARDEF") { + f(a: $a) + } + `) +} + +// NonSpecArgumentHandling keeps rejecting it. +func TestValidate_VariableDefaultValuesOfCorrectType_NonSpecRejectsNonNullVariableDefault(t *testing.T) { + schema := nonNullVariableDefaultSchema(t, true) + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.DefaultValuesOfCorrectTypeRule, ` + query Probe($a: String! = "VARDEF") { + f(a: $a) + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Variable "$a" of type "String!" is required and will not use the default value. Perhaps you meant to use type "String".`, 2, 33), + }) +} diff --git a/rules_variables_in_allowed_position_test.go b/rules_variables_in_allowed_position_test.go index 8739d3b..246e003 100644 --- a/rules_variables_in_allowed_position_test.go +++ b/rules_variables_in_allowed_position_test.go @@ -244,3 +244,74 @@ func TestValidate_VariablesInAllowedPosition_StringToNonNullableBooleanInDirecti `expecting type "Boolean!".`, 2, 19, 3, 26), }) } + +// Two non-null arguments, one declaring a default and one not, under whichever +// mode the caller asks for. +func nonNullArgDefaultPositionSchema(t *testing.T, nonSpec bool) graphql.Schema { + t.Helper() + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "withDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: "NNDEF", + }, + }, + }, + "withoutDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{Type: graphql.NewNonNull(graphql.String)}, + }, + }, + }, + }), + NonSpecArgumentHandling: nonSpec, + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + return schema +} + +// Spec §5.8.5: hasLocationDefaultValue permits a nullable variable at a non-null +// location that declares a default. +func TestValidate_VariablesInAllowedPosition_StringToNonNullStringWithArgumentDefault(t *testing.T) { + schema := nonNullArgDefaultPositionSchema(t, false) + testutil.ExpectPassesRuleWithSchema(t, &schema, graphql.VariablesInAllowedPositionRule, ` + query Probe($x: String) { + withDefault(a: $x) + } + `) +} + +// Without a default on either side the usage stays rejected. +func TestValidate_VariablesInAllowedPosition_StringToNonNullStringWithoutArgumentDefault(t *testing.T) { + schema := nonNullArgDefaultPositionSchema(t, false) + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.VariablesInAllowedPositionRule, ` + query Probe($x: String) { + withoutDefault(a: $x) + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Variable "$x" of type "String" used in position `+ + `expecting type "String!".`, 2, 19, 3, 27), + }) +} + +// NonSpecArgumentHandling only ever considered the variable's own default, so it +// keeps rejecting the usage. +func TestValidate_VariablesInAllowedPosition_NonSpecIgnoresArgumentDefault(t *testing.T) { + schema := nonNullArgDefaultPositionSchema(t, true) + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.VariablesInAllowedPositionRule, ` + query Probe($x: String) { + withDefault(a: $x) + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Variable "$x" of type "String" used in position `+ + `expecting type "String!".`, 2, 19, 3, 24), + }) +} diff --git a/subscription.go b/subscription.go index 8f87de1..4c30ef0 100644 --- a/subscription.go +++ b/subscription.go @@ -166,7 +166,14 @@ func ExecuteSubscription(p ExecuteParams) chan *Result { Key: responseName, } - args := getArgumentValues(fieldDef.Args, fieldNode.Arguments, exeContext.VariableValues, exeContext.Schema.nonSpecArgumentHandling) + args, argErr := getArgumentValues(fieldDef.Args, fieldNode.Arguments, exeContext.VariableValues, exeContext.Schema.nonSpecArgumentHandling) + if argErr != nil { + resultChannel <- &Result{ + Errors: gqlerrors.FormatErrors(argErr), + } + + return + } info := ResolveInfo{ FieldName: fieldName, FieldASTs: fieldNodes, diff --git a/type_info.go b/type_info.go index f296444..dd2dce1 100644 --- a/type_info.go +++ b/type_info.go @@ -14,14 +14,15 @@ import ( type fieldDefFn func(schema *Schema, parentType Type, fieldAST *ast.Field) *FieldDefinition type TypeInfo struct { - schema *Schema - typeStack []Output - parentTypeStack []Composite - inputTypeStack []Input - fieldDefStack []*FieldDefinition - directive *Directive - argument *Argument - getFieldDef fieldDefFn + schema *Schema + typeStack []Output + parentTypeStack []Composite + inputTypeStack []Input + defaultValueStack []interface{} + fieldDefStack []*FieldDefinition + directive *Directive + argument *Argument + getFieldDef fieldDefFn } type TypeInfoConfig struct { @@ -64,6 +65,16 @@ func (ti *TypeInfo) InputType() Input { } return nil } + +// DefaultValue returns the default value declared by the argument or input +// object field currently being visited, or nil when the position declares none. +// Spec §5.8.5 calls this hasLocationDefaultValue. +func (ti *TypeInfo) DefaultValue() interface{} { + if len(ti.defaultValueStack) > 0 { + return ti.defaultValueStack[len(ti.defaultValueStack)-1] + } + return nil +} func (ti *TypeInfo) FieldDef() *FieldDefinition { if len(ti.fieldDefStack) > 0 { return ti.fieldDefStack[len(ti.fieldDefStack)-1] @@ -163,9 +174,16 @@ func (ti *TypeInfo) Enter(node ast.Node) { argType = argDef.Type } ti.argument = argDef + var argDefault interface{} + if argDef != nil { + argDefault = argDef.DefaultValue + } + ti.defaultValueStack = append(ti.defaultValueStack, argDefault) ti.inputTypeStack = append(ti.inputTypeStack, argType) case *ast.ListValue: listType := GetNullable(ti.InputType()) + // List positions never have a default value. + ti.defaultValueStack = append(ti.defaultValueStack, nil) if list, ok := listType.(*List); ok { ti.inputTypeStack = append(ti.inputTypeStack, list.OfType) } else { @@ -173,6 +191,7 @@ func (ti *TypeInfo) Enter(node ast.Node) { } case *ast.ObjectField: var fieldType Input + var fieldDefault interface{} objectType := GetNamed(ti.InputType()) if objectType, ok := objectType.(*InputObject); ok { @@ -182,8 +201,10 @@ func (ti *TypeInfo) Enter(node ast.Node) { } if inputField, ok := objectType.Fields()[nameVal]; ok { fieldType = inputField.Type + fieldDefault = inputField.DefaultValue } } + ti.defaultValueStack = append(ti.defaultValueStack, fieldDefault) ti.inputTypeStack = append(ti.inputTypeStack, fieldType) } } @@ -218,11 +239,19 @@ func (ti *TypeInfo) Leave(node ast.Node) { } case kinds.Argument: ti.argument = nil + // pop ti.defaultValueStack + if len(ti.defaultValueStack) > 0 { + _, ti.defaultValueStack = ti.defaultValueStack[len(ti.defaultValueStack)-1], ti.defaultValueStack[:len(ti.defaultValueStack)-1] + } // pop ti.inputTypeStack if len(ti.inputTypeStack) > 0 { _, ti.inputTypeStack = ti.inputTypeStack[len(ti.inputTypeStack)-1], ti.inputTypeStack[:len(ti.inputTypeStack)-1] } case kinds.ListValue, kinds.ObjectField: + // pop ti.defaultValueStack + if len(ti.defaultValueStack) > 0 { + _, ti.defaultValueStack = ti.defaultValueStack[len(ti.defaultValueStack)-1], ti.defaultValueStack[:len(ti.defaultValueStack)-1] + } // pop ti.inputTypeStack if len(ti.inputTypeStack) > 0 { _, ti.inputTypeStack = ti.inputTypeStack[len(ti.inputTypeStack)-1], ti.inputTypeStack[:len(ti.inputTypeStack)-1] diff --git a/validator.go b/validator.go index b003aa0..af76ab6 100644 --- a/validator.go +++ b/validator.go @@ -83,6 +83,9 @@ var _ HasSelectionSet = (*ast.FragmentDefinition)(nil) type VariableUsage struct { Node *ast.Variable Type Input + // LocationDefaultValue is the default value declared by the argument or input + // object field where this usage sits. Spec §5.8.5 hasLocationDefaultValue. + LocationDefaultValue interface{} } type ValidationContext struct { @@ -239,8 +242,9 @@ func (ctx *ValidationContext) VariableUsages(node HasSelectionSet) []*VariableUs Kind: func(p visitor.VisitFuncParams) (string, interface{}) { if node, ok := p.Node.(*ast.Variable); ok && node != nil { usages = append(usages, &VariableUsage{ - Node: node, - Type: typeInfo.InputType(), + Node: node, + Type: typeInfo.InputType(), + LocationDefaultValue: typeInfo.DefaultValue(), }) } return visitor.ActionNoChange, nil diff --git a/values.go b/values.go index 85b8e4e..513dbc0 100644 --- a/values.go +++ b/values.go @@ -43,7 +43,7 @@ func getVariableValues( // definitions and list of argument AST nodes. func getArgumentValues( argDefs []*Argument, argASTs []*ast.Argument, - variableValues map[string]interface{}, nonSpec bool) map[string]interface{} { + variableValues map[string]interface{}, nonSpec bool) (map[string]interface{}, error) { argASTMap := map[string]*ast.Argument{} for _, argAST := range argASTs { @@ -84,11 +84,30 @@ func getArgumentValues( if isNullish(tmp) && !isProvidedNullVariable(value, variableValues) { tmp = argDef.DefaultValue } + // Spec §6.4.1 ②: a non-null argument is a field error when no value was + // supplied or the supplied value is null. The default was already applied + // above, so a nullish value here means the caller really sent null, or sent + // nothing and the argument declares no default. + if _, isNonNull := argDef.Type.(*NonNull); isNonNull && isNullish(tmp) { + var nodes []ast.Node + if argAST != nil { + nodes = []ast.Node{argAST} + } + return nil, gqlerrors.NewError( + fmt.Sprintf(`Argument "%v" of non-null type "%v" must not be null.`, + argDef.PrivateName, argDef.Type), + nodes, + "", + nil, + []int{}, + nil, + ) + } if !isUndefined || !isNullish(tmp) { results[argDef.PrivateName] = tmp } } - return results + return results, nil } // Returns true if value is a reference to a variable the caller did not supply. @@ -134,13 +153,21 @@ func getVariableValue(schema Schema, definitionAST *ast.VariableDefinition, inpu ) } - isValid, messages := isValidInputValue(input, ttype) + // Spec §6.1.2 ①: the default stands in for a value the caller did not supply, + // and that step precedes the non-null requirement in ②. Applying it here is + // what lets a non-null variable declare a default — the validation below + // would otherwise reject the very case the default exists for. An explicitly + // supplied null is a value, so it does not reach this branch. + if !nonSpec && !provided && definitionAST.DefaultValue != nil { + return valueFromAST(definitionAST.DefaultValue, ttype, nil, nonSpec), nil + } + + isValid, messages := isValidInputValue(input, ttype, nonSpec) if isValid { if isNullish(input) { - // The default stands in for a value the caller did not supply. By the - // specification an explicitly supplied null is a value, so it must not - // be replaced by the default. - if definitionAST.DefaultValue != nil && (nonSpec || !provided) { + // Only a schema that opted out still lets the default replace a supplied + // null; the spec-compliant path applied the default above. + if nonSpec && definitionAST.DefaultValue != nil { return valueFromAST(definitionAST.DefaultValue, ttype, nil, nonSpec), nil } } @@ -267,7 +294,7 @@ func typeFromAST(schema Schema, inputTypeAST ast.Type) (Type, error) { // Given a value and a GraphQL type, determine if the value will be // accepted for that type. This is primarily useful for validating the // runtime values of query variables. -func isValidInputValue(value interface{}, ttype Input) (bool, []string) { +func isValidInputValue(value interface{}, ttype Input, nonSpec bool) (bool, []string) { if isNullish(value) { if ttype, ok := ttype.(*NonNull); ok { if ttype.OfType.Name() != "" { @@ -279,7 +306,7 @@ func isValidInputValue(value interface{}, ttype Input) (bool, []string) { } switch ttype := ttype.(type) { case *NonNull: - return isValidInputValue(value, ttype.OfType) + return isValidInputValue(value, ttype.OfType, nonSpec) case *List: valType := reflect.ValueOf(value) if valType.Kind() == reflect.Ptr { @@ -289,14 +316,14 @@ func isValidInputValue(value interface{}, ttype Input) (bool, []string) { messagesReduce := []string{} for i := 0; i < valType.Len(); i++ { val := valType.Index(i).Interface() - _, messages := isValidInputValue(val, ttype.OfType) + _, messages := isValidInputValue(val, ttype.OfType, nonSpec) for idx, message := range messages { messagesReduce = append(messagesReduce, fmt.Sprintf(`In element #%v: %v`, idx+1, message)) } } return (len(messagesReduce) == 0), messagesReduce } - return isValidInputValue(value, ttype.OfType) + return isValidInputValue(value, ttype.OfType, nonSpec) case *InputObject: messagesReduce := []string{} @@ -330,7 +357,17 @@ func isValidInputValue(value interface{}, ttype Input) (bool, []string) { // Ensure every defined field is valid. for _, fieldName := range fieldNames { - _, messages := isValidInputValue(valueMap[fieldName], fields[fieldName].Type) + field := fields[fieldName] + v, ok := valueMap[fieldName] + // 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, because the default stands in for the value the caller did + // not supply. A key that is present and holds null is a supplied value, + // so it is still validated. + if !nonSpec && !ok && field.DefaultValue != nil { + continue + } + _, messages := isValidInputValue(v, field.Type, nonSpec) if messages != nil { for _, message := range messages { messagesReduce = append(messagesReduce, fmt.Sprintf(`In field "%v": %v`, fieldName, message)) diff --git a/values_test.go b/values_test.go index 59b70db..83fdbc3 100644 --- a/values_test.go +++ b/values_test.go @@ -77,3 +77,36 @@ func Test_coerceValue(t *testing.T) { } } } + +// Spec §3.10 / §5.6.4: an input field is required only when its type is non-null +// AND it declares no default value. Pinned at the function level so the three +// states stay distinct: absent-with-default, absent-without-default, explicit null. +func Test_isValidInputValue_NonNullFieldWithDefault(t *testing.T) { + withDefault := NewInputObject(InputObjectConfig{ + Name: "WithDefault", + Fields: InputObjectConfigFieldMap{ + "a": &InputObjectFieldConfig{Type: NewNonNull(String), DefaultValue: "FIELDDEF"}, + }, + }) + withoutDefault := NewInputObject(InputObjectConfig{ + Name: "WithoutDefault", + Fields: InputObjectConfigFieldMap{ + "a": &InputObjectFieldConfig{Type: NewNonNull(String)}, + }, + }) + + if isValid, messages := isValidInputValue(map[string]interface{}{}, withDefault, false); !isValid { + t.Errorf("spec mode: expected an omitted field with a default to be valid, got: %v", messages) + } + if isValid, _ := isValidInputValue(map[string]interface{}{}, withDefault, true); isValid { + t.Error("non-spec mode: expected an omitted non-null field to be invalid") + } + for _, nonSpec := range []bool{false, true} { + if isValid, _ := isValidInputValue(map[string]interface{}{"a": nil}, withDefault, nonSpec); isValid { + t.Errorf("expected an explicit null to be invalid (nonSpec=%v)", nonSpec) + } + if isValid, _ := isValidInputValue(map[string]interface{}{}, withoutDefault, nonSpec); isValid { + t.Errorf("expected an omitted field without a default to be invalid (nonSpec=%v)", nonSpec) + } + } +} diff --git a/variables_test.go b/variables_test.go index a6813b9..8f89e62 100644 --- a/variables_test.go +++ b/variables_test.go @@ -898,21 +898,29 @@ func TestVariables_NonNullableScalars_AllowsNonNullableInputsToBeSetToAValueDire t.Fatalf("Unexpected result, Diff: %v", testutil.Diff(expected, result)) } } -func TestVariables_NonNullableScalars_PassesAlongNullForNonNullableInputsIfExplicitlySetInTheQuery(t *testing.T) { + +// Spec §6.4.1 ②: a non-null argument is a field error when no value was supplied +// or the supplied value is null. Reached here only through Execute, because +// ProvidedNonNullArgumentsRule rejects such a document during validation. Before +// the coercion fix this returned null with no error, leaving the resolver unable +// to tell that a required argument never arrived. +func TestVariables_NonNullableScalars_DoesNotAllowNonNullableInputsToBeOmittedDirectly(t *testing.T) { doc := ` { fieldWithNonNullableStringInput } ` - params := map[string]interface{}{ - "value": "a", - } - expected := &graphql.Result{ Data: map[string]interface{}{ "fieldWithNonNullableStringInput": nil, }, + Errors: []gqlerrors.FormattedError{ + { + Message: `Argument "input" of non-null type "String!" must not be null.`, + Locations: []location.SourceLocation{}, + }, + }, } ast := testutil.TestParse(t, doc) @@ -921,13 +929,9 @@ func TestVariables_NonNullableScalars_PassesAlongNullForNonNullableInputsIfExpli ep := graphql.ExecuteParams{ Schema: variablesTestSchema, AST: ast, - Args: params, } result := testutil.TestExecute(t, ep) - if len(result.Errors) > 0 { - t.Fatalf("wrong result, unexpected errors: %v", result.Errors) - } - if !reflect.DeepEqual(expected, result) { + if !testutil.EqualResults(expected, result) { t.Fatalf("Unexpected result, Diff: %v", testutil.Diff(expected, result)) } } From 2e639ad052287bba9a3606915098b988076ede2b Mon Sep 17 00:00:00 2001 From: ikawaha Date: Tue, 18 Aug 2026 10:00:24 +0900 Subject: [PATCH 06/10] test: cover a nullable variable at a non-null list item --- argument_coercion_test.go | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index 5f39b57..d58c794 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -226,6 +226,19 @@ var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ }, Resolve: probeArgs, }, + // A list whose item type is non-null, with a default on the argument. Spec + // §5.8.5 draws hasLocationDefaultValue from the argument or input object + // field a usage sits in, and a list position carries none. + "probeNonNullItemList": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{ + Type: graphql.NewList(graphql.NewNonNull(graphql.String)), + DefaultValue: []interface{}{"LISTDEF"}, + }, + }, + Resolve: probeArgs, + }, "probeObjectRequired": &graphql.Field{ Type: graphql.String, Args: graphql.FieldConfigArgument{ @@ -920,3 +933,14 @@ func TestArgumentCoercion_NullableVariableAtNonNullWithoutDefault_StaysRejected( `query Probe($x: String) { probeObjectRequired(input: {a: $x}) }`, map[string]interface{}{}, want, want) } + +// Spec §5.8.5 takes hasLocationDefaultValue from the argument or input object +// field the usage sits in; a list position never carries one. The argument's own +// default must therefore not rescue a nullable variable used as a non-null list +// item. +func TestArgumentCoercion_NullableVariableAtNonNullListItem_StaysRejected(t *testing.T) { + want := `ERROR: Variable "$x" of type "String" used in position expecting type "String!".` + runDoModes(t, "probeNonNullItemList", + `query Probe($x: String) { probeNonNullItemList(a: [$x]) }`, + map[string]interface{}{}, want, want) +} From ef933135d0b939c5b9091480243f5981226272e9 Mon Sep 17 00:00:00 2001 From: ikawaha Date: Tue, 18 Aug 2026 11:04:18 +0900 Subject: [PATCH 07/10] fix: raise a field error for a null variable at a non-null input field --- argument_coercion_test.go | 49 +++++++++++++++++++++++ values.go | 84 ++++++++++++++++++++++++++++++++------- 2 files changed, 119 insertions(+), 14 deletions(-) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index d58c794..42e32bc 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -944,3 +944,52 @@ func TestArgumentCoercion_NullableVariableAtNonNullListItem_StaysRejected(t *tes `query Probe($x: String) { probeNonNullItemList(a: [$x]) }`, map[string]interface{}{}, want, want) } + +// Spec §3.10 Input Coercion, fourth rule: "If a variable is provided for an +// input object field, the runtime value of that variable must be used. If the +// runtime value is null and the field type is non-null, a field error must be +// raised. If no runtime value is provided, the variable definition's default +// value should be used. If the variable definition does not provide a default +// value, the input object field definition's default value should be used." +// +// The null case only becomes reachable once §5.8.5 lets a nullable variable sit +// at a non-null field that declares a default; before that the document was +// rejected during validation. +func TestArgumentCoercion_NullVariableAtNonNullInputField_IsAFieldError(t *testing.T) { + nonSpecWant := `ERROR: Variable "$x" of type "String" used in position expecting type "String!".` + + t.Run("field with a default, variable is null -> field error", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldDefault", + `query Probe($x: String) { probeNonNullFieldDefault(input: {a: $x}) }`, + map[string]interface{}{"x": nil}, + nonSpecWant, + "ERROR: Argument \"input\" has invalid value.\nIn field \"a\": Expected \"String!\", found null.") + }) + t.Run("nested field with a default, variable is null -> field error", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldNested", + `query Probe($x: String) { probeNonNullFieldNested(input: {inner: {a: $x}}) }`, + map[string]interface{}{"x": nil}, + nonSpecWant, + "ERROR: Argument \"input\" has invalid value.\nIn field \"inner\": In field \"a\": Expected \"String!\", found null.") + }) + t.Run("list element field with a default, variable is null -> field error", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldList", + `query Probe($x: String) { probeNonNullFieldList(input: [{a: $x}]) }`, + map[string]interface{}{"x": nil}, + nonSpecWant, + "ERROR: Argument \"input\" has invalid value.\nIn element #1: In field \"a\": Expected \"String!\", found null.") + }) + t.Run("field with a default, variable has a value -> value", func(t *testing.T) { + runDoModes(t, "probeNonNullFieldDefault", + `query Probe($x: String) { probeNonNullFieldDefault(input: {a: $x}) }`, + map[string]interface{}{"x": "v"}, + nonSpecWant, `{"keys":["a"],"obj":{"a":"v"}}`) + }) + // A field with no default keeps being rejected during validation, because + // §5.8.5 finds neither a variable nor a location default. + t.Run("field without a default stays a validation error", func(t *testing.T) { + runDoModes(t, "probeObjectRequired", + `query Probe($x: String) { probeObjectRequired(input: {a: $x}) }`, + map[string]interface{}{"x": nil}, nonSpecWant, nonSpecWant) + }) +} diff --git a/values.go b/values.go index 513dbc0..420f544 100644 --- a/values.go +++ b/values.go @@ -84,24 +84,24 @@ func getArgumentValues( if isNullish(tmp) && !isProvidedNullVariable(value, variableValues) { tmp = argDef.DefaultValue } - // Spec §6.4.1 ②: a non-null argument is a field error when no value was - // supplied or the supplied value is null. The default was already applied - // above, so a nullish value here means the caller really sent null, or sent - // nothing and the argument declares no default. - if _, isNonNull := argDef.Type.(*NonNull); isNonNull && isNullish(tmp) { + // Spec §6.4.1 ② for the argument itself, and the fourth rule of input + // object coercion (§3.10) for a field whose value came from a variable: a + // null at a non-null position is a field error. The default was already + // applied above, so a null still standing here is one the caller supplied + // — or, for the argument itself, a value that was never supplied and has + // no default to stand in. + if message, found := firstNonNullViolation(tmp, argDef.Type); found { var nodes []ast.Node if argAST != nil { nodes = []ast.Node{argAST} } - return nil, gqlerrors.NewError( - fmt.Sprintf(`Argument "%v" of non-null type "%v" must not be null.`, - argDef.PrivateName, argDef.Type), - nodes, - "", - nil, - []int{}, - nil, - ) + text := fmt.Sprintf("Argument %q has invalid value.\n%v", argDef.PrivateName, message) + if _, isNonNull := argDef.Type.(*NonNull); isNonNull && isNullish(tmp) { + // The argument itself is the null, so there is no path to report. + text = fmt.Sprintf(`Argument "%v" of non-null type "%v" must not be null.`, + argDef.PrivateName, argDef.Type) + } + return nil, gqlerrors.NewError(text, nodes, "", nil, []int{}, nil) } if !isUndefined || !isNullish(tmp) { results[argDef.PrivateName] = tmp @@ -110,6 +110,62 @@ func getArgumentValues( return results, nil } +// Walks a coerced argument value and reports the first null sitting at a +// non-null position, together with the path leading to it. Spec §6.4.1 ② covers +// the argument itself and the fourth rule of input object coercion (§3.10) +// covers a field whose value came from a variable: in both cases a null that the +// caller supplied where the type is non-null is a field error. +// +// Only entries the coerced value actually carries are visited. A field the +// caller never supplied is absent from the map, and whether that is allowed is +// the validation rules' business, not this walk's. +func firstNonNullViolation(value interface{}, ttype Input) (string, bool) { + switch ttype := ttype.(type) { + case *NonNull: + if isNullish(value) { + if name := ttype.OfType.Name(); name != "" { + return fmt.Sprintf(`Expected "%v!", found null.`, name), true + } + return "Expected non-null value, found null.", true + } + ofType, _ := ttype.OfType.(Input) + return firstNonNullViolation(value, ofType) + case *List: + itemType, _ := ttype.OfType.(Input) + valType := reflect.ValueOf(value) + if valType.Kind() != reflect.Slice { + return "", false + } + for i := 0; i < valType.Len(); i++ { + if message, found := firstNonNullViolation(valType.Index(i).Interface(), itemType); found { + return fmt.Sprintf("In element #%v: %v", i+1, message), true + } + } + case *InputObject: + valueMap, ok := value.(map[string]interface{}) + if !ok { + return "", false + } + // Sorted so the reported field is stable across runs. + fieldNames := make([]string, 0, len(valueMap)) + for fieldName := range valueMap { + fieldNames = append(fieldNames, fieldName) + } + sort.Strings(fieldNames) + fields := ttype.Fields() + for _, fieldName := range fieldNames { + field, ok := fields[fieldName] + if !ok { + continue + } + if message, found := firstNonNullViolation(valueMap[fieldName], field.Type); found { + return fmt.Sprintf("In field %q: %v", fieldName, message), true + } + } + } + return "", false +} + // Returns true if value is a reference to a variable the caller did not supply. func isUnprovidedVariable(value ast.Value, variables map[string]interface{}) bool { v, ok := value.(*ast.Variable) From fac9a8721c6d58639b7f99e09c1efc4f35e33dea Mon Sep 17 00:00:00 2001 From: ikawaha Date: Tue, 18 Aug 2026 14:32:23 +0900 Subject: [PATCH 08/10] fix: treat a nullish default as no default, matching coercion --- argument_coercion_test.go | 77 +++++++++++++++++++++ rules.go | 21 ++++-- rules_provided_non_null_arguments_test.go | 32 +++++++++ rules_variables_in_allowed_position_test.go | 34 +++++++++ values.go | 7 +- values_test.go | 32 +++++++++ 6 files changed, 195 insertions(+), 8 deletions(-) diff --git a/argument_coercion_test.go b/argument_coercion_test.go index 42e32bc..ca26308 100644 --- a/argument_coercion_test.go +++ b/argument_coercion_test.go @@ -108,6 +108,21 @@ var coercionProbeNonNullFieldNestedInputObject = graphql.NewInputObject(graphql. }, }) +// A non-null field whose default is a typed nil pointer. isNullish treats that as +// no value, so coercion will not substitute it; validation has to reach the same +// conclusion or the field goes missing from the coerced map. +var coercionProbeNilString *string + +var coercionProbeNullishDefaultInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ + Name: "CoercionProbeNullishDefaultInput", + Fields: graphql.InputObjectConfigFieldMap{ + "a": &graphql.InputObjectFieldConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: coercionProbeNilString, + }, + }, +}) + // A non-null field with no default: genuinely required, so it stays required in // both modes. var coercionProbeRequiredInputObject = graphql.NewInputObject(graphql.InputObjectConfig{ @@ -239,6 +254,25 @@ var coercionProbeType = graphql.NewObject(graphql.ObjectConfig{ }, Resolve: probeArgs, }, + // A non-null argument whose default is a typed nil pointer: nullish, so + // coercion will not substitute it and the argument stays required. + "probeNullishArgDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: coercionProbeNilString, + }, + }, + Resolve: probeArgs, + }, + "probeNullishFieldDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "input": &graphql.ArgumentConfig{Type: coercionProbeNullishDefaultInputObject}, + }, + Resolve: probeObjectArgs, + }, "probeObjectRequired": &graphql.Field{ Type: graphql.String, Args: graphql.FieldConfigArgument{ @@ -993,3 +1027,46 @@ func TestArgumentCoercion_NullVariableAtNonNullInputField_IsAFieldError(t *testi map[string]interface{}{"x": nil}, nonSpecWant, nonSpecWant) }) } + +// Spec §3.10: a field is optional when its definition "provides a default +// value". A default that coercion cannot substitute — isNullish treats a typed +// nil pointer as no value — does not make the field optional, so a non-null +// field carrying one stays required. Without this the field passed validation +// and was then dropped by coercion, handing the resolver an input object that +// is missing a field its own schema declares non-null. +func TestArgumentCoercion_NullishFieldDefault_DoesNotMakeFieldOptional(t *testing.T) { + t.Run("literal omits the field", func(t *testing.T) { + want := "ERROR: Argument \"input\" has invalid value {}.\nIn field \"a\": Expected \"String!\", found null." + runDoModes(t, "probeNullishFieldDefault", + `{ probeNullishFieldDefault(input: {}) }`, nil, want, want) + }) + t.Run("variable object omits the key", func(t *testing.T) { + want := "ERROR: Variable \"$in\" got invalid value {}.\nIn field \"a\": Expected \"String!\", found null." + runDoModes(t, "probeNullishFieldDefault", + `query Probe($in: CoercionProbeNullishDefaultInput) { probeNullishFieldDefault(input: $in) }`, + map[string]interface{}{"in": map[string]interface{}{}}, want, want) + }) +} + +// The same reasoning as TestArgumentCoercion_NullishFieldDefault, one level up: +// a default coercion will not substitute does not make an argument optional +// either. Spec §5.4.2.1 (§5.4.3 in draft) for the first case, §5.8.5 for the +// rest. +func TestArgumentCoercion_NullishArgumentDefault_KeepsArgumentRequired(t *testing.T) { + t.Run("argument omitted from the document", func(t *testing.T) { + want := `ERROR: Field "probeNullishArgDefault" argument "a" of type "String!" is required but not provided.` + runDoModes(t, "probeNullishArgDefault", `{ probeNullishArgDefault }`, nil, want, want) + }) + + doc := `query Probe($x: String) { probeNullishArgDefault(a: $x) }` + want := `ERROR: Variable "$x" of type "String" used in position expecting type "String!".` + + t.Run("nullable variable, not supplied", func(t *testing.T) { + runDoModes(t, "probeNullishArgDefault", doc, map[string]interface{}{}, want, want) + }) + // Validation is static: the document must be rejected whether or not this + // particular request happens to supply a value for the variable. + t.Run("nullable variable with a value, still rejected", func(t *testing.T) { + runDoModes(t, "probeNullishArgDefault", doc, map[string]interface{}{"x": "v"}, want, want) + }) +} diff --git a/rules.go b/rules.go index 87f67dc..d5ed077 100644 --- a/rules.go +++ b/rules.go @@ -1257,9 +1257,11 @@ func PossibleFragmentSpreadsRule(context *ValidationContext) *ValidationRuleInst 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. 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. + // it declares no default value. Nullish is the test coercion applies, so a + // default 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{ @@ -1286,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 && (nonSpec || argDef.DefaultValue == nil) { + if argDefType, ok := argDef.Type.(*NonNull); ok && (nonSpec || isNullish(argDef.DefaultValue)) { fieldName := "" if fieldAST.Name != nil { fieldName = fieldAST.Name.Value @@ -1327,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 && (nonSpec || argDef.DefaultValue == nil) { + if argDefType, ok := argDef.Type.(*NonNull); ok && (nonSpec || isNullish(argDef.DefaultValue)) { directiveName := "" if directiveAST.Name != nil { directiveName = directiveAST.Name.Value @@ -1682,7 +1684,9 @@ func allowedVariableUsage(schema *Schema, varType Type, varDefaultValue ast.Valu // 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 - hasLocationDefaultValue := locationDefaultValue != nil + // Nullish is the test 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 } @@ -1839,7 +1843,10 @@ func isValidLiteralValue(ttype Input, valueAST ast.Value, nonSpec bool) (bool, [ // 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. - if !nonSpec && !ok && field.DefaultValue != nil { + // + // Nullish is the same test coercion applies, so a default 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 { diff --git a/rules_provided_non_null_arguments_test.go b/rules_provided_non_null_arguments_test.go index 32baf26..9a0483c 100644 --- a/rules_provided_non_null_arguments_test.go +++ b/rules_provided_non_null_arguments_test.go @@ -312,3 +312,35 @@ func TestValidate_ProvidedNonNullArguments_DirectiveArguments_NonSpecErrorsOnNon testutil.RuleError(`Directive "@defer" argument "if" of type "Boolean!" is required but not provided.`, 3, 24), }) } + +// A default coercion will not substitute — a typed nil pointer is nullish — +// does not make a non-null argument optional. +func TestValidate_ProvidedNonNullArguments_FieldArguments_NullishDefaultKeepsArgumentRequired(t *testing.T) { + var nilString *string + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "fieldWithNullishDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "arg": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.Boolean), + DefaultValue: nilString, + }, + }, + }, + }, + }), + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.ProvidedNonNullArgumentsRule, ` + { + fieldWithNullishDefault + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Field "fieldWithNullishDefault" argument "arg" of type "Boolean!" is required but not provided.`, 3, 11), + }) +} diff --git a/rules_variables_in_allowed_position_test.go b/rules_variables_in_allowed_position_test.go index 246e003..76004d0 100644 --- a/rules_variables_in_allowed_position_test.go +++ b/rules_variables_in_allowed_position_test.go @@ -315,3 +315,37 @@ func TestValidate_VariablesInAllowedPosition_NonSpecIgnoresArgumentDefault(t *te `expecting type "String!".`, 2, 19, 3, 24), }) } + +// Spec §5.8.5 hasLocationDefaultValue: a default coercion will not substitute +// does not permit a nullable variable at a non-null location. Validation is +// static, so this holds whatever values the request later supplies. +func TestValidate_VariablesInAllowedPosition_NullishArgumentDefaultIsNotADefault(t *testing.T) { + var nilString *string + schema, err := graphql.NewSchema(graphql.SchemaConfig{ + Query: graphql.NewObject(graphql.ObjectConfig{ + Name: "Query", + Fields: graphql.Fields{ + "withNullishDefault": &graphql.Field{ + Type: graphql.String, + Args: graphql.FieldConfigArgument{ + "a": &graphql.ArgumentConfig{ + Type: graphql.NewNonNull(graphql.String), + DefaultValue: nilString, + }, + }, + }, + }, + }), + }) + if err != nil { + t.Fatalf("Unexpected error, got: %v", err) + } + testutil.ExpectFailsRuleWithSchema(t, &schema, graphql.VariablesInAllowedPositionRule, ` + query Probe($x: String) { + withNullishDefault(a: $x) + } + `, []gqlerrors.FormattedError{ + testutil.RuleError(`Variable "$x" of type "String" used in position `+ + `expecting type "String!".`, 2, 19, 3, 31), + }) +} diff --git a/values.go b/values.go index 420f544..e7cb245 100644 --- a/values.go +++ b/values.go @@ -420,7 +420,12 @@ func isValidInputValue(value interface{}, ttype Input, nonSpec bool) (bool, []st // non-null, because the default stands in for the value the caller did // not supply. A key that is present and holds null is a supplied value, // so it is still validated. - if !nonSpec && !ok && field.DefaultValue != nil { + // + // The test matches the one coercion applies in coerceValue and + // valueFromAST: a nullish default is not a value either of them will + // substitute, so treating it as one here would pass a field that then + // goes missing from the coerced map. + if !nonSpec && !ok && !isNullish(field.DefaultValue) { continue } _, messages := isValidInputValue(v, field.Type, nonSpec) diff --git a/values_test.go b/values_test.go index 83fdbc3..dc03ebf 100644 --- a/values_test.go +++ b/values_test.go @@ -1,6 +1,7 @@ package graphql import ( + "math" "reflect" "testing" ) @@ -110,3 +111,34 @@ func Test_isValidInputValue_NonNullFieldWithDefault(t *testing.T) { } } } + +// A default that is nullish but not nil — a typed nil pointer, say — is not a +// default coercion can substitute: coerceValue and valueFromAST both skip it +// under isNullish. Validation has to agree, otherwise a non-null field passes +// validation and then goes missing from the coerced map. +func Test_isValidInputValue_NullishDefaultIsNotADefault(t *testing.T) { + var nilString *string + for _, tc := range []struct { + name string + defaultVal interface{} + wantValid bool + }{ + {"usable default", "FIELDDEF", true}, + {"no default", nil, false}, + {"typed nil pointer", nilString, false}, + {"NaN", math.NaN(), false}, + } { + t.Run(tc.name, func(t *testing.T) { + in := NewInputObject(InputObjectConfig{ + Name: "NullishDefault" + tc.name, + Fields: InputObjectConfigFieldMap{ + "a": &InputObjectFieldConfig{Type: NewNonNull(String), DefaultValue: tc.defaultVal}, + }, + }) + isValid, messages := isValidInputValue(map[string]interface{}{}, in, false) + if isValid != tc.wantValid { + t.Errorf("isValid = %v, want %v (messages: %v)", isValid, tc.wantValid, messages) + } + }) + } +} From 840fbbe1c3a6dc1d4015ba1bc849cabffbea75c8 Mon Sep 17 00:00:00 2001 From: ikawaha Date: Tue, 18 Aug 2026 15:38:22 +0900 Subject: [PATCH 09/10] fix: test @skip and @include structurally, as CollectFields does --- directives_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++++++ executor.go | 37 +++++++++++++++---------- 2 files changed, 91 insertions(+), 15 deletions(-) diff --git a/directives_test.go b/directives_test.go index 991a8c8..0e6f36a 100644 --- a/directives_test.go +++ b/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"}`) + }) +} diff --git a/executor.go b/executor.go index 6f2cdcd..ddaec40 100644 --- a/executor.go +++ b/executor.go @@ -493,33 +493,40 @@ func shouldIncludeNode(eCtx *executionContext, directives []*ast.Directive) bool includeAST = directive } } + nonSpec := eCtx.Schema.nonSpecArgumentHandling // precedence: skipAST > includeAST if skipAST != nil { - argValues, err := getArgumentValues(SkipDirective.Args, skipAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) - if err != nil { - // @skip declares "if: Boolean!" with no default, so a null can only get - // here if both variable coercion and validation let it through. Record - // the error rather than dropping it, and leave the node out. - eCtx.Errors = append(eCtx.Errors, gqlerrors.FormatErrorsFromError(err)...) - return false // excluded selectionSet's fields - } - 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, err := getArgumentValues(IncludeDirective.Args, includeAST.Arguments, eCtx.VariableValues, eCtx.Schema.nonSpecArgumentHandling) - if err != nil { - eCtx.Errors = append(eCtx.Errors, gqlerrors.FormatErrorsFromError(err)...) - return false // excluded selectionSet's fields - } - 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 { From f498364eafeb316329a3b3880121f3326511355f Mon Sep 17 00:00:00 2001 From: ikawaha Date: Tue, 18 Aug 2026 15:51:01 +0900 Subject: [PATCH 10/10] docs: add the missing relative pronouns to the isNullish comments --- rules.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/rules.go b/rules.go index d5ed077..b3240e9 100644 --- a/rules.go +++ b/rules.go @@ -1257,11 +1257,11 @@ func PossibleFragmentSpreadsRule(context *ValidationContext) *ValidationRuleInst 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 coercion applies, so a - // default 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. + // 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{ @@ -1684,7 +1684,7 @@ func allowedVariableUsage(schema *Schema, varType Type, varDefaultValue ast.Valu // 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 coercion applies to a declared default, so a + // 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 { @@ -1844,8 +1844,9 @@ func isValidLiteralValue(ttype Input, valueAST ast.Value, nonSpec bool) (bool, [ // 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 coercion applies, so a default coercion will - // not substitute does not make the field optional here either. + // 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 }