Skip to content

Commit cb33ff1

Browse files
committed
Move variable position validation for OneOf fields to VariablesInAllowedPosition Rule
1 parent 7875552 commit cb33ff1

5 files changed

+60
-54
lines changed

src/validation/ValidationContext.ts

+3
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode;
3434
interface VariableUsage {
3535
readonly node: VariableNode;
3636
readonly type: Maybe<GraphQLInputType>;
37+
readonly parentType: Maybe<GraphQLInputType>;
3738
readonly defaultValue: Maybe<unknown>;
3839
readonly fragmentVariableDefinition: Maybe<VariableDefinitionNode>;
3940
}
@@ -225,13 +226,15 @@ export class ValidationContext extends ASTValidationContext {
225226
newUsages.push({
226227
node: variable,
227228
type: typeInfo.getInputType(),
229+
parentType: typeInfo.getParentInputType(),
228230
defaultValue: undefined, // fragment variables have a variable default but no location default, which is what this default value represents
229231
fragmentVariableDefinition,
230232
});
231233
} else {
232234
newUsages.push({
233235
node: variable,
234236
type: typeInfo.getInputType(),
237+
parentType: typeInfo.getParentInputType(),
235238
defaultValue: typeInfo.getDefaultValue(),
236239
fragmentVariableDefinition: undefined,
237240
});

src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts

-16
Original file line numberDiff line numberDiff line change
@@ -1094,22 +1094,6 @@ describe('Validate: Values of correct type', () => {
10941094
]);
10951095
});
10961096

