diff --git a/src/utilities/TypeInfo.ts b/src/utilities/TypeInfo.ts index a99580ab95..54281101d4 100644 --- a/src/utilities/TypeInfo.ts +++ b/src/utilities/TypeInfo.ts @@ -69,8 +69,6 @@ export class TypeInfo { /** @deprecated will be removed in 17.0.0 */ getFieldDefFn?: GetFieldDefFn, ) { - this._fragmentDefinitions = Object.create(null); - this._schema = schema; this._typeStack = []; this._parentTypeStack = []; @@ -81,6 +79,7 @@ export class TypeInfo { this._argument = null; this._enumValue = null; this._fragmentDefinition = null; + this._fragmentDefinitions = Object.create(null); this._getFieldDef = getFieldDefFn ?? getFieldDef; if (initialType) { if (isInputType(initialType)) { diff --git a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts index 6f0d223c15..66545ddb59 100644 --- a/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts +++ b/src/validation/__tests__/ProvidedRequiredArgumentsRule-test.ts @@ -356,4 +356,112 @@ describe('Validate: Provided required arguments', () => { ]); }); }); + + describe('Fragment required arguments', () => { + it('ignores unknown arguments', () => { + expectValid(` + { + ...Foo(unknownArgument: true) + } + fragment Foo on Query { + dog + } + `); + }); + + // Query: should this be allowed? + // We could differentiate between required/optional (i.e. no default value) + // vs. nullable/non-nullable (i.e. no !), whereas now they are conflated. + // So today: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (MAY BE a nullable variable) + // $x: Int `x:` is not required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // It feels weird to collapse the nullable cases but not the non-nullable ones. + // Whereas all four feel like they ought to mean something explicitly different. + // + // Potential proposal: + // $x: Int! `x:` is required and must not be null (NOT a nullable variable) + // $x: Int! = 3 `x:` is not required and must not be null (NOT a nullable variable) + // $x: Int `x:` is required and may be null + // $x: Int = 3 `x:` is not required and may be null + // + // Required then is whether there's a default value, + // and nullable is whether there's a ! + it('Missing nullable argument with default is allowed', () => { + expectValid(` + { + ...F + + } + fragment F($x: Int = 3) on Query { + foo + } + `); + }); + // Above proposal: this should be an error + it('Missing nullable argument is allowed', () => { + expectValid(` + { + ...F + + } + fragment F($x: Int) on Query { + foo + } + `); + }); + it('Missing non-nullable argument with default is allowed', () => { + expectValid(` + { + ...F + + } + fragment F($x: Int! = 3) on Query { + foo + } + `); + }); + it('Missing non-nullable argument is not allowed', () => { + expectErrors(` + { + ...F + + } + fragment F($x: Int!) on Query { + foo + } + `).toDeepEqual([ + { + message: + 'Fragment "F" argument "x" of type "{ kind: "NonNullType", type: { kind: "NamedType", name: [Object], loc: [Object] }, loc: [Object] }" is required, but it was not provided.', + locations: [ + { line: 3, column: 13 }, + { line: 6, column: 22 }, + ], + }, + ]); + }); + + it('Supplies required variables', () => { + expectValid(` + { + ...F(x: 3) + + } + fragment F($x: Int!) on Query { + foo + } + `); + }); + + it('Skips missing fragments', () => { + expectValid(` + { + ...Missing(x: 3) + } + `); + }); + }); }); diff --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts index f0b7dfa57e..d843139d89 100644 --- a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts +++ b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts @@ -1198,4 +1198,35 @@ describe('Validate: Values of correct type', () => { ]); }); }); + + describe('Fragment argument values', () => { + it('list variables with invalid item', () => { + expectErrors(` + fragment InvalidItem($a: [String] = ["one", 2]) on Query { + dog { name } + } + `).toDeepEqual([ + { + message: 'String cannot represent a non string value: 2', + locations: [{ line: 2, column: 53 }], + }, + ]); + }); + + it('fragment spread with invalid argument value', () => { + expectErrors(` + fragment GivesString on Query { + ...ExpectsInt(a: "three") + } + fragment ExpectsInt($a: Int) on Query { + dog { name } + } + `).toDeepEqual([ + { + message: 'Int cannot represent non-integer value: "three"', + locations: [{ line: 3, column: 28 }], + }, + ]); + }); + }); }); diff --git a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts index 8027a35826..eddd202df8 100644 --- a/src/validation/__tests__/VariablesAreInputTypesRule-test.ts +++ b/src/validation/__tests__/VariablesAreInputTypesRule-test.ts @@ -18,6 +18,9 @@ describe('Validate: Variables are input types', () => { query Foo($a: Unknown, $b: [[Unknown!]]!) { field(a: $a, b: $b) } + fragment Bar($a: Unknown, $b: [[Unknown!]]!) on Query { + field(a: $a, b: $b) + } `); }); @@ -26,6 +29,9 @@ describe('Validate: Variables are input types', () => { query Foo($a: String, $b: [Boolean!]!, $c: ComplexInput) { field(a: $a, b: $b, c: $c) } + fragment Bar($a: String, $b: [Boolean!]!, $c: ComplexInput) on Query { + field(a: $a, b: $b, c: $c) + } `); }); @@ -49,4 +55,25 @@ describe('Validate: Variables are input types', () => { }, ]); }); + + it('output types on fragment arguments are invalid', () => { + expectErrors(` + fragment Bar($a: Dog, $b: [[CatOrDog!]]!, $c: Pet) on Query { + field(a: $a, b: $b, c: $c) + } + `).toDeepEqual([ + { + locations: [{ line: 2, column: 24 }], + message: 'Variable "$a" cannot be non-input type "Dog".', + }, + { + locations: [{ line: 2, column: 33 }], + message: 'Variable "$b" cannot be non-input type "[[CatOrDog!]]!".', + }, + { + locations: [{ line: 2, column: 53 }], + message: 'Variable "$c" cannot be non-input type "Pet".', + }, + ]); + }); }); diff --git a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts index 140ff2cfe8..d0da8f5305 100644 --- a/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts +++ b/src/validation/__tests__/VariablesInAllowedPositionRule-test.ts @@ -438,5 +438,27 @@ describe('Validate: Variables are in allowed positions', () => { }, ]); }); + + it('Int fragment arg => Int! field arg fails even when shadowed by Int! variable', () => { + expectErrors(` + query Query($intVar: Int!) { + complicatedArgs { + ...A(i: $intVar) + } + } + fragment A($intVar: Int) on ComplicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + `).toDeepEqual([ + { + message: + 'Variable "$intVar" of type "Int" used in position expecting type "Int!".', + locations: [ + { line: 7, column: 20 }, + { line: 8, column: 45 }, + ], + }, + ]); + }); }); }); diff --git a/src/validation/rules/ProvidedRequiredArgumentsRule.ts b/src/validation/rules/ProvidedRequiredArgumentsRule.ts index 350264496f..ab1093adb1 100644 --- a/src/validation/rules/ProvidedRequiredArgumentsRule.ts +++ b/src/validation/rules/ProvidedRequiredArgumentsRule.ts @@ -4,7 +4,10 @@ import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { InputValueDefinitionNode } from '../../language/ast.js'; +import type { + InputValueDefinitionNode, + VariableDefinitionNode, +} from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import { print } from '../../language/printer.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -56,6 +59,37 @@ export function ProvidedRequiredArgumentsRule( } }, }, + FragmentSpread: { + // Validate on leave to allow for directive errors to appear first. + leave(spreadNode) { + const fragmentDef = context.getFragment(spreadNode.name.value); + if (!fragmentDef) { + return false; + } + + const providedArgs = new Set( + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + spreadNode.arguments?.map((arg) => arg.name.value), + ); + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + for (const argDef of fragmentDef.argumentDefinitions ?? []) { + if ( + !providedArgs.has(argDef.variable.name.value) && + isRequiredArgumentNode(argDef) + ) { + const argTypeStr = inspect(argDef.type); + context.reportError( + new GraphQLError( + `Fragment "${spreadNode.name.value}" argument "${argDef.variable.name.value}" of type "${argTypeStr}" is required, but it was not provided.`, + { nodes: [spreadNode, argDef] }, + ), + ); + } + } + }, + }, }; } @@ -122,6 +156,8 @@ export function ProvidedRequiredArgumentsOnDirectivesRule( }; } -function isRequiredArgumentNode(arg: InputValueDefinitionNode): boolean { +function isRequiredArgumentNode( + arg: InputValueDefinitionNode | VariableDefinitionNode, +): boolean { return arg.type.kind === Kind.NON_NULL_TYPE && arg.defaultValue == null; } diff --git a/src/validation/rules/UniqueVariableNamesRule.ts b/src/validation/rules/UniqueVariableNamesRule.ts index 8c067263fd..3960f1582c 100644 --- a/src/validation/rules/UniqueVariableNamesRule.ts +++ b/src/validation/rules/UniqueVariableNamesRule.ts @@ -1,7 +1,9 @@ import { groupBy } from '../../jsutils/groupBy.js'; +import type { Maybe } from '../../jsutils/Maybe.js'; import { GraphQLError } from '../../error/GraphQLError.js'; +import type { VariableDefinitionNode } from '../../language/ast.js'; import type { ASTVisitor } from '../../language/visitor.js'; import type { ASTValidationContext } from '../ValidationContext.js'; @@ -16,25 +18,34 @@ export function UniqueVariableNamesRule( ): ASTVisitor { return { OperationDefinition(operationNode) { - // See: https://github.com/graphql/graphql-js/issues/2203 - /* c8 ignore next */ - const variableDefinitions = operationNode.variableDefinitions ?? []; - - const seenVariableDefinitions = groupBy( - variableDefinitions, - (node) => node.variable.name.value, - ); - - for (const [variableName, variableNodes] of seenVariableDefinitions) { - if (variableNodes.length > 1) { - context.reportError( - new GraphQLError( - `There can be only one variable named "$${variableName}".`, - { nodes: variableNodes.map((node) => node.variable.name) }, - ), - ); - } - } + validateVariableDefinitions(operationNode.variableDefinitions, context); + }, + FragmentDefinition(fragmentNode) { + validateVariableDefinitions(fragmentNode.argumentDefinitions, context); }, }; } + +function validateVariableDefinitions( + maybeVariableDefinitions: Maybe>, + context: ASTValidationContext, +) { + // See: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const variableDefinitions = maybeVariableDefinitions ?? []; + const seenVariableDefinitions = groupBy( + variableDefinitions, + (node) => node.variable.name.value, + ); + + for (const [variableName, variableNodes] of seenVariableDefinitions) { + if (variableNodes.length > 1) { + context.reportError( + new GraphQLError( + `There can be only one variable named "$${variableName}".`, + { nodes: variableNodes.map((node) => node.variable.name) }, + ), + ); + } + } +} diff --git a/src/validation/rules/VariablesInAllowedPositionRule.ts b/src/validation/rules/VariablesInAllowedPositionRule.ts index a2c57fa4b0..d525000a31 100644 --- a/src/validation/rules/VariablesInAllowedPositionRule.ts +++ b/src/validation/rules/VariablesInAllowedPositionRule.ts @@ -1,9 +1,14 @@ import { inspect } from '../../jsutils/inspect.js'; import type { Maybe } from '../../jsutils/Maybe.js'; +import type { ObjMap } from '../../jsutils/ObjMap.js'; import { GraphQLError } from '../../error/GraphQLError.js'; -import type { ValueNode } from '../../language/ast.js'; +import type { + ValueNode, + VariableDefinitionNode, + VariableNode, +} from '../../language/ast.js'; import { Kind } from '../../language/kinds.js'; import type { ASTVisitor } from '../../language/visitor.js'; @@ -26,40 +31,69 @@ import type { ValidationContext } from '../ValidationContext.js'; export function VariablesInAllowedPositionRule( context: ValidationContext, ): ASTVisitor { + let localArgumentDefinitions: ObjMap = + Object.create(null); return { + FragmentDefinition: { + enter(fragment) { + // FIXME: https://github.com/graphql/graphql-js/issues/2203 + /* c8 ignore next */ + const argDefs = fragment.argumentDefinitions ?? []; + for (const argDef of argDefs) { + localArgumentDefinitions[argDef.variable.name.value] = argDef; + } + }, + leave() { + localArgumentDefinitions = Object.create(null); + }, + }, Variable(node) { + const fragmentArgDef = localArgumentDefinitions[node.name.value]; + if (fragmentArgDef) { + validateVariableWithDefinition(node, fragmentArgDef, context); + return; + } + const varDefs = context.getVariableDefinitions(node); - const type = context.getInputType(); - const defaultValue = context.getDefaultValue(); - const varName = node.name.value; for (const { variableDefinition } of varDefs) { - const schema = context.getSchema(); - const varType = typeFromAST(schema, variableDefinition.type); - if ( - type && - varType && - !allowedVariableUsage( - schema, - varType, - variableDefinition.defaultValue, - type, - defaultValue, - ) - ) { - const varTypeStr = inspect(varType); - const typeStr = inspect(type); - context.reportError( - new GraphQLError( - `Variable "$${varName}" of type "${varTypeStr}" used in position expecting type "${typeStr}".`, - { nodes: [variableDefinition, node] }, - ), - ); - } + validateVariableWithDefinition(node, variableDefinition, context); } }, }; } +function validateVariableWithDefinition( + locationVariable: VariableNode, + variableDefinition: VariableDefinitionNode, + context: ValidationContext, +) { + const type = context.getInputType(); + const defaultValue = context.getDefaultValue(); + const varName = locationVariable.name.value; + const schema = context.getSchema(); + const varType = typeFromAST(schema, variableDefinition.type); + if ( + type && + varType && + !allowedVariableUsage( + schema, + varType, + variableDefinition.defaultValue, + type, + defaultValue, + ) + ) { + const varTypeStr = inspect(varType); + const typeStr = inspect(type); + context.reportError( + new GraphQLError( + `Variable "$${varName}" of type "${varTypeStr}" used in position expecting type "${typeStr}".`, + { nodes: [variableDefinition, locationVariable] }, + ), + ); + } +} + /** * Returns true if the variable is allowed in the location it was found, * which includes considering if default values exist for either the variable