[Validation] perf improvements for fragment cycle detection

This converts the O(N^2) cycle detector to an O(N) detector and optimizes the visit to short-circuit where parts of the validation visitor were unnecessary.

On an internal very large stress test query this was previously ~30s and after this diff runs in 15ms.
Commit:
4cf2190b54fefc34a7b37d22a489933fc15e14ce [4cf2190]
Parents:
a738c6d90d
Author:
Lee Byron <[email protected]>
Date:
13 November 2015 at 3:47:01 PM SGT

[Validation] Performance improvements

Fairly dramatic improvement of some validation rules by short-circuiting branches which do not need to be checked and memoizing the process of collecting fragment spreads from a given context - which is necessary by multiple rules.
Commit:
0bc9088187b9902ab19c0ec34e0e9f036dc9d9ea [0bc9088]
Parents:
7278b7c92c
Author:
Lee Byron <[email protected]>
Date:
13 November 2015 at 4:26:05 PM SGT
Labels:
HEAD

[Validation] Report errors rather than return them

This replaces the mechanism of returning errors or lists of errors from a validator step to instead report those errors via calling a function on the context.

This simplifies the implementation of the visitor mechanism, but also opens the doors for future rules being specified as warnings instead of errors.

Author: sogko

language/ast/selections.go

@@ -5,6 +5,7 @@ import (
 )
 
 type Selection interface {
+	GetSelectionSet() *SelectionSet
 }
 
 // Ensure that all definition types implements Selection interface

rules.go

@@ -713,93 +713,113 @@ func (set *nodeSet) Add(node ast.Node) bool {
 	return true
 }
 
