Added constructor for HttpHeaders that accepts a Map. This is useful in tests where we don't need to do a two step creation of HttpHeaders and then add the headers - it can be single line now.

Refactored few tests in existing code that could take advantage of the new constructor.

Comment From: simonbasle

Let's wait for an opinion from another team member, but my personal feeling is that if this is for test convenience (one liners) then this new Map<String, String>-focused constructor is maybe a bit over-complicated.

It creates a lot of intermediate structures (stream, mapped lists, collector map, etc...) for little benefit, and even for one-liners the user is required to call Map.of(k, v) on top of that.

Looking at the amended tests, it looks like a vast majority use a single header entry, with a few tests that have several entries (where going through an intermediate Map.of(...) makes a bit more sense, but not that much compared to the old code to be honest).

How about truly focusing on the single header case and provide a factory method like HttpHeaders#of(String singleKey, String singleValue), mirroring in a way the Map.of factory method?

Comment From: krzyk

Thank you for detailed response. Yes the factory method with just single header makes more sense, as that's the exact case I keep seeing in my own code.

I'll refactor the PR with that so we can discuss further.

Comment From: krzyk

I've updated the PR with suggested changes.

Comment From: simonbasle

Thanks again for working on this and sorry for the back-and-forth. The team has discussed this issue further and feels that modifying HttpHeaders almost exclusively for test convenience is not the direction we want to go.

  • in production users don't typically create the HttpHeaders even if the end goal is to only add a single entry
  • It is also generally better to use the fluent setters than it is to use free-form String, String
  • Map.of isn't a great showcase for the feature, as it produces immutable maps while from a production perspective HttpHeaders are supposed to be mutable, which could create some cognitive dissonance

We were starting to discuss if doing something at the level of MultiValueMap itself could be beneficial but that's a different direction that isn't likely to improve the HttpHeaders case, as its MVM-based constructor expects a very specific kind of MultiValueMap: must be mutable and must be case-insensitive for keys. For users to respect that contract would mean additional boilerplate, eliminating the benefits of a MultiValueMap<K, V> from Map<K, V> constructor.

Comment From: krzyk

Sure, that's understandable.