Merge pull request #231 from graphql/fast-cycle-detection

[Validation] perf improvements for fragment cycle detection

Author: leebyron

src/validation/__tests__/NoFragmentCycles.js

@@ -136,24 +136,29 @@ describe('Validate: No circular fragment spreads', () => {
       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 }
     `, [
-      { message: cycleErrorMessage('fragA', [ 'fragB', 'fragC', 'fragO' ]),
+      { message:
+          cycleErrorMessage('fragA', [ 'fragB', 'fragC', 'fragO', 'fragP' ]),
         locations: [
           { line: 2, column: 31 },
           { line: 3, column: 31 },
           { line: 4, column: 31 },
-          { line: 8, column: 31 } ] },
-      { message: cycleErrorMessage('fragX', [ 'fragY', 'fragZ', 'fragO' ]),
+          { line: 8, column: 31 },
+          { line: 9, column: 31 } ] },
+      { message:
+          cycleErrorMessage('fragO', [ '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 },
-          { line: 8, column: 41 } ] }
+          { line: 7, column: 31 } ] }
     ]);
   });
 
-  it('no spreading itself deeply two paths -- new rule', () => {
+  it('no spreading itself deeply two paths', () => {
     expectFailsRule(NoFragmentCycles, `
       fragment fragA on Dog { ...fragB, ...fragC }
       fragment fragB on Dog { ...fragA }
@@ -166,4 +171,35 @@ describe('Validate: No circular fragment spreads', () => {
     ]);
   });
 
+  it('no spreading itself deeply two paths -- alt traverse order', () => {
+    expectFailsRule(NoFragmentCycles, `
+      fragment fragA on Dog { ...fragC }
+      fragment fragB on Dog { ...fragC }
+      fragment fragC on Dog { ...fragA, ...fragB }
+    `, [
+      { message: cycleErrorMessage('fragA', [ 'fragC' ]),
+        locations: [ { line: 2, column: 31 }, { line: 4, column: 31 } ] },
+      { message: cycleErrorMessage('fragC', [ 'fragB' ]),
+        locations: [ { line: 4, column: 41 }, { line: 3, column: 31 } ] }
+    ]);
+  });
+
+  it('no spreading itself deeply and immediately', () => {
+    expectFailsRule(NoFragmentCycles, `
+      fragment fragA on Dog { ...fragB }
+      fragment fragB on Dog { ...fragB, ...fragC }
+      fragment fragC on Dog { ...fragA, ...fragB }
+    `, [
+      { message: cycleErrorMessage('fragB', []),
+        locations: [ { line: 3, column: 31 } ] },
+      { message: cycleErrorMessage('fragA', [ 'fragB', 'fragC' ]),
+        locations: [
+          { line: 2, column: 31 },
+          { line: 3, column: 41 },
+          { line: 4, column: 31 } ] },
+      { message: cycleErrorMessage('fragB', [ 'fragC' ]),
+        locations: [ { line: 3, column: 41 }, { line: 4, column: 41 } ] }
+    ]);
+  });
+
 });

src/validation/rules/NoFragmentCycles.js

@@ -10,9 +10,12 @@
 
 import type { ValidationContext } from '../index';
 import { GraphQLError } from '../../error';
