Update subscribe function to return a Promise of AsyncIterator or ExecutionResult #918

robzhu · 2017-06-16T22:37:28Z

This change updates the Subscribe() function to return a Promise<AsyncIterator | ExecutionResult>.

The reason for this change is to allow callers to distinguish between initialization and publish errors.

When calling subscribe:
If the operation is successful, returns an AsyncIterator representing the response stream.
If the operation failed with an informative error for the client, returns an ExecutionResult with an errors field but no value field.
If the operation failed due to a system related problem, such as an unavailable pubsub, system, an error will be thrown.

Example of how to call subscribe after these changes:

try {
  const result = await subscribe(...);
  if (result.errors) {
    // errors are informative and can be sent to the client
  } else {
    // result is guaranteed to be an AsyncIterable at this point
    return result;
  }
} catch (error) {
  // system error, log the problem and send a failure response to the client.
}

Currently the `subscribe()` function throws Errors, however this is awkward when used along with async functions which would expect a rejected iteration to represent failure. Also GraphQLErrors should be reported back to the client since they represent client-provided errors. Updates test cases to represent this new behavior. Includes a new utility `asyncIteratorReject`, and extends the behavior of `mapAsyncIterator` to help implement this.

If a subscribe resolve function throws or returns an error, that typically indicates an issue to be returned to the requesting client. This coerces errors into located GraphQLErrors so they are correctly reported.

After discussion in #868, decided that errors emitted from a source event stream should be considered "internal" errors and pass through. However errors encountered during GraphQL execution on a source event should be considered "field" or "query" errors and be represented within that Response.

…/ExecutionResult

Currently the `subscribe()` function throws Errors, however this is awkward when used along with async functions which would expect a rejected iteration to represent failure. Also GraphQLErrors should be reported back to the client since they represent client-provided errors. Updates test cases to represent this new behavior. Includes a new utility `asyncIteratorReject`, and extends the behavior of `mapAsyncIterator` to help implement this.

If a subscribe resolve function throws or returns an error, that typically indicates an issue to be returned to the requesting client. This coerces errors into located GraphQLErrors so they are correctly reported.

After discussion in #868, decided that errors emitted from a source event stream should be considered "internal" errors and pass through. However errors encountered during GraphQL execution on a source event should be considered "field" or "query" errors and be represented within that Response.

…/ExecutionResult

…ql/graphql-js into subscription-promise-union

leebyron

This looks awesome. Sorry for the long wait on a review. I've added a few line items for improvements and some additional suggested tests, but this API is much better.

A follow-up to this should be to extend the top level graphql() function to directly support subscriptions!

leebyron · 2017-07-05T12:27:07Z

src/subscription/subscribe.js

@@ -32,10 +35,17 @@ import type { GraphQLFieldResolver } from '../type/definition';
 /**
 * Implements the "Subscribe" algorithm described in the GraphQL specification.
 *
- * Returns an AsyncIterator
+ * Returns a Promise<AsyncIterator | Error | ExecutionResult>


Promise<AsyncIterator | ExecutionResult>

Or maybe it's easier to read if this spells it out. Something like:

Returns a Promise which resolves to either an ExecutionResult (indicating failure to subscribe) or an AsyncIterator of ExecutionResult values, triggered by the underlying subscribed event stream.

leebyron · 2017-07-05T12:29:26Z

src/subscription/subscribe.js

- * If the arguments to this function do not result in a legal execution context,
- * a GraphQLError will be thrown immediately explaining the invalid input.
+ * If the the source stream could not be created due to faulty subscription
+ * resolver logic or underlying systems, an Error will be returned, not thrown.


Promise rejected?

leebyron · 2017-07-05T12:29:57Z

src/subscription/subscribe.js

@@ -59,7 +69,7 @@ declare function subscribe(
  operationName?: ?string,
  fieldResolver?: ?GraphQLFieldResolver<any, any>,
  subscribeFieldResolver?: ?GraphQLFieldResolver<any, any>
-): AsyncIterator<ExecutionResult>;
+): Promise<AsyncIterator<ExecutionResult> | Error | ExecutionResult>;


This type should match the type above

leebyron · 2017-07-05T12:31:12Z

src/subscription/subscribe.js

@@ -96,6 +107,16 @@ export function subscribe(
    );
 }

+// This function checks if the error is a GraphQLError. If it is, convert it to


Nits:

Use /* for multi-line doc blocks.

Also, it would be nice to move this fn lower in the file - keeping subscribe() and subscribeImpl() directly next to each other in the file.

leebyron · 2017-07-05T12:32:10Z

src/subscription/subscribe.js

-      fieldResolver
-    )
-  );
+  ).then(subscription => {


remove the wrapping {} and return since this arrow function is of a single expression

leebyron · 2017-07-05T12:34:07Z

src/subscription/subscribe.js

+      ),
+      convertOrThrowError
+    );
+  }).catch(convertOrThrowError);


You can provide convertOrThrowError as the 2nd argument to then() - which both reduces the number of wrapped promises, but also avoids calling convertOrThrowError two times in the case an error is thrown.

Moved convertOrThrowError to then and pulled out MapSourceToResponseEvent definition to align more clearly with the spec.

leebyron · 2017-07-05T12:40:15Z

src/subscription/subscribe.js

-  if (subscription instanceof Error) {
-    throw subscription;
-  }
+  return Promise.resolve(subscription).then(resolvedSubscription => {


buildExecutionContext above may throw a GraphQLError if client-provided values don't coerce properly - those should be properly formatted in the response via returning a rejected promise. You can test this by passing bad variable values along with a query.

The fix should probably be to wrap return new Promise(resolve => {})starting above line 196, ending in resolve(subscription) and followed by this .then()

Did you mean returning an ExecutionResult instead of rejecting the Promise? Bad variables seems to be an error case for which we can give the client useful feedback.

I added a test case that validates bad query variables are returned as an ExecutionResult. However, the return new Promise approach you suggested led to pretty gnarly logic for dealing with the nested promise coming back from resolveFieldValueOrError. The current version uses await/async to work around it, but happy to revise if you don't think the tradeoff in code clarity is worth it.

Here's what the code looks like using a wrapped promise: https://gist.github.com/robzhu/d2d9e3238b586e67569646a5944154b8

cedrelek · 2017-07-14T13:48:00Z

Thx for this. I hope the next release will include this PR :)

leebyron · 2017-08-14T01:51:24Z

src/subscription/subscribe.js

 *
 * This may be useful when hosting the stateful subscription service in a
 * different process or machine than the stateless GraphQL execution engine,
 * or otherwise separating these two steps. For more on this, see the
 * "Supporting Subscriptions at Scale" information in the GraphQL specification.
 */
