Rework `StompHeaders`

[joffrey-bion]

What's wrong in the current design?

The current design of StompHeaders has a couple flaws:

1. StompHeaders extends MutableMap, making the headers mutable, which leads to errors like https://github.com/joffrey-bion/krossbow/issues/482 when the instance is reused. We need a way to copy headers and frames, and use it instead of mutating the headers.
2. the implementation is a bit too restrictive. The spec is not 100% clear about forbidding custom headers to other frames than SEND and MESSAGE. In the section about Frames and Headers, it is mentioned at the end that "STOMP servers MAY use additional headers to give access to features like persistency or expiration", without mentioning any specific frames. See https://github.com/joffrey-bion/krossbow/issues/507. Also, while constructors are restricted, the mutability of the StompHeaders interface defeats the purpose of omitting some options in the constructor.
3. the data class approach is questionable because the generated methods don't make much sense since the primary constructor only has a raw headers instance as property. The copy method is practically useless because it takes these raw headers as parameter instead of each header as a property, so users can't actually use it to reuse a part of the headers and replace some. Same for component1.
4. the constructor-based approach is not easy to evolve, we can't protect single headers with opt-in or mark them as deprecated. Using a builder approach would be generally better for a safer evolution of these classes (although we don't expect any change in the protocol, probably for the rest of the library's life).

Usage patterns

• instantiation with a raw map of headers
  • (internal) in StompFrame decoding
• instantiation with typed properties/args (or builder in the future)
  • (internal) in the StompSession implementation to create frames to send
  • (internal) in Krossbow tests
  • in user tests (to mock server frames, or maybe even client frames sent by higher layers)
  • MAYBE in user production code if we ever provide a generic sendRaw(StompFrame)
• write (or copy with modifications)
  • (internal) to add content length/type, and receipt
  • in user tests (to mock frames based on a common one)
  • MAYBE in user production code if we ever provide a generic sendRaw(StompFrame)
• read access of raw headers
  • (internal) in StompFrame encoding
  • in user code to read custom headers from server frames (could be replaced with a customHeaders property that would just contain the custom headers)
• read access of typed properties
  • (internal) in some parts of the StompSession implementation
  • in user code to read server frames
  • MAYBE in user production code if we ever provide a generic sendRaw(StompFrame)
• instance equality checks
  • (internal) in Krossbow tests
  • in user tests

[freynder]

I think you also mentioned performance as one of the reasons for implementing it using a MutableMap structure.

The specification regarding headers is rather general, using wording as MAY and SHOULD, depending on frame types, presence of body, preference, stomp version, transaction context, receipt requirement.

If we want to provide maximum flexibility, we could potentially model this by declaring directives on how to process the communication activities. e.g.:

    class StompFrameDirectives(
        val includeReceiptHeader = false,
        val includeContentLengthHeader = true,
        val includeContentTypeHeader = true,
        val includeCustomHeaders = true
        val useVersion : StomVersion = negotiatedStompVersion
    )

StompFrameDirectives should include all the possible allowed variations or interpretations of the specification with sensible defaults.

The header types can just be regular data classes, they should not be concerned with how they will be encoded. They just need to represent a consistent header definition.

    data class StompSendHeaders(
            val destination: String,
            override val transaction: String? = null,
            val customHeaders: Map<String, String> = emptyMap(),
        ) : Transactional(transaction) 

Finally, the responsibility of including these headers into frames can be moved to the communication functions in StompSession. We can provide several overloaded functions here to accommodate the level of abstraction that the caller wants to use. The higher level functions can use the directives to make decisions on when and how to include the headers e.g:

    directivesFromConfig = StompFrameDirectives(config.autoReceipt, config.includeContentLength, config.includeContentType)

    // Lowest level for high performance. Caller is responsible for generating correct headers. Special headers are not managed
    suspend fun send(headers: RawStompHeaders, body: FrameBody?)

    // Note that FrameBody has a contentType property, the functions below may use that value to construct content-type headers if required by the spec or requested by directives

    // Using RawStompHeaders for headers without special meaning. Special headers are managed according to directives
    suspend fun send(headers: RawStompHeaders, body: FrameBody?, directives: StompFrameDirectives = directivesFromConfig)

    // Using StompSendHeaders. Special headers are managed according to specific directives or config defaults
    suspend fun send(headers: StompSendHeaders, body: FrameBody?, directives: StompFrameDirectives = directivesFromConfig)

