diff --git a/src/execution/execute.js b/src/execution/execute.js index f272b65aef..35f440c720 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -1,5 +1,3 @@ -import arrayFrom from '../polyfills/arrayFrom'; - import type { Path } from '../jsutils/Path'; import type { ObjMap } from '../jsutils/ObjMap'; import type { PromiseOrValue } from '../jsutils/PromiseOrValue'; @@ -9,7 +7,7 @@ import invariant from '../jsutils/invariant'; import devAssert from '../jsutils/devAssert'; import isPromise from '../jsutils/isPromise'; import isObjectLike from '../jsutils/isObjectLike'; -import isCollection from '../jsutils/isCollection'; +import safeArrayFrom from '../jsutils/safeArrayFrom'; import promiseReduce from '../jsutils/promiseReduce'; import promiseForObject from '../jsutils/promiseForObject'; import { addPath, pathToArray } from '../jsutils/Path'; @@ -867,17 +865,11 @@ function completeListValue( path: Path, result: mixed, ): PromiseOrValue<$ReadOnlyArray> { - if (!isCollection(result)) { - throw new GraphQLError( - `Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`, - ); - } - // This is specified as a simple map, however we're optimizing the path // where the list contains no Promises by avoiding creating another Promise. const itemType = returnType.ofType; let containsPromise = false; - const completedResults = arrayFrom(result, (item, index) => { + const completedResults = safeArrayFrom(result, (item, index) => { // No need to modify the info object containing the path, // since from here on it is not ever accessed by resolver functions. const itemPath = addPath(path, index, undefined); @@ -925,6 +917,12 @@ function completeListValue( } }); + if (completedResults == null) { + throw new GraphQLError( + `Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`, + ); + } + return containsPromise ? Promise.all(completedResults) : completedResults; } diff --git a/src/jsutils/__tests__/isCollection-test.js b/src/jsutils/__tests__/isCollection-test.js deleted file mode 100644 index 0277d2eb53..0000000000 --- a/src/jsutils/__tests__/isCollection-test.js +++ /dev/null @@ -1,71 +0,0 @@ -import { expect } from 'chai'; -import { describe, it } from 'mocha'; - -import identityFunc from '../identityFunc'; -import isCollection from '../isCollection'; - -describe('isCollection', () => { - it('should return `true` for collections', () => { - expect(isCollection([])).to.equal(true); - expect(isCollection(new Int8Array(1))).to.equal(true); - - // eslint-disable-next-line no-new-wrappers - expect(isCollection(new String('ABC'))).to.equal(true); - - function getArguments() { - return arguments; - } - expect(isCollection(getArguments())).to.equal(true); - - const arrayLike = {}; - arrayLike[0] = 'Alpha'; - arrayLike[1] = 'Bravo'; - arrayLike[2] = 'Charlie'; - arrayLike.length = 3; - - expect(isCollection(arrayLike)).to.equal(true); - - const iterator = { [Symbol.iterator]: identityFunc }; - expect(isCollection(iterator)).to.equal(true); - - // istanbul ignore next (Never called and use just as a placeholder) - function* generatorFunc() { - /* do nothing */ - } - expect(isCollection(generatorFunc())).to.equal(true); - - // But generator function itself is not iteratable - expect(isCollection(generatorFunc)).to.equal(false); - }); - - it('should return `false` for non-collections', () => { - expect(isCollection(null)).to.equal(false); - expect(isCollection(undefined)).to.equal(false); - - expect(isCollection('ABC')).to.equal(false); - expect(isCollection('0')).to.equal(false); - expect(isCollection('')).to.equal(false); - - expect(isCollection(1)).to.equal(false); - expect(isCollection(0)).to.equal(false); - expect(isCollection(NaN)).to.equal(false); - // eslint-disable-next-line no-new-wrappers - expect(isCollection(new Number(123))).to.equal(false); - - expect(isCollection(true)).to.equal(false); - expect(isCollection(false)).to.equal(false); - // eslint-disable-next-line no-new-wrappers - expect(isCollection(new Boolean(true))).to.equal(false); - - expect(isCollection({})).to.equal(false); - expect(isCollection({ iterable: true })).to.equal(false); - - const iteratorWithoutSymbol = { next: identityFunc }; - expect(isCollection(iteratorWithoutSymbol)).to.equal(false); - - const invalidIteratable = { - [Symbol.iterator]: { next: identityFunc }, - }; - expect(isCollection(invalidIteratable)).to.equal(false); - }); -}); diff --git a/src/jsutils/__tests__/safeArrayFrom-test.js b/src/jsutils/__tests__/safeArrayFrom-test.js new file mode 100644 index 0000000000..3c30bb5cbe --- /dev/null +++ b/src/jsutils/__tests__/safeArrayFrom-test.js @@ -0,0 +1,91 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import identityFunc from '../identityFunc'; +import safeArrayFrom from '../safeArrayFrom'; + +describe('safeArrayFrom', () => { + it('should convert collections into arrays', () => { + expect(safeArrayFrom([])).to.deep.equal([]); + expect(safeArrayFrom(new Set([1, 2, 3]))).to.deep.equal([1, 2, 3]); + expect(safeArrayFrom(new Int8Array([1, 2, 3]))).to.deep.equal([1, 2, 3]); + + // eslint-disable-next-line no-new-wrappers + expect(safeArrayFrom(new String('ABC'))).to.deep.equal(['A', 'B', 'C']); + + function getArguments() { + return arguments; + } + expect(safeArrayFrom(getArguments())).to.deep.equal([]); + + const arrayLike = {}; + arrayLike[0] = 'Alpha'; + arrayLike[1] = 'Bravo'; + arrayLike[2] = 'Charlie'; + arrayLike.length = 3; + + expect(safeArrayFrom(arrayLike)).to.deep.equal([ + 'Alpha', + 'Bravo', + 'Charlie', + ]); + + const iteratable = { + [Symbol.iterator]() { + const values = [1, 2, 3]; + return { + next() { + const done = values.length === 0; + const value = values.shift(); + + return { done, value }; + }, + }; + }, + }; + expect(safeArrayFrom(iteratable)).to.deep.equal([1, 2, 3]); + + // istanbul ignore next (Never called and use just as a placeholder) + function* generatorFunc() { + yield 1; + yield 2; + yield 3; + } + expect(safeArrayFrom(generatorFunc())).to.deep.equal([1, 2, 3]); + + // But generator function itself is not iteratable + expect(safeArrayFrom(generatorFunc)).to.equal(null); + }); + + it('should return `null` for non-collections', () => { + expect(safeArrayFrom(null)).to.equal(null); + expect(safeArrayFrom(undefined)).to.equal(null); + + expect(safeArrayFrom('ABC')).to.equal(null); + expect(safeArrayFrom('0')).to.equal(null); + expect(safeArrayFrom('')).to.equal(null); + + expect(safeArrayFrom(1)).to.equal(null); + expect(safeArrayFrom(0)).to.equal(null); + expect(safeArrayFrom(NaN)).to.equal(null); + // eslint-disable-next-line no-new-wrappers + expect(safeArrayFrom(new Number(123))).to.equal(null); + + expect(safeArrayFrom(true)).to.equal(null); + expect(safeArrayFrom(false)).to.equal(null); + // eslint-disable-next-line no-new-wrappers + expect(safeArrayFrom(new Boolean(true))).to.equal(null); + + expect(safeArrayFrom({})).to.equal(null); + expect(safeArrayFrom({ length: 3 })).to.equal(null); + expect(safeArrayFrom({ iterable: true })).to.equal(null); + + const iteratorWithoutSymbol = { next: identityFunc }; + expect(safeArrayFrom(iteratorWithoutSymbol)).to.equal(null); + + const invalidIteratable = { + [Symbol.iterator]: { next: identityFunc }, + }; + expect(safeArrayFrom(invalidIteratable)).to.equal(null); + }); +}); diff --git a/src/jsutils/isCollection.js b/src/jsutils/isCollection.js deleted file mode 100644 index 731470256d..0000000000 --- a/src/jsutils/isCollection.js +++ /dev/null @@ -1,37 +0,0 @@ -import { SYMBOL_ITERATOR } from '../polyfills/symbols'; - -/** - * Returns true if the provided object is an Object (i.e. not a string literal) - * and is either Iterable or Array-like. - * - * This may be used in place of [Array.isArray()][isArray] to determine if an - * object should be iterated-over. It always excludes string literals and - * includes Arrays (regardless of if it is Iterable). It also includes other - * Array-like objects such as NodeList, TypedArray, and Buffer. - * - * @example - * - * isCollection([ 1, 2, 3 ]) // true - * isCollection('ABC') // false - * isCollection({ length: 1, 0: 'Alpha' }) // true - * isCollection({ key: 'value' }) // false - * isCollection(new Map()) // true - * - * @param obj - * An Object value which might implement the Iterable or Array-like protocols. - * @return {boolean} true if Iterable or Array-like Object. - */ -export default function isCollection(obj: mixed): boolean { - if (obj == null || typeof obj !== 'object') { - return false; - } - - // Is Array like? - const length = obj.length; - if (typeof length === 'number' && length >= 0 && length % 1 === 0) { - return true; - } - - // Is Iterable? - return typeof obj[SYMBOL_ITERATOR] === 'function'; -} diff --git a/src/jsutils/safeArrayFrom.js b/src/jsutils/safeArrayFrom.js new file mode 100644 index 0000000000..d04d7d8a31 --- /dev/null +++ b/src/jsutils/safeArrayFrom.js @@ -0,0 +1,58 @@ +import { SYMBOL_ITERATOR } from '../polyfills/symbols'; + +/** + * Safer version of `Array.from` that return `null` if value isn't convertible to array. + * Also protects against Array-like objects without items. + * + * @example + * + * safeArrayFrom([ 1, 2, 3 ]) // [1, 2, 3] + * safeArrayFrom('ABC') // null + * safeArrayFrom({ length: 1 }) // null + * safeArrayFrom({ length: 1, 0: 'Alpha' }) // ['Alpha'] + * safeArrayFrom({ key: 'value' }) // null + * safeArrayFrom(new Map()) // [] + * + */ +export default function safeArrayFrom( + collection: mixed, + mapFn: (elem: mixed, index: number) => T = (item) => ((item: any): T), +): Array | null { + if (collection == null || typeof collection !== 'object') { + return null; + } + + if (Array.isArray(collection)) { + return collection.map(mapFn); + } + + // Is Iterable? + const iteratorMethod = collection[SYMBOL_ITERATOR]; + if (typeof iteratorMethod === 'function') { + // $FlowFixMe[incompatible-use] + const iterator = iteratorMethod.call(collection); + const result = []; + let step; + + for (let i = 0; !(step = iterator.next()).done; ++i) { + result.push(mapFn(step.value, i)); + } + return result; + } + + // Is Array like? + const length = collection.length; + if (typeof length === 'number' && length >= 0 && length % 1 === 0) { + const result = []; + for (let i = 0; i < length; ++i) { + if (!Object.prototype.hasOwnProperty.call(collection, i)) { + return null; + } + result.push(mapFn(collection[String(i)], i)); + } + + return result; + } + + return null; +} diff --git a/src/utilities/__tests__/coerceInputValue-test.js b/src/utilities/__tests__/coerceInputValue-test.js index 05f7ca12c2..e743e0d715 100644 --- a/src/utilities/__tests__/coerceInputValue-test.js +++ b/src/utilities/__tests__/coerceInputValue-test.js @@ -8,8 +8,8 @@ import { GraphQLInt } from '../../type/scalars'; import { GraphQLList, GraphQLNonNull, - GraphQLScalarType, GraphQLEnumType, + GraphQLScalarType, GraphQLInputObjectType, } from '../../type/definition'; @@ -335,6 +335,20 @@ describe('coerceInputValue', () => { expectValue(result).to.deep.equal([42]); }); + it('returns a list for a non-list object value', () => { + const TestListOfObjects = new GraphQLList( + new GraphQLInputObjectType({ + name: 'TestObject', + fields: { + length: { type: GraphQLInt }, + }, + }), + ); + + const result = coerceValue({ length: 100500 }, TestListOfObjects); + expectValue(result).to.deep.equal([{ length: 100500 }]); + }); + it('returns an error for a non-list invalid value', () => { const result = coerceValue('INVALID', TestList); expectErrors(result).to.deep.equal([ diff --git a/src/utilities/astFromValue.js b/src/utilities/astFromValue.js index 39ab089d27..4ce2acfe51 100644 --- a/src/utilities/astFromValue.js +++ b/src/utilities/astFromValue.js @@ -1,11 +1,10 @@ import isFinite from '../polyfills/isFinite'; -import arrayFrom from '../polyfills/arrayFrom'; import objectValues from '../polyfills/objectValues'; import inspect from '../jsutils/inspect'; import invariant from '../jsutils/invariant'; import isObjectLike from '../jsutils/isObjectLike'; -import isCollection from '../jsutils/isCollection'; +import safeArrayFrom from '../jsutils/safeArrayFrom'; import type { ValueNode } from '../language/ast'; import { Kind } from '../language/kinds'; @@ -64,11 +63,11 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode { // the value is not an array, convert the value using the list's item type. if (isListType(type)) { const itemType = type.ofType; - if (isCollection(value)) { + + const items = safeArrayFrom(value); + if (items != null) { const valuesNodes = []; - // Since we transpile for-of in loose mode it doesn't support iterators - // and it's required to first convert iteratable into array - for (const item of arrayFrom(value)) { + for (const item of items) { const itemNode = astFromValue(item, itemType); if (itemNode != null) { valuesNodes.push(itemNode); @@ -76,6 +75,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode { } return { kind: Kind.LIST, values: valuesNodes }; } + return astFromValue(value, itemType); } diff --git a/src/utilities/coerceInputValue.js b/src/utilities/coerceInputValue.js index be8526108a..1f0ef21f22 100644 --- a/src/utilities/coerceInputValue.js +++ b/src/utilities/coerceInputValue.js @@ -1,4 +1,3 @@ -import arrayFrom from '../polyfills/arrayFrom'; import objectValues from '../polyfills/objectValues'; import type { Path } from '../jsutils/Path'; @@ -6,7 +5,7 @@ import inspect from '../jsutils/inspect'; import invariant from '../jsutils/invariant'; import didYouMean from '../jsutils/didYouMean'; import isObjectLike from '../jsutils/isObjectLike'; -import isCollection from '../jsutils/isCollection'; +import safeArrayFrom from '../jsutils/safeArrayFrom'; import suggestionList from '../jsutils/suggestionList'; import printPathArray from '../jsutils/printPathArray'; import { addPath, pathToArray } from '../jsutils/Path'; @@ -78,12 +77,16 @@ function coerceInputValueImpl( if (isListType(type)) { const itemType = type.ofType; - if (isCollection(inputValue)) { - return arrayFrom(inputValue, (itemValue, index) => { - const itemPath = addPath(path, index, undefined); - return coerceInputValueImpl(itemValue, itemType, onError, itemPath); - }); + + const coercedList = safeArrayFrom(inputValue, (itemValue, index) => { + const itemPath = addPath(path, index, undefined); + return coerceInputValueImpl(itemValue, itemType, onError, itemPath); + }); + + if (coercedList != null) { + return coercedList; } + // Lists accept a non-list value as a list of one. return [coerceInputValueImpl(inputValue, itemType, onError, path)]; }