Skip to content

Commit 686fe56

Browse files
mjmahonemjmahone
committed
Update all validation rules: tests pass
1 parent 76fb6a6 commit 686fe56

14 files changed

+666
-207
lines changed

‎src/language/directiveLocation.ts

+1
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ export enum DirectiveLocation {
1111
FRAGMENT_SPREAD = 'FRAGMENT_SPREAD',
1212
INLINE_FRAGMENT = 'INLINE_FRAGMENT',
1313
VARIABLE_DEFINITION = 'VARIABLE_DEFINITION',
14+
FRAGMENT_ARGUMENT_DEFINITION = 'FRAGMENT_ARGUMENT_DEFINITION',
1415
/** Type System Definitions */
1516
SCHEMA = 'SCHEMA',
1617
SCALAR = 'SCALAR',

‎src/utilities/__tests__/TypeInfo-test.ts

+107
Original file line numberDiff line numberDiff line change
@@ -515,4 +515,111 @@ describe('visitWithTypeInfo', () => {
515515
['leave', 'SelectionSet', null, 'Human', 'Human'],
516516
]);
517517
});
518+
519+
it('supports traversals of fragment arguments', () => {
520+
const typeInfo = new TypeInfo(testSchema);
521+
522+
const ast = parse(`
523+
query {
524+
...Foo(x: 4)
525+
}
526+
527+
fragment Foo(
528+
"Human to get"
529+
$x: ID!
530+
) on QueryRoot {
531+
human(id: $x) { name }
532+
}
533+
`);
534+
535+
const visited: Array<any> = [];
536+
visit(
537+
ast,
538+
visitWithTypeInfo(typeInfo, {
539+
enter(node) {
540+
const type = typeInfo.getType();
541+
const inputType = typeInfo.getInputType();
542+
visited.push([
543+
'enter',
544+
node.kind,
545+
node.kind === 'Name' ? node.value : null,
546+
String(type),
547+
String(inputType),
548+
]);
549+
},
550+
leave(node) {
551+
const type = typeInfo.getType();
552+
const inputType = typeInfo.getInputType();
553+
visited.push([
554+
'leave',
555+
node.kind,
556+
node.kind === 'Name' ? node.value : null,
557+
String(type),
558+
String(inputType),
559+
]);
560+
},
561+
}),
562+
);
563+
564+
expect(visited).to.deep.equal([
565+
['enter', 'Document', null, 'undefined', 'undefined'],
566+
['enter', 'OperationDefinition', null, 'QueryRoot', 'undefined'],
567+
['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'],
568+
['enter', 'FragmentSpread', null, 'QueryRoot', 'undefined'],
569+
['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'],
570+
['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'],
571+
['enter', 'Argument', null, 'QueryRoot', 'ID!'],
572+
['enter', 'Name', 'x', 'QueryRoot', 'ID!'],
573+
['leave', 'Name', 'x', 'QueryRoot', 'ID!'],
574+
['enter', 'IntValue', null, 'QueryRoot', 'ID!'],
575+
['leave', 'IntValue', null, 'QueryRoot', 'ID!'],
576+
['leave', 'Argument', null, 'QueryRoot', 'ID!'],
577+
['leave', 'FragmentSpread', null, 'QueryRoot', 'undefined'],
578+
['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'],
579+
['leave', 'OperationDefinition', null, 'QueryRoot', 'undefined'],
580+
['enter', 'FragmentDefinition', null, 'QueryRoot', 'undefined'],
581+
['enter', 'Name', 'Foo', 'QueryRoot', 'undefined'],
582+
['leave', 'Name', 'Foo', 'QueryRoot', 'undefined'],
583+
['enter', 'FragmentArgumentDefinition', null, 'QueryRoot', 'ID!'],
584+
['enter', 'StringValue', null, 'QueryRoot', 'ID!'],
585+
['leave', 'StringValue', null, 'QueryRoot', 'ID!'],
586+
['enter', 'Variable', null, 'QueryRoot', 'ID!'],
587+
['enter', 'Name', 'x', 'QueryRoot', 'ID!'],
588+
['leave', 'Name', 'x', 'QueryRoot', 'ID!'],
589+
['leave', 'Variable', null, 'QueryRoot', 'ID!'],
590+
['enter', 'NonNullType', null, 'QueryRoot', 'ID!'],
591+
['enter', 'NamedType', null, 'QueryRoot', 'ID!'],
592+
['enter', 'Name', 'ID', 'QueryRoot', 'ID!'],
593+
['leave', 'Name', 'ID', 'QueryRoot', 'ID!'],
594+
['leave', 'NamedType', null, 'QueryRoot', 'ID!'],
595+
['leave', 'NonNullType', null, 'QueryRoot', 'ID!'],
596+
['leave', 'FragmentArgumentDefinition', null, 'QueryRoot', 'ID!'],
597+
['enter', 'NamedType', null, 'QueryRoot', 'undefined'],
598+
['enter', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'],
599+
['leave', 'Name', 'QueryRoot', 'QueryRoot', 'undefined'],
600+
['leave', 'NamedType', null, 'QueryRoot', 'undefined'],
601+
['enter', 'SelectionSet', null, 'QueryRoot', 'undefined'],
602+
['enter', 'Field', null, 'Human', 'undefined'],
603+
['enter', 'Name', 'human', 'Human', 'undefined'],
604+
['leave', 'Name', 'human', 'Human', 'undefined'],
605+
['enter', 'Argument', null, 'Human', 'ID'],
606+
['enter', 'Name', 'id', 'Human', 'ID'],
607+
['leave', 'Name', 'id', 'Human', 'ID'],
608+
['enter', 'Variable', null, 'Human', 'ID'],
609+
['enter', 'Name', 'x', 'Human', 'ID'],
610+
['leave', 'Name', 'x', 'Human', 'ID'],
611+
['leave', 'Variable', null, 'Human', 'ID'],
612+
['leave', 'Argument', null, 'Human', 'ID'],
613+
['enter', 'SelectionSet', null, 'Human', 'undefined'],
614+
['enter', 'Field', null, 'String', 'undefined'],
615+
['enter', 'Name', 'name', 'String', 'undefined'],
616+
['leave', 'Name', 'name', 'String', 'undefined'],
617+
['leave', 'Field', null, 'String', 'undefined'],
618+
['leave', 'SelectionSet', null, 'Human', 'undefined'],
619+
['leave', 'Field', null, 'Human', 'undefined'],
620+
['leave', 'SelectionSet', null, 'QueryRoot', 'undefined'],
621+
['leave', 'FragmentDefinition', null, 'QueryRoot', 'undefined'],
622+
['leave', 'Document', null, 'undefined', 'undefined'],
623+
]);
624+
});
518625
});

