Skip to content

Commit b4bddc7

Browse files
mjmahonemjmahone
committed
Updates to get to 100% code coverage, plus simplify visit so it works even with nested variable inputs
1 parent 8e2db7b commit b4bddc7

File tree

3 files changed

+136
-31
lines changed

3 files changed

+136
-31
lines changed

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

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1014,6 +1014,23 @@ describe('Execute: Handles inputs', () => {
10141014
});
10151015

10161016
describe('using fragment arguments', () => {
1017+
it('when there are no fragment arguments', () => {
1018+
const result = executeQuery(`
1019+
query {
1020+
...a
1021+
}
1022+
1023+
fragment a on TestType {
1024+
fieldWithNonNullableStringInput(input: "A")
1025+
}
1026+
`);
1027+
expect(result).to.deep.equal({
1028+
data: {
1029+
fieldWithNonNullableStringInput: '"A"',
1030+
},
1031+
});
1032+
});
1033+
10171034
it('when a value is required and provided', () => {
10181035
const result = executeQueryWithFragmentArguments(`
10191036
query {
@@ -1031,6 +1048,33 @@ describe('Execute: Handles inputs', () => {
10311048
});
10321049
});
10331050

1051+
it('when a value is required and not provided', () => {
1052+
const result = executeQueryWithFragmentArguments(`
1053+
query {
1054+
...a
1055+
}
1056+
1057+
fragment a($value: String!) on TestType {
1058+
fieldWithNullableStringInput(input: $value)
1059+
}
1060+
`);
1061+
expect(result).to.deep.equal({
1062+
data: {
1063+
fieldWithNullableStringInput: null,
1064+
},
1065+
errors: [
1066+
{
1067+
message:
1068+
'Fragment argument "$value" on fragment "a" is required but not provided.',
1069+
locations: [
1070+
{ line: 3, column: 11 },
1071+
{ line: 6, column: 20 },
1072+
],
1073+
},
1074+
],
1075+
});
1076+
});
1077+
10341078
it('when the definition has a default and is provided', () => {
10351079
const result = executeQueryWithFragmentArguments(`
10361080
query {
@@ -1065,6 +1109,33 @@ describe('Execute: Handles inputs', () => {
10651109
});
10661110
});
10671111

1112+
it('when the definition has a non-nullable default and is provided null', () => {
1113+
const result = executeQueryWithFragmentArguments(`
1114+
query {
1115+
...a(value: null)
1116+
}
1117+
1118+
fragment a($value: String! = "B") on TestType {
1119+
fieldWithNullableStringInput(input: $value)
1120+
}
1121+
`);
1122+
expect(result).to.deep.equal({
1123+
data: {
1124+
fieldWithNullableStringInput: 'null',
1125+
},
1126+
errors: [
1127+
{
1128+
message:
1129+
'Fragment argument "$value" on fragment "a" is non-null, but null was provided.',
1130+
locations: [
1131+
{ line: 3, column: 16 },
1132+
{ line: 6, column: 20 },
1133+
],
1134+
},
1135+
],
1136+
});
1137+
});
1138+
10681139
it('when the definition has no default and is not provided', () => {
10691140
const result = executeQueryWithFragmentArguments(`
10701141
query {
@@ -1082,6 +1153,44 @@ describe('Execute: Handles inputs', () => {
10821153
},
10831154
});
10841155
});
1156+
1157+
it('when the argument variable is nested in a complex type', () => {
1158+
const result = executeQueryWithFragmentArguments(`
1159+
query {
1160+
...a(value: "C")
1161+
}
1162+
1163+
fragment a($value: String) on TestType {
1164+
list(input: ["A", "B", $value, "D"])
1165+
}
1166+
`);
1167+
expect(result).to.deep.equal({
1168+
data: {
1169+
list: '["A", "B", "C", "D"]',
1170+
},
1171+
});
1172+
});
1173+
1174+
it('when argument variables are used recursively', () => {
1175+
const result = executeQueryWithFragmentArguments(`
1176+
query {
1177+
...a(aValue: "C")
1178+
}
1179+
1180+
fragment a($aValue: String) on TestType {
1181+
...b(bValue: $aValue)
1182+
}
1183+
1184+
fragment b($bValue: String) on TestType {
1185+
list(input: ["A", "B", $bValue, "D"])
1186+
}
1187+
`);
1188+
expect(result).to.deep.equal({
1189+
data: {
1190+
list: '["A", "B", "C", "D"]',
1191+
},
1192+
});
1193+
});
10851194
});
10861195

10871196
describe('getVariableValues: limit maximum number of coercion errors', () => {

‎src/execution/execute.ts

Lines changed: 20 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -538,20 +538,24 @@ function selectionSetWithFragmentArgumentsApplied(
538538
fragment: FragmentDefinitionNode,
539539
fragmentSpread: FragmentSpreadNode,
540540
): SelectionSetNode {
541-
if (fragment.variableDefinitions == null) {
541+
const variableDefinitions = fragment.variableDefinitions;
542+
if (variableDefinitions == null) {
542543
return fragment.selectionSet;
543544
}
544545

545546
const providedArguments: Map<string, ArgumentNode> = new Map();
546547
for (const arg of fragmentSpread.arguments ?? []) {
547548
providedArguments.set(arg.name.value, arg);
548549
}
549-
const fragmentArgumentValues: Map<string, ValueNode | null> = new Map();
550-
for (const argDef of fragment.variableDefinitions ?? []) {
550+
const fragmentArgumentValues: Map<string, ValueNode> = new Map();
551+
for (const argDef of variableDefinitions) {
551552
const variableName = argDef.variable.name.value;
552553
const providedArg = providedArguments.get(variableName);
553554
if (providedArg != null) {
554-
if (providedArg.value.kind === Kind.NULL && isNonNullType(argDef.type)) {
555+
if (
556+
providedArg.value.kind === Kind.NULL &&
557+
argDef.type.kind === Kind.NON_NULL_TYPE
558+
) {
555559
exeContext.errors.push(
556560
new GraphQLError(
557561
`Fragment argument "$${variableName}" on fragment "${fragment.name.value}" is non-null, but null was provided.`,
@@ -562,34 +566,22 @@ function selectionSetWithFragmentArgumentsApplied(
562566
fragmentArgumentValues.set(variableName, providedArg.value);
563567
} else if (argDef.defaultValue != null) {
564568
fragmentArgumentValues.set(variableName, argDef.defaultValue);
565-
} else {
566-
if (isNonNullType(argDef.type)) {
567-
exeContext.errors.push(
568-
new GraphQLError(
569-
`Fragment argument "$${variableName}" on fragment "${fragment.name.value}" is required but not provided.`,
570-
[fragmentSpread, argDef],
571-
),
572-
);
573-
}
574-
fragmentArgumentValues.set(variableName, null);
569+
} else if (argDef.type.kind === Kind.NON_NULL_TYPE) {
570+
exeContext.errors.push(
571+
new GraphQLError(
572+
`Fragment argument "$${variableName}" on fragment "${fragment.name.value}" is required but not provided.`,
573+
[fragmentSpread, argDef],
574+
),
575+
);
575576
}
577+
// Otherwise just preserve the variable as-is: it will be treated as unset by the executor.
576578
}
577579

578580
return visit(fragment.selectionSet, {
579-
Argument(node) {
580-
if (node.value.kind === Kind.VARIABLE) {
581-
const variable = node.value;
582-
if (fragmentArgumentValues.has(variable.name.value)) {
583-
const replacementValue = fragmentArgumentValues.get(
584-
variable.name.value,
585-
);
586-
if (replacementValue == null) {
587-
// Remove the argument if the provided value is unset.
588-
return null;
589-
}
590-
591-
return { ...node, value: replacementValue };
592-
}
581+
Variable(node) {
582+
const replacementValue = fragmentArgumentValues.get(node.name.value);
583+
if (replacementValue != null) {
584+
return replacementValue;
593585
}
594586
},
595587
});

‎src/language/parser.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -449,17 +449,21 @@ export class Parser {
449449

450450
const hasTypeCondition = this.expectOptionalKeyword('on');
451451
if (!hasTypeCondition && this.peek(TokenKind.NAME)) {
452-
if (this._options?.allowFragmentArguments === true) {
452+
const name = this.parseFragmentName();
453+
if (
454+
this._options?.allowFragmentArguments === true &&
455+
this.peek(TokenKind.PAREN_L)
456+
) {
453457
return this.node<FragmentSpreadNode>(start, {
454458
kind: Kind.FRAGMENT_SPREAD,
455-
name: this.parseFragmentName(),
459+
name,
456460
arguments: this.parseArguments(false),
457461
directives: this.parseDirectives(false),
458462
});
459463
}
460464
return this.node<FragmentSpreadNode>(start, {
461465
kind: Kind.FRAGMENT_SPREAD,
462-
name: this.parseFragmentName(),
466+
name,
463467
directives: this.parseDirectives(false),
464468
});
465469
}

0 commit comments

Comments
 (0)