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

Add support for SSE-based subscriptions for caliban-quick #2141

Merged
merged 5 commits into from
Feb 29, 2024

Conversation

kyri-petrou
Copy link
Collaborator

Followup of #2126.

@SvenW can you take a look and let me know if everything looks okay to you?

Comment on lines 207 to 211
case ObjectValue((fieldName, StreamValue(stream)) :: Nil) =>
stream.either.map {
case Right(r) => GraphQLResponse(ObjectValue(List(fieldName -> r)), resp.errors)
case Left(err) => GraphQLResponse(ObjectValue(List(fieldName -> NullValue)), List(err))
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@paulpdaniels Is this error handling sound? Also, is there a better way to merge the error and success channels? I couldn't for the life of me figure it out 😕

@SvenW
Copy link
Contributor

SvenW commented Feb 26, 2024

Followup of #2126.

@SvenW can you take a look and let me know if everything looks okay to you?

Should follow the protocol from what I can tell. Nice rewrites of the accept header and the tapirspec as well. A lot more readable

val stream = (resp.data match {
case ObjectValue((fieldName, StreamValue(stream)) :: Nil) =>
stream.either.map {
case Right(r) => GraphQLResponse(ObjectValue(List(fieldName -> r)), resp.errors)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this saying: for all response bodies return the original errors? Would we want errors to be null here perhaps?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm I think you're right. It's probably better to send an initial one with all the errors (if they exist) and then the subsequent ones not containing errors

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I made the the GraphQLResponse stream handling generic and placed it in HttpUtils in the core module so that both the caliban-quick and tapir adapters use the same one. Also the errors now are reported in the first event only

@kyri-petrou kyri-petrou merged commit 3d1ad9a into series/2.x Feb 29, 2024
10 checks passed
@kyri-petrou kyri-petrou deleted the quick-adapter-sse branch February 29, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants