Simplify DgsSSESubscriptionHandler by returning Flux #1001
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DgsSSESubscriptionHandler was rolling its own Subscriber implementation in order to adapt the Publisher returned by the GraphQL query to the SseEmitter; however, Spring itself will handle this when the content type is "text/event-stream" and we simply return a reactive type, such as Flux. Doing so greatly simplifies the code in the controller, and allows for future enhancements such as propagating context to the publisher. Another change is that any error condition occurring before the GraphQL query is executed successfully (e.g., a Publisher is returned) will result in a non-SSE response; that is, the status code of the response will be non-200, and the error message will no longer be written as a single server-sent event. Finally, the way errors are written has been fixed. Previously the entire stack trace was accidentally written to the event, now only the exception message is propagated.
kilink
commented
Apr 22, 2022
| } | ||
|
|
||
| val subscriber = object : Subscriber<ExecutionResult> { | ||
| lateinit var subscription: Subscription |
There was a problem hiding this comment.
I think this actually wasn't thread safe, since the property wasn't volatile / access wasn't synchronized, although in practice I never saw any actual issues.
| } | ||
|
|
||
| publisher.subscribe(subscriber) | ||
| val subscriptionId = UUID.randomUUID().toString() |
There was a problem hiding this comment.
How does the execution Id get set here?
There was a problem hiding this comment.
I don't think we can get the ExecutionId here.
srinivasankavitha
approved these changes
Apr 22, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DgsSSESubscriptionHandler was rolling its own Subscriber implementation in order
to adapt the Publisher returned by the GraphQL query to the SseEmitter; however,
Spring itself will handle this when the content type is "text/event-stream" and
we simply return a reactive type, such as Flux. Doing so greatly simplifies the
code in the controller, and allows for future enhancements such as propagating
context to the publisher.
Another change is that any error condition occurring before the GraphQL query is
executed successfully (e.g., a Publisher is returned) will result in a non-SSE response;
that is, the status code of the response will be non-200, and the error message
will no longer be written as a single server-sent event.
Finally, the way errors are written has been fixed. Previously the entire stack trace
was accidentally written to the event, now only the exception message is propagated.