search

joffrey-bion / krossbow

A Kotlin multiplatform coroutine-based STOMP client over websockets, with built-in conversions.
MIT License
189 stars 14 forks source link

Allow sending custom headers in more frames #507

Open aruke opened 3 months ago

aruke commented 3 months ago

Problem / use case

I ran into a use case where I would like to send custom headers in an ACK frame. This might sound unusual, given the specification only mentions the headers id and transaction. However, I'm porting a JS implementation, and the JS client allows arbitrary headers. This might be because of loose typing from JS/TS, but sending headers works.

Feature (solution)

This is not a standard requirement, so if the library allows sending raw STOMP frames from the session, it will be more flexible. One solution can be adding a rawFrame(frame: StompFrame) in the StompSession interface. There is already such implementation, but it's a private method.

Current alternatives

I haven't found any workarounds yet. If there is one, I would like to go with it instead of requesting library changes.

joffrey-bion commented 3 months ago

Hello! Thanks for sharing your use case. I intend to do some rework around headers in general at some point, and this is good input.

Indeed the spec technically only allows user-defined headers in MESSAGE, SEND, and ERROR frames, and that's why Krossbow doesn't currently allow sending custom headers in other frames. That said, it doesn't really hurt to be lenient in the client and allow extra headers in any frame, possibly protected by an opt-in annotation for frames that shouldn't support it.

Sending a raw StompFrame is currently internal on purpose, because the StompSession needs to do some special handling for subscriptions and receipts and sending single frames generically might cause problems. It's not necessary to make this public in order to solve your issue, but maybe it would be a more general way to solve a whole class of problems around deviating from the spec. An opt-in annotation would be sufficient to discourage misuses.

aruke commented 3 months ago

It doesn't really hurt to be lenient in the client and allow extra headers in any frame, possibly protected by an opt-in annotation for frames that shouldn't support it.

This would have been an ideal way to deal with custom headers, but not only it deviates the library from specifications, but also adds extra code to be tested.

The StompSession needs to do some special handling for subscriptions and receipts and sending single frames generically might cause problems.

I understand, but this adds flexibility to whoever wants to use the library for custom needs.

The current problem with making sendStompFrame public is that the HeartBeater will not be notified with the public version of sendStompFrame. At the same time, I realized that even making sendStompFrame won't solve custom header problem, because, let's say I want to send custom headers for RECEIPT frame, I cannot because:

StompFrame.Receipt(StompReceiptHeaders(StompHeaders())) // StompHeaders is interface
StompFrame.Receipt(StompReceiptHeaders(SimpleStompHeaders())) // SimpleStompHeaders is internal

With this the first option starts to look better.

joffrey-bion commented 3 months ago

Yes, I enforced the spec constraints in multiple places :) That's why I will need to change things around the headers to make this happen. But anyway headers need to change. I don't like the current mutability. It was done as a premature performance optimization, but it has a cost in correctness.

I'll keep this issue in mind when I do rework this part.

And also I'll see what's best for sendStompFrame.

aruke commented 3 months ago

Meanwhile, is there a workaround for sending custom headers without writing my own WebsocketClient?

joffrey-bion commented 3 months ago

I tried to look for one, but I don't think there is, unfortunately. Sorry

aruke commented 3 months ago

I tried making changes in this PR the header constructors. Can you please let me know if anything more is needed regarding code or unit tests?