From 0d9951e95275a49cd73ed3d901eb2e955b0c03e8 Mon Sep 17 00:00:00 2001
From: mmahoney <mmahoney@fb.com>
Date: Mon, 16 Jul 2018 17:49:02 -0400
Subject: [PATCH 1/6] Fragment variables: enforce fragment variable validation
Treats fragments with variable definitions as their own top-level
"executable definition": this means a query cannot include a variable
definition found solely beneath a fragment-variable definition, and
a fragment must include all variable definitions that exist under it,
excluding other variables defined by other fragments.
---
src/validation/ValidationContext.js | 53 +++++++++++----
.../__tests__/NoUndefinedVariables-test.js | 38 +++++++++++
.../__tests__/NoUnusedVariables-test.js | 68 +++++++++++++++++++
src/validation/__tests__/harness.js | 12 +++-
src/validation/rules/NoUndefinedVariables.js | 52 ++++++++------
src/validation/rules/NoUnusedVariables.js | 67 +++++++++++-------
6 files changed, 229 insertions(+), 61 deletions(-)
diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js
index 1d6ef75741..7cf1718660 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,
@@ -50,12 +51,12 @@ export default class ValidationContext {
_fragments: ObjMap<FragmentDefinitionNode>;
_fragmentSpreads: Map<SelectionSetNode, $ReadOnlyArray<FragmentSpreadNode>>;
_recursivelyReferencedFragments: Map<
- OperationDefinitionNode,
+ ExecutableDefinitionNode,
$ReadOnlyArray<FragmentDefinitionNode>,
>;
_variableUsages: Map<NodeWithSelectionSet, $ReadOnlyArray<VariableUsage>>;
_recursiveVariableUsages: Map<
- OperationDefinitionNode,
+ ExecutableDefinitionNode,
$ReadOnlyArray<VariableUsage>,
>;
@@ -129,14 +130,21 @@ export default class ValidationContext {
return spreads;
}
+ /*
+ * Finds all fragments referenced via the definition, recursively.
+ *
+ * NOTE: if experimentalFragmentVariables are being used, it excludes all
+ * fragments with their own variable definitions: these are considered their
+ * own "root" executable definition.
+ */
getRecursivelyReferencedFragments(
- operation: OperationDefinitionNode,
+ definition: ExecutableDefinitionNode,
): $ReadOnlyArray<FragmentDefinitionNode> {
- let fragments = this._recursivelyReferencedFragments.get(operation);
+ let fragments = this._recursivelyReferencedFragments.get(definition);
if (!fragments) {
fragments = [];
const collectedNames = Object.create(null);
- const nodesToVisit: Array<SelectionSetNode> = [operation.selectionSet];
+ const nodesToVisit: Array<SelectionSetNode> = [definition.selectionSet];
while (nodesToVisit.length !== 0) {
const node = nodesToVisit.pop();
const spreads = this.getFragmentSpreads(node);
@@ -145,14 +153,17 @@ export default class ValidationContext {
if (collectedNames[fragName] !== true) {
collectedNames[fragName] = true;
const fragment = this.getFragment(fragName);
- if (fragment) {
+ if (
+ fragment &&
+ !this.isExecutableDefinitionWithVariables(fragment)
+ ) {
fragments.push(fragment);
nodesToVisit.push(fragment.selectionSet);
}
}
}
}
- this._recursivelyReferencedFragments.set(operation, fragments);
+ this._recursivelyReferencedFragments.set(definition, fragments);
}
return fragments;
}
@@ -181,20 +192,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 "root" executable definition.
+ */
getRecursiveVariableUsages(
- operation: OperationDefinitionNode,
+ definition: ExecutableDefinitionNode,
): $ReadOnlyArray<VariableUsage> {
- 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;
}
@@ -226,4 +244,15 @@ export default class ValidationContext {
getArgument(): ?GraphQLArgument {
return this._typeInfo.getArgument();
}
+
+ // All OperationDefinitions, or FragmentDefinitions with variable definitions
+ isExecutableDefinitionWithVariables(
+ definition: ExecutableDefinitionNode,
+ ): boolean {
+ return (
+ definition.kind === Kind.OPERATION_DEFINITION ||
+ (definition.variableDefinitions != null &&
+ definition.variableDefinitions.length > 0)
+ );
+ }
}
diff --git a/src/validation/__tests__/NoUndefinedVariables-test.js b/src/validation/__tests__/NoUndefinedVariables-test.js
index ff7e096ca1..67156e4e1d 100644
--- a/src/validation/__tests__/NoUndefinedVariables-test.js
+++ b/src/validation/__tests__/NoUndefinedVariables-test.js
@@ -329,4 +329,42 @@ describe('Validate: No undefined variables', () => {
],
);
});
+
+ // Experimental Fragment Variables
+ it('uses all variables in fragments with variable definitions', () => {
+ expectPassesRule(
+ 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', () => {
+ expectFailsRule(
+ 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..bbdff51ffc 100644
--- a/src/validation/__tests__/NoUnusedVariables-test.js
+++ b/src/validation/__tests__/NoUnusedVariables-test.js
@@ -237,4 +237,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', () => {
+ expectPassesRule(
+ 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', () => {
+ expectFailsRule(
+ NoUnusedVariables,
+ `
+ fragment FragA($a: String) on Type {
+ field
+ }
+ `,
+ [unusedVar('a', 'FragA', 2, 22)],
+ );
+ });
+
+ it('variable used in query defined by fragment', () => {
+ expectFailsRule(
+ 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', () => {
+ expectFailsRule(
+ 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..ca92c211c0 100644
--- a/src/validation/__tests__/harness.js
+++ b/src/validation/__tests__/harness.js
@@ -422,12 +422,20 @@ export const testSchema = new GraphQLSchema({
});
function expectValid(schema, rules, queryString) {
- const errors = validate(schema, parse(queryString), rules);
+ const errors = validate(
+ schema,
+ parse(queryString, { experimentalFragmentVariables: true }),
+ rules,
+ );
expect(errors).to.deep.equal([], 'Should validate');
}
function expectInvalid(schema, rules, queryString, expectedErrors) {
- const errors = validate(schema, parse(queryString), rules);
+ const errors = validate(
+ schema,
+ parse(queryString, { experimentalFragmentVariables: true }),
+ rules,
+ );
expect(errors).to.have.length.of.at.least(1, 'Should not validate');
expect(errors).to.deep.equal(expectedErrors);
return errors;
diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js
index d74a49265b..858d3d5ddd 100644
--- a/src/validation/rules/NoUndefinedVariables.js
+++ b/src/validation/rules/NoUndefinedVariables.js
@@ -10,6 +10,7 @@
import type ValidationContext from '../ValidationContext';
import { GraphQLError } from '../../error';
import type { ASTVisitor } from '../../language/visitor';
+import type { ExecutableDefinitionNode } from '../../language';
export function undefinedVarMessage(varName: string, opName: ?string): string {
return opName
@@ -26,30 +27,39 @@ export function undefinedVarMessage(varName: string, opName: ?string): string {
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(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {
+ return;
+ }
+ variableNameDefined = Object.create(null);
+ },
+ leave(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {
+ return;
+ }
+ const usages = context.getRecursiveVariableUsages(definition);
- usages.forEach(({ node }) => {
- const varName = node.name.value;
- if (variableNameDefined[varName] !== true) {
- context.reportError(
- new GraphQLError(
- undefinedVarMessage(
- varName,
- operation.name && operation.name.value,
- ),
- [node, operation],
+ usages.forEach(({ node }) => {
+ 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 c5637e22db..a0f7ee9940 100644
--- a/src/validation/rules/NoUnusedVariables.js
+++ b/src/validation/rules/NoUnusedVariables.js
@@ -9,6 +9,7 @@
import type ValidationContext from '../ValidationContext';
import { GraphQLError } from '../../error';
+import type { ExecutableDefinitionNode } from '../../language';
import type { ASTVisitor } from '../../language/visitor';
export function unusedVariableMessage(
@@ -23,38 +24,52 @@ export function unusedVariableMessage(
/**
* No unused variables
*
- * A GraphQL operation is only valid if all variables defined by an operation
+ * A GraphQL executable tree is only valid if all variables defined by that tree
* are used, either directly or within a spread fragment.
+ *
+ * NOTE: if experimentalFragmentVariables are used, then fragments with
+ * variables defined are considered an independent "executable tree":
+ * fragments defined under them do not count as "within a fragment spread".
*/
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;
-
- usages.forEach(({ node }) => {
- variableNameUsed[node.name.value] = true;
- });
-
- variableDefs.forEach(variableDef => {
- const variableName = variableDef.variable.name.value;
- if (variableNameUsed[variableName] !== true) {
- context.reportError(
- new GraphQLError(unusedVariableMessage(variableName, opName), [
- variableDef,
- ]),
- );
- }
- });
- },
+ const executableDefinitionVisitor = {
+ enter(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {
+ return;
+ }
+ variableDefs = [];
},
+ leave(definition: ExecutableDefinitionNode) {
+ if (!context.isExecutableDefinitionWithVariables(definition)) {
+ return;
+ }
+
+ const variableNameUsed = Object.create(null);
+ const usages = context.getRecursiveVariableUsages(definition);
+ const opName = definition.name ? definition.name.value : null;
+
+ usages.forEach(({ node }) => {
+ variableNameUsed[node.name.value] = true;
+ });
+
+ variableDefs.forEach(variableDef => {
+ 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);
},
From cbd3abf87effbf5daa187be3c56c0baae9914d0a Mon Sep 17 00:00:00 2001
From: mmahoney <mmahoney@fb.com>
Date: Mon, 16 Jul 2018 19:53:20 -0400
Subject: [PATCH 2/6] Modify comments to be a bit more accurate
---
src/validation/ValidationContext.js | 2 +-
src/validation/rules/NoUndefinedVariables.js | 8 ++++++--
src/validation/rules/NoUnusedVariables.js | 8 ++++----
3 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js
index 7cf1718660..495e24fe9e 100644
--- a/src/validation/ValidationContext.js
+++ b/src/validation/ValidationContext.js
@@ -197,7 +197,7 @@ export default class ValidationContext {
*
* NOTE: if experimentalFragmentVariables are being used, it excludes all
* fragments with their own variable definitions: these are considered their
- * own "root" executable definition.
+ * own independent executable definition for the purposes of variable usage.
*/
getRecursiveVariableUsages(
definition: ExecutableDefinitionNode,
diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js
index 858d3d5ddd..4554e200c1 100644
--- a/src/validation/rules/NoUndefinedVariables.js
+++ b/src/validation/rules/NoUndefinedVariables.js
@@ -21,8 +21,12 @@ 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
+ * variables defined are considered an independent "executable definitions":
+ * variables defined under them do not count as "within a fragment spread".
*/
export function NoUndefinedVariables(context: ValidationContext): ASTVisitor {
let variableNameDefined = Object.create(null);
diff --git a/src/validation/rules/NoUnusedVariables.js b/src/validation/rules/NoUnusedVariables.js
index a0f7ee9940..cf291babf6 100644
--- a/src/validation/rules/NoUnusedVariables.js
+++ b/src/validation/rules/NoUnusedVariables.js
@@ -24,12 +24,12 @@ export function unusedVariableMessage(
/**
* No unused variables
*
- * A GraphQL executable tree is only valid if all variables defined by that tree
- * 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
- * variables defined are considered an independent "executable tree":
- * fragments defined under them do not count as "within a fragment spread".
+ * variables defined are considered an independent "executable definitions":
+ * variables defined under them do not count as "within a fragment spread".
*/
export function NoUnusedVariables(context: ValidationContext): ASTVisitor {
let variableDefs = [];
From e9a7991b20e9a144801931c481aa8cc24a304628 Mon Sep 17 00:00:00 2001
From: mmahoney <mmahoney@fb.com>
Date: Mon, 16 Jul 2018 20:23:30 -0400
Subject: [PATCH 3/6] More comment cleanup
---
src/validation/rules/NoUndefinedVariables.js | 5 +++--
src/validation/rules/NoUnusedVariables.js | 5 +++--
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js
index 4554e200c1..b3878de987 100644
--- a/src/validation/rules/NoUndefinedVariables.js
+++ b/src/validation/rules/NoUndefinedVariables.js
@@ -25,8 +25,9 @@ export function undefinedVarMessage(varName: string, opName: ?string): string {
* directly and via fragment spreads, are defined by that definition.
*
* NOTE: if experimentalFragmentVariables are used, then fragments with
- * variables defined are considered an independent "executable definitions":
- * variables defined under them do not count as "within a fragment spread".
+ * variables defined are considered independent "executable definitions".
+ * If a fragment defines at least 1 variable, it must define all recursively
+ * vused ariables, excluding other fragments with variables defined.
*/
export function NoUndefinedVariables(context: ValidationContext): ASTVisitor {
let variableNameDefined = Object.create(null);
diff --git a/src/validation/rules/NoUnusedVariables.js b/src/validation/rules/NoUnusedVariables.js
index cf291babf6..3566ac77bd 100644
--- a/src/validation/rules/NoUnusedVariables.js
+++ b/src/validation/rules/NoUnusedVariables.js
@@ -28,8 +28,9 @@ export function unusedVariableMessage(
* definition are used, either directly or within a spread fragment.
*
* NOTE: if experimentalFragmentVariables are used, then fragments with
- * variables defined are considered an independent "executable definitions":
- * variables defined under them do not count as "within a fragment spread".
+ * variables defined are considered independent "executable definitions".
+ * 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 = [];
From 0e1c6de7b5b2013c36d92495e2f3d10896c1f144 Mon Sep 17 00:00:00 2001
From: mmahoney <mmahoney@fb.com>
Date: Tue, 17 Jul 2018 13:36:27 -0400
Subject: [PATCH 4/6] Remove dependencies outside of ValidationContext on
variableDefinitions + comments
---
src/validation/ValidationContext.js | 17 ++++++-----------
src/validation/rules/NoUndefinedVariables.js | 11 ++++++-----
src/validation/rules/NoUnusedVariables.js | 11 ++++++-----
3 files changed, 18 insertions(+), 21 deletions(-)
diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js
index 495e24fe9e..31fd0884b3 100644
--- a/src/validation/ValidationContext.js
+++ b/src/validation/ValidationContext.js
@@ -155,7 +155,7 @@ export default class ValidationContext {
const fragment = this.getFragment(fragName);
if (
fragment &&
- !this.isExecutableDefinitionWithVariables(fragment)
+ !isExperimentalFragmentWithVariableDefinitions(fragment)
) {
fragments.push(fragment);
nodesToVisit.push(fragment.selectionSet);
@@ -244,15 +244,10 @@ export default class ValidationContext {
getArgument(): ?GraphQLArgument {
return this._typeInfo.getArgument();
}
+}
- // All OperationDefinitions, or FragmentDefinitions with variable definitions
- isExecutableDefinitionWithVariables(
- definition: ExecutableDefinitionNode,
- ): boolean {
- return (
- definition.kind === Kind.OPERATION_DEFINITION ||
- (definition.variableDefinitions != null &&
- definition.variableDefinitions.length > 0)
- );
- }
+function isExperimentalFragmentWithVariableDefinitions(
+ ast: FragmentDefinitionNode,
+): boolean %checks {
+ return ast.variableDefinitions != null && ast.variableDefinitions.length > 0;
}
diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js
index b3878de987..478046d0e1 100644
--- a/src/validation/rules/NoUndefinedVariables.js
+++ b/src/validation/rules/NoUndefinedVariables.js
@@ -10,6 +10,7 @@
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 {
@@ -33,14 +34,14 @@ export function NoUndefinedVariables(context: ValidationContext): ASTVisitor {
let variableNameDefined = Object.create(null);
const executableDefinitionVisitor = {
- enter(definition: ExecutableDefinitionNode) {
- if (!context.isExecutableDefinitionWithVariables(definition)) {
- return;
- }
+ enter() {
variableNameDefined = Object.create(null);
},
leave(definition: ExecutableDefinitionNode) {
- if (!context.isExecutableDefinitionWithVariables(definition)) {
+ if (
+ definition.kind === Kind.FRAGMENT_DEFINITION &&
+ Object.keys(variableNameDefined).length === 0
+ ) {
return;
}
const usages = context.getRecursiveVariableUsages(definition);
diff --git a/src/validation/rules/NoUnusedVariables.js b/src/validation/rules/NoUnusedVariables.js
index 3566ac77bd..a5f0bbeb83 100644
--- a/src/validation/rules/NoUnusedVariables.js
+++ b/src/validation/rules/NoUnusedVariables.js
@@ -9,6 +9,7 @@
import type ValidationContext from '../ValidationContext';
import { GraphQLError } from '../../error';
+import { Kind } from '../../language';
import type { ExecutableDefinitionNode } from '../../language';
import type { ASTVisitor } from '../../language/visitor';
@@ -36,14 +37,14 @@ export function NoUnusedVariables(context: ValidationContext): ASTVisitor {
let variableDefs = [];
const executableDefinitionVisitor = {
- enter(definition: ExecutableDefinitionNode) {
- if (!context.isExecutableDefinitionWithVariables(definition)) {
- return;
- }
+ enter() {
variableDefs = [];
},
leave(definition: ExecutableDefinitionNode) {
- if (!context.isExecutableDefinitionWithVariables(definition)) {
+ if (
+ definition.kind === Kind.FRAGMENT_DEFINITION &&
+ variableDefs.length === 0
+ ) {
return;
}
From 618349e6f7b7ce7ba94d723b74a1e50292fcdc73 Mon Sep 17 00:00:00 2001
From: mmahoney <mmahoney@fb.com>
Date: Tue, 17 Jul 2018 13:39:44 -0400
Subject: [PATCH 5/6] typo
---
src/validation/rules/NoUndefinedVariables.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/validation/rules/NoUndefinedVariables.js b/src/validation/rules/NoUndefinedVariables.js
index 478046d0e1..e81cdb718e 100644
--- a/src/validation/rules/NoUndefinedVariables.js
+++ b/src/validation/rules/NoUndefinedVariables.js
@@ -28,7 +28,7 @@ export function undefinedVarMessage(varName: string, opName: ?string): string {
* NOTE: if experimentalFragmentVariables are used, then fragments with
* variables defined are considered independent "executable definitions".
* If a fragment defines at least 1 variable, it must define all recursively
- * vused ariables, excluding other fragments with variables defined.
+ * used variables, excluding other fragments with variables defined.
*/
export function NoUndefinedVariables(context: ValidationContext): ASTVisitor {
let variableNameDefined = Object.create(null);
From f8be13549e76c255cafe49596791dfe84db7db04 Mon Sep 17 00:00:00 2001
From: mmahoney <mmahoney@fb.com>
Date: Tue, 17 Jul 2018 18:38:21 -0400
Subject: [PATCH 6/6] Add an optional param for validation: whether to validate
specially for fragment variables
---
src/validation/ValidationContext.js | 48 ++++++++++++++-----
.../__tests__/NoUndefinedVariables-test.js | 11 +++--
.../__tests__/NoUnusedVariables-test.js | 15 ++++--
src/validation/__tests__/harness.js | 34 +++++++++++--
src/validation/rules/NoUndefinedVariables.js | 5 +-
src/validation/rules/NoUnusedVariables.js | 2 +-
src/validation/validate.js | 6 ++-
7 files changed, 91 insertions(+), 30 deletions(-)
diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js
index 31fd0884b3..6df71fd0ed 100644
--- a/src/validation/ValidationContext.js
+++ b/src/validation/ValidationContext.js
@@ -38,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
@@ -59,11 +75,13 @@ export default class ValidationContext {
ExecutableDefinitionNode,
$ReadOnlyArray<VariableUsage>,
>;
+ _options: ValidationOptions;
constructor(
schema: GraphQLSchema,
ast: DocumentNode,
typeInfo: TypeInfo,
+ options: ValidationOptions = {},
): void {
this._schema = schema;
this._ast = ast;
@@ -73,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 {
@@ -133,14 +152,14 @@ export default class ValidationContext {
/*
* Finds all fragments referenced via the definition, recursively.
*
- * NOTE: if experimentalFragmentVariables are being used, it excludes all
- * fragments with their own variable definitions: these are considered their
- * own "root" executable definition.
+ * If the experimentalFragmentVariables option is used, this will not visit
+ * fragment definitions that include variable definitions.
*/
getRecursivelyReferencedFragments(
definition: ExecutableDefinitionNode,
): $ReadOnlyArray<FragmentDefinitionNode> {
let fragments = this._recursivelyReferencedFragments.get(definition);
+
if (!fragments) {
fragments = [];
const collectedNames = Object.create(null);
@@ -153,16 +172,25 @@ export default class ValidationContext {
if (collectedNames[fragName] !== true) {
collectedNames[fragName] = true;
const fragment = this.getFragment(fragName);
+
+ if (!fragment) {
+ continue;
+ }
+
if (
- fragment &&
- !isExperimentalFragmentWithVariableDefinitions(fragment)
+ this._options.experimentalFragmentVariables &&
+ fragment.variableDefinitions &&
+ fragment.variableDefinitions.length > 0
) {
- fragments.push(fragment);
- nodesToVisit.push(fragment.selectionSet);
+ continue;
}
+
+ fragments.push(fragment);
+ nodesToVisit.push(fragment.selectionSet);
}
}
}
+
this._recursivelyReferencedFragments.set(definition, fragments);
}
return fragments;
@@ -245,9 +273,3 @@ export default class ValidationContext {
return this._typeInfo.getArgument();
}
}
-
-function isExperimentalFragmentWithVariableDefinitions(
- ast: FragmentDefinitionNode,
-): boolean %checks {
- return ast.variableDefinitions != null && ast.variableDefinitions.length > 0;
-}
diff --git a/src/validation/__tests__/NoUndefinedVariables-test.js b/src/validation/__tests__/NoUndefinedVariables-test.js
index 67156e4e1d..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,
@@ -332,7 +337,7 @@ describe('Validate: No undefined variables', () => {
// Experimental Fragment Variables
it('uses all variables in fragments with variable definitions', () => {
- expectPassesRule(
+ expectPassesRuleWithFragmentVariables(
NoUndefinedVariables,
`
fragment Foo($a: String, $b: String, $c: String) on Type {
@@ -356,7 +361,7 @@ describe('Validate: No undefined variables', () => {
});
it('variable used in fragment not defined', () => {
- expectFailsRule(
+ expectFailsRuleWithFragmentVariables(
NoUndefinedVariables,
`
fragment FragA($a: String) on Type {
diff --git a/src/validation/__tests__/NoUnusedVariables-test.js b/src/validation/__tests__/NoUnusedVariables-test.js
index bbdff51ffc..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,
@@ -240,7 +245,7 @@ describe('Validate: No unused variables', () => {
// Experimental Fragment Variables
it('uses all variables in fragments with variable definitions', () => {
- expectPassesRule(
+ expectPassesRuleWithFragmentVariables(
NoUnusedVariables,
`
fragment Foo($a: String, $b: String, $c: String) on Type {
@@ -264,7 +269,7 @@ describe('Validate: No unused variables', () => {
});
it('variable not used by fragment', () => {
- expectFailsRule(
+ expectFailsRuleWithFragmentVariables(
NoUnusedVariables,
`
fragment FragA($a: String) on Type {
@@ -276,7 +281,7 @@ describe('Validate: No unused variables', () => {
});
it('variable used in query defined by fragment', () => {
- expectFailsRule(
+ expectFailsRuleWithFragmentVariables(
NoUnusedVariables,
`
query Foo($a: String) {
@@ -291,7 +296,7 @@ describe('Validate: No unused variables', () => {
);
it('variable defined in fragment used by query', () => {
- expectFailsRule(
+ expectFailsRuleWithFragmentVariables(
NoUnusedVariables,
`
query Foo($a: String) {
diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js
index ca92c211c0..ba3664e60f 100644
--- a/src/validation/__tests__/harness.js
+++ b/src/validation/__tests__/harness.js
@@ -421,20 +421,30 @@ export const testSchema = new GraphQLSchema({
],
});
-function expectValid(schema, rules, queryString) {
+function expectValid(schema, rules, queryString, options = {}) {
const errors = validate(
schema,
- parse(queryString, { experimentalFragmentVariables: true }),
+ parse(queryString, options),
rules,
+ null, // default TypeInfo
+ options,
);
expect(errors).to.deep.equal([], 'Should validate');
}
-function expectInvalid(schema, rules, queryString, expectedErrors) {
+function expectInvalid(
+ schema,
+ rules,
+ queryString,
+ expectedErrors,
+ options = {},
+) {
const errors = validate(
schema,
- parse(queryString, { experimentalFragmentVariables: true }),
+ 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);
@@ -456,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 b7f78288ca..18ca4b6589 100644
--- a/src/validation/rules/NoUndefinedVariables.js
+++ b/src/validation/rules/NoUndefinedVariables.js
@@ -26,9 +26,8 @@ export function undefinedVarMessage(varName: string, opName: ?string): string {
* directly and via fragment spreads, are defined by that definition.
*
* NOTE: if experimentalFragmentVariables are used, then fragments with
- * variables defined are considered independent "executable definitions".
- * If a fragment defines at least 1 variable, it must define all recursively
- * used variables, excluding other fragments with variables defined.
+ * 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);
diff --git a/src/validation/rules/NoUnusedVariables.js b/src/validation/rules/NoUnusedVariables.js
index 39aa19fe66..36b3b15c0c 100644
--- a/src/validation/rules/NoUnusedVariables.js
+++ b/src/validation/rules/NoUnusedVariables.js
@@ -29,7 +29,7 @@ export function unusedVariableMessage(
* definition are used, either directly or within a spread fragment.
*
* NOTE: if experimentalFragmentVariables are used, then fragments with
- * variables defined are considered independent "executable definitions".
+ * variable definitions will validate independently.
* So `query Foo` must not define `$a` when `$a` is only used inside
* `fragment FragA($a: Type)`
*/
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<any>,
typeInfo?: TypeInfo,
+ options?: ValidationOptions,
): $ReadOnlyArray<GraphQLError> {
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<GraphQLError> {
- 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)));