Deprecate HttpHeaders.writableHttpHeaders

[m42cel]

Affects: 5.1 / 6.1

---

What

This problem arises from the fact the Spring allows to remove the ReadOnlyHttpHeaders wrapper without creating a modifiable copy but instead modifies the underlying MultiValueMap and thereby allows modifying an ReadOnlyHttpHeaders object.

When using ReadOnlyHttpHeaders::getContentType it will cache the returned Content-Type, which was introduced in Improve WebFlux performance for header management [SPR-17250].

However, if the Content-Type of the original HttpHeaders object is changed, the cache is not invalidated and a successive call to readOnlyHttpHeaders.getContentType() will return the outdated Content-Type, while readOnlyHttpHeaders.getFirst(HttpHeaders.CONTENT_TYPE) will return the new value.

Background

With the update from Spring Boot 3.1 to 3.2 many HttpHeaders created by Spring Web seem to be changed to ReadOnlyHttpHeaders. This broke many parts of our project, since we often intercept requests to do some modifications (like removing specific headers before forwarding the request to an internal host). Since it's not possible to set a new HttpHeaders object to a HttpRequest or HttpServletRequest we now use HttpHeaders::writableHttpHeaders method to do modifications of the original headers.

This however does not work for the Content-Type once it is cached in the HttpServletRequest's ReadOnlyHttpHeaders.

One solution would be to completely wrap the original HttpRequest before passing it on in the interceptor chain (which I think is the recommended approach) but that doesn't change the fact that the HttpHeaders::writableHttpHeaders and cachedContentType cause some inconsistency.

Example

@Test
void testReadOnlyHeaders() {
  HttpHeaders originalHeaders = new HttpHeaders();
  originalHeaders.setContentType(MediaType.APPLICATION_XML);

  // only a ready-only wrapper around the original HttpHeaders
  HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(originalHeaders);

  // caches the content type value
  assertThat(readOnlyHttpHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_XML);

  // creates a writable variant of the ReadOnlyHttpHeaders
  HttpHeaders writableHttpHeaders = HttpHeaders.writableHttpHeaders(readOnlyHttpHeaders);

  // changes the value in the underlying MultiValueMap but doesn't invalidate the cached value
  writableHttpHeaders.setContentType(MediaType.APPLICATION_JSON);

  // passes since it doesn't use the cached value
  assertThat(writableHttpHeaders.getContentType()).hasToString(writableHttpHeaders.getFirst(HttpHeaders.CONTENT_TYPE));
  // fails since the cached value is still application/xml
  assertThat(readOnlyHttpHeaders.getContentType()).hasToString(readOnlyHttpHeaders.getFirst(HttpHeaders.CONTENT_TYPE));
}

[snicoll]

The Javadoc of readOnlyHttpHeaders states:

Apply a read-only HttpHeaders wrapper around the given headers, if necessary. Also caches the parsed representations of the "Accept" and "Content-Type" headers.

This looks like the expected behavior to me. I am having trouble understanding the report. If the headers are read-only, that's specifically to avoid changing them. If you insist in doing that, it's asking for trouble really.

[m42cel]

I agree that trying to change read-only objects is asking for trouble, which is a general design-flaw with how HttpHeaders.writableHttpHeaders works, because it doesn't return a modifiable copy but instead allows modifications to the original read-only object.
However, as-is, returning a writable copy would also be problematic. Intercepting HttpRequests and doing some modifications to it before continuing in the processing chain is a valid use-case. But since the HttpRequest only provides a getHeaders() and no setHeaders(), it calls for modifying the object that was returned via getHeaders().

It looks like the ReadOnlyHttpHeaders was introduced to improve performance by caching some often requested properties. I currently could think of two options to solve the described issue:

1. Instead of putting the two cached headers into the underlying MultiValueMap store them in dedicated fields in the HttpHeaders. As it is now, reading via the getContentType() could directly go to the field. When reading via get(CONTENT_TYPE) there would have to be a check if the key is Content-Type or Accept and then use the fields instead of the map (same for set). This would have to be done in the original HttpHeaders class.
2. Make HttpHeaders.writableHttpHeaders return a writable-copy. Then there also has to be a way to set HttpHeaders to HttpRequests or to easily create a modifiable copy of the request. Like for example it is available with Spring Reactive's ServerHttpRequest::mutate.

[bclozel]

Thanks for raising this.
Indeed, ReadOnlyHttpHeaders operate under the assumption that the underlying collection of headers is immutable in order to cache expensive values. It's not meant to work while another process is mutating those values - the behavior you're demonstrating is expected. Adapting to this use case would partially undo this performance improvement as we'd need to perform map lookups and value comparison every time the content type is asked for, which can be multiple times during request processing.

If you'd like to mutate the HttpServletRequest, I think wrapping it is the usual solution for that use case. In Spring web frameworks (and our own request types), we often provide mutators for this very purpose.

We'll use this issue to deprecate HttpHeaders.writableHttpHeaders and prepare for its removal, as it was mainly for internal use in the first place. I guess we can live with some duplication in our codebase.

[m42cel]

I hope the people of the future won't lynch me after the removal breaks their code base and they find this issue 😰 (if someone from the future is reading this, feel free to leave a "👋").

Jokes aside, I feel like there could be a little more support for handling, mutating and testing ReadOnlyHttpHeaders. The update to Spring Boot 3.2 did cost us quite some effort with HttpHeaders being made read-only. A few points I missed when adapting to the ReadOnlyHttpHeaders:

• There's org.springframework.http.client.support.HttpRequestWrapper but no HttpResponseWrapper
• There's no copy-constructor of copy method in HttpHeaders which makes creating a writable copy in a wrapper class a bit awkward
• ReadOnlyHttpHeaders are often only created during runtime, while spring-testing provides a MockHttpInputMessage containing a writable HttpHeaders instance. This makes it harder to unit-test if your code can handle ReadOnlyHttpHeaders, that are usually created by spring-web during runtime
  (I created a ReadOnlyHeaderMockHttpInputMessage for that purpose, which contains ReadOnlyHttpHeaders but allows setting headers for test initialization via separated methods)

Maybe I'm overlooking some mutators but to me it looked like most mutator functionality was only provided for Spring Reactive.

[bclozel]

@m42cel that's interesting feedback. We'll have a look and see if we need to create new issues to help with that.