From 1aa12291be42671c24431dac8c3dc300a700eb5e Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 30 Sep 2022 15:16:50 +0300 Subject: [PATCH 1/5] isolate async iterator next rejection within try block to improve readability --- src/execution/execute.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 1bc6c4267b..e022cbb896 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1056,15 +1056,16 @@ async function completeAsyncIteratorValue( 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; } + if (iteration.done) { + break; + } + if ( completeListItemValue( iteration.value, @@ -1910,20 +1911,23 @@ async function executeStreamIteratorItem( asyncPayloadRecord: StreamRecord, itemPath: Path, ): Promise> { - let item; + let iteration; try { - const { value, done } = await iterator.next(); - if (done) { - asyncPayloadRecord.setIsCompletedIterator(); - return { done, value: undefined }; - } - item = value; + iteration = await iterator.next(); } catch (rawError) { const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); const value = handleFieldError(error, itemType, asyncPayloadRecord.errors); // don't continue if iterator throws return { done: true, value }; } + + const { done, value: item } = iteration; + + if (done) { + asyncPayloadRecord.setIsCompletedIterator(); + return { done, value: undefined }; + } + let completedItem; try { completedItem = completeValue( From 307c5e53be74ec2e21484ea0e982a0417a5be635 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 30 Sep 2022 15:19:46 +0300 Subject: [PATCH 2/5] type result of asyncIterator `.next()` call --- src/execution/execute.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index e022cbb896..d608c953f5 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1052,7 +1052,7 @@ async function completeAsyncIteratorValue( } const itemPath = addPath(path, index, undefined); - let iteration; + let iteration: IteratorResult; try { // eslint-disable-next-line no-await-in-loop iteration = await iterator.next(); @@ -1911,7 +1911,7 @@ async function executeStreamIteratorItem( asyncPayloadRecord: StreamRecord, itemPath: Path, ): Promise> { - let iteration; + let iteration: IteratorResult; try { iteration = await iterator.next(); } catch (rawError) { @@ -1985,7 +1985,7 @@ async function executeStreamIterator( exeContext, }); - let iteration; + let iteration: IteratorResult; try { // eslint-disable-next-line no-await-in-loop iteration = await executeStreamIteratorItem( From 78d7a21dc790969b44f573a71d9e4f1b0b203f4a Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Fri, 30 Sep 2022 15:27:07 +0300 Subject: [PATCH 3/5] introduce addError instead of handleFieldError addError calls locatedError and then adds the error as previously in handleFieldError, throwing if the type is non-null. As opposed to handleFieldError, it does not return null, so as to require the calling function to be more explicit about the new value. --- src/execution/execute.ts | 95 ++++++++++++++++++++++------------------ 1 file changed, 52 insertions(+), 43 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index d608c953f5..5db73f8aa1 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -687,7 +687,6 @@ function executeField( path: Path, asyncPayloadRecord?: AsyncPayloadRecord, ): PromiseOrValue { - const errors = asyncPayloadRecord?.errors ?? exeContext.errors; const fieldName = fieldNodes[0].name.value; const fieldDef = exeContext.schema.getField(parentType, fieldName); if (!fieldDef) { @@ -749,18 +748,18 @@ function executeField( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. return completed.then(undefined, (rawError) => { - const error = locatedError(rawError, fieldNodes, pathToArray(path)); - const handledError = handleFieldError(error, returnType, errors); + const errors = asyncPayloadRecord?.errors ?? exeContext.errors; + addError(rawError, fieldNodes, returnType, path, errors); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); - return handledError; + return null; }); } return completed; } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(path)); - const handledError = handleFieldError(error, returnType, errors); + const errors = asyncPayloadRecord?.errors ?? exeContext.errors; + addError(rawError, fieldNodes, returnType, path, errors); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); - return handledError; + return null; } } @@ -791,11 +790,15 @@ export function buildResolveInfo( }; } -function handleFieldError( - error: GraphQLError, +function addError( + rawError: unknown, + fieldNodes: ReadonlyArray, returnType: GraphQLOutputType, + path: Path, errors: Array, -): null { +): void { + const error = locatedError(rawError, fieldNodes, pathToArray(path)); + // If the field type is non-nullable, then it is resolved without any // protection from errors, however it still properly locates the error. if (isNonNullType(returnType)) { @@ -805,7 +808,6 @@ function handleFieldError( // Otherwise, error protection is applied, logging the error and resolving // a null value for this field if one is encountered. errors.push(error); - return null; } /** @@ -947,10 +949,9 @@ async function completePromisedValue( return completed; } catch (rawError) { const errors = asyncPayloadRecord?.errors ?? exeContext.errors; - const error = locatedError(rawError, fieldNodes, pathToArray(path)); - const handledError = handleFieldError(error, returnType, errors); + addError(rawError, fieldNodes, returnType, path, errors); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); - return handledError; + return null; } } @@ -1057,8 +1058,8 @@ async function completeAsyncIteratorValue( // eslint-disable-next-line no-await-in-loop iteration = await iterator.next(); } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - completedResults.push(handleFieldError(error, itemType, errors)); + addError(rawError, fieldNodes, itemType, itemPath, errors); + completedResults.push(null); break; } @@ -1225,14 +1226,9 @@ function completeListItemValue( // 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); + addError(rawError, fieldNodes, itemType, itemPath, errors); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return handledError; + return null; }), ); @@ -1241,10 +1237,9 @@ function completeListItemValue( completedResults.push(completedItem); } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const handledError = handleFieldError(error, itemType, errors); + addError(rawError, fieldNodes, itemType, itemPath, errors); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - completedResults.push(handledError); + completedResults.push(null); } return false; @@ -1858,12 +1853,14 @@ function executeStreamField( asyncPayloadRecord, ); } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - completedItem = handleFieldError( - error, + addError( + rawError, + fieldNodes, itemType, + itemPath, asyncPayloadRecord.errors, ); + completedItem = null; filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); } } catch (error) { @@ -1876,14 +1873,15 @@ function executeStreamField( if (isPromise(completedItem)) { const completedItems = completedItem .then(undefined, (rawError) => { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const handledError = handleFieldError( - error, + addError( + rawError, + fieldNodes, itemType, + itemPath, asyncPayloadRecord.errors, ); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return handledError; + return null; }) .then( (value) => [value], @@ -1915,10 +1913,15 @@ async function executeStreamIteratorItem( try { iteration = await iterator.next(); } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const value = handleFieldError(error, itemType, asyncPayloadRecord.errors); + addError( + rawError, + fieldNodes, + itemType, + itemPath, + asyncPayloadRecord.errors, + ); // don't continue if iterator throws - return { done: true, value }; + return { done: true, value: null }; } const { done, value: item } = iteration; @@ -1942,22 +1945,28 @@ async function executeStreamIteratorItem( if (isPromise(completedItem)) { completedItem = completedItem.then(undefined, (rawError) => { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const handledError = handleFieldError( - error, + addError( + rawError, + fieldNodes, itemType, + itemPath, asyncPayloadRecord.errors, ); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return handledError; + return null; }); } return { done: false, value: completedItem }; } catch (rawError) { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const value = handleFieldError(error, itemType, asyncPayloadRecord.errors); + addError( + rawError, + fieldNodes, + itemType, + itemPath, + asyncPayloadRecord.errors, + ); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return { done: false, value }; + return { done: false, value: null }; } } From 2ba85cb798d86065d2a0c6bf8a8e02be2c535d49 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Tue, 3 Jan 2023 16:23:58 +0200 Subject: [PATCH 4/5] remove errors from parameters to completeListItemValue --- src/execution/execute.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 5db73f8aa1..c3da6d0f59 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1025,7 +1025,6 @@ async function completeAsyncIteratorValue( iterator: AsyncIterator, asyncPayloadRecord?: AsyncPayloadRecord, ): Promise> { - const errors = asyncPayloadRecord?.errors ?? exeContext.errors; const stream = getStreamValues(exeContext, fieldNodes, path); let containsPromise = false; const completedResults: Array = []; @@ -1058,6 +1057,10 @@ async function completeAsyncIteratorValue( // eslint-disable-next-line no-await-in-loop iteration = await iterator.next(); } catch (rawError) { + // FIXME: add coverage for non-streamed async iterator error within a deferred payload + const errors = + /* c8 ignore start */ asyncPayloadRecord?.errors ?? + /* c8 ignore stop */ exeContext.errors; addError(rawError, fieldNodes, itemType, itemPath, errors); completedResults.push(null); break; @@ -1071,7 +1074,6 @@ async function completeAsyncIteratorValue( completeListItemValue( iteration.value, completedResults, - errors, exeContext, itemType, fieldNodes, @@ -1101,7 +1103,6 @@ function completeListValue( asyncPayloadRecord?: AsyncPayloadRecord, ): PromiseOrValue> { const itemType = returnType.ofType; - const errors = asyncPayloadRecord?.errors ?? exeContext.errors; if (isAsyncIterable(result)) { const iterator = result[Symbol.asyncIterator](); @@ -1160,7 +1161,6 @@ function completeListValue( completeListItemValue( item, completedResults, - errors, exeContext, itemType, fieldNodes, @@ -1186,7 +1186,6 @@ function completeListValue( function completeListItemValue( item: unknown, completedResults: Array, - errors: Array, exeContext: ExecutionContext, itemType: GraphQLOutputType, fieldNodes: ReadonlyArray, @@ -1226,6 +1225,10 @@ function completeListItemValue( // to take a second callback for the error case. completedResults.push( completedItem.then(undefined, (rawError) => { + // FIXME: add coverage for async rejection of a promise item within a deferred payload + const errors = + /* c8 ignore start */ asyncPayloadRecord?.errors ?? + /* c8 ignore stop */ exeContext.errors; addError(rawError, fieldNodes, itemType, itemPath, errors); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); return null; @@ -1237,6 +1240,10 @@ function completeListItemValue( completedResults.push(completedItem); } catch (rawError) { + // FIXME: add coverage for sync rejection of a promise item within a deferred payload + const errors = + /* c8 ignore start */ asyncPayloadRecord?.errors ?? + /* c8 ignore stop */ exeContext.errors; addError(rawError, fieldNodes, itemType, itemPath, errors); filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); completedResults.push(null); From 86a84acc85aa3ed7b54bfe0a5e8e19e43c4c0ce0 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 12 Oct 2022 21:59:11 +0300 Subject: [PATCH 5/5] pass subsequentPayloads rather than entire exeContext --- src/execution/execute.ts | 116 +++++++++++++++++++++++++++++---------- 1 file changed, 87 insertions(+), 29 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index c3da6d0f59..0c8fb1f838 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -363,7 +363,9 @@ function executeImpl( ...initialResult, hasNext: true, }, - subsequentResults: yieldSubsequentPayloads(exeContext), + subsequentResults: yieldSubsequentPayloads( + exeContext.subsequentPayloads, + ), }; } return initialResult; @@ -381,7 +383,9 @@ function executeImpl( ...initialResult, hasNext: true, }, - subsequentResults: yieldSubsequentPayloads(exeContext), + subsequentResults: yieldSubsequentPayloads( + exeContext.subsequentPayloads, + ), }; } return initialResult; @@ -750,7 +754,11 @@ function executeField( return completed.then(undefined, (rawError) => { const errors = asyncPayloadRecord?.errors ?? exeContext.errors; addError(rawError, fieldNodes, returnType, path, errors); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); return null; }); } @@ -758,7 +766,11 @@ function executeField( } catch (rawError) { const errors = asyncPayloadRecord?.errors ?? exeContext.errors; addError(rawError, fieldNodes, returnType, path, errors); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); return null; } } @@ -950,7 +962,11 @@ async function completePromisedValue( } catch (rawError) { const errors = asyncPayloadRecord?.errors ?? exeContext.errors; addError(rawError, fieldNodes, returnType, path, errors); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); return null; } } @@ -1230,7 +1246,11 @@ function completeListItemValue( /* c8 ignore start */ asyncPayloadRecord?.errors ?? /* c8 ignore stop */ exeContext.errors; addError(rawError, fieldNodes, itemType, itemPath, errors); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + itemPath, + asyncPayloadRecord, + ); return null; }), ); @@ -1245,7 +1265,11 @@ function completeListItemValue( /* c8 ignore start */ asyncPayloadRecord?.errors ?? /* c8 ignore stop */ exeContext.errors; addError(rawError, fieldNodes, itemType, itemPath, errors); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + itemPath, + asyncPayloadRecord, + ); completedResults.push(null); } @@ -1838,7 +1862,11 @@ function executeStreamField( (value) => [value], (error) => { asyncPayloadRecord.errors.push(error); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); return null; }, ); @@ -1868,11 +1896,19 @@ function executeStreamField( asyncPayloadRecord.errors, ); completedItem = null; - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + itemPath, + asyncPayloadRecord, + ); } } catch (error) { asyncPayloadRecord.errors.push(error); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); asyncPayloadRecord.addItems(null); return asyncPayloadRecord; } @@ -1887,14 +1923,22 @@ function executeStreamField( itemPath, asyncPayloadRecord.errors, ); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + itemPath, + asyncPayloadRecord, + ); return null; }) .then( (value) => [value], (error) => { asyncPayloadRecord.errors.push(error); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); return null; }, ); @@ -1959,7 +2003,11 @@ async function executeStreamIteratorItem( itemPath, asyncPayloadRecord.errors, ); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + itemPath, + asyncPayloadRecord, + ); return null; }); } @@ -1972,7 +2020,11 @@ async function executeStreamIteratorItem( itemPath, asyncPayloadRecord.errors, ); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + itemPath, + asyncPayloadRecord, + ); return { done: false, value: null }; } } @@ -2015,7 +2067,11 @@ async function executeStreamIterator( ); } catch (error) { asyncPayloadRecord.errors.push(error); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); asyncPayloadRecord.addItems(null); // entire stream has errored and bubbled upwards if (iterator?.return) { @@ -2034,7 +2090,11 @@ async function executeStreamIterator( (value) => [value], (error) => { asyncPayloadRecord.errors.push(error); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + filterSubsequentPayloads( + exeContext.subsequentPayloads, + path, + asyncPayloadRecord, + ); return null; }, ); @@ -2053,12 +2113,12 @@ async function executeStreamIterator( } function filterSubsequentPayloads( - exeContext: ExecutionContext, + subsequentPayloads: Set, nullPath: Path, currentAsyncRecord: AsyncPayloadRecord | undefined, ): void { const nullPathArray = pathToArray(nullPath); - exeContext.subsequentPayloads.forEach((asyncRecord) => { + subsequentPayloads.forEach((asyncRecord) => { if (asyncRecord === currentAsyncRecord) { // don't remove payload from where error originates return; @@ -2075,20 +2135,20 @@ function filterSubsequentPayloads( // ignore error }); } - exeContext.subsequentPayloads.delete(asyncRecord); + subsequentPayloads.delete(asyncRecord); }); } function getCompletedIncrementalResults( - exeContext: ExecutionContext, + subsequentPayloads: Set, ): Array { const incrementalResults: Array = []; - for (const asyncPayloadRecord of exeContext.subsequentPayloads) { + for (const asyncPayloadRecord of subsequentPayloads) { const incrementalResult: IncrementalResult = {}; if (!asyncPayloadRecord.isCompleted) { continue; } - exeContext.subsequentPayloads.delete(asyncPayloadRecord); + subsequentPayloads.delete(asyncPayloadRecord); if (isStreamPayload(asyncPayloadRecord)) { const items = asyncPayloadRecord.items; if (asyncPayloadRecord.isCompletedIterator) { @@ -2114,7 +2174,7 @@ function getCompletedIncrementalResults( } function yieldSubsequentPayloads( - exeContext: ExecutionContext, + subsequentPayloads: Set, ): AsyncGenerator { let isDone = false; @@ -2125,17 +2185,15 @@ function yieldSubsequentPayloads( return { value: undefined, done: true }; } - await Promise.race( - Array.from(exeContext.subsequentPayloads).map((p) => p.promise), - ); + await Promise.race(Array.from(subsequentPayloads).map((p) => p.promise)); if (isDone) { // a different call to next has exhausted all payloads return { value: undefined, done: true }; } - const incremental = getCompletedIncrementalResults(exeContext); - const hasNext = exeContext.subsequentPayloads.size > 0; + const incremental = getCompletedIncrementalResults(subsequentPayloads); + const hasNext = subsequentPayloads.size > 0; if (!incremental.length && hasNext) { return next(); @@ -2153,7 +2211,7 @@ function yieldSubsequentPayloads( function returnStreamIterators() { const promises: Array>> = []; - exeContext.subsequentPayloads.forEach((asyncPayloadRecord) => { + subsequentPayloads.forEach((asyncPayloadRecord) => { if ( isStreamPayload(asyncPayloadRecord) && asyncPayloadRecord.iterator?.return