[Validation] Memoize collecting variable usage.

During multiple validation passes we need to know about variable usage within a de-fragmented operation. Memoizing this ensures each pass is O(N) - each fragment is no longer visited per operation, but once total.

In doing so, `visitSpreadFragments` is no longer used, which will be cleaned up in a later PR

Author: leebyron

src/validation/__tests__/NoUndefinedVariables.js

@@ -12,20 +12,12 @@ import { expectPassesRule, expectFailsRule } from './harness';
 import {
   NoUndefinedVariables,
   undefinedVarMessage,
-  undefinedVarByOpMessage,
 } from '../rules/NoUndefinedVariables';
 
 
-function undefVar(varName, line, column) {
+function undefVar(varName, l1, c1, opName, l2, c2) {
   return {
-    message: undefinedVarMessage(varName),
-    locations: [ { line, column } ],
-  };
-}
-
-function undefVarByOp(varName, l1, c1, opName, l2, c2) {
-  return {
-    message: undefinedVarByOpMessage(varName, opName),
+    message: undefinedVarMessage(varName, opName),
     locations: [ { line: l1, column: c1 }, { line: l2, column: c2 } ],
   };
 }
@@ -139,7 +131,7 @@ describe('Validate: No undefined variables', () => {
         field(a: $a, b: $b, c: $c, d: $d)
       }
     `, [
-      undefVar('d', 3, 39)
+      undefVar('d', 3, 39, 'Foo', 2, 7)
     ]);
   });
 
@@ -149,7 +141,7 @@ describe('Validate: No undefined variables', () => {
         field(a: $a)
       }
     `, [
-      undefVar('a', 3, 18)
+      undefVar('a', 3, 18, '', 2, 7)
     ]);
   });
 
@@ -159,8 +151,8 @@ describe('Validate: No undefined variables', () => {
         field(a: $a, b: $b, c: $c)
       }
     `, [
-      undefVar('a', 3, 18),
-      undefVar('c', 3, 32)
+      undefVar('a', 3, 18, 'Foo', 2, 7),
+      undefVar('c', 3, 32, 'Foo', 2, 7)
     ]);
   });
 
@@ -173,7 +165,7 @@ describe('Validate: No undefined variables', () => {
         field(a: $a)
       }
     `, [
-      undefVar('a', 6, 18)
+      undefVar('a', 6, 18, '', 2, 7)
     ]);
   });
 
@@ -196,7 +188,7 @@ describe('Validate: No undefined variables', () => {
         field(c: $c)
       }
     `, [
-      undefVarByOp('c', 16, 18, 'Foo', 2, 7)
+      undefVar('c', 16, 18, 'Foo', 2, 7)
     ]);
   });
 
@@ -219,8 +211,8 @@ describe('Validate: No undefined variables', () => {
         field(c: $c)
       }
     `, [
-      undefVarByOp('a', 6, 18, 'Foo', 2, 7),
-      undefVarByOp('c', 16, 18, 'Foo', 2, 7)
+      undefVar('a', 6, 18, 'Foo', 2, 7),
+      undefVar('c', 16, 18, 'Foo', 2, 7)
     ]);
   });
 
@@ -236,8 +228,8 @@ describe('Validate: No undefined variables', () => {
         field(a: $a, b: $b)
       }
     `, [
-      undefVarByOp('b', 9, 25, 'Foo', 2, 7),
-      undefVarByOp('b', 9, 25, 'Bar', 5, 7)
+      undefVar('b', 9, 25, 'Foo', 2, 7),
+      undefVar('b', 9, 25, 'Bar', 5, 7)
     ]);
   });
 
@@ -253,8 +245,8 @@ describe('Validate: No undefined variables', () => {
         field(a: $a, b: $b)
       }
     `, [
-      undefVarByOp('a', 9, 18, 'Foo', 2, 7),
-      undefVarByOp('b', 9, 25, 'Bar', 5, 7)
+      undefVar('a', 9, 18, 'Foo', 2, 7),
+      undefVar('b', 9, 25, 'Bar', 5, 7)
     ]);
   });
 
