Make error handling consistent in createSourceEventStream #1467

taion · 2018-08-13T16:55:37Z

Currently, errors in createSourceEventStream from buildExecutionContext will be returned in an ExecutionResult, while all other GraphQL errors will be thrown. This is inconsistent with the handling in subscribe, and makes it harder to build error handling logic, as there are then two ways to handle the same class of errors.

This makes the error handling consistent and updates the documentation to describe the new logic.

This is technically breaking, but is unlikely to cause problems for any properly-written code, as full error handling logic required handling resolved ExecutionResult objects with errors anyway.

taion · 2018-08-13T16:59:52Z

Sorry, I accidentally deleted the PR branch while cleaning up my fork. The build failed because that branch got deleted while it was running.

Can someone re-run the build?

taion · 2018-08-13T17:00:01Z

Ah, never mind, it's re-running on its own.

IvanGoncharov · 2018-08-13T19:25:02Z

@taion Can you please add test cases that would cover behavior that you described?

IvanGoncharov · 2018-08-13T19:30:28Z

src/subscription/subscribe.js

-    return Promise.reject(error);
+    return error instanceof GraphQLError
+      ? Promise.resolve({ errors: [error] })
+      : Promise.reject(error);


@taion Not sure that I fully understand this code, but why need different behavior for Error and GraphQLError?

This matches the logic for:

graphql-js/src/subscription/subscribe.js

Lines 114 to 124 in b24cbcd

/**

* This function checks if the error is a GraphQLError. If it is, report it as

* an ExecutionResult, containing only errors and no data. Otherwise treat the

* error as a system-class error and re-throw it.

*/

function reportGraphQLError(error) {

if (error instanceof GraphQLError) {

return { errors: [error] };

}

throw error;

}

GraphQL errors resolve the promise as an ExecutionResult with just errors, while other errors reject the promise.

@taion Ok, maybe this code will make it more obvious:

return new Promise(resolve => resolve(reportGraphQLError(error)));

It would be, but is it worth the complexity? What do you think of just adding a comment?

taion · 2018-08-14T00:37:47Z

src/subscription/subscribe.js

@@ -171,19 +172,28 @@ function subscribeImpl(
            reportGraphQLError,
          )
        : ((resultOrStream: any): ExecutionResult),
-    reportGraphQLError,


@IvanGoncharov The removal of this "catch" handler means that the existing tests for error handling in subscribe already do exercise the new logic. Do you still think explicit new tests are needed?

@taion I mean you change a code to change how certain errors handled and no test cases were broken. It means that there no test cases covering this behaviour and without test cases, we can't be sure it's working as expected.

Plus coverage for subscribe.js decreased from 100% in this PR:
https://coveralls.io/builds/18460657/source?filename=src/subscription/subscribe.js#L121

Ah, I didn't see the Coveralls check. I'll add explicit test coverage then.

taion · 2018-08-18T03:04:16Z

I believe I've addressed the PR comments and resolved the coverage hit (which came from an existing code path not being covered by tests).

taion · 2018-08-18T03:14:13Z

src/subscription/__tests__/subscribe-test.js

