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

… on the constructor. Fixes #34340
spring-projects · Jan 30, 2025 · 5b57c5f · 5b57c5f
1 parent d4030a8
commit 5b57c5f
Show file tree

Hide file tree

Showing 3 changed files with 101 additions and 8 deletions.
diff --git a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java
@@ -452,7 +452,9 @@ public HttpHeaders() {
 	 * headers map structures, primarily for internal use within the framework.
 	 * @param headers the headers map (expected to operate with case-insensitive keys)
 	 * @since 5.1
+	 * @deprecated Will be made private in favor of {@link #backedBy(MultiValueMap)} in a future release.
 	 */
+	@Deprecated
 	public HttpHeaders(MultiValueMap<String, String> headers) {
 		Assert.notNull(headers, "MultiValueMap must not be null");
 		if (headers == EMPTY) {
@@ -477,6 +479,7 @@ else if (headers instanceof HttpHeaders httpHeaders) {
 	 * likely to be out of sync and should be discarded.
 	 * @param httpHeaders the headers to expose
 	 * @since 7.0
+	 * @deprecated Will be made private in favor of {@link #backedBy(HttpHeaders)} in a future release.
 	 */
 	public HttpHeaders(HttpHeaders httpHeaders) {
 		Assert.notNull(httpHeaders, "HttpHeaders must not be null");
@@ -491,6 +494,47 @@ public HttpHeaders(HttpHeaders httpHeaders) {
 		}
 	}
 
+	/**
+	 * Construct a new {@code HttpHeaders} instance backed by an existing map.
+	 * <p>This constructor is available as an optimization for adapting to existing
+	 * headers map structures, primarily for internal use within the framework.
+	 * @param headers the headers map (expected to operate with case-insensitive keys)
+	 */
+	public static HttpHeaders backedBy(MultiValueMap<String, String> headers) {
+		return new HttpHeaders(headers);
+	}
+
+	/**
+	 * Construct a new {@code HttpHeaders} instance by removing any read-only
+	 * wrapper that may have been previously applied around the given
+	 * {@code HttpHeaders} 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 httpHeaders the headers to expose
+	 */
+	public static HttpHeaders backedBy(HttpHeaders httpHeaders) {
+		return backedBy(httpHeaders.headers);
+	}
+
+	/**
+	 * Constructs a new {@code HttpHeaders} instance with the given headers.
+	 * Changes made to this new instance will NOT be reflected in the original headers.
+	 * @param headers The headers to copy
+	 */
+	public static HttpHeaders copyOf(MultiValueMap<String, String> headers) {
+		HttpHeaders httpHeadersCopy = new HttpHeaders();
+		headers.forEach((key, values) -> httpHeadersCopy.put(key, new ArrayList<>(values)));
+		return httpHeadersCopy;
+	}
+
+	/**
+	 * Constructs a new {@code HttpHeaders} instance with the given headers.
+	 * Changes made to this new instance will NOT be reflected in the original headers.
+	 * @param httpHeaders The headers to copy
+	 */
+	public static HttpHeaders copyOf(HttpHeaders httpHeaders) {
+		return copyOf(httpHeaders.headers);
+	}
 
 	/**
 	 * Get the list of header values for the given header name, if any.

diff --git a/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java b/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java
@@ -36,6 +36,7 @@
  * <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.
  *
+ * @implNote This does NOT make the underlying MultiValueMap readonly.
  * @author Brian Clozel
  * @author Sam Brannen
  * @since 5.1.1

diff --git a/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java b/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java
@@ -58,23 +58,71 @@ class HttpHeadersTests {
 	final HttpHeaders headers = new HttpHeaders();
 
 	@Test
-	void constructorUnwrapsReadonly() {
+	void backedByFactoryUnwrapsReadonly() {
 		headers.setContentType(MediaType.APPLICATION_JSON);
 		HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers);
 		assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
-		HttpHeaders writable = new HttpHeaders(readOnly);
+		HttpHeaders writable = HttpHeaders.backedBy(readOnly);
 		writable.setContentType(MediaType.TEXT_PLAIN);
+
 		// content-type value is cached by ReadOnlyHttpHeaders
+		assertThat(readOnly.getContentType())
+				.describedAs("readOnly HttpHeaders should NOT have updated content-type value")
+				.isEqualTo(MediaType.APPLICATION_JSON);
+		assertThat(writable.getContentType())
+				.describedAs("writable HttpHeaders should have updated content-type value")
+				.isEqualTo(MediaType.TEXT_PLAIN);
+		assertThat(headers.getContentType())
+				.describedAs("initial HttpHeaders should have updated content-type value")
+				.isEqualTo(MediaType.TEXT_PLAIN);
+	}
+
+	@Test
+	void backedByFactoryUnwrapsMultipleHeaders() {
+		headers.setContentType(MediaType.APPLICATION_JSON);
+		HttpHeaders readonlyOriginalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(headers);
+		HttpHeaders firewallHeaders = HttpHeaders.backedBy(readonlyOriginalExchangeHeaders);
+		HttpHeaders writeable = HttpHeaders.backedBy(firewallHeaders);
+		writeable.setContentType(MediaType.TEXT_PLAIN);
+
+		// If readonly headers are unwrapped multiple times, the content-type value should be updated
+		assertThat(writeable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
+		assertThat(firewallHeaders.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
+		assertThat(readonlyOriginalExchangeHeaders.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
+		assertThat(headers.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
+	}
+
+	@Test
+	void copyOfFactoryUnwrapsReadonly() {
+		headers.setContentType(MediaType.APPLICATION_JSON);
+		HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers);
 		assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
-		assertThat(writable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
+		HttpHeaders writable = HttpHeaders.copyOf(readOnly);
+		writable.setContentType(MediaType.TEXT_PLAIN);
+
+		assertThat(readOnly.getContentType())
+				.describedAs("readOnly HttpHeaders should NOT have updated content-type value")
+				.isEqualTo(MediaType.APPLICATION_JSON);
+		assertThat(writable.getContentType())
+				.describedAs("writable HttpHeaders should have updated content-type value")
+				.isEqualTo(MediaType.TEXT_PLAIN);
+		assertThat(headers.getContentType())
+				.describedAs("initial HttpHeaders should have updated content-type value")
+				.isEqualTo(MediaType.APPLICATION_JSON);
 	}
 
 	@Test
-	void writableHttpHeadersUnwrapsMultiple() {
-		HttpHeaders originalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(new HttpHeaders());
-		HttpHeaders firewallHeaders = new HttpHeaders(originalExchangeHeaders);
-		HttpHeaders writeable = new HttpHeaders(firewallHeaders);
-		writeable.setContentType(MediaType.APPLICATION_JSON);
+	void copyOfFactoryUnwrapsMultipleHeaders() {
+		headers.setContentType(MediaType.APPLICATION_JSON);
+		HttpHeaders readonlyOriginalExchangeHeaders = HttpHeaders.readOnlyHttpHeaders(headers);
+		HttpHeaders firewallHeaders = HttpHeaders.copyOf(readonlyOriginalExchangeHeaders);
+		HttpHeaders writeable = HttpHeaders.copyOf(firewallHeaders);
+		writeable.setContentType(MediaType.TEXT_PLAIN);
+
+		assertThat(writeable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
+		assertThat(firewallHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
+		assertThat(readonlyOriginalExchangeHeaders.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
+		assertThat(headers.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
 	}
 
 	@Test