From c2135f3ead4c76e479af4ae89e6cd9d1abdc2220 Mon Sep 17 00:00:00 2001 From: Richard Musiol Date: Tue, 23 May 2017 17:32:46 +0200 Subject: [PATCH] validation: NoFragmentCycles Fixes #38. --- internal/query/query.go | 3 + internal/tests/testdata/export.js | 2 +- internal/tests/testdata/tests.json | 307 +++++++++++++++++++++++++++++ internal/validation/validation.go | 103 +++++++--- 4 files changed, 392 insertions(+), 23 deletions(-) diff --git a/internal/query/query.go b/internal/query/query.go index ff83ae57629..bdd63444767 100644 --- a/internal/query/query.go +++ b/internal/query/query.go @@ -90,6 +90,7 @@ type InlineFragment struct { type FragmentSpread struct { Name lexer.Ident Directives common.DirectiveList + Loc errors.Location } func (Field) isSelection() {} @@ -212,6 +213,7 @@ func parseField(l *lexer.Lexer) *Field { } func parseSpread(l *lexer.Lexer) Selection { + loc := l.Location() l.ConsumeToken('.') l.ConsumeToken('.') l.ConsumeToken('.') @@ -222,6 +224,7 @@ func parseSpread(l *lexer.Lexer) Selection { if ident.Name != "on" { fs := &FragmentSpread{ Name: ident, + Loc: loc, } fs.Directives = common.ParseDirectives(l) return fs diff --git a/internal/tests/testdata/export.js b/internal/tests/testdata/export.js index 4908de89cf5..2a0842b4160 100644 --- a/internal/tests/testdata/export.js +++ b/internal/tests/testdata/export.js @@ -63,7 +63,7 @@ require('./src/validation/__tests__/KnownDirectives-test'); 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__/NoFragmentCycles-test'); // require('./src/validation/__tests__/NoUndefinedVariables-test'); require('./src/validation/__tests__/NoUnusedFragments-test'); // require('./src/validation/__tests__/NoUnusedVariables-test'); diff --git a/internal/tests/testdata/tests.json b/internal/tests/testdata/tests.json index 04180b0c040..9b8afb34319 100644 --- a/internal/tests/testdata/tests.json +++ b/internal/tests/testdata/tests.json @@ -1649,6 +1649,313 @@ } ] }, + { + "name": "Validate: No circular fragment spreads/single reference is valid", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB }\n fragment fragB on Dog { name }\n ", + "errors": [] + }, + { + "name": "Validate: No circular fragment spreads/spreading twice is not circular", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB, ...fragB }\n fragment fragB on Dog { name }\n ", + "errors": [] + }, + { + "name": "Validate: No circular fragment spreads/spreading twice indirectly is not circular", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB, ...fragC }\n fragment fragB on Dog { ...fragC }\n fragment fragC on Dog { name }\n ", + "errors": [] + }, + { + "name": "Validate: No circular fragment spreads/double spread within abstract types", + "rule": "NoFragmentCycles", + "query": "\n fragment nameFragment on Pet {\n ... on Dog { name }\n ... on Cat { name }\n }\n\n fragment spreadsInAnon on Pet {\n ... on Dog { ...nameFragment }\n ... on Cat { ...nameFragment }\n }\n ", + "errors": [] + }, + { + "name": "Validate: No circular fragment spreads/does not false positive on unknown fragment", + "rule": "NoFragmentCycles", + "query": "\n fragment nameFragment on Pet {\n ...UnknownFragment\n }\n ", + "errors": [] + }, + { + "name": "Validate: No circular fragment spreads/spreading recursively within field fails", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Human { relatives { ...fragA } },\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself.", + "locations": [ + { + "line": 2, + "column": 45 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself directly", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragA }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself.", + "locations": [ + { + "line": 2, + "column": 31 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself directly within inline fragment", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Pet {\n ... on Dog {\n ...fragA\n }\n }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself.", + "locations": [ + { + "line": 4, + "column": 11 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself indirectly", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB }\n fragment fragB on Dog { ...fragA }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself via fragB.", + "locations": [ + { + "line": 2, + "column": 31 + }, + { + "line": 3, + "column": 31 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself indirectly reports opposite order", + "rule": "NoFragmentCycles", + "query": "\n fragment fragB on Dog { ...fragA }\n fragment fragA on Dog { ...fragB }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragB\" within itself via fragA.", + "locations": [ + { + "line": 2, + "column": 31 + }, + { + "line": 3, + "column": 31 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself indirectly within inline fragment", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Pet {\n ... on Dog {\n ...fragB\n }\n }\n fragment fragB on Pet {\n ... on Dog {\n ...fragA\n }\n }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself via fragB.", + "locations": [ + { + "line": 4, + "column": 11 + }, + { + "line": 9, + "column": 11 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself deeply", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB }\n fragment fragB on Dog { ...fragC }\n fragment fragC on Dog { ...fragO }\n fragment fragX on Dog { ...fragY }\n fragment fragY on Dog { ...fragZ }\n fragment fragZ on Dog { ...fragO }\n fragment fragO on Dog { ...fragP }\n fragment fragP on Dog { ...fragA, ...fragX }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself via fragB, fragC, fragO, fragP.", + "locations": [ + { + "line": 2, + "column": 31 + }, + { + "line": 3, + "column": 31 + }, + { + "line": 4, + "column": 31 + }, + { + "line": 8, + "column": 31 + }, + { + "line": 9, + "column": 31 + } + ] + }, + { + "message": "Cannot spread fragment \"fragO\" within itself via fragP, fragX, fragY, fragZ.", + "locations": [ + { + "line": 8, + "column": 31 + }, + { + "line": 9, + "column": 41 + }, + { + "line": 5, + "column": 31 + }, + { + "line": 6, + "column": 31 + }, + { + "line": 7, + "column": 31 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself deeply two paths", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB, ...fragC }\n fragment fragB on Dog { ...fragA }\n fragment fragC on Dog { ...fragA }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself via fragB.", + "locations": [ + { + "line": 2, + "column": 31 + }, + { + "line": 3, + "column": 31 + } + ] + }, + { + "message": "Cannot spread fragment \"fragA\" within itself via fragC.", + "locations": [ + { + "line": 2, + "column": 41 + }, + { + "line": 4, + "column": 31 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself deeply two paths -- alt traverse order", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragC }\n fragment fragB on Dog { ...fragC }\n fragment fragC on Dog { ...fragA, ...fragB }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragA\" within itself via fragC.", + "locations": [ + { + "line": 2, + "column": 31 + }, + { + "line": 4, + "column": 31 + } + ] + }, + { + "message": "Cannot spread fragment \"fragC\" within itself via fragB.", + "locations": [ + { + "line": 4, + "column": 41 + }, + { + "line": 3, + "column": 31 + } + ] + } + ] + }, + { + "name": "Validate: No circular fragment spreads/no spreading itself deeply and immediately", + "rule": "NoFragmentCycles", + "query": "\n fragment fragA on Dog { ...fragB }\n fragment fragB on Dog { ...fragB, ...fragC }\n fragment fragC on Dog { ...fragA, ...fragB }\n ", + "errors": [ + { + "message": "Cannot spread fragment \"fragB\" within itself.", + "locations": [ + { + "line": 3, + "column": 31 + } + ] + }, + { + "message": "Cannot spread fragment \"fragA\" within itself via fragB, fragC.", + "locations": [ + { + "line": 2, + "column": 31 + }, + { + "line": 3, + "column": 41 + }, + { + "line": 4, + "column": 31 + } + ] + }, + { + "message": "Cannot spread fragment \"fragB\" within itself via fragC.", + "locations": [ + { + "line": 3, + "column": 41 + }, + { + "line": 4, + "column": 41 + } + ] + } + ] + }, { "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 6f73c3c6de1..7f01c2d705a 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -3,8 +3,8 @@ package validation import ( "fmt" "math" - "sort" "strconv" + "strings" "text/scanner" "github.com/neelance/graphql-go/errors" @@ -15,10 +15,9 @@ import ( ) type context struct { - schema *schema.Schema - doc *query.Document - errs []*errors.QueryError - usedFragments map[*query.FragmentDecl]struct{} + schema *schema.Schema + doc *query.Document + errs []*errors.QueryError } func (c *context) addErr(loc errors.Location, rule string, format string, a ...interface{}) { @@ -35,12 +34,12 @@ func (c *context) addErrMultiLoc(locs []errors.Location, rule string, format str func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { c := context{ - schema: s, - doc: doc, - usedFragments: make(map[*query.FragmentDecl]struct{}), + schema: s, + doc: doc, } opNames := make(nameSet) + fragUsed := make(map[*query.FragmentDecl]struct{}) 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.") @@ -83,32 +82,35 @@ func Validate(s *schema.Schema, doc *query.Document) []*errors.QueryError { panic("unreachable") } c.shallowValidateSelectionSet(op.SelSet, entryPoint) + c.markUsedFragments(op.SelSet, fragUsed) } 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) + t := c.resolveType(&frag.On) // continue even if t is nil if t != nil && !canBeFragment(t) { c.addErr(frag.On.Loc, "FragmentsOnCompositeTypes", "Fragment %q cannot condition on non composite type %q.", frag.Name.Name, t) continue } + c.shallowValidateSelectionSet(frag.SelSet, t) - } - for _, op := range doc.Operations { - c.deepValidateSelectionSet(op.SelSet) + if _, ok := fragVisited[frag]; !ok { + c.detectFragmentCycle(frag.SelSet, fragVisited, nil, map[string]int{frag.Name.Name: 0}) + } } for _, frag := range doc.Fragments { - if _, ok := c.usedFragments[frag]; !ok { + if _, ok := fragUsed[frag]; !ok { c.addErr(frag.Loc, "NoUnusedFragments", "Fragment %q is never used.", frag.Name.Name) } } - sort.Slice(c.errs, func(i, j int) bool { return c.errs[i].Locations[0].Before(c.errs[j].Locations[0]) }) return c.errs } @@ -207,32 +209,89 @@ func (c *context) shallowValidateSelection(sel query.Selection, t common.Type) { } } -func (c *context) deepValidateSelectionSet(selSet *query.SelectionSet) { +func (c *context) markUsedFragments(selSet *query.SelectionSet, fragUsed map[*query.FragmentDecl]struct{}) { + for _, sel := range selSet.Selections { + switch sel := sel.(type) { + case *query.Field: + if sel.SelSet != nil { + c.markUsedFragments(sel.SelSet, fragUsed) + } + + case *query.InlineFragment: + c.markUsedFragments(sel.SelSet, fragUsed) + + case *query.FragmentSpread: + frag := c.doc.Fragments.Get(sel.Name.Name) + if frag == nil { + return + } + + if _, ok := fragUsed[frag]; ok { + return + } + fragUsed[frag] = struct{}{} + c.markUsedFragments(frag.SelSet, fragUsed) + + default: + panic("unreachable") + } + } +} + +func (c *context) detectFragmentCycle(selSet *query.SelectionSet, fragVisited map[*query.FragmentDecl]struct{}, spreadPath []*query.FragmentSpread, spreadPathIndex map[string]int) { for _, sel := range selSet.Selections { - c.deepValidateSelection(sel) + c.detectFragmentCycleSel(sel, fragVisited, spreadPath, spreadPathIndex) } } -func (c *context) deepValidateSelection(sel query.Selection) { +func (c *context) detectFragmentCycleSel(sel query.Selection, fragVisited map[*query.FragmentDecl]struct{}, spreadPath []*query.FragmentSpread, spreadPathIndex map[string]int) { switch sel := sel.(type) { case *query.Field: if sel.SelSet != nil { - c.deepValidateSelectionSet(sel.SelSet) + c.detectFragmentCycle(sel.SelSet, fragVisited, spreadPath, spreadPathIndex) } case *query.InlineFragment: - c.deepValidateSelectionSet(sel.SelSet) + c.detectFragmentCycle(sel.SelSet, fragVisited, spreadPath, spreadPathIndex) case *query.FragmentSpread: - if frag := c.doc.Fragments.Get(sel.Name.Name); frag != nil { - c.usedFragments[frag] = struct{}{} - c.deepValidateSelectionSet(frag.SelSet) + frag := c.doc.Fragments.Get(sel.Name.Name) + if frag == nil { + return + } + + spreadPath = append(spreadPath, sel) + if i, ok := spreadPathIndex[frag.Name.Name]; ok { + cyclePath := spreadPath[i:] + via := "" + if len(cyclePath) > 1 { + names := make([]string, len(cyclePath)-1) + for i, frag := range cyclePath[:len(cyclePath)-1] { + names[i] = frag.Name.Name + } + via = " via " + strings.Join(names, ", ") + } + + locs := make([]errors.Location, len(cyclePath)) + for i, frag := range cyclePath { + locs[i] = frag.Loc + } + c.addErrMultiLoc(locs, "NoFragmentCycles", "Cannot spread fragment %q within itself%s.", frag.Name.Name, via) + return + } + + if _, ok := fragVisited[frag]; ok { + return } + fragVisited[frag] = struct{}{} + + spreadPathIndex[frag.Name.Name] = len(spreadPath) + c.detectFragmentCycle(frag.SelSet, fragVisited, spreadPath, spreadPathIndex) + delete(spreadPathIndex, frag.Name.Name) default: panic("unreachable") } - return } func fields(t common.Type) schema.FieldList {