Skip to content

Commit

Permalink
[Validation] Memoize collecting variable usage.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@leebyron
leebyron committed Nov 17, 2015
1 parent 88acc01 commit 2afbff7
Show file tree
Hide file tree
Showing 5 changed files with 140 additions and 121 deletions.
52 changes: 22 additions & 30 deletions src/validation/__tests__/NoUndefinedVariables.js
View file
Expand Up @@ -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 } ],
};
}
Expand Down Expand Up @@ -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)
]);
});

Expand All @@ -149,7 +141,7 @@ describe('Validate: No undefined variables', () => {
field(a: $a)
}
`, [
undefVar('a', 3, 18)
undefVar('a', 3, 18, '', 2, 7)
]);
});

Expand All @@ -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)
]);
});

Expand All @@ -173,7 +165,7 @@ describe('Validate: No undefined variables', () => {
field(a: $a)
}
`, [
undefVar('a', 6, 18)
undefVar('a', 6, 18, '', 2, 7)
]);
});

Expand All @@ -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)
]);
});

Expand All @@ -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)
]);
});

Expand All @@ -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)
]);
});

Expand All @@ -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)
]);
});

Expand All @@ -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)
]);
});

Expand All @@ -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),
]);
});

Expand Down
75 changes: 33 additions & 42 deletions src/validation/rules/NoUndefinedVariables.js
View file
Expand Up @@ -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.`;
}

/**
Expand All @@ -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;
}
};
}
34 changes: 12 additions & 22 deletions src/validation/rules/NoUnusedVariables.js
View file
Expand Up @@ -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';


Expand All @@ -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(
Expand All @@ -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;
}
};
}
53 changes: 26 additions & 27 deletions src/validation/rules/VariablesInAllowedPosition.js
View file
Expand Up @@ -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 ]
);
}
}
};
}
Expand Down

0 comments on commit 2afbff7

Please sign in to comment.