Including all factors that can influence the logic of these operations into StompFrameDirectives parameter makes these functions rather pure and testable (disregarding side effects further down the line of course).

[joffrey-bion]

Thanks a lot for the valuable input and interesting considerations!

I think you also mentioned performance as one of the reasons for implementing it using a MutableMap structure.

Yes, the idea was to reduce allocations. Copying header objects for every send would generate extra allocations, so for intense messaging, it might be important to avoid this. The problem is that we can't control the header instances provided by the users, so we open the door for #482 if we change user input directly. This could be mitigated by removing the overloads taking actual headers objects, and only create instances internally. If individual headers are parameters of the function overload, we are responsible for creating the headers instance, and could directly add all the auto-headers in a single instance creation.

But I'm now of the idea of thinking more in terms of correctness first, and provide performance escape hatches later if there is a measured issue. On server-side it can be a different story, though.

If we want to provide maximum flexibility, we could potentially model this by declaring directives on how to process the communication activities. [...] StompFrameDirectives should include all the possible allowed variations or interpretations of the specification with sensible defaults.

That's an interesting perspective. Giving it a bit of thought on the concrete examples, I'm not sure how useful it would be, though. Which particular problem are you trying to solve with this? Taking your example piece by piece, I don't clearly see a practical use for the following:

• includeContentTypeHeader: we cannot infer such header value internally, because what the body means is decided by the caller. If we provide the user with the ability to add this information to the FrameBody (rather than the headers directly), then why not simply use the presence/absence of this information to add the header? Why do we need a second parameter for this?
• includeCustomHeaders: the local decision on whether to include custom headers can already be controlled by... including custom headers? Not sure what we would achieve by this local parameter. What would sending custom headers + includeCustomHeaders=false mean? The only thing it seems to enable is to forbid custom headers generally, and allow them locally for some calls. But in that case, we aren't protecting anything at compile time (the API to add custom headers has to be there), and adding custom headers just becomes more hassle than necessary because we now need to add a local includeCustomHeaders for it. I believe a better approach for this would be to use headers builders (instead of data class constructors) so we can add an opt-in requirement on custom headers for frames that are not supposed to have them.
• useVersion: this is puzzling to me. The version of the protocol that the client speaks with the server is negotiated during the connection. We cannot change it on a frame-by-frame basis.

Now for the other 2 (receipt and content-length), there is already the option to disable globally and add the header manually on a frame-by-frame basis. However, there is indeed no way to globally enable it, and then opt-out on a frame-by-frame basis. That said, it would be worth verifying that this use case is not just theoretical. For instance, it's quite unlikely that people actually don't want to send a content-length header.

Including all factors that can influence the logic of these operations into StompFrameDirectives parameter makes these functions rather pure and testable (disregarding side effects further down the line of course).

To be frank, none of these functions will ever be pure, as the StompSession is a stateful object by essence, and tracks all sorts of things. Testing them using a fake web socket has been fairly easy so far, so I wouldn't worry too much about this aspect.

My concern with these local directives is the extra complexity. It seems we could still offer ultimate local flexibility by just providing a way to send a raw frame constructed manually (an escape hatch to avoid any auto-headers), without the need for a lot of bells and whistles. Multiplying overloads has an adverse effect on maintainability, and especially binary compatibilty, so it must be done with careful considerations (that said, the potential future dataarg feature of Kotlin, KT-8214, might help here).

Compatibility concerns is also why I'm considering refraining from using data classes. For the best compatibility, I was thinking of probably using exclusively interfaces (for easy consumption to read the frames), and builder functions (for easy construction), without exposing any constructor.