From b105b846e82ae3b0c473c3e513f4e9ad4a85ef36 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Mon, 5 Mar 2018 12:17:32 -0800 Subject: [PATCH] SPEC/BUG: Ambiguity with null variable values and default values This change corresponds to a spec proposal which solves an ambiguity in how variable values and default values behave with explicit null values. Otherwise, this ambiguity allows for null values to appear in non-null argument values, which may result in unforseen null-pointer-errors. This appears in three distinct but related issues: **VariablesInAllowedPosition validation rule** The explicit value `null` may be used as a default value for a variable, however `VariablesInAllowedPositions` allowed a nullable type with a default value to flow into an argument expecting a non-null type. This validation rule must explicitly not allow `null` default values to flow in this manner. **Coercing Variable Values** coerceVariableValues allows the explicit `null` value to be used over a default value, which can result in flowing a null value to a non-null argument when paired with the validation rule mentioned above. Instead a default value must be used even when an explicit `null` value is provided. **Coercing Argument Values** coerceArgumentValues allows the explicit `null` default value to be used before checking for a non-null type. This could inadvertently allow a null value into a non-null typed argument. --- src/execution/__tests__/nonnull-test.js | 188 ++++++++++++++++++ src/execution/__tests__/variables-test.js | 106 +++++++++- src/execution/values.js | 56 ++++-- .../VariablesInAllowedPosition-test.js | 22 +- .../rules/VariablesInAllowedPosition.js | 17 +- 5 files changed, 362 insertions(+), 27 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.js b/src/execution/__tests__/nonnull-test.js index 73b120be8d0..bb7bdca7960 100644 --- a/src/execution/__tests__/nonnull-test.js +++ b/src/execution/__tests__/nonnull-test.js @@ -486,4 +486,192 @@ 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), + defaultValue: null, + }, + }, + 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('succeeds when null variable has default 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: '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'], + }, + ], + }); + }); + }); }); diff --git a/src/execution/__tests__/variables-test.js b/src/execution/__tests__/variables-test.js index 8d7c8c12ed5..d02d06d634e 100644 --- a/src/execution/__tests__/variables-test.js +++ b/src/execution/__tests__/variables-test.js @@ -82,6 +82,16 @@ const TestType = new GraphQLObjectType({ args: { input: { type: GraphQLString, defaultValue: 'Hello World' } }, resolve: (_, { input }) => input && JSON.stringify(input), }, + fieldWithNonNullableStringInputAndDefaultArgumentValue: { + type: GraphQLString, + args: { + input: { + type: GraphQLNonNull(GraphQLString), + defaultValue: 'Unreachable', + }, + }, + resolve: (_, { input }) => input && JSON.stringify(input), + }, fieldWithNestedInputObject: { type: GraphQLString, args: { @@ -264,6 +274,72 @@ describe('Execute: Handles inputs', () => { }); }); + it('does not use default value when provided', async () => { + const withDefaultsAST = parse(` + query q($input: String = "Default value") { + fieldWithNullableStringInput(input: $input) + } + `); + + const result = await execute({ + schema, + document: withDefaultsAST, + variableValues: { + input: 'Variable value', + }, + }); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: '"Variable value"', + }, + }); + }); + + it('uses default value when explicit null value provided', async () => { + const withDefaultsAST = parse(` + query q($input: String = "Default value") { + fieldWithNullableStringInput(input: $input) + } + `); + + const result = await execute({ + schema, + document: withDefaultsAST, + variableValues: { + input: null, + }, + }); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: '"Default value"', + }, + }); + }); + + it('uses null default value when not provided', async () => { + const withDefaultsAST = parse(` + query q($input: String = null) { + fieldWithNullableStringInput(input: $input) + } + `); + + const result = await execute({ + schema, + document: withDefaultsAST, + variableValues: { + // Intentionally missing value. + }, + }); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: null, + }, + }); + }); + it('properly parses single value to list', async () => { const params = { input: { a: 'foo', b: 'bar', c: 'baz' } }; const result = await execute(schema, ast, null, null, params); @@ -534,8 +610,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 required type "String!" was not provided.', locations: [{ line: 2, column: 31 }], path: undefined, }, @@ -733,8 +808,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 required type "[String]!" was not provided.', locations: [{ line: 2, column: 17 }], path: undefined, }, @@ -846,8 +920,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 required type "[String!]!" was not provided.', locations: [{ line: 2, column: 17 }], path: undefined, }, @@ -985,5 +1058,26 @@ describe('Execute: Handles inputs', () => { ], }); }); + + it('not when argument type is non-null', async () => { + const ast = parse(`query optionalVariable($optional: String) { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional) + }`); + + expect(await execute(schema, ast)).to.deep.equal({ + data: { + fieldWithNonNullableStringInputAndDefaultArgumentValue: null, + }, + errors: [ + { + message: + 'Argument "input" of required type "String!" was provided the ' + + 'variable "$optional" which was not provided a runtime value.', + locations: [{ line: 2, column: 71 }], + path: ['fieldWithNonNullableStringInputAndDefaultArgumentValue'], + }, + ], + }); + }); }); }); diff --git a/src/execution/values.js b/src/execution/values.js index 23ecca20451..9fbbf4901b2 100644 --- a/src/execution/values.js +++ b/src/execution/values.js @@ -10,6 +10,7 @@ import { GraphQLError } from '../error'; import find from '../jsutils/find'; import isInvalid from '../jsutils/isInvalid'; +import isNullish from '../jsutils/isNullish'; import keyMap from '../jsutils/keyMap'; import { coerceValue } from '../utilities/coerceValue'; import { typeFromAST } from '../utilities/typeFromAST'; @@ -54,6 +55,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 ` + @@ -64,9 +67,12 @@ export function getVariableValues( ), ); } else { + const hasValue = hasOwnProperty(inputs, varName); const value = inputs[varName]; - if (isInvalid(value)) { + if (!hasValue || isNullish(value)) { if (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( `Variable "$${varName}" of required type ` + @@ -75,12 +81,20 @@ export function getVariableValues( ), ); } else if (varDefNode.defaultValue) { + // If no value or a nullish value was provided to a variable with a + // default value, use the default value. coercedValues[varName] = valueFromAST( varDefNode.defaultValue, varType, ); + } else if (hasValue && isNullish(value)) { + // If the explcit value `null` was provided, an entry in the coerced + // values must exist as the value `null`. + coercedValues[varName] = null; } } else { + // 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) { @@ -128,29 +142,33 @@ export function getArgumentValues( const argType = argDef.type; const argumentNode = argNodeMap[name]; const defaultValue = argDef.defaultValue; - if (!argumentNode) { - if (!isInvalid(defaultValue)) { + if (!argumentNode || argumentNode.value.kind === Kind.NULL) { + if (isNonNullType(argType)) { + if (!argumentNode) { + throw new GraphQLError( + `Argument "${name}" of required type "${String(argType)}" ` + + 'was not provided.', + [node], + ); + } else { + throw new GraphQLError( + `Argument "${name}" of non-null type "${String(argType)}" ` + + 'must not be null.', + [argumentNode.value], + ); + } + } else 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 && argumentNode.value.kind === Kind.NULL) { + coercedValues[name] = null; } } else if (argumentNode.value.kind === Kind.VARIABLE) { const variableName = (argumentNode.value: VariableNode).name.value; - if ( - variableValues && - Object.prototype.hasOwnProperty.call(variableValues, variableName) && - !isInvalid(variableValues[variableName]) - ) { + if (variableValues && hasOwnProperty(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)) { throw new GraphQLError( `Argument "${name}" of required type "${String(argType)}" was ` + @@ -158,6 +176,8 @@ export function getArgumentValues( 'a runtime value.', [argumentNode.value], ); + } else if (!isInvalid(defaultValue)) { + coercedValues[name] = defaultValue; } } else { const valueNode = argumentNode.value; @@ -204,3 +224,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/validation/__tests__/VariablesInAllowedPosition-test.js b/src/validation/__tests__/VariablesInAllowedPosition-test.js index 94afbedf94c..cd1f9c849f1 100644 --- a/src/validation/__tests__/VariablesInAllowedPosition-test.js +++ b/src/validation/__tests__/VariablesInAllowedPosition-test.js @@ -91,7 +91,7 @@ describe('Validate: Variables are in allowed positions', () => { ); }); - it('Int => Int! with default', () => { + it('Int => Int! with non-null default value', () => { expectPassesRule( VariablesInAllowedPosition, ` @@ -233,6 +233,26 @@ describe('Validate: Variables are in allowed positions', () => { ); }); + it('Int => Int! with null default value', () => { + expectFailsRule( + VariablesInAllowedPosition, + ` + query Query($intArg: Int = null) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intArg) + } + } + `, + [ + { + message: badVarPosMessage('intArg', 'Int', 'Int!'), + locations: [{ line: 2, column: 19 }, { line: 4, column: 45 }], + path: undefined, + }, + ], + ); + }); + it('Int => Int! within fragment', () => { expectFailsRule( VariablesInAllowedPosition, diff --git a/src/validation/rules/VariablesInAllowedPosition.js b/src/validation/rules/VariablesInAllowedPosition.js index 47073e5757b..65286685b22 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/src/validation/rules/VariablesInAllowedPosition.js @@ -9,6 +9,7 @@ import type ValidationContext from '../ValidationContext'; import { GraphQLError } from '../../error'; +import { Kind } from '../../language/kinds'; import type { ASTVisitor } from '../../language/visitor'; import { isNonNullType } from '../../type/definition'; import { GraphQLNonNull } from '../../type/wrappers'; @@ -75,9 +76,17 @@ export function VariablesInAllowedPosition( }; } -// If a variable definition has a default value, it's effectively non-null. +/** + * If varType is not non-null and defaultValue is provided and not null: + * Let varType be the non-null of varType. + */ function effectiveType(varType, varDef) { - return !varDef.defaultValue || isNonNullType(varType) - ? varType - : GraphQLNonNull(varType); + if ( + !isNonNullType(varType) && + varDef.defaultValue && + varDef.defaultValue.kind !== Kind.NULL + ) { + return GraphQLNonNull(varType); + } + return varType; }