diff --git a/src/execution/__tests__/nonnull-test.js b/src/execution/__tests__/nonnull-test.js index 73b120be8d..58aef906b0 100644 --- a/src/execution/__tests__/nonnull-test.js +++ b/src/execution/__tests__/nonnull-test.js @@ -486,4 +486,199 @@ describe('Execute: handles non-nullable types', () => { ], }, ); + + describe('Handles non-null argument', () => { + const schemaWithNonNullArg = new GraphQLSchema({ + query: new GraphQLObjectType({ + name: 'Query', + fields: { + withNonNullArg: { + type: GraphQLString, + args: { + cannotBeNull: { + type: GraphQLNonNull(GraphQLString), + }, + }, + resolve: async (_, args) => { + if (typeof args.cannotBeNull === 'string') { + return 'Passed: ' + args.cannotBeNull; + } + }, + }, + }, + }), + }); + + it('succeeds when passed non-null literal value', async () => { + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query { + withNonNullArg (cannotBeNull: "literal value") + } + `), + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: 'Passed: literal value', + }, + }); + }); + + it('succeeds when passed non-null variable value', async () => { + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query ($testVar: String!) { + withNonNullArg (cannotBeNull: $testVar) + } + `), + variableValues: { + testVar: 'variable value', + }, + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: 'Passed: variable value', + }, + }); + }); + + it('succeeds when missing variable has default value', async () => { + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query ($testVar: String = "default value") { + withNonNullArg (cannotBeNull: $testVar) + } + `), + variableValues: { + // Intentionally missing variable + }, + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: 'Passed: default value', + }, + }); + }); + + it('field error when missing non-null arg', async () => { + // Note: validation should identify this issue first (missing args rule) + // however execution should still protect against this. + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query { + withNonNullArg + } + `), + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: null, + }, + errors: [ + { + message: + 'Argument "cannotBeNull" of required type "String!" was not provided.', + locations: [{ line: 3, column: 13 }], + path: ['withNonNullArg'], + }, + ], + }); + }); + + it('field error when non-null arg provided null', async () => { + // Note: validation should identify this issue first (values of correct + // type rule) however execution should still protect against this. + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query { + withNonNullArg(cannotBeNull: null) + } + `), + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: null, + }, + errors: [ + { + message: + 'Argument "cannotBeNull" of non-null type "String!" must ' + + 'not be null.', + locations: [{ line: 3, column: 42 }], + path: ['withNonNullArg'], + }, + ], + }); + }); + + it('field error when non-null arg not provided variable value', async () => { + // Note: validation should identify this issue first (variables in allowed + // position rule) however execution should still protect against this. + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query ($testVar: String) { + withNonNullArg(cannotBeNull: $testVar) + } + `), + variableValues: { + // Intentionally missing variable + }, + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: null, + }, + errors: [ + { + message: + 'Argument "cannotBeNull" of required type "String!" was ' + + 'provided the variable "$testVar" which was not provided a ' + + 'runtime value.', + locations: [{ line: 3, column: 42 }], + path: ['withNonNullArg'], + }, + ], + }); + }); + + it('field error when non-null arg provided variable with explicit null value', async () => { + const result = await execute({ + schema: schemaWithNonNullArg, + document: parse(` + query ($testVar: String = "default value") { + withNonNullArg (cannotBeNull: $testVar) + } + `), + variableValues: { + testVar: null, + }, + }); + + expect(result).to.deep.equal({ + data: { + withNonNullArg: null, + }, + errors: [ + { + message: + 'Argument "cannotBeNull" of non-null type "String!" must not be null.', + locations: [{ line: 3, column: 43 }], + path: ['withNonNullArg'], + }, + ], + }); + }); + }); }); diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js index 30850c447b..49596c5bfb 100644 --- a/src/execution/__tests__/variables-test.js +++ b/src/execution/__tests__/variables-test.js @@ -84,6 +84,10 @@ const TestType = new GraphQLObjectType({ type: GraphQLString, defaultValue: 'Hello World', }), + fieldWithNonNullableStringInputAndDefaultArgumentValue: fieldWithInputArg({ + type: GraphQLNonNull(GraphQLString), + defaultValue: 'Hello World', + }), fieldWithNestedInputObject: fieldWithInputArg({ type: TestNestedInputObject, defaultValue: 'Hello World', @@ -222,6 +226,40 @@ describe('Execute: Handles inputs', () => { }); }); + it('uses undefined when variable not provided', () => { + const result = executeQuery( + ` + query q($input: String) { + fieldWithNullableStringInput(input: $input) + }`, + { + // Intentionally missing variable values. + }, + ); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + + it('uses null when variable provided explicit null value', () => { + const result = executeQuery( + ` + query q($input: String) { + fieldWithNullableStringInput(input: $input) + }`, + { input: null }, + ); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: 'null', + }, + }); + }); + it('uses default value when not provided', () => { const result = executeQuery(` query ($input: TestInputObject = {a: "foo", b: ["bar"], c: "baz"}) { @@ -236,6 +274,55 @@ describe('Execute: Handles inputs', () => { }); }); + it('does not use default value when provided', () => { + const result = executeQuery( + `query q($input: String = "Default value") { + fieldWithNullableStringInput(input: $input) + }`, + { input: 'Variable value' }, + ); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: "'Variable value'", + }, + }); + }); + + it('uses explicit null value instead of default value', () => { + const result = executeQuery( + ` + query q($input: String = "Default value") { + fieldWithNullableStringInput(input: $input) + }`, + { input: null }, + ); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: 'null', + }, + }); + }); + + it('uses null default value when not provided', () => { + const result = executeQuery( + ` + query q($input: String = null) { + fieldWithNullableStringInput(input: $input) + }`, + { + // Intentionally missing variable values. + }, + ); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: 'null', + }, + }); + }); + it('properly parses single value to list', () => { const params = { input: { a: 'foo', b: 'bar', c: 'baz' } }; const result = executeQuery(doc, params); @@ -485,8 +572,7 @@ describe('Execute: Handles inputs', () => { errors: [ { message: - 'Variable "$value" got invalid value null; ' + - 'Expected non-nullable type String! not to be null.', + 'Variable "$value" of non-null type "String!" must not be null.', locations: [{ line: 2, column: 16 }], }, ], @@ -644,8 +730,7 @@ describe('Execute: Handles inputs', () => { errors: [ { message: - 'Variable "$input" got invalid value null; ' + - 'Expected non-nullable type [String]! not to be null.', + 'Variable "$input" of non-null type "[String]!" must not be null.', locations: [{ line: 2, column: 16 }], }, ], @@ -728,8 +813,7 @@ describe('Execute: Handles inputs', () => { errors: [ { message: - 'Variable "$input" got invalid value null; ' + - 'Expected non-nullable type [String!]! not to be null.', + 'Variable "$input" of non-null type "[String!]!" must not be null.', locations: [{ line: 2, column: 16 }], }, ], @@ -853,5 +937,20 @@ describe('Execute: Handles inputs', () => { ], }); }); + + it('when no runtime value is provided to a non-null argument', () => { + const result = executeQuery(` + query optionalVariable($optional: String) { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional) + } + `); + + expect(result).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: + "'Hello World'", + }, + }); + }); }); }); diff --git a/src/execution/values.js b/src/execution/values.js index ab9834e6de..5690bbe366 100644 --- a/src/execution/values.js +++ b/src/execution/values.js @@ -9,7 +9,7 @@ import { GraphQLError } from '../error'; import find from '../jsutils/find'; -import isInvalid from '../jsutils/isInvalid'; +import invariant from '../jsutils/invariant'; import keyMap from '../jsutils/keyMap'; import { coerceValue } from '../utilities/coerceValue'; import { typeFromAST } from '../utilities/typeFromAST'; @@ -53,6 +53,8 @@ export function getVariableValues( const varName = varDefNode.variable.name.value; const varType = typeFromAST(schema, varDefNode.type); if (!isInputType(varType)) { + // Must use input types for variables. This should be caught during + // validation, however is checked again here for safety. errors.push( new GraphQLError( `Variable "$${varName}" expected value of type ` + @@ -63,35 +65,45 @@ export function getVariableValues( ), ); } else { - const value = inputs[varName]; - if (isInvalid(value)) { - if (isNonNullType(varType)) { - errors.push( - new GraphQLError( - `Variable "$${varName}" of required type ` + + const hasValue = hasOwnProperty(inputs, varName); + const value = hasValue ? inputs[varName] : undefined; + if (!hasValue && varDefNode.defaultValue) { + // If no value was provided to a variable with a default value, + // use the default value. + coercedValues[varName] = valueFromAST(varDefNode.defaultValue, varType); + } else if ((!hasValue || value === null) && isNonNullType(varType)) { + // If no value or a nullish value was provided to a variable with a + // non-null type (required), produce an error. + errors.push( + new GraphQLError( + hasValue + ? `Variable "$${varName}" of non-null type ` + + `"${String(varType)}" must not be null.` + : `Variable "$${varName}" of required type ` + `"${String(varType)}" was not provided.`, - [varDefNode], - ), - ); - } else if (varDefNode.defaultValue) { - coercedValues[varName] = valueFromAST( - varDefNode.defaultValue, - varType, - ); - } - } else { - const coerced = coerceValue(value, varType, varDefNode); - const coercionErrors = coerced.errors; - if (coercionErrors) { - const messagePrelude = `Variable "$${varName}" got invalid value ${JSON.stringify( - value, - )}; `; - coercionErrors.forEach(error => { - error.message = messagePrelude + error.message; - }); - errors.push(...coercionErrors); + [varDefNode], + ), + ); + } else if (hasValue) { + if (value === null) { + // If the explicit value `null` was provided, an entry in the coerced + // values must exist as the value `null`. + coercedValues[varName] = null; } else { - coercedValues[varName] = coerced.value; + // Otherwise, a non-null value was provided, coerce it to the expected + // type or report an error if coercion fails. + const coerced = coerceValue(value, varType, varDefNode); + const coercionErrors = coerced.errors; + if (coercionErrors) { + coercionErrors.forEach(error => { + error.message = + `Variable "$${varName}" got invalid ` + + `value ${JSON.stringify(value)}; ${error.message}`; + }); + errors.push(...coercionErrors); + } else { + coercedValues[varName] = coerced.value; + } } } } @@ -126,51 +138,71 @@ export function getArgumentValues( const name = argDef.name; const argType = argDef.type; const argumentNode = argNodeMap[name]; - const defaultValue = argDef.defaultValue; - if (!argumentNode) { - if (!isInvalid(defaultValue)) { - coercedValues[name] = defaultValue; - } else if (isNonNullType(argType)) { - throw new GraphQLError( - `Argument "${name}" of required type ` + - `"${String(argType)}" was not provided.`, - [node], - ); - } - } else if (argumentNode.value.kind === Kind.VARIABLE) { + let hasValue; + let isNull; + if (argumentNode && argumentNode.value.kind === Kind.VARIABLE) { const variableName = argumentNode.value.name.value; - if ( - variableValues && - Object.prototype.hasOwnProperty.call(variableValues, variableName) && - !isInvalid(variableValues[variableName]) - ) { - // Note: this does not check that this variable value is correct. - // This assumes that this query has been validated and the variable - // usage here is of the correct type. - coercedValues[name] = variableValues[variableName]; - } else if (!isInvalid(defaultValue)) { - coercedValues[name] = defaultValue; - } else if (isNonNullType(argType)) { + hasValue = variableValues && hasOwnProperty(variableValues, variableName); + isNull = variableValues && variableValues[variableName] === null; + } else { + hasValue = argumentNode != null; + isNull = argumentNode && argumentNode.value.kind === Kind.NULL; + } + + if (!hasValue && argDef.defaultValue !== undefined) { + // If no argument was provided where the definition has a default value, + // use the default value. + coercedValues[name] = argDef.defaultValue; + } else if ((!hasValue || isNull) && isNonNullType(argType)) { + // If no argument or a null value was provided to an argument with a + // non-null type (required), produce a field error. + if (isNull) { throw new GraphQLError( - `Argument "${name}" of required type "${String(argType)}" was ` + - `provided the variable "$${variableName}" which was not provided ` + - 'a runtime value.', + `Argument "${name}" of non-null type "${String(argType)}" ` + + 'must not be null.', [argumentNode.value], ); - } - } else { - const valueNode = argumentNode.value; - const coercedValue = valueFromAST(valueNode, argType, variableValues); - if (isInvalid(coercedValue)) { - // Note: ValuesOfCorrectType validation should catch this before - // execution. This is a runtime check to ensure execution does not - // continue with an invalid argument value. + } else if (argumentNode && argumentNode.value.kind === Kind.VARIABLE) { + const variableName = argumentNode.value.name.value; throw new GraphQLError( - `Argument "${name}" has invalid value ${print(valueNode)}.`, + `Argument "${name}" of required type "${String(argType)}" ` + + `was provided the variable "$${variableName}" ` + + 'which was not provided a runtime value.', [argumentNode.value], ); + } else { + throw new GraphQLError( + `Argument "${name}" of required type "${String(argType)}" ` + + 'was not provided.', + [node], + ); + } + } else if (hasValue) { + if (argumentNode.value.kind === Kind.NULL) { + // If the explicit value `null` was provided, an entry in the coerced + // values must exist as the value `null`. + coercedValues[name] = null; + } else if (argumentNode.value.kind === Kind.VARIABLE) { + const variableName = argumentNode.value.name.value; + invariant(variableValues, 'Must exist for hasValue to be true.'); + // Note: This does no further checking that this variable is correct. + // This assumes that this query has been validated and the variable + // usage here is of the correct type. + coercedValues[name] = variableValues[variableName]; + } else { + const valueNode = argumentNode.value; + const coercedValue = valueFromAST(valueNode, argType, variableValues); + if (coercedValue === undefined) { + // Note: ValuesOfCorrectType validation should catch this before + // execution. This is a runtime check to ensure execution does not + // continue with an invalid argument value. + throw new GraphQLError( + `Argument "${name}" has invalid value ${print(valueNode)}.`, + [argumentNode.value], + ); + } + coercedValues[name] = coercedValue; } - coercedValues[name] = coercedValue; } } return coercedValues; @@ -203,3 +235,7 @@ export function getDirectiveValues( return getArgumentValues(directiveDef, directiveNode, variableValues); } } + +function hasOwnProperty(obj: mixed, prop: string): boolean { + return Object.prototype.hasOwnProperty.call(obj, prop); +} diff --git a/src/index.js b/src/index.js index 34fbd4b640..eaade28c45 100644 --- a/src/index.js +++ b/src/index.js @@ -285,7 +285,7 @@ export { NoUnusedVariablesRule, OverlappingFieldsCanBeMergedRule, PossibleFragmentSpreadsRule, - ProvidedNonNullArgumentsRule, + ProvidedRequiredArgumentsRule, ScalarLeafsRule, SingleFieldSubscriptionsRule, UniqueArgumentNamesRule, @@ -296,7 +296,6 @@ export { UniqueVariableNamesRule, ValuesOfCorrectTypeRule, VariablesAreInputTypesRule, - VariablesDefaultValueAllowedRule, VariablesInAllowedPositionRule, } from './validation'; diff --git a/src/type/__tests__/introspection-test.js b/src/type/__tests__/introspection-test.js index cf39e5a71d..9437beb4e2 100644 --- a/src/type/__tests__/introspection-test.js +++ b/src/type/__tests__/introspection-test.js @@ -8,7 +8,7 @@ import { expect } from 'chai'; import { describe, it } from 'mocha'; -import { missingFieldArgMessage } from '../../validation/rules/ProvidedNonNullArguments'; +import { missingFieldArgMessage } from '../../validation/rules/ProvidedRequiredArguments'; import { graphqlSync, GraphQLSchema, diff --git a/src/utilities/TypeInfo.js b/src/utilities/TypeInfo.js index 80e9e957bd..67911758c2 100644 --- a/src/utilities/TypeInfo.js +++ b/src/utilities/TypeInfo.js @@ -27,6 +27,7 @@ import type { GraphQLCompositeType, GraphQLField, GraphQLArgument, + GraphQLInputField, GraphQLEnumValue, } from '../type/definition'; import type { GraphQLDirective } from '../type/directives'; @@ -51,6 +52,7 @@ export class TypeInfo { _parentTypeStack: Array; _inputTypeStack: Array; _fieldDefStack: Array>; + _defaultValueStack: Array; _directive: ?GraphQLDirective; _argument: ?GraphQLArgument; _enumValue: ?GraphQLEnumValue; @@ -71,6 +73,7 @@ export class TypeInfo { this._parentTypeStack = []; this._inputTypeStack = []; this._fieldDefStack = []; + this._defaultValueStack = []; this._directive = null; this._argument = null; this._enumValue = null; @@ -118,6 +121,12 @@ export class TypeInfo { } } + getDefaultValue(): ?mixed { + if (this._defaultValueStack.length > 0) { + return this._defaultValueStack[this._defaultValueStack.length - 1]; + } + } + getDirective(): ?GraphQLDirective { return this._directive; } @@ -199,6 +208,7 @@ export class TypeInfo { } } this._argument = argDef; + this._defaultValueStack.push(argDef ? argDef.defaultValue : undefined); this._inputTypeStack.push(isInputType(argType) ? argType : undefined); break; case Kind.LIST: @@ -206,17 +216,23 @@ export class TypeInfo { const itemType: mixed = isListType(listType) ? listType.ofType : listType; + // List positions never have a default value. + this._defaultValueStack.push(undefined); this._inputTypeStack.push(isInputType(itemType) ? itemType : undefined); break; case Kind.OBJECT_FIELD: const objectType: mixed = getNamedType(this.getInputType()); - let inputFieldType: mixed; + let inputFieldType: GraphQLInputType | void; + let inputField: GraphQLInputField | void; if (isInputObjectType(objectType)) { - const inputField = objectType.getFields()[node.name.value]; + inputField = objectType.getFields()[node.name.value]; if (inputField) { inputFieldType = inputField.type; } } + this._defaultValueStack.push( + inputField ? inputField.defaultValue : undefined, + ); this._inputTypeStack.push( isInputType(inputFieldType) ? inputFieldType : undefined, ); @@ -254,10 +270,12 @@ export class TypeInfo { break; case Kind.ARGUMENT: this._argument = null; + this._defaultValueStack.pop(); this._inputTypeStack.pop(); break; case Kind.LIST: case Kind.OBJECT_FIELD: + this._defaultValueStack.pop(); this._inputTypeStack.pop(); break; case Kind.ENUM: diff --git a/src/utilities/valueFromAST.js b/src/utilities/valueFromAST.js index b0de000465..f198d1fcca 100644 --- a/src/utilities/valueFromAST.js +++ b/src/utilities/valueFromAST.js @@ -71,10 +71,14 @@ export function valueFromAST( // No valid return value. return; } - // Note: we're not doing any checking that this variable is correct. We're - // assuming that this query has been validated and the variable usage here - // is of the correct type. - return variables[variableName]; + const variableValue = variables[variableName]; + if (variableValue === null && isNonNullType(type)) { + return; // Invalid: intentionally return no value. + } + // Note: This does no further checking that this variable is correct. + // This assumes that this query has been validated and the variable + // usage here is of the correct type. + return variableValue; } if (isListType(type)) { @@ -118,7 +122,7 @@ export function valueFromAST( const field = fields[i]; const fieldNode = fieldNodes[field.name]; if (!fieldNode || isMissingVariable(fieldNode.value, variables)) { - if (!isInvalid(field.defaultValue)) { + if (field.defaultValue !== undefined) { coercedObj[field.name] = field.defaultValue; } else if (isNonNullType(field.type)) { return; // Invalid: intentionally return no value. diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js index 98f2e21cc4..1d6ef75741 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -31,7 +31,11 @@ import type { GraphQLDirective } from '../type/directives'; import { TypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; -type VariableUsage = { node: VariableNode, type: ?GraphQLInputType }; +type VariableUsage = {| + +node: VariableNode, + +type: ?GraphQLInputType, + +defaultValue: ?mixed, +|}; /** * An instance of this class is passed as the "this" context to all validators, @@ -163,7 +167,11 @@ export default class ValidationContext { visitWithTypeInfo(typeInfo, { VariableDefinition: () => false, Variable(variable) { - newUsages.push({ node: variable, type: typeInfo.getInputType() }); + newUsages.push({ + node: variable, + type: typeInfo.getInputType(), + defaultValue: typeInfo.getDefaultValue(), + }); }, }), ); diff --git a/src/validation/__tests__/ProvidedNonNullArguments-test.js b/src/validation/__tests__/ProvidedRequiredArguments-test.js similarity index 84% rename from src/validation/__tests__/ProvidedNonNullArguments-test.js rename to src/validation/__tests__/ProvidedRequiredArguments-test.js index 42b645e441..f003aab2a9 100644 --- a/src/validation/__tests__/ProvidedNonNullArguments-test.js +++ b/src/validation/__tests__/ProvidedRequiredArguments-test.js @@ -8,10 +8,10 @@ import { describe, it } from 'mocha'; import { expectPassesRule, expectFailsRule } from './harness'; import { - ProvidedNonNullArguments, + ProvidedRequiredArguments, missingFieldArgMessage, missingDirectiveArgMessage, -} from '../rules/ProvidedNonNullArguments'; +} from '../rules/ProvidedRequiredArguments'; function missingFieldArg(fieldName, argName, typeName, line, column) { return { @@ -30,7 +30,7 @@ function missingDirectiveArg(directiveName, argName, typeName, line, column) { describe('Validate: Provided required arguments', () => { it('ignores unknown arguments', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { dog { @@ -44,7 +44,7 @@ describe('Validate: Provided required arguments', () => { describe('Valid non-nullable value', () => { it('Arg on optional arg', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { dog { @@ -57,7 +57,7 @@ describe('Validate: Provided required arguments', () => { it('No Arg on optional arg', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { dog { @@ -68,9 +68,22 @@ describe('Validate: Provided required arguments', () => { ); }); + it('No arg on non-null field with default', () => { + expectPassesRule( + ProvidedRequiredArguments, + ` + { + complicatedArgs { + nonNullFieldWithDefault + } + } + `, + ); + }); + it('Multiple args', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -83,7 +96,7 @@ describe('Validate: Provided required arguments', () => { it('Multiple args reverse order', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -96,7 +109,7 @@ describe('Validate: Provided required arguments', () => { it('No args on multiple optional', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -109,7 +122,7 @@ describe('Validate: Provided required arguments', () => { it('One arg on multiple optional', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -122,7 +135,7 @@ describe('Validate: Provided required arguments', () => { it('Second arg on multiple optional', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -135,7 +148,7 @@ describe('Validate: Provided required arguments', () => { it('Multiple reqs on mixedList', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -148,7 +161,7 @@ describe('Validate: Provided required arguments', () => { it('Multiple reqs and one opt on mixedList', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -161,7 +174,7 @@ describe('Validate: Provided required arguments', () => { it('All reqs and opts on mixedList', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -176,7 +189,7 @@ describe('Validate: Provided required arguments', () => { describe('Invalid non-nullable value', () => { it('Missing one non-nullable argument', () => { expectFailsRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -190,7 +203,7 @@ describe('Validate: Provided required arguments', () => { it('Missing multiple non-nullable arguments', () => { expectFailsRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -207,7 +220,7 @@ describe('Validate: Provided required arguments', () => { it('Incorrect value and missing argument', () => { expectFailsRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { complicatedArgs { @@ -223,7 +236,7 @@ describe('Validate: Provided required arguments', () => { describe('Directive arguments', () => { it('ignores unknown directives', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { dog @unknown @@ -234,7 +247,7 @@ describe('Validate: Provided required arguments', () => { it('with directives of valid types', () => { expectPassesRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { dog @include(if: true) { @@ -250,7 +263,7 @@ describe('Validate: Provided required arguments', () => { it('with directive with missing types', () => { expectFailsRule( - ProvidedNonNullArguments, + ProvidedRequiredArguments, ` { dog @include { diff --git a/src/validation/__tests__/ValuesOfCorrectType-test.js b/src/validation/__tests__/ValuesOfCorrectType-test.js index 8237675a3b..3f74754a27 100644 --- a/src/validation/__tests__/ValuesOfCorrectType-test.js +++ b/src/validation/__tests__/ValuesOfCorrectType-test.js @@ -828,7 +828,7 @@ describe('Validate: Values of correct type', () => { ); }); - it('Incorrect value and missing argument (ProvidedNonNullArguments)', () => { + it('Incorrect value and missing argument (ProvidedRequiredArguments)', () => { expectFailsRule( ValuesOfCorrectType, ` @@ -981,6 +981,23 @@ describe('Validate: Values of correct type', () => { ); }); + it('Partial object, null to non-null field', () => { + expectFailsRule( + ValuesOfCorrectType, + ` + { + complicatedArgs { + complexArgField(complexArg: { + requiredField: true, + nonNullField: null, + }) + } + } + `, + [badValue('Boolean!', 'null', 6, 29)], + ); + }); + it('Partial object, unknown field arg', () => { expectFailsRule( ValuesOfCorrectType, @@ -1000,7 +1017,7 @@ describe('Validate: Values of correct type', () => { 'unknownField', 6, 15, - 'Did you mean intField or booleanField?', + 'Did you mean nonNullField, intField, or booleanField?', ), ], ); @@ -1088,6 +1105,7 @@ describe('Validate: Values of correct type', () => { $a: Int = 1, $b: String = "ok", $c: ComplexInput = { requiredField: true, intField: 3 } + $d: Int! = 123 ) { dog { name } } diff --git a/src/validation/__tests__/VariablesDefaultValueAllowed-test.js b/src/validation/__tests__/VariablesDefaultValueAllowed-test.js deleted file mode 100644 index 26707d0b36..0000000000 --- a/src/validation/__tests__/VariablesDefaultValueAllowed-test.js +++ /dev/null @@ -1,104 +0,0 @@ -/** - * Copyright (c) 2015-present, Facebook, Inc. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - */ - -import { describe, it } from 'mocha'; -import { expectPassesRule, expectFailsRule } from './harness'; -import { - VariablesDefaultValueAllowed, - defaultForRequiredVarMessage, -} from '../rules/VariablesDefaultValueAllowed'; - -function defaultForRequiredVar(varName, typeName, guessTypeName, line, column) { - return { - message: defaultForRequiredVarMessage(varName, typeName, guessTypeName), - locations: [{ line, column }], - }; -} - -describe('Validate: Variable default value is allowed', () => { - it('variables with no default values', () => { - expectPassesRule( - VariablesDefaultValueAllowed, - ` - query NullableValues($a: Int, $b: String, $c: ComplexInput) { - dog { name } - } - `, - ); - }); - - it('required variables without default values', () => { - expectPassesRule( - VariablesDefaultValueAllowed, - ` - query RequiredValues($a: Int!, $b: String!) { - dog { name } - } - `, - ); - }); - - it('variables with valid default values', () => { - expectPassesRule( - VariablesDefaultValueAllowed, - ` - query WithDefaultValues( - $a: Int = 1, - $b: String = "ok", - $c: ComplexInput = { requiredField: true, intField: 3 } - ) { - dog { name } - } - `, - ); - }); - - it('variables with valid default null values', () => { - expectPassesRule( - VariablesDefaultValueAllowed, - ` - query WithDefaultValues( - $a: Int = null, - $b: String = null, - $c: ComplexInput = { requiredField: true, intField: null } - ) { - dog { name } - } - `, - ); - }); - - it('no required variables with default values', () => { - expectFailsRule( - VariablesDefaultValueAllowed, - ` - query UnreachableDefaultValues($a: Int! = 3, $b: String! = "default") { - dog { name } - } - `, - [ - defaultForRequiredVar('a', 'Int!', 'Int', 2, 49), - defaultForRequiredVar('b', 'String!', 'String', 2, 66), - ], - ); - }); - - it('variables with invalid default null values', () => { - expectFailsRule( - VariablesDefaultValueAllowed, - ` - query WithDefaultValues($a: Int! = null, $b: String! = null) { - dog { name } - } - `, - [ - defaultForRequiredVar('a', 'Int!', 'Int', 2, 42), - defaultForRequiredVar('b', 'String!', 'String', 2, 62), - ], - ); - }); -}); diff --git a/src/validation/__tests__/VariablesInAllowedPosition-test.js b/src/validation/__tests__/VariablesInAllowedPosition-test.js index d522e5b6a2..c8b40e90ee 100644 --- a/src/validation/__tests__/VariablesInAllowedPosition-test.js +++ b/src/validation/__tests__/VariablesInAllowedPosition-test.js @@ -91,20 +91,6 @@ describe('Validate: Variables are in allowed positions', () => { ); }); - it('Int => Int! with default', () => { - expectPassesRule( - VariablesInAllowedPosition, - ` - query Query($intArg: Int = 1) - { - complicatedArgs { - nonNullIntArgField(nonNullIntArg: $intArg) - } - } - `, - ); - }); - it('[String] => [String]', () => { expectPassesRule( VariablesInAllowedPosition, @@ -201,18 +187,6 @@ describe('Validate: Variables are in allowed positions', () => { ); }); - it('Boolean => Boolean! in directive with default', () => { - expectPassesRule( - VariablesInAllowedPosition, - ` - query Query($boolVar: Boolean = false) - { - dog @include(if: $boolVar) - } - `, - ); - }); - it('Int => Int!', () => { expectFailsRule( VariablesInAllowedPosition, @@ -373,4 +347,58 @@ describe('Validate: Variables are in allowed positions', () => { ], ); }); + + describe('Allows optional (nullable) variables with default values', () => { + it('Int => Int! fails when variable provides null default value', () => { + expectFailsRule( + VariablesInAllowedPosition, + ` + query Query($intVar: Int = null) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + }`, + [ + { + message: badVarPosMessage('intVar', 'Int', 'Int!'), + locations: [{ line: 2, column: 21 }, { line: 4, column: 47 }], + }, + ], + ); + }); + + it('Int => Int! when variable provides non-null default value', () => { + expectPassesRule( + VariablesInAllowedPosition, + ` + query Query($intVar: Int = 1) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + }`, + ); + }); + + it('Int => Int! when optional argument provides default value', () => { + expectPassesRule( + VariablesInAllowedPosition, + ` + query Query($intVar: Int) { + complicatedArgs { + nonNullFieldWithDefault(nonNullIntArg: $intVar) + } + }`, + ); + }); + + it('Boolean => Boolean! in directive with default value with option', () => { + expectPassesRule( + VariablesInAllowedPosition, + ` + query Query($boolVar: Boolean = false) { + dog @include(if: $boolVar) + }`, + ); + }); + }); }); diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js index 7efefb8923..6d428faa79 100644 --- a/src/validation/__tests__/harness.js +++ b/src/validation/__tests__/harness.js @@ -182,6 +182,7 @@ const ComplexInput = new GraphQLInputObjectType({ name: 'ComplexInput', fields: { requiredField: { type: GraphQLNonNull(GraphQLBoolean) }, + nonNullField: { type: GraphQLNonNull(GraphQLBoolean), defaultValue: false }, intField: { type: GraphQLInt }, stringField: { type: GraphQLString }, booleanField: { type: GraphQLBoolean }, @@ -246,6 +247,12 @@ const ComplicatedArgs = new GraphQLObjectType({ req2: { type: GraphQLNonNull(GraphQLInt) }, }, }, + nonNullFieldWithDefault: { + type: GraphQLString, + args: { + arg: { type: GraphQLNonNull(GraphQLInt), defaultValue: 0 }, + }, + }, multipleOpts: { type: GraphQLString, args: { diff --git a/src/validation/index.js b/src/validation/index.js index 6d33dce405..afcff6994b 100644 --- a/src/validation/index.js +++ b/src/validation/index.js @@ -80,8 +80,8 @@ export { // Spec Section: "Argument Optionality" export { - ProvidedNonNullArguments as ProvidedNonNullArgumentsRule, -} from './rules/ProvidedNonNullArguments'; + ProvidedRequiredArguments as ProvidedRequiredArgumentsRule, +} from './rules/ProvidedRequiredArguments'; // Spec Section: "Leaf Field Selections" export { ScalarLeafs as ScalarLeafsRule } from './rules/ScalarLeafs'; @@ -131,11 +131,6 @@ export { VariablesAreInputTypes as VariablesAreInputTypesRule, } from './rules/VariablesAreInputTypes'; -// Spec Section: "Variables Default Value Is Allowed" -export { - VariablesDefaultValueAllowed as VariablesDefaultValueAllowedRule, -} from './rules/VariablesDefaultValueAllowed'; - // Spec Section: "All Variable Usages Are Allowed" export { VariablesInAllowedPosition as VariablesInAllowedPositionRule, diff --git a/src/validation/rules/ProvidedNonNullArguments.js b/src/validation/rules/ProvidedRequiredArguments.js similarity index 85% rename from src/validation/rules/ProvidedNonNullArguments.js rename to src/validation/rules/ProvidedRequiredArguments.js index 5eda68eb32..4819e54592 100644 --- a/src/validation/rules/ProvidedNonNullArguments.js +++ b/src/validation/rules/ProvidedRequiredArguments.js @@ -39,10 +39,10 @@ export function missingDirectiveArgMessage( /** * Provided required arguments * - * A field or directive is only valid if all required (non-null) field arguments - * have been provided. + * A field or directive is only valid if all required (non-null without a + * default value) field arguments have been provided. */ -export function ProvidedNonNullArguments( +export function ProvidedRequiredArguments( context: ValidationContext, ): ASTVisitor { return { @@ -58,7 +58,11 @@ export function ProvidedNonNullArguments( const argNodeMap = keyMap(argNodes, arg => arg.name.value); fieldDef.args.forEach(argDef => { const argNode = argNodeMap[argDef.name]; - if (!argNode && isNonNullType(argDef.type)) { + if ( + !argNode && + isNonNullType(argDef.type) && + argDef.defaultValue === undefined + ) { context.reportError( new GraphQLError( missingFieldArgMessage( @@ -86,7 +90,11 @@ export function ProvidedNonNullArguments( const argNodeMap = keyMap(argNodes, arg => arg.name.value); directiveDef.args.forEach(argDef => { const argNode = argNodeMap[argDef.name]; - if (!argNode && isNonNullType(argDef.type)) { + if ( + !argNode && + isNonNullType(argDef.type) && + argDef.defaultValue === undefined + ) { context.reportError( new GraphQLError( missingDirectiveArgMessage( diff --git a/src/validation/rules/ValuesOfCorrectType.js b/src/validation/rules/ValuesOfCorrectType.js index d440a4ec3e..d17a7a80c0 100644 --- a/src/validation/rules/ValuesOfCorrectType.js +++ b/src/validation/rules/ValuesOfCorrectType.js @@ -95,9 +95,14 @@ export function ValuesOfCorrectType(context: ValidationContext): ASTVisitor { const inputFields = type.getFields(); const fieldNodeMap = keyMap(node.fields, field => field.name.value); Object.keys(inputFields).forEach(fieldName => { - const fieldType = inputFields[fieldName].type; + const fieldDef = inputFields[fieldName]; + const fieldType = fieldDef.type; const fieldNode = fieldNodeMap[fieldName]; - if (!fieldNode && isNonNullType(fieldType)) { + if ( + !fieldNode && + isNonNullType(fieldType) && + fieldDef.defaultValue === undefined + ) { context.reportError( new GraphQLError( requiredFieldMessage(type.name, fieldName, String(fieldType)), diff --git a/src/validation/rules/VariablesDefaultValueAllowed.js b/src/validation/rules/VariablesDefaultValueAllowed.js deleted file mode 100644 index 019b55e9fe..0000000000 --- a/src/validation/rules/VariablesDefaultValueAllowed.js +++ /dev/null @@ -1,55 +0,0 @@ -/** - * Copyright (c) 2015-present, Facebook, Inc. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow strict - */ - -import type ValidationContext from '../ValidationContext'; -import { GraphQLError } from '../../error'; -import type { ASTVisitor } from '../../language/visitor'; -import { isNonNullType } from '../../type/definition'; -import type { GraphQLType } from '../../type/definition'; - -export function defaultForRequiredVarMessage( - varName: string, - type: GraphQLType, - guessType: GraphQLType, -): string { - return ( - `Variable "$${varName}" of type "${String(type)}" is required and ` + - 'will not use the default value. ' + - `Perhaps you meant to use type "${String(guessType)}".` - ); -} - -/** - * Variable's default value is allowed - * - * A GraphQL document is only valid if all variable default values are allowed - * due to a variable not being required. - */ -export function VariablesDefaultValueAllowed( - context: ValidationContext, -): ASTVisitor { - return { - VariableDefinition(node) { - const name = node.variable.name.value; - const defaultValue = node.defaultValue; - const type = context.getInputType(); - if (isNonNullType(type) && defaultValue) { - context.reportError( - new GraphQLError( - defaultForRequiredVarMessage(name, type, type.ofType), - [defaultValue], - ), - ); - } - return false; // Do not traverse further. - }, - SelectionSet: () => false, - FragmentDefinition: () => false, - }; -} diff --git a/src/validation/rules/VariablesInAllowedPosition.js b/src/validation/rules/VariablesInAllowedPosition.js index 25a95f5943..20805feb49 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/src/validation/rules/VariablesInAllowedPosition.js @@ -9,11 +9,14 @@ import type ValidationContext from '../ValidationContext'; import { GraphQLError } from '../../error'; +import { Kind } from '../../language/kinds'; +import type { ValueNode } from '../../language/ast'; import type { ASTVisitor } from '../../language/visitor'; -import { GraphQLNonNull, isNonNullType } from '../../type/definition'; +import { isNonNullType } from '../../type/definition'; import { isTypeSubTypeOf } from '../../utilities/typeComparators'; import { typeFromAST } from '../../utilities/typeFromAST'; import type { GraphQLType } from '../../type/definition'; +import type { GraphQLSchema } from '../../type/schema'; export function badVarPosMessage( varName: string, @@ -42,7 +45,7 @@ export function VariablesInAllowedPosition( leave(operation) { const usages = context.getRecursiveVariableUsages(operation); - usages.forEach(({ node, type }) => { + usages.forEach(({ node, type, defaultValue }) => { const varName = node.name.value; const varDef = varDefMap[varName]; if (varDef && type) { @@ -55,7 +58,13 @@ export function VariablesInAllowedPosition( const varType = typeFromAST(schema, varDef.type); if ( varType && - !isTypeSubTypeOf(schema, effectiveType(varType, varDef), type) + !allowedVariableUsage( + schema, + varType, + varDef.defaultValue, + type, + defaultValue, + ) ) { context.reportError( new GraphQLError(badVarPosMessage(varName, varType, type), [ @@ -74,9 +83,27 @@ export function VariablesInAllowedPosition( }; } -// If a variable definition has a default value, it's effectively non-null. -function effectiveType(varType, varDef) { - return !varDef.defaultValue || isNonNullType(varType) - ? varType - : GraphQLNonNull(varType); +/** + * Returns true if the variable is allowed in the location it was found, + * which includes considering if default values exist for either the variable + * or the location at which it is located. + */ +function allowedVariableUsage( + schema: GraphQLSchema, + varType: GraphQLType, + varDefaultValue: ?ValueNode, + locationType: GraphQLType, + locationDefaultValue: ?mixed, +): boolean { + if (isNonNullType(locationType) && !isNonNullType(varType)) { + const hasNonNullVariableDefaultValue = + varDefaultValue && varDefaultValue.kind !== Kind.NULL; + const hasLocationDefaultValue = locationDefaultValue !== undefined; + if (!hasNonNullVariableDefaultValue && !hasLocationDefaultValue) { + return false; + } + const nullableLocationType = locationType.ofType; + return isTypeSubTypeOf(schema, varType, nullableLocationType); + } + return isTypeSubTypeOf(schema, varType, locationType); } diff --git a/src/validation/specifiedRules.js b/src/validation/specifiedRules.js index b5e45f90cb..77792d6a05 100644 --- a/src/validation/specifiedRules.js +++ b/src/validation/specifiedRules.js @@ -74,10 +74,7 @@ import { UniqueArgumentNames } from './rules/UniqueArgumentNames'; import { ValuesOfCorrectType } from './rules/ValuesOfCorrectType'; // Spec Section: "Argument Optionality" -import { ProvidedNonNullArguments } from './rules/ProvidedNonNullArguments'; - -// Spec Section: "Variables Default Value Is Allowed" -import { VariablesDefaultValueAllowed } from './rules/VariablesDefaultValueAllowed'; +import { ProvidedRequiredArguments } from './rules/ProvidedRequiredArguments'; // Spec Section: "All Variable Usages Are Allowed" import { VariablesInAllowedPosition } from './rules/VariablesInAllowedPosition'; @@ -119,8 +116,7 @@ export const specifiedRules: Array<(context: ValidationContext) => any> = [ KnownArgumentNames, UniqueArgumentNames, ValuesOfCorrectType, - ProvidedNonNullArguments, - VariablesDefaultValueAllowed, + ProvidedRequiredArguments, VariablesInAllowedPosition, OverlappingFieldsCanBeMerged, UniqueInputFieldNames,