spring-projects / spring-framework

Spring Framework
https://spring.io/projects/spring-framework
Apache License 2.0
56.77k stars 38.16k forks source link

Improve iteration methods in native headers to MultiValueMap adapters #33823

Closed simonbasle closed 3 weeks ago

simonbasle commented 1 month ago

In 6.1, HttpHeaders can be constructed from a native container/server's HTTP headers implementation by wrapping these various implementations in dedicated MultiValueMap<String, String> adapters. This is critical for performance in terms of throughput and memory pressure, as this allows for efficient storage of headers by avoiding copies when not necessary.

However, the iteration methods of MultiValueMap don't align entirely well with native implementations. For instance, some of these implementation only ignore the casing of a given header name when accessing it, not storing it. As such, when queried for the list of header names they include case variants.

This can lead to subtle bugs where an attempt to iterate over the entries of such an HttpHeaders produces duplicate keys AND duplicate values : as the pairing of names and values is not native, it is reconstructed by iterating over the names provided by the underlying structure and calling get(name) on each.

For example, considering the native headers map below contains TestHeader=first and TestHEADER=second:

HttpHeaders original = new HttpHeaders(someNativeHeaderMap);

HttpHeaders headers = new HttpHeaders();
for (Entry<String, List<String>> h : original.entrySet()) {
    headers.addAll(h.getKey(), h.getValue());
}

will result in an instance which contains TestHeader=[first,second,firstSecond], TestHEADER=[first,second,first,second].

The goal of this issue is to maintain current performance status quo as much as possible while introducing an alternative to entrySet that guarantees case-insensitive iteration. The javadoc should expose this limitation and offer guidance towards using the new method.

simonbasle commented 3 weeks ago

The new method is headerSet(), instance method in HttpHeaders.

simonbasle commented 3 weeks ago

Note that in the snippet above, headers.put(...) instead of headers.addAll(...) would be a relevant workaround.