SSE support #2126
SSE support #2126
Conversation
interop/tapir/src/main/scala/caliban/interop/tapir/TapirAdapter.scala
Outdated
Show resolved
Hide resolved
interop/tapir/src/main/scala/caliban/interop/tapir/TapirAdapter.scala
Outdated
Show resolved
Hide resolved
interop/tapir/src/main/scala/caliban/interop/tapir/TapirAdapter.scala
Outdated
Show resolved
Hide resolved
interop/tapir/src/main/scala/caliban/interop/tapir/TapirAdapter.scala
Outdated
Show resolved
Hide resolved
interop/tapir/src/main/scala/caliban/interop/tapir/TapirAdapter.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Really happy with the changes! I tested it locally and it works as expected, well done 🎉. There a few things missing though:
- (Needed in this PR - sorry for this) We need to add at least 1 test to the
TapirAdaptertest suite. It doesn't need to be something too thorough, but we need to have some sort of an automated test that any future changes don't break this functionality - (Follow-up) We need to check if the operation type is a subscription in the
isSSEcheck. Currently, if the client sends anAccept: application/json, text/event-streamheader, we'll always respond with a streamed response even if the operation is a query / mutation. - (Follow-up) It'd be good to add this functionality also to the
caliban-quickadapter. Sincecaliban-quickdoesn't support subscriptions at all currently, it'd be a great addition there since it'll support at least one type of subscriptions now
Regarding (2) and (3) I'm happy for it to be in a different PR or for me to work on those after this PR is merged in if you don't currently have the capacity. I think (2) might a few changes in core so it might be a bit more involved. Just let me if you prefer to work on those @SvenW or whether you're happy for me to take it from there
Hi @kyri-petrou, I will definitely fix 1, battling the tapir client at the moment. But would love it if you could take 2 and 3 after that. My time has been a little limited this week but I hope to be able to focus more and fix number 1 during next week at least |
| ) { case Right(_) => true }, | ||
| oneOfVariantValueMatcher[CalibanBody.Stream[stream.BinaryStream]]( | ||
| streamBinaryBody(stream)(CodecFormat.TextEventStream()).toEndpointIO | ||
| .map(Right(_)) { case Right(value) => value } |
There was a problem hiding this comment.
I'm quite surprised this works since there is a Right(_) => true above this, does the selector take into account the Accepts headers when computing the match?
There was a problem hiding this comment.
I honestly don't know if it does take the Accept header into account but it does work. If we can do it in another way that is more readable or so I'm all for it
There was a problem hiding this comment.
If memory serves well, I also came across this some time ago and I was also surprised about it. I think I also concluded that it takes the Accept header into account
| @@ -114,6 +120,8 @@ object TapirAdapter { | |||
| case _ => false | |||
| } | |||
| ) | |||
| val isSSE = | |||
There was a problem hiding this comment.
Could make this a def or lazy as well since we don't need to compute it if we match another case first
There was a problem hiding this comment.
Adding to this, we could probably check the Accept header just a single time (we currently check it once for acceptsGqlJson and once for isSSE)
|
@kyri-petrou I added tests but now the QuickAdapter fails of course. Can we skip parts of the TapirAdapterSpec for that specific adapter or how should I handle it? |
@SvenW Awesome! Yeah disable it for and I'll enable in my PR which will add the functionality for it |
|
@SvenW sorry perhaps I wasn't very clear; What I meant above was to add an argument to |
|
❤️ |
This adds support for subscriptions over SSE. Only the Distinct connection mode part of the spec. I've verified it by running the zio-http example app and receiving updates there