diff --git a/src/execution/__tests__/lists-test.ts b/src/execution/__tests__/lists-test.ts index 1df9be7571..6e4338146b 100644 --- a/src/execution/__tests__/lists-test.ts +++ b/src/execution/__tests__/lists-test.ts @@ -89,7 +89,17 @@ describe('Execute: Accepts async iterables as list value', () => { function completeObjectList( resolve: GraphQLFieldResolver<{ index: number }, unknown>, + nonNullable = false, ): PromiseOrValue { + const ObjectWrapperType = new GraphQLObjectType({ + name: 'ObjectWrapper', + fields: { + index: { + type: new GraphQLNonNull(GraphQLString), + resolve, + }, + }, + }); const schema = new GraphQLSchema({ query: new GraphQLObjectType({ name: 'Query', @@ -101,15 +111,9 @@ describe('Execute: Accepts async iterables as list value', () => { yield await Promise.resolve({ index: 2 }); }, type: new GraphQLList( - new GraphQLObjectType({ - name: 'ObjectWrapper', - fields: { - index: { - type: new GraphQLNonNull(GraphQLString), - resolve, - }, - }, - }), + nonNullable + ? new GraphQLNonNull(ObjectWrapperType) + : ObjectWrapperType, ), }, }, @@ -216,6 +220,27 @@ describe('Execute: Accepts async iterables as list value', () => { ], }); }); + + it('Handles mixture of synchronous and asynchronous errors from `completeValue` in AsyncIterables', async () => { + expectJSON( + await completeObjectList(({ index }) => { + if (index === 0) { + return Promise.reject(new Error('bad')); + } + throw new Error('also bad'); + }, true), + ).toDeepEqual({ + data: { listField: null }, + errors: [ + { + message: 'also bad', + locations: [{ line: 1, column: 15 }], + path: ['listField', 1, 'index'], + }, + ], + }); + }); + it('Handles nulls yielded by async generator', async () => { async function* listField() { yield await Promise.resolve(1); @@ -265,6 +290,11 @@ describe('Execute: Handles list nullability', () => { expectJSON(await executeQuery(promisify(listOfPromises))).toDeepEqual( result, ); + + // Test mix of synchronous and non-synchronous values + const [first, ...rest] = listField; + const listOfSomePromises = [first, ...rest.map(promisify)]; + expectJSON(await executeQuery(listOfSomePromises)).toDeepEqual(result); } return result; @@ -322,6 +352,32 @@ describe('Execute: Handles list nullability', () => { }); }); + it('Contains multiple nulls', async () => { + const listField = [null, null, 2]; + const errors = [ + { + message: 'Cannot return null for non-nullable field Query.listField.', + locations: [{ line: 1, column: 3 }], + path: ['listField', 0], + }, + ]; + + expect(await complete({ listField, as: '[Int]' })).to.deep.equal({ + data: { listField: [null, null, 2] }, + }); + expect(await complete({ listField, as: '[Int]!' })).to.deep.equal({ + data: { listField: [null, null, 2] }, + }); + expectJSON(await complete({ listField, as: '[Int!]' })).toDeepEqual({ + data: { listField: null }, + errors, + }); + expectJSON(await complete({ listField, as: '[Int!]!' })).toDeepEqual({ + data: null, + errors, + }); + }); + it('Returns null', async () => { const listField = null; const errors = [ @@ -376,6 +432,39 @@ describe('Execute: Handles list nullability', () => { }); }); + it('Contains multiple errors', async () => { + const listField = [new Error('bad'), new Error('also bad'), 2]; + + const firstError = { + message: 'bad', + locations: [{ line: 1, column: 3 }], + path: ['listField', 0], + }; + + const secondError = { + message: 'also bad', + locations: [{ line: 1, column: 3 }], + path: ['listField', 1], + }; + + expectJSON(await complete({ listField, as: '[Int]' })).toDeepEqual({ + data: { listField: [null, null, 2] }, + errors: [firstError, secondError], + }); + expectJSON(await complete({ listField, as: '[Int]!' })).toDeepEqual({ + data: { listField: [null, null, 2] }, + errors: [firstError, secondError], + }); + expectJSON(await complete({ listField, as: '[Int!]' })).toDeepEqual({ + data: { listField: null }, + errors: [firstError], + }); + expectJSON(await complete({ listField, as: '[Int!]!' })).toDeepEqual({ + data: null, + errors: [firstError], + }); + }); + it('Results in error', async () => { const listField = new Error('bad'); const errors = [ diff --git a/src/execution/__tests__/stream-test.ts b/src/execution/__tests__/stream-test.ts index c4836eea49..46f09e5ec5 100644 --- a/src/execution/__tests__/stream-test.ts +++ b/src/execution/__tests__/stream-test.ts @@ -483,11 +483,6 @@ describe('Execute: stream directive', () => { }, ], }, - ], - hasNext: true, - }, - { - incremental: [ { items: [{ name: 'Leia', id: '3' }], path: ['friendList', 2], diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 061a3ea759..910244d40f 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -982,49 +982,51 @@ async function completeAsyncIteratorValue( ): Promise> { const errors = asyncPayloadRecord?.errors ?? exeContext.errors; const stream = getStreamValues(exeContext, fieldNodes, path); - let containsPromise = false; + const promises: Array> = []; const completedResults: Array = []; let index = 0; - // eslint-disable-next-line no-constant-condition - while (true) { - if ( - stream && - typeof stream.initialCount === 'number' && - index >= stream.initialCount - ) { - // eslint-disable-next-line @typescript-eslint/no-floating-promises - executeStreamIterator( - index, - iterator, - exeContext, - fieldNodes, - info, - itemType, - path, - stream.label, - asyncPayloadRecord, - ); - break; - } + try { + // eslint-disable-next-line no-constant-condition + while (true) { + if ( + stream && + typeof stream.initialCount === 'number' && + index >= stream.initialCount + ) { + // eslint-disable-next-line @typescript-eslint/no-floating-promises + executeStreamIterator( + index, + iterator, + exeContext, + fieldNodes, + info, + itemType, + path, + stream.label, + asyncPayloadRecord, + ); + break; + } - const itemPath = addPath(path, index, undefined); - let iteration; - try { - // eslint-disable-next-line no-await-in-loop - iteration = await iterator.next(); - if (iteration.done) { + const itemPath = addPath(path, index, undefined); + let iteration; + try { + // eslint-disable-next-line no-await-in-loop + iteration = await iterator.next(); + if (iteration.done) { + break; + } + } catch (rawError) { + const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); + completedResults.push(handleFieldError(error, itemType, errors)); break; } - } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - completedResults.push(handleFieldError(error, itemType, errors)); - break; - } - if ( completeListItemValue( iteration.value, completedResults, + index, + promises, errors, exeContext, itemType, @@ -1032,13 +1034,20 @@ async function completeAsyncIteratorValue( info, itemPath, asyncPayloadRecord, - ) - ) { - containsPromise = true; + ); + index += 1; } - index += 1; + } catch (error) { + if (promises.length) { + return Promise.all(promises).finally(() => { + throw error; + }); + } + throw error; } - return containsPromise ? Promise.all(completedResults) : completedResults; + return promises.length + ? Promise.all(promises).then(() => completedResults) + : completedResults; } /** @@ -1081,39 +1090,45 @@ function completeListValue( // This is specified as a simple map, however we're optimizing the path // where the list contains no Promises by avoiding creating another Promise. - let containsPromise = false; + const promises: Array> = []; let previousAsyncPayloadRecord = asyncPayloadRecord; const completedResults: Array = []; let index = 0; - for (const item of result) { - // 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); + const iterator = result[Symbol.iterator](); + try { + let iteration = iterator.next(); + while (!iteration.done) { + const item = iteration.value; + // 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); - if ( - stream && - typeof stream.initialCount === 'number' && - index >= stream.initialCount - ) { - previousAsyncPayloadRecord = executeStreamField( - path, - itemPath, - item, - exeContext, - fieldNodes, - info, - itemType, - stream.label, - previousAsyncPayloadRecord, - ); - index++; - continue; - } + if ( + stream && + typeof stream.initialCount === 'number' && + index >= stream.initialCount + ) { + previousAsyncPayloadRecord = executeStreamField( + path, + itemPath, + item, + exeContext, + fieldNodes, + info, + itemType, + stream.label, + previousAsyncPayloadRecord, + ); + index++; + iteration = iterator.next(); + continue; + } - if ( completeListItemValue( item, completedResults, + index, + promises, errors, exeContext, itemType, @@ -1121,15 +1136,31 @@ function completeListValue( info, itemPath, asyncPayloadRecord, - ) - ) { - containsPromise = true; - } + ); - index++; + index++; + iteration = iterator.next(); + } + } catch (error) { + let iteration = iterator.next(); + while (!iteration.done) { + const item = iteration.value; + if (isPromise(item)) { + promises.push(item as Promise); + } + iteration = iterator.next(); + } + if (promises.length) { + return Promise.all(promises).finally(() => { + throw error; + }); + } + throw error; } - return containsPromise ? Promise.all(completedResults) : completedResults; + return promises.length + ? Promise.all(promises).then(() => completedResults) + : completedResults; } /** @@ -1140,6 +1171,8 @@ function completeListValue( function completeListItemValue( item: unknown, completedResults: Array, + index: number, + promises: Array>, errors: Array, exeContext: ExecutionContext, itemType: GraphQLOutputType, @@ -1147,7 +1180,7 @@ function completeListItemValue( info: GraphQLResolveInfo, itemPath: Path, asyncPayloadRecord?: AsyncPayloadRecord, -): boolean { +): void { try { let completedItem; if (isPromise(item)) { @@ -1177,20 +1210,26 @@ function completeListItemValue( if (isPromise(completedItem)) { // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. - completedResults.push( - completedItem.then(undefined, (rawError) => { - const error = locatedError( - rawError, - fieldNodes, - pathToArray(itemPath), - ); - const handledError = handleFieldError(error, itemType, errors); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return handledError; - }), + completedResults.push(undefined); + promises.push( + completedItem.then( + (resolved) => { + completedResults[index] = resolved; + }, + (rawError) => { + const error = locatedError( + rawError, + fieldNodes, + pathToArray(itemPath), + ); + const handledError = handleFieldError(error, itemType, errors); + filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + completedResults[index] = handledError; + }, + ), ); - return true; + return; } completedResults.push(completedItem); @@ -1200,8 +1239,6 @@ function completeListItemValue( filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); completedResults.push(handledError); } - - return false; } /**