1097-
it('Exactly one nullable variable', () => {
1098-
expectErrors(`
1099-
query ($string: String) {
1100-
complicatedArgs {
1101-
oneOfArgField(oneOfArg: { stringField: $string })
1102-
}
1103-
}
1104-
`).toDeepEqual([
1105-
{
1106-
message:
1107-
'Variable "$string" must be non-nullable to be used for OneOf Input Object "OneOfInput".',
1108-
locations: [{ line: 4, column: 37 }],
1109-
},
1110-
]);
1111-
});
1112-
11131097
it('More than one field', () => {
11141098
expectErrors(`
11151099
{

src/validation/__tests__/VariablesInAllowedPositionRule-test.ts

+31
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,37 @@ describe('Validate: Variables are in allowed positions', () => {
365365
});
366366
});
367367

368+
describe('Validates OneOf Input Objects', () => {
369+
it('Allows exactly one non-nullable variable', () => {
370+
expectValid(`
371+
query ($string: String!) {
372+
complicatedArgs {
373+
oneOfArgField(oneOfArg: { stringField: $string })
374+
}
375+
}
376+
`);
377+
});
378+
379+
it('Forbids one nullable variable', () => {
380+
expectErrors(`
381+
query ($string: String) {
382+
complicatedArgs {
383+
oneOfArgField(oneOfArg: { stringField: $string })
384+
}
385+
}
386+
`).toDeepEqual([
387+
{
388+
message:
389+
'Variable "$string" is of type "String" but must be non-nullable to be used for OneOf Input Object "OneOfInput".',
390+
locations: [
391+
{ line: 2, column: 16 },
392+
{ line: 4, column: 52 },
393+
],
394+
},
395+
]);
396+
});
397+
});
398+
368399
describe('Fragment arguments are validated', () => {
369400
it('Boolean => Boolean', () => {
370401
expectValid(`

src/validation/rules/ValuesOfCorrectTypeRule.ts

+1-36
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import type {
88
ObjectFieldNode,
99
ObjectValueNode,
1010
ValueNode,
11-
VariableDefinitionNode,
1211
} from '../../language/ast.js';
1312
import { Kind } from '../../language/kinds.js';
1413
import { print } from '../../language/printer.js';
@@ -38,17 +37,7 @@ import type { ValidationContext } from '../ValidationContext.js';
3837
export function ValuesOfCorrectTypeRule(
3938
context: ValidationContext,
4039
): ASTVisitor {
41-
let variableDefinitions: { [key: string]: VariableDefinitionNode } = {};
42-
4340
return {
44-
OperationDefinition: {
45-
enter() {
46-
variableDefinitions = {};
47-
},
48-
},
49-
VariableDefinition(definition) {
50-
variableDefinitions[definition.variable.name.value] = definition;
51-
},
5241
ListValue(node) {
5342
// Note: TypeInfo will traverse into a list's item type, so look to the
5443
// parent input type to check if it is a list.
@@ -82,13 +71,7 @@ export function ValuesOfCorrectTypeRule(
8271
}
8372

8473
if (type.isOneOf) {
85-
validateOneOfInputObject(
86-
context,
87-
node,
88-
type,
89-
fieldNodeMap,
90-
variableDefinitions,
91-
);
74+
validateOneOfInputObject(context, node, type, fieldNodeMap);
9275
}
9376
},
9477
ObjectField(node) {
@@ -185,7 +168,6 @@ function validateOneOfInputObject(
185168
node: ObjectValueNode,
186169
type: GraphQLInputObjectType,
187170
fieldNodeMap: Map<string, ObjectFieldNode>,
188-
variableDefinitions: { [key: string]: VariableDefinitionNode },
189171
): void {
190172
const keys = Array.from(fieldNodeMap.keys());
191173
const isNotExactlyOneField = keys.length !== 1;
@@ -202,29 +184,12 @@ function validateOneOfInputObject(
202184

203185
const value = fieldNodeMap.get(keys[0])?.value;
204186
const isNullLiteral = !value || value.kind === Kind.NULL;
205-
const isVariable = value?.kind === Kind.VARIABLE;
206187

207188
if (isNullLiteral) {
208189
context.reportError(
209190
new GraphQLError(`Field "${type}.${keys[0]}" must be non-null.`, {
210191
nodes: [node],
211192
}),
212193
);
213-
return;
214-
}
215-
216-
if (isVariable) {
217-
const variableName = value.name.value;
218-
const definition = variableDefinitions[variableName];
219-
const isNullableVariable = definition.type.kind !== Kind.NON_NULL_TYPE;
220-
221-
if (isNullableVariable) {
222-
context.reportError(
223-
new GraphQLError(
224-
`Variable "$${variableName}" must be non-nullable to be used for OneOf Input Object "${type}".`,
225-
{ nodes: [node] },
226-
),
227-
);
228-
}
229194
}
230195
}

src/validation/rules/VariablesInAllowedPositionRule.ts

+25-2
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,11 @@ import { Kind } from '../../language/kinds.js';
88
import type { ASTVisitor } from '../../language/visitor.js';
99

1010
import type { GraphQLType } from '../../type/definition.js';
11-
import { isNonNullType } from '../../type/definition.js';
11+
import {
12+
isInputObjectType,
13+
isNonNullType,
14+
isNullableType,
15+
} from '../../type/definition.js';
1216
import type { GraphQLSchema } from '../../type/schema.js';
1317

1418
import { isTypeSubTypeOf } from '../../utilities/typeComparators.js';
@@ -39,6 +43,7 @@ export function VariablesInAllowedPositionRule(
3943
for (const {
4044
node,
4145
type,
46+
parentType,
4247
defaultValue,
4348
fragmentVariableDefinition,
4449
} of usages) {
@@ -75,6 +80,21 @@ export function VariablesInAllowedPositionRule(
7580
),
7681
);
7782
}
83+
84+
if (
85+
isInputObjectType(parentType) &&
86+
parentType.isOneOf &&
87+
isNullableType(varType)
88+
) {
89+
const varTypeStr = inspect(varType);
90+
const parentTypeStr = inspect(parentType);
91+
context.reportError(
92+
new GraphQLError(
93+
`Variable "$${varName}" is of type "${varTypeStr}" but must be non-nullable to be used for OneOf Input Object "${parentTypeStr}".`,
94+
{ nodes: [varDef, node] },
95+
),
96+
);
97+
}
7898
}
7999
}
80100
},
@@ -87,8 +107,11 @@ export function VariablesInAllowedPositionRule(
87107

88108
/**
89109
* Returns true if the variable is allowed in the location it was found,
90-
* which includes considering if default values exist for either the variable
110+
* including considering if default values exist for either the variable
91111
* or the location at which it is located.
112+
*
113+
* OneOf Input Object Type fields are considered separately above to
114+
* provide a more descriptive error message.
92115
*/
93116
function allowedVariableUsage(
94117
schema: GraphQLSchema,

0 commit comments

Comments
 (0)