Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make error handling consistent in createSourceEventStream #1467

Merged

Conversation

taion
Copy link
Contributor

@taion taion commented Aug 13, 2018

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 taion closed this Aug 13, 2018
@taion taion deleted the createSourceEventStream-error-handling branch August 13, 2018 16:56
@taion taion restored the createSourceEventStream-error-handling branch August 13, 2018 16:59
@taion taion reopened this Aug 13, 2018
@taion
Copy link
Contributor Author

taion commented Aug 13, 2018

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
Copy link
Contributor Author

taion commented Aug 13, 2018

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

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Aug 13, 2018

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

return Promise.reject(error);
return error instanceof GraphQLError
? Promise.resolve({ errors: [error] })
: Promise.reject(error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This matches the logic for:

/**
* 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@taion
Copy link
Contributor Author

taion commented Aug 18, 2018

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).

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@taion
Copy link
Contributor Author

taion commented Nov 19, 2018

Any updates here?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to be updated.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

robzhu commented Nov 21, 2018

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
Copy link
Contributor Author

taion commented Nov 23, 2018

@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
Copy link
Contributor

robzhu commented Nov 24, 2018

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
Copy link
Contributor Author

taion commented Nov 24, 2018

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
Copy link
Contributor Author

taion commented Mar 8, 2019

Any update here? Can we move forward with this?

@taion
Copy link
Contributor Author

taion commented Jun 25, 2019

Any updates here?

@robzhu
Copy link
Contributor

robzhu commented Jul 3, 2019

Looks good to me. @IvanGoncharov any other concerns?

@IvanGoncharov IvanGoncharov added bug PR: bug fix 🐞 requires increase of "patch" version number and removed bug labels Jul 4, 2019
@IvanGoncharov 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
IvanGoncharov added a commit that referenced this pull request Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants