Skip to content

Fragment variables: enforce fragment variable validation #1422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from
Closed

Fragment variables: enforce fragment variable validation #1422

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d9951e
Fragment variables: enforce fragment variable validation
mjmahone Jul 16, 2018
cbd3abf
Modify comments to be a bit more accurate
mjmahone Jul 16, 2018
e9a7991
More comment cleanup
mjmahone Jul 17, 2018
0e1c6de
Remove dependencies outside of ValidationContext on variableDefinitio…
mjmahone Jul 17, 2018
618349e
typo
mjmahone Jul 17, 2018
90408dc
Merge branch 'master' of https://github.com/graphql/graphql-js into f…
mjmahone Jul 17, 2018
f8be135
Add an optional param for validation: whether to validate specially f…
mjmahone Jul 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 36 additions & 12 deletions src/validation/ValidationContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { visit, visitWithTypeInfo } from '../language/visitor';
import { Kind } from '../language/kinds';
import type {
DocumentNode,
ExecutableDefinitionNode,
OperationDefinitionNode,
VariableNode,
SelectionSetNode,
Expand Down Expand Up @@ -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>,
>;

Expand Down Expand Up @@ -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(
Copy link
Member

@IvanGoncharov IvanGoncharov Jul 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very concern about changes in this function since it's called getRecursivelyReferencedFragments but not return all recursive fragments and is part of public API.
+ it breaks NoUnusedFragments

An alternative solution would be to extract visitReferencedFragmentsRecursively that accept visitor function and if you return false it doesn't look inside the current fragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only breaks NoUnusedFragments if you're using fragments with variable definitions. I think this is OK, as fragments with variable definitions are their own independent "unit": they have semantic meaning that is very different from normal fragments without variable definitions. An unused fragment that has variable definitions I see as being "stand-alone" in the same way a query is "stand-alone": you could reasonably figure out a way to persist these to the server and execute them, provided you started on the correct type.

fragment A($id: ID!) on Query {
  node(id: $id) {
    name
  }
}

is, and in my mind should be, identical to

query A($id: ID!) {
  node(id: $id) {
    name
  }
}

I'm very OK with breaking people who are actually using fragments with variable definitions.

I can see the appeal to extracting it out, but I'd much prefer taking an opinionated stance about what a fragment with variable definitions is. It will also make it easier to make an RFC in the spec for them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in that case, it's outside of scope for graphql-js and would make sense to discuss it in PR/RFC.

Just a quick note:

and execute them, provided you started on the correct type

There is no one to one matching between root types and operation types so you can write:

schema {
  query: Type
  mutation: Type
  subscription: Type
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah after thinking this through I think it does make more sense to add a second function specifically for excluding fragments with variable definitions. I'll make that change.

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);
Expand All @@ -145,14 +153,17 @@ export default class ValidationContext {
if (collectedNames[fragName] !== true) {
collectedNames[fragName] = true;
const fragment = this.getFragment(fragName);
if (fragment) {
if (
fragment &&
!isExperimentalFragmentWithVariableDefinitions(fragment)
) {
fragments.push(fragment);
nodesToVisit.push(fragment.selectionSet);
}
}
}
}
this._recursivelyReferencedFragments.set(operation, fragments);
this._recursivelyReferencedFragments.set(definition, fragments);
}
return fragments;
}
Expand Down Expand Up @@ -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 independent executable definition for the purposes of variable usage.
*/
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]),
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah after thinking this through I think it does make more sense to add a second function specifically for excluding fragments with variable definitions. I'll make that change.

@mjmahone Idea mode 💡: I think we can have one function to do both

visitReferencedFragmentsRecursively(definition, fragment => {
  if (isExperimentalFragmentWithVariableDefinitions(fragment)) {
    return false;
  }
  Array.prototype.push.apply(
    usages,
    this.getVariableUsages(fragments[i]),
  );
});

And it will also simplify this code:

graphql-js/src/validation/rules/NoUnusedFragments.js

Lines 41 to 45 in e6c36e0

for (const fragment of context.getRecursivelyReferencedFragments(
operation,
)) {
fragmentNameUsed[fragment.name.value] = true;
}

Plus you don't need to create intermidiate array in a process.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjmahone Stupid idea 🤦‍♂️ because it will break caching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm would it make more sense to create the ValidationContext with a flag indicating we should use fragment variables as a "validation cut" point? Then you're either opting in to it or not, and we could for instance validate "human-written" graphql with fragment variables as well as "transformed" (whatever that means) graphql in two different validation steps, just by creating a new validation context without the flag.

Basically this means people using fragment variables might consume more memory/make two passes, but I'd rather have people on the experimental version have some pain than make things confusing for those on mainline GraphQL.

this._recursiveVariableUsages.set(operation, usages);
this._recursiveVariableUsages.set(definition, usages);
}
return usages;
}
Expand Down Expand Up @@ -227,3 +245,9 @@ export default class ValidationContext {
return this._typeInfo.getArgument();
}
}

function isExperimentalFragmentWithVariableDefinitions(
ast: FragmentDefinitionNode,
): boolean %checks {
return ast.variableDefinitions != null && ast.variableDefinitions.length > 0;
}
38 changes: 38 additions & 0 deletions src/validation/__tests__/NoUndefinedVariables-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)],
);
});
});
68 changes: 68 additions & 0 deletions src/validation/__tests__/NoUnusedVariables-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)],
);
});
});
});
12 changes: 10 additions & 2 deletions src/validation/__tests__/harness.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are maintainers of GraphQL 3rd-party implementations that use graphql-js test suite as the refence so I don't think we should enable the experimental feature by default for all tests.
It's better to just add options as 4th argument, it would make it more explicit and also allow to easily grep for all tests cases with the particular experimental feature.

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;
Expand Down
62 changes: 39 additions & 23 deletions src/validation/rules/NoUndefinedVariables.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -20,36 +22,50 @@ 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 independent "executable definitions".
* If a fragment defines at least 1 variable, it must define all recursively
* used variables, excluding other fragments with variables defined.
*/
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);

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;
},
Expand Down
Loading