@@ -273,8 +265,8 @@ describe('Validate: No undefined variables', () => {
         field(b: $b)
       }
     `, [
-      undefVarByOp('a', 9, 18, 'Foo', 2, 7),
-      undefVarByOp('b', 12, 18, 'Bar', 5, 7)
+      undefVar('a', 9, 18, 'Foo', 2, 7),
+      undefVar('b', 12, 18, 'Bar', 5, 7)
     ]);
   });
 
@@ -295,12 +287,12 @@ describe('Validate: No undefined variables', () => {
         field2(c: $c)
       }
     `, [
-      undefVarByOp('a', 9, 19, 'Foo', 2, 7),
-      undefVarByOp('c', 14, 19, 'Foo', 2, 7),
-      undefVarByOp('a', 11, 19, 'Foo', 2, 7),
-      undefVarByOp('b', 9, 26, 'Bar', 5, 7),
-      undefVarByOp('c', 14, 19, 'Bar', 5, 7),
-      undefVarByOp('b', 11, 26, 'Bar', 5, 7),
+      undefVar('a', 9, 19, 'Foo', 2, 7),
+      undefVar('a', 11, 19, 'Foo', 2, 7),
+      undefVar('c', 14, 19, 'Foo', 2, 7),
+      undefVar('b', 9, 26, 'Bar', 5, 7),
+      undefVar('b', 11, 26, 'Bar', 5, 7),
+      undefVar('c', 14, 19, 'Bar', 5, 7),
     ]);
   });
 

src/validation/rules/NoUndefinedVariables.js

@@ -8,16 +8,14 @@
  *  of patent rights can be found in the PATENTS file in the same directory.
  */
 
+import type { ValidationContext } from '../index';
 import { GraphQLError } from '../../error';
