root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions Packages Projects Releases Wiki Activity

Deprecate HttpHeaders.writableHttpHeaders 

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
This commit is contained in:
Brian Clozel 2024-03-12 12:06:30 +01:00
parent 670fc9bb4e
commit 4b732d62c2
5 changed files with 183 additions and 155 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
spring-web/src/main/java/org/springframework/http/HttpHeaders.java
View File

@ -441,7 +441,15 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
*/ */
public HttpHeaders(MultiValueMap<String, String> headers) { public HttpHeaders(MultiValueMap<String, String> headers) {
Assert.notNull(headers, "MultiValueMap must not be null"); 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;
}
} }
@ -1869,7 +1877,7 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
* Apply a read-only {@code HttpHeaders} wrapper around the given headers, if necessary. * 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. * <p>Also caches the parsed representations of the "Accept" and "Content-Type" headers.
* @param headers the headers to expose * @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) { public static HttpHeaders readOnlyHttpHeaders(HttpHeaders headers) {
Assert.notNull(headers, "HttpHeaders must not be null"); Assert.notNull(headers, "HttpHeaders must not be null");
@ -1879,16 +1887,16 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
/** /**
* Remove any read-only wrapper that may have been previously applied around * Remove any read-only wrapper that may have been previously applied around
* the given headers via {@link #readOnlyHttpHeaders(HttpHeaders)}. * 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 * @param headers the headers to expose
* @return a writable variant of the headers, or the original headers as-is * @return a writable variant of the headers, or the original headers as-is
* @since 5.1.1 * @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) { public static HttpHeaders writableHttpHeaders(HttpHeaders headers) {
Assert.notNull(headers, "HttpHeaders must not be null"); return new HttpHeaders(headers);
if (headers == EMPTY) {
return new HttpHeaders();
}
return (headers instanceof ReadOnlyHttpHeaders ? new HttpHeaders(headers.headers) : headers);
} }
/** /**

4
spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java
View File

@ -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"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
@ -31,6 +31,8 @@ import org.springframework.util.MultiValueMap;
/** /**
* {@code HttpHeaders} object that can only be read, not written to. * {@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 Brian Clozel
* @author Sam Brannen * @author Sam Brannen

2
spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java
View File

@ -70,7 +70,7 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder {
Assert.notNull(original, "ServerHttpRequest is required"); Assert.notNull(original, "ServerHttpRequest is required");
this.uri = original.getURI(); this.uri = original.getURI();
this.headers = HttpHeaders.writableHttpHeaders(original.getHeaders()); this.headers = new HttpHeaders(original.getHeaders());
this.httpMethod = original.getMethod(); this.httpMethod = original.getMethod();
this.contextPath = original.getPath().contextPath().value(); this.contextPath = original.getPath().contextPath().value();
this.remoteAddress = original.getRemoteAddress(); this.remoteAddress = original.getRemoteAddress();

308
spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java
View File

@ -35,6 +35,7 @@ import java.util.Map.Entry;
import java.util.Set; import java.util.Set;
import java.util.TimeZone; import java.util.TimeZone;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Test;
import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toList;
@ -54,8 +55,19 @@ import static org.assertj.core.api.Assertions.entry;
*/ */
class HttpHeadersTests { class HttpHeadersTests {
private final HttpHeaders headers = new HttpHeaders(); final HttpHeaders headers = new HttpHeaders();
@Test
void constructorUnwrapsReadonly() {
headers.setContentType(MediaType.APPLICATION_JSON);
HttpHeaders readOnly = HttpHeaders.readOnlyHttpHeaders(headers);
assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
HttpHeaders writable = new HttpHeaders(readOnly);
writable.setContentType(MediaType.TEXT_PLAIN);
// content-type value is cached by ReadOnlyHttpHeaders
assertThat(readOnly.getContentType()).isEqualTo(MediaType.APPLICATION_JSON);
assertThat(writable.getContentType()).isEqualTo(MediaType.TEXT_PLAIN);
}
@Test @Test
void getOrEmpty() { void getOrEmpty() {
@ -578,182 +590,188 @@ class HttpHeadersTests {
assertThat(authorization).isEqualTo("Bearer foo"); assertThat(authorization).isEqualTo("Bearer foo");
} }
@Test
void keySetOperations() {
headers.add("Alpha", "apple");
headers.add("Bravo", "banana");
Set<String> keySet = headers.keySet();
// Please DO NOT simplify the following with AssertJ's fluent API. @Nested
// class MapEntriesTests {
// We explicitly invoke methods directly on HttpHeaders#keySet()
// here to check the behavior of the entire contract.
// isEmpty() and size() @Test
assertThat(keySet).isNotEmpty(); void keySetOperations() {
assertThat(keySet).hasSize(2); headers.add("Alpha", "apple");
headers.add("Bravo", "banana");
Set<String> keySet = headers.keySet();
// contains() // Please DO NOT simplify the following with AssertJ's fluent API.
assertThat(keySet.contains("Alpha")).as("Alpha should be present").isTrue(); //
assertThat(keySet.contains("alpha")).as("alpha should be present").isTrue(); // We explicitly invoke methods directly on HttpHeaders#keySet()
assertThat(keySet.contains("Bravo")).as("Bravo should be present").isTrue(); // here to check the behavior of the entire contract.
assertThat(keySet.contains("BRAVO")).as("BRAVO should be present").isTrue();
assertThat(keySet.contains("Charlie")).as("Charlie should not be present").isFalse();
// toArray() // isEmpty() and size()
assertThat(keySet.toArray()).isEqualTo(new String[] {"Alpha", "Bravo"}); assertThat(keySet).isNotEmpty();
assertThat(keySet).hasSize(2);
// spliterator() via stream() // contains()
assertThat(keySet.stream().collect(toList())).isEqualTo(Arrays.asList("Alpha", "Bravo")); assertThat(keySet.contains("Alpha")).as("Alpha should be present").isTrue();
assertThat(keySet.contains("alpha")).as("alpha should be present").isTrue();
assertThat(keySet.contains("Bravo")).as("Bravo should be present").isTrue();
assertThat(keySet.contains("BRAVO")).as("BRAVO should be present").isTrue();
assertThat(keySet.contains("Charlie")).as("Charlie should not be present").isFalse();
// iterator() // toArray()
List<String> results = new ArrayList<>(); assertThat(keySet.toArray()).isEqualTo(new String[] {"Alpha", "Bravo"});
keySet.iterator().forEachRemaining(results::add);
assertThat(results).isEqualTo(Arrays.asList("Alpha", "Bravo"));
// remove() // spliterator() via stream()
assertThat(keySet.remove("Alpha")).isTrue(); assertThat(keySet.stream().collect(toList())).isEqualTo(Arrays.asList("Alpha", "Bravo"));
assertThat(keySet).hasSize(1);
assertThat(headers).hasSize(1);
assertThat(keySet.remove("Alpha")).isFalse();
assertThat(keySet).hasSize(1);
assertThat(headers).hasSize(1);
// clear() // iterator()
keySet.clear(); List<String> results = new ArrayList<>();
assertThat(keySet).isEmpty(); keySet.iterator().forEachRemaining(results::add);
assertThat(keySet).isEmpty(); assertThat(results).isEqualTo(Arrays.asList("Alpha", "Bravo"));
assertThat(headers).isEmpty();
assertThat(headers).isEmpty();
// Unsupported operations // remove()
assertThatExceptionOfType(UnsupportedOperationException.class) assertThat(keySet.remove("Alpha")).isTrue();
.isThrownBy(() -> keySet.add("x")); assertThat(keySet).hasSize(1);
assertThatExceptionOfType(UnsupportedOperationException.class) assertThat(headers).hasSize(1);
.isThrownBy(() -> keySet.addAll(Collections.singleton("enigma"))); assertThat(keySet.remove("Alpha")).isFalse();
} assertThat(keySet).hasSize(1);
assertThat(headers).hasSize(1);
/** // clear()
* This method intentionally checks a wider/different range of functionality keySet.clear();
* than {@link #removalFromKeySetRemovesEntryFromUnderlyingMap()}. assertThat(keySet).isEmpty();
*/ assertThat(keySet).isEmpty();
@Test // https://github.com/spring-projects/spring-framework/issues/23633 assertThat(headers).isEmpty();
void keySetRemovalChecks() { assertThat(headers).isEmpty();
// --- Given ---
headers.add("Alpha", "apple");
headers.add("Bravo", "banana");
assertThat(headers).containsOnlyKeys("Alpha", "Bravo");
// --- When --- // Unsupported operations
boolean removed = headers.keySet().remove("Alpha"); assertThatExceptionOfType(UnsupportedOperationException.class)
.isThrownBy(() -> keySet.add("x"));
assertThatExceptionOfType(UnsupportedOperationException.class)
.isThrownBy(() -> keySet.addAll(Collections.singleton("enigma")));
}
// --- Then --- /**
* This method intentionally checks a wider/different range of functionality
* than {@link #removalFromKeySetRemovesEntryFromUnderlyingMap()}.
*/
@Test // https://github.com/spring-projects/spring-framework/issues/23633
void keySetRemovalChecks() {
// --- Given ---
headers.add("Alpha", "apple");
headers.add("Bravo", "banana");
assertThat(headers).containsOnlyKeys("Alpha", "Bravo");
// Please DO NOT simplify the following with AssertJ's fluent API. // --- When ---
// boolean removed = headers.keySet().remove("Alpha");
// We explicitly invoke methods directly on HttpHeaders here to check
// the behavior of the entire contract.
assertThat(removed).isTrue(); // --- Then ---
assertThat(headers.keySet().remove("Alpha")).isFalse();
assertThat(headers).hasSize(1);
assertThat(headers.containsKey("Alpha")).as("Alpha should have been removed").isFalse();
assertThat(headers.containsKey("Bravo")).as("Bravo should be present").isTrue();
assertThat(headers.keySet()).containsOnly("Bravo");
assertThat(headers.entrySet()).containsOnly(entry("Bravo", List.of("banana")));
}
@Test // Please DO NOT simplify the following with AssertJ's fluent API.
void removalFromKeySetRemovesEntryFromUnderlyingMap() { //
String headerName = "MyHeader"; // We explicitly invoke methods directly on HttpHeaders here to check
String headerValue = "value"; // the behavior of the entire contract.
assertThat(headers).isEmpty(); assertThat(removed).isTrue();
headers.add(headerName, headerValue); assertThat(headers.keySet().remove("Alpha")).isFalse();
assertThat(headers.containsKey(headerName)).isTrue(); assertThat(headers).hasSize(1);
headers.keySet().removeIf(key -> key.equals(headerName)); assertThat(headers.containsKey("Alpha")).as("Alpha should have been removed").isFalse();
assertThat(headers).isEmpty(); assertThat(headers.containsKey("Bravo")).as("Bravo should be present").isTrue();
headers.add(headerName, headerValue); assertThat(headers.keySet()).containsOnly("Bravo");
assertThat(headers.get(headerName)).containsExactly(headerValue); assertThat(headers.entrySet()).containsOnly(entry("Bravo", List.of("banana")));
} }
@Test @Test
void removalFromEntrySetRemovesEntryFromUnderlyingMap() { void removalFromKeySetRemovesEntryFromUnderlyingMap() {
String headerName = "MyHeader"; String headerName = "MyHeader";
String headerValue = "value"; String headerValue = "value";
assertThat(headers).isEmpty(); assertThat(headers).isEmpty();
headers.add(headerName, headerValue); headers.add(headerName, headerValue);
assertThat(headers.containsKey(headerName)).isTrue(); assertThat(headers.containsKey(headerName)).isTrue();
headers.entrySet().removeIf(entry -> entry.getKey().equals(headerName)); headers.keySet().removeIf(key -> key.equals(headerName));
assertThat(headers).isEmpty(); assertThat(headers).isEmpty();
headers.add(headerName, headerValue); headers.add(headerName, headerValue);
assertThat(headers.get(headerName)).containsExactly(headerValue); assertThat(headers.get(headerName)).containsExactly(headerValue);
} }
@Test @Test
void readOnlyHttpHeadersRetainEntrySetOrder() { void removalFromEntrySetRemovesEntryFromUnderlyingMap() {
headers.add("aardvark", "enigma"); String headerName = "MyHeader";
headers.add("beaver", "enigma"); String headerValue = "value";
headers.add("cat", "enigma");
headers.add("dog", "enigma");
headers.add("elephant", "enigma");
String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; assertThat(headers).isEmpty();
headers.add(headerName, headerValue);
assertThat(headers.containsKey(headerName)).isTrue();
headers.entrySet().removeIf(entry -> entry.getKey().equals(headerName));
assertThat(headers).isEmpty();
headers.add(headerName, headerValue);
assertThat(headers.get(headerName)).containsExactly(headerValue);
}
assertThat(headers.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); @Test
void readOnlyHttpHeadersRetainEntrySetOrder() {
headers.add("aardvark", "enigma");
headers.add("beaver", "enigma");
headers.add("cat", "enigma");
headers.add("dog", "enigma");
headers.add("elephant", "enigma");
HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" };
assertThat(readOnlyHttpHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
}
@Test assertThat(headers.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
void readOnlyHttpHeadersCopyOrderTest() {
headers.add("aardvark", "enigma");
headers.add("beaver", "enigma");
headers.add("cat", "enigma");
headers.add("dog", "enigma");
headers.add("elephant", "enigma");
String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers);
assertThat(readOnlyHttpHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
}
HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); @Test
void readOnlyHttpHeadersCopyOrderTest() {
headers.add("aardvark", "enigma");
headers.add("beaver", "enigma");
headers.add("cat", "enigma");
headers.add("dog", "enigma");
headers.add("elephant", "enigma");
HttpHeaders forEachHeaders = new HttpHeaders(); String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" };
readOnlyHttpHeaders.forEach(forEachHeaders::putIfAbsent);
assertThat(forEachHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
HttpHeaders putAllHeaders = new HttpHeaders(); HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers);
putAllHeaders.putAll(readOnlyHttpHeaders);
assertThat(putAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
HttpHeaders addAllHeaders = new HttpHeaders(); HttpHeaders forEachHeaders = new HttpHeaders();
addAllHeaders.addAll(readOnlyHttpHeaders); readOnlyHttpHeaders.forEach(forEachHeaders::putIfAbsent);
assertThat(addAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); assertThat(forEachHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
}
@Test // gh-25034 HttpHeaders putAllHeaders = new HttpHeaders();
void equalsUnwrapsHttpHeaders() { putAllHeaders.putAll(readOnlyHttpHeaders);
HttpHeaders headers1 = new HttpHeaders(); assertThat(putAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1));
assertThat(headers1).isEqualTo(headers2); HttpHeaders addAllHeaders = new HttpHeaders();
assertThat(headers2).isEqualTo(headers1); addAllHeaders.addAll(readOnlyHttpHeaders);
} assertThat(addAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys);
}
@Test @Test // gh-25034
void getValuesAsList() { void equalsUnwrapsHttpHeaders() {
HttpHeaders headers = new HttpHeaders(); HttpHeaders headers1 = new HttpHeaders();
headers.add("Foo", "Bar"); HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1));
headers.add("Foo", "Baz, Qux");
headers.add("Quux", "\t\"Corge\", \"Grault\""); assertThat(headers1).isEqualTo(headers2);
headers.add("Garply", " Waldo \"Fred\\!\", \"\tPlugh, Xyzzy! \""); assertThat(headers2).isEqualTo(headers1);
headers.add("Example-Dates", "\"Sat, 04 May 1996\", \"Wed, 14 Sep 2005\""); }
@Test
void getValuesAsList() {
HttpHeaders headers = new HttpHeaders();
headers.add("Foo", "Bar");
headers.add("Foo", "Baz, Qux");
headers.add("Quux", "\t\"Corge\", \"Grault\"");
headers.add("Garply", " Waldo \"Fred\\!\", \"\tPlugh, Xyzzy! \"");
headers.add("Example-Dates", "\"Sat, 04 May 1996\", \"Wed, 14 Sep 2005\"");
assertThat(headers.getValuesAsList("Foo")).containsExactly("Bar", "Baz", "Qux");
assertThat(headers.getValuesAsList("Quux")).containsExactly("Corge", "Grault");
assertThat(headers.getValuesAsList("Garply")).containsExactly("Waldo \"Fred\\!\"", "\tPlugh, Xyzzy! ");
assertThat(headers.getValuesAsList("Example-Dates")).containsExactly("Sat, 04 May 1996", "Wed, 14 Sep 2005");
}
assertThat(headers.getValuesAsList("Foo")).containsExactly("Bar", "Baz", "Qux");
assertThat(headers.getValuesAsList("Quux")).containsExactly("Corge", "Grault");
assertThat(headers.getValuesAsList("Garply")).containsExactly("Waldo \"Fred\\!\"", "\tPlugh, Xyzzy! ");
assertThat(headers.getValuesAsList("Example-Dates")).containsExactly("Sat, 04 May 1996", "Wed, 14 Sep 2005");
} }
} }

2
spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java
View File

@ -140,7 +140,7 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder {
@SuppressWarnings("ConstantConditions") @SuppressWarnings("ConstantConditions")
private HttpHeaders getHeaders() { private HttpHeaders getHeaders() {
if (this.headers == null) { if (this.headers == null) {
this.headers = HttpHeaders.writableHttpHeaders(this.originalResponse.headers().asHttpHeaders()); this.headers = new HttpHeaders(this.originalResponse.headers().asHttpHeaders());
} }
return this.headers; return this.headers;
} }