Skip to content

Commit

Permalink
Deprecate HttpHeaders.writableHttpHeaders
Browse files Browse the repository at this point in the history
Prior to this commit, gh-21783 introduced `ReadOnlyHttpHeaders` to avoid
parsing media types multiple times during the lifetime of an HTTP
exchange: such values are cached and the headers map is made read-only.
This also added a new `HttpHeaders.writableHttpHeaders` method to unwrap
the read-only variant when needed.

It turns out this method sends the wrong signal to the community
because:
* the underlying map might be unmodifiable even if this is not an
  instance of ReadOnlyHttpHeaders
* developers were assuming that modifying the collection that backs the
  read-only instance would work around the cached values for
  Content-Type and Accept headers

This commit adds more documentation to highlight the desired behavior
for cached values by the read-only variant, and deprecates the
`writableHttpHeaders` method as `ReadOnlyHttpHeaders` is package private
and we should not surface that concept anyway.
Instead, this commit unwraps the read-only variant if needed when a new
HttpHeaders instance is created.

Closes gh-32116
  • Loading branch information
bclozel committed Mar 12, 2024
1 parent 670fc9b commit 4b732d6
Show file tree
Hide file tree
Showing 5 changed files with 197 additions and 169 deletions.
22 changes: 15 additions & 7 deletions spring-web/src/main/java/org/springframework/http/HttpHeaders.java
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,15 @@ public HttpHeaders() {
*/
public HttpHeaders(MultiValueMap<String, String> headers) {
Assert.notNull(headers, "MultiValueMap must not be null");
this.headers = headers;
if (headers == EMPTY) {
this.headers = CollectionUtils.toMultiValueMap(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH));
}
else if (headers instanceof ReadOnlyHttpHeaders readOnlyHttpHeaders) {
this.headers = readOnlyHttpHeaders.headers;
}
else {
this.headers = headers;
}
}


Expand Down Expand Up @@ -1869,7 +1877,7 @@ public static HttpHeaders readOnlyHttpHeaders(MultiValueMap<String, String> head
* Apply a read-only {@code HttpHeaders} wrapper around the given headers, if necessary.
* <p>Also caches the parsed representations of the "Accept" and "Content-Type" headers.
* @param headers the headers to expose
* @return a read-only variant of the headers, or the original headers as-is
* @return a read-only variant of the headers, or the original headers as-is if already read-only
*/
public static HttpHeaders readOnlyHttpHeaders(HttpHeaders headers) {
Assert.notNull(headers, "HttpHeaders must not be null");
Expand All @@ -1879,16 +1887,16 @@ public static HttpHeaders readOnlyHttpHeaders(HttpHeaders headers) {
/**
* Remove any read-only wrapper that may have been previously applied around
* the given headers via {@link #readOnlyHttpHeaders(HttpHeaders)}.
* <p>Once the writable instance is mutated, the read-only instance is likely
* to be out of sync and should be discarded.
* @param headers the headers to expose
* @return a writable variant of the headers, or the original headers as-is
* @since 5.1.1
* @deprecated as of 6.2 in favor of {@link #HttpHeaders(MultiValueMap)}.
*/
@Deprecated(since = "6.2", forRemoval = true)
public static HttpHeaders writableHttpHeaders(HttpHeaders headers) {
Assert.notNull(headers, "HttpHeaders must not be null");
if (headers == EMPTY) {
return new HttpHeaders();
}
return (headers instanceof ReadOnlyHttpHeaders ? new HttpHeaders(headers.headers) : headers);
return new HttpHeaders(headers);
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2023 the original author or authors.
* Copyright 2002-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -31,6 +31,8 @@

/**
* {@code HttpHeaders} object that can only be read, not written to.
* <p>This caches the parsed representations of the "Accept" and "Content-Type" headers
* and will get out of sync with the backing map it is mutated at runtime.
*
* @author Brian Clozel
* @author Sam Brannen
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ public DefaultServerHttpRequestBuilder(ServerHttpRequest original) {
Assert.notNull(original, "ServerHttpRequest is required");

this.uri = original.getURI();
this.headers = HttpHeaders.writableHttpHeaders(original.getHeaders());
this.headers = new HttpHeaders(original.getHeaders());
this.httpMethod = original.getMethod();
this.contextPath = original.getPath().contextPath().value();
this.remoteAddress = original.getRemoteAddress();
Expand Down
Loading

0 comments on commit 4b732d6

Please sign in to comment.