diff --git a/src/execution/__tests__/oneof-test.ts b/src/execution/__tests__/oneof-test.ts deleted file mode 100644 index f4a11f8997..0000000000 --- a/src/execution/__tests__/oneof-test.ts +++ /dev/null @@ -1,185 +0,0 @@ -import { describe, it } from 'mocha'; - -import { expectJSON } from '../../__testUtils__/expectJSON.js'; - -import { parse } from '../../language/parser.js'; - -import { buildSchema } from '../../utilities/buildASTSchema.js'; - -import { execute } from '../execute.js'; -import type { ExecutionResult } from '../types.js'; - -const schema = buildSchema(` - type Query { - test(input: TestInputObject!): TestObject - } - - input TestInputObject @oneOf { - a: String - b: Int - } - - type TestObject { - a: String - b: Int - } -`); - -function executeQuery( - query: string, - rootValue: unknown, - variableValues?: { [variable: string]: unknown }, -): ExecutionResult | Promise { - return execute({ schema, document: parse(query), rootValue, variableValues }); -} - -describe('Execute: Handles OneOf Input Objects', () => { - describe('OneOf Input Objects', () => { - const rootValue = { - test({ input }: { input: { a?: string; b?: number } }) { - return input; - }, - }; - - it('accepts a good default value', () => { - const query = ` - query ($input: TestInputObject! = {a: "abc"}) { - test(input: $input) { - a - b - } - } - `; - const result = executeQuery(query, rootValue); - - expectJSON(result).toDeepEqual({ - data: { - test: { - a: 'abc', - b: null, - }, - }, - }); - }); - - it('rejects a bad default value', () => { - const query = ` - query ($input: TestInputObject! = {a: "abc", b: 123}) { - test(input: $input) { - a - b - } - } - `; - const result = executeQuery(query, rootValue); - - expectJSON(result).toDeepEqual({ - data: { - test: null, - }, - errors: [ - { - locations: [{ column: 23, line: 3 }], - message: - // This type of error would be caught at validation-time - // hence the vague error message here. - 'Argument "input" of non-null type "TestInputObject!" must not be null.', - path: ['test'], - }, - ], - }); - }); - - it('accepts a good variable', () => { - const query = ` - query ($input: TestInputObject!) { - test(input: $input) { - a - b - } - } - `; - const result = executeQuery(query, rootValue, { input: { a: 'abc' } }); - - expectJSON(result).toDeepEqual({ - data: { - test: { - a: 'abc', - b: null, - }, - }, - }); - }); - - it('accepts a good variable with an undefined key', () => { - const query = ` - query ($input: TestInputObject!) { - test(input: $input) { - a - b - } - } - `; - const result = executeQuery(query, rootValue, { - input: { a: 'abc', b: undefined }, - }); - - expectJSON(result).toDeepEqual({ - data: { - test: { - a: 'abc', - b: null, - }, - }, - }); - }); - - it('rejects a variable with multiple non-null keys', () => { - const query = ` - query ($input: TestInputObject!) { - test(input: $input) { - a - b - } - } - `; - const result = executeQuery(query, rootValue, { - input: { a: 'abc', b: 123 }, - }); - - expectJSON(result).toDeepEqual({ - errors: [ - { - locations: [{ column: 16, line: 2 }], - message: - 'Variable "$input" got invalid value { a: "abc", b: 123 }; Exactly one key must be specified for OneOf type "TestInputObject".', - }, - ], - }); - }); - - it('rejects a variable with multiple nullable keys', () => { - const query = ` - query ($input: TestInputObject!) { - test(input: $input) { - a - b - } - } - `; - const result = executeQuery(query, rootValue, { - input: { a: 'abc', b: null }, - }); - - expectJSON(result).toDeepEqual({ - errors: [ - { - locations: [{ column: 16, line: 2 }], - message: - 'Variable "$input" got invalid value { a: "abc", b: null }; Exactly one key must be specified for OneOf type "TestInputObject".', - }, - ], - }); - }); - }); -}); diff --git a/src/execution/__tests__/variables-test.ts b/src/execution/__tests__/variables-test.ts index 7fe5a8a10d..5810a77251 100644 --- a/src/execution/__tests__/variables-test.ts +++ b/src/execution/__tests__/variables-test.ts @@ -27,7 +27,11 @@ import { GraphQLDirective, GraphQLIncludeDirective, } from '../../type/directives.js'; -import { GraphQLBoolean, GraphQLString } from '../../type/scalars.js'; +import { + GraphQLBoolean, + GraphQLInt, + GraphQLString, +} from '../../type/scalars.js'; import { GraphQLSchema } from '../../type/schema.js'; import { executeSync, experimentalExecuteIncrementally } from '../execute.js'; @@ -90,6 +94,15 @@ const TestNestedInputObject = new GraphQLInputObjectType({ }, }); +const TestOneOfInputObject = new GraphQLInputObjectType({ + name: 'TestOneOfInputObject', + isOneOf: true, + fields: { + a: { type: GraphQLString }, + b: { type: GraphQLInt }, + }, +}); + const TestEnum = new GraphQLEnumType({ name: 'TestEnum', values: { @@ -140,6 +153,9 @@ const TestType = new GraphQLObjectType({ type: TestNestedInputObject, defaultValue: 'Hello World', }), + fieldWithOneOfObjectInput: fieldWithInputArg({ + type: TestOneOfInputObject, + }), list: fieldWithInputArg({ type: new GraphQLList(GraphQLString) }), nested: { type: NestedType, @@ -1114,6 +1130,270 @@ describe('Execute: Handles inputs', () => { }); }); + describe('Handles OneOf Input Object types', () => { + it('allows OneOf Input Object with single field', () => { + const result = executeQuery(` + { + fieldWithOneOfObjectInput(input: { a: "abc" }) + } + `); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: '{ a: "abc" }', + }, + }); + }); + + it('errors with OneOf Input Object with more than one field', () => { + const result = executeQuery(` + { + fieldWithOneOfObjectInput(input: { a: "abc", b: 123 }) + } + `); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: null, + }, + errors: [ + { + message: + 'Argument "input" of type "TestOneOfInputObject" has invalid value { a: "abc", b: 123 }.', + path: ['fieldWithOneOfObjectInput'], + locations: [{ line: 3, column: 44 }], + }, + ], + }); + }); + + it('errors with OneOf Input Object with no fields', () => { + const result = executeQuery(` + { + fieldWithOneOfObjectInput(input: {}) + } + `); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: null, + }, + errors: [ + { + message: + 'Argument "input" of type "TestOneOfInputObject" has invalid value { }.', + path: ['fieldWithOneOfObjectInput'], + locations: [{ line: 3, column: 44 }], + }, + ], + }); + }); + + it('errors with OneOf Input Object with a single null value', () => { + const result = executeQuery(` + { + fieldWithOneOfObjectInput(input: { a: null }) + } + `); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: null, + }, + errors: [ + { + message: + 'Argument "input" of type "TestOneOfInputObject" has invalid value { a: null }.', + path: ['fieldWithOneOfObjectInput'], + locations: [{ line: 3, column: 44 }], + }, + ], + }); + }); + + it('errors with OneOf Input Object with multiple values, only one non-null', () => { + const result = executeQuery(` + { + fieldWithOneOfObjectInput(input: { a: "abc", b: null }) + } + `); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: null, + }, + errors: [ + { + message: + 'Argument "input" of type "TestOneOfInputObject" has invalid value { a: "abc", b: null }.', + path: ['fieldWithOneOfObjectInput'], + locations: [{ line: 3, column: 44 }], + }, + ], + }); + }); + + it('allows a variable for the entire OneOf Object with a single value', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject) { + fieldWithOneOfObjectInput(input: $input) + } + `, + { input: { a: 'abc' } }, + ); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: '{ a: "abc" }', + }, + }); + }); + + it('allows a variable for the entire OneOf Object with a single defined value and additional undefined value', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject) { + fieldWithOneOfObjectInput(input: $input) + } + `, + // Note: this is non-normative graphql-js specific behavior. + { input: { a: 'abc', b: undefined } }, + ); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: '{ a: "abc" }', + }, + }); + }); + + it('errors with variable object with no fields', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject) { + fieldWithOneOfObjectInput(input: $input) + } + `, + { input: {} }, + ); + + expectJSON(result).toDeepEqual({ + errors: [ + { + message: + 'Variable "$input" got invalid value {}; Within OneOf Input Object type "TestOneOfInputObject", exactly one field must be specified, and the value for that field must be non-null.', + locations: [{ line: 2, column: 18 }], + }, + ], + }); + }); + + it('errors with variable object with multiple fields', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject) { + fieldWithOneOfObjectInput(input: $input) + } + `, + { input: { a: 'abc', b: 123 } }, + ); + + expectJSON(result).toDeepEqual({ + errors: [ + { + message: + 'Variable "$input" got invalid value { a: "abc", b: 123 }; Within OneOf Input Object type "TestOneOfInputObject", exactly one field must be specified, and the value for that field must be non-null.', + locations: [{ line: 2, column: 18 }], + }, + ], + }); + }); + + it('errors with variable object with single null field', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject) { + fieldWithOneOfObjectInput(input: $input) + } + `, + { input: { a: null } }, + ); + + expectJSON(result).toDeepEqual({ + errors: [ + { + message: + 'Variable "$input" got invalid value null at "input.a"; Within OneOf Input Object type "TestOneOfInputObject", exactly one field must be specified, and the value for that field must be non-null.', + locations: [{ line: 2, column: 18 }], + }, + ], + }); + }); + + it('errors with variable object with multiple fields, only one non-null', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject) { + fieldWithOneOfObjectInput(input: $input) + } + `, + { input: { a: 'abc', b: null } }, + ); + + expectJSON(result).toDeepEqual({ + errors: [ + { + message: + 'Variable "$input" got invalid value { a: "abc", b: null }; Within OneOf Input Object type "TestOneOfInputObject", exactly one field must be specified, and the value for that field must be non-null.', + locations: [{ line: 2, column: 18 }], + }, + ], + }); + }); + + it('accepts a good default value', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject = { a: "abc" }) { + fieldWithOneOfObjectInput(input: $input) + } + `, + ); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: '{ a: "abc" }', + }, + }); + }); + + it('rejects a bad default value', () => { + const result = executeQuery( + ` + query ($input: TestOneOfInputObject = { a: "abc", b: 123 }) { + fieldWithOneOfObjectInput(input: $input) + } + `, + ); + + expectJSON(result).toDeepEqual({ + data: { + fieldWithOneOfObjectInput: null, + }, + errors: [ + { + locations: [{ line: 3, column: 46 }], + message: + // Default values are not currently validated separately, hence the vague error message here. + 'Argument "input" of type "TestOneOfInputObject" has invalid value $input.', + path: ['fieldWithOneOfObjectInput'], + }, + ], + }); + }); + }); + describe('getVariableValues: limit maximum number of coercion errors', () => { const doc = parse(` query ($input: [String!]) { diff --git a/src/utilities/__tests__/coerceInputValue-test.ts b/src/utilities/__tests__/coerceInputValue-test.ts index 15467e8456..99c6b55007 100644 --- a/src/utilities/__tests__/coerceInputValue-test.ts +++ b/src/utilities/__tests__/coerceInputValue-test.ts @@ -302,29 +302,60 @@ describe('coerceInputValue', () => { expectValue(result).to.deep.equal({ foo: 123 }); }); + it('returns an error if no fields are specified', () => { + const result = coerceValue({}, TestInputObject); + expectErrors(result).to.deep.equal([ + { + error: + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', + path: [], + value: {}, + }, + ]); + }); + it('returns an error if more than one field is specified', () => { - const result = coerceValue({ foo: 123, bar: null }, TestInputObject); + const result = coerceValue({ foo: 123, bar: 456 }, TestInputObject); expectErrors(result).to.deep.equal([ { error: - 'Exactly one key must be specified for OneOf type "TestInputObject".', + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', path: [], - value: { foo: 123, bar: null }, + value: { foo: 123, bar: 456 }, }, ]); }); - it('returns an error the one field is null', () => { + it('returns an error if the one field is null', () => { const result = coerceValue({ bar: null }, TestInputObject); expectErrors(result).to.deep.equal([ { - error: 'Field "bar" must be non-null.', + error: + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', path: ['bar'], value: null, }, ]); }); + it('returns an error if there are multiple fields with only a single non-null', () => { + const result = coerceValue({ foo: 123, bar: null }, TestInputObject); + expectErrors(result).to.deep.equal([ + { + error: + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', + path: [], + value: { foo: 123, bar: null }, + }, + ]); + }); + + // special non-normative graphql-js behavior + it('returns no error for an additional fields with value of undefined', () => { + const result = coerceValue({ foo: 123, bar: undefined }, TestInputObject); + expectValue(result).to.deep.equal({ foo: 123 }); + }); + it('returns an error for an invalid field', () => { const result = coerceValue({ foo: NaN }, TestInputObject); expectErrors(result).to.deep.equal([ @@ -351,7 +382,7 @@ describe('coerceInputValue', () => { }, { error: - 'Exactly one key must be specified for OneOf type "TestInputObject".', + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', path: [], value: { foo: 'abc', bar: 'def' }, }, @@ -370,6 +401,12 @@ describe('coerceInputValue', () => { path: [], value: { foo: 123, unknownField: 123 }, }, + { + error: + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', + path: [], + value: { foo: 123, unknownField: 123 }, + }, ]); }); @@ -384,7 +421,7 @@ describe('coerceInputValue', () => { }, { error: - 'Exactly one key must be specified for OneOf type "TestInputObject".', + 'Within OneOf Input Object type "TestInputObject", exactly one field must be specified, and the value for that field must be non-null.', path: [], value: { bart: 123 }, }, @@ -785,6 +822,25 @@ describe('coerceInputLiteral', () => { testWithVariables({ var: null }, '$var', nonNullBool, undefined); }); + it('accepts variable values for fields of OneOf Input Objects', () => { + testWithVariables({ a: 'abc' }, '{ a: $a }', testOneOfInputObj, { + a: 'abc', + }); + testWithVariables({ a: null }, '{ a: $a }', testOneOfInputObj, undefined); + testWithVariables( + { a: 'abc', b: 'def' }, + '{ a: $a, b: $b }', + testOneOfInputObj, + undefined, + ); + testWithVariables( + { a: 'abc', b: null }, + '{ a: $a, b: $b }', + testOneOfInputObj, + undefined, + ); + }); + it('asserts variables are provided as items in lists', () => { test('[ $foo ]', listOfBool, [null]); test('[ $foo ]', listOfNonNullBool, undefined); diff --git a/src/utilities/coerceInputValue.ts b/src/utilities/coerceInputValue.ts index 88c97c8405..6d33386054 100644 --- a/src/utilities/coerceInputValue.ts +++ b/src/utilities/coerceInputValue.ts @@ -134,8 +134,28 @@ function coerceInputValueImpl( ); } + /** + * Keys with the value "undefined" do not appear within the coerced input + * object, but do appear within the original input object. + * + * Note: in graphql-js, supplying the value "undefined" for a variable + * (as opposed to entirely omitting the variable from the map) will cause + * the variable value to be coerced to "null", but supplying "undefined" + * for an input object field will cause the field to be dropped as if the + * field did not exist in the map. We could consider changing this for + * input object fields to match the behavior of scalars variables and + * output variables where an actual value of "undefined" is considered to + * have the same intent as the "null" value. + * + * This use of `fieldsWithDefinedValues` is a workaround for this JavaScript- + * specific behavior. + * + * See: https://github.com/graphql/graphql-js/issues/2533 + */ + let fieldsWithDefinedValues = 0; + const keys = Object.keys(inputValue); // Ensure every provided field is defined. - for (const fieldName of Object.keys(inputValue)) { + for (const fieldName of keys) { if (fieldDefs[fieldName] == null) { const suggestions = suggestionList( fieldName, @@ -150,28 +170,34 @@ function coerceInputValueImpl( ), ); } + + if (inputValue[fieldName] !== undefined) { + fieldsWithDefinedValues++; + } } if (type.isOneOf) { - const keys = Object.keys(coercedValue); - if (keys.length !== 1) { + const coercedKeys = Object.keys(coercedValue); + if (fieldsWithDefinedValues !== 1 || coercedKeys.length !== 1) { onError( pathToArray(path), inputValue, new GraphQLError( - `Exactly one key must be specified for OneOf type "${type}".`, + `Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, ), ); - } - - const key = keys[0]; - const value = coercedValue[key]; - if (value === null) { - onError( - pathToArray(path).concat(key), - value, - new GraphQLError(`Field "${key}" must be non-null.`), - ); + } else { + const key = keys[0]; + const value = inputValue[key]; + if (value === null || coercedValue[key] === null) { + onError( + pathToArray(path).concat(key), + value, + new GraphQLError( + `Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, + ), + ); + } } } @@ -344,13 +370,14 @@ export function coerceInputLiteral( } if (type.isOneOf) { - const keys = Object.keys(coercedValue); - if (keys.length !== 1) { + const coercedKeys = Object.keys(coercedValue); + if (fieldNodes.size !== 1 || coercedKeys.length !== 1) { return; // Invalid: not exactly one key, intentionally return no value. } - - if (coercedValue[keys[0]] === null) { - return; // Invalid: value not non-null, intentionally return no value. + for (const [key, value] of fieldNodes) { + if (value.value.kind === Kind.NULL || coercedValue[key] === null) { + return; // Invalid: not exactly one key, intentionally return no value. + } } } diff --git a/src/utilities/valueFromAST.ts b/src/utilities/valueFromAST.ts index add9153680..0cf6d8c601 100644 --- a/src/utilities/valueFromAST.ts +++ b/src/utilities/valueFromAST.ts @@ -129,13 +129,15 @@ export function valueFromAST( } if (type.isOneOf) { - const keys = Object.keys(coercedObj); - if (keys.length !== 1) { + const coercedKeys = Object.keys(coercedObj); + if (fieldNodes.size !== 1 || coercedKeys.length !== 1) { return; // Invalid: not exactly one key, intentionally return no value. } - if (coercedObj[keys[0]] === null) { - return; // Invalid: value not non-null, intentionally return no value. + const fieldNode = valueNode.fields[0]; + const key = fieldNode.name.value; + if (fieldNode.value.kind === Kind.NULL || coercedObj[key] === null) { + return; // Invalid: not exactly one key, intentionally return no value. } } diff --git a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts index 819d103e6a..53a771da24 100644 --- a/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts +++ b/src/validation/__tests__/ValuesOfCorrectTypeRule-test.ts @@ -1088,7 +1088,8 @@ describe('Validate: Values of correct type', () => { } `).toDeepEqual([ { - message: 'Field "OneOfInput.stringField" must be non-null.', + message: + 'Within OneOf Input Object type "OneOfInput", exactly one field must be specified, and the value for that field must be non-null.', locations: [{ line: 4, column: 37 }], }, ]); @@ -1104,7 +1105,7 @@ describe('Validate: Values of correct type', () => { `).toDeepEqual([ { message: - 'Variable "$string" must be non-nullable to be used for OneOf Input Object "OneOfInput".', + 'Variable "$string" must be non-nullable to be used within OneOf Input Object type "OneOfInput".', locations: [{ line: 4, column: 37 }], }, ]); @@ -1120,7 +1121,7 @@ describe('Validate: Values of correct type', () => { `).toDeepEqual([ { message: - 'OneOf Input Object "OneOfInput" must specify exactly one key.', + 'Within OneOf Input Object type "OneOfInput", exactly one field must be specified, and the value for that field must be non-null.', locations: [{ line: 4, column: 37 }], }, ]); diff --git a/src/validation/rules/ValuesOfCorrectTypeRule.ts b/src/validation/rules/ValuesOfCorrectTypeRule.ts index a86a66bd47..550bda562e 100644 --- a/src/validation/rules/ValuesOfCorrectTypeRule.ts +++ b/src/validation/rules/ValuesOfCorrectTypeRule.ts @@ -193,7 +193,7 @@ function validateOneOfInputObject( if (isNotExactlyOneField) { context.reportError( new GraphQLError( - `OneOf Input Object "${type}" must specify exactly one key.`, + `Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, { nodes: [node] }, ), ); @@ -206,9 +206,12 @@ function validateOneOfInputObject( if (isNullLiteral) { context.reportError( - new GraphQLError(`Field "${type}.${keys[0]}" must be non-null.`, { - nodes: [node], - }), + new GraphQLError( + `Within OneOf Input Object type "${type}", exactly one field must be specified, and the value for that field must be non-null.`, + { + nodes: [node], + }, + ), ); return; } @@ -221,7 +224,7 @@ function validateOneOfInputObject( if (isNullableVariable) { context.reportError( new GraphQLError( - `Variable "$${variableName}" must be non-nullable to be used for OneOf Input Object "${type}".`, + `Variable "$${variableName}" must be non-nullable to be used within OneOf Input Object type "${type}".`, { nodes: [node] }, ), );