From 1dfa1ed94a339a362bb51742f1e7eb3c53f9407c Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Mon, 5 Mar 2018 12:17:32 -0800 Subject: [PATCH 1/5] 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 | 83 +++++++- src/execution/values.js | 62 ++++-- .../VariablesInAllowedPosition-test.js | 22 +- .../rules/VariablesInAllowedPosition.js | 17 +- 5 files changed, 343 insertions(+), 29 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.js b/src/execution/__tests__/nonnull-test.js index 73b120be8d..bb7bdca796 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 30850c447b..ecaf6830cb 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: 'Unreachable', + }), fieldWithNestedInputObject: fieldWithInputArg({ type: TestNestedInputObject, defaultValue: 'Hello World', @@ -236,6 +240,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 default value when explicit null value provided', () => { + const result = executeQuery( + ` + query q($input: String = "Default value") { + fieldWithNullableStringInput(input: $input) + }`, + { input: null }, + ); + + expect(result).to.deep.equal({ + data: { + fieldWithNullableStringInput: "'Default value'", + }, + }); + }); + + 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 +538,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 +696,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 +779,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 +903,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 ab9834e6de..46e3b6bff6 100644 --- a/src/execution/values.js +++ b/src/execution/values.js @@ -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,23 +65,37 @@ export function getVariableValues( ), ); } else { + const hasValue = hasOwnProperty(inputs, varName); const value = inputs[varName]; - if (isInvalid(value)) { + if (!hasValue || value == null) { 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 ` + - `"${String(varType)}" was not provided.`, + 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) { + // 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 && value == null) { + // 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) { @@ -127,29 +143,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.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 ` + @@ -157,6 +177,8 @@ export function getArgumentValues( 'a runtime value.', [argumentNode.value], ); + } else if (!isInvalid(defaultValue)) { + coercedValues[name] = defaultValue; } } else { const valueNode = argumentNode.value; @@ -203,3 +225,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 d522e5b6a2..467ae5fa6e 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, ` @@ -232,6 +232,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 25a95f5943..3c3e77f5d1 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 { GraphQLNonNull, isNonNullType } from '../../type/definition'; import { isTypeSubTypeOf } from '../../utilities/typeComparators'; @@ -74,9 +75,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; } From b468956a614e8658de200192533f223b9c8b5aa2 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Thu, 29 Mar 2018 19:36:51 -0700 Subject: [PATCH 2/5] Changes based on feedback: This preserves the behavior of explicit null values taking precedence over default values, greatly reducing the scope of breaking changes. This adds a change to argument coercion to enforce null checking on variable valuesand removes one validation rule: VariablesDefaultValueAllowed. This allows supplying default values to non-nullable variables, widening the gap between "required" and "nullable". It also changes the validation rules for arguments - allowing non-null arguments with default values to be omitted. This preserves all existing behavior (with the critical exception of no longer allowing `null` values into non-null arguments) while allowing queries which were previously considered invalid to be valid. --- src/execution/__tests__/nonnull-test.js | 49 ++--- src/execution/__tests__/variables-test.js | 64 +++++-- src/execution/values.js | 176 +++++++++--------- src/index.js | 3 +- src/type/__tests__/introspection-test.js | 2 +- src/utilities/valueFromAST.js | 14 +- ...t.js => ProvidedRequiredArguments-test.js} | 51 +++-- .../__tests__/ValuesOfCorrectType-test.js | 22 ++- .../VariablesDefaultValueAllowed-test.js | 104 ----------- src/validation/__tests__/harness.js | 7 + src/validation/index.js | 9 +- ...uments.js => ProvidedRequiredArguments.js} | 18 +- src/validation/rules/ValuesOfCorrectType.js | 9 +- .../rules/VariablesDefaultValueAllowed.js | 55 ------ .../rules/VariablesInAllowedPosition.js | 5 + src/validation/specifiedRules.js | 8 +- 16 files changed, 266 insertions(+), 330 deletions(-) rename src/validation/__tests__/{ProvidedNonNullArguments-test.js => ProvidedRequiredArguments-test.js} (84%) delete mode 100644 src/validation/__tests__/VariablesDefaultValueAllowed-test.js rename src/validation/rules/{ProvidedNonNullArguments.js => ProvidedRequiredArguments.js} (85%) delete mode 100644 src/validation/rules/VariablesDefaultValueAllowed.js diff --git a/src/execution/__tests__/nonnull-test.js b/src/execution/__tests__/nonnull-test.js index bb7bdca796..58aef906b0 100644 --- a/src/execution/__tests__/nonnull-test.js +++ b/src/execution/__tests__/nonnull-test.js @@ -497,7 +497,6 @@ describe('Execute: handles non-nullable types', () => { args: { cannotBeNull: { type: GraphQLNonNull(GraphQLString), - defaultValue: null, }, }, resolve: async (_, args) => { @@ -567,26 +566,6 @@ describe('Execute: handles non-nullable types', () => { }); }); - 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. @@ -673,5 +652,33 @@ describe('Execute: handles non-nullable types', () => { ], }); }); + + 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 ecaf6830cb..49596c5bfb 100644 --- a/src/execution/__tests__/variables-test.js +++ b/src/execution/__tests__/variables-test.js @@ -86,7 +86,7 @@ const TestType = new GraphQLObjectType({ }), fieldWithNonNullableStringInputAndDefaultArgumentValue: fieldWithInputArg({ type: GraphQLNonNull(GraphQLString), - defaultValue: 'Unreachable', + defaultValue: 'Hello World', }), fieldWithNestedInputObject: fieldWithInputArg({ type: TestNestedInputObject, @@ -226,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"}) { @@ -255,7 +289,7 @@ describe('Execute: Handles inputs', () => { }); }); - it('uses default value when explicit null value provided', () => { + it('uses explicit null value instead of default value', () => { const result = executeQuery( ` query q($input: String = "Default value") { @@ -266,7 +300,7 @@ describe('Execute: Handles inputs', () => { expect(result).to.deep.equal({ data: { - fieldWithNullableStringInput: "'Default value'", + fieldWithNullableStringInput: 'null', }, }); }); @@ -904,24 +938,18 @@ describe('Execute: Handles inputs', () => { }); }); - it('not when argument type is non-null', async () => { - const ast = parse(`query optionalVariable($optional: String) { - fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional) - }`); + it('when no runtime value is provided to a non-null argument', () => { + const result = executeQuery(` + query optionalVariable($optional: String) { + fieldWithNonNullableStringInputAndDefaultArgumentValue(input: $optional) + } + `); - expect(await execute(schema, ast)).to.deep.equal({ + expect(result).to.deep.equal({ data: { - fieldWithNonNullableStringInputAndDefaultArgumentValue: null, + fieldWithNonNullableStringInputAndDefaultArgumentValue: + "'Hello World'", }, - 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 46e3b6bff6..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'; @@ -66,48 +66,44 @@ export function getVariableValues( ); } else { const hasValue = hasOwnProperty(inputs, varName); - const value = inputs[varName]; - if (!hasValue || value == null) { - 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( - 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) { - // 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 && value == null) { - // If the explcit value `null` was provided, an entry in the coerced + 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 (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 { - // 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) { - const messagePrelude = `Variable "$${varName}" got invalid value ${JSON.stringify( - value, - )}; `; - coercionErrors.forEach(error => { - error.message = messagePrelude + error.message; - }); - errors.push(...coercionErrors); } 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; + } } } } @@ -142,57 +138,71 @@ export function getArgumentValues( const name = argDef.name; const argType = argDef.type; const argumentNode = argNodeMap[name]; - const defaultValue = argDef.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 (argumentNode && argumentNode.value.kind === Kind.NULL) { - coercedValues[name] = null; - } - } 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 && 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 (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 if (!isInvalid(defaultValue)) { - coercedValues[name] = defaultValue; - } - } 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; 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/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/__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__/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 3c3e77f5d1..b44915313d 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/src/validation/rules/VariablesInAllowedPosition.js @@ -78,6 +78,11 @@ export function VariablesInAllowedPosition( /** * If varType is not non-null and defaultValue is provided and not null: * Let varType be the non-null of varType. + * + * Note: the explicit value null may still be explicitly provided as a variable + * value at runtime. While this validation rule could be more strict, this + * pattern was very common before the changed behavior of null values so it is + * still allowed. */ function effectiveType(varType, varDef) { if ( 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, From 3a5602ca70701640bd7b236384dd85fb3d4c7cf8 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Mon, 2 Apr 2018 19:50:12 -0700 Subject: [PATCH 3/5] Updated to most recent version of spec proposal. This is now *a breaking change*. The default validation rules are stricter, however with a configuration flag the previous lax behavior can be used which will ensure an existing service can support all existing incoming operations. For example to continue to support existing queries after updating to the new version, replace: ```js graphql({ schema, source }) ``` With: ```js graphql({ schema, source, options: { allowNullableVariablesInNonNullPositions: true }}) ``` Another more minor breaking change is that the final `typeInfo` argument to `validate` has moved positions. However very few should be reliant on this experimental arg. --- src/graphql.js | 22 +++- src/index.js | 1 + src/validation/ValidationContext.js | 18 +++ .../VariablesInAllowedPosition-test.js | 122 +++++++++++------- src/validation/__tests__/harness.js | 18 ++- src/validation/__tests__/validation-test.js | 2 +- src/validation/index.js | 1 + .../rules/VariablesInAllowedPosition.js | 28 ++-- src/validation/validate.js | 7 +- 9 files changed, 149 insertions(+), 70 deletions(-) diff --git a/src/graphql.js b/src/graphql.js index 8b0fe061f2..95e922bc14 100644 --- a/src/graphql.js +++ b/src/graphql.js @@ -9,13 +9,14 @@ import { validateSchema } from './type/validate'; import { parse } from './language/parser'; -import { validate } from './validation/validate'; +import { validate, specifiedRules } from './validation'; import { execute } from './execution/execute'; import type { ObjMap } from './jsutils/ObjMap'; -import type { Source } from './language/source'; +import type { ParseOptions, Source } from './language'; import type { GraphQLFieldResolver } from './type/definition'; import type { GraphQLSchema } from './type/schema'; import type { ExecutionResult } from './execution/execute'; +import type { ValidationOptions } from './validation'; import type { MaybePromise } from './jsutils/MaybePromise'; /** @@ -47,6 +48,9 @@ import type { MaybePromise } from './jsutils/MaybePromise'; * A resolver function to use when one is not provided by the schema. * If not provided, the default field resolver is used (which looks for a * value or method on the source value with the field's name). + * options: + * An additional set of flags which enable legacy, compataibility, or + * experimental behavior. */ export type GraphQLArgs = {| schema: GraphQLSchema, @@ -56,6 +60,7 @@ export type GraphQLArgs = {| variableValues?: ?ObjMap, operationName?: ?string, fieldResolver?: ?GraphQLFieldResolver, + options?: ParseOptions & ValidationOptions, |}; declare function graphql(GraphQLArgs, ..._: []): Promise; /* eslint-disable no-redeclare */ @@ -67,6 +72,7 @@ declare function graphql( variableValues?: ?ObjMap, operationName?: ?string, fieldResolver?: ?GraphQLFieldResolver, + options?: ParseOptions & ValidationOptions, ): Promise; export function graphql( argsOrSchema, @@ -76,6 +82,7 @@ export function graphql( variableValues, operationName, fieldResolver, + options, ) { /* eslint-enable no-redeclare */ // Always return a Promise for a consistent API. @@ -91,6 +98,7 @@ export function graphql( argsOrSchema.variableValues, argsOrSchema.operationName, argsOrSchema.fieldResolver, + argsOrSchema.options, ) : graphqlImpl( argsOrSchema, @@ -100,6 +108,7 @@ export function graphql( variableValues, operationName, fieldResolver, + options, ), ), ); @@ -121,6 +130,7 @@ declare function graphqlSync( variableValues?: ?ObjMap, operationName?: ?string, fieldResolver?: ?GraphQLFieldResolver, + options?: ParseOptions & ValidationOptions, ): ExecutionResult; export function graphqlSync( argsOrSchema, @@ -130,6 +140,7 @@ export function graphqlSync( variableValues, operationName, fieldResolver, + options, ) { // Extract arguments from object args if provided. const result = @@ -142,6 +153,7 @@ export function graphqlSync( argsOrSchema.variableValues, argsOrSchema.operationName, argsOrSchema.fieldResolver, + argsOrSchema.options, ) : graphqlImpl( argsOrSchema, @@ -151,6 +163,7 @@ export function graphqlSync( variableValues, operationName, fieldResolver, + options, ); // Assert that the execution was synchronous. @@ -169,6 +182,7 @@ function graphqlImpl( variableValues, operationName, fieldResolver, + options, ): MaybePromise { // Validate Schema const schemaValidationErrors = validateSchema(schema); @@ -179,13 +193,13 @@ function graphqlImpl( // Parse let document; try { - document = parse(source); + document = parse(source, options); } catch (syntaxError) { return { errors: [syntaxError] }; } // Validate - const validationErrors = validate(schema, document); + const validationErrors = validate(schema, document, specifiedRules, options); if (validationErrors.length > 0) { return { errors: validationErrors }; } diff --git a/src/index.js b/src/index.js index eaade28c45..3f6d152d7e 100644 --- a/src/index.js +++ b/src/index.js @@ -268,6 +268,7 @@ export { subscribe, createSourceEventStream } from './subscription'; // Validate GraphQL queries. export { validate, + ValidationOptions, ValidationContext, // All validation rules in the GraphQL Specification. specifiedRules, diff --git a/src/validation/ValidationContext.js b/src/validation/ValidationContext.js index 98f2e21cc4..14b56bc0bd 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -33,6 +33,21 @@ import { TypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; type VariableUsage = { node: VariableNode, type: ?GraphQLInputType }; +/** + * Configuration options to control validation behavior. + */ +export type ValidationOptions = { + /** + * If enabled, validation will allow nullable variables with default values + * to be used in non-null positions. Earlier versions explicitly allowed such + * operations due to a slightly different interpretation of default values and + * null values. GraphQL services accepting operations written before this + * version may continue to allow such operations by enabling this option, + * however GraphQL services established after this version should not. + */ + allowNullableVariablesInNonNullPositions?: boolean, +}; + /** * An instance of this class is passed as the "this" context to all validators, * allowing access to commonly useful contextual information from within a @@ -42,6 +57,7 @@ export default class ValidationContext { _schema: GraphQLSchema; _ast: DocumentNode; _typeInfo: TypeInfo; + options: ValidationOptions; _errors: Array; _fragments: ObjMap; _fragmentSpreads: Map>; @@ -59,10 +75,12 @@ export default class ValidationContext { schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo, + options?: ValidationOptions, ): void { this._schema = schema; this._ast = ast; this._typeInfo = typeInfo; + this.options = options || {}; this._errors = []; this._fragmentSpreads = new Map(); this._recursivelyReferencedFragments = new Map(); diff --git a/src/validation/__tests__/VariablesInAllowedPosition-test.js b/src/validation/__tests__/VariablesInAllowedPosition-test.js index 467ae5fa6e..8e04efce7e 100644 --- a/src/validation/__tests__/VariablesInAllowedPosition-test.js +++ b/src/validation/__tests__/VariablesInAllowedPosition-test.js @@ -6,7 +6,12 @@ */ import { describe, it } from 'mocha'; -import { expectPassesRule, expectFailsRule } from './harness'; +import { + expectPassesRule, + expectFailsRule, + expectPassesRuleWithOptions, + expectFailsRuleWithOptions, +} from './harness'; import { VariablesInAllowedPosition, badVarPosMessage, @@ -91,20 +96,6 @@ describe('Validate: Variables are in allowed positions', () => { ); }); - it('Int => Int! with non-null default value', () => { - expectPassesRule( - VariablesInAllowedPosition, - ` - query Query($intArg: Int = 1) - { - complicatedArgs { - nonNullIntArgField(nonNullIntArg: $intArg) - } - } - `, - ); - }); - it('[String] => [String]', () => { expectPassesRule( VariablesInAllowedPosition, @@ -201,18 +192,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, @@ -232,26 +211,6 @@ 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, @@ -393,4 +352,73 @@ describe('Validate: Variables are in allowed positions', () => { ], ); }); + + describe('allowNullableVariablesInNonNullPositions', () => { + it('Int => Int! with non-null default value without option', () => { + expectFailsRule( + VariablesInAllowedPosition, + ` + query Query($intVar: Int = 1) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + } + `, + [ + { + message: badVarPosMessage('intVar', 'Int', 'Int!'), + locations: [{ line: 2, column: 21 }, { line: 4, column: 47 }], + path: undefined, + }, + ], + ); + }); + + it('Int => Int! with null default value fails with option', () => { + expectFailsRuleWithOptions( + { allowNullableVariablesInNonNullPositions: true }, + VariablesInAllowedPosition, + ` + query Query($intVar: Int = null) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + } + `, + [ + { + message: badVarPosMessage('intVar', 'Int', 'Int!'), + locations: [{ line: 2, column: 23 }, { line: 4, column: 49 }], + path: undefined, + }, + ], + ); + }); + + it('Int => Int! with non-null default value with option', () => { + expectPassesRuleWithOptions( + { allowNullableVariablesInNonNullPositions: true }, + VariablesInAllowedPosition, + ` + query Query($intVar: Int = 1) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) + } + } + `, + ); + }); + + it('Boolean => Boolean! in directive with default value with option', () => { + expectPassesRuleWithOptions( + { allowNullableVariablesInNonNullPositions: true }, + 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 6d428faa79..a634a13a48 100644 --- a/src/validation/__tests__/harness.js +++ b/src/validation/__tests__/harness.js @@ -421,13 +421,15 @@ export const testSchema = new GraphQLSchema({ ], }); -function expectValid(schema, rules, queryString) { - const errors = validate(schema, parse(queryString), rules); +function expectValid(schema, rules, queryString, options) { + const ast = parse(queryString); + const errors = validate(schema, ast, rules, options); expect(errors).to.deep.equal([], 'Should validate'); } -function expectInvalid(schema, rules, queryString, expectedErrors) { - const errors = validate(schema, parse(queryString), rules); +function expectInvalid(schema, rules, queryString, expectedErrors, options) { + const ast = parse(queryString); + const errors = validate(schema, ast, rules, options); expect(errors).to.have.length.of.at.least(1, 'Should not validate'); expect(errors).to.deep.equal(expectedErrors); return errors; @@ -441,6 +443,14 @@ export function expectFailsRule(rule, queryString, errors) { return expectInvalid(testSchema, [rule], queryString, errors); } +export function expectPassesRuleWithOptions(options, rule, queryString) { + return expectValid(testSchema, [rule], queryString, options); +} + +export function expectFailsRuleWithOptions(options, rule, queryString, errors) { + return expectInvalid(testSchema, [rule], queryString, errors, options); +} + export function expectPassesRuleWithSchema(schema, rule, queryString, errors) { return expectValid(schema, [rule], queryString, errors); } diff --git a/src/validation/__tests__/validation-test.js b/src/validation/__tests__/validation-test.js index 56881dc843..3f105eda46 100644 --- a/src/validation/__tests__/validation-test.js +++ b/src/validation/__tests__/validation-test.js @@ -72,7 +72,7 @@ describe('Validate: Supports full validation', () => { } `); - const errors = validate(testSchema, ast, specifiedRules, typeInfo); + const errors = validate(testSchema, ast, specifiedRules, {}, typeInfo); const errorMessages = errors.map(err => err.message); diff --git a/src/validation/index.js b/src/validation/index.js index afcff6994b..6982e4e82b 100644 --- a/src/validation/index.js +++ b/src/validation/index.js @@ -12,6 +12,7 @@ export { validate } from './validate'; // https://github.com/tc39/proposal-export-default-from import ValidationContext from './ValidationContext'; export { ValidationContext }; +export type { ValidationOptions } from './ValidationContext'; export { specifiedRules } from './specifiedRules'; diff --git a/src/validation/rules/VariablesInAllowedPosition.js b/src/validation/rules/VariablesInAllowedPosition.js index b44915313d..a47590e328 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/src/validation/rules/VariablesInAllowedPosition.js @@ -53,11 +53,8 @@ export function VariablesInAllowedPosition( // If both are list types, the variable item type can be more strict // than the expected item type (contravariant). const schema = context.getSchema(); - const varType = typeFromAST(schema, varDef.type); - if ( - varType && - !isTypeSubTypeOf(schema, effectiveType(varType, varDef), type) - ) { + const varType = effectiveVariableType(context, schema, varDef); + if (varType && !isTypeSubTypeOf(schema, varType, type)) { context.reportError( new GraphQLError(badVarPosMessage(varName, varType, type), [ varDef, @@ -76,16 +73,23 @@ export function VariablesInAllowedPosition( } /** - * If varType is not non-null and defaultValue is provided and not null: - * Let varType be the non-null of varType. + * See "supporting legacy operations" sub-section of this validation + * rule specification. * - * Note: the explicit value null may still be explicitly provided as a variable - * value at runtime. While this validation rule could be more strict, this - * pattern was very common before the changed behavior of null values so it is - * still allowed. + * EffectiveVariableType(variableDefinition): + * * Let {variableType} be the expected type of {variableDefinition}. + * * If service supports operations written before this specification edition: + * * If {variableType} is not a non-null type: + * * Let {defaultValue} be the default value of {variableDefinition}. + * * If {defaultValue} is provided and not the value {null}: + * * Return the non-null of {variableType}. + * * Otherwise, return {variableType}. */ -function effectiveType(varType, varDef) { +function effectiveVariableType(context, schema, varDef) { + const varType = typeFromAST(schema, varDef.type); if ( + context.options.allowNullableVariablesInNonNullPositions && + varType && !isNonNullType(varType) && varDef.defaultValue && varDef.defaultValue.kind !== Kind.NULL diff --git a/src/validation/validate.js b/src/validation/validate.js index 61213f8b86..8d459480b3 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -16,7 +16,7 @@ import { GraphQLSchema } from '../type/schema'; import { assertValidSchema } from '../type/validate'; import { TypeInfo } from '../utilities/TypeInfo'; import { specifiedRules } from './specifiedRules'; -import ValidationContext from './ValidationContext'; +import ValidationContext, { type ValidationOptions } from './ValidationContext'; /** * Implements the "Validation" section of the spec. @@ -38,6 +38,7 @@ export function validate( schema: GraphQLSchema, ast: DocumentNode, rules?: $ReadOnlyArray, + options?: ValidationOptions, typeInfo?: TypeInfo, ): $ReadOnlyArray { invariant(ast, 'Must provide document'); @@ -48,6 +49,7 @@ export function validate( typeInfo || new TypeInfo(schema), ast, rules || specifiedRules, + options, ); } @@ -62,8 +64,9 @@ function visitUsingRules( typeInfo: TypeInfo, documentAST: DocumentNode, rules: $ReadOnlyArray<(ValidationContext) => ASTVisitor>, + options?: ValidationOptions, ): $ReadOnlyArray { - const context = new ValidationContext(schema, documentAST, typeInfo); + const context = new ValidationContext(schema, documentAST, typeInfo, options); const visitors = rules.map(rule => rule(context)); // Visit the whole document with each instance of all provided rules. visit(documentAST, visitWithTypeInfo(typeInfo, visitInParallel(visitors))); From e38d04204c359f1d16128d4fc277a92d23ee4e53 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Thu, 5 Apr 2018 21:13:16 -0700 Subject: [PATCH 4/5] Revert change requiring config flag & more specific allowedInPosition definition Based on discussion with @dschafer Adds getDefaultValue() to TypeInfo so the default value at any position in an AST visit is known. --- src/graphql.js | 22 ++----- src/index.js | 1 - src/utilities/TypeInfo.js | 22 ++++++- src/validation/ValidationContext.js | 30 +++------ .../VariablesInAllowedPosition-test.js | 60 ++++++------------ src/validation/__tests__/harness.js | 18 ++---- src/validation/__tests__/validation-test.js | 2 +- src/validation/index.js | 1 - .../rules/VariablesInAllowedPosition.js | 61 +++++++++++-------- src/validation/validate.js | 7 +-- 10 files changed, 96 insertions(+), 128 deletions(-) diff --git a/src/graphql.js b/src/graphql.js index 95e922bc14..8b0fe061f2 100644 --- a/src/graphql.js +++ b/src/graphql.js @@ -9,14 +9,13 @@ import { validateSchema } from './type/validate'; import { parse } from './language/parser'; -import { validate, specifiedRules } from './validation'; +import { validate } from './validation/validate'; import { execute } from './execution/execute'; import type { ObjMap } from './jsutils/ObjMap'; -import type { ParseOptions, Source } from './language'; +import type { Source } from './language/source'; import type { GraphQLFieldResolver } from './type/definition'; import type { GraphQLSchema } from './type/schema'; import type { ExecutionResult } from './execution/execute'; -import type { ValidationOptions } from './validation'; import type { MaybePromise } from './jsutils/MaybePromise'; /** @@ -48,9 +47,6 @@ import type { MaybePromise } from './jsutils/MaybePromise'; * A resolver function to use when one is not provided by the schema. * If not provided, the default field resolver is used (which looks for a * value or method on the source value with the field's name). - * options: - * An additional set of flags which enable legacy, compataibility, or - * experimental behavior. */ export type GraphQLArgs = {| schema: GraphQLSchema, @@ -60,7 +56,6 @@ export type GraphQLArgs = {| variableValues?: ?ObjMap, operationName?: ?string, fieldResolver?: ?GraphQLFieldResolver, - options?: ParseOptions & ValidationOptions, |}; declare function graphql(GraphQLArgs, ..._: []): Promise; /* eslint-disable no-redeclare */ @@ -72,7 +67,6 @@ declare function graphql( variableValues?: ?ObjMap, operationName?: ?string, fieldResolver?: ?GraphQLFieldResolver, - options?: ParseOptions & ValidationOptions, ): Promise; export function graphql( argsOrSchema, @@ -82,7 +76,6 @@ export function graphql( variableValues, operationName, fieldResolver, - options, ) { /* eslint-enable no-redeclare */ // Always return a Promise for a consistent API. @@ -98,7 +91,6 @@ export function graphql( argsOrSchema.variableValues, argsOrSchema.operationName, argsOrSchema.fieldResolver, - argsOrSchema.options, ) : graphqlImpl( argsOrSchema, @@ -108,7 +100,6 @@ export function graphql( variableValues, operationName, fieldResolver, - options, ), ), ); @@ -130,7 +121,6 @@ declare function graphqlSync( variableValues?: ?ObjMap, operationName?: ?string, fieldResolver?: ?GraphQLFieldResolver, - options?: ParseOptions & ValidationOptions, ): ExecutionResult; export function graphqlSync( argsOrSchema, @@ -140,7 +130,6 @@ export function graphqlSync( variableValues, operationName, fieldResolver, - options, ) { // Extract arguments from object args if provided. const result = @@ -153,7 +142,6 @@ export function graphqlSync( argsOrSchema.variableValues, argsOrSchema.operationName, argsOrSchema.fieldResolver, - argsOrSchema.options, ) : graphqlImpl( argsOrSchema, @@ -163,7 +151,6 @@ export function graphqlSync( variableValues, operationName, fieldResolver, - options, ); // Assert that the execution was synchronous. @@ -182,7 +169,6 @@ function graphqlImpl( variableValues, operationName, fieldResolver, - options, ): MaybePromise { // Validate Schema const schemaValidationErrors = validateSchema(schema); @@ -193,13 +179,13 @@ function graphqlImpl( // Parse let document; try { - document = parse(source, options); + document = parse(source); } catch (syntaxError) { return { errors: [syntaxError] }; } // Validate - const validationErrors = validate(schema, document, specifiedRules, options); + const validationErrors = validate(schema, document); if (validationErrors.length > 0) { return { errors: validationErrors }; } diff --git a/src/index.js b/src/index.js index 3f6d152d7e..eaade28c45 100644 --- a/src/index.js +++ b/src/index.js @@ -268,7 +268,6 @@ export { subscribe, createSourceEventStream } from './subscription'; // Validate GraphQL queries. export { validate, - ValidationOptions, ValidationContext, // All validation rules in the GraphQL Specification. specifiedRules, 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/validation/ValidationContext.js b/src/validation/ValidationContext.js index 14b56bc0bd..1d6ef75741 100644 --- a/src/validation/ValidationContext.js +++ b/src/validation/ValidationContext.js @@ -31,22 +31,11 @@ import type { GraphQLDirective } from '../type/directives'; import { TypeInfo } from '../utilities/TypeInfo'; type NodeWithSelectionSet = OperationDefinitionNode | FragmentDefinitionNode; -type VariableUsage = { node: VariableNode, type: ?GraphQLInputType }; - -/** - * Configuration options to control validation behavior. - */ -export type ValidationOptions = { - /** - * If enabled, validation will allow nullable variables with default values - * to be used in non-null positions. Earlier versions explicitly allowed such - * operations due to a slightly different interpretation of default values and - * null values. GraphQL services accepting operations written before this - * version may continue to allow such operations by enabling this option, - * however GraphQL services established after this version should not. - */ - allowNullableVariablesInNonNullPositions?: boolean, -}; +type VariableUsage = {| + +node: VariableNode, + +type: ?GraphQLInputType, + +defaultValue: ?mixed, +|}; /** * An instance of this class is passed as the "this" context to all validators, @@ -57,7 +46,6 @@ export default class ValidationContext { _schema: GraphQLSchema; _ast: DocumentNode; _typeInfo: TypeInfo; - options: ValidationOptions; _errors: Array; _fragments: ObjMap; _fragmentSpreads: Map>; @@ -75,12 +63,10 @@ export default class ValidationContext { schema: GraphQLSchema, ast: DocumentNode, typeInfo: TypeInfo, - options?: ValidationOptions, ): void { this._schema = schema; this._ast = ast; this._typeInfo = typeInfo; - this.options = options || {}; this._errors = []; this._fragmentSpreads = new Map(); this._recursivelyReferencedFragments = new Map(); @@ -181,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__/VariablesInAllowedPosition-test.js b/src/validation/__tests__/VariablesInAllowedPosition-test.js index 8e04efce7e..c8b40e90ee 100644 --- a/src/validation/__tests__/VariablesInAllowedPosition-test.js +++ b/src/validation/__tests__/VariablesInAllowedPosition-test.js @@ -6,12 +6,7 @@ */ import { describe, it } from 'mocha'; -import { - expectPassesRule, - expectFailsRule, - expectPassesRuleWithOptions, - expectFailsRuleWithOptions, -} from './harness'; +import { expectPassesRule, expectFailsRule } from './harness'; import { VariablesInAllowedPosition, badVarPosMessage, @@ -353,71 +348,56 @@ describe('Validate: Variables are in allowed positions', () => { ); }); - describe('allowNullableVariablesInNonNullPositions', () => { - it('Int => Int! with non-null default value without option', () => { + describe('Allows optional (nullable) variables with default values', () => { + it('Int => Int! fails when variable provides null default value', () => { expectFailsRule( VariablesInAllowedPosition, ` - query Query($intVar: Int = 1) { + query Query($intVar: Int = null) { complicatedArgs { nonNullIntArgField(nonNullIntArg: $intVar) } - } - `, + }`, [ { message: badVarPosMessage('intVar', 'Int', 'Int!'), locations: [{ line: 2, column: 21 }, { line: 4, column: 47 }], - path: undefined, }, ], ); }); - it('Int => Int! with null default value fails with option', () => { - expectFailsRuleWithOptions( - { allowNullableVariablesInNonNullPositions: true }, + it('Int => Int! when variable provides non-null default value', () => { + expectPassesRule( VariablesInAllowedPosition, ` - query Query($intVar: Int = null) { - complicatedArgs { - nonNullIntArgField(nonNullIntArg: $intVar) - } + query Query($intVar: Int = 1) { + complicatedArgs { + nonNullIntArgField(nonNullIntArg: $intVar) } - `, - [ - { - message: badVarPosMessage('intVar', 'Int', 'Int!'), - locations: [{ line: 2, column: 23 }, { line: 4, column: 49 }], - path: undefined, - }, - ], + }`, ); }); - it('Int => Int! with non-null default value with option', () => { - expectPassesRuleWithOptions( - { allowNullableVariablesInNonNullPositions: true }, + it('Int => Int! when optional argument provides default value', () => { + expectPassesRule( VariablesInAllowedPosition, ` - query Query($intVar: Int = 1) { + query Query($intVar: Int) { complicatedArgs { - nonNullIntArgField(nonNullIntArg: $intVar) + nonNullFieldWithDefault(nonNullIntArg: $intVar) } - } - `, + }`, ); }); it('Boolean => Boolean! in directive with default value with option', () => { - expectPassesRuleWithOptions( - { allowNullableVariablesInNonNullPositions: true }, + expectPassesRule( VariablesInAllowedPosition, ` - query Query($boolVar: Boolean = false) { - dog @include(if: $boolVar) - } - `, + query Query($boolVar: Boolean = false) { + dog @include(if: $boolVar) + }`, ); }); }); diff --git a/src/validation/__tests__/harness.js b/src/validation/__tests__/harness.js index a634a13a48..6d428faa79 100644 --- a/src/validation/__tests__/harness.js +++ b/src/validation/__tests__/harness.js @@ -421,15 +421,13 @@ export const testSchema = new GraphQLSchema({ ], }); -function expectValid(schema, rules, queryString, options) { - const ast = parse(queryString); - const errors = validate(schema, ast, rules, options); +function expectValid(schema, rules, queryString) { + const errors = validate(schema, parse(queryString), rules); expect(errors).to.deep.equal([], 'Should validate'); } -function expectInvalid(schema, rules, queryString, expectedErrors, options) { - const ast = parse(queryString); - const errors = validate(schema, ast, rules, options); +function expectInvalid(schema, rules, queryString, expectedErrors) { + const errors = validate(schema, parse(queryString), rules); expect(errors).to.have.length.of.at.least(1, 'Should not validate'); expect(errors).to.deep.equal(expectedErrors); return errors; @@ -443,14 +441,6 @@ export function expectFailsRule(rule, queryString, errors) { return expectInvalid(testSchema, [rule], queryString, errors); } -export function expectPassesRuleWithOptions(options, rule, queryString) { - return expectValid(testSchema, [rule], queryString, options); -} - -export function expectFailsRuleWithOptions(options, rule, queryString, errors) { - return expectInvalid(testSchema, [rule], queryString, errors, options); -} - export function expectPassesRuleWithSchema(schema, rule, queryString, errors) { return expectValid(schema, [rule], queryString, errors); } diff --git a/src/validation/__tests__/validation-test.js b/src/validation/__tests__/validation-test.js index 3f105eda46..56881dc843 100644 --- a/src/validation/__tests__/validation-test.js +++ b/src/validation/__tests__/validation-test.js @@ -72,7 +72,7 @@ describe('Validate: Supports full validation', () => { } `); - const errors = validate(testSchema, ast, specifiedRules, {}, typeInfo); + const errors = validate(testSchema, ast, specifiedRules, typeInfo); const errorMessages = errors.map(err => err.message); diff --git a/src/validation/index.js b/src/validation/index.js index 6982e4e82b..afcff6994b 100644 --- a/src/validation/index.js +++ b/src/validation/index.js @@ -12,7 +12,6 @@ export { validate } from './validate'; // https://github.com/tc39/proposal-export-default-from import ValidationContext from './ValidationContext'; export { ValidationContext }; -export type { ValidationOptions } from './ValidationContext'; export { specifiedRules } from './specifiedRules'; diff --git a/src/validation/rules/VariablesInAllowedPosition.js b/src/validation/rules/VariablesInAllowedPosition.js index a47590e328..12e92c2b02 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/src/validation/rules/VariablesInAllowedPosition.js @@ -10,11 +10,13 @@ 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, @@ -43,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) { @@ -53,8 +55,17 @@ export function VariablesInAllowedPosition( // If both are list types, the variable item type can be more strict // than the expected item type (contravariant). const schema = context.getSchema(); - const varType = effectiveVariableType(context, schema, varDef); - if (varType && !isTypeSubTypeOf(schema, varType, type)) { + const varType = typeFromAST(schema, varDef.type); + if ( + varType && + !allowedInPosition( + schema, + varType, + varDef.defaultValue, + type, + defaultValue, + ) + ) { context.reportError( new GraphQLError(badVarPosMessage(varName, varType, type), [ varDef, @@ -73,28 +84,26 @@ export function VariablesInAllowedPosition( } /** - * See "supporting legacy operations" sub-section of this validation - * rule specification. - * - * EffectiveVariableType(variableDefinition): - * * Let {variableType} be the expected type of {variableDefinition}. - * * If service supports operations written before this specification edition: - * * If {variableType} is not a non-null type: - * * Let {defaultValue} be the default value of {variableDefinition}. - * * If {defaultValue} is provided and not the value {null}: - * * Return the non-null of {variableType}. - * * Otherwise, return {variableType}. + * Returns true if the variable is allowed in the position it was found, + * which includes considering if default values exist for either the variable + * or the location at which it is located. */ -function effectiveVariableType(context, schema, varDef) { - const varType = typeFromAST(schema, varDef.type); - if ( - context.options.allowNullableVariablesInNonNullPositions && - varType && - !isNonNullType(varType) && - varDef.defaultValue && - varDef.defaultValue.kind !== Kind.NULL - ) { - return GraphQLNonNull(varType); +function allowedInPosition( + schema: GraphQLSchema, + varType: GraphQLType, + varDefaultValue: ?ValueNode, + locationType: GraphQLType, + locationDefaultValue: ?mixed, +): boolean { + if (isNonNullType(locationType) && !isNonNullType(varType)) { + const hasLocationDefaultValue = locationDefaultValue !== undefined; + const hasNonNullVariableDefaultValue = + varDefaultValue && varDefaultValue.kind !== Kind.NULL; + if (!hasLocationDefaultValue && !hasNonNullVariableDefaultValue) { + return false; + } + const locationNullableType = locationType.ofType; + return isTypeSubTypeOf(schema, varType, locationNullableType); } - return varType; + return isTypeSubTypeOf(schema, varType, locationType); } diff --git a/src/validation/validate.js b/src/validation/validate.js index 8d459480b3..61213f8b86 100644 --- a/src/validation/validate.js +++ b/src/validation/validate.js @@ -16,7 +16,7 @@ import { GraphQLSchema } from '../type/schema'; import { assertValidSchema } from '../type/validate'; import { TypeInfo } from '../utilities/TypeInfo'; import { specifiedRules } from './specifiedRules'; -import ValidationContext, { type ValidationOptions } from './ValidationContext'; +import ValidationContext from './ValidationContext'; /** * Implements the "Validation" section of the spec. @@ -38,7 +38,6 @@ export function validate( schema: GraphQLSchema, ast: DocumentNode, rules?: $ReadOnlyArray, - options?: ValidationOptions, typeInfo?: TypeInfo, ): $ReadOnlyArray { invariant(ast, 'Must provide document'); @@ -49,7 +48,6 @@ export function validate( typeInfo || new TypeInfo(schema), ast, rules || specifiedRules, - options, ); } @@ -64,9 +62,8 @@ function visitUsingRules( typeInfo: TypeInfo, documentAST: DocumentNode, rules: $ReadOnlyArray<(ValidationContext) => ASTVisitor>, - options?: ValidationOptions, ): $ReadOnlyArray { - const context = new ValidationContext(schema, documentAST, typeInfo, options); + const context = new ValidationContext(schema, documentAST, typeInfo); const visitors = rules.map(rule => rule(context)); // Visit the whole document with each instance of all provided rules. visit(documentAST, visitWithTypeInfo(typeInfo, visitInParallel(visitors))); From 4c17d15d9210bfee31e1e9e106bdb4beaae44317 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Fri, 6 Apr 2018 12:48:16 -0700 Subject: [PATCH 5/5] Minor edits to match spec text --- src/validation/rules/VariablesInAllowedPosition.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/validation/rules/VariablesInAllowedPosition.js b/src/validation/rules/VariablesInAllowedPosition.js index 12e92c2b02..20805feb49 100644 --- a/src/validation/rules/VariablesInAllowedPosition.js +++ b/src/validation/rules/VariablesInAllowedPosition.js @@ -58,7 +58,7 @@ export function VariablesInAllowedPosition( const varType = typeFromAST(schema, varDef.type); if ( varType && - !allowedInPosition( + !allowedVariableUsage( schema, varType, varDef.defaultValue, @@ -84,11 +84,11 @@ export function VariablesInAllowedPosition( } /** - * Returns true if the variable is allowed in the position it was found, + * 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 allowedInPosition( +function allowedVariableUsage( schema: GraphQLSchema, varType: GraphQLType, varDefaultValue: ?ValueNode, @@ -96,14 +96,14 @@ function allowedInPosition( locationDefaultValue: ?mixed, ): boolean { if (isNonNullType(locationType) && !isNonNullType(varType)) { - const hasLocationDefaultValue = locationDefaultValue !== undefined; const hasNonNullVariableDefaultValue = varDefaultValue && varDefaultValue.kind !== Kind.NULL; - if (!hasLocationDefaultValue && !hasNonNullVariableDefaultValue) { + const hasLocationDefaultValue = locationDefaultValue !== undefined; + if (!hasNonNullVariableDefaultValue && !hasLocationDefaultValue) { return false; } - const locationNullableType = locationType.ofType; - return isTypeSubTypeOf(schema, varType, locationNullableType); + const nullableLocationType = locationType.ofType; + return isTypeSubTypeOf(schema, varType, nullableLocationType); } return isTypeSubTypeOf(schema, varType, locationType); }