From 4ed99d59e2ea717360980c68d50ad3adb43c29ae Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 3 Jan 2023 14:46:17 +0200 Subject: [PATCH] including mixture of sync/async errors in lists following #3706 fix for field execution --- src/execution/__tests__/lists-test.ts | 107 ++++++++++++-- src/execution/execute.ts | 203 +++++++++++++++----------- 2 files changed, 216 insertions(+), 94 deletions(-) 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/execute.ts b/src/execution/execute.ts index 1bc6c4267b..2eeb49adf7 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1029,59 +1029,69 @@ async function completeAsyncIteratorValue( let containsPromise = false; 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, - errors, - exeContext, - itemType, - fieldNodes, - info, - itemPath, - asyncPayloadRecord, - ) - ) { - containsPromise = true; + if ( + completeListItemValue( + iteration.value, + completedResults, + errors, + exeContext, + itemType, + fieldNodes, + info, + itemPath, + asyncPayloadRecord, + ) + ) { + containsPromise = true; + } + index += 1; } - index += 1; + } catch (error) { + if (containsPromise) { + return Promise.all(completedResults).finally(() => { + throw error; + }); + } + throw error; } + return containsPromise ? Promise.all(completedResults) : completedResults; } @@ -1129,48 +1139,71 @@ function completeListValue( 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++; + iteration = iterator.next(); + continue; + } + + if ( + completeListItemValue( + item, + completedResults, + errors, + exeContext, + itemType, + fieldNodes, + info, + itemPath, + asyncPayloadRecord, + ) + ) { + containsPromise = true; + } - if ( - stream && - typeof stream.initialCount === 'number' && - index >= stream.initialCount - ) { - previousAsyncPayloadRecord = executeStreamField( - path, - itemPath, - item, - exeContext, - fieldNodes, - info, - itemType, - stream.label, - previousAsyncPayloadRecord, - ); index++; - continue; + iteration = iterator.next(); } - - if ( - completeListItemValue( - item, - completedResults, - errors, - exeContext, - itemType, - fieldNodes, - info, - itemPath, - asyncPayloadRecord, - ) - ) { - containsPromise = true; + } catch (error) { + let iteration = iterator.next(); + while (!iteration.done) { + const item = iteration.value; + if (isPromise(item)) { + containsPromise = true; + completedResults.push(item); + } + iteration = iterator.next(); } - - index++; + if (containsPromise) { + return Promise.all(completedResults).finally(() => { + throw error; + }); + } + throw error; } return containsPromise ? Promise.all(completedResults) : completedResults;