impl: HttpHeaders.backedBy and HttpHeaders.copyOf to reduce ambiguity…

[craftmaster2190]

… on the constructor.

Fixes #34340

[craftmaster2190]

I think these assertions point out the confusing behavior of getting a read-only copy and then a writable copy that changes the underlying map through the read-only instance.

[bclozel]

Picking comments from the related issue in #34340

The above code is incorrect in that the second instance of HttpHeaders is not a true deep copy.
Changes to the copy are also changed in the first instance.
If (and that is a big if) the developer reads the documentation and HttpHeadersTest unit test;
they might be led to write:

HttpHeaders httpHeadersCopy = new HttpHeaders(HttpHeaders.readOnlyHttpHeaders(headers));

However, this is not intuitive and also does not protect the original headers object from being mutated since ReadOnlyHttpHeaders does not make the underlying MultiValueMap read-only.
So then the developer writes this code since there is not a copy/clone method.

HttpHeaders httpHeadersCopy = new HttpHeaders();  
headers.forEach((key, values) -> httpHeadersCopy.put(key, new ArrayList<>(values)));

This behavior is well documented in the Javadoc. We can always refine it, feel free to suggest changes in this PR.

This comment, while explaining the desired behavior, acknowledges the root of the surprise in that there is an implied norm given by collection-related constructors. For developers who have trained on collections, the conflicting behavior of HttpHeaders is disorienting.

You're right. This came from the fact that HttpHeaders implemented and somewhat behaved like a MultiValueMap. This is not the case anymore as of #33913.

I'd ask that the new HttpHeaders(...) constructors be deprecated, made private, and eventually fully replaced with static factory methods HttpHeaders.backedBy(...) and HttpHeaders.copyOf(...) to remove any ambiguity, keep the desired behavior for adapting existing structures, and also provide the functionality of creating deep clones.

We have already made some changes in #33913 to align the behavior and expectations; we've seen that while those changes are useful they already create some friction for the 7.0 upgrade. I don't think we should spend more upgrade pain budget on something like this. new HttpHeaders in its current form is being used in many places and inflincting that breaking change will make the upgrade experience worse, especially in addition to #33913.

I think that HttpHeaders.copyOf could be a nice addition; could you revisit your PR to limit to that change?

Note: we recently switched to using a DCO for contributions. Could you squash your commits into one and sign them with git amend -s please? More details on the DCO here.

[craftmaster2190]

Maybe I need to be educated a little.
Is marking the constructor deprecated to removed after 7.0 (maybe 7.1 or 8.0 or whenever) creating lots of friction?
Other consumers would still be able to use it in the meantime and not need to change for the upgrade?

[bclozel]

Deprecating a method sends a signal to the community: you should adapt for a future removal.
Unfortunately this calls for action for developers compiling with warnings as errors to detect those as early as possible. The experience is much worse for developers ignoring those in the first place: it breaks their app when the removal happens.

We do use deprecations and removals, we just try to not ask too much from our community when they upgrade. Here we have fixed already the collection contract of HttpHeaders and we have experienced already some disruption in other Spring projects as a result.

Deprecating these constructors is mainly for consistency, so I am not sure this is wise to do this now. Maybe later?

Here is an example of feedback we get when we deprecate something: #33934 (comment)

[craftmaster2190]

Understood. You've convinced me.
I've removed the @Deprecated annotations and updated the related Javadocs.