-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||
|
@@ -171,19 +172,28 @@ function subscribeImpl( | |||||||||||||||||||||||
reportGraphQLError, | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
: ((resultOrStream: any): ExecutionResult), | ||||||||||||||||||||||||
reportGraphQLError, | ||||||||||||||||||||||||
); | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
/** | ||||||||||||||||||||||||
* 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. | ||||||||||||||||||||||||
|
@@ -270,7 +280,11 @@ export function createSourceEventStream( | |||||||||||||||||||||||
return Promise.resolve(result).then(eventStream => { | ||||||||||||||||||||||||
// If eventStream is an Error, rethrow a located error. | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||||||||||||||||||||||||
|
@@ -284,6 +298,8 @@ export function createSourceEventStream( | |||||||||||||||||||||||
); | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
} catch (error) { | ||||||||||||||||||||||||
return Promise.reject(error); | ||||||||||||||||||||||||
return error instanceof GraphQLError | ||||||||||||||||||||||||
? Promise.resolve({ errors: [error] }) | ||||||||||||||||||||||||
: Promise.reject(error); | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This matches the logic for: graphql-js/src/subscription/subscribe.js Lines 114 to 124 in b24cbcd
GraphQL errors resolve the promise as an There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||||||||||||||||||||
} | ||||||||||||||||||||||||
} |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.