+func CycleErrorMessage(fragName string, spreadNames []string) string {
+	via := ""
+	if len(spreadNames) > 0 {
+		via = " via " + strings.Join(spreadNames, ", ")
+	}
+	return fmt.Sprintf(`Cannot spread fragment "%v" within itself%v.`, fragName, via)
+}
+
 /**
  * NoFragmentCyclesRule
  */
 func NoFragmentCyclesRule(context *ValidationContext) *ValidationRuleInstance {
-	// Gather all the fragment spreads ASTs for each fragment definition.
-	// Importantly this does not include inline fragments.
-	definitions := context.Document().Definitions
-	spreadsInFragment := map[string][]*ast.FragmentSpread{}
-	for _, node := range definitions {
-		if node.GetKind() == kinds.FragmentDefinition {
-			if node, ok := node.(*ast.FragmentDefinition); ok && node != nil {
-				nodeName := ""
-				if node.Name != nil {
-					nodeName = node.Name.Value
+
+	// Tracks already visited fragments to maintain O(N) and to ensure that cycles
+	// are not redundantly reported.
+	visitedFrags := map[string]bool{}
+
+	// Array of AST nodes used to produce meaningful errors
+	spreadPath := []*ast.FragmentSpread{}
+
+	// Position in the spread path
+	spreadPathIndexByName := map[string]int{}
+
+	// This does a straight-forward DFS to find cycles.
+	// It does not terminate when a cycle was found but continues to explore
+	// the graph to find all possible cycles.
+	var detectCycleRecursive func(fragment *ast.FragmentDefinition)
+	detectCycleRecursive = func(fragment *ast.FragmentDefinition) {
+
+		fragmentName := ""
+		if fragment.Name != nil {
+			fragmentName = fragment.Name.Value
+		}
+		visitedFrags[fragmentName] = true
+
+		spreadNodes := context.FragmentSpreads(fragment)
+		if len(spreadNodes) == 0 {
+			return
+		}
+
+		spreadPathIndexByName[fragmentName] = len(spreadPath)
+
+		for _, spreadNode := range spreadNodes {
+
+			spreadName := ""
+			if spreadNode.Name != nil {
+				spreadName = spreadNode.Name.Value
+			}
+			cycleIndex, ok := spreadPathIndexByName[spreadName]
+			if !ok {
+				spreadPath = append(spreadPath, spreadNode)
+				if visited, ok := visitedFrags[spreadName]; !ok || !visited {
+					spreadFragment := context.Fragment(spreadName)
+					if spreadFragment != nil {
+						detectCycleRecursive(spreadFragment)
+					}
+				}
+				spreadPath = spreadPath[:len(spreadPath)-1]
+			} else {
+				cyclePath := spreadPath[cycleIndex:]
+
+				spreadNames := []string{}
+				for _, s := range cyclePath {
+					name := ""
+					if s.Name != nil {
+						name = s.Name.Value
+					}
+					spreadNames = append(spreadNames, name)
+				}
+
+				nodes := []ast.Node{}
+				for _, c := range cyclePath {
+					nodes = append(nodes, c)
 				}
-				spreadsInFragment[nodeName] = gatherSpreads(node)
+				nodes = append(nodes, spreadNode)
+
+				reportError(
+					context,
+					CycleErrorMessage(spreadName, spreadNames),
+					nodes,
+				)
 			}
+
 		}
+		delete(spreadPathIndexByName, fragmentName)
+
 	}
-	// Tracks spreads known to lead to cycles to ensure that cycles are not
-	// redundantly reported.
-	knownToLeadToCycle := newNodeSet()
 
 	visitorOpts := &visitor.VisitorOptions{
 		KindFuncMap: map[string]visitor.NamedVisitFuncs{
+			kinds.OperationDefinition: visitor.NamedVisitFuncs{
+				Kind: func(p visitor.VisitFuncParams) (string, interface{}) {
+					return visitor.ActionSkip, nil
+				},
+			},
 			kinds.FragmentDefinition: visitor.NamedVisitFuncs{
 				Kind: func(p visitor.VisitFuncParams) (string, interface{}) {
 					if node, ok := p.Node.(*ast.FragmentDefinition); ok && node != nil {
-						spreadPath := []*ast.FragmentSpread{}
-						initialName := ""
+						nodeName := ""
 						if node.Name != nil {
-							initialName = node.Name.Value
+							nodeName = node.Name.Value
 						}
-						var detectCycleRecursive func(fragmentName string)
-						detectCycleRecursive = func(fragmentName string) {
-							spreadNodes, _ := spreadsInFragment[fragmentName]
-							for _, spreadNode := range spreadNodes {
-								if knownToLeadToCycle.Has(spreadNode) {
-									continue
-								}
-								spreadNodeName := ""
-								if spreadNode.Name != nil {
-									spreadNodeName = spreadNode.Name.Value
-								}
-								if spreadNodeName == initialName {
-									cyclePath := []ast.Node{}
-									for _, path := range spreadPath {
-										cyclePath = append(cyclePath, path)
-									}
-									cyclePath = append(cyclePath, spreadNode)
-									for _, spread := range cyclePath {
-										knownToLeadToCycle.Add(spread)
-									}
-									via := ""
-									spreadNames := []string{}
-									for _, s := range spreadPath {
-										if s.Name != nil {
-											spreadNames = append(spreadNames, s.Name.Value)
-										}
-									}
-									if len(spreadNames) > 0 {
-										via = " via " + strings.Join(spreadNames, ", ")
-									}
-									reportError(
-										context,
-										fmt.Sprintf(`Cannot spread fragment "%v" within itself%v.`, initialName, via),
-										cyclePath,
-									)
-									continue
-								}
-								spreadPathHasCurrentNode := false
-								for _, spread := range spreadPath {
-									if spread == spreadNode {
-										spreadPathHasCurrentNode = true
-									}
-								}
-								if spreadPathHasCurrentNode {
-									continue
-								}
-								spreadPath = append(spreadPath, spreadNode)
-								detectCycleRecursive(spreadNodeName)
-								_, spreadPath = spreadPath[len(spreadPath)-1], spreadPath[:len(spreadPath)-1]
-							}
+						if _, ok := visitedFrags[nodeName]; !ok {
+							detectCycleRecursive(node)
 						}
-						detectCycleRecursive(initialName)
 					}
-					return visitor.ActionNoChange, nil
+					return visitor.ActionSkip, nil
 				},
 			},
 		},
@@ -2163,25 +2183,3 @@ func isValidLiteralValue(ttype Input, valueAST ast.Value) (bool, []string) {
 
 	return true, nil
 }
-
-/**
- * Given an operation or fragment AST node, gather all the
- * named spreads defined within the scope of the fragment
- * or operation
- */
-func gatherSpreads(node ast.Node) (spreadNodes []*ast.FragmentSpread) {
-	visitorOpts := &visitor.VisitorOptions{
-		KindFuncMap: map[string]visitor.NamedVisitFuncs{
-			kinds.FragmentSpread: visitor.NamedVisitFuncs{
-				Kind: func(p visitor.VisitFuncParams) (string, interface{}) {
-					if node, ok := p.Node.(*ast.FragmentSpread); ok && node != nil {
-						spreadNodes = append(spreadNodes, node)
-					}
-					return visitor.ActionNoChange, nil
-				},
-			},
-		},
-	}
-	visitor.Visit(node, visitorOpts, nil)
-	return spreadNodes
-}

rules_no_fragment_cycles_test.go

@@ -115,10 +115,21 @@ func TestValidate_NoCircularFragmentSpreads_NoSpreadingItselfDeeply(t *testing.T
       fragment fragX on Dog { ...fragY }
       fragment fragY on Dog { ...fragZ }
       fragment fragZ on Dog { ...fragO }
-      fragment fragO on Dog { ...fragA, ...fragX }
+      fragment fragO on Dog { ...fragP }
+      fragment fragP on Dog { ...fragA, ...fragX }
     `, []gqlerrors.FormattedError{
-		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragB, fragC, fragO.`, 2, 31, 3, 31, 4, 31, 8, 31),
-		testutil.RuleError(`Cannot spread fragment "fragX" within itself via fragY, fragZ, fragO.`, 5, 31, 6, 31, 7, 31, 8, 41),
+		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragB, fragC, fragO, fragP.`,
+			2, 31,
+			3, 31,
+			4, 31,
+			8, 31,
+			9, 31),
+		testutil.RuleError(`Cannot spread fragment "fragO" within itself via fragP, fragX, fragY, fragZ.`,
+			8, 31,
+			9, 41,
+			5, 31,
+			6, 31,
+			7, 31),
 	})
 }
 func TestValidate_NoCircularFragmentSpreads_NoSpreadingItselfDeeplyTwoPaths(t *testing.T) {
@@ -127,7 +138,41 @@ func TestValidate_NoCircularFragmentSpreads_NoSpreadingItselfDeeplyTwoPaths(t *t
       fragment fragB on Dog { ...fragA }
       fragment fragC on Dog { ...fragA }
     `, []gqlerrors.FormattedError{
-		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragB.`, 2, 31, 3, 31),
-		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragC.`, 2, 41, 4, 31),
+		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragB.`,
+			2, 31,
+			3, 31),
+		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragC.`,
+			2, 41,
+			4, 31),
+	})
+}
+func TestValidate_NoCircularFragmentSpreads_NoSpreadingItselfDeeplyTwoPaths_AltTraverseOrder(t *testing.T) {
+	testutil.ExpectFailsRule(t, graphql.NoFragmentCyclesRule, `
+      fragment fragA on Dog { ...fragC }
+      fragment fragB on Dog { ...fragC }
+      fragment fragC on Dog { ...fragA, ...fragB }
+    `, []gqlerrors.FormattedError{
+		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragC.`,
+			2, 31,
+			4, 31),
+		testutil.RuleError(`Cannot spread fragment "fragC" within itself via fragB.`,
+			4, 41,
+			3, 31),
+	})
+}
+func TestValidate_NoCircularFragmentSpreads_NoSpreadingItselfDeeplyAndImmediately(t *testing.T) {
+	testutil.ExpectFailsRule(t, graphql.NoFragmentCyclesRule, `
+      fragment fragA on Dog { ...fragB }
+      fragment fragB on Dog { ...fragB, ...fragC }
+      fragment fragC on Dog { ...fragA, ...fragB }
+    `, []gqlerrors.FormattedError{
+		testutil.RuleError(`Cannot spread fragment "fragB" within itself.`, 3, 31),
+		testutil.RuleError(`Cannot spread fragment "fragA" within itself via fragB, fragC.`,
+			2, 31,
+			3, 41,
+			4, 31),
+		testutil.RuleError(`Cannot spread fragment "fragB" within itself via fragC.`,
+			3, 41,
+			4, 41),
 	})
 }