-export function createSourceEventStream(
+export async function createSourceEventStream(


My only mild concern is that by including an async function in non-test code that we may need to add a dependency to package.json for an async function runtime.

As a follow-up task, let's test this to see if we need to do so, ideally we can ensure no new dependencies.

leebyron · 2017-08-14T01:58:00Z

No excuse for my long delay on this supremely excellent PR. Error behavior is so difficult, and getting all the corner cases correct and well tested deserves a serious shout out. Thank you for making GraphQL more awesome!

leebyron and others added 6 commits May 30, 2017 13:33

Subscriptions: Respond with error when failing to create source

02ded50

If a subscribe resolve function throws or returns an error, that typically indicates an issue to be returned to the requesting client. This coerces errors into located GraphQLErrors so they are correctly reported.

Update subscribe signature to return a promise of AsyncIterator/Error…

15b8251

…/ExecutionResult

Throw errors instead of returning them from Subscribe()

9c377ce

Lint, refactor error mapper and add comments

8816dbf

robzhu requested a review from leebyron June 16, 2017 22:37

facebook-github-bot added the CLA Signed label Jun 16, 2017

leebyron and others added 8 commits June 18, 2017 14:58

Subscriptions: Respond with error when failing to create source

0132123

If a subscribe resolve function throws or returns an error, that typically indicates an issue to be returned to the requesting client. This coerces errors into located GraphQLErrors so they are correctly reported.

Update subscribe signature to return a promise of AsyncIterator/Error…

a947ca5

…/ExecutionResult

Throw errors instead of returning them from Subscribe()

acf5041

Lint, refactor error mapper and add comments

fedd7fd

Merge branch 'subscription-promise-union' of https://github.com/graph…

2fc393b

…ql/graphql-js into subscription-promise-union

Update test case assertions for stream errors to be more precise

3c166a0

robzhu mentioned this pull request Jun 19, 2017

Allow subscribe function to return a Promise #895

Closed

leebyron approved these changes Jul 5, 2017

View reviewed changes

Robert Zhu added 2 commits July 11, 2017 17:54

Add test case for wrong variable types

8a8bf4b

Fix lint error (extra spaces)

ff6c202

Robert Zhu and others added 2 commits July 14, 2017 14:37

Fix multi-line doc block format

d70e4d5

Minor edits

3ed9ad2

leebyron reviewed Aug 14, 2017

View reviewed changes

Trim trailing whitespace

3f52a41

leebyron merged commit 0779c36 into master Aug 14, 2017

leebyron deleted the subscription-promise-union branch August 14, 2017 01:58

IvanGoncharov mentioned this pull request Aug 27, 2017

Flow error in subscription/mapAsyncIterator in 0.11.1 #1011

Closed

kachkaev mentioned this pull request Aug 28, 2017

An error has occured for subscription 'chatMessageAdded' # apollographql/apollo-server#528

Closed

groundmuffin mentioned this pull request Sep 12, 2017

Add AsyncIterator Support cdmbase/graphql-rabbitmq-subscriptions#8

Merged

glentakahashi mentioned this pull request Nov 25, 2019

Support async topics apollographql/graphql-subscriptions#213

Closed

yaacovCR mentioned this pull request Mar 19, 2024

Errors thrown when iterating over subscription source event streams (AsyncIterables) should be caught #4001

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update subscribe function to return a Promise of AsyncIterator or ExecutionResult #918

Update subscribe function to return a Promise of AsyncIterator or ExecutionResult #918

robzhu commented Jun 16, 2017 •

edited

Loading

leebyron left a comment

leebyron Jul 5, 2017

leebyron Jul 5, 2017

leebyron Jul 5, 2017

leebyron Jul 5, 2017

leebyron Jul 5, 2017

leebyron Jul 5, 2017

leebyron Jul 5, 2017

robzhu Jul 11, 2017

leebyron Jul 5, 2017

robzhu Jul 11, 2017

robzhu Jul 11, 2017 •

edited

Loading

robzhu Jul 14, 2017

cedrelek commented Jul 14, 2017

leebyron Aug 14, 2017

leebyron commented Aug 14, 2017

Update subscribe function to return a Promise of AsyncIterator or ExecutionResult #918

Update subscribe function to return a Promise of AsyncIterator or ExecutionResult #918

Conversation

robzhu commented Jun 16, 2017 • edited Loading

leebyron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robzhu Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedrelek commented Jul 14, 2017

Choose a reason for hiding this comment

leebyron commented Aug 14, 2017

robzhu commented Jun 16, 2017 •

edited

Loading

robzhu Jul 11, 2017 •

edited

Loading