Skip to content

Commit 7bade9d

Browse files
mjmahonemjmahone
committed
Add fragment arguments to AST and related utility functions
Fragment Args rebased for 2022 Add fragment arguments to AST and related utility functions Replace fragment argument variables at execution time with passed-in arguments. Ensure arguments for unset fragment argument variables are removed so they execute as "unset" rather than as an invalid, undefined variable. Updates to get to 100% code coverage, plus simplify visit so it works even with nested variable inputs Remove flag for parsing, but add default validation to indicate fragment args are not yet spec-supported, and may be missing validation. Update collectFields: no longer throws errors that validation should catch Back to main: quicker to reset and manually redo than to deal with stacked rebase conflicts. Will squash this commit away Re-apply changes, address olds comments, but do not start on functional work
1 parent 67aefd9 commit 7bade9d

File tree

9 files changed

+308
-59
lines changed

9 files changed

+308
-59
lines changed

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

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,6 +1006,156 @@ describe('Execute: Handles inputs', () => {
10061006
});
10071007
});
10081008

1009+
describe('using fragment arguments', () => {
1010+
it('when there are no fragment arguments', () => {
1011+
const result = executeQuery(`
1012+
query {
1013+
...a
1014+
}
1015+
fragment a on TestType {
1016+
fieldWithNonNullableStringInput(input: "A")
1017+
}
1018+
`);
1019+
expect(result).to.deep.equal({
1020+
data: {
1021+
fieldWithNonNullableStringInput: '"A"',
1022+
},
1023+
});
1024+
});
1025+
1026+
it('when a value is required and provided', () => {
1027+
const result = executeQuery(`
1028+
query {
1029+
...a(value: "A")
1030+
}
1031+
fragment a($value: String!) on TestType {
1032+
fieldWithNonNullableStringInput(input: $value)
1033+
}
1034+
`);
1035+
expect(result).to.deep.equal({
1036+
data: {
1037+
fieldWithNonNullableStringInput: '"A"',
1038+
},
1039+
});
1040+
});
1041+
1042+
it('when a value is required and not provided', () => {
1043+
const result = executeQuery(`
1044+
query {
1045+
...a
1046+
}
1047+
fragment a($value: String!) on TestType {
1048+
fieldWithNullableStringInput(input: $value)
1049+
}
1050+
`);
1051+
expect(result).to.deep.equal({
1052+
data: {
1053+
fieldWithNullableStringInput: null,
1054+
},
1055+
});
1056+
});
1057+
1058+
it('when the definition has a default and is provided', () => {
1059+
const result = executeQuery(`
1060+
query {
1061+
...a(value: "A")
1062+
}
1063+
fragment a($value: String! = "B") on TestType {
1064+
fieldWithNonNullableStringInput(input: $value)
1065+
}
1066+
`);
1067+
expect(result).to.deep.equal({
1068+
data: {
1069+
fieldWithNonNullableStringInput: '"A"',
1070+
},
1071+
});
1072+
});
1073+
1074+
it('when the definition has a default and is not provided', () => {
1075+
const result = executeQuery(`
1076+
query {
1077+
...a
1078+
}
1079+
fragment a($value: String! = "B") on TestType {
1080+
fieldWithNonNullableStringInput(input: $value)
1081+
}
1082+
`);
1083+
expect(result).to.deep.equal({
1084+
data: {
1085+
fieldWithNonNullableStringInput: '"B"',
1086+
},
1087+
});
1088+
});
1089+
1090+
it('when the definition has a non-nullable default and is provided null', () => {
1091+
const result = executeQuery(`
1092+
query {
1093+
...a(value: null)
1094+
}
1095+
fragment a($value: String! = "B") on TestType {
1096+
fieldWithNullableStringInput(input: $value)
1097+
}
1098+
`);
1099+
expect(result).to.deep.equal({
1100+
data: {
1101+
fieldWithNullableStringInput: 'null',
1102+
},
1103+
});
1104+
});
1105+
1106+
it('when the definition has no default and is not provided', () => {
1107+
const result = executeQuery(`
1108+
query {
1109+
...a
1110+
}
1111+
fragment a($value: String) on TestType {
1112+
fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $value)
1113+
}
1114+
`);
1115+
expect(result).to.deep.equal({
1116+
data: {
1117+
fieldWithNonNullableStringInputAndDefaultArgumentValue:
1118+
'"Hello World"',
1119+
},
1120+
});
1121+
});
1122+
1123+
it('when the argument variable is nested in a complex type', () => {
1124+
const result = executeQuery(`
1125+
query {
1126+
...a(value: "C")
1127+
}
1128+
fragment a($value: String) on TestType {
1129+
list(input: ["A", "B", $value, "D"])
1130+
}
1131+
`);
1132+
expect(result).to.deep.equal({
1133+
data: {
1134+
list: '["A", "B", "C", "D"]',
1135+
},
1136+
});
1137+
});
1138+
1139+
it('when argument variables are used recursively', () => {
1140+
const result = executeQuery(`
1141+
query {
1142+
...a(aValue: "C")
1143+
}
1144+
fragment a($aValue: String) on TestType {
1145+
...b(bValue: $aValue)
1146+
}
1147+
fragment b($bValue: String) on TestType {
1148+
list(input: ["A", "B", $bValue, "D"])
1149+
}
1150+
`);
1151+
expect(result).to.deep.equal({
1152+
data: {
1153+
list: '["A", "B", "C", "D"]',
1154+
},
1155+
});
1156+
});
1157+
});
1158+
10091159
describe('getVariableValues: limit maximum number of coercion errors', () => {
10101160
const doc = parse(`
10111161
query ($input: [String!]) {

‎src/execution/collectFields.ts

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,17 @@ import { AccumulatorMap } from '../jsutils/AccumulatorMap';
22
import type { ObjMap } from '../jsutils/ObjMap';
33

44
import type {
5+
ArgumentNode,
56
FieldNode,
67
FragmentDefinitionNode,
78
FragmentSpreadNode,
89
InlineFragmentNode,
910
SelectionSetNode,
11+
ValueNode,
12+
VariableDefinitionNode,
1013
} from '../language/ast';
1114
import { Kind } from '../language/kinds';
15+
import { visit } from '../language/visitor';
1216

1317
import type { GraphQLObjectType } from '../type/definition';
1418
import { isAbstractType } from '../type/definition';
@@ -139,12 +143,18 @@ function collectFieldsImpl(
139143
) {
140144
continue;
141145
}
146+
const selectionSetWithAppliedArguments =
147+
selectionSetWithFragmentArgumentsApplied(
148+
fragment.argumentDefinitions,
149+
selection.arguments,
150+
fragment.selectionSet,
151+
);
142152
collectFieldsImpl(
143153
schema,
144154
fragments,
145155
variableValues,
146156
runtimeType,
147-
fragment.selectionSet,
157+
selectionSetWithAppliedArguments,
148158
fields,
149159
visitedFragmentNames,
150160
);
@@ -154,6 +164,41 @@ function collectFieldsImpl(
154164
}
155165
}
156166

167+
function selectionSetWithFragmentArgumentsApplied(
168+
argumentDefinitions: Readonly<Array<VariableDefinitionNode>> | undefined,
169+
fragmentArguments: Readonly<Array<ArgumentNode>> | undefined,
170+
selectionSet: SelectionSetNode,
171+
): SelectionSetNode {
172+
const providedArguments = new Map<string, ArgumentNode>();
173+
if (fragmentArguments) {
174+
for (const argument of fragmentArguments) {
175+
providedArguments.set(argument.name.value, argument);
176+
}
177+
}
178+
179+
const fragmentArgumentValues = new Map<string, ValueNode>();
180+
if (argumentDefinitions) {
181+
for (const argumentDef of argumentDefinitions) {
182+
const variableName = argumentDef.variable.name.value;
183+
const providedArg = providedArguments.get(variableName);
184+
if (providedArg) {
185+
// Not valid if the providedArg is null and argumentDef is non-null
186+
fragmentArgumentValues.set(variableName, providedArg.value);
187+
} else if (argumentDef.defaultValue) {
188+
fragmentArgumentValues.set(variableName, argumentDef.defaultValue);
189+
}
190+
// If argumentDef is non-null, expect a provided arg or non-null default value.
191+
// Otherwise just preserve the variable as-is: it will be treated as unset by the executor.
192+
}
193+
}
194+
195+
return visit(selectionSet, {
196+
Variable(node) {
197+
return fragmentArgumentValues.get(node.name.value);
198+
},
199+
});
200+
}
201+
157202
/**
158203
* Determines if a field should be included based on the `@include` and `@skip`
159204
* directives, where `@skip` has higher precedence than `@include`.

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

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -591,13 +591,16 @@ describe('Parser', () => {
591591
expect('loc' in result).to.equal(false);
592592
});
593593

594-
it('Legacy: allows parsing fragment defined variables', () => {
594+
it('allows parsing fragment defined arguments', () => {
595595
const document = 'fragment a($v: Boolean = false) on t { f(v: $v) }';
596596

597-
expect(() =>
598-
parse(document, { allowLegacyFragmentVariables: true }),
599-
).to.not.throw();
600-
expect(() => parse(document)).to.throw('Syntax Error');
597+
expect(() => parse(document)).to.not.throw();
598+
});
599+
600+
it('allows parsing fragment spread arguments', () => {
601+
const document = 'fragment a on t { ...b(v: $v) }';
602+
603+
expect(() => parse(document)).to.not.throw();
601604
});
602605

603606
it('contains location that can be Object.toStringified, JSON.stringified, or jsutils.inspected', () => {

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

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -110,34 +110,58 @@ describe('Printer: Query document', () => {
110110
`);
111111
});
112112