+    const erroringEmailSchema = emailSchemaWithResolvers(
+      async function*() {
+        yield { email: { subject: 'Hello' } };
+        throw new GraphQLError('test error');


this is sort of silly, though.

i wonder if we should just drop reportGraphQLError there entirely; the case where the async iterable throws a graphql error seems exceedingly unlikely

IvanGoncharov · 2018-08-19T14:38:29Z

@robzhu @leebyron Can you please take a look at this PR?

taion · 2018-11-19T16:34:55Z

Any updates here?

robzhu · 2018-11-21T16:04:59Z

src/subscription/subscribe.js

@@ -270,7 +280,11 @@ export function createSourceEventStream(
    return Promise.resolve(result).then(eventStream => {
      // If eventStream is an Error, rethrow a located error.


This comment needs to be updated.

robzhu · 2018-11-21T16:11:56Z

src/subscription/subscribe.js

@@ -270,7 +280,11 @@ export function createSourceEventStream(
    return Promise.resolve(result).then(eventStream => {
      // If eventStream is an Error, rethrow a located error.
      if (eventStream instanceof Error) {
-        throw locatedError(eventStream, fieldNodes, responsePathAsArray(path));
+        return {


What if this error is something like, "Redis connection to 1.1.1.8:6379 failed"? Wouldn't this get wrapped into a GraphQLError and returned to the client?

robzhu · 2018-11-21T16:17:22Z

The reason for this seemingly inconsistent error handling behavior in createSourceEventStream is that we wanted to provide more control over error handling. GraphQLErrors are typically communicated back to the client-- that is, errors that contain useful information for the client, similar to the HTTP 400 status code. However, createSourceEventStream would typically depend on some sort of real-time event bus, for which failures are generally going to be 500-class errors (the server's fault, not the client's), and detailed error messages should not be sent to the client. As it's currently implemented, the caller is expected to catch all exceptions coming from createSourceEventStream and can still decide to wrap it in a GraphQLError if appropriate. Does that make sense?

taion · 2018-11-23T20:49:43Z

@robzhu

I don't understand the difference – doesn't subscribe also depend on the same sorts of real-time event buses? Per your comment on #1467 (comment), even when using the higher-level API, 500-class errors like failures to connect to e.g. Redis would get reported as GraphQL errors to the client.

But this is also consistent with what would happen for non-subscription fields. If e.g. the GraphQL server were unable to connect to some upstream service in resolving a normal (non-subscription) field, even when this is a 500-class error, it would still get surfaced as a GraphQL error to the client.

The only case right now where this doesn't happen is in the case where subscribe on the subscription field doesn't return an async iterable. I'm not totally sure that this is the correct behavior, but at least there isn't an inconsistency there.

robzhu · 2018-11-24T00:06:16Z

The only case right now where this doesn't happen is in the case where subscribe on the subscription field doesn't return an async iterable. I'm not totally sure that this is the correct behavior, but at least there isn't an inconsistency there.

Ah, my misunderstanding then. If what you're describing is true, then I think this change makes sense.

taion · 2018-11-24T00:33:00Z

To clarify, the behavior of subscribe isn't changing at all, which is why none of the existing test cases needed to be changed. Failures in the subscribe method on a subscription field continue to surface errors to clients when using the subscribe API here.

This purely serves to unify the error handling a bit between when one uses createSourceEventStream v. when one uses subscribe, and only to remove a case where the GraphQLError is thrown rather than returned in an ExecutionResult when using the lower-level API. The higher-level API never throws a GraphQLError and only returns them in ExecutionResult objects.

taion · 2019-03-08T16:45:08Z

Any update here? Can we move forward with this?

taion · 2019-06-25T16:52:12Z

Any updates here?

robzhu · 2019-07-03T20:07:14Z

Looks good to me. @IvanGoncharov any other concerns?

Make error handling consistent in createSourceEventStream

b24cbcd

facebook-github-bot added the CLA Signed label Aug 13, 2018

taion closed this Aug 13, 2018

taion deleted the createSourceEventStream-error-handling branch August 13, 2018 16:56

taion restored the createSourceEventStream-error-handling branch August 13, 2018 16:59

taion reopened this Aug 13, 2018

IvanGoncharov reviewed Aug 13, 2018

View reviewed changes

taion commented Aug 14, 2018

View reviewed changes

taion added 2 commits August 17, 2018 20:41

Merge branch 'master' into createSourceEventStream-error-handling

e83e676

Address PR feedback

889344d

Actually fix coverage

c7b3344

taion commented Aug 18, 2018

View reviewed changes

IvanGoncharov requested review from leebyron and robzhu August 19, 2018 14:38

robzhu reviewed Nov 21, 2018

View reviewed changes

IvanGoncharov force-pushed the master branch from e7509a6 to 16f5ca0 Compare June 26, 2019 17:05

IvanGoncharov added bug PR: bug fix 🐞 requires increase of "patch" version number and removed bug labels Jul 4, 2019

IvanGoncharov merged commit d4a1362 into graphql:master Jul 4, 2019

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jul 4, 2019

Fix build after graphql#1467

39701e5

IvanGoncharov added a commit that referenced this pull request Jul 4, 2019

Fix build after #1467 (#2014)

2afbf0b

snyk-bot mentioned this pull request Sep 11, 2019

[Snyk] Upgrade graphql from 14.0.2 to 14.5.4 JoeyFenny/fullstack-apollo-express-mongodb-boilerplate#20

Open

This was referenced Oct 26, 2019

[Snyk] Upgrade graphql from 0.10.5 to 14.5.8 ajesse11x/graphiql#1

Open

[Snyk] Upgrade graphql from 0.5.0 to 14.5.8 ajesse11x/reindex-api#2

Open

snyk-bot mentioned this pull request Dec 5, 2019

[Snyk] Upgrade graphql from 14.2.1 to 14.5.8 dyna-dot/github-atom-fork#1

Open

snyk-bot mentioned this pull request Jan 16, 2020

[Snyk] Upgrade graphql from 14.1.1 to 14.5.8 silver-xu/yame-api#6

Open

This was referenced Feb 12, 2020

[Snyk] Upgrade graphql from 14.0.2 to 14.5.8 EverlongProject/create-react-app#12

Closed

[Snyk] Upgrade graphql from 14.0.2 to 14.5.8 DavidKindler/fullstack-apollo-express-postgresql-boilerplate#8

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error handling consistent in createSourceEventStream #1467

Make error handling consistent in createSourceEventStream #1467

taion commented Aug 13, 2018

taion commented Aug 13, 2018

taion commented Aug 13, 2018

IvanGoncharov commented Aug 13, 2018 •

edited

Loading

IvanGoncharov Aug 13, 2018

taion Aug 13, 2018

IvanGoncharov Aug 13, 2018 •

edited

Loading

taion Aug 14, 2018

taion Aug 14, 2018

IvanGoncharov Aug 14, 2018

taion Aug 14, 2018

taion commented Aug 18, 2018

taion Aug 18, 2018

IvanGoncharov commented Aug 19, 2018

taion commented Nov 19, 2018

robzhu Nov 21, 2018

robzhu Nov 21, 2018

robzhu commented Nov 21, 2018

taion commented Nov 23, 2018

robzhu commented Nov 24, 2018

taion commented Nov 24, 2018

taion commented Mar 8, 2019

taion commented Jun 25, 2019

robzhu commented Jul 3, 2019

	/**
	* This function checks if the error is a GraphQLError. If it is, report it as
	* an ExecutionResult, containing only errors and no data. Otherwise treat the
	* error as a system-class error and re-throw it.
	*/
	function reportGraphQLError(error) {
	if (error instanceof GraphQLError) {
	return { errors: [error] };
	}
	throw error;
	}

		@@ -270,7 +280,11 @@ export function createSourceEventStream(
		return Promise.resolve(result).then(eventStream => {
		// If eventStream is an Error, rethrow a located error.

Make error handling consistent in createSourceEventStream #1467

Make error handling consistent in createSourceEventStream #1467

Conversation

taion commented Aug 13, 2018

taion commented Aug 13, 2018

taion commented Aug 13, 2018

IvanGoncharov commented Aug 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov Aug 13, 2018 • edited Loading

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

taion commented Aug 18, 2018

Choose a reason for hiding this comment

IvanGoncharov commented Aug 19, 2018

taion commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robzhu commented Nov 21, 2018

taion commented Nov 23, 2018

robzhu commented Nov 24, 2018

taion commented Nov 24, 2018

taion commented Mar 8, 2019

taion commented Jun 25, 2019

robzhu commented Jul 3, 2019

IvanGoncharov commented Aug 13, 2018 •

edited

Loading

IvanGoncharov Aug 13, 2018 •

edited

Loading