Drop support for non-iteratable ArrayLike objects (#2917)

Author: IvanGoncharov

src/execution/execute.js

@@ -7,10 +7,10 @@ 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 { promiseReduce } from '../jsutils/promiseReduce';
 import { promiseForObject } from '../jsutils/promiseForObject';
 import { addPath, pathToArray } from '../jsutils/Path';
+import { isIteratableObject } from '../jsutils/isIteratableObject';
 
 import type { GraphQLFormattedError } from '../error/formatError';
 import { GraphQLError } from '../error/GraphQLError';
@@ -821,7 +821,7 @@ function completeListValue(
   path: Path,
   result: mixed,
 ): PromiseOrValue<$ReadOnlyArray<mixed>> {
-  if (!isCollection(result)) {
+  if (!isIteratableObject(result)) {
     throw new GraphQLError(
       `Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`,
     );

src/jsutils/__tests__/isCollection-test.js

@@ -0,0 +1,71 @@
+import { expect } from 'chai';
+import { describe, it } from 'mocha';
+
+import { identityFunc } from '../identityFunc';
+import { isIteratableObject } from '../isIteratableObject';
+
+describe('isIteratableObject', () => {
+  it('should return `true` for collections', () => {
+    expect(isIteratableObject([])).to.equal(true);
+    expect(isIteratableObject(new Int8Array(1))).to.equal(true);
+
+    // eslint-disable-next-line no-new-wrappers
+    expect(isIteratableObject(new String('ABC'))).to.equal(true);
+
+    function getArguments() {
+      return arguments;
+    }
+    expect(isIteratableObject(getArguments())).to.equal(true);
+
+    const iterator = { [Symbol.iterator]: identityFunc };
+    expect(isIteratableObject(iterator)).to.equal(true);
+
+    // istanbul ignore next (Never called and use just as a placeholder)
+    function* generatorFunc() {
+      /* do nothing */
+    }
+    expect(isIteratableObject(generatorFunc())).to.equal(true);
+
+    // But generator function itself is not iteratable
+    expect(isIteratableObject(generatorFunc)).to.equal(false);
+  });
+
+  it('should return `false` for non-collections', () => {
+    expect(isIteratableObject(null)).to.equal(false);
+    expect(isIteratableObject(undefined)).to.equal(false);
+
+    expect(isIteratableObject('ABC')).to.equal(false);
+    expect(isIteratableObject('0')).to.equal(false);
+    expect(isIteratableObject('')).to.equal(false);
+
+    expect(isIteratableObject(1)).to.equal(false);
+    expect(isIteratableObject(0)).to.equal(false);
+    expect(isIteratableObject(NaN)).to.equal(false);
+    // eslint-disable-next-line no-new-wrappers
+    expect(isIteratableObject(new Number(123))).to.equal(false);
+
+    expect(isIteratableObject(true)).to.equal(false);
+    expect(isIteratableObject(false)).to.equal(false);
+    // eslint-disable-next-line no-new-wrappers
+    expect(isIteratableObject(new Boolean(true))).to.equal(false);
+
+    expect(isIteratableObject({})).to.equal(false);
+    expect(isIteratableObject({ iterable: true })).to.equal(false);
+
+    const iteratorWithoutSymbol = { next: identityFunc };
+    expect(isIteratableObject(iteratorWithoutSymbol)).to.equal(false);
+
+    const invalidIteratable = {
+      [Symbol.iterator]: { next: identityFunc },
+    };
+    expect(isIteratableObject(invalidIteratable)).to.equal(false);
+
+    const arrayLike = {};
+    arrayLike[0] = 'Alpha';
+    arrayLike[1] = 'Bravo';
+    arrayLike[2] = 'Charlie';
+    arrayLike.length = 3;
+
+    expect(isIteratableObject(arrayLike)).to.equal(false);
+  });
+});

src/jsutils/__tests__/isIteratableObject-test.js

@@ -0,0 +1,27 @@
+/**
+ * Returns true if the provided object is an Object (i.e. not a string literal)
+ * and implements the Iterator protocol.
+ *
+ * This may be used in place of [Array.isArray()][isArray] to determine if
+ * an object should be iterated-over e.g. Array, Map, Set, Int8Array,
+ * TypedArray, etc. but excludes string literals.
+ *
+ * @example
+ *
+ * isIteratableObject([ 1, 2, 3 ]) // true
+ * isIteratableObject(new Map()) // true
+ * isIteratableObject('ABC') // false
+ * isIteratableObject({ key: 'value' }) // false
+ * isIteratableObject({ length: 1, 0: 'Alpha' }) // false
+ */
+declare function isIteratableObject(
+  value: mixed,
+): boolean %checks(value instanceof Iterable);
+
+// eslint-disable-next-line no-redeclare
+export function isIteratableObject(maybeIteratable: mixed): boolean {
+  return (
+    typeof maybeIteratable === 'object' &&
+    typeof maybeIteratable?.[Symbol.iterator] === 'function'
+  );
+}

src/jsutils/isCollection.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([

src/jsutils/isIteratableObject.js

@@ -3,7 +3,7 @@ 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 { isIteratableObject } from '../jsutils/isIteratableObject';
 
 import type { ValueNode } from '../language/ast';
 import { Kind } from '../language/kinds';
@@ -62,7 +62,7 @@ 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)) {
+    if (isIteratableObject(value)) {
       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

src/utilities/__tests__/coerceInputValue-test.js

@@ -5,10 +5,10 @@ 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 { suggestionList } from '../jsutils/suggestionList';
 import { printPathArray } from '../jsutils/printPathArray';
 import { addPath, pathToArray } from '../jsutils/Path';
+import { isIteratableObject } from '../jsutils/isIteratableObject';
 
 import { GraphQLError } from '../error/GraphQLError';
 
@@ -77,7 +77,7 @@ function coerceInputValueImpl(
 
   if (isListType(type)) {
     const itemType = type.ofType;
-    if (isCollection(inputValue)) {
+    if (isIteratableObject(inputValue)) {
       return Array.from(inputValue, (itemValue, index) => {
         const itemPath = addPath(path, index, undefined);
         return coerceInputValueImpl(itemValue, itemType, onError, itemPath);