From 83e2f31aa8b8fd94d6fb0d47dbf1676907e07631 Mon Sep 17 00:00:00 2001 From: Richard Musiol Date: Tue, 23 May 2017 21:39:07 +0200 Subject: [PATCH] validation: NoUndefinedVariables --- internal/query/query.go | 16 +- internal/tests/testdata/export.js | 2 +- internal/tests/testdata/tests.json | 372 +++++++++++++++++++++++++++++ internal/validation/validation.go | 75 ++++-- 4 files changed, 433 insertions(+), 32 deletions(-) diff --git a/internal/query/query.go b/internal/query/query.go index 78f07b4caca..69994d5b591 100644 --- a/internal/query/query.go +++ b/internal/query/query.go @@ -42,6 +42,7 @@ type Operation struct { Vars common.InputValueList SelSet *SelectionSet Directives common.DirectiveList + Loc errors.Location } type OperationType string @@ -119,8 +120,7 @@ func parseDocument(l *common.Lexer) *Document { d := &Document{} for l.Peek() != scanner.EOF { if l.Peek() == '{' { - op := &Operation{Type: Query} - op.Name.Loc = l.Location() + op := &Operation{Type: Query, Loc: l.Location()} op.SelSet = parseSelectionSet(l) d.Operations = append(d.Operations, op) continue @@ -129,7 +129,9 @@ func parseDocument(l *common.Lexer) *Document { loc := l.Location() switch x := l.ConsumeIdent(); x { case "query": - d.Operations = append(d.Operations, parseOperation(l, Query)) + op := parseOperation(l, Query) + op.Loc = loc + d.Operations = append(d.Operations, op) case "mutation": d.Operations = append(d.Operations, parseOperation(l, Mutation)) @@ -138,7 +140,9 @@ func parseDocument(l *common.Lexer) *Document { d.Operations = append(d.Operations, parseOperation(l, Subscription)) case "fragment": - d.Fragments = append(d.Fragments, parseFragment(l, loc)) + frag := parseFragment(l) + frag.Loc = loc + d.Fragments = append(d.Fragments, frag) default: l.SyntaxError(fmt.Sprintf(`unexpected %q, expecting "fragment"`, x)) @@ -166,8 +170,8 @@ func parseOperation(l *common.Lexer, opType OperationType) *Operation { return op } -func parseFragment(l *common.Lexer, loc errors.Location) *FragmentDecl { - f := &FragmentDecl{Loc: loc} +func parseFragment(l *common.Lexer) *FragmentDecl { + f := &FragmentDecl{} f.Name = l.ConsumeIdentWithLoc() l.ConsumeKeyword("on") f.On = common.TypeName{Ident: l.ConsumeIdentWithLoc()} diff --git a/internal/tests/testdata/export.js b/internal/tests/testdata/export.js index b295682fc1f..0403d99edf2 100644 --- a/internal/tests/testdata/export.js +++ b/internal/tests/testdata/export.js @@ -64,7 +64,7 @@ require('./src/validation/__tests__/KnownFragmentNames-test'); require('./src/validation/__tests__/KnownTypeNames-test'); require('./src/validation/__tests__/LoneAnonymousOperation-test'); require('./src/validation/__tests__/NoFragmentCycles-test'); -// require('./src/validation/__tests__/NoUndefinedVariables-test'); +require('./src/validation/__tests__/NoUndefinedVariables-test'); require('./src/validation/__tests__/NoUnusedFragments-test'); // require('./src/validation/__tests__/NoUnusedVariables-test'); // require('./src/validation/__tests__/OverlappingFieldsCanBeMerged-test'); diff --git a/internal/tests/testdata/tests.json b/internal/tests/testdata/tests.json index 43e247639ee..10c026ad1be 100644 --- a/internal/tests/testdata/tests.json +++ b/internal/tests/testdata/tests.json @@ -1956,6 +1956,378 @@ } ] }, + { + "name": "Validate: No undefined variables/all variables defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String, $b: String, $c: String) {\n field(a: $a, b: $b, c: $c)\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/all variables deeply defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String, $b: String, $c: String) {\n field(a: $a) {\n field(b: $b) {\n field(c: $c)\n }\n }\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/all variables deeply in inline fragments defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String, $b: String, $c: String) {\n ... on Type {\n field(a: $a) {\n field(b: $b) {\n ... on Type {\n field(c: $c)\n }\n }\n }\n }\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/all variables in fragments deeply defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String, $b: String, $c: String) {\n ...FragA\n }\n fragment FragA on Type {\n field(a: $a) {\n ...FragB\n }\n }\n fragment FragB on Type {\n field(b: $b) {\n ...FragC\n }\n }\n fragment FragC on Type {\n field(c: $c)\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/variable within single fragment defined in multiple operations", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String) {\n ...FragA\n }\n query Bar($a: String) {\n ...FragA\n }\n fragment FragA on Type {\n field(a: $a)\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/variable within fragments defined in operations", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String) {\n ...FragA\n }\n query Bar($b: String) {\n ...FragB\n }\n fragment FragA on Type {\n field(a: $a)\n }\n fragment FragB on Type {\n field(b: $b)\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/variable within recursive fragment defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String) {\n ...FragA\n }\n fragment FragA on Type {\n field(a: $a) {\n ...FragA\n }\n }\n ", + "errors": [] + }, + { + "name": "Validate: No undefined variables/variable not defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String, $b: String, $c: String) {\n field(a: $a, b: $b, c: $c, d: $d)\n }\n ", + "errors": [ + { + "message": "Variable \"$d\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 3, + "column": 39 + }, + { + "line": 2, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/variable not defined by un-named query", + "rule": "NoUndefinedVariables", + "query": "\n {\n field(a: $a)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined.", + "locations": [ + { + "line": 3, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/multiple variables not defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($b: String) {\n field(a: $a, b: $b, c: $c)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 3, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$c\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 3, + "column": 32 + }, + { + "line": 2, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/variable in fragment not defined by un-named query", + "rule": "NoUndefinedVariables", + "query": "\n {\n ...FragA\n }\n fragment FragA on Type {\n field(a: $a)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined.", + "locations": [ + { + "line": 6, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/variable in fragment not defined by operation", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String, $b: String) {\n ...FragA\n }\n fragment FragA on Type {\n field(a: $a) {\n ...FragB\n }\n }\n fragment FragB on Type {\n field(b: $b) {\n ...FragC\n }\n }\n fragment FragC on Type {\n field(c: $c)\n }\n ", + "errors": [ + { + "message": "Variable \"$c\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 16, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/multiple variables in fragments not defined", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($b: String) {\n ...FragA\n }\n fragment FragA on Type {\n field(a: $a) {\n ...FragB\n }\n }\n fragment FragB on Type {\n field(b: $b) {\n ...FragC\n }\n }\n fragment FragC on Type {\n field(c: $c)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 6, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$c\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 16, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/single variable in fragment not defined by multiple operations", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($a: String) {\n ...FragAB\n }\n query Bar($a: String) {\n ...FragAB\n }\n fragment FragAB on Type {\n field(a: $a, b: $b)\n }\n ", + "errors": [ + { + "message": "Variable \"$b\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 9, + "column": 25 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$b\" is not defined by operation \"Bar\".", + "locations": [ + { + "line": 9, + "column": 25 + }, + { + "line": 5, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/variables in fragment not defined by multiple operations", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($b: String) {\n ...FragAB\n }\n query Bar($a: String) {\n ...FragAB\n }\n fragment FragAB on Type {\n field(a: $a, b: $b)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 9, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$b\" is not defined by operation \"Bar\".", + "locations": [ + { + "line": 9, + "column": 25 + }, + { + "line": 5, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/variable in fragment used by other operation", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($b: String) {\n ...FragA\n }\n query Bar($a: String) {\n ...FragB\n }\n fragment FragA on Type {\n field(a: $a)\n }\n fragment FragB on Type {\n field(b: $b)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 9, + "column": 18 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$b\" is not defined by operation \"Bar\".", + "locations": [ + { + "line": 12, + "column": 18 + }, + { + "line": 5, + "column": 7 + } + ] + } + ] + }, + { + "name": "Validate: No undefined variables/multiple undefined variables produce multiple errors", + "rule": "NoUndefinedVariables", + "query": "\n query Foo($b: String) {\n ...FragAB\n }\n query Bar($a: String) {\n ...FragAB\n }\n fragment FragAB on Type {\n field1(a: $a, b: $b)\n ...FragC\n field3(a: $a, b: $b)\n }\n fragment FragC on Type {\n field2(c: $c)\n }\n ", + "errors": [ + { + "message": "Variable \"$a\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 9, + "column": 19 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$a\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 11, + "column": 19 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$c\" is not defined by operation \"Foo\".", + "locations": [ + { + "line": 14, + "column": 19 + }, + { + "line": 2, + "column": 7 + } + ] + }, + { + "message": "Variable \"$b\" is not defined by operation \"Bar\".", + "locations": [ + { + "line": 9, + "column": 26 + }, + { + "line": 5, + "column": 7 + } + ] + }, + { + "message": "Variable \"$b\" is not defined by operation \"Bar\".", + "locations": [ + { + "line": 11, + "column": 26 + }, + { + "line": 5, + "column": 7 + } + ] + }, + { + "message": "Variable \"$c\" is not defined by operation \"Bar\".", + "locations": [ + { + "line": 14, + "column": 19 + }, + { + "line": 5, + "column": 7 + } + ] + } + ] + }, { "name": "Validate: No unused fragments/all fragment names are used", "rule": "NoUnusedFragments", diff --git a/internal/validation/validation.go b/internal/validation/validation.go index ef25d3f10d4..e30ba156365 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -17,6 +17,7 @@ type context struct { schema *schema.Schema doc *query.Document errs []*errors.QueryError + opErrs map[*query.Operation][]*errors.QueryError } func (c *context) addErr(loc errors.Location, rule string, format string, a ...interface{}) { @@ -35,19 +36,20 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { c := context{ schema: s, doc: doc, + opErrs: make(map[*query.Operation][]*errors.QueryError), } opNames := make(nameSet) - fragUsed := make(map[*query.FragmentDecl]struct{}) + fragUsedBy := make(map[*query.FragmentDecl][]*query.Operation) for _, op := range doc.Operations { if op.Name.Name == "" && len(doc.Operations) != 1 { - c.addErr(op.Name.Loc, "LoneAnonymousOperation", "This anonymous operation must be the only defined operation.") + c.addErr(op.Loc, "LoneAnonymousOperation", "This anonymous operation must be the only defined operation.") } if op.Name.Name != "" { c.validateName(opNames, op.Name, "UniqueOperationNames", "operation") } - c.validateDirectives(string(op.Type), op.Directives) + c.validateDirectives(string(op.Type), op.Directives, nil) varNames := make(nameSet) for _, v := range op.Vars { @@ -59,7 +61,7 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { } if v.Default != nil { - c.validateLiteral(v.Default) + c.validateLiteral(v.Default, nil) if t != nil { if nn, ok := t.(*common.NonNull); ok { @@ -84,15 +86,21 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { default: panic("unreachable") } - c.validateSelectionSet(op.SelSet, entryPoint) + + c.validateSelectionSet(op.SelSet, entryPoint, []*query.Operation{op}) + + fragUsed := make(map[*query.FragmentDecl]struct{}) c.markUsedFragments(op.SelSet, fragUsed) + for frag := range fragUsed { + fragUsedBy[frag] = append(fragUsedBy[frag], op) + } } fragNames := make(nameSet) fragVisited := make(map[*query.FragmentDecl]struct{}) for _, frag := range doc.Fragments { c.validateName(fragNames, frag.Name, "UniqueFragmentNames", "fragment") - c.validateDirectives("FRAGMENT_DEFINITION", frag.Directives) + c.validateDirectives("FRAGMENT_DEFINITION", frag.Directives, nil) t := c.resolveType(&frag.On) // continue even if t is nil @@ -101,7 +109,7 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { continue } - c.validateSelectionSet(frag.SelSet, t) + c.validateSelectionSet(frag.SelSet, t, fragUsedBy[frag]) if _, ok := fragVisited[frag]; !ok { c.detectFragmentCycle(frag.SelSet, fragVisited, nil, map[string]int{frag.Name.Name: 0}) @@ -109,24 +117,28 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { } for _, frag := range doc.Fragments { - if _, ok := fragUsed[frag]; !ok { + if len(fragUsedBy[frag]) == 0 { c.addErr(frag.Loc, "NoUnusedFragments", "Fragment %q is never used.", frag.Name.Name) } } + for _, op := range doc.Operations { + c.errs = append(c.errs, c.opErrs[op]...) + } + return c.errs } -func (c *context) validateSelectionSet(selSet *query.SelectionSet, t common.Type) { +func (c *context) validateSelectionSet(selSet *query.SelectionSet, t common.Type, ops []*query.Operation) { for _, sel := range selSet.Selections { - c.validateSelection(sel, t) + c.validateSelection(sel, t, ops) } } -func (c *context) validateSelection(sel query.Selection, t common.Type) { +func (c *context) validateSelection(sel query.Selection, t common.Type, ops []*query.Operation) { switch sel := sel.(type) { case *query.Field: - c.validateDirectives("FIELD", sel.Directives) + c.validateDirectives("FIELD", sel.Directives, ops) fieldName := sel.Name.Name var f *schema.Field @@ -160,7 +172,7 @@ func (c *context) validateSelection(sel query.Selection, t common.Type) { } } - c.validateArgumentLiterals(sel.Arguments) + c.validateArgumentLiterals(sel.Arguments, ops) if f != nil { c.validateArgumentTypes(sel.Arguments, f.Args, sel.Alias.Loc, func() string { return fmt.Sprintf("field %q of type %q", fieldName, t) }, @@ -180,11 +192,11 @@ func (c *context) validateSelection(sel query.Selection, t common.Type) { } } if sel.SelSet != nil { - c.validateSelectionSet(sel.SelSet, unwrapType(ft)) + c.validateSelectionSet(sel.SelSet, unwrapType(ft), ops) } case *query.InlineFragment: - c.validateDirectives("INLINE_FRAGMENT", sel.Directives) + c.validateDirectives("INLINE_FRAGMENT", sel.Directives, ops) if sel.On.Name != "" { fragTyp := c.resolveType(&sel.On) if fragTyp != nil && !compatible(t, fragTyp) { @@ -197,10 +209,10 @@ func (c *context) validateSelection(sel query.Selection, t common.Type) { c.addErr(sel.On.Loc, "FragmentsOnCompositeTypes", "Fragment cannot condition on non composite type %q.", t) return } - c.validateSelectionSet(sel.SelSet, unwrapType(t)) + c.validateSelectionSet(sel.SelSet, unwrapType(t), ops) case *query.FragmentSpread: - c.validateDirectives("FRAGMENT_SPREAD", sel.Directives) + c.validateDirectives("FRAGMENT_SPREAD", sel.Directives, ops) frag := c.doc.Fragments.Get(sel.Name.Name) if frag == nil { c.addErr(sel.Name.Loc, "KnownFragmentNames", "Unknown fragment %q.", sel.Name.Name) @@ -355,7 +367,7 @@ func (c *context) resolveType(t common.Type) common.Type { return t2 } -func (c *context) validateDirectives(loc string, directives common.DirectiveList) { +func (c *context) validateDirectives(loc string, directives common.DirectiveList, ops []*query.Operation) { directiveNames := make(nameSet) for _, d := range directives { dirName := d.Name.Name @@ -363,7 +375,7 @@ func (c *context) validateDirectives(loc string, directives common.DirectiveList return fmt.Sprintf("The directive %q can only be used once at this location.", dirName) }) - c.validateArgumentLiterals(d.Args) + c.validateArgumentLiterals(d.Args, ops) dd, ok := c.schema.Directives[dirName] if !ok { @@ -428,25 +440,39 @@ func (c *context) validateArgumentTypes(args common.ArgumentList, argDecls commo } } -func (c *context) validateArgumentLiterals(args common.ArgumentList) { +func (c *context) validateArgumentLiterals(args common.ArgumentList, ops []*query.Operation) { argNames := make(nameSet) for _, arg := range args { c.validateName(argNames, arg.Name, "UniqueArgumentNames", "argument") - c.validateLiteral(arg.Value) + c.validateLiteral(arg.Value, ops) } } -func (c *context) validateLiteral(l common.Literal) { +func (c *context) validateLiteral(l common.Literal, ops []*query.Operation) { switch l := l.(type) { case *common.ObjectLit: fieldNames := make(nameSet) for _, f := range l.Fields { c.validateName(fieldNames, f.Name, "UniqueInputFieldNames", "input field") - c.validateLiteral(f.Value) + c.validateLiteral(f.Value, ops) } case *common.ListLit: for _, entry := range l.Entries { - c.validateLiteral(entry) + c.validateLiteral(entry, ops) + } + case *common.Variable: + for _, op := range ops { + if op.Vars.Get(l.Name) == nil { + byOp := "" + if op.Name.Name != "" { + byOp = fmt.Sprintf(" by operation %q", op.Name.Name) + } + c.opErrs[op] = append(c.opErrs[op], &errors.QueryError{ + Message: fmt.Sprintf("Variable %q is not defined%s.", "$"+l.Name, byOp), + Locations: []errors.Location{l.Loc, op.Loc}, + Rule: "NoUndefinedVariables", + }) + } } } } @@ -463,7 +489,6 @@ func validateValueType(v common.Literal, t common.Type) (bool, string) { } if _, ok := v.(*common.Variable); ok { - // TODO return true, "" }