113-
it('Legacy: prints fragment with variable directives', () => {
114-
const queryASTWithVariableDirective = parse(
113+
it('prints fragment with argument definition directives', () => {
114+
const fragmentWithArgumentDefinitionDirective = parse(
115115
'fragment Foo($foo: TestType @test) on TestType @testDirective { id }',
116-
{ allowLegacyFragmentVariables: true },
117116
);
118-
expect(print(queryASTWithVariableDirective)).to.equal(dedent`
117+
expect(print(fragmentWithArgumentDefinitionDirective)).to.equal(dedent`
119118
fragment Foo($foo: TestType @test) on TestType @testDirective {
120119
id
121120
}
122121
`);
123122
});
124123

125-
it('Legacy: correctly prints fragment defined variables', () => {
126-
const fragmentWithVariable = parse(
124+
it('correctly prints fragment defined arguments', () => {
125+
const fragmentWithArgumentDefinition = parse(
127126
`
128127
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
129128
id
130129
}
131130
`,
132-
{ allowLegacyFragmentVariables: true },
133131
);
134-
expect(print(fragmentWithVariable)).to.equal(dedent`
132+
expect(print(fragmentWithArgumentDefinition)).to.equal(dedent`
135133
fragment Foo($a: ComplexType, $b: Boolean = false) on TestType {
136134
id
137135
}
138136
`);
139137
});
140138

139+
it('prints fragment spread with arguments', () => {
140+
const fragmentSpreadWithArguments = parse(
141+
'fragment Foo on TestType { ...Bar(a: {x: $x}, b: true) }',
142+
);
143+
expect(print(fragmentSpreadWithArguments)).to.equal(dedent`
144+
fragment Foo on TestType {
145+
...Bar(a: { x: $x }, b: true)
146+
}
147+
`);
148+
});
149+
150+
it('prints fragment spread with multi-line arguments', () => {
151+
const fragmentSpreadWithArguments = parse(
152+
'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") }',
153+
);
154+
expect(print(fragmentSpreadWithArguments)).to.equal(dedent`
155+
fragment Foo on TestType {
156+
...Bar(
157+
a: { x: $x, y: $y, z: $z, xy: $xy }
158+
b: true
159+
c: "a long string extending arguments over max length"
160+
)
161+
}
162+
`);
163+
});
164+
141165
it('prints kitchen sink without altering ast', () => {
142166
const ast = parse(kitchenSinkQuery, {
143167
noLocation: true,

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

Lines changed: 44 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,10 +455,9 @@ describe('Visitor', () => {
455455
]);
456456
});
457457

458-
it('Legacy: visits variables defined in fragments', () => {
458+
it('visits arguments defined on fragments', () => {
459459
const ast = parse('fragment a($v: Boolean = false) on t { f }', {
460460
noLocation: true,
461-
allowLegacyFragmentVariables: true,
462461
});
463462
const visited: Array<any> = [];
464463

@@ -505,6 +504,49 @@ describe('Visitor', () => {
505504
]);
506505
});
507506

507+
it('visits arguments on fragment spreads', () => {
508+
const ast = parse('fragment a on t { ...s(v: false) }', {
509+
noLocation: true,
510+
});
511+
const visited: Array<any> = [];
512+
513+
visit(ast, {
514+
enter(node) {
515+
checkVisitorFnArgs(ast, arguments);
516+
visited.push(['enter', node.kind, getValue(node)]);
517+
},
518+
leave(node) {
519+
checkVisitorFnArgs(ast, arguments);
520+
visited.push(['leave', node.kind, getValue(node)]);
521+
},
522+
});
523+
524+
expect(visited).to.deep.equal([
525+
['enter', 'Document', undefined],
526+
['enter', 'FragmentDefinition', undefined],
527+
['enter', 'Name', 'a'],
528+
['leave', 'Name', 'a'],
529+
['enter', 'NamedType', undefined],
530+
['enter', 'Name', 't'],
531+
['leave', 'Name', 't'],
532+
['leave', 'NamedType', undefined],
533+
['enter', 'SelectionSet', undefined],
534+
['enter', 'FragmentSpread', undefined],
535+
['enter', 'Name', 's'],
536+
['leave', 'Name', 's'],
537+
['enter', 'Argument', { kind: 'BooleanValue', value: false }],
538+
['enter', 'Name', 'v'],
539+
['leave', 'Name', 'v'],
540+
['enter', 'BooleanValue', false],
541+
['leave', 'BooleanValue', false],
542+
['leave', 'Argument', { kind: 'BooleanValue', value: false }],
543+
['leave', 'FragmentSpread', undefined],
544+
['leave', 'SelectionSet', undefined],
545+
['leave', 'FragmentDefinition', undefined],
546+
['leave', 'Document', undefined],
547+
]);
548+
});
549+
508550
it('n', () => {
509551
const ast = parse(kitchenSinkQuery, {
510552
experimentalClientControlledNullability: true,

0 commit comments

Comments
 (0)