Repository: graphql/graphql-js
Pull request #3621 (2022-05-31 11:20:52)

change incorrect subscribe return type to a GraphQLError
…rather than a systems error

to match spec (in parallel to field-level resolver errors in execute)

note the addition of the path, as execution has begun

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *compassionate-pike-271cb3* processing.


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 30f65966cb82f1ab08629b0a87e07567194ad94d |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6295fa150c5f7400096c441f |
yaacovCR: I think fixing the entire spec in terms of how it clarifies errors is beyond the scope of this PR. See also https://github.com/graphql/graphql-spec/pull/894

And the entire `Response` section of the spec doesn't take into account subscriptions, so it's tough to say exactly what's going on, but more on that below.

Fundamentally, however, I imagine that the spec purports there to be unified behavior as much as possible between all operations, as within the spec they are all considered different forms of execution, with subscriptions simply a different operation type. That is why this PR, which is one of several on the road toward integrating subscriptions into `execute`, should require no spec change and actually is more "correct" by the spec.

More specifically, there is a rough equivalence given between `ResolveFieldEventStream` and `ResolveFieldValue` and insofar as errors within root field resolvers are field errors for `ResolveFieldValue` with a path ([see your comment, actually](https://github.com/graphql/graphql-spec/pull/894#issuecomment-953037385), they are presumably field errors also for subscriptions. Just as request errors get a map of just errors for subscriptions, so, too, should root field errors.

In my opinion. :)

Patch:
diff --git a/src/execution/subscribe.ts b/src/execution/subscribe.ts
index 7a04480b..e5494983 100644
--- a/src/execution/subscribe.ts
+++ b/src/execution/subscribe.ts
@@ -157,28 +157,15 @@ export async function createSourceEventStream(
   try {
     const eventStream = await executeSubscription(exeContext);
 
-    // Assert field returned an event stream, otherwise yield an error.
-    if (!isAsyncIterable(eventStream)) {
-      throw new Error(
-        'Subscription field must return Async Iterable. ' +
-          `Received: ${inspect(eventStream)}.`,
-      );
-    }
-
     return eventStream;
   } catch (error) {
-    // If it GraphQLError, report it as an ExecutionResult, containing only errors and no data.
-    // Otherwise treat the error as a system-class error and re-throw it.
-    if (error instanceof GraphQLError) {
-      return { errors: [error] };
-    }
-    throw error;
+    return { errors: [error] };
   }
 }
 
 async function executeSubscription(
   exeContext: ExecutionContext,
-): Promise<unknown> {
+): Promise<AsyncIterable<unknown>> {
   const { schema, fragments, operation, variableValues, rootValue } =
     exeContext;
 
@@ -238,6 +225,15 @@ async function executeSubscription(
     if (eventStream instanceof Error) {
       throw eventStream;
     }
+
+    // Assert field returned an event stream, otherwise yield an error.
+    if (!isAsyncIterable(eventStream)) {
+      throw new GraphQLError(
+        'Subscription field must return Async Iterable. ' +
+          `Received: ${inspect(eventStream)}.`,
+      );
+    }
+
     return eventStream;
   } catch (error) {
     throw locatedError(error, fieldNodes, pathToArray(path));


Test patch:
diff --git a/src/execution/__tests__/subscribe-test.ts b/src/execution/__tests__/subscribe-test.ts
index c8f4aca7..d943ef40 100644
--- a/src/execution/__tests__/subscribe-test.ts
+++ b/src/execution/__tests__/subscribe-test.ts
@@ -454,9 +454,16 @@ describe('Subscription Initialization Phase', () => {
   });
 
   it('throws an error if subscribe does not return an iterator', async () => {
-    (await expectPromise(subscribeWithBadFn(() => 'test'))).toRejectWith(
-      'Subscription field must return Async Iterable. Received: "test".',
-    );
+    expectJSON(await subscribeWithBadFn(() => 'test')).toDeepEqual({
+      errors: [
+        {
+          message:
+            'Subscription field must return Async Iterable. Received: "test".',
+          locations: [{ line: 1, column: 16 }],
+          path: ['foo'],
+        },
+      ],
+    });
   });
 
   it('resolves to an error for subscription resolver errors', async () => {