search

softwaremill / tapir

Rapid development of self-documenting APIs
https://tapir.softwaremill.com
Apache License 2.0
1.34k stars 405 forks source link

WebSocket `concatenateFragmentedFrames` is a no-op #3339

Open kamilkloch opened 9 months ago

kamilkloch commented 9 months ago

Current implementation of optionallyConcatenateFrames returns the original stream:

https://github.com/softwaremill/tapir/blob/fdfa3da9a7d96b49625412a3aebed3ffe4e87988/server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sWebSockets.scala#L71

adamw commented 9 months ago

Do you see where the problem is? I guess it would be high time to add some tests for the pipeToBody specifically

kamilkloch commented 9 months ago

Accumutor is always None - lines https://github.com/softwaremill/tapir/blob/fdfa3da9a7d96b49625412a3aebed3ffe4e87988/server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sWebSockets.scala#L76 through https://github.com/softwaremill/tapir/blob/fdfa3da9a7d96b49625412a3aebed3ffe4e87988/server/http4s-server/src/main/scala/sttp/tapir/server/http4s/Http4sWebSockets.scala#L79 are never invoked.

kamilkloch commented 9 months ago

Question, if such functionality is actually needed. http4s concatenates web socket frames by default, not sure about the other servers.

adamw commented 9 months ago

It is, not all servers do :) Though of course, can't tell right now which don't ;) I just remember this being the case ...

kamilkloch commented 9 months ago

But, astonishingly, nobody is complaining about non-concatenating the frames by tapir interpreter for other servers.

adamw commented 9 months ago

Yes, maybe these are unpopular servers, or people don't send fragmented frames that often. I don't know really :)