‎src/validation/__tests__/KnownDirectivesRule-test.ts

+11-3
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ const schemaWithDirectives = buildSchema(`
4141
directive @onSubscription on SUBSCRIPTION
4242
directive @onField on FIELD
4343
directive @onFragmentDefinition on FRAGMENT_DEFINITION
44+
directive @onFragmentArgumentDefinition on FRAGMENT_ARGUMENT_DEFINITION
4445
directive @onFragmentSpread on FRAGMENT_SPREAD
4546
directive @onInlineFragment on INLINE_FRAGMENT
4647
directive @onVariableDefinition on VARIABLE_DEFINITION
@@ -150,7 +151,9 @@ describe('Validate: Known directives', () => {
150151
someField @onField
151152
}
152153
153-
fragment Frag on Human @onFragmentDefinition {
154+
fragment Frag(
155+
$arg: Int @onFragmentArgumentDefinition
156+
) on Human @onFragmentDefinition {
154157
name @onField
155158
}
156159
`);
@@ -175,7 +178,7 @@ describe('Validate: Known directives', () => {
175178
someField @onQuery
176179
}
177180
178-
fragment Frag on Human @onQuery {
181+
fragment Frag($arg: Int @onField) on Human @onQuery {
179182
name @onQuery
180183
}
181184
`).toDeepEqual([
@@ -219,9 +222,14 @@ describe('Validate: Known directives', () => {
219222
message: 'Directive "@onQuery" may not be used on FIELD.',
220223
locations: [{ column: 19, line: 16 }],
221224
},
225+
{
226+
message:
227+
'Directive "@onField" may not be used on FRAGMENT_ARGUMENT_DEFINITION.',
228+
locations: [{ column: 31, line: 19 }],
229+
},
222230
{
223231
message: 'Directive "@onQuery" may not be used on FRAGMENT_DEFINITION.',
224-
locations: [{ column: 30, line: 19 }],
232+
locations: [{ column: 50, line: 19 }],
225233
},
226234
{
227235
message: 'Directive "@onQuery" may not be used on FIELD.',

‎src/validation/__tests__/NoUnusedFragmentArgumentsRule-test.ts

+120
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
import { describe, it } from 'mocha';
2+
3+
import { NoUnusedFragmentArgumentsRule } from '../rules/NoUnusedFragmentArgumentsRule.js';
4+
5+
import { expectValidationErrors } from './harness.js';
6+
7+
function expectErrors(queryStr: string) {
8+
return expectValidationErrors(NoUnusedFragmentArgumentsRule, queryStr);
9+
}
10+
11+
function expectValid(queryStr: string) {
12+
expectErrors(queryStr).toDeepEqual([]);
13+
}
14+
15+
describe('Validate: No unused fragment arguments', () => {
16+
it('uses all arguments', () => {
17+
expectValid(`
18+
fragment Foo($a: String, $b: String, $c: String) on Type {
19+
field(a: $a, b: $b, c: $c)
20+
}
21+
`);
22+
});
23+
24+
it('uses all arguments deeply', () => {
25+
expectValid(`
26+
fragment Foo($a: String, $b: String, $c: String) on Type {
27+
field(a: $a) {
28+
field(b: $b) {
29+
field(c: $c)
30+
}
31+
}
32+
}
33+
`);
34+
});
35+
36+
it('uses all arguments deeply in inline fragments', () => {
37+
expectValid(`
38+
fragment Foo($a: String, $b: String, $c: String) on Type {
39+
... on Type {
40+
field(a: $a) {
41+
field(b: $b) {
42+
... on Type {
43+
field(c: $c)
44+
}
45+
}
46+
}
47+
}
48+
}
49+
`);
50+
});
51+
52+
it('argument not used', () => {
53+
expectErrors(`
54+
fragment Foo($a: String, $b: String, $c: String) on Type {
55+
field(a: $a, b: $b)
56+
}
57+
`).toDeepEqual([
58+
{
59+
message: 'Argument "$c" is never used in fragment "Foo".',
60+
locations: [{ line: 2, column: 44 }],
61+
},
62+
]);
63+
});
64+
65+
it('query passes in unused argument', () => {
66+
expectErrors(`
67+
query Q($c: String) {
68+
type {
69+
...Foo(a: "", b: "", c: $c)
70+
}
71+
}
72+
fragment Foo($a: String, $b: String, $c: String) on Type {
73+
field(a: $a, b: $b)
74+
}
75+
`).toDeepEqual([
76+
{
77+
message: 'Argument "$c" is never used in fragment "Foo".',
78+
locations: [{ line: 7, column: 44 }],
79+
},
80+
]);
81+
});
82+
83+
it('child fragment uses a variable of the same name', () => {
84+
expectErrors(`
85+
query Q($a: String) {
86+
type {
87+
...Foo
88+
}
89+
}
90+
fragment Foo($a: String) on Type {
91+
...Bar
92+
}
93+
fragment Bar on Type {
94+
field(a: $a)
95+
}
96+
`).toDeepEqual([
97+
{
98+
message: 'Argument "$a" is never used in fragment "Foo".',
99+
locations: [{ line: 7, column: 20 }],
100+
},
101+
]);
102+
});
103+
104+
it('multiple arguments not used', () => {
105+
expectErrors(`
106+
fragment Foo($a: String, $b: String, $c: String) on Type {
107+
field(b: $b)
108+
}
109+
`).toDeepEqual([
110+
{
111+
message: 'Argument "$a" is never used in fragment "Foo".',
112+
locations: [{ line: 2, column: 20 }],
113+
},
114+
{
115+
message: 'Argument "$c" is never used in fragment "Foo".',
116+
locations: [{ line: 2, column: 44 }],
117+
},
118+
]);
119+
});
120+
});

‎src/validation/index.ts

+3
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,9 @@ export { NoUndefinedVariablesRule } from './rules/NoUndefinedVariablesRule.js';
4848
// Spec Section: "Fragments must be used"
4949
export { NoUnusedFragmentsRule } from './rules/NoUnusedFragmentsRule.js';
5050

51+
// Spec Section: "All Fragment Arguments Used"
52+
export { NoUnusedFragmentArgumentsRule } from './rules/NoUnusedFragmentArgumentsRule.js';
53+
5154
// Spec Section: "All Variables Used"
5255
export { NoUnusedVariablesRule } from './rules/NoUnusedVariablesRule.js';
5356

‎src/validation/rules/KnownDirectivesRule.ts

+2
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,8 @@ function getDirectiveLocationForASTPath(
8686
return DirectiveLocation.INLINE_FRAGMENT;
8787
case Kind.FRAGMENT_DEFINITION:
8888
return DirectiveLocation.FRAGMENT_DEFINITION;
89+
case Kind.FRAGMENT_ARGUMENT_DEFINITION:
90+
return DirectiveLocation.FRAGMENT_ARGUMENT_DEFINITION;
8991
case Kind.VARIABLE_DEFINITION:
9092
return DirectiveLocation.VARIABLE_DEFINITION;
9193
case Kind.SCHEMA_DEFINITION:

‎src/validation/rules/NoUndefinedVariablesRule.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,10 @@ export function NoUndefinedVariablesRule(
2222
);
2323

2424
const usages = context.getRecursiveVariableUsages(operation);
25-
for (const { node } of usages) {
25+
for (const { node, fragmentArgDef } of usages) {
26+
if (fragmentArgDef) {
27+
continue;
28+
}
2629
const varName = node.name.value;
2730
if (!variableNameDefined.has(varName)) {
2831
context.reportError(

‎src/validation/rules/NoUnusedFragmentArgumentsRule.ts

+40
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
import { GraphQLError } from '../../error/GraphQLError.js';
2+
3+
import type { ASTVisitor } from '../../language/visitor.js';
4+
5+
import type { ValidationContext } from '../ValidationContext.js';
6+
7+
/**
8+
* No unused variables
9+
*
10+
* A GraphQL fragment is only valid if all arguments defined by it
11+
* are used within the same fragment.
12+
*
13+
* See https://spec.graphql.org/draft/#sec-All-Variables-Used
14+
*/
15+
export function NoUnusedFragmentArgumentsRule(
16+
context: ValidationContext,
17+
): ASTVisitor {
18+
return {
19+
FragmentDefinition(fragment) {
20+
const usages = context.getVariableUsages(fragment);
21+
const argumentNameUsed = new Set<string>(
22+
usages.map(({ node }) => node.name.value),
23+
);
24+
// FIXME: https://github.com/graphql/graphql-js/issues/2203
25+
/* c8 ignore next */
26+
const argumentDefinitions = fragment.arguments ?? [];
27+
for (const argDef of argumentDefinitions) {
28+
const argName = argDef.variable.name.value;
29+
if (!argumentNameUsed.has(argName)) {
30+
context.reportError(
31+
new GraphQLError(
32+
`Argument "$${argName}" is never used in fragment "${fragment.name.value}".`,
33+
{ nodes: argDef },
34+
),
35+
);
36+
}
37+
}
38+
},
39+
};
40+
}

‎src/validation/rules/NoUnusedVariablesRule.ts

+4-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@ export function NoUnusedVariablesRule(context: ValidationContext): ASTVisitor {
1717
OperationDefinition(operation) {
1818
const usages = context.getRecursiveVariableUsages(operation);
1919
const variableNameUsed = new Set<string>(
20-
usages.map(({ node }) => node.name.value),
20+
usages
21+
// Skip variables used as fragment arguments
22+
.filter(({ fragmentArgDef }) => !fragmentArgDef)
23+
.map(({ node }) => node.name.value),
2124
);
2225

2326
// FIXME: https://github.com/graphql/graphql-js/issues/2203

0 commit comments

Comments
 (0)