including mixture of sync/async errors in lists

yaacovCR · yaacovCR · commit bac21838d03d · 2023-01-03T14:46:17.000+02:00
following #3706 fix for field execution
diff --git 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<ExecutionResult> {
+    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
@@ -1029,59 +1029,69 @@ async function completeAsyncIteratorValue(
   let containsPromise = false;
   const completedResults: Array<unknown> = [];
   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,70 @@ function completeListValue(
   let previousAsyncPayloadRecord = asyncPayloadRecord;
   const completedResults: Array<unknown> = [];
   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)) {
+        completedResults.push(item);
+      }
+      iteration = iterator.next();
     }
-
-    index++;
+    if (containsPromise) {
+      return Promise.all(completedResults).finally(() => {
+        throw error;
+      });
+    }
+    throw error;
   }
 
   return containsPromise ? Promise.all(completedResults) : completedResults;
