diff --git a/src/validation/__tests__/NoUndefinedVariables.js b/src/validation/__tests__/NoUndefinedVariables.js index 1c80ebe8e4..9ebf5a0594 100644 --- a/src/validation/__tests__/NoUndefinedVariables.js +++ b/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), ]); }); diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js index 35f57d9509..73c2ccb721 100644 --- a/src/validation/rules/NoUndefinedVariables.js +++ b/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; } }; } diff --git a/src/validation/rules/NoUnusedVariables.js b/src/validation/rules/NoUnusedVariables.js index e02c8ff0c3..4169ea0aeb 100644 --- a/src/validation/rules/NoUnusedVariables.js +++ b/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; } }; } diff --git a/src/validation/rules/VariablesInAllowedPosition.js b/src/validation/rules/VariablesInAllowedPosition.js index 0a84536ba6..315a8c47b7 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/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 ] - ); - } } }; } diff --git a/src/validation/validate.js b/src/validation/validate.js index b180235be4..132cda1dfd 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -15,6 +15,7 @@ import * as Kind from '../language/kinds'; import type { Document, OperationDefinition, + Variable, SelectionSet, FragmentSpread, FragmentDefinition, @@ -175,6 +176,7 @@ function append(arr, items) { } type HasSelectionSet = OperationDefinition | FragmentDefinition; +type VariableUsage = { node: Variable, type: ?GraphQLInputType }; /** * An instance of this class is passed as the "this" context to all validators, @@ -189,6 +191,8 @@ export class ValidationContext { _fragmentSpreads: Map>; _recursivelyReferencedFragments: Map>; + _variableUsages: Map>; + _recursiveVariableUsages: Map>; constructor(schema: GraphQLSchema, ast: Document, typeInfo: TypeInfo) { this._schema = schema; @@ -196,6 +200,8 @@ export class ValidationContext { this._typeInfo = typeInfo; this._fragmentSpreads = new Map(); this._recursivelyReferencedFragments = new Map(); + this._variableUsages = new Map(); + this._recursiveVariableUsages = new Map(); } getSchema(): GraphQLSchema { @@ -258,6 +264,47 @@ export class ValidationContext { return fragments; } + getVariableUsages(node: HasSelectionSet): Array { + let usages = this._variableUsages.get(node); + if (!usages) { + usages = []; + const typeInfo = new TypeInfo(this._schema); + visit(node, { + enter(subnode) { + typeInfo.enter(subnode); + if (subnode.kind === Kind.VARIABLE_DEFINITION) { + return false; + } else if (subnode.kind === Kind.VARIABLE) { + usages.push({ node: subnode, type: typeInfo.getInputType() }); + } + }, + leave(subnode) { + typeInfo.leave(subnode); + } + }); + this._variableUsages.set(node, usages); + } + return usages; + } + + getRecursiveVariableUsages( + operation: OperationDefinition + ): Array { + let usages = this._recursiveVariableUsages.get(operation); + if (!usages) { + usages = this.getVariableUsages(operation); + const fragments = this.getRecursivelyReferencedFragments(operation); + for (let i = 0; i < fragments.length; i++) { + Array.prototype.push.apply( + usages, + this.getVariableUsages(fragments[i]) + ); + } + this._recursiveVariableUsages.set(operation, usages); + } + return usages; + } + getType(): ?GraphQLOutputType { return this._typeInfo.getType(); }