-import type { SelectionSet, FragmentSpread } from '../../language/ast';
-import { FRAGMENT_DEFINITION } from '../../language/kinds';
-import { visit } from '../../language/visitor';
+import { FRAGMENT_SPREAD } from '../../language/kinds';
+import type {
+  SelectionSet,
+  FragmentSpread,
+  FragmentDefinition
+} from '../../language/ast';
 
 
 export function cycleErrorMessage(
@@ -24,83 +27,92 @@ export function cycleErrorMessage(
 }
 
 export function NoFragmentCycles(context: ValidationContext): any {
+  var errors = [];
 
-  // Gather all the fragment spreads ASTs for each fragment definition.
-  // Importantly this does not include inline fragments.
-  var definitions = context.getDocument().definitions;
-  var spreadsInFragment = definitions.reduce((map, node) => {
-    if (node.kind === FRAGMENT_DEFINITION) {
-      map[node.name.value] = gatherSpreads(node);
-    }
-    return map;
-  }, {});
+  // Tracks already visited fragments to maintain O(N) and to ensure that cycles
+  // are not redundantly reported.
+  var visitedFrags = Object.create(null);
+
+  // Array of AST nodes used to produce meaningful errors
+  var spreadPath = [];
 
-  // Tracks spreads known to lead to cycles to ensure that cycles are not
-  // redundantly reported.
-  var knownToLeadToCycle = new Set();
+  // Position in the spread path
+  var spreadPathIndexByName = Object.create(null);
 
   return {
+    Document: {
+      leave() {
+        if (errors.length) {
+          return errors;
+        }
+      }
+    },
+    OperationDefinition: () => false,
     FragmentDefinition(node) {
-      var errors = [];
-      var initialName = node.name.value;
+      if (!visitedFrags[node.name.value]) {
+        detectCycleRecursive(node);
+      }
+      return false;
+    },
+  };
 
-      // Array of AST nodes used to produce meaningful errors
-      var spreadPath = [];
+  // 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.
+  function detectCycleRecursive(fragment: FragmentDefinition) {
+    const fragmentName = fragment.name.value;
+    visitedFrags[fragmentName] = true;
 
-      // 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.
-      function detectCycleRecursive(fragmentName) {
-        var spreadNodes = spreadsInFragment[fragmentName];
-        if (spreadNodes) {
-          for (var i = 0; i < spreadNodes.length; ++i) {
-            var spreadNode = spreadNodes[i];
-            if (knownToLeadToCycle.has(spreadNode)) {
-              continue;
-            }
-            if (spreadNode.name.value === initialName) {
-              var cyclePath = spreadPath.concat(spreadNode);
-              cyclePath.forEach(spread => knownToLeadToCycle.add(spread));
-              errors.push(new GraphQLError(
-                cycleErrorMessage(
-                  initialName,
-                  spreadPath.map(s => s.name.value)
-                ),
-                cyclePath
-              ));
-              continue;
-            }
-            if (spreadPath.some(spread => spread === spreadNode)) {
-              continue;
-            }
+    const spreadNodes = [];
+    gatherSpreads(spreadNodes, fragment.selectionSet);
+    if (spreadNodes.length === 0) {
+      return;
+    }
+
+    spreadPathIndexByName[fragmentName] = spreadPath.length;
 
-            spreadPath.push(spreadNode);
-            detectCycleRecursive(spreadNode.name.value);
-            spreadPath.pop();
+    for (let i = 0; i < spreadNodes.length; i++) {
+      const spreadNode = spreadNodes[i];
+      const spreadName = spreadNode.name.value;
+      const cycleIndex = spreadPathIndexByName[spreadName];
+
+      if (cycleIndex === undefined) {
+        spreadPath.push(spreadNode);
+        if (!visitedFrags[spreadName]) {
+          const spreadFragment = context.getFragment(spreadName);
+          if (spreadFragment) {
+            detectCycleRecursive(spreadFragment);
           }
         }
+        spreadPath.pop();
+      } else {
+        const cyclePath = spreadPath.slice(cycleIndex);
+        errors.push(new GraphQLError(
+          cycleErrorMessage(
+            spreadName,
+            cyclePath.map(s => s.name.value)
+          ),
+          cyclePath.concat(spreadNode)
+        ));
       }
+    }
 
-      detectCycleRecursive(initialName);
-
-      if (errors.length > 0) {
-        return errors;
-      }
-    },
-  };
+    spreadPathIndexByName[fragmentName] = undefined;
+  }
 }
 
 /**
  * Given an operation or fragment AST node, gather all the
  * named spreads defined within the scope of the fragment
  * or operation
  */
-function gatherSpreads(node: SelectionSet): Array<FragmentSpread> {
-  var spreadNodes = [];
-  visit(node, {
-    FragmentSpread(spread) {
-      spreadNodes.push(spread);
+function gatherSpreads(spreads: Array<FragmentSpread>, node: SelectionSet) {
+  for (let i = 0; i < node.selections.length; i++) {
+    const selection = node.selections[i];
+    if (selection.kind === FRAGMENT_SPREAD) {
+      spreads.push(selection);
+    } else if (selection.selectionSet) {
+      gatherSpreads(spreads, selection.selectionSet);
     }
-  });
-  return spreadNodes;
+  }
 }