-import { FRAGMENT_DEFINITION } from '../../language/kinds';
 
 
-export function undefinedVarMessage(varName: any): string {
-  return `Variable "$${varName}" is not defined.`;
-}
-
-export function undefinedVarByOpMessage(varName: any, opName: any): string {
-  return `Variable "$${varName}" is not defined by operation "${opName}".`;
+export function undefinedVarMessage(varName: string, opName: ?string): string {
+  return opName ?
+    `Variable "$${varName}" is not defined by operation "${opName}".` :
+    `Variable "$${varName}" is not defined.`;
 }
 
 /**
@@ -26,47 +24,40 @@ export function undefinedVarByOpMessage(varName: any, opName: any): string {
  * A GraphQL operation is only valid if all variables encountered, both directly
  * and via fragment spreads, are defined by that operation.
  */
-export function NoUndefinedVariables(): any {
-  var operation;
-  var visitedFragmentNames = {};
-  var definedVariableNames = {};
+export function NoUndefinedVariables(context: ValidationContext): any {
+  let variableNameDefined = Object.create(null);
 
   return {
-    // Visit FragmentDefinition after visiting FragmentSpread
-    visitSpreadFragments: true,
+    OperationDefinition: {
+      enter() {
+        variableNameDefined = Object.create(null);
+      },
+      leave(operation) {
+        const usages = context.getRecursiveVariableUsages(operation);
+        const errors = [];
 
-    OperationDefinition(node) {
-      operation = node;
-      visitedFragmentNames = {};
-      definedVariableNames = {};
-    },
-    VariableDefinition(def) {
-      definedVariableNames[def.variable.name.value] = true;
-    },
-    Variable(variable, key, parent, path, ancestors) {
-      var varName = variable.name.value;
-      if (definedVariableNames[varName] !== true) {
-        var withinFragment = ancestors.some(
-          node => node.kind === FRAGMENT_DEFINITION
-        );
-        if (withinFragment && operation && operation.name) {
-          return new GraphQLError(
-            undefinedVarByOpMessage(varName, operation.name.value),
-            [ variable, operation ]
-          );
+        usages.forEach(({ node }) => {
+          var varName = node.name.value;
+          if (variableNameDefined[varName] !== true) {
+            errors.push(
+              new GraphQLError(
+                undefinedVarMessage(
+                  varName,
+                  operation.name && operation.name.value
+                ),
+                [ node, operation ]
+              )
+            );
+          }
+        });
+
+        if (errors.length > 0) {
+          return errors;
         }
-        return new GraphQLError(
-          undefinedVarMessage(varName),
-          [ variable ]
-        );
       }
     },
-    FragmentSpread(spreadAST) {
-      // Only visit fragments of a particular name once per operation
-      if (visitedFragmentNames[spreadAST.name.value] === true) {
-        return false;
-      }
-      visitedFragmentNames[spreadAST.name.value] = true;
+    VariableDefinition(varDefAST) {
+      variableNameDefined[varDefAST.variable.name.value] = true;
     }
   };
 }

src/validation/rules/NoUnusedVariables.js

@@ -8,6 +8,7 @@
  *  of patent rights can be found in the PATENTS file in the same directory.
  */
 
+import type { ValidationContext } from '../index';
 import { GraphQLError } from '../../error';
 
 
@@ -21,22 +22,23 @@ export function unusedVariableMessage(varName: any): string {
  * A GraphQL operation is only valid if all variables defined by an operation
  * are used, either directly or within a spread fragment.
  */
-export function NoUnusedVariables(): any {
-  var visitedFragmentNames = {};
-  var variableDefs = [];
-  var variableNameUsed = {};
+export function NoUnusedVariables(context: ValidationContext): any {
+  let variableDefs = [];
 
   return {
-    // Visit FragmentDefinition after visiting FragmentSpread
-    visitSpreadFragments: true,
-
     OperationDefinition: {
       enter() {
-        visitedFragmentNames = {};
         variableDefs = [];
-        variableNameUsed = {};
       },
-      leave() {
+      leave(operation) {
+
+        const variableNameUsed = Object.create(null);
+        const usages = context.getRecursiveVariableUsages(operation);
+
+        usages.forEach(({ node }) => {
+          variableNameUsed[node.name.value] = true;
+        });
+
         var errors = variableDefs
           .filter(def => variableNameUsed[def.variable.name.value] !== true)
           .map(def => new GraphQLError(
@@ -50,18 +52,6 @@ export function NoUnusedVariables(): any {
     },
     VariableDefinition(def) {
       variableDefs.push(def);
-      // Do not visit deeper, or else the defined variable name will be visited.
-      return false;
-    },
-    Variable(variable) {
-      variableNameUsed[variable.name.value] = true;
-    },
-    FragmentSpread(spreadAST) {
-      // Only visit fragments of a particular name once per operation
-      if (visitedFragmentNames[spreadAST.name.value] === true) {
-        return false;
-      }
-      visitedFragmentNames[spreadAST.name.value] = true;
     }
   };
 }

src/validation/rules/VariablesInAllowedPosition.js

@@ -27,39 +27,38 @@ export function badVarPosMessage(
  * Variables passed to field arguments conform to type
  */
 export function VariablesInAllowedPosition(context: ValidationContext): any {
-  var varDefMap = {};
-  var visitedFragmentNames = {};
+  let varDefMap = Object.create(null);
 
   return {
-    // Visit FragmentDefinition after visiting FragmentSpread
-    visitSpreadFragments: true,
+    OperationDefinition: {
+      enter() {
+        varDefMap = Object.create(null);
+      },
+      leave(operation) {
+        const usages = context.getRecursiveVariableUsages(operation);
+        const errors = [];
 
-    OperationDefinition() {
-      varDefMap = {};
-      visitedFragmentNames = {};
+        usages.forEach(({ node, type }) => {
+          const varName = node.name.value;
+          const varDef = varDefMap[varName];
+          const varType =
+            varDef && typeFromAST(context.getSchema(), varDef.type);
+          if (varType && type &&
+              !varTypeAllowedForType(effectiveType(varType, varDef), type)) {
+            errors.push(new GraphQLError(
+              badVarPosMessage(varName, varType, type),
+              [ node ]
+            ));
+          }
+        });
+
+        if (errors.length > 0) {
+          return errors;
+        }
+      }
     },
     VariableDefinition(varDefAST) {
       varDefMap[varDefAST.variable.name.value] = varDefAST;
-    },
-    FragmentSpread(spreadAST) {
-      // Only visit fragments of a particular name once per operation
-      if (visitedFragmentNames[spreadAST.name.value]) {
-        return false;
-      }
-      visitedFragmentNames[spreadAST.name.value] = true;
-    },
-    Variable(variableAST) {
-      var varName = variableAST.name.value;
-      var varDef = varDefMap[varName];
-      var varType = varDef && typeFromAST(context.getSchema(), varDef.type);
-      var inputType = context.getInputType();
-      if (varType && inputType &&
-          !varTypeAllowedForType(effectiveType(varType, varDef), inputType)) {
-        return new GraphQLError(
-          badVarPosMessage(varName, varType, inputType),
-          [ variableAST ]
-        );
-      }
     }
   };
 }