add abrupt return handling for sync iterables (#4533)

in parallel to handling for async iterables

Author: yaacovCR

src/execution/__tests__/lists-test.ts

@@ -78,6 +78,123 @@ describe('Execute: Accepts any iterable as list value', () => {
   });
 });
 
+describe('Execute: Handles abrupt completion in synchronous iterables', () => {
+  function complete(rootValue: unknown, as: string = '[String]') {
+    return execute({
+      schema: buildSchema(`type Query { listField: ${as} }`),
+      document: parse('{ listField }'),
+      rootValue,
+    });
+  }
+
+  it('closes the iterator when `next` throws', async () => {
+    let returned = false;
+    let nextCalls = 0;
+
+    const listField: IterableIterator<string> = {
+      [Symbol.iterator](): IterableIterator<string> {
+        return this;
+      },
+      next(): IteratorResult<string> {
+        nextCalls++;
+        if (nextCalls === 1) {
+          return { done: false, value: 'ok' };
+        }
+        throw new Error('bad');
+      },
+      return(): IteratorResult<string> {
+        returned = true;
+        return { done: true, value: undefined };
+      },
+    };
+
+    expectJSON(await complete({ listField })).toDeepEqual({
+      data: { listField: null },
+      errors: [
+        {
+          message: 'bad',
+          locations: [{ line: 1, column: 3 }],
+          path: ['listField'],
+        },
+      ],
+    });
+    expect(nextCalls).to.equal(2);
+    expect(returned).to.equal(true);
+  });
+
+  it('closes the iterator when a null bubbles up from a non-null item', async () => {
+    const values = [1, null, 2];
+    let index = 0;
+    let returned = false;
+
+    const listField: IterableIterator<number | null> = {
+      [Symbol.iterator](): IterableIterator<number | null> {
+        return this;
+      },
+      next(): IteratorResult<number | null> {
+        const value = values[index++];
+        if (value === undefined) {
+          return { done: true, value: undefined };
+        }
+        return { done: false, value };
+      },
+      return(): IteratorResult<number | null> {
+        returned = true;
+        return { done: true, value: undefined };
+      },
+    };
+
+    expectJSON(await complete({ listField }, '[Int!]')).toDeepEqual({
+      data: { listField: null },
+      errors: [
+        {
+          message: 'Cannot return null for non-nullable field Query.listField.',
+          locations: [{ line: 1, column: 3 }],
+          path: ['listField', 1],
+        },
+      ],
+    });
+    expect(index).to.equal(2);
+    expect(returned).to.equal(true);
+  });
+
+  it('ignores errors thrown by the iterator `return` method', async () => {
+    const values = [1, null, 2];
+    let index = 0;
+    let returned = false;
+
+    const listField: IterableIterator<number | null> = {
+      [Symbol.iterator](): IterableIterator<number | null> {
+        return this;
+      },
+      next(): IteratorResult<number | null> {
+        const value = values[index++];
+        if (value === undefined) {
+          return { done: true, value: undefined };
+        }
+        return { done: false, value };
+      },
+      return(): IteratorResult<number | null> {
+        returned = true;
+        throw new Error('ignored return error');
+      },
+    };
+
+    expectJSON(await complete({ listField }, '[Int!]')).toDeepEqual({
+      data: { listField: null },
+      errors: [
+        {
+          message: 'Cannot return null for non-nullable field Query.listField.',
+          locations: [{ line: 1, column: 3 }],
+          path: ['listField', 1],
+        },
+      ],
+    });
+    expect(index).to.equal(2);
+    expect(returned).to.equal(true);
+  });
+});
+
 describe('Execute: Accepts async iterables as list value', () => {
   function complete(rootValue: unknown, as: string = '[String]') {
     return execute({

src/execution/execute.ts

@@ -703,16 +703,17 @@ async function completePromisedValue(
  * Complete a async iterator value by completing the result and calling
  * recursively until all the results are completed.
  */
-async function completeAsyncIteratorValue(
+async function completeAsyncIterableValue(
   exeContext: ExecutionContext,
   itemType: GraphQLOutputType,
   fieldDetailsList: FieldDetailsList,
   info: GraphQLResolveInfo,
   path: Path,
-  asyncIterator: AsyncIterator<unknown>,
+  items: AsyncIterable<unknown>,
 ): Promise<ReadonlyArray<unknown>> {
   let containsPromise = false;
   const completedResults: Array<unknown> = [];
+  const asyncIterator = items[Symbol.asyncIterator]();
   let index = 0;
   try {
     while (true) {
@@ -762,10 +763,7 @@ async function completeAsyncIteratorValue(
       index++;
     }
   } catch (error) {
-    asyncIterator.return?.().catch(() => {
-      /* c8 ignore next 1 */
-      // ignore error
-    });
+    returnIteratorIgnoringErrors(asyncIterator);
     throw error;
   }
   return containsPromise ? Promise.all(completedResults) : completedResults;
@@ -790,15 +788,14 @@ function completeListValue(
     const maybeCancellableIterable = abortSignalListener
       ? cancellableIterable(result, abortSignalListener)
       : result;
-    const asyncIterator = maybeCancellableIterable[Symbol.asyncIterator]();
 
-    return completeAsyncIteratorValue(
+    return completeAsyncIterableValue(
       exeContext,
       itemType,
       fieldDetailsList,
       info,
       path,
-      asyncIterator,
+      maybeCancellableIterable,
     );
   }
 
@@ -826,48 +823,53 @@ function completeIterableValue(
   path: Path,
   items: Iterable<unknown>,
 ): PromiseOrValue<ReadonlyArray<unknown>> {
-  // 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 completedResults: Array<unknown> = [];
   let index = 0;
   const iterator = items[Symbol.iterator]();
-  let iteration = iterator.next();
-  while (!iteration.done) {
-    const item = iteration.value;
+  try {
+    while (true) {
+      const iteration = iterator.next();
+      if (iteration.done) {
+        break;
+      }
+
+      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);
+      // 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 (isPromise(item)) {
-      completedResults.push(
-        completePromisedListItemValue(
+      if (isPromise(item)) {
+        completedResults.push(
+          completePromisedListItemValue(
+            item,
+            exeContext,
+            itemType,
+            fieldDetailsList,
+            info,
+            itemPath,
+          ),
+        );
+        containsPromise = true;
+      } else if (
+        completeListItemValue(
           item,
+          completedResults,
           exeContext,
           itemType,
           fieldDetailsList,
           info,
           itemPath,
-        ),
-      );
-      containsPromise = true;
-    } else if (
-      completeListItemValue(
-        item,
-        completedResults,
-        exeContext,
-        itemType,
-        fieldDetailsList,
-        info,
-        itemPath,
-      )
-    ) {
-      containsPromise = true;
+        )
+      ) {
+        containsPromise = true;
+      }
+      index++;
     }
-
-    index++;
-    iteration = iterator.next();
+  } catch (error) {
+    returnIteratorIgnoringErrors(iterator);
+    throw error;
   }
 
   return containsPromise ? Promise.all(completedResults) : completedResults;
@@ -1360,3 +1362,18 @@ function assertEventStream(result: unknown): AsyncIterable<unknown> {
 
   return result;
 }
+
+function returnIteratorIgnoringErrors(
+  iterator: Iterator<unknown> | AsyncIterator<unknown>,
+) {
+  try {
+    const result = iterator.return?.();
+    if (isPromise(result)) {
+      result.catch(() => {
+        // ignore errors
+      });
+    }
+  } catch {
+    // ignore errors
+  }
+}