Skip to content

Commit 7e5f567

Browse files
IvanGoncharovIvanGoncharov
authored
Fix handling of input objects with 'length' property (#2893)
1 parent 3bce13f commit 7e5f567

8 files changed

Lines changed: 188 additions & 132 deletions

File tree

‎src/execution/execute.js‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
import arrayFrom from '../polyfills/arrayFrom';
2-
31
import type { Path } from '../jsutils/Path';
42
import type { ObjMap } from '../jsutils/ObjMap';
53
import type { PromiseOrValue } from '../jsutils/PromiseOrValue';
@@ -9,7 +7,7 @@ import invariant from '../jsutils/invariant';
97
import devAssert from '../jsutils/devAssert';
108
import isPromise from '../jsutils/isPromise';
119
import isObjectLike from '../jsutils/isObjectLike';
12-
import isCollection from '../jsutils/isCollection';
10+
import safeArrayFrom from '../jsutils/safeArrayFrom';
1311
import promiseReduce from '../jsutils/promiseReduce';
1412
import promiseForObject from '../jsutils/promiseForObject';
1513
import { addPath, pathToArray } from '../jsutils/Path';
@@ -867,17 +865,11 @@ function completeListValue(
867865
path: Path,
868866
result: mixed,
869867
): PromiseOrValue<$ReadOnlyArray<mixed>> {
870-
if (!isCollection(result)) {
871-
throw new GraphQLError(
872-
`Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`,
873-
);
874-
}
875-
876868
// This is specified as a simple map, however we're optimizing the path
877869
// where the list contains no Promises by avoiding creating another Promise.
878870
const itemType = returnType.ofType;
879871
let containsPromise = false;
880-
const completedResults = arrayFrom(result, (item, index) => {
872+
const completedResults = safeArrayFrom(result, (item, index) => {
881873
// No need to modify the info object containing the path,
882874
// since from here on it is not ever accessed by resolver functions.
883875
const itemPath = addPath(path, index, undefined);
@@ -925,6 +917,12 @@ function completeListValue(
925917
}
926918
});
927919

920+
if (completedResults == null) {
921+
throw new GraphQLError(
922+
`Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`,
923+
);
924+
}
925+
928926
return containsPromise ? Promise.all(completedResults) : completedResults;
929927
}
930928

‎src/jsutils/__tests__/isCollection-test.js‎

Lines changed: 0 additions & 71 deletions
This file was deleted.

‎src/jsutils/__tests__/safeArrayFrom-test.js‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
import { expect } from 'chai';
2+
import { describe, it } from 'mocha';
3+
4+
import identityFunc from '../identityFunc';
5+
import safeArrayFrom from '../safeArrayFrom';
6+
7+
describe('safeArrayFrom', () => {
8+
it('should convert collections into arrays', () => {
9+
expect(safeArrayFrom([])).to.deep.equal([]);
10+
expect(safeArrayFrom(new Set([1, 2, 3]))).to.deep.equal([1, 2, 3]);
11+
expect(safeArrayFrom(new Int8Array([1, 2, 3]))).to.deep.equal([1, 2, 3]);
12+
13+
// eslint-disable-next-line no-new-wrappers
14+
expect(safeArrayFrom(new String('ABC'))).to.deep.equal(['A', 'B', 'C']);
15+
16+
function getArguments() {
17+
return arguments;
18+
}
19+
expect(safeArrayFrom(getArguments())).to.deep.equal([]);
20+
21+
const arrayLike = {};
22+
arrayLike[0] = 'Alpha';
23+
arrayLike[1] = 'Bravo';
24+
arrayLike[2] = 'Charlie';
25+
arrayLike.length = 3;
26+
27+
expect(safeArrayFrom(arrayLike)).to.deep.equal([
28+
'Alpha',
29+
'Bravo',
30+
'Charlie',
31+
]);
32+
33+
const iteratable = {
34+
[Symbol.iterator]() {
35+
const values = [1, 2, 3];
36+
return {
37+
next() {
38+
const done = values.length === 0;
39+
const value = values.shift();
40+
41+
return { done, value };
42+
},
43+
};
44+
},
45+
};
46+
expect(safeArrayFrom(iteratable)).to.deep.equal([1, 2, 3]);
47+
48+
// istanbul ignore next (Never called and use just as a placeholder)
49+
function* generatorFunc() {
50+
yield 1;
51+
yield 2;
52+
yield 3;
53+
}
54+
expect(safeArrayFrom(generatorFunc())).to.deep.equal([1, 2, 3]);
55+
56+
// But generator function itself is not iteratable
57+
expect(safeArrayFrom(generatorFunc)).to.equal(null);
58+
});
59+
60+
it('should return `null` for non-collections', () => {
61+
expect(safeArrayFrom(null)).to.equal(null);
62+
expect(safeArrayFrom(undefined)).to.equal(null);
63+
64+
expect(safeArrayFrom('ABC')).to.equal(null);
65+
expect(safeArrayFrom('0')).to.equal(null);
66+
expect(safeArrayFrom('')).to.equal(null);
67+
68+
expect(safeArrayFrom(1)).to.equal(null);
69+
expect(safeArrayFrom(0)).to.equal(null);
70+
expect(safeArrayFrom(NaN)).to.equal(null);
71+
// eslint-disable-next-line no-new-wrappers
72+
expect(safeArrayFrom(new Number(123))).to.equal(null);
73+
74+
expect(safeArrayFrom(true)).to.equal(null);
75+
expect(safeArrayFrom(false)).to.equal(null);
76+
// eslint-disable-next-line no-new-wrappers
77+
expect(safeArrayFrom(new Boolean(true))).to.equal(null);
78+
79+
expect(safeArrayFrom({})).to.equal(null);
80+
expect(safeArrayFrom({ length: 3 })).to.equal(null);
81+
expect(safeArrayFrom({ iterable: true })).to.equal(null);
82+
83+
const iteratorWithoutSymbol = { next: identityFunc };
84+
expect(safeArrayFrom(iteratorWithoutSymbol)).to.equal(null);
85+
86+
const invalidIteratable = {
87+
[Symbol.iterator]: { next: identityFunc },
88+
};
89+
expect(safeArrayFrom(invalidIteratable)).to.equal(null);
90+
});
91+
});

‎src/jsutils/isCollection.js‎

Lines changed: 0 additions & 37 deletions
This file was deleted.

‎src/jsutils/safeArrayFrom.js‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
import { SYMBOL_ITERATOR } from '../polyfills/symbols';
2+
3+
/**
4+
* Safer version of `Array.from` that return `null` if value isn't convertible to array.
5+
* Also protects against Array-like objects without items.
6+
*
7+
* @example
8+
*
9+
* safeArrayFrom([ 1, 2, 3 ]) // [1, 2, 3]
10+
* safeArrayFrom('ABC') // null
11+
* safeArrayFrom({ length: 1 }) // null
12+
* safeArrayFrom({ length: 1, 0: 'Alpha' }) // ['Alpha']
13+
* safeArrayFrom({ key: 'value' }) // null
14+
* safeArrayFrom(new Map()) // []
15+
*
16+
*/
17+
export default function safeArrayFrom<T>(
18+
collection: mixed,
19+
mapFn: (elem: mixed, index: number) => T = (item) => ((item: any): T),
20+
): Array<T> | null {
21+
if (collection == null || typeof collection !== 'object') {
22+
return null;
23+
}
24+
25+
if (Array.isArray(collection)) {
26+
return collection.map(mapFn);
27+
}
28+
29+
// Is Iterable?
30+
const iteratorMethod = collection[SYMBOL_ITERATOR];
31+
if (typeof iteratorMethod === 'function') {
32+
// $FlowFixMe[incompatible-use]
33+
const iterator = iteratorMethod.call(collection);
34+
const result = [];
35+
let step;
36+
37+
for (let i = 0; !(step = iterator.next()).done; ++i) {
38+
result.push(mapFn(step.value, i));
39+
}
40+
return result;
41+
}
42+
43+
// Is Array like?
44+
const length = collection.length;
45+
if (typeof length === 'number' && length >= 0 && length % 1 === 0) {
46+
const result = [];
47+
for (let i = 0; i < length; ++i) {
48+
if (!Object.prototype.hasOwnProperty.call(collection, i)) {
49+
return null;
50+
}
51+
result.push(mapFn(collection[String(i)], i));
52+
}
53+
54+
return result;
55+
}
56+
57+
return null;
58+
}

‎src/utilities/__tests__/coerceInputValue-test.js‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import { GraphQLInt } from '../../type/scalars';
88
import {
99
GraphQLList,
1010
GraphQLNonNull,
11-
GraphQLScalarType,
1211
GraphQLEnumType,
12+
GraphQLScalarType,
1313
GraphQLInputObjectType,
1414
} from '../../type/definition';
1515

@@ -335,6 +335,20 @@ describe('coerceInputValue', () => {
335335
expectValue(result).to.deep.equal([42]);
336336
});
337337

338+
it('returns a list for a non-list object value', () => {
339+
const TestListOfObjects = new GraphQLList(
340+
new GraphQLInputObjectType({
341+
name: 'TestObject',
342+
fields: {
343+
length: { type: GraphQLInt },
344+
},
345+
}),
346+
);
347+
348+
const result = coerceValue({ length: 100500 }, TestListOfObjects);
349+
expectValue(result).to.deep.equal([{ length: 100500 }]);
350+
});
351+
338352
it('returns an error for a non-list invalid value', () => {
339353
const result = coerceValue('INVALID', TestList);
340354
expectErrors(result).to.deep.equal([

‎src/utilities/astFromValue.js‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
import isFinite from '../polyfills/isFinite';
2-
import arrayFrom from '../polyfills/arrayFrom';
32
import objectValues from '../polyfills/objectValues';
43

54
import inspect from '../jsutils/inspect';
65
import invariant from '../jsutils/invariant';
76
import isObjectLike from '../jsutils/isObjectLike';
8-
import isCollection from '../jsutils/isCollection';
7+
import safeArrayFrom from '../jsutils/safeArrayFrom';
98

109
import type { ValueNode } from '../language/ast';
1110
import { Kind } from '../language/kinds';
@@ -64,18 +63,19 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
6463
// the value is not an array, convert the value using the list's item type.
6564
if (isListType(type)) {
6665
const itemType = type.ofType;
67-
if (isCollection(value)) {
66+
67+
const items = safeArrayFrom(value);
68+
if (items != null) {
6869
const valuesNodes = [];
69-
// Since we transpile for-of in loose mode it doesn't support iterators
70-
// and it's required to first convert iteratable into array
71-
for (const item of arrayFrom(value)) {
70+
for (const item of items) {
7271
const itemNode = astFromValue(item, itemType);
7372
if (itemNode != null) {
7473
valuesNodes.push(itemNode);
7574
}
7675
}
7776
return { kind: Kind.LIST, values: valuesNodes };
7877
}
78+
7979
return astFromValue(value, itemType);
8080
}
8181

0 commit comments

Comments
 (0)