diff --git a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java index 80a2d84b5e6..e39b8845813 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java @@ -441,7 +441,15 @@ public class HttpHeaders implements MultiValueMap, Serializable */ public HttpHeaders(MultiValueMap 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; + } } @@ -1869,7 +1877,7 @@ public class HttpHeaders implements MultiValueMap, Serializable * Apply a read-only {@code HttpHeaders} wrapper around the given headers, if necessary. *

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"); @@ -1879,16 +1887,16 @@ public class HttpHeaders implements MultiValueMap, Serializable /** * Remove any read-only wrapper that may have been previously applied around * the given headers via {@link #readOnlyHttpHeaders(HttpHeaders)}. + *

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); } /** diff --git a/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java b/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java index 7c16b19d679..87eac6a9ae7 100644 --- a/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/ReadOnlyHttpHeaders.java @@ -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. @@ -31,6 +31,8 @@ import org.springframework.util.MultiValueMap; /** * {@code HttpHeaders} object that can only be read, not written to. + *

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 diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java b/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java index 54e2b589264..a94e378e3c3 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/DefaultServerHttpRequestBuilder.java @@ -70,7 +70,7 @@ class DefaultServerHttpRequestBuilder implements ServerHttpRequest.Builder { 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(); diff --git a/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java b/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java index 059e3a98162..de8ab6ac0cb 100644 --- a/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java +++ b/spring-web/src/test/java/org/springframework/http/HttpHeadersTests.java @@ -35,6 +35,7 @@ import java.util.Map.Entry; import java.util.Set; import java.util.TimeZone; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import static java.util.stream.Collectors.toList; @@ -54,8 +55,19 @@ import static org.assertj.core.api.Assertions.entry; */ 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 void getOrEmpty() { @@ -578,182 +590,188 @@ class HttpHeadersTests { assertThat(authorization).isEqualTo("Bearer foo"); } - @Test - void keySetOperations() { - headers.add("Alpha", "apple"); - headers.add("Bravo", "banana"); - Set keySet = headers.keySet(); - // Please DO NOT simplify the following with AssertJ's fluent API. - // - // We explicitly invoke methods directly on HttpHeaders#keySet() - // here to check the behavior of the entire contract. + @Nested + class MapEntriesTests { - // isEmpty() and size() - assertThat(keySet).isNotEmpty(); - assertThat(keySet).hasSize(2); + @Test + void keySetOperations() { + headers.add("Alpha", "apple"); + headers.add("Bravo", "banana"); + Set keySet = headers.keySet(); - // contains() - 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(); + // Please DO NOT simplify the following with AssertJ's fluent API. + // + // We explicitly invoke methods directly on HttpHeaders#keySet() + // here to check the behavior of the entire contract. - // toArray() - assertThat(keySet.toArray()).isEqualTo(new String[] {"Alpha", "Bravo"}); + // isEmpty() and size() + assertThat(keySet).isNotEmpty(); + assertThat(keySet).hasSize(2); - // spliterator() via stream() - assertThat(keySet.stream().collect(toList())).isEqualTo(Arrays.asList("Alpha", "Bravo")); + // contains() + 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() - List results = new ArrayList<>(); - keySet.iterator().forEachRemaining(results::add); - assertThat(results).isEqualTo(Arrays.asList("Alpha", "Bravo")); + // toArray() + assertThat(keySet.toArray()).isEqualTo(new String[] {"Alpha", "Bravo"}); - // remove() - assertThat(keySet.remove("Alpha")).isTrue(); - assertThat(keySet).hasSize(1); - assertThat(headers).hasSize(1); - assertThat(keySet.remove("Alpha")).isFalse(); - assertThat(keySet).hasSize(1); - assertThat(headers).hasSize(1); + // spliterator() via stream() + assertThat(keySet.stream().collect(toList())).isEqualTo(Arrays.asList("Alpha", "Bravo")); - // clear() - keySet.clear(); - assertThat(keySet).isEmpty(); - assertThat(keySet).isEmpty(); - assertThat(headers).isEmpty(); - assertThat(headers).isEmpty(); + // iterator() + List results = new ArrayList<>(); + keySet.iterator().forEachRemaining(results::add); + assertThat(results).isEqualTo(Arrays.asList("Alpha", "Bravo")); - // Unsupported operations - assertThatExceptionOfType(UnsupportedOperationException.class) - .isThrownBy(() -> keySet.add("x")); - assertThatExceptionOfType(UnsupportedOperationException.class) - .isThrownBy(() -> keySet.addAll(Collections.singleton("enigma"))); - } + // remove() + assertThat(keySet.remove("Alpha")).isTrue(); + assertThat(keySet).hasSize(1); + assertThat(headers).hasSize(1); + assertThat(keySet.remove("Alpha")).isFalse(); + assertThat(keySet).hasSize(1); + assertThat(headers).hasSize(1); - /** - * 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"); + // clear() + keySet.clear(); + assertThat(keySet).isEmpty(); + assertThat(keySet).isEmpty(); + assertThat(headers).isEmpty(); + assertThat(headers).isEmpty(); - // --- When --- - boolean removed = headers.keySet().remove("Alpha"); + // Unsupported operations + 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. - // - // We explicitly invoke methods directly on HttpHeaders here to check - // the behavior of the entire contract. + // --- When --- + boolean removed = headers.keySet().remove("Alpha"); - assertThat(removed).isTrue(); - 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"))); - } + // --- Then --- - @Test - void removalFromKeySetRemovesEntryFromUnderlyingMap() { - String headerName = "MyHeader"; - String headerValue = "value"; + // Please DO NOT simplify the following with AssertJ's fluent API. + // + // We explicitly invoke methods directly on HttpHeaders here to check + // the behavior of the entire contract. - assertThat(headers).isEmpty(); - headers.add(headerName, headerValue); - assertThat(headers.containsKey(headerName)).isTrue(); - headers.keySet().removeIf(key -> key.equals(headerName)); - assertThat(headers).isEmpty(); - headers.add(headerName, headerValue); - assertThat(headers.get(headerName)).containsExactly(headerValue); - } + assertThat(removed).isTrue(); + 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 - void removalFromEntrySetRemovesEntryFromUnderlyingMap() { - String headerName = "MyHeader"; - String headerValue = "value"; + @Test + void removalFromKeySetRemovesEntryFromUnderlyingMap() { + String headerName = "MyHeader"; + String headerValue = "value"; - 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).isEmpty(); + headers.add(headerName, headerValue); + assertThat(headers.containsKey(headerName)).isTrue(); + headers.keySet().removeIf(key -> key.equals(headerName)); + assertThat(headers).isEmpty(); + headers.add(headerName, headerValue); + assertThat(headers.get(headerName)).containsExactly(headerValue); + } - @Test - void readOnlyHttpHeadersRetainEntrySetOrder() { - headers.add("aardvark", "enigma"); - headers.add("beaver", "enigma"); - headers.add("cat", "enigma"); - headers.add("dog", "enigma"); - headers.add("elephant", "enigma"); + @Test + void removalFromEntrySetRemovesEntryFromUnderlyingMap() { + String headerName = "MyHeader"; + String headerValue = "value"; - 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); - assertThat(readOnlyHttpHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - } + String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; - @Test - void readOnlyHttpHeadersCopyOrderTest() { - headers.add("aardvark", "enigma"); - headers.add("beaver", "enigma"); - headers.add("cat", "enigma"); - headers.add("dog", "enigma"); - headers.add("elephant", "enigma"); + assertThat(headers.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - 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(); - readOnlyHttpHeaders.forEach(forEachHeaders::putIfAbsent); - assertThat(forEachHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + String[] expectedKeys = new String[] { "aardvark", "beaver", "cat", "dog", "elephant" }; - HttpHeaders putAllHeaders = new HttpHeaders(); - putAllHeaders.putAll(readOnlyHttpHeaders); - assertThat(putAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + HttpHeaders readOnlyHttpHeaders = HttpHeaders.readOnlyHttpHeaders(headers); - HttpHeaders addAllHeaders = new HttpHeaders(); - addAllHeaders.addAll(readOnlyHttpHeaders); - assertThat(addAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - } + HttpHeaders forEachHeaders = new HttpHeaders(); + readOnlyHttpHeaders.forEach(forEachHeaders::putIfAbsent); + assertThat(forEachHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - @Test // gh-25034 - void equalsUnwrapsHttpHeaders() { - HttpHeaders headers1 = new HttpHeaders(); - HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1)); + HttpHeaders putAllHeaders = new HttpHeaders(); + putAllHeaders.putAll(readOnlyHttpHeaders); + assertThat(putAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); - assertThat(headers1).isEqualTo(headers2); - assertThat(headers2).isEqualTo(headers1); - } + HttpHeaders addAllHeaders = new HttpHeaders(); + addAllHeaders.addAll(readOnlyHttpHeaders); + assertThat(addAllHeaders.entrySet()).extracting(Entry::getKey).containsExactly(expectedKeys); + } - @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\""); + @Test // gh-25034 + void equalsUnwrapsHttpHeaders() { + HttpHeaders headers1 = new HttpHeaders(); + HttpHeaders headers2 = new HttpHeaders(new HttpHeaders(headers1)); + + assertThat(headers1).isEqualTo(headers2); + assertThat(headers2).isEqualTo(headers1); + } + + @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"); } } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java index e089b1f3ccb..29845f26f1c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponseBuilder.java @@ -140,7 +140,7 @@ final class DefaultClientResponseBuilder implements ClientResponse.Builder { @SuppressWarnings("ConstantConditions") private HttpHeaders getHeaders() { if (this.headers == null) { - this.headers = HttpHeaders.writableHttpHeaders(this.originalResponse.headers().asHttpHeaders()); + this.headers = new HttpHeaders(this.originalResponse.headers().asHttpHeaders()); } return this.headers; }