Skip to content

Commit b05352c

Browse files
mjmahonemjmahone
committed
Remove flag for parsing, but add default validation to indicate fragment args are not yet spec-supported, and may be missing validation.
1 parent b4bddc7 commit b05352c

File tree

10 files changed

+105
-59
lines changed

10 files changed

+105
-59
lines changed

‎src/execution/__tests__/variables-test.ts

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -130,14 +130,6 @@ function executeQuery(
130130
return executeSync({ schema, document, variableValues });
131131
}
132132

133-
function executeQueryWithFragmentArguments(
134-
query: string,
135-
variableValues?: { [variable: string]: unknown },
136-
) {
137-
const document = parse(query, { allowFragmentArguments: true });
138-
return executeSync({ schema, document, variableValues });
139-
}
140-
141133
describe('Execute: Handles inputs', () => {
142134
describe('Handles objects and nullability', () => {
143135
describe('using inline structs', () => {
@@ -1032,7 +1024,7 @@ describe('Execute: Handles inputs', () => {
10321024
});
10331025

10341026
it('when a value is required and provided', () => {
1035-
const result = executeQueryWithFragmentArguments(`
1027+
const result = executeQuery(`
10361028
query {
10371029
...a(value: "A")
10381030
}
@@ -1049,7 +1041,7 @@ describe('Execute: Handles inputs', () => {
10491041
});
10501042

10511043
it('when a value is required and not provided', () => {
1052-
const result = executeQueryWithFragmentArguments(`
1044+
const result = executeQuery(`
10531045
query {
10541046
...a
10551047
}
@@ -1076,7 +1068,7 @@ describe('Execute: Handles inputs', () => {
10761068
});
10771069

10781070
it('when the definition has a default and is provided', () => {
1079-
const result = executeQueryWithFragmentArguments(`
1071+
const result = executeQuery(`
10801072
query {
10811073
...a(value: "A")
10821074
}
@@ -1093,7 +1085,7 @@ describe('Execute: Handles inputs', () => {
10931085
});
10941086

10951087
it('when the definition has a default and is not provided', () => {
1096-
const result = executeQueryWithFragmentArguments(`
1088+
const result = executeQuery(`
10971089
query {
10981090
...a
10991091
}
@@ -1110,7 +1102,7 @@ describe('Execute: Handles inputs', () => {
11101102
});
11111103

11121104
it('when the definition has a non-nullable default and is provided null', () => {
1113-
const result = executeQueryWithFragmentArguments(`
1105+
const result = executeQuery(`
11141106
query {
11151107
...a(value: null)
11161108
}
@@ -1137,7 +1129,7 @@ describe('Execute: Handles inputs', () => {
11371129
});
11381130

11391131
it('when the definition has no default and is not provided', () => {
1140-
const result = executeQueryWithFragmentArguments(`
1132+
const result = executeQuery(`
11411133
query {
11421134
...a
11431135
}
@@ -1155,7 +1147,7 @@ describe('Execute: Handles inputs', () => {
11551147
});
11561148

11571149
it('when the argument variable is nested in a complex type', () => {
1158-
const result = executeQueryWithFragmentArguments(`
1150+
const result = executeQuery(`
11591151
query {
11601152
...a(value: "C")
11611153
}
@@ -1172,7 +1164,7 @@ describe('Execute: Handles inputs', () => {
11721164
});
11731165

11741166
it('when argument variables are used recursively', () => {
1175-
const result = executeQueryWithFragmentArguments(`
1167+
const result = executeQuery(`
11761168
query {
11771169
...a(aValue: "C")
11781170
}

‎src/language/__tests__/parser-test.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -366,19 +366,13 @@ describe('Parser', () => {
366366
it('allows parsing fragment defined variables', () => {
367367
const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }';
368368

369-
expect(() =>
370-
parse(document, { allowFragmentArguments: true }),
371-
).to.not.throw();
372-
expect(() => parse(document)).to.throw('Syntax Error');
369+
expect(() => parse(document)).to.not.throw();
373370
});
374371

375372
it('allows parsing fragment spread arguments', () => {
376373
const document = 'fragment a on t { ...b(v: $v) }';
377374

378-
expect(() =>
379-
parse(document, { allowFragmentArguments: true }),
380-
).to.not.throw();
381-
expect(() => parse(document)).to.throw('Syntax Error');
375+
expect(() => parse(document)).to.not.throw();
382376
});
383377

384378
it('contains location information that only stringifies start/end', () => {

‎src/language/__tests__/printer-test.ts

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,6 @@ describe('Printer: Query document', () => {
116116
it('prints fragment with variable directives', () => {
117117
const queryASTWithVariableDirective = parse(
118118
'fragment Foo($foo: TestType @test) on TestType @testDirective { id }',
119-
{ allowFragmentArguments: true },
120119
);
121120
expect(print(queryASTWithVariableDirective)).to.equal(dedent`
122121
fragment Foo($foo: TestType @test) on TestType @testDirective {
@@ -132,7 +131,6 @@ describe('Printer: Query document', () => {
132131
id
133132
}
134133
`,
135-
{ allowFragmentArguments: true },
136134
);
137135
expect(print(fragmentWithVariable)).to.equal(dedent`
138136
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
@@ -144,7 +142,6 @@ describe('Printer: Query document', () => {
144142
it('prints fragment spread with arguments', () => {
145143
const queryASTWithVariableDirective = parse(
146144
'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }',
147-
{ allowFragmentArguments: true },
148145
);
149146
expect(print(queryASTWithVariableDirective)).to.equal(dedent`
150147
fragment Foo on TestType {
@@ -156,7 +153,6 @@ describe('Printer: Query document', () => {
156153
it('prints fragment spread with multi-line arguments', () => {
157154
const queryASTWithVariableDirective = parse(
158155
'fragment Foo on TestType { ...Bar(a: {x: $x, y: $y, z: $z, xy: $xy}, b: true, c: "a long string extending arguments over max length") }',
159-
{ allowFragmentArguments: true },
160156
);
161157
expect(print(queryASTWithVariableDirective)).to.equal(dedent`
162158
fragment Foo on TestType {

‎src/language/__tests__/visitor-test.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,6 @@ describe('Visitor', () => {
465465
it('visits variables defined in fragments', () => {
466466
const ast = parse('fragment a($v: Boolean = false) on t { f }', {
467467
noLocation: true,
468-
allowFragmentArguments: true,
469468
});
470469
const visited: Array<any> = [];
471470

@@ -515,7 +514,6 @@ describe('Visitor', () => {
515514
it('visits arguments on fragment spreads', () => {
516515
const ast = parse('fragment a on t { ...s(v: false) }', {
517516
noLocation: true,
518-
allowFragmentArguments: true,
519517
});
520518
const visited: Array<any> = [];
521519

‎src/language/parser.ts

Lines changed: 5 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -80,24 +80,6 @@ export interface ParseOptions {
8080
* disables that behavior for performance or testing.
8181
*/
8282
noLocation?: boolean;
83-
84-
/**
85-
* If enabled, the parser will understand and parse variable definitions
86-
* contained in a fragment definition. They'll be represented in the
87-
* `variableDefinitions` field of the FragmentDefinitionNode.
88-
*
89-
* Additionally, the parser will understand arguments passed to fragment
90-
* spreads. They'll be represented in the `arguments` field of the
91-
* FragmentSpreadNode.
92-
*
93-
* The syntax is identical to normal, query-defined variables. For example:
94-
*
95-
* fragment A($var: Boolean = false) on T {
96-
* ...
97-
* }
98-
*
99-
*/
100-
allowFragmentArguments?: boolean;
10183
}
10284

10385
/**
@@ -450,10 +432,7 @@ export class Parser {
450432
const hasTypeCondition = this.expectOptionalKeyword('on');
451433
if (!hasTypeCondition && this.peek(TokenKind.NAME)) {
452434
const name = this.parseFragmentName();
453-
if (
454-
this._options?.allowFragmentArguments === true &&
455-
this.peek(TokenKind.PAREN_L)
456-
) {
435+
if (this.peek(TokenKind.PAREN_L)) {
457436
return this.node<FragmentSpreadNode>(start, {
458437
kind: Kind.FRAGMENT_SPREAD,
459438
name,
@@ -484,13 +463,11 @@ export class Parser {
484463
parseFragmentDefinition(): FragmentDefinitionNode {
485464
const start = this._lexer.token;
486465
this.expectKeyword('fragment');
487-
// Legacy support for defining variables within fragments changes
488-
// the grammar of FragmentDefinition:
489-
// - fragment FragmentName VariableDefinitions? on TypeCondition Directives? SelectionSet
490-
if (this._options?.allowFragmentArguments === true) {
466+
const name = this.parseFragmentName();
467+
if (this.peek(TokenKind.PAREN_L)) {
491468
return this.node<FragmentDefinitionNode>(start, {
492469
kind: Kind.FRAGMENT_DEFINITION,
493-
name: this.parseFragmentName(),
470+
name,
494471
variableDefinitions: this.parseVariableDefinitions(),
495472
typeCondition: (this.expectKeyword('on'), this.parseNamedType()),
496473
directives: this.parseDirectives(false),
@@ -499,7 +476,7 @@ export class Parser {
499476
}
500477
return this.node<FragmentDefinitionNode>(start, {
501478
kind: Kind.FRAGMENT_DEFINITION,
502-
name: this.parseFragmentName(),
479+
name,
503480
typeCondition: (this.expectKeyword('on'), this.parseNamedType()),
504481
directives: this.parseDirectives(false),
505482
selectionSet: this.parseSelectionSet(),

‎src/utilities/buildASTSchema.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,6 @@ export function buildSchema(
9999
): GraphQLSchema {
100100
const document = parse(source, {
101101
noLocation: options?.noLocation,
102-
allowFragmentArguments: options?.allowFragmentArguments,
103102
});
104103

105104
return buildASTSchema(document, {

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

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
import { describe, it } from 'mocha';
2+
3+
import { NoFragmentArgumentUsageRule } from '../rules/custom/NoFragmentArgumentUsageRule';
4+
5+
import { expectValidationErrors } from './harness';
6+
7+
function expectErrors(queryStr: string) {
8+
return expectValidationErrors(NoFragmentArgumentUsageRule, queryStr);
9+
}
10+
11+
function expectValid(queryStr: string) {
12+
expectErrors(queryStr).to.deep.equal([]);
13+
}
14+
15+
describe('Validate: Known fragment names', () => {
16+
it('known fragment names are valid', () => {
17+
expectValid(`
18+
{
19+
...HumanFields
20+
}
21+
fragment HumanFields on Query {
22+
human(id: 4) {
23+
name
24+
}
25+
}
26+
`);
27+
});
28+
29+
it('unknown fragment names are invalid', () => {
30+
expectErrors(`
31+
{
32+
...HumanFields(x: 4)
33+
}
34+
fragment HumanFields($x: ID) on Query {
35+
human(id: $x) {
36+
name
37+
}
38+
}
39+
`).to.deep.equal([
40+
{
41+
message: 'Fragment arguments are not enabled.',
42+
locations: [{ line: 3, column: 24 }],
43+
},
44+
{
45+
message: 'Fragment argument definitions are not enabled.',
46+
locations: [{ line: 5, column: 28 }],
47+
},
48+
]);
49+
});
50+
});

‎src/validation/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,3 +96,4 @@ export { PossibleTypeExtensionsRule } from './rules/PossibleTypeExtensionsRule';
9696
/** Optional rules not defined by the GraphQL Specification */
9797
export { NoDeprecatedCustomRule } from './rules/custom/NoDeprecatedCustomRule';
9898
export { NoSchemaIntrospectionCustomRule } from './rules/custom/NoSchemaIntrospectionCustomRule';
99+
export { NoFragmentArgumentUsageRule } from './rules/custom/NoFragmentArgumentUsageRule';

‎src/validation/rules/custom/NoFragmentArgumentUsageRule.ts

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import { GraphQLError } from '../../../error/GraphQLError';
2+
3+
import type { ASTVisitor } from '../../../language/visitor';
4+
5+
import type { ASTValidationContext } from '../../ValidationContext';
6+
7+
/**
8+
* Fragment arguments are, by default, not allowed to be used.
9+
*/
10+
export function NoFragmentArgumentUsageRule(
11+
context: ASTValidationContext,
12+
): ASTVisitor {
13+
return {
14+
FragmentDefinition(node) {
15+
if (node.variableDefinitions && node.variableDefinitions.length > 0) {
16+
context.reportError(
17+
new GraphQLError(
18+
'Fragment argument definitions are not enabled.',
19+
node.variableDefinitions[0],
20+
),
21+
);
22+
}
23+
},
24+
FragmentSpread(node) {
25+
if (node.arguments && node.arguments.length > 0) {
26+
context.reportError(
27+
new GraphQLError(
28+
'Fragment arguments are not enabled.',
29+
node.arguments[0],
30+
),
31+
);
32+
}
33+
},
34+
};
35+
}

‎src/validation/specifiedRules.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,9 @@ import { UniqueFieldDefinitionNamesRule } from './rules/UniqueFieldDefinitionNam
9393
import { UniqueDirectiveNamesRule } from './rules/UniqueDirectiveNamesRule';
9494
import { PossibleTypeExtensionsRule } from './rules/PossibleTypeExtensionsRule';
9595

96+
// Prevent not-yet-in-spec usage of Fragment Arguments by default
97+
import { NoFragmentArgumentUsageRule } from './rules/custom/NoFragmentArgumentUsageRule';
98+
9699
/**
97100
* This set includes all validation rules defined by the GraphQL spec.
98101
*
@@ -114,6 +117,7 @@ export const specifiedRules: ReadonlyArray<ValidationRule> = Object.freeze([
114117
NoUnusedFragmentsRule,
115118
PossibleFragmentSpreadsRule,
116119
NoFragmentCyclesRule,
120+
NoFragmentArgumentUsageRule,
117121
UniqueVariableNamesRule,
118122
NoUndefinedVariablesRule,
119123
NoUnusedVariablesRule,

0 commit comments

Comments
 (0)