diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js index 1d6ef75741..6df71fd0ed 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -13,6 +13,7 @@ import { visit, visitWithTypeInfo } from '../language/visitor'; import { Kind } from '../language/kinds'; import type { DocumentNode, + ExecutableDefinitionNode, OperationDefinitionNode, VariableNode, SelectionSetNode, @@ -37,6 +38,22 @@ type VariableUsage = {| +defaultValue: ?mixed, |}; +export type ValidationOptions = { + /** + * EXPERIMENTAL: + * + * If enabled, the validator will treat fragments with variable definitions + * as "independent" definitions, in the same way operations are "independent". + * + * This changes the behavior of "getRecursivelyReferencedFragments": + * it will not include fragments that have variable definitions in the result. + * + * Likewise, "getRecursiveVariableUsages" will not include variables used + * only by recursively referenced fragments with variable definitions. + */ + experimentalFragmentVariables?: boolean, +}; + /** * An instance of this class is passed as the "this" context to all validators, * allowing access to commonly useful contextual information from within a @@ -50,19 +67,21 @@ export default class ValidationContext { _fragments: ObjMap; _fragmentSpreads: Map>; _recursivelyReferencedFragments: Map< - OperationDefinitionNode, + ExecutableDefinitionNode, $ReadOnlyArray, >; _variableUsages: Map>; _recursiveVariableUsages: Map< - OperationDefinitionNode, + ExecutableDefinitionNode, $ReadOnlyArray, >; + _options: ValidationOptions; constructor( schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo, + options: ValidationOptions = {}, ): void { this._schema = schema; this._ast = ast; @@ -72,6 +91,7 @@ export default class ValidationContext { this._recursivelyReferencedFragments = new Map(); this._variableUsages = new Map(); this._recursiveVariableUsages = new Map(); + this._options = options; } reportError(error: GraphQLError): void { @@ -129,14 +149,21 @@ export default class ValidationContext { return spreads; } + /* + * Finds all fragments referenced via the definition, recursively. + * + * If the experimentalFragmentVariables option is used, this will not visit + * fragment definitions that include variable definitions. + */ getRecursivelyReferencedFragments( - operation: OperationDefinitionNode, + definition: ExecutableDefinitionNode, ): $ReadOnlyArray { - let fragments = this._recursivelyReferencedFragments.get(operation); + let fragments = this._recursivelyReferencedFragments.get(definition); + if (!fragments) { fragments = []; const collectedNames = Object.create(null); - const nodesToVisit: Array = [operation.selectionSet]; + const nodesToVisit: Array = [definition.selectionSet]; while (nodesToVisit.length !== 0) { const node = nodesToVisit.pop(); const spreads = this.getFragmentSpreads(node); @@ -145,14 +172,26 @@ export default class ValidationContext { if (collectedNames[fragName] !== true) { collectedNames[fragName] = true; const fragment = this.getFragment(fragName); - if (fragment) { - fragments.push(fragment); - nodesToVisit.push(fragment.selectionSet); + + if (!fragment) { + continue; } + + if ( + this._options.experimentalFragmentVariables && + fragment.variableDefinitions && + fragment.variableDefinitions.length > 0 + ) { + continue; + } + + fragments.push(fragment); + nodesToVisit.push(fragment.selectionSet); } } } - this._recursivelyReferencedFragments.set(operation, fragments); + + this._recursivelyReferencedFragments.set(definition, fragments); } return fragments; } @@ -181,20 +220,27 @@ export default class ValidationContext { return usages; } + /* + * Finds all variables used by the definition, recursively. + * + * NOTE: if experimentalFragmentVariables are being used, it excludes all + * fragments with their own variable definitions: these are considered their + * own independent executable definition for the purposes of variable usage. + */ getRecursiveVariableUsages( - operation: OperationDefinitionNode, + definition: ExecutableDefinitionNode, ): $ReadOnlyArray { - let usages = this._recursiveVariableUsages.get(operation); + let usages = this._recursiveVariableUsages.get(definition); if (!usages) { - usages = this.getVariableUsages(operation); - const fragments = this.getRecursivelyReferencedFragments(operation); + usages = this.getVariableUsages(definition); + const fragments = this.getRecursivelyReferencedFragments(definition); for (let i = 0; i < fragments.length; i++) { Array.prototype.push.apply( usages, this.getVariableUsages(fragments[i]), ); } - this._recursiveVariableUsages.set(operation, usages); + this._recursiveVariableUsages.set(definition, usages); } return usages; } diff --git a/src/validation/__tests__/NoUndefinedVariables-test.js b/src/validation/__tests__/NoUndefinedVariables-test.js index ff7e096ca1..a3e707ff77 100644 --- a/src/validation/__tests__/NoUndefinedVariables-test.js +++ b/src/validation/__tests__/NoUndefinedVariables-test.js @@ -6,7 +6,12 @@ */ import { describe, it } from 'mocha'; -import { expectPassesRule, expectFailsRule } from './harness'; +import { + expectPassesRule, + expectFailsRule, + expectPassesRuleWithFragmentVariables, + expectFailsRuleWithFragmentVariables, +} from './harness'; import { NoUndefinedVariables, undefinedVarMessage, @@ -329,4 +334,42 @@ describe('Validate: No undefined variables', () => { ], ); }); + + // Experimental Fragment Variables + it('uses all variables in fragments with variable definitions', () => { + expectPassesRuleWithFragmentVariables( + NoUndefinedVariables, + ` + fragment Foo($a: String, $b: String, $c: String) on Type { + ...FragA + } + fragment FragA on Type { + field(a: $a) { + ...FragB + } + } + fragment FragB on Type { + field(b: $b) { + ...FragC + } + } + fragment FragC on Type { + field(c: $c) + } + `, + ); + }); + + it('variable used in fragment not defined', () => { + expectFailsRuleWithFragmentVariables( + NoUndefinedVariables, + ` + fragment FragA($a: String) on Type { + field(a: $a) + field(b: $b) + } + `, + [undefVar('b', 4, 18, 'FragA', 2, 7)], + ); + }); }); diff --git a/src/validation/__tests__/NoUnusedVariables-test.js b/src/validation/__tests__/NoUnusedVariables-test.js index fabe5844de..b9762f43d6 100644 --- a/src/validation/__tests__/NoUnusedVariables-test.js +++ b/src/validation/__tests__/NoUnusedVariables-test.js @@ -6,7 +6,12 @@ */ import { describe, it } from 'mocha'; -import { expectPassesRule, expectFailsRule } from './harness'; +import { + expectPassesRule, + expectFailsRule, + expectPassesRuleWithFragmentVariables, + expectFailsRuleWithFragmentVariables, +} from './harness'; import { NoUnusedVariables, unusedVariableMessage, @@ -237,4 +242,72 @@ describe('Validate: No unused variables', () => { [unusedVar('b', 'Foo', 2, 17), unusedVar('a', 'Bar', 5, 17)], ); }); + + // Experimental Fragment Variables + it('uses all variables in fragments with variable definitions', () => { + expectPassesRuleWithFragmentVariables( + NoUnusedVariables, + ` + fragment Foo($a: String, $b: String, $c: String) on Type { + ...FragA + } + fragment FragA on Type { + field(a: $a) { + ...FragB + } + } + fragment FragB on Type { + field(b: $b) { + ...FragC + } + } + fragment FragC on Type { + field(c: $c) + } + `, + ); + }); + + it('variable not used by fragment', () => { + expectFailsRuleWithFragmentVariables( + NoUnusedVariables, + ` + fragment FragA($a: String) on Type { + field + } + `, + [unusedVar('a', 'FragA', 2, 22)], + ); + }); + + it('variable used in query defined by fragment', () => { + expectFailsRuleWithFragmentVariables( + NoUnusedVariables, + ` + query Foo($a: String) { + field(a: $a) + ...FragA + } + fragment FragA($a: String) on Type { + field + } + `, + [unusedVar('a', 'FragA', 6, 22)], + ); + + it('variable defined in fragment used by query', () => { + expectFailsRuleWithFragmentVariables( + NoUnusedVariables, + ` + query Foo($a: String) { + ...FragA + } + fragment FragA($a: String) on Type { + field(a: $a) + } + `, + [unusedVar('a', 'Foo', 1, 19)], + ); + }); + }); }); diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js index 6d428faa79..ba3664e60f 100644 --- a/src/validation/__tests__/harness.js +++ b/src/validation/__tests__/harness.js @@ -421,13 +421,31 @@ export const testSchema = new GraphQLSchema({ ], }); -function expectValid(schema, rules, queryString) { - const errors = validate(schema, parse(queryString), rules); +function expectValid(schema, rules, queryString, options = {}) { + const errors = validate( + schema, + parse(queryString, options), + rules, + null, // default TypeInfo + options, + ); expect(errors).to.deep.equal([], 'Should validate'); } -function expectInvalid(schema, rules, queryString, expectedErrors) { - const errors = validate(schema, parse(queryString), rules); +function expectInvalid( + schema, + rules, + queryString, + expectedErrors, + options = {}, +) { + const errors = validate( + schema, + parse(queryString, options), + rules, + null, // default TypeInfo + options, + ); expect(errors).to.have.length.of.at.least(1, 'Should not validate'); expect(errors).to.deep.equal(expectedErrors); return errors; @@ -448,3 +466,19 @@ export function expectPassesRuleWithSchema(schema, rule, queryString, errors) { export function expectFailsRuleWithSchema(schema, rule, queryString, errors) { return expectInvalid(schema, [rule], queryString, errors); } + +export function expectPassesRuleWithFragmentVariables(rule, queryString) { + return expectValid(testSchema, [rule], queryString, { + experimentalFragmentVariables: true, + }); +} + +export function expectFailsRuleWithFragmentVariables( + rule, + queryString, + errors, +) { + return expectInvalid(testSchema, [rule], queryString, errors, { + experimentalFragmentVariables: true, + }); +} diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js index 142142f437..18ca4b6589 100644 --- a/src/validation/rules/NoUndefinedVariables.js +++ b/src/validation/rules/NoUndefinedVariables.js @@ -10,6 +10,8 @@ import type ValidationContext from '../ValidationContext'; import { GraphQLError } from '../../error'; import type { ASTVisitor } from '../../language/visitor'; +import { Kind } from '../../language'; +import type { ExecutableDefinitionNode } from '../../language'; export function undefinedVarMessage(varName: string, opName: ?string): string { return opName @@ -20,36 +22,49 @@ export function undefinedVarMessage(varName: string, opName: ?string): string { /** * No undefined variables * - * A GraphQL operation is only valid if all variables encountered, both directly - * and via fragment spreads, are defined by that operation. + * A GraphQL definition is only valid if all variables encountered, both + * directly and via fragment spreads, are defined by that definition. + * + * NOTE: if experimentalFragmentVariables are used, then fragments with + * variable definitions will validate independently, and a variable is not + * "encountered" if it's only used in a child with variable definitions. */ export function NoUndefinedVariables(context: ValidationContext): ASTVisitor { let variableNameDefined = Object.create(null); - return { - OperationDefinition: { - enter() { - variableNameDefined = Object.create(null); - }, - leave(operation) { - const usages = context.getRecursiveVariableUsages(operation); + const executableDefinitionVisitor = { + enter() { + variableNameDefined = Object.create(null); + }, + leave(definition: ExecutableDefinitionNode) { + if ( + definition.kind === Kind.FRAGMENT_DEFINITION && + Object.keys(variableNameDefined).length === 0 + ) { + return; + } + const usages = context.getRecursiveVariableUsages(definition); - for (const { node } of usages) { - const varName = node.name.value; - if (variableNameDefined[varName] !== true) { - context.reportError( - new GraphQLError( - undefinedVarMessage( - varName, - operation.name && operation.name.value, - ), - [node, operation], + for (const { node } of usages) { + const varName = node.name.value; + if (variableNameDefined[varName] !== true) { + context.reportError( + new GraphQLError( + undefinedVarMessage( + varName, + definition.name && definition.name.value, ), - ); - } + [node, definition], + ), + ); } - }, + } }, + }; + + return { + OperationDefinition: executableDefinitionVisitor, + FragmentDefinition: executableDefinitionVisitor, VariableDefinition(node) { variableNameDefined[node.variable.name.value] = true; }, diff --git a/src/validation/rules/NoUnusedVariables.js b/src/validation/rules/NoUnusedVariables.js index a964837cc0..36b3b15c0c 100644 --- a/src/validation/rules/NoUnusedVariables.js +++ b/src/validation/rules/NoUnusedVariables.js @@ -9,6 +9,8 @@ import type ValidationContext from '../ValidationContext'; import { GraphQLError } from '../../error'; +import { Kind } from '../../language'; +import type { ExecutableDefinitionNode } from '../../language'; import type { ASTVisitor } from '../../language/visitor'; export function unusedVariableMessage( @@ -23,38 +25,53 @@ export function unusedVariableMessage( /** * No unused variables * - * A GraphQL operation is only valid if all variables defined by an operation - * are used, either directly or within a spread fragment. + * A GraphQL definition is only valid if all variables defined by that + * definition are used, either directly or within a spread fragment. + * + * NOTE: if experimentalFragmentVariables are used, then fragments with + * variable definitions will validate independently. + * So `query Foo` must not define `$a` when `$a` is only used inside + * `fragment FragA($a: Type)` */ export function NoUnusedVariables(context: ValidationContext): ASTVisitor { let variableDefs = []; - return { - OperationDefinition: { - enter() { - variableDefs = []; - }, - leave(operation) { - const variableNameUsed = Object.create(null); - const usages = context.getRecursiveVariableUsages(operation); - const opName = operation.name ? operation.name.value : null; - - for (const { node } of usages) { - variableNameUsed[node.name.value] = true; - } + const executableDefinitionVisitor = { + enter() { + variableDefs = []; + }, + leave(definition: ExecutableDefinitionNode) { + if ( + definition.kind === Kind.FRAGMENT_DEFINITION && + variableDefs.length === 0 + ) { + return; + } + + const variableNameUsed = Object.create(null); + const usages = context.getRecursiveVariableUsages(definition); + const opName = definition.name ? definition.name.value : null; - for (const variableDef of variableDefs) { - const variableName = variableDef.variable.name.value; - if (variableNameUsed[variableName] !== true) { - context.reportError( - new GraphQLError(unusedVariableMessage(variableName, opName), [ - variableDef, - ]), - ); - } + for (const { node } of usages) { + variableNameUsed[node.name.value] = true; + } + + for (const variableDef of variableDefs) { + const variableName = variableDef.variable.name.value; + if (variableNameUsed[variableName] !== true) { + context.reportError( + new GraphQLError(unusedVariableMessage(variableName, opName), [ + variableDef, + ]), + ); } - }, + } }, + }; + + return { + OperationDefinition: executableDefinitionVisitor, + FragmentDefinition: executableDefinitionVisitor, VariableDefinition(def) { variableDefs.push(def); }, diff --git a/src/validation/validate.js b/src/validation/validate.js index 61213f8b86..0b8c383a85 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -17,6 +17,7 @@ import { assertValidSchema } from '../type/validate'; import { TypeInfo } from '../utilities/TypeInfo'; import { specifiedRules } from './specifiedRules'; import ValidationContext from './ValidationContext'; +import type { ValidationOptions } from './ValidationContext'; /** * Implements the "Validation" section of the spec. @@ -39,6 +40,7 @@ export function validate( ast: DocumentNode, rules?: $ReadOnlyArray, typeInfo?: TypeInfo, + options?: ValidationOptions, ): $ReadOnlyArray { invariant(ast, 'Must provide document'); // If the schema used for validation is invalid, throw an error. @@ -48,6 +50,7 @@ export function validate( typeInfo || new TypeInfo(schema), ast, rules || specifiedRules, + options || {}, ); } @@ -62,8 +65,9 @@ function visitUsingRules( typeInfo: TypeInfo, documentAST: DocumentNode, rules: $ReadOnlyArray<(ValidationContext) => ASTVisitor>, + options: ValidationOptions, ): $ReadOnlyArray { - const context = new ValidationContext(schema, documentAST, typeInfo); + const context = new ValidationContext(schema, documentAST, typeInfo, options); const visitors = rules.map(rule => rule(context)); // Visit the whole document with each instance of all provided rules. visit(documentAST, visitWithTypeInfo(typeInfo, visitInParallel(visitors)));