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
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
34 changes: 25 additions & 9 deletions src/subscription/subscribe.js
Expand Up @@ -35,8 +35,9 @@ import { getOperationRootType } from '../utilities/getOperationRootType';
* Implements the "Subscribe" algorithm described in the GraphQL specification.
*
* Returns a Promise which resolves to either an AsyncIterator (if successful)
* or an ExecutionResult (client error). The promise will be rejected if a
* server error occurs.
* or an ExecutionResult (error). The promise will be rejected if the schema or
* other arguments to this function are invalid, or if the resolved event stream
* is not an async iterable.
*
* If the client-provided arguments to this function do not result in a
* compliant subscription, a GraphQL Response (ExecutionResult) with
Expand Down Expand Up @@ -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.

);
}

/**
* Implements the "CreateSourceEventStream" algorithm described in the
* GraphQL specification, resolving the subscription source event stream.
*
* Returns a Promise<AsyncIterable>.
* Returns a Promise which resolves to either an AsyncIterable (if successful)
* or an ExecutionResult (error). The promise will be rejected if the schema or
* other arguments to this function are invalid, or if the resolved event stream
* is not an async iterable.
*
* If the client-provided invalid arguments, the source stream could not be
* created, or the resolver did not return an AsyncIterable, this function will
* will throw an error, which should be caught and handled by the caller.
* If the client-provided arguments to this function do not result in a
* compliant subscription, a GraphQL Response (ExecutionResult) with
* descriptive errors and no data will be returned.
*
* If the the source stream could not be created due to faulty subscription
* resolver logic or underlying systems, the promise will resolve to a single
* ExecutionResult containing `errors` and no `data`.
*
* If the operation succeeded, the promise resolves to the AsyncIterable for the
* event stream returned by the resolver.
*
* A Source Event Stream represents a sequence of events, each of which triggers
* a GraphQL execution for that event.
Expand Down Expand Up @@ -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.

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?

errors: [
locatedError(eventStream, fieldNodes, responsePathAsArray(path)),
],
};
}

// Assert field returned an event stream, otherwise yield an error.
Expand All @@ -284,6 +298,8 @@ export function createSourceEventStream(
);
});
} catch (error) {
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?

}
}