Fix handling of input objects with 'length' property (#2893)

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<mixed>> {
-  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;
 }
 

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);
+  });
+});

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<T>(
+  collection: mixed,
+  mapFn: (elem: mixed, index: number) => T = (item) => ((item: any): T),
+): Array<T> | 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;
+}

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([

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,18 +63,19 @@ 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);
         }
       }
       return { kind: Kind.LIST, values: valuesNodes };
     }
+
     return astFromValue(